refactor(operator): route render inputs through ClusterIntent seam

[WentingWu666666]

Summary

Routes the CNPG cluster render path through the product-neutral ClusterIntent seam introduced in #446. GetCnpgClusterSpecFromIntent(intent, storageClass, isPrimaryRegion, log) now renders the CNPG Cluster from a resolved, product-neutral model instead of reading *DocumentDB directly. No runtime behavior change — the rendered CNPG Cluster is byte-identical, guarded by a drift test.

What changed

• internal/product — ClusterIntent is now a self-contained, resolved desired-state model. Groups mirror the DocumentDB CRD where the mapping is a direct passthrough:
  • Images, Topology{NodeCount, InstancesPerNode, Affinity}, Storage{PvcSize}, Identity, Postgres, Resource, TLS{GatewaySecretName, PostgresCertificates}, Monitoring (otel.MonitoringConfig), LogLevel, Timeouts{StopDelay}, FeatureGates{IOUring}, Bootstrap, CredentialSecret, Plugins{SidecarInjectorName, WalReplicaName}, Product.
  • DocumentDBAdapter.ToClusterIntent resolves images, credential secret, and plugin names (honoring the spec, else profile defaults), and contributes DocumentDB change-streams as a Postgres.Parameters entry (wal_level=logical) rather than leaking a product-specific feature gate into the neutral model.
• internal/cnpg — the renderer reads every input from the intent. Neutralized MergeParameters / ComputeResourceSplit via MergeParametersResolved / ComputeResourceSplitFromResource to avoid a product↔cnpg import cycle (the *DocumentDB functions remain thin wrappers). Params, resource split, log level, stop delay, Postgres certs, gateway TLS secret, and OTel/monitoring config all flow through the intent.
• Reconcile-time inputs (storageClass, isPrimaryRegion) are passed as render params, not carried on the intent, keeping the intent a pure function of the CR.

Not in scope (owned by other reconcile paths)

ClusterReplication, ExposeViaService, Backup, and SchemaVersion are not render inputs and remain owned by their existing controllers. NodeCount is carried on the intent for completeness but is not yet rendered (no active reader today).

Testing

• cnpg_intent_drift_test.go: byte-identical guard asserting the intent render path matches the retained *DocumentDB computations across a spec matrix (TLS spec+status, user params, resource envelope/overrides, IOUring, change-streams, custom logLevel/stopDelay, process identity, monitoring).
• make test (envtest) green across all packages; go build / go vet / gofmt clean. No CRD/API/Helm changes; no generated-file drift.

[documentdb-triage-tool]

🤖 Auto-triaged by documentdb-triage-tool.

Applied: go, test, enhancement
Project fields suggested: Component controllers · Priority P2 · Effort L · Status In Progress
Confidence: 0.82 (mixed)

Reasoning

component from path globs (controllers, test); effort from diff stats (364+63 LOC, 8 files); LLM: Multi-file internal refactor routing CNPG cluster rendering through a product-neutral ClusterIntent seam, touching multiple packages (internal/product, internal/cnpg) with new tests and no API/schema changes, as groundwork for a future shared operator.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

[Copilot]

🟡 Changes recommended

The compatibility wrapper can produce incorrect object identity, and the advertised ChangeStreams intent field is missing; scope and drift-test alignment also remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR routes additional CNPG rendering inputs through ClusterIntent while adding neutral resource, PostgreSQL, and monitoring seams.

Changes:

• Expands intent and adapter mappings.
• Refactors CNPG and OTel helpers around neutral inputs.
• Updates rendering and adds related test coverage.

File summaries

File	Reviewed changes
operator/src/internal/product/intent.go	Extends ClusterIntent models.
operator/src/internal/product/intent_test.go	Tests intent mappings.
operator/src/internal/product/documentdb.go	Maps DocumentDB configuration into intent.
operator/src/internal/otel/config.go	Adds neutral monitoring configuration.
operator/src/internal/otel/config_test.go	Tests monitoring configuration.
operator/src/internal/controller/documentdb_controller.go	Routes rendering and configuration through intent.
operator/src/internal/cnpg/resource_split.go	Adds neutral resource splitting.
operator/src/internal/cnpg/pg_defaults.go	Adds neutral parameter merging.
operator/src/internal/cnpg/pg_defaults_test.go	Tests parameter merging.
operator/src/internal/cnpg/cnpg_intent_test.go	Tests intent-based rendering.
operator/src/internal/cnpg/cnpg_intent_drift_test.go	Adds render drift coverage.
operator/src/internal/cnpg/cnpg_cluster.go	Routes cluster rendering through intent.
operator/src/internal/cnpg/cnpg_cluster_test.go	Updates cluster renderer tests.

Review details

Suppressed comments (3)

operator/src/internal/cnpg/cnpg_intent_drift_test.go:146

• This drift test does not compare the intent path with an independent pre-refactor path: GetCnpgClusterSpec already delegates to GetCnpgClusterSpecFromIntent, and the expected parameter/resource helpers also delegate through the new adapter/neutral implementations. It can therefore pass even if the intent mapping or renderer changes, and it does not establish byte-identical full Cluster output. Keep an independent baseline or compare the complete rendered object against a legacy/fixture result.

			spec := GetCnpgClusterSpec(req, db, "", "test-sa", "", true, log).Spec

operator/src/internal/controller/documentdb_controller.go:1035

