feat(otel): use dedicated monitoring identity for OTel Collector sidecar

[udsmicrosoft]

Summary

Addresses part of #344.

The OTel Collector sidecar no longer uses the CNPG application user's credentials. It now connects as a dedicated otel_monitor PostgreSQL identity.

PostgreSQL host authentication currently uses trust, so this PR intentionally does not generate or inject a password: it would not be checked and would require unnecessary cluster-wide Secret write permissions. This change improves query attribution and prepares the monitoring path for future authentication hardening, but it is not an isolation boundary for a compromised sidecar until trust is replaced.

What changed

1. Dedicated identity — while monitoring is enabled, CNPG manages otel_monitor with LOGIN, DisablePassword, and no inherited monitoring roles. The current SQL receiver query is only SELECT 1, so broader pg_monitor access is not required.
2. No monitoring Secret — the sidecar sets PGUSER=otel_monitor directly, does not set PGPASSWORD, and the SQL datasource contains no password parameter.
3. Correct disable lifecycle — while monitoring is disabled, the desired CNPG role remains declared with ensure: absent, allowing CNPG to drop an existing or previously orphaned role. Reconciliation preserves unrelated managed roles and services.
4. Read-only Secret RBAC — the Helm and generated ClusterRoles retain only get/list/watch on Secrets.
5. Plugin validation — all OTel parameters now require the collector image and ConfigMap together, and mutation round-trips the image, ConfigMap, config hash, resource settings, and Prometheus port.

Security scope

• The dedicated identity avoids normal monitoring queries running as the application user.
• trust authentication still allows any process that can reach PostgreSQL to claim another database role. Replacing it with SCRAM or certificate authentication remains necessary before claiming compromised-sidecar blast-radius isolation.
• Statistics privileges should be granted only when concrete collector queries require them.

Testing

• go test ./internal/otel ./internal/cnpg ./internal/controller
• go vet ./internal/otel ./internal/cnpg ./internal/controller
• go test ./internal/config ./internal/lifecycle ./internal/operator
• go vet ./internal/config ./internal/lifecycle ./internal/operator
• helm unittest -f 'tests/05_clusterrole_test.yaml' .
• make manifests

[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.85 (mixed)

Reasoning

component from path globs (controllers, test, manifests); effort from diff stats (568+33 LOC, 17 files); LLM: Multi-file, cross-component security improvement introducing a dedicated least-privilege monitoring role, new secret management, RBAC updates, and sidecar wiring — touches controllers, manifests, and the kubectl-plugin sidecar-injector.

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]

Pull request overview

This PR updates the DocumentDB operator and CNPG sidecar-injector plugin to use a dedicated least-privilege PostgreSQL role (otel_monitor) and a dedicated per-cluster Secret (<cluster>-otel-monitor) for OTel Collector sidecar credentials, instead of reusing the broad CNPG application user credentials.

Changes:

• Introduces otel_monitor role + <cluster>-otel-monitor basic-auth Secret generation/cleanup in the operator and wires CNPG managed roles + plugin params to use it.
• Extends CNPG cluster sync to reconcile managed roles independently (so monitoring can be toggled on/off for existing clusters).
• Updates the sidecar-injector plugin to require and consume the new otelMonitorSecret parameter for PGUSER/PGPASSWORD.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
operator/src/internal/otel/config.go	Adds constants/helpers for monitoring role name and Secret naming.
operator/src/internal/otel/config_test.go	Adds unit tests for the new role/secret helpers.
operator/src/internal/controller/physical_replication.go	Refactors Azure Fleet managed-services setup to preserve existing spec.managed (e.g., roles).
operator/src/internal/controller/physical_replication_test.go	Adds tests for Azure Fleet managed-services behavior and role preservation.
operator/src/internal/controller/documentdb_controller.go	Reconciles the new monitoring Secret and cleans it up when monitoring is disabled; adds Secrets RBAC annotation.
operator/src/internal/controller/documentdb_controller_test.go	Adds tests for monitoring Secret create/stability and delete behavior.
operator/src/internal/cnpg/cnpg_sync.go	Syncs otelMonitorSecret plugin param and adds managed-roles reconciliation logic.
operator/src/internal/cnpg/cnpg_sync_test.go	Adds tests for managed-role add/remove/preserve/no-op sync scenarios.
operator/src/internal/cnpg/cnpg_patch.go	Adds JSON patch paths for /spec/managed and /spec/managed/roles.
operator/src/internal/cnpg/cnpg_cluster.go	Declares the managed otel_monitor role and passes otelMonitorSecret plugin param when monitoring is enabled.
operator/src/internal/cnpg/cnpg_cluster_test.go	Adds tests validating managed role + plugin param when monitoring enabled/disabled.
operator/src/config/rbac/role.yaml	Grants Secrets write verbs; adjusts ClusterRole rules (kustomize path).
operator/documentdb-helm-chart/templates/05_clusterrole.yaml	Grants Secrets write verbs and documents the new per-cluster monitoring Secret.
operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go	Requires otelMonitorSecret for sidecar injection and sources PG creds from it.
operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go	Adds tests asserting the sidecar uses the monitoring Secret for PG creds.
operator/cnpg-plugins/sidecar-injector/internal/config/config.go	Adds otelMonitorSecret parameter parsing into typed configuration.
operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go	Adds unit test coverage for parsing the new OTel parameters.

