Skip to content

refactor(operator): route render inputs through ClusterIntent seam #461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
WentingWu666666 wants to merge 9 commits into
base: main
Choose a base branch
from
Open

refactor(operator): route render inputs through ClusterIntent seam #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e52214c
refactor(operator): route render inputs through ClusterIntent seam (P…
wentingwu000 Sep 15, 2026
a777f59
refactor(operator): route OTel/monitoring through ClusterIntent (Phas…
wentingwu000 Sep 17, 2026
6e4cd82
refactor: fold render context onto ClusterIntent seam
wentingwu000 Sep 17, 2026
d8c91a9
refactor: group ClusterIntent fields to mirror CRD; add RenderContext
wentingwu000 Sep 17, 2026
d61c0ca
refactor: pass storageClass/isPrimaryRegion as params instead of Rend…
wentingwu000 Sep 17, 2026
e5c91c3
refactor: tidy ClusterIntent seam comments and dead alias
wentingwu000 Sep 17, 2026
88f76aa
refactor: drop unnecessary blank-assignments for unused params
wentingwu000 Sep 17, 2026
b30f246
refactor: add NodeCount and rename Topology.Instances to InstancesPer…
wentingwu000 Sep 18, 2026
1802f79
refactor: conciseness pass on ClusterIntent render seam
wentingwu000 Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions operator/src/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ require (
github.com/go-task/slim-sprig/v3 v3.0.0 // indirect
github.com/google/cel-go v0.26.0 // indirect
github.com/google/gnostic-models v0.7.1 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/google/go-cmp v0.7.0
github.com/google/pprof v0.0.0-20260402051712-545e8a4df936 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 // indirect
Expand Down Expand Up @@ -126,5 +126,5 @@ require (
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0 // indirect
sigs.k8s.io/gateway-api v1.4.0 // indirect
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect
sigs.k8s.io/yaml v1.6.0 // indirect
sigs.k8s.io/yaml v1.6.0
)
92 changes: 34 additions & 58 deletions operator/src/internal/cnpg/cnpg_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,30 @@ import (

// GetCnpgClusterSpec renders the Cluster for a DocumentDB instance. The
// documentdbImage argument overrides the resolved extension image; pass "" to
// use the image resolved from the instance. Retained for callers that supply an
// explicit extension image.
// use the image resolved from the instance. req and serviceAccountName are
// accepted for call-site compatibility but no longer consumed: object
// coordinates now come from the intent's Identity, and the service account is
// not read by the renderer.
func GetCnpgClusterSpec(req ctrl.Request, documentdb *dbpreview.DocumentDB, documentdbImage, serviceAccountName, storageClass string, isPrimaryRegion bool, log logr.Logger) *cnpgv1.Cluster {

@guanzhousongmicrosoft guanzhousongmicrosoft Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCnpgClusterSpec, MergeParameters, ComputeResourceSplit and ProtectedParameters all have zero non-test callers at head — the controller now calls GetCnpgClusterSpecFromIntent directly. The doc comment says "retained for call-site compatibility," but there are no production call sites left, and it concedes two of the six parameters are ignored (serviceAccountName was already dead before this PR).

Worth deleting here rather than deferring: it also resolves the drift-test issue above, since the tests would then have to state their expectations directly instead of calling back through the new path.

Same vein, both unread at head: Topology.NodeCount (no reader anywhere, as the commit message notes — it's also what motivated the Instances → InstancesPerNode rename), and the new Plugins.WalReplicaName override in product/documentdb.go:94-97 (the only other resolution site, physical_replication.go:90-102, is inside a commented-out TODO re-enable block).

intent := product.DocumentDBAdapter{}.ToClusterIntent(documentdb)
if documentdbImage != "" {
intent.Images.PostgresExtension = documentdbImage
}
return GetCnpgClusterSpecFromIntent(req, documentdb, intent, serviceAccountName, storageClass, isPrimaryRegion, log)
return GetCnpgClusterSpecFromIntent(intent, storageClass, isPrimaryRegion, log)

@WentingWu666666 WentingWu666666 Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this isn't a production issue, and I'd like to keep the current behavior. Rationale:

  • GetCnpgClusterSpec(req, …) is now a test-only compatibility shim. The reconciler calls GetCnpgClusterSpecFromIntent directly, and it builds identity from the fetched CR (documentdb.ObjectMeta) via intent.Identity. In Reconcile, documentdb is loaded with r.Get(ctx, req.NamespacedName, …), so its coordinates always equal req — production identity (and the OTel namespace hash) is correct.
  • Sourcing identity from the CR itself is deliberate and more correct than req: the CNPG Cluster is owned by that CR, so its name/namespace/owner-refs/labels must track the CR. The pre-refactor wrapper was actually inconsistent here (ObjectMeta from req, but OTel/labels/owner-refs from db.Name); this unifies on the CR.
  • Copying req into Identity would also break TestRenderIntentSeamNoDrift: its cases carry no ObjectMeta, and the expected OTel config hash is computed from db.Namespace, so forcing req.Namespace would diverge.

The only real smell is that req/serviceAccountName are now vestigial on this shim (documented as accepted-for-compatibility). Happy to drop them and migrate the ~40 test call-sites in a follow-up if preferred.

}

// GetCnpgClusterSpecFromIntent renders a CNPG Cluster from a product-neutral
// ClusterIntent. This is the seam the reconciler drives: product-varying values
// (images, credential secret, plugin name) come from the intent rather than from
// product-specific lookups.
func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.DocumentDB, intent product.ClusterIntent, serviceAccountName, storageClass string, isPrimaryRegion bool, log logr.Logger) *cnpgv1.Cluster {
split := ComputeResourceSplit(documentdb, DefaultSplitConfig())
// ClusterIntent plus reconcile-time inputs (storage class, region role) that
// are not part of any product's spec. This is the seam the reconciler drives:
// every render input comes from these arguments rather than from loose
// parameters or product-specific lookups.
func GetCnpgClusterSpecFromIntent(intent product.ClusterIntent, storageClass string, isPrimaryRegion bool, log logr.Logger) *cnpgv1.Cluster {
split := ComputeResourceSplitFromResource(intent.Resource, intent.Monitoring.Enabled, DefaultSplitConfig())

sidecarPluginName := intent.SidecarInjectorPlugin
sidecarPluginName := intent.Plugins.SidecarInjectorName

gatewayImage := intent.Images.Gateway
log.Info("Creating CNPG cluster with gateway image", "gatewayImage", gatewayImage, "documentdbName", documentdb.Name, "specGatewayImage", imageGateway(documentdb))
log.Info("Creating CNPG cluster with gateway image", "gatewayImage", gatewayImage, "documentdbName", intent.Identity.Name)

credentialSecretName := intent.CredentialSecret

Expand All @@ -65,8 +68,8 @@ func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.Docume

return &cnpgv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: req.Name,
Namespace: req.Namespace,
Name: intent.Identity.Name,
Namespace: intent.Identity.Namespace,
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: intent.Identity.APIVersion,
Expand All @@ -80,7 +83,7 @@ func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.Docume
},
Spec: func() cnpgv1.ClusterSpec {
spec := cnpgv1.ClusterSpec{
Instances: intent.Topology.Instances,
Instances: intent.Topology.InstancesPerNode,
ImageName: intent.Images.Postgres,
ImagePullSecrets: toCNPGImagePullSecrets(intent.Images.PullSecrets),
PrimaryUpdateMethod: cnpgv1.PrimaryUpdateMethodSwitchover,
Expand All @@ -102,26 +105,26 @@ func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.Docume
addPluginParamIfSet(params, util.PLUGIN_PARAM_GATEWAY_CPU_REQUEST, split.Gateway.CPURequest)
addPluginParamIfSet(params, util.PLUGIN_PARAM_GATEWAY_CPU_LIMIT, split.Gateway.CPULimit)
// If TLS is ready, surface secret name to plugin so it can mount certs.
if documentdb.Status.TLS != nil && documentdb.Status.TLS.Ready && documentdb.Status.TLS.SecretName != "" {
params["gatewayTLSSecret"] = documentdb.Status.TLS.SecretName
if intent.TLS.GatewaySecretName != "" {
params["gatewayTLSSecret"] = intent.TLS.GatewaySecretName
}
// Pass monitoring parameters to plugin for OTel sidecar injection.
// Sidecar is only injected when monitoring is enabled.
// Config hash triggers operator-initiated rolling restart on config changes.
if split.MonitoringEnabled {
params["otelCollectorImage"] = util.DEFAULT_OTEL_COLLECTOR_IMAGE
params["otelConfigMapName"] = otelcfg.ConfigMapName(documentdb.Name)
params["otelConfigMapName"] = otelcfg.ConfigMapName(intent.Identity.Name)
addPluginParamIfSet(params, util.PLUGIN_PARAM_OTEL_MEMORY_REQUEST, split.OTel.MemoryRequest)
addPluginParamIfSet(params, util.PLUGIN_PARAM_OTEL_MEMORY_LIMIT, split.OTel.MemoryLimit)
addPluginParamIfSet(params, util.PLUGIN_PARAM_OTEL_CPU_REQUEST, split.OTel.CPURequest)
addPluginParamIfSet(params, util.PLUGIN_PARAM_OTEL_CPU_LIMIT, split.OTel.CPULimit)
if promPort := otelcfg.ResolvePrometheusPort(documentdb.Spec.Monitoring); promPort > 0 {
if promPort := otelcfg.ResolvePrometheusPort(intent.Monitoring); promPort > 0 {
params["prometheusPort"] = fmt.Sprintf("%d", promPort)
}
// Compute config hash for change detection. The operator triggers a
// rolling restart (via restart annotation) when plugin parameters
// change, ensuring pods pick up new config.
if configData, err := otelcfg.GenerateConfigMapData(documentdb.Name, req.Namespace, documentdb.Spec.Monitoring); err == nil {
if configData, err := otelcfg.GenerateConfigMapData(intent.Identity.Name, intent.Identity.Namespace, intent.Monitoring); err == nil {
params["otelConfigHash"] = otelcfg.HashConfigMapData(configData)
} else {
log.Error(err, "Failed to generate OTel config hash; config changes may not trigger rolling restart")
Expand All @@ -133,10 +136,10 @@ func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.Docume
Parameters: params,
}}
}(),
PostgresConfiguration: buildPostgresConfiguration(documentdb, extensionImageSource, split.PostgresMemoryBytes),
PostgresConfiguration: buildPostgresConfiguration(MergeParametersResolved(intent.Postgres.Parameters, intent.FeatureGates, split.PostgresMemoryBytes), extensionImageSource),
Bootstrap: bootstrapConfigurationFromIntent(intent, isPrimaryRegion, log),
LogLevel: cmp.Or(documentdb.Spec.LogLevel, "info"),
Certificates: postgresCertificates(documentdb),
LogLevel: cmp.Or(intent.LogLevel, "info"),
Certificates: intent.TLS.PostgresCertificates,
Backup: &cnpgv1.BackupConfiguration{
VolumeSnapshot: &cnpgv1.VolumeSnapshotConfiguration{
SnapshotOwnerReference: "backup", // Set owner reference to 'backup' so that snapshots are deleted when Backup resource is deleted
Expand All @@ -146,10 +149,10 @@ func GetCnpgClusterSpecFromIntent(req ctrl.Request, documentdb *dbpreview.Docume
Affinity: intent.Topology.Affinity,
Resources: buildResourceRequirements(split.Postgres),
}
spec.MaxStopDelay = getMaxStopDelayOrDefault(documentdb)
spec.MaxStopDelay = intent.Timeouts.StopDelay
applyPostgresProcessIdentity(&spec, intent)
applyIOUringSeccomp(&spec, intent)
applyOtelMonitorRole(&spec, documentdb)
applyOtelMonitorRoleFromIntent(&spec, intent.Monitoring.Enabled)

return spec
}(),
Expand Down Expand Up @@ -209,8 +212,8 @@ func bootstrapConfigurationFromIntent(intent product.ClusterIntent, isPrimaryReg
return defaultBootstrapConfigurationFromIntent(intent)
}

// getBootstrapConfiguration adapts a DocumentDB instance onto the intent-based
// bootstrap builder. Retained for direct callers that hold the custom resource.
// getBootstrapConfiguration builds the bootstrap configuration for a DocumentDB
// instance via the intent-based builder.
func getBootstrapConfiguration(documentdb *dbpreview.DocumentDB, isPrimaryRegion bool, log logr.Logger) *cnpgv1.BootstrapConfiguration {
return bootstrapConfigurationFromIntent(product.DocumentDBAdapter{}.ToClusterIntent(documentdb), isPrimaryRegion, log)
}
Expand All @@ -231,20 +234,12 @@ func defaultBootstrapConfigurationFromIntent(intent product.ClusterIntent) *cnpg
}
}

// getDefaultBootstrapConfiguration adapts a DocumentDB instance onto the
// intent-based default bootstrap builder. Retained for direct callers.
// getDefaultBootstrapConfiguration builds the default bootstrap configuration for
// a DocumentDB instance via the intent-based builder.
func getDefaultBootstrapConfiguration(documentdb *dbpreview.DocumentDB) *cnpgv1.BootstrapConfiguration {
return defaultBootstrapConfigurationFromIntent(product.DocumentDBAdapter{}.ToClusterIntent(documentdb))
}

// getMaxStopDelayOrDefault returns StopDelay if set, otherwise util.CNPG_DEFAULT_STOP_DELAY
func getMaxStopDelayOrDefault(documentdb *dbpreview.DocumentDB) int32 {
if documentdb.Spec.Timeouts.StopDelay != 0 {
return documentdb.Spec.Timeouts.StopDelay
}
return util.CNPG_DEFAULT_STOP_DELAY
}

// parseMemoryToBytes converts a Kubernetes quantity string (e.g., "2Gi", "4096Mi")
// to bytes. Returns 0 if the string is empty or "0" (meaning unlimited/unset).
func parseMemoryToBytes(memoryStr string) int64 {
Expand Down Expand Up @@ -311,22 +306,6 @@ func parsePullPolicy(value string) corev1.PullPolicy {
}
}

// imageGateway returns spec.image.gateway or empty string when unset.
// Nil-safe.
func imageGateway(documentdb *dbpreview.DocumentDB) string {
if documentdb == nil || documentdb.Spec.Image == nil {
return ""
}
return documentdb.Spec.Image.Gateway
}

func postgresCertificates(documentdb *dbpreview.DocumentDB) *cnpgv1.CertificatesConfiguration {
if documentdb.Spec.TLS == nil {
return nil
}
return documentdb.Spec.TLS.Postgres
}

// toCNPGImagePullSecrets translates a list of corev1.LocalObjectReference
// (the Kubernetes-native shape used on spec.imagePullSecrets) into the
// CNPG-flavoured cnpgv1.LocalObjectReference shape that
Expand Down Expand Up @@ -387,15 +366,12 @@ func applyIOUringSeccomp(spec *cnpgv1.ClusterSpec, intent product.ClusterIntent)
// PostgreSQL host authentication currently uses trust, so a generated password
// would not be checked. Disable the role password explicitly until authentication
// is tightened rather than creating an unused credential and widening Secret RBAC.
func applyOtelMonitorRole(spec *cnpgv1.ClusterSpec, documentdb *dbpreview.DocumentDB) {
if documentdb == nil {
return
}
func applyOtelMonitorRoleFromIntent(spec *cnpgv1.ClusterSpec, monitoringEnabled bool) {
if spec.Managed == nil {
spec.Managed = &cnpgv1.ManagedConfiguration{}
}
role := absentOtelMonitorRole()
if documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled {
if monitoringEnabled {
// The current health query is SELECT 1, so the role needs LOGIN only
// and is not granted broad monitoring memberships.
role = cnpgv1.RoleConfiguration{
Expand Down Expand Up @@ -429,7 +405,7 @@ func absentOtelMonitorRole() cnpgv1.RoleConfiguration {
// stanza (mounted from spec.image.documentDB as an ImageVolumeSource),
// sets a fixed AdditionalLibraries list, and applies a small set of
// operator-managed GUCs.
func buildPostgresConfiguration(documentdb *dbpreview.DocumentDB, extensionImageSource corev1.ImageVolumeSource, pgMemoryBytes int64) cnpgv1.PostgresConfiguration {
func buildPostgresConfiguration(parameters map[string]string, extensionImageSource corev1.ImageVolumeSource) cnpgv1.PostgresConfiguration {
pgHBA := []string{
"host all all localhost trust",
"hostssl replication streaming_replica all cert",
Expand All @@ -446,7 +422,7 @@ func buildPostgresConfiguration(documentdb *dbpreview.DocumentDB, extensionImage
},
},
AdditionalLibraries: []string{"pg_cron", "pg_documentdb_core", "pg_documentdb"},
Parameters: MergeParameters(documentdb, pgMemoryBytes),
Parameters: parameters,
PgHBA: pgHBA,
}
}
9 changes: 7 additions & 2 deletions operator/src/internal/cnpg/cnpg_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log/zap"

dbpreview "github.com/documentdb/documentdb-operator/api/preview"
"github.com/documentdb/documentdb-operator/internal/product"
util "github.com/documentdb/documentdb-operator/internal/utils"
)

Expand Down Expand Up @@ -263,6 +264,10 @@ var _ = Describe("GetCnpgClusterSpec", func() {
req.Namespace = "default"

documentdb := &dbpreview.DocumentDB{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
Spec: dbpreview.DocumentDBSpec{
InstancesPerNode: 3,
Image: &dbpreview.ImageSpec{
Expand Down Expand Up @@ -1077,7 +1082,7 @@ func TestGetInheritedMetadataLabels(t *testing.T) {
}
}

func TestGetMaxStopDelayOrDefault(t *testing.T) {
func TestMaxStopDelayFromIntent(t *testing.T) {
tests := []struct {
name string
documentdb *dbpreview.DocumentDB
Expand Down Expand Up @@ -1127,7 +1132,7 @@ func TestGetMaxStopDelayOrDefault(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := getMaxStopDelayOrDefault(tt.documentdb)
result := product.DocumentDBAdapter{}.ToClusterIntent(tt.documentdb).Timeouts.StopDelay

if result != tt.expected {
t.Errorf("Expected %d, got %d", tt.expected, result)
Expand Down
13 changes: 9 additions & 4 deletions operator/src/internal/cnpg/cnpg_intent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,13 @@ var _ = Describe("GetCnpgClusterSpecFromIntent", func() {
PostgresExtension: "reg/ext:test",
Gateway: "reg/gw:test",
},
CredentialSecret: "custom-secret",
SidecarInjectorPlugin: "custom-injector.example.io",
CredentialSecret: "custom-secret",
Plugins: product.Plugins{SidecarInjectorName: "custom-injector.example.io"},
}

result := GetCnpgClusterSpecFromIntent(newRequest(), newDocumentDB(), intent, "test-sa", "", true, log)
intent.Identity.Name = "test-cluster"
intent.Identity.Namespace = "default"
result := GetCnpgClusterSpecFromIntent(intent, "", true, log)

Expect(result.Spec.PostgresConfiguration.Extensions[0].ImageVolumeSource.Reference).To(Equal("reg/ext:test"))
Expect(result.Spec.Plugins[0].Name).To(Equal("custom-injector.example.io"))
Expand All @@ -56,7 +58,10 @@ var _ = Describe("GetCnpgClusterSpecFromIntent", func() {
documentdb := newDocumentDB()
intent := product.DocumentDBAdapter{}.ToClusterIntent(documentdb)

fromIntent := GetCnpgClusterSpecFromIntent(newRequest(), documentdb, intent, "test-sa", "", true, log)
intent.Identity.Name = "test-cluster"
intent.Identity.Namespace = "default"

fromIntent := GetCnpgClusterSpecFromIntent(intent, "", true, log)
fromWrapper := GetCnpgClusterSpec(newRequest(), documentdb, "", "test-sa", "", true, log)

Expect(fromIntent.Spec.PostgresConfiguration.Extensions[0].ImageVolumeSource.Reference).
Expand Down
Loading
Loading