• The PR description says Phase 0b intentionally leaves Monitoring/OTel inputs reading *DocumentDB, but this change maps the monitoring spec to a neutral type and routes ConfigMap generation through it here. Please either keep this out of the Phase 0a implementation or update the description and acceptance/testing scope so the phase boundary is explicit.

		configData, err := otelcfg.GenerateConfigMapData(documentdb.Name, namespace, product.MonitoringConfigFromSpec(documentdb.Spec.Monitoring))

operator/src/internal/product/intent.go:79

• The PR description says ClusterIntent.FeatureGates is extended with ChangeStreams, but this definition omits that field and instead folds the gate into Postgres.Parameters["wal_level"]. That may be the intended product-neutral design, but it contradicts the advertised seam and prevents intent consumers from inspecting the gate; please align the implementation and description before merging.

// FeatureGates carries the resolved, product-neutral feature-gate flags the
// builder acts on. Only genuinely cross-product (infrastructure) gates belong
// here; product-specific gates are expressed through their concrete effect (for
// example DocumentDB change streams contributing Postgres.Parameters
// wal_level=logical) instead of leaking into this struct.
type FeatureGates struct {
	// IOUring relaxes the postgres seccomp profile and enables io_method=io_uring.
	// It is an infrastructure concern shared across products.
	IOUring bool

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[WentingWu666666]

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.

[WentingWu666666]

Resolved by aligning the PR description with the implementation. The "Phase 0b deferred" wording was stale from an earlier revision; the description has been updated to state that monitoring/OTel inputs are routed through the intent (Monitoring is a resolved otel.MonitoringConfig, and the renderer consumes config-map name/hash + Prometheus port from it). Mapping the full exporter config into ClusterIntent here is intentional and remains byte-identical — TestRenderIntentSeamNoDrift cross-checks the OTel config hash against a direct otelcfg.GenerateConfigMapData(db.Name, db.Namespace, mon) computation.

[WentingWu666666]

Re: Copilot review — addressing the suppressed comments

Thanks for the review. Summarizing how the three suppressed comments are addressed, since they don't have inline threads:

1. FeatureGates.ChangeStreams "missing" (intent.go:79). This is intentional, and the description has been corrected. Change-streams is a DocumentDB-specific gate whose only cluster effect is a PostgreSQL GUC, so the adapter contributes it as a resolved Postgres.Parameters["wal_level"]=logical rather than leaking a product concept into the product-neutral FeatureGates. Only genuinely cross-product (infrastructure) gates — currently IOUring — live on FeatureGates. The earlier description advertising a ChangeStreams field was stale and has been updated.

2. Monitoring routed through the intent (controller.go). Same root cause — the "Phase 0b deferred" note was stale. Monitoring/OTel now flows through the intent by design; the description reflects this.

3. Drift test independence (cnpg_intent_drift_test.go). Fair point that the parameters and resource-split comparisons delegate through the same neutral helpers on both sides. However, byte-identical-ness is established by the suite as a whole, not the drift test alone:

• The neutral helpers have independent literal-value tests: resource_split_test.go (e.g. want 3Gi, want 13184Mi, exact byte counts) and pg_defaults_test.go (e.g. shared_buffers == "512MB", exact param maps).
• cnpg_cluster_test.go asserts the full rendered Cluster against literals (name/namespace, certs, storage class, images, bootstrap).
• The drift test's remaining checks — OTel config hash (via a direct otelcfg call), logLevel, stopDelay, Postgres certs, gateway-TLS secret — are independent of the intent path.

So the drift test is a supplementary consistency guard on top of a literal-baseline suite. The only fully-independent alternative for the circular parts would be golden fixtures captured from main, which would be redundant given the existing literal tests.

[guanzhousongmicrosoft]

Three things before merge, one of which I'd call blocking.

[guanzhousongmicrosoft]

This guard can't detect drift introduced by this PR — both sides run the same code.

MergeParameters (pg_defaults.go:115-118) and ComputeResourceSplit (resource_split.go:106-109) were rewritten in this PR to delegate through ToClusterIntent, which is exactly what the renderer calls. So wantParams and the rendered parameters are the same expression.

I verified it: commenting out Parameters: db.Spec.Postgres.Parameters in ToClusterIntent — silently dropping every user-supplied GUC from the rendered cluster — leaves TestRenderIntentSeamNoDrift green. The existing pg_defaults_test.go catches that regression; this test does not. Same structure for the resource-split assertions at L150/L191.

The logLevel / maxStopDelay / certificates / gatewayTLSSecret assertions are independent and worth keeping. For the parameter and split cases, either pin concrete expected values (e.g. assert wal_level=logical literally in the change-streams case), or drop the *DocumentDB wrappers so the test is forced to state its expectations directly.

This is the PR's central correctness claim, so I'd want it resolved before merge. It also matches Copilot's suppressed comment on this line.

[guanzhousongmicrosoft]

This mapping has no test anywhere.

The otel/config_test.go cases that used to cover CRD → collector config were converted to build MonitoringConfig{...} literals, so the downstream half kept its coverage and the CRD half lost all of it. Now untested: spec == nil, Exporter == nil, Exporter.Prometheus != nil → Prometheus = true (the pointer-presence → bool conversion, the easiest line here to get wrong), OTLP.Endpoint == "", and the Enabled passthrough.

The drift test uses this same function as its own oracle, so it won't catch a mismapping either. A small table test in internal/product covering those five cases would close it.

[guanzhousongmicrosoft]

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).