[xgerman]

Summary

[Required] No blocking issues found in this PR.

I validated the key behavior changes locally (including kind):

• dedicated <cluster>-otel-monitor secret lifecycle (create/preserve/delete)
• spec.managed.roles reconciliation for otel_monitor
• sidecar credential wiring (PGUSER/PGPASSWORD) to otelMonitorSecret
• monitoring disable/enable cleanup and re-create path

Targeted tests run clean:

• go test ./... (sidecar-injector module)
• go test ./internal/otel ./internal/cnpg ./internal/controller (operator module)

I left one non-blocking inline suggestion about documenting RBAC upgrade ordering, based on local rollout behavior with stale ClusterRole.

[xgerman]

Summary

Reviewed against the actual CNPG managed-role/secret semantics and the collector's queries (both modules build, vet, and test green in an isolated worktree). This is a clean, security-positive change and the reasoning in the code comments is excellent. Verified the parts most likely to be subtly wrong:

Verified correct:

• Password generation — crypto/rand, 24 bytes (192-bit) via base64.RawURLEncoding → URL-safe, no chars that could break a PG connection string; generated once and preserved across reconciles (test: preserves the existing password). Secret carries an owner reference so it's GC'd with the CR. ✅
• No perpetual re-patch — managedRolesPatch uses reflect.DeepEqual and applyOtelMonitorRole sets the CNPG/CRD-defaulted fields (ConnectionLimit=-1, Inherit=true, Ensure=present) explicitly so the desired role matches the API-server-defaulted live form (test: does not patch when managed roles are unchanged). ✅
• No managed-block clobber — roles patch targets /spec/managed/roles, the replication flow owns /spec/managed/services/additional; disjoint paths, and addAzureFleetManagedServices now preserves the existing managed block instead of overwriting it (test: preserves managed.services when patching only roles). nil-vs-empty roles handled. ✅
• Disable ordering is safe — on disable, applyOtelMonitorRole is a no-op so the sync removes the role reference before deleteOtelMonitorSecret runs, so CNPG never dangles a missing passwordSecret; and the delete-after-sync guard retains OTel resources if sync fails (test: retains OTel resources when cluster sync fails). ✅
• Sidecar gating — injection now requires all three OTel params, and credentials are sourced from <cluster>-otel-monitor instead of <cluster>-app. ✅
• pg_monitor is sufficient — the only static query is SELECT 1 against dbname=postgres (LOGIN + PUBLIC CONNECT), and the dynamic pg_stat_* metrics are exactly what pg_monitor grants — no functional regression. ✅

Nice catch: the validation.go change fixes a real latent bug — ValidateChanges previously overwrote the FromParameters errors (so the new required-OTel-param validation would have been silently dropped); appending is correct.

Approving. Two non-blocking observations inline.

[xgerman]

🟢 Observation (non-blocking) — the Postgres role outlives the secret on disable. With Ensure: present (and no absent on teardown), removing the role from spec.managed.roles makes CNPG stop managing otel_monitor but does not DROP it. So after monitoring is disabled the <cluster>-otel-monitor secret is deleted while the DB role lingers. It's benign (pg_monitor-only, no application access) and re-enabling monitoring cleanly re-manages it with a fresh password — but worth a one-line comment here, or Ensure: absent on the removal path if a clean teardown is desired. Given pg_hba is currently trust, the lingering role can still connect without a password, which is another reason to note it (scope is still just monitoring views).

[xgerman]

🟢 Nitpick (non-blocking) — broad secrets grant. This widens the operator ClusterRole from read-only to create/update/patch/delete on all Secrets cluster-wide. It's needed (the operator already had cluster-wide get/list/watch, and it now owns <cluster>-otel-monitor), and a cluster-scoped operator can't easily namespace this — so acceptable. Just calling it out since a compromised operator can now write/delete any secret in the cluster; worth keeping in mind if the operator ever moves toward a namespaced-Role model. Kept correctly in sync with the generated config/rbac/role.yaml.

[xgerman]

Security review of the least-privilege monitoring role. The direction (dedicated pg_monitor-only role instead of the app user) is the right call and the role itself is not a Postgres privilege-escalation vector — otel_monitor has no SUPERUSER/CREATEDB/CREATEROLE (tests confirm), and pg_monitor is read-only observability. But three things blunt the security value and one increases the operator's blast radius. Details inline; summary:

1. 🔴 Cluster-wide secrets write — the operator already reads every secret cluster-wide; adding create/update/patch/delete cluster-wide is a strong k8s escalation primitive (forge SA tokens / overwrite secrets privileged workloads consume). Prefer a namespaced Role.
2. 🟠 Lingering role on disable — removing the entry from spec.managed.roles makes CNPG stop managing the role, not drop it. The otel_monitor LOGIN role persists with its last password while its secret is deleted → orphaned, un-rotatable credential. Use ensure: absent for one reconcile before removing.
3. 🟠 pg_hba is trust — the generated password is unused today; anyone who can reach 5432 connects as any role incl. superuser. So the whole secret + RBAC-widening buys nothing until auth is tightened. Consider skipping the secret entirely for now.
4. 🟡 pg_monitor is broader than needed — leaks query text (pg_stat_activity/pg_stat_statements) and config. Consider granting only the specific views the collector queries.

Not blocking the concept — flagging (2) and (3) as the items that determine whether this actually delivers least-privilege.