From cfb7373a58a63da0da737bbfa520be13fb61c626 Mon Sep 17 00:00:00 2001 From: urismiley Date: Thu, 9 Jul 2026 14:21:26 -0400 Subject: [PATCH 1/6] feat: use dedicated least-privilege otel_monitor user for OTel Collector sidecar Copilot-Session: ba406fac-b035-4384-a495-46e2661fafdb Signed-off-by: urismiley --- .../internal/config/config.go | 3 + .../internal/config/config_test.go | 21 ++++ .../internal/lifecycle/lifecycle.go | 28 +++--- .../internal/lifecycle/lifecycle_test.go | 33 ++++++- .../templates/05_clusterrole.yaml | 5 +- operator/src/config/rbac/role.yaml | 10 ++ operator/src/internal/cnpg/cnpg_cluster.go | 39 +++++++- .../src/internal/cnpg/cnpg_cluster_test.go | 64 +++++++++++++ operator/src/internal/cnpg/cnpg_patch.go | 2 + operator/src/internal/cnpg/cnpg_sync.go | 34 ++++++- operator/src/internal/cnpg/cnpg_sync_test.go | 96 +++++++++++++++++++ .../controller/documentdb_controller.go | 86 +++++++++++++++++ .../controller/documentdb_controller_test.go | 70 ++++++++++++++ .../controller/physical_replication.go | 42 ++++---- .../controller/physical_replication_test.go | 41 ++++++++ operator/src/internal/otel/config.go | 15 +++ operator/src/internal/otel/config_test.go | 12 +++ 17 files changed, 568 insertions(+), 33 deletions(-) diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go index a2903fb17..e0ee9813f 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go @@ -22,6 +22,7 @@ const ( documentDbCredentialSecretParameter = "documentDbCredentialSecret" otelCollectorImageParameter = "otelCollectorImage" otelConfigMapNameParameter = "otelConfigMapName" + otelMonitorSecretParameter = "otelMonitorSecret" prometheusPortParameter = "prometheusPort" ) @@ -34,6 +35,7 @@ type Configuration struct { DocumentDbCredentialSecret string OtelCollectorImage string OtelConfigMapName string + OtelMonitorSecret string PrometheusPort int32 } @@ -89,6 +91,7 @@ func FromParameters( DocumentDbCredentialSecret: credentialSecret, OtelCollectorImage: helper.Parameters[otelCollectorImageParameter], OtelConfigMapName: helper.Parameters[otelConfigMapNameParameter], + OtelMonitorSecret: helper.Parameters[otelMonitorSecretParameter], PrometheusPort: prometheusPort, } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go index 3e8f3205a..302e35de7 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go @@ -83,6 +83,27 @@ func TestFromParameters(t *testing.T) { t.Errorf("GatewayImagePullPolicy = %q, want IfNotPresent", config.GatewayImagePullPolicy) } }) + + t.Run("parses OTel monitoring parameters", func(t *testing.T) { + helper := &common.Plugin{Parameters: map[string]string{ + "otelCollectorImage": "otel/opentelemetry-collector-contrib:test", + "otelConfigMapName": "demo-otel-config", + "otelMonitorSecret": "demo-otel-monitor", + }} + config, errs := FromParameters(helper) + if len(errs) != 0 { + t.Fatalf("unexpected validation errors: %v", errs) + } + if config.OtelCollectorImage != "otel/opentelemetry-collector-contrib:test" { + t.Errorf("OtelCollectorImage = %q", config.OtelCollectorImage) + } + if config.OtelConfigMapName != "demo-otel-config" { + t.Errorf("OtelConfigMapName = %q", config.OtelConfigMapName) + } + if config.OtelMonitorSecret != "demo-otel-monitor" { + t.Errorf("OtelMonitorSecret = %q, want demo-otel-monitor", config.OtelMonitorSecret) + } + }) } func TestToParametersRoundTrip(t *testing.T) { diff --git a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go index f028741cf..190b564ef 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go @@ -233,9 +233,11 @@ func (impl Implementation) reconcileMetadata( } // Inject OTel Collector sidecar when monitoring is enabled. - // The sidecar is only injected when the operator passes otelCollectorImage - // and otelConfigMapName parameters (i.e., monitoring.enabled is true). - if configuration.OtelCollectorImage != "" && configuration.OtelConfigMapName != "" { + // The sidecar is only injected when the operator passes otelCollectorImage, + // otelConfigMapName and otelMonitorSecret parameters (i.e., monitoring.enabled + // is true). otelMonitorSecret is required because the sidecar sources its + // PGUSER/PGPASSWORD from that secret. + if configuration.OtelCollectorImage != "" && configuration.OtelConfigMapName != "" && configuration.OtelMonitorSecret != "" { log.Printf("Injecting OTel Collector sidecar with image: %s", configuration.OtelCollectorImage) // Add ConfigMap volume for operator-generated config files (static.yaml + dynamic.yaml) @@ -260,7 +262,7 @@ func (impl Implementation) reconcileMetadata( }) } - otelSidecar := newOtelCollectorSidecar(configuration.OtelCollectorImage, cluster.Name) + otelSidecar := newOtelCollectorSidecar(configuration.OtelCollectorImage, configuration.OtelMonitorSecret) // Expose Prometheus metrics port when configured if configuration.PrometheusPort > 0 { @@ -435,8 +437,9 @@ func gatewaySecurityContext() *corev1.SecurityContext { // the caller). It carries the shared PSA-restricted SecurityContext without an // explicit UID so the upstream collector image keeps its own baked-in non-root // user (UID 10001); PSA "restricted" only requires runAsNonRoot, not a fixed -// UID. clusterName selects the CNPG-managed "-app" credential secret. -func newOtelCollectorSidecar(image, clusterName string) *corev1.Container { +// UID. monitorSecret is the operator-managed basic-auth secret holding the +// dedicated least-privilege monitoring role's credentials. +func newOtelCollectorSidecar(image, monitorSecret string) *corev1.Container { return &corev1.Container{ Name: otelCollectorContainerName, Image: image, @@ -444,10 +447,11 @@ func newOtelCollectorSidecar(image, clusterName string) *corev1.Container { "--config=file:/config/static.yaml", "--config=file:/config/dynamic.yaml", }, - // PGUSER and PGPASSWORD are sourced from the CNPG-managed application secret - // ("-app"). CNPG auto-creates this secret with "username" and "password" - // keys for the application database user. The OTel Collector's sqlquery receiver - // uses these credentials to connect to PostgreSQL and collect health metrics. + // PGUSER and PGPASSWORD are sourced from the operator-managed monitoring + // secret ("-otel-monitor"), which holds the credentials for the + // dedicated least-privilege "otel_monitor" role (member of pg_monitor). + // The OTel Collector's sqlquery receiver uses these credentials to connect + // to PostgreSQL and collect health metrics without application-level access. Env: []corev1.EnvVar{ { Name: "POD_NAME", @@ -462,7 +466,7 @@ func newOtelCollectorSidecar(image, clusterName string) *corev1.Container { ValueFrom: &corev1.EnvVarSource{ SecretKeyRef: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ - Name: clusterName + "-app", + Name: monitorSecret, }, Key: "username", }, @@ -473,7 +477,7 @@ func newOtelCollectorSidecar(image, clusterName string) *corev1.Container { ValueFrom: &corev1.EnvVarSource{ SecretKeyRef: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ - Name: clusterName + "-app", + Name: monitorSecret, }, Key: "password", }, diff --git a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go index 044edf42c..855833d76 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go @@ -186,7 +186,7 @@ func TestGatewaySecurityContext_PSARestrictedAsUID1000(t *testing.T) { // restricted and, unlike the gateway, does not force a UID so the collector // image keeps its own non-root user (UID 10001). func TestNewOtelCollectorSidecar_Hardened(t *testing.T) { - c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test", "demo") + c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test", "demo-otel-monitor") if c.Name != otelCollectorContainerName { t.Fatalf("container name = %q, want %q", c.Name, otelCollectorContainerName) @@ -199,3 +199,34 @@ func TestNewOtelCollectorSidecar_Hardened(t *testing.T) { t.Errorf("otel-collector must not force a GID, got %d", *c.SecurityContext.RunAsGroup) } } + +// TestNewOtelCollectorSidecar_MonitorSecret asserts the sidecar sources its +// PostgreSQL credentials from the dedicated least-privilege monitoring secret +// (not the app secret), so the collector connects as the pg_monitor-only role. +func TestNewOtelCollectorSidecar_MonitorSecret(t *testing.T) { + const secretName = "demo-otel-monitor" + c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test", secretName) + + want := map[string]string{"PGUSER": "username", "PGPASSWORD": "password"} + for envName, key := range want { + var found bool + for _, e := range c.Env { + if e.Name != envName { + continue + } + found = true + if e.ValueFrom == nil || e.ValueFrom.SecretKeyRef == nil { + t.Fatalf("%s must be sourced from a secret key ref", envName) + } + if got := e.ValueFrom.SecretKeyRef.Name; got != secretName { + t.Errorf("%s secret = %q, want %q", envName, got, secretName) + } + if got := e.ValueFrom.SecretKeyRef.Key; got != key { + t.Errorf("%s key = %q, want %q", envName, got, key) + } + } + if !found { + t.Errorf("missing %s env var", envName) + } + } +} diff --git a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml index cff4bae4f..1be829463 100644 --- a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml +++ b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml @@ -31,10 +31,11 @@ rules: resources: ["serviceexports", "multiclusterservices", "serviceimports", "internalserviceexports"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] # Secrets: certificate_controller reads cert-manager-issued TLS secrets to -# stamp into Cluster spec; no controller writes Secrets. Read-only. +# stamp into Cluster spec; the DocumentDB controller also generates and manages +# the per-cluster OTel monitoring credential secret (-otel-monitor). - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch"] + verbs: ["get", "list", "watch", "create", "update", "patch"] - apiGroups: ["postgresql.cnpg.io"] resources: ["clusters", "publications", "subscriptions", "clusters/status"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/operator/src/config/rbac/role.yaml b/operator/src/config/rbac/role.yaml index 7c481a3a1..fe2470603 100644 --- a/operator/src/config/rbac/role.yaml +++ b/operator/src/config/rbac/role.yaml @@ -8,10 +8,20 @@ rules: - "" resources: - persistentvolumeclaims + verbs: + - get + - list + - watch +- apiGroups: + - "" + resources: - secrets verbs: + - create - get - list + - patch + - update - watch - apiGroups: - "" diff --git a/operator/src/internal/cnpg/cnpg_cluster.go b/operator/src/internal/cnpg/cnpg_cluster.go index 591c5410b..4278c84b3 100644 --- a/operator/src/internal/cnpg/cnpg_cluster.go +++ b/operator/src/internal/cnpg/cnpg_cluster.go @@ -95,6 +95,9 @@ func GetCnpgClusterSpec(req ctrl.Request, documentdb *dbpreview.DocumentDB, docu if documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled { params["otelCollectorImage"] = util.DEFAULT_OTEL_COLLECTOR_IMAGE params["otelConfigMapName"] = otelcfg.ConfigMapName(documentdb.Name) + // Sidecar sources PGUSER/PGPASSWORD from the dedicated + // least-privilege monitoring secret rather than the app secret. + params["otelMonitorSecret"] = otelcfg.MonitorSecretName(documentdb.Name) if promPort := otelcfg.ResolvePrometheusPort(documentdb.Spec.Monitoring); promPort > 0 { params["prometheusPort"] = fmt.Sprintf("%d", promPort) } @@ -128,6 +131,7 @@ func GetCnpgClusterSpec(req ctrl.Request, documentdb *dbpreview.DocumentDB, docu spec.MaxStopDelay = getMaxStopDelayOrDefault(documentdb) applyPostgresProcessIdentity(&spec, documentdb) applyIOUringSeccomp(&spec, documentdb) + applyOtelMonitorRole(&spec, documentdb) return spec }(), @@ -348,7 +352,40 @@ func applyIOUringSeccomp(spec *cnpgv1.ClusterSpec, documentdb *dbpreview.Documen } } -// buildPostgresConfiguration returns the cnpgv1.PostgresConfiguration block +// applyOtelMonitorRole declares a dedicated least-privilege PostgreSQL role for +// the OTel Collector sidecar via CNPG's managed-roles reconciler. The role has +// LOGIN and is a member of the built-in pg_monitor role (read access to all +// pg_stat_* views) and nothing else — following the principle of least +// privilege, the monitoring sidecar cannot read or modify application data. +// +// The role password is sourced from the operator-generated basic-auth secret +// (-otel-monitor); CNPG requires the secret's "username" to match the +// role name exactly. No-op when monitoring is disabled, so clusters without +// monitoring keep no managed roles. +func applyOtelMonitorRole(spec *cnpgv1.ClusterSpec, documentdb *dbpreview.DocumentDB) { + if documentdb == nil || documentdb.Spec.Monitoring == nil || !documentdb.Spec.Monitoring.Enabled { + return + } + if spec.Managed == nil { + spec.Managed = &cnpgv1.ManagedConfiguration{} + } + spec.Managed.Roles = append(spec.Managed.Roles, cnpgv1.RoleConfiguration{ + Name: otelcfg.MonitorRoleName, + Comment: "Least-privilege role for the OTel Collector monitoring sidecar", + Ensure: cnpgv1.EnsurePresent, + Login: true, + InRoles: []string{"pg_monitor"}, + PasswordSecret: &cnpgv1.LocalObjectReference{ + Name: otelcfg.MonitorSecretName(documentdb.Name), + }, + // Set the CNPG/CRD-defaulted fields explicitly so the desired role + // matches the API-server-defaulted form stored on the live cluster, + // keeping SyncCnpgCluster's diff stable (no perpetual re-patching). + ConnectionLimit: -1, + Inherit: pointer.Bool(true), + }) +} + // for the cluster. // // The operator declares the DocumentDB extension via CNPG's Extensions diff --git a/operator/src/internal/cnpg/cnpg_cluster_test.go b/operator/src/internal/cnpg/cnpg_cluster_test.go index 8b7c7ad19..adab721b0 100644 --- a/operator/src/internal/cnpg/cnpg_cluster_test.go +++ b/operator/src/internal/cnpg/cnpg_cluster_test.go @@ -697,6 +697,70 @@ var _ = Describe("GetCnpgClusterSpec", func() { Expect(pluginParams).NotTo(HaveKey("otelConfigMapName")) }) + It("declares the otel_monitor managed role and passes otelMonitorSecret when monitoring is enabled", func() { + req := ctrl.Request{} + req.Name = "test-cluster" + req.Namespace = "default" + + documentdb := &dbpreview.DocumentDB{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "default", + }, + Spec: dbpreview.DocumentDBSpec{ + InstancesPerNode: 1, + Resource: dbpreview.Resource{ + Storage: dbpreview.StorageConfiguration{ + PvcSize: "10Gi", + }, + }, + Monitoring: &dbpreview.MonitoringSpec{Enabled: true}, + }, + } + + cluster := GetCnpgClusterSpec(req, documentdb, "test-image:latest", "test-sa", "", true, log) + + pluginParams := cluster.Spec.Plugins[0].Parameters + Expect(pluginParams).To(HaveKeyWithValue("otelMonitorSecret", "test-cluster-otel-monitor")) + + Expect(cluster.Spec.Managed).NotTo(BeNil()) + Expect(cluster.Spec.Managed.Roles).To(HaveLen(1)) + role := cluster.Spec.Managed.Roles[0] + Expect(role.Name).To(Equal("otel_monitor")) + Expect(role.Login).To(BeTrue()) + Expect(role.Ensure).To(Equal(cnpgv1.EnsurePresent)) + Expect(role.InRoles).To(ConsistOf("pg_monitor")) + Expect(role.PasswordSecret).NotTo(BeNil()) + Expect(role.PasswordSecret.Name).To(Equal("test-cluster-otel-monitor")) + Expect(role.Superuser).To(BeFalse()) + Expect(role.CreateDB).To(BeFalse()) + Expect(role.CreateRole).To(BeFalse()) + }) + + It("does not declare managed roles when monitoring is disabled", func() { + req := ctrl.Request{} + req.Name = "test-cluster" + req.Namespace = "default" + + documentdb := &dbpreview.DocumentDB{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, + Spec: dbpreview.DocumentDBSpec{ + InstancesPerNode: 1, + Resource: dbpreview.Resource{ + Storage: dbpreview.StorageConfiguration{PvcSize: "10Gi"}, + }, + Monitoring: &dbpreview.MonitoringSpec{Enabled: false}, + }, + } + + cluster := GetCnpgClusterSpec(req, documentdb, "test-image:latest", "test-sa", "", true, log) + + Expect(cluster.Spec.Plugins[0].Parameters).NotTo(HaveKey("otelMonitorSecret")) + if cluster.Spec.Managed != nil { + Expect(cluster.Spec.Managed.Roles).To(BeEmpty()) + } + }) + It("propagates spec.imagePullSecrets to the CNPG cluster spec", func() { req := ctrl.Request{} req.Name = "test-cluster" diff --git a/operator/src/internal/cnpg/cnpg_patch.go b/operator/src/internal/cnpg/cnpg_patch.go index f42e414e3..e34676bde 100644 --- a/operator/src/internal/cnpg/cnpg_patch.go +++ b/operator/src/internal/cnpg/cnpg_patch.go @@ -25,6 +25,8 @@ const ( PatchPathReplicationSlots = "/spec/replicationSlots" PatchPathExternalClusters = "/spec/externalClusters" PatchPathManagedServices = "/spec/managed/services/additional" + PatchPathManaged = "/spec/managed" + PatchPathManagedRoles = "/spec/managed/roles" PatchPathSynchronous = "/spec/postgresql/synchronous" PatchPathBootstrap = "/spec/bootstrap" diff --git a/operator/src/internal/cnpg/cnpg_sync.go b/operator/src/internal/cnpg/cnpg_sync.go index 4d32f66e3..885fc5a86 100644 --- a/operator/src/internal/cnpg/cnpg_sync.go +++ b/operator/src/internal/cnpg/cnpg_sync.go @@ -103,7 +103,7 @@ func SyncCnpgCluster( // (e.g. Prometheus port, collector image) can take effect without restarting // database pods — for example, by updating the ConfigMap in-place and // signalling the OTel Collector to reload its configuration. - otelKeys := []string{"otelCollectorImage", "otelConfigMapName", "prometheusPort", "otelConfigHash"} + otelKeys := []string{"otelCollectorImage", "otelConfigMapName", "prometheusPort", "otelConfigHash", "otelMonitorSecret"} for _, key := range otelKeys { desiredVal := getParam(desiredPlugin.Parameters, key) currentVal := getParam(currentPlugin.Parameters, key) @@ -215,6 +215,29 @@ func SyncCnpgCluster( }) } + // Managed roles (the OTel monitoring role) — added when monitoring is + // enabled, cleared when disabled. Reconciled independently of + // managed.services (owned by the replication flow via extraOps) so the two + // never clobber each other. When the cluster has no managed config yet, add + // the whole desired managed block (which also carries any services the + // replication flow populated on the same desired object); otherwise patch + // only the roles subtree to preserve existing services. + if !reflect.DeepEqual(managedRoles(current), managedRoles(desired)) { + if current.Spec.Managed == nil { + patchOps = append(patchOps, JSONPatch{ + Op: PatchOpAdd, + Path: PatchPathManaged, + Value: desired.Spec.Managed, + }) + } else { + patchOps = append(patchOps, JSONPatch{ + Op: PatchOpAdd, + Path: PatchPathManagedRoles, + Value: managedRoles(desired), + }) + } + } + // Extra operations (e.g., replication changes) patchOps = append(patchOps, extraOps...) @@ -290,3 +313,12 @@ func getParam(params map[string]string, key string) string { } return params[key] } + +// managedRoles returns the cluster's managed roles, nil-safe against an absent +// managed configuration. +func managedRoles(cluster *cnpgv1.Cluster) []cnpgv1.RoleConfiguration { + if cluster == nil || cluster.Spec.Managed == nil { + return nil + } + return cluster.Spec.Managed.Roles +} diff --git a/operator/src/internal/cnpg/cnpg_sync_test.go b/operator/src/internal/cnpg/cnpg_sync_test.go index 098511382..9ae4175c4 100644 --- a/operator/src/internal/cnpg/cnpg_sync_test.go +++ b/operator/src/internal/cnpg/cnpg_sync_test.go @@ -479,6 +479,102 @@ var _ = Describe("SyncCnpgCluster - mutable spec fields", func() { }) }) +var _ = Describe("SyncCnpgCluster - managed roles", func() { + const namespace = "test-ns" + + otelRole := func(secret string) cnpgv1.RoleConfiguration { + return cnpgv1.RoleConfiguration{ + Name: "otel_monitor", + Ensure: cnpgv1.EnsurePresent, + Login: true, + InRoles: []string{"pg_monitor"}, + PasswordSecret: &cnpgv1.LocalObjectReference{Name: secret}, + ConnectionLimit: -1, + Inherit: pointer.Bool(true), + } + } + + It("adds the managed role when monitoring is enabled on a cluster without managed config", func() { + current := baseCluster("test-cluster", namespace) + Expect(current.Spec.Managed).To(BeNil()) + desired := current.DeepCopy() + desired.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + } + + c := buildFakeClient(current).Build() + Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) + + updated := &cnpgv1.Cluster{} + Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) + Expect(updated.Spec.Managed).NotTo(BeNil()) + Expect(updated.Spec.Managed.Roles).To(HaveLen(1)) + Expect(updated.Spec.Managed.Roles[0].Name).To(Equal("otel_monitor")) + Expect(updated.Spec.Managed.Roles[0].PasswordSecret.Name).To(Equal("test-cluster-otel-monitor")) + }) + + It("removes the managed role when monitoring is disabled", func() { + current := baseCluster("test-cluster", namespace) + current.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + } + desired := current.DeepCopy() + desired.Spec.Managed = nil + + c := buildFakeClient(current).Build() + Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) + + updated := &cnpgv1.Cluster{} + Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) + Expect(managedRoles(updated)).To(BeEmpty()) + }) + + It("preserves managed.services when patching only roles", func() { + current := baseCluster("test-cluster", namespace) + current.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Services: &cnpgv1.ManagedServices{ + Additional: []cnpgv1.ManagedService{ + { + SelectorType: cnpgv1.ServiceSelectorTypeRW, + ServiceTemplate: cnpgv1.ServiceTemplateSpec{ + ObjectMeta: cnpgv1.Metadata{Name: "extra-svc"}, + }, + }, + }, + }, + } + desired := current.DeepCopy() + desired.Spec.Managed.Roles = []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")} + + c := buildFakeClient(current).Build() + Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) + + updated := &cnpgv1.Cluster{} + Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) + Expect(updated.Spec.Managed.Roles).To(HaveLen(1)) + Expect(updated.Spec.Managed.Services).NotTo(BeNil()) + Expect(updated.Spec.Managed.Services.Additional).To(HaveLen(1)) + Expect(updated.Spec.Managed.Services.Additional[0].ServiceTemplate.ObjectMeta.Name).To(Equal("extra-svc")) + }) + + It("does not patch when managed roles are unchanged", func() { + current := baseCluster("test-cluster", namespace) + current.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + } + desired := current.DeepCopy() + + c := buildFakeClient(current).Build() + Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) + + updated := &cnpgv1.Cluster{} + Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) + Expect(updated.Spec.Managed.Roles).To(HaveLen(1)) + // No spec drift, so no restart annotation is added. + Expect(updated.Annotations).ToNot(HaveKey("kubectl.kubernetes.io/restartedAt")) + }) +}) + var _ = Describe("Helper functions", func() { It("findExtensionImage returns -1 for cluster without extensions", func() { cluster := &cnpgv1.Cluster{ diff --git a/operator/src/internal/controller/documentdb_controller.go b/operator/src/internal/controller/documentdb_controller.go index 4a8b7dd43..1dc2417b9 100644 --- a/operator/src/internal/controller/documentdb_controller.go +++ b/operator/src/internal/controller/documentdb_controller.go @@ -6,6 +6,8 @@ package controller import ( "bytes" "context" + "crypto/rand" + "encoding/base64" "fmt" "slices" "strconv" @@ -74,6 +76,7 @@ var reconcileMutex sync.Mutex // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { reconcileMutex.Lock() defer reconcileMutex.Unlock() @@ -179,6 +182,10 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) // the operator triggers a rolling restart (via restart annotation) // and CNPG manages the pod rollout. if documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled { + if err := r.reconcileOtelMonitorSecret(ctx, documentdb, req.Namespace); err != nil { + logger.Error(err, "Failed to reconcile OTel monitoring secret") + return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil + } if err := r.reconcileOtelConfigMap(ctx, documentdb, req.Namespace); err != nil { logger.Error(err, "Failed to reconcile OTel ConfigMap") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil @@ -188,6 +195,10 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) logger.Error(err, "Failed to clean up OTel ConfigMap") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil } + if err := r.deleteOtelMonitorSecret(ctx, documentdb.Name, req.Namespace); err != nil { + logger.Error(err, "Failed to clean up OTel monitoring secret") + return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil + } } if err := r.Client.Get(ctx, types.NamespacedName{Name: desiredCnpgCluster.Name, Namespace: req.Namespace}, currentCnpgCluster); err != nil { @@ -1105,3 +1116,78 @@ func (r *DocumentDBReconciler) deleteOtelConfigMap(ctx context.Context, clusterN logger.Info("OTel ConfigMap deleted", "name", cmName) return nil } + +// reconcileOtelMonitorSecret ensures the dedicated basic-auth Secret holding the +// least-privilege OTel monitoring role's credentials exists. CNPG consumes it as +// the managed role's passwordSecret and the OTel Collector sidecar sources +// PGUSER/PGPASSWORD from it. The password is generated once and preserved across +// reconciles so it stays stable for the managed role and the running sidecar. +func (r *DocumentDBReconciler) reconcileOtelMonitorSecret(ctx context.Context, documentdb *dbpreview.DocumentDB, namespace string) error { + logger := log.FromContext(ctx) + secretName := otelcfg.MonitorSecretName(documentdb.Name) + + secret := &corev1.Secret{} + secret.Name = secretName + secret.Namespace = namespace + + result, err := controllerutil.CreateOrUpdate(ctx, r.Client, secret, func() error { + // Owner reference so the Secret is garbage-collected with the DocumentDB CR. + if err := controllerutil.SetControllerReference(documentdb, secret, r.Scheme); err != nil { + return fmt.Errorf("failed to set owner reference: %w", err) + } + secret.Type = corev1.SecretTypeBasicAuth + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + // CNPG requires the secret username to match the managed role name exactly. + secret.Data[corev1.BasicAuthUsernameKey] = []byte(otelcfg.MonitorRoleName) + // Generate the password only once; preserve it on subsequent reconciles so + // the managed role's password and the sidecar's cached env stay in sync. + if len(secret.Data[corev1.BasicAuthPasswordKey]) == 0 { + password, genErr := generateRandomPassword() + if genErr != nil { + return fmt.Errorf("failed to generate monitoring password: %w", genErr) + } + secret.Data[corev1.BasicAuthPasswordKey] = []byte(password) + } + return nil + }) + if err != nil { + return fmt.Errorf("failed to reconcile OTel monitoring secret %s: %w", secretName, err) + } + if result != controllerutil.OperationResultNone { + logger.Info("OTel monitoring secret reconciled", "name", secretName, "operation", result) + } + return nil +} + +// deleteOtelMonitorSecret removes the OTel monitoring Secret when monitoring is +// no longer configured. +func (r *DocumentDBReconciler) deleteOtelMonitorSecret(ctx context.Context, clusterName, namespace string) error { + logger := log.FromContext(ctx) + secretName := otelcfg.MonitorSecretName(clusterName) + + secret := &corev1.Secret{} + secret.Name = secretName + secret.Namespace = namespace + + err := r.Client.Delete(ctx, secret) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to delete OTel monitoring secret %s: %w", secretName, err) + } + logger.Info("OTel monitoring secret deleted", "name", secretName) + return nil +} + +// generateRandomPassword returns a cryptographically random, URL-safe password +// suitable for a PostgreSQL role. +func generateRandomPassword() (string, error) { + buf := make([]byte, 24) + if _, err := rand.Read(buf); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} diff --git a/operator/src/internal/controller/documentdb_controller_test.go b/operator/src/internal/controller/documentdb_controller_test.go index 547f11e0e..ec63f0f1d 100644 --- a/operator/src/internal/controller/documentdb_controller_test.go +++ b/operator/src/internal/controller/documentdb_controller_test.go @@ -3342,4 +3342,74 @@ var _ = Describe("DocumentDB Controller", func() { Expect(err.Error()).To(ContainSubstring("failed to delete OTel ConfigMap")) }) }) + + Describe("reconcileOtelMonitorSecret", func() { + monitorSecretName := documentDBName + "-otel-monitor" + + newDocumentDB := func() *dbpreview.DocumentDB { + return &dbpreview.DocumentDB{ + ObjectMeta: metav1.ObjectMeta{ + Name: documentDBName, + Namespace: documentDBNamespace, + }, + Spec: dbpreview.DocumentDBSpec{ + Monitoring: &dbpreview.MonitoringSpec{Enabled: true}, + }, + } + } + + It("creates a basic-auth secret with the otel_monitor username and a generated password", func() { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + + Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) + + secret := &corev1.Secret{} + Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, secret)).To(Succeed()) + Expect(secret.Type).To(Equal(corev1.SecretTypeBasicAuth)) + Expect(string(secret.Data[corev1.BasicAuthUsernameKey])).To(Equal("otel_monitor")) + Expect(secret.Data[corev1.BasicAuthPasswordKey]).ToNot(BeEmpty()) + Expect(secret.OwnerReferences).To(HaveLen(1)) + Expect(secret.OwnerReferences[0].Name).To(Equal(documentDBName)) + }) + + It("preserves the existing password across reconciles", func() { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + + Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) + first := &corev1.Secret{} + Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, first)).To(Succeed()) + original := append([]byte(nil), first.Data[corev1.BasicAuthPasswordKey]...) + + Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) + second := &corev1.Secret{} + Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, second)).To(Succeed()) + Expect(second.Data[corev1.BasicAuthPasswordKey]).To(Equal(original)) + }) + }) + + Describe("deleteOtelMonitorSecret", func() { + monitorSecretName := documentDBName + "-otel-monitor" + + It("deletes the monitoring secret when present", func() { + existing := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: monitorSecretName, Namespace: documentDBNamespace}, + Type: corev1.SecretTypeBasicAuth, + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + + Expect(reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace)).To(Succeed()) + secret := &corev1.Secret{} + err := fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, secret) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) + + It("is a no-op when the monitoring secret does not exist", func() { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + Expect(reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace)).To(Succeed()) + }) + }) }) diff --git a/operator/src/internal/controller/physical_replication.go b/operator/src/internal/controller/physical_replication.go index 668117bd1..877bbe20f 100644 --- a/operator/src/internal/controller/physical_replication.go +++ b/operator/src/internal/controller/physical_replication.go @@ -109,22 +109,7 @@ func (r *DocumentDBReconciler) AddClusterReplicationToClusterSpec( if replicationContext.IsAzureFleetNetworking() { // need to create services for each of the other clusters - cnpgCluster.Spec.Managed = &cnpgv1.ManagedConfiguration{ - Services: &cnpgv1.ManagedServices{ - Additional: []cnpgv1.ManagedService{}, - }, - } - for serviceName := range replicationContext.GenerateOutgoingServiceNames(documentdb.Name, documentdb.Namespace) { - cnpgCluster.Spec.Managed.Services.Additional = append(cnpgCluster.Spec.Managed.Services.Additional, - cnpgv1.ManagedService{ - SelectorType: cnpgv1.ServiceSelectorTypeRW, - ServiceTemplate: cnpgv1.ServiceTemplateSpec{ - ObjectMeta: cnpgv1.Metadata{ - Name: serviceName, - }, - }, - }) - } + addAzureFleetManagedServices(cnpgCluster, replicationContext, documentdb) } selfHost := replicationContext.CNPGClusterName + "-rw." + documentdb.Namespace + ".svc" cnpgCluster.Spec.ExternalClusters = []cnpgv1.ExternalCluster{ @@ -153,6 +138,31 @@ func (r *DocumentDBReconciler) AddClusterReplicationToClusterSpec( return nil } +// addAzureFleetManagedServices populates spec.managed.services.additional with a +// managed RW service per outgoing fleet member. It preserves any existing managed +// configuration (e.g. the OTel monitoring role set by GetCnpgClusterSpec) rather +// than replacing the whole managed block, so enabling fleet networking does not +// drop managed roles. +func addAzureFleetManagedServices(cnpgCluster *cnpgv1.Cluster, replicationContext *util.ReplicationContext, documentdb *dbpreview.DocumentDB) { + if cnpgCluster.Spec.Managed == nil { + cnpgCluster.Spec.Managed = &cnpgv1.ManagedConfiguration{} + } + cnpgCluster.Spec.Managed.Services = &cnpgv1.ManagedServices{ + Additional: []cnpgv1.ManagedService{}, + } + for serviceName := range replicationContext.GenerateOutgoingServiceNames(documentdb.Name, documentdb.Namespace) { + cnpgCluster.Spec.Managed.Services.Additional = append(cnpgCluster.Spec.Managed.Services.Additional, + cnpgv1.ManagedService{ + SelectorType: cnpgv1.ServiceSelectorTypeRW, + ServiceTemplate: cnpgv1.ServiceTemplateSpec{ + ObjectMeta: cnpgv1.Metadata{ + Name: serviceName, + }, + }, + }) + } +} + func (r *DocumentDBReconciler) CreateIstioRemoteServices(ctx context.Context, replicationContext *util.ReplicationContext, documentdb *dbpreview.DocumentDB) error { // Create dummy -rw services for remote clusters so DNS resolution works // These services have non-matching selectors, so they have no local endpoints diff --git a/operator/src/internal/controller/physical_replication_test.go b/operator/src/internal/controller/physical_replication_test.go index 50cd65978..87a2e0f54 100644 --- a/operator/src/internal/controller/physical_replication_test.go +++ b/operator/src/internal/controller/physical_replication_test.go @@ -654,3 +654,44 @@ var _ = Describe("Physical Replication", func() { Expect(updated.Spec.PostgresConfiguration.Synchronous.Number).To(Equal(2)) }) }) + +var _ = Describe("addAzureFleetManagedServices", func() { + newContext := func() *util.ReplicationContext { + return &util.ReplicationContext{ + CrossCloudNetworkingStrategy: util.AzureFleet, + CNPGClusterName: "cluster-a", + OtherCNPGClusterNames: []string{"cluster-b"}, + } + } + + It("adds managed RW services for outgoing fleet members", func() { + documentdb := baseDocumentDB("docdb-fleet", "default") + cnpgCluster := &cnpgv1.Cluster{} + + addAzureFleetManagedServices(cnpgCluster, newContext(), documentdb) + + Expect(cnpgCluster.Spec.Managed).NotTo(BeNil()) + Expect(cnpgCluster.Spec.Managed.Services).NotTo(BeNil()) + Expect(cnpgCluster.Spec.Managed.Services.Additional).To(HaveLen(1)) + Expect(cnpgCluster.Spec.Managed.Services.Additional[0].SelectorType).To(Equal(cnpgv1.ServiceSelectorTypeRW)) + }) + + It("preserves pre-existing managed roles when adding fleet services", func() { + documentdb := baseDocumentDB("docdb-fleet", "default") + cnpgCluster := &cnpgv1.Cluster{ + Spec: cnpgv1.ClusterSpec{ + Managed: &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{ + {Name: "otel_monitor", Login: true, InRoles: []string{"pg_monitor"}}, + }, + }, + }, + } + + addAzureFleetManagedServices(cnpgCluster, newContext(), documentdb) + + Expect(cnpgCluster.Spec.Managed.Roles).To(HaveLen(1)) + Expect(cnpgCluster.Spec.Managed.Roles[0].Name).To(Equal("otel_monitor")) + Expect(cnpgCluster.Spec.Managed.Services.Additional).To(HaveLen(1)) + }) +}) diff --git a/operator/src/internal/otel/config.go b/operator/src/internal/otel/config.go index 59c1584d7..07daa141d 100644 --- a/operator/src/internal/otel/config.go +++ b/operator/src/internal/otel/config.go @@ -19,6 +19,21 @@ var baseConfigYAML []byte const defaultPrometheusPort = 8888 +// MonitorRoleName is the dedicated least-privilege PostgreSQL role the OTel +// Collector sidecar uses to run health-check queries and read pg_stat_* views. +// It is granted membership in the built-in pg_monitor role and nothing else. +// CNPG's managed-roles reconciler requires the username stored in the password +// secret to match this name exactly. +const MonitorRoleName = "otel_monitor" + +// MonitorSecretName returns the name of the basic-auth Secret holding the OTel +// monitoring role's credentials for a given DocumentDB cluster. The operator +// generates and owns this Secret; CNPG reads it (as the managed role's +// passwordSecret) and the sidecar sources PGUSER/PGPASSWORD from it. +func MonitorSecretName(clusterName string) string { + return fmt.Sprintf("%s-otel-monitor", clusterName) +} + // collectorConfig represents the OTel Collector configuration structure. type collectorConfig struct { Receivers map[string]any `yaml:"receivers,omitempty"` diff --git a/operator/src/internal/otel/config_test.go b/operator/src/internal/otel/config_test.go index e44502650..9f0a94008 100644 --- a/operator/src/internal/otel/config_test.go +++ b/operator/src/internal/otel/config_test.go @@ -25,6 +25,18 @@ var _ = Describe("ConfigMapName", func() { }) }) +var _ = Describe("MonitorSecretName", func() { + It("returns the expected monitoring secret name", func() { + Expect(MonitorSecretName("my-cluster")).To(Equal("my-cluster-otel-monitor")) + }) +}) + +var _ = Describe("MonitorRoleName", func() { + It("is the dedicated least-privilege monitoring role", func() { + Expect(MonitorRoleName).To(Equal("otel_monitor")) + }) +}) + // parseCfg is a helper to unmarshal YAML into a collectorConfig struct. func parseCfg(yamlStr string) collectorConfig { var cfg collectorConfig From 130bcbcbd222d6a0f0339be57d0c1b61a05c7b3a Mon Sep 17 00:00:00 2001 From: urismiley Date: Thu, 9 Jul 2026 17:23:31 -0400 Subject: [PATCH 2/6] fix: address OTel monitoring review feedback Copilot-Session: ba406fac-b035-4384-a495-46e2661fafdb Signed-off-by: urismiley --- .../internal/config/config.go | 35 +++++- .../internal/config/config_test.go | 36 +++++++ .../internal/operator/validation.go | 5 +- .../internal/operator/validation_test.go | 54 ++++++++++ operator/src/config/rbac/role.yaml | 2 + operator/src/internal/cnpg/cnpg_sync.go | 59 ++++++---- operator/src/internal/cnpg/cnpg_sync_test.go | 16 +++ .../controller/documentdb_controller.go | 30 +++--- .../controller/documentdb_controller_test.go | 101 ++++++++++++++++++ 9 files changed, 302 insertions(+), 36 deletions(-) create mode 100644 operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go index e0ee9813f..681ad5734 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go @@ -22,6 +22,7 @@ const ( documentDbCredentialSecretParameter = "documentDbCredentialSecret" otelCollectorImageParameter = "otelCollectorImage" otelConfigMapNameParameter = "otelConfigMapName" + otelConfigHashParameter = "otelConfigHash" otelMonitorSecretParameter = "otelMonitorSecret" prometheusPortParameter = "prometheusPort" ) @@ -69,6 +70,9 @@ func FromParameters( gatewayImage := helper.Parameters[gatewayImageParameter] credentialSecret := helper.Parameters[documentDbCredentialSecretParameter] pullPolicy := parsePullPolicy(helper.Parameters[gatewayImagePullPolicyParameter]) + otelCollectorImage := helper.Parameters[otelCollectorImageParameter] + otelConfigMapName := helper.Parameters[otelConfigMapNameParameter] + otelMonitorSecret := helper.Parameters[otelMonitorSecretParameter] var prometheusPort int32 if portStr := helper.Parameters[prometheusPortParameter]; portStr != "" { @@ -83,15 +87,40 @@ func FromParameters( } } + requiredOtelParameters := []string{ + otelCollectorImageParameter, + otelConfigMapNameParameter, + otelMonitorSecretParameter, + } + otelConfigured := helper.Parameters[prometheusPortParameter] != "" || + helper.Parameters[otelConfigHashParameter] != "" + for _, parameter := range requiredOtelParameters { + otelConfigured = otelConfigured || helper.Parameters[parameter] != "" + } + if otelConfigured { + for _, parameter := range requiredOtelParameters { + if helper.Parameters[parameter] == "" { + validationErrors = append( + validationErrors, + validation.BuildErrorForParameter( + helper, + parameter, + "required when any OTel sidecar parameter is configured", + ), + ) + } + } + } + configuration := &Configuration{ Labels: labels, Annotations: annotations, GatewayImage: gatewayImage, GatewayImagePullPolicy: pullPolicy, DocumentDbCredentialSecret: credentialSecret, - OtelCollectorImage: helper.Parameters[otelCollectorImageParameter], - OtelConfigMapName: helper.Parameters[otelConfigMapNameParameter], - OtelMonitorSecret: helper.Parameters[otelMonitorSecretParameter], + OtelCollectorImage: otelCollectorImage, + OtelConfigMapName: otelConfigMapName, + OtelMonitorSecret: otelMonitorSecret, PrometheusPort: prometheusPort, } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go index 302e35de7..3d6918736 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go @@ -104,6 +104,42 @@ func TestFromParameters(t *testing.T) { t.Errorf("OtelMonitorSecret = %q, want demo-otel-monitor", config.OtelMonitorSecret) } }) + + for _, tt := range []struct { + name string + parameters map[string]string + wantErrors int + }{ + { + name: "rejects collector image without config map and monitor secret", + parameters: map[string]string{ + "otelCollectorImage": "otel/opentelemetry-collector-contrib:test", + }, + wantErrors: 2, + }, + { + name: "rejects config map and monitor secret without collector image", + parameters: map[string]string{ + "otelConfigMapName": "demo-otel-config", + "otelMonitorSecret": "demo-otel-monitor", + }, + wantErrors: 1, + }, + { + name: "rejects optional OTel parameter without required parameters", + parameters: map[string]string{ + "prometheusPort": "8888", + }, + wantErrors: 3, + }, + } { + t.Run(tt.name, func(t *testing.T) { + _, errs := FromParameters(&common.Plugin{Parameters: tt.parameters}) + if len(errs) != tt.wantErrors { + t.Fatalf("validation errors = %d, want %d: %v", len(errs), tt.wantErrors, errs) + } + }) + } } func TestToParametersRoundTrip(t *testing.T) { diff --git a/operator/cnpg-plugins/sidecar-injector/internal/operator/validation.go b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation.go index b191ddc4f..fabd1e9c6 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/operator/validation.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation.go @@ -66,7 +66,10 @@ func (Implementation) ValidateClusterChange( var newConfiguration *config.Configuration newConfiguration, result.ValidationErrors = config.FromParameters(newClusterHelper) oldConfiguration, _ := config.FromParameters(oldClusterHelper) - result.ValidationErrors = config.ValidateChanges(oldConfiguration, newConfiguration, newClusterHelper) + result.ValidationErrors = append( + result.ValidationErrors, + config.ValidateChanges(oldConfiguration, newConfiguration, newClusterHelper)..., + ) return result, nil } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go new file mode 100644 index 000000000..bb66519db --- /dev/null +++ b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go @@ -0,0 +1,54 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package operator + +import ( + "context" + "encoding/json" + "testing" + + cnpgv1 "github.com/cloudnative-pg/api/pkg/api/v1" + cnpgoperator "github.com/cloudnative-pg/cnpg-i/pkg/operator" + + "github.com/documentdb/cnpg-i-sidecar-injector/pkg/metadata" +) + +func TestValidateClusterChangeRetainsParameterErrors(t *testing.T) { + oldCluster := &cnpgv1.Cluster{} + newCluster := &cnpgv1.Cluster{ + Spec: cnpgv1.ClusterSpec{ + Plugins: []cnpgv1.PluginConfiguration{ + { + Name: metadata.PluginName, + Parameters: map[string]string{ + "otelCollectorImage": "otel/opentelemetry-collector-contrib:test", + }, + }, + }, + }, + } + + oldJSON, err := json.Marshal(oldCluster) + if err != nil { + t.Fatalf("marshal old cluster: %v", err) + } + newJSON, err := json.Marshal(newCluster) + if err != nil { + t.Fatalf("marshal new cluster: %v", err) + } + + result, err := (Implementation{}).ValidateClusterChange( + context.Background(), + &cnpgoperator.OperatorValidateClusterChangeRequest{ + OldCluster: oldJSON, + NewCluster: newJSON, + }, + ) + if err != nil { + t.Fatalf("ValidateClusterChange() error: %v", err) + } + if got, want := len(result.ValidationErrors), 2; got != want { + t.Fatalf("validation errors = %d, want %d: %v", got, want, result.ValidationErrors) + } +} diff --git a/operator/src/config/rbac/role.yaml b/operator/src/config/rbac/role.yaml index fe2470603..f5f68b821 100644 --- a/operator/src/config/rbac/role.yaml +++ b/operator/src/config/rbac/role.yaml @@ -9,6 +9,8 @@ rules: resources: - persistentvolumeclaims verbs: + - create + - delete - get - list - watch diff --git a/operator/src/internal/cnpg/cnpg_sync.go b/operator/src/internal/cnpg/cnpg_sync.go index 885fc5a86..457fac12f 100644 --- a/operator/src/internal/cnpg/cnpg_sync.go +++ b/operator/src/internal/cnpg/cnpg_sync.go @@ -215,27 +215,11 @@ func SyncCnpgCluster( }) } - // Managed roles (the OTel monitoring role) — added when monitoring is - // enabled, cleared when disabled. Reconciled independently of + // Managed roles (the OTel monitoring role) are reconciled independently of // managed.services (owned by the replication flow via extraOps) so the two - // never clobber each other. When the cluster has no managed config yet, add - // the whole desired managed block (which also carries any services the - // replication flow populated on the same desired object); otherwise patch - // only the roles subtree to preserve existing services. - if !reflect.DeepEqual(managedRoles(current), managedRoles(desired)) { - if current.Spec.Managed == nil { - patchOps = append(patchOps, JSONPatch{ - Op: PatchOpAdd, - Path: PatchPathManaged, - Value: desired.Spec.Managed, - }) - } else { - patchOps = append(patchOps, JSONPatch{ - Op: PatchOpAdd, - Path: PatchPathManagedRoles, - Value: managedRoles(desired), - }) - } + // never clobber each other. + if patch := managedRolesPatch(current, desired); patch != nil { + patchOps = append(patchOps, *patch) } // Extra operations (e.g., replication changes) @@ -322,3 +306,38 @@ func managedRoles(cluster *cnpgv1.Cluster) []cnpgv1.RoleConfiguration { } return cluster.Spec.Managed.Roles } + +// managedRolesPatch returns the JSON Patch operation needed to reconcile +// spec.managed.roles. Removing the field when no roles are desired avoids +// assigning JSON null to the CRD array field, which Kubernetes validation can +// reject. Other managed configuration, such as services, remains untouched. +func managedRolesPatch(current, desired *cnpgv1.Cluster) *JSONPatch { + currentRoles := managedRoles(current) + desiredRoles := managedRoles(desired) + + // Treat nil and empty role lists as equivalent. + if len(currentRoles) == 0 && len(desiredRoles) == 0 { + return nil + } + if reflect.DeepEqual(currentRoles, desiredRoles) { + return nil + } + if len(desiredRoles) == 0 { + return &JSONPatch{ + Op: PatchOpRemove, + Path: PatchPathManagedRoles, + } + } + if current.Spec.Managed == nil { + return &JSONPatch{ + Op: PatchOpAdd, + Path: PatchPathManaged, + Value: desired.Spec.Managed, + } + } + return &JSONPatch{ + Op: PatchOpAdd, + Path: PatchPathManagedRoles, + Value: desiredRoles, + } +} diff --git a/operator/src/internal/cnpg/cnpg_sync_test.go b/operator/src/internal/cnpg/cnpg_sync_test.go index 9ae4175c4..9b8091269 100644 --- a/operator/src/internal/cnpg/cnpg_sync_test.go +++ b/operator/src/internal/cnpg/cnpg_sync_test.go @@ -521,6 +521,12 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { desired := current.DeepCopy() desired.Spec.Managed = nil + patch := managedRolesPatch(current, desired) + Expect(patch).NotTo(BeNil()) + Expect(patch.Op).To(Equal(PatchOpRemove)) + Expect(patch.Path).To(Equal(PatchPathManagedRoles)) + Expect(patch.Value).To(BeNil()) + c := buildFakeClient(current).Build() Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) @@ -573,6 +579,16 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { // No spec drift, so no restart annotation is added. Expect(updated.Annotations).ToNot(HaveKey("kubectl.kubernetes.io/restartedAt")) }) + + It("treats nil and empty managed role lists as equivalent", func() { + current := baseCluster("test-cluster", namespace) + desired := current.DeepCopy() + desired.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{}, + } + + Expect(managedRolesPatch(current, desired)).To(BeNil()) + }) }) var _ = Describe("Helper functions", func() { diff --git a/operator/src/internal/controller/documentdb_controller.go b/operator/src/internal/controller/documentdb_controller.go index 1dc2417b9..65e8b9617 100644 --- a/operator/src/internal/controller/documentdb_controller.go +++ b/operator/src/internal/controller/documentdb_controller.go @@ -176,12 +176,12 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) return result, nil } - // Reconcile OTel Collector ConfigMap when monitoring is enabled. - // When monitoring is disabled or removed, delete the ConfigMap. + // Reconcile OTel resources when monitoring is enabled. // The sidecar itself is added/removed via CNPG plugin parameters; // the operator triggers a rolling restart (via restart annotation) // and CNPG manages the pod rollout. - if documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled { + monitoringEnabled := documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled + if monitoringEnabled { if err := r.reconcileOtelMonitorSecret(ctx, documentdb, req.Namespace); err != nil { logger.Error(err, "Failed to reconcile OTel monitoring secret") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil @@ -190,15 +190,6 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) logger.Error(err, "Failed to reconcile OTel ConfigMap") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil } - } else { - if err := r.deleteOtelConfigMap(ctx, documentdb.Name, req.Namespace); err != nil { - logger.Error(err, "Failed to clean up OTel ConfigMap") - return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil - } - if err := r.deleteOtelMonitorSecret(ctx, documentdb.Name, req.Namespace); err != nil { - logger.Error(err, "Failed to clean up OTel monitoring secret") - return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil - } } if err := r.Client.Get(ctx, types.NamespacedName{Name: desiredCnpgCluster.Name, Namespace: req.Namespace}, currentCnpgCluster); err != nil { @@ -230,6 +221,21 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil } + // Delete the OTel resources only after the CNPG Cluster spec no longer + // references them. If cluster sync fails, retaining these objects keeps the + // existing managed role and sidecar configuration functional and allows a + // subsequent reconcile to retry safely. + if !monitoringEnabled { + if err := r.deleteOtelConfigMap(ctx, documentdb.Name, req.Namespace); err != nil { + logger.Error(err, "Failed to clean up OTel ConfigMap") + return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil + } + if err := r.deleteOtelMonitorSecret(ctx, documentdb.Name, req.Namespace); err != nil { + logger.Error(err, "Failed to clean up OTel monitoring secret") + return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil + } + } + if slices.Contains(currentCnpgCluster.Status.InstancesStatus[cnpgv1.PodHealthy], currentCnpgCluster.Status.CurrentPrimary) && replicationContext.IsPrimary() { // Check if permissions have already been granted checkCommand := "SELECT 1 FROM pg_roles WHERE rolname = 'streaming_replica' AND pg_has_role('streaming_replica', 'documentdb_admin_role', 'USAGE');" diff --git a/operator/src/internal/controller/documentdb_controller_test.go b/operator/src/internal/controller/documentdb_controller_test.go index ec63f0f1d..4ab141c4f 100644 --- a/operator/src/internal/controller/documentdb_controller_test.go +++ b/operator/src/internal/controller/documentdb_controller_test.go @@ -23,6 +23,7 @@ import ( kubefake "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -2924,6 +2925,106 @@ var _ = Describe("DocumentDB Controller", func() { Expect(result.Requeue).To(BeFalse()) }) + It("retains OTel resources when cluster sync fails while disabling monitoring", func() { + Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) + + documentdb := &dbpreview.DocumentDB{ + ObjectMeta: metav1.ObjectMeta{ + Name: documentDBName, + Namespace: documentDBNamespace, + Finalizers: []string{documentDBFinalizer}, + }, + Spec: dbpreview.DocumentDBSpec{ + InstancesPerNode: 1, + Resource: dbpreview.Resource{ + Storage: dbpreview.StorageConfiguration{PvcSize: "1Gi"}, + }, + }, + } + monitorSecretName := documentDBName + "-otel-monitor" + configMapName := documentDBName + "-otel-config" + cnpgCluster := &cnpgv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: documentDBName, + Namespace: documentDBNamespace, + }, + Spec: cnpgv1.ClusterSpec{ + Instances: 1, + PostgresConfiguration: cnpgv1.PostgresConfiguration{ + Extensions: []cnpgv1.ExtensionConfiguration{ + { + Name: "documentdb", + ImageVolumeSource: corev1.ImageVolumeSource{ + Reference: util.DEFAULT_DOCUMENTDB_IMAGE, + }, + }, + }, + }, + Plugins: []cnpgv1.PluginConfiguration{ + { + Name: util.DEFAULT_SIDECAR_INJECTOR_PLUGIN, + Enabled: ptr.To(true), + Parameters: map[string]string{ + "gatewayImage": util.DEFAULT_GATEWAY_IMAGE, + "documentDbCredentialSecret": util.DEFAULT_DOCUMENTDB_CREDENTIALS_SECRET, + "otelCollectorImage": util.DEFAULT_OTEL_COLLECTOR_IMAGE, + "otelConfigMapName": configMapName, + "otelMonitorSecret": monitorSecretName, + }, + }, + }, + Managed: &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{ + { + Name: "otel_monitor", + Ensure: cnpgv1.EnsurePresent, + Login: true, + InRoles: []string{"pg_monitor"}, + PasswordSecret: &cnpgv1.LocalObjectReference{Name: monitorSecretName}, + ConnectionLimit: -1, + Inherit: ptr.To(true), + }, + }, + }, + }, + } + monitorSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: monitorSecretName, Namespace: documentDBNamespace}, + Type: corev1.SecretTypeBasicAuth, + } + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: configMapName, Namespace: documentDBNamespace}, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(documentdb, cnpgCluster, monitorSecret, configMap). + WithStatusSubresource(&dbpreview.DocumentDB{}). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*cnpgv1.Cluster); ok { + return fmt.Errorf("simulated CNPG sync failure") + } + return c.Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + reconciler := &DocumentDBReconciler{ + Client: fakeClient, + Scheme: scheme, + Recorder: recorder, + } + result, err := reconciler.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: documentDBName, Namespace: documentDBNamespace}, + }) + + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(RequeueAfterShort)) + Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, &corev1.Secret{})).To(Succeed()) + Expect(fakeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: documentDBNamespace}, &corev1.ConfigMap{})).To(Succeed()) + }) + It("should add restart annotation when TLS secret name changes", func() { Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) From 146dbdfb13e502a9153a68aa1d3ba2ade4c74ea6 Mon Sep 17 00:00:00 2001 From: urismiley Date: Thu, 9 Jul 2026 17:36:56 -0400 Subject: [PATCH 3/6] test: cover OTel monitoring error paths Copilot-Session: ba406fac-b035-4384-a495-46e2661fafdb Signed-off-by: urismiley --- .../controller/documentdb_controller.go | 7 +- .../controller/documentdb_controller_test.go | 81 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/operator/src/internal/controller/documentdb_controller.go b/operator/src/internal/controller/documentdb_controller.go index 65e8b9617..c693115fb 100644 --- a/operator/src/internal/controller/documentdb_controller.go +++ b/operator/src/internal/controller/documentdb_controller.go @@ -9,6 +9,7 @@ import ( "crypto/rand" "encoding/base64" "fmt" + "io" "slices" "strconv" "strings" @@ -1150,7 +1151,7 @@ func (r *DocumentDBReconciler) reconcileOtelMonitorSecret(ctx context.Context, d // Generate the password only once; preserve it on subsequent reconciles so // the managed role's password and the sidecar's cached env stay in sync. if len(secret.Data[corev1.BasicAuthPasswordKey]) == 0 { - password, genErr := generateRandomPassword() + password, genErr := generateRandomPassword(rand.Reader) if genErr != nil { return fmt.Errorf("failed to generate monitoring password: %w", genErr) } @@ -1190,9 +1191,9 @@ func (r *DocumentDBReconciler) deleteOtelMonitorSecret(ctx context.Context, clus // generateRandomPassword returns a cryptographically random, URL-safe password // suitable for a PostgreSQL role. -func generateRandomPassword() (string, error) { +func generateRandomPassword(reader io.Reader) (string, error) { buf := make([]byte, 24) - if _, err := rand.Read(buf); err != nil { + if _, err := io.ReadFull(reader, buf); err != nil { return "", err } return base64.RawURLEncoding.EncodeToString(buf), nil diff --git a/operator/src/internal/controller/documentdb_controller_test.go b/operator/src/internal/controller/documentdb_controller_test.go index 4ab141c4f..03f892363 100644 --- a/operator/src/internal/controller/documentdb_controller_test.go +++ b/operator/src/internal/controller/documentdb_controller_test.go @@ -3025,6 +3025,50 @@ var _ = Describe("DocumentDB Controller", func() { Expect(fakeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: documentDBNamespace}, &corev1.ConfigMap{})).To(Succeed()) }) + It("requeues when the OTel monitoring secret cannot be created", func() { + Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) + + documentdb := &dbpreview.DocumentDB{ + ObjectMeta: metav1.ObjectMeta{ + Name: documentDBName, + Namespace: documentDBNamespace, + Finalizers: []string{documentDBFinalizer}, + }, + Spec: dbpreview.DocumentDBSpec{ + InstancesPerNode: 1, + Resource: dbpreview.Resource{ + Storage: dbpreview.StorageConfiguration{PvcSize: "1Gi"}, + }, + Monitoring: &dbpreview.MonitoringSpec{Enabled: true}, + }, + } + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(documentdb). + WithStatusSubresource(&dbpreview.DocumentDB{}). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return fmt.Errorf("simulated secret create failure") + } + return c.Create(ctx, obj, opts...) + }, + }). + Build() + reconciler := &DocumentDBReconciler{ + Client: fakeClient, + Scheme: scheme, + Recorder: recorder, + } + + result, err := reconciler.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: documentDBName, Namespace: documentDBNamespace}, + }) + + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(RequeueAfterShort)) + }) + It("should add restart annotation when TLS secret name changes", func() { Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) @@ -3488,6 +3532,19 @@ var _ = Describe("DocumentDB Controller", func() { Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, second)).To(Succeed()) Expect(second.Data[corev1.BasicAuthPasswordKey]).To(Equal(original)) }) + + It("returns an error when the owner reference cannot be set", func() { + documentdb := newDocumentDB() + documentdb.Namespace = "different-namespace" + documentdb.UID = "test-uid" + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + + err := reconciler.reconcileOtelMonitorSecret(ctx, documentdb, documentDBNamespace) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("owner reference")) + }) }) Describe("deleteOtelMonitorSecret", func() { @@ -3512,5 +3569,29 @@ var _ = Describe("DocumentDB Controller", func() { reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} Expect(reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace)).To(Succeed()) }) + + It("returns an error when deletion fails", func() { + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return fmt.Errorf("simulated delete failure") + }, + }). + Build() + reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} + + err := reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to delete OTel monitoring secret")) + }) + }) + + Describe("generateRandomPassword", func() { + It("returns an error when the entropy source fails", func() { + _, err := generateRandomPassword(strings.NewReader("")) + Expect(err).To(HaveOccurred()) + }) }) }) From 2154a59556b3225144a5e5b7a332a0fde87fda33 Mon Sep 17 00:00:00 2001 From: urismiley Date: Thu, 9 Jul 2026 17:42:03 -0400 Subject: [PATCH 4/6] test: update OTel secret RBAC expectation Copilot-Session: ba406fac-b035-4384-a495-46e2661fafdb Signed-off-by: urismiley --- operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml index 0d749fb16..edc8682e8 100644 --- a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml +++ b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml @@ -102,14 +102,14 @@ tests: resources: ["serviceexports", "multiclusterservices", "serviceimports", "internalserviceexports"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - - it: should include secrets permissions (read-only) + - it: should include secrets permissions for OTel credential management asserts: - contains: path: rules content: apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch"] + verbs: ["get", "list", "watch", "create", "update", "patch"] - it: should include CNPG backup permissions asserts: From b7c40d057c96c59ae991a8b72c3d0951b446360c Mon Sep 17 00:00:00 2001 From: urismiley Date: Fri, 10 Jul 2026 12:39:22 -0400 Subject: [PATCH 5/6] fix: allow operator to delete OTel monitoring secrets Copilot-Session: ba406fac-b035-4384-a495-46e2661fafdb Signed-off-by: urismiley --- operator/documentdb-helm-chart/templates/05_clusterrole.yaml | 2 +- operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml | 2 +- operator/src/config/rbac/role.yaml | 1 + operator/src/internal/controller/documentdb_controller.go | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml index 1be829463..2bc157c72 100644 --- a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml +++ b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml @@ -35,7 +35,7 @@ rules: # the per-cluster OTel monitoring credential secret (-otel-monitor). - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch", "create", "update", "patch"] + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - apiGroups: ["postgresql.cnpg.io"] resources: ["clusters", "publications", "subscriptions", "clusters/status"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml index edc8682e8..89524a637 100644 --- a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml +++ b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml @@ -109,7 +109,7 @@ tests: content: apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch", "create", "update", "patch"] + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - it: should include CNPG backup permissions asserts: diff --git a/operator/src/config/rbac/role.yaml b/operator/src/config/rbac/role.yaml index f5f68b821..c5569f7d8 100644 --- a/operator/src/config/rbac/role.yaml +++ b/operator/src/config/rbac/role.yaml @@ -20,6 +20,7 @@ rules: - secrets verbs: - create + - delete - get - list - patch diff --git a/operator/src/internal/controller/documentdb_controller.go b/operator/src/internal/controller/documentdb_controller.go index c693115fb..ed2ad6d4c 100644 --- a/operator/src/internal/controller/documentdb_controller.go +++ b/operator/src/internal/controller/documentdb_controller.go @@ -77,7 +77,7 @@ var reconcileMutex sync.Mutex // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { reconcileMutex.Lock() defer reconcileMutex.Unlock() From d1ccd5a1c9401400ccbf5979b2a2daf9242b6de3 Mon Sep 17 00:00:00 2001 From: urismiley Date: Wed, 22 Jul 2026 11:57:25 -0400 Subject: [PATCH 6/6] fix: address OTel monitoring security feedback Copilot-Session: 7eb76c0f-914f-4e38-9087-393d15fa259d Signed-off-by: urismiley --- .../internal/config/config.go | 26 ++- .../internal/config/config_test.go | 64 ++++++- .../internal/lifecycle/lifecycle.go | 46 ++--- .../internal/lifecycle/lifecycle_test.go | 43 ++--- .../internal/operator/validation_test.go | 2 +- .../templates/05_clusterrole.yaml | 5 +- .../tests/05_clusterrole_test.yaml | 4 +- operator/src/config/rbac/role.yaml | 8 +- operator/src/internal/cnpg/cnpg_cluster.go | 58 ++++--- .../src/internal/cnpg/cnpg_cluster_test.go | 19 +- operator/src/internal/cnpg/cnpg_sync.go | 50 ++++-- operator/src/internal/cnpg/cnpg_sync_test.go | 84 +++++++-- .../controller/documentdb_controller.go | 94 +--------- .../controller/documentdb_controller_test.go | 164 +----------------- .../controller/physical_replication_test.go | 2 +- .../src/internal/controller/pv_controller.go | 4 +- operator/src/internal/otel/base_config.yaml | 2 +- operator/src/internal/otel/config.go | 15 +- operator/src/internal/otel/config_test.go | 11 +- 19 files changed, 265 insertions(+), 436 deletions(-) diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go index 48a803db1..d5d933faa 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config.go @@ -28,7 +28,6 @@ const ( otelCollectorImageParameter = "otelCollectorImage" otelConfigMapNameParameter = "otelConfigMapName" otelConfigHashParameter = "otelConfigHash" - otelMonitorSecretParameter = "otelMonitorSecret" otelMemoryRequestParameter = "otelMemoryRequest" otelMemoryLimitParameter = "otelMemoryLimit" otelCPURequestParameter = "otelCpuRequest" @@ -49,7 +48,7 @@ type Configuration struct { DocumentDbCredentialSecret string OtelCollectorImage string OtelConfigMapName string - OtelMonitorSecret string + OtelConfigHash string OTelMemoryRequest string OTelMemoryLimit string OTelCPURequest string @@ -89,7 +88,6 @@ func FromParameters( pullPolicy := parsePullPolicy(helper.Parameters[gatewayImagePullPolicyParameter]) otelCollectorImage := helper.Parameters[otelCollectorImageParameter] otelConfigMapName := helper.Parameters[otelConfigMapNameParameter] - otelMonitorSecret := helper.Parameters[otelMonitorSecretParameter] validateQuantityParameters(helper, &validationErrors, gatewayMemoryRequestParameter, gatewayMemoryLimitParameter, @@ -117,11 +115,17 @@ func FromParameters( requiredOtelParameters := []string{ otelCollectorImageParameter, otelConfigMapNameParameter, - otelMonitorSecretParameter, } - otelConfigured := helper.Parameters[prometheusPortParameter] != "" || - helper.Parameters[otelConfigHashParameter] != "" - for _, parameter := range requiredOtelParameters { + otelParameters := append([]string{ + prometheusPortParameter, + otelConfigHashParameter, + otelMemoryRequestParameter, + otelMemoryLimitParameter, + otelCPURequestParameter, + otelCPULimitParameter, + }, requiredOtelParameters...) + otelConfigured := false + for _, parameter := range otelParameters { otelConfigured = otelConfigured || helper.Parameters[parameter] != "" } if otelConfigured { @@ -151,7 +155,7 @@ func FromParameters( DocumentDbCredentialSecret: credentialSecret, OtelCollectorImage: otelCollectorImage, OtelConfigMapName: otelConfigMapName, - OtelMonitorSecret: otelMonitorSecret, + OtelConfigHash: helper.Parameters[otelConfigHashParameter], OTelMemoryRequest: helper.Parameters[otelMemoryRequestParameter], OTelMemoryLimit: helper.Parameters[otelMemoryLimitParameter], OTelCPURequest: helper.Parameters[otelCPURequestParameter], @@ -263,10 +267,16 @@ func (config *Configuration) ToParameters() (map[string]string, error) { setIfNotEmpty(gatewayCPURequestParameter, config.GatewayCPURequest) setIfNotEmpty(gatewayCPULimitParameter, config.GatewayCPULimit) result[documentDbCredentialSecretParameter] = config.DocumentDbCredentialSecret + setIfNotEmpty(otelCollectorImageParameter, config.OtelCollectorImage) + setIfNotEmpty(otelConfigMapNameParameter, config.OtelConfigMapName) + setIfNotEmpty(otelConfigHashParameter, config.OtelConfigHash) setIfNotEmpty(otelMemoryRequestParameter, config.OTelMemoryRequest) setIfNotEmpty(otelMemoryLimitParameter, config.OTelMemoryLimit) setIfNotEmpty(otelCPURequestParameter, config.OTelCPURequest) setIfNotEmpty(otelCPULimitParameter, config.OTelCPULimit) + if config.PrometheusPort > 0 { + result[prometheusPortParameter] = strconv.FormatInt(int64(config.PrometheusPort), 10) + } return result, nil } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go index 606d88a86..585e47510 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/config/config_test.go @@ -88,7 +88,7 @@ func TestFromParameters(t *testing.T) { helper := &common.Plugin{Parameters: map[string]string{ "otelCollectorImage": "otel/opentelemetry-collector-contrib:test", "otelConfigMapName": "demo-otel-config", - "otelMonitorSecret": "demo-otel-monitor", + "otelConfigHash": "abc123", }} config, errs := FromParameters(helper) if len(errs) != 0 { @@ -100,8 +100,8 @@ func TestFromParameters(t *testing.T) { if config.OtelConfigMapName != "demo-otel-config" { t.Errorf("OtelConfigMapName = %q", config.OtelConfigMapName) } - if config.OtelMonitorSecret != "demo-otel-monitor" { - t.Errorf("OtelMonitorSecret = %q, want demo-otel-monitor", config.OtelMonitorSecret) + if config.OtelConfigHash != "abc123" { + t.Errorf("OtelConfigHash = %q", config.OtelConfigHash) } }) @@ -111,17 +111,16 @@ func TestFromParameters(t *testing.T) { wantErrors int }{ { - name: "rejects collector image without config map and monitor secret", + name: "rejects collector image without config map", parameters: map[string]string{ "otelCollectorImage": "otel/opentelemetry-collector-contrib:test", }, - wantErrors: 2, + wantErrors: 1, }, { - name: "rejects config map and monitor secret without collector image", + name: "rejects config map without collector image", parameters: map[string]string{ "otelConfigMapName": "demo-otel-config", - "otelMonitorSecret": "demo-otel-monitor", }, wantErrors: 1, }, @@ -130,7 +129,42 @@ func TestFromParameters(t *testing.T) { parameters: map[string]string{ "prometheusPort": "8888", }, - wantErrors: 3, + wantErrors: 2, + }, + { + name: "rejects config hash without required parameters", + parameters: map[string]string{ + "otelConfigHash": "abc123", + }, + wantErrors: 2, + }, + { + name: "rejects memory request without required parameters", + parameters: map[string]string{ + "otelMemoryRequest": "64Mi", + }, + wantErrors: 2, + }, + { + name: "rejects memory limit without required parameters", + parameters: map[string]string{ + "otelMemoryLimit": "128Mi", + }, + wantErrors: 2, + }, + { + name: "rejects CPU request without required parameters", + parameters: map[string]string{ + "otelCpuRequest": "100m", + }, + wantErrors: 2, + }, + { + name: "rejects CPU limit without required parameters", + parameters: map[string]string{ + "otelCpuLimit": "300m", + }, + wantErrors: 2, }, } { t.Run(tt.name, func(t *testing.T) { @@ -147,6 +181,8 @@ func TestFromParameters(t *testing.T) { "gatewayMemoryLimit": "3Gi", "gatewayCpuRequest": "500m", "gatewayCpuLimit": "2", + "otelCollectorImage": "otel:latest", + "otelConfigMapName": "otel-config", "otelMemoryRequest": "64Mi", "otelMemoryLimit": "128Mi", "otelCpuRequest": "100m", @@ -187,6 +223,9 @@ func TestToParametersRoundTrip(t *testing.T) { GatewayMemoryLimit: "3Gi", GatewayCPURequest: "500m", GatewayCPULimit: "2", + OtelCollectorImage: "otel:latest", + OtelConfigMapName: "otel-config", + OtelConfigHash: "abc123", OTelMemoryRequest: "64Mi", OTelMemoryLimit: "128Mi", OTelCPURequest: "100m", @@ -221,6 +260,15 @@ func TestToParametersRoundTrip(t *testing.T) { if restored.GatewayCPULimit != original.GatewayCPULimit { t.Errorf("round-trip gateway cpu limit = %q, want %q", restored.GatewayCPULimit, original.GatewayCPULimit) } + if restored.OtelCollectorImage != original.OtelCollectorImage { + t.Errorf("round-trip OTel collector image = %q, want %q", restored.OtelCollectorImage, original.OtelCollectorImage) + } + if restored.OtelConfigMapName != original.OtelConfigMapName { + t.Errorf("round-trip OTel config map = %q, want %q", restored.OtelConfigMapName, original.OtelConfigMapName) + } + if restored.OtelConfigHash != original.OtelConfigHash { + t.Errorf("round-trip OTel config hash = %q, want %q", restored.OtelConfigHash, original.OtelConfigHash) + } if restored.OTelMemoryRequest != original.OTelMemoryRequest { t.Errorf("round-trip otel memory request = %q, want %q", restored.OTelMemoryRequest, original.OTelMemoryRequest) } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go index 197e3c40f..5bfcfbe09 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle.go @@ -243,11 +243,9 @@ func (impl Implementation) reconcileMetadata( } // Inject OTel Collector sidecar when monitoring is enabled. - // The sidecar is only injected when the operator passes otelCollectorImage, - // otelConfigMapName and otelMonitorSecret parameters (i.e., monitoring.enabled - // is true). otelMonitorSecret is required because the sidecar sources its - // PGUSER/PGPASSWORD from that secret. - if configuration.OtelCollectorImage != "" && configuration.OtelConfigMapName != "" && configuration.OtelMonitorSecret != "" { + // The sidecar is only injected when the operator passes otelCollectorImage + // and otelConfigMapName parameters (i.e., monitoring.enabled is true). + if configuration.OtelCollectorImage != "" && configuration.OtelConfigMapName != "" { log.Printf("Injecting OTel Collector sidecar with image: %s", configuration.OtelCollectorImage) // Add ConfigMap volume for operator-generated config files (static.yaml + dynamic.yaml) @@ -272,7 +270,7 @@ func (impl Implementation) reconcileMetadata( }) } - otelSidecar := newOtelCollectorSidecar(configuration.OtelCollectorImage, configuration.OtelMonitorSecret) + otelSidecar := newOtelCollectorSidecar(configuration.OtelCollectorImage) if resources := buildResources( configuration.OTelCPURequest, configuration.OTelCPULimit, @@ -492,6 +490,7 @@ func injectGatewayOTelEnv(pod *corev1.Pod) { // otelCollectorContainerName is the name of the injected OpenTelemetry // Collector sidecar. const otelCollectorContainerName = "otel-collector" +const otelMonitorRoleName = "otel_monitor" // gatewaySecurityContext returns the SecurityContext for the documentdb-gateway // sidecar: the shared PSA-restricted hardening plus an explicit UID/GID of @@ -508,9 +507,8 @@ func gatewaySecurityContext() *corev1.SecurityContext { // the caller). It carries the shared PSA-restricted SecurityContext without an // explicit UID so the upstream collector image keeps its own baked-in non-root // user (UID 10001); PSA "restricted" only requires runAsNonRoot, not a fixed -// UID. monitorSecret is the operator-managed basic-auth secret holding the -// dedicated least-privilege monitoring role's credentials. -func newOtelCollectorSidecar(image, monitorSecret string) *corev1.Container { +// UID. +func newOtelCollectorSidecar(image string) *corev1.Container { return &corev1.Container{ Name: otelCollectorContainerName, Image: image, @@ -518,11 +516,9 @@ func newOtelCollectorSidecar(image, monitorSecret string) *corev1.Container { "--config=file:/config/static.yaml", "--config=file:/config/dynamic.yaml", }, - // PGUSER and PGPASSWORD are sourced from the operator-managed monitoring - // secret ("-otel-monitor"), which holds the credentials for the - // dedicated least-privilege "otel_monitor" role (member of pg_monitor). - // The OTel Collector's sqlquery receiver uses these credentials to connect - // to PostgreSQL and collect health metrics without application-level access. + // PostgreSQL currently uses trust authentication, so injecting a password + // would not enforce access control. Use the dedicated password-disabled + // identity directly until database authentication is tightened. Env: []corev1.EnvVar{ { Name: "POD_NAME", @@ -533,26 +529,8 @@ func newOtelCollectorSidecar(image, monitorSecret string) *corev1.Container { }, }, { - Name: "PGUSER", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: monitorSecret, - }, - Key: "username", - }, - }, - }, - { - Name: "PGPASSWORD", - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: monitorSecret, - }, - Key: "password", - }, - }, + Name: "PGUSER", + Value: otelMonitorRoleName, }, }, VolumeMounts: []corev1.VolumeMount{ diff --git a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go index ef4d371f0..03b5699f9 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/lifecycle/lifecycle_test.go @@ -144,7 +144,6 @@ func TestLifecycleHookInjectsContainerResourcesAndGoMemLimit(t *testing.T) { "gatewayCpuLimit": "2", "otelCollectorImage": "otel:latest", "otelConfigMapName": "otel-config", - "otelMonitorSecret": "cluster-otel-monitor", "otelMemoryRequest": "64Mi", "otelMemoryLimit": "128Mi", "otelCpuRequest": "100m", @@ -353,7 +352,7 @@ func TestGatewaySecurityContext_PSARestrictedAsUID1000(t *testing.T) { // restricted and, unlike the gateway, does not force a UID so the collector // image keeps its own non-root user (UID 10001). func TestNewOtelCollectorSidecar_Hardened(t *testing.T) { - c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test", "demo-otel-monitor") + c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test") if c.Name != otelCollectorContainerName { t.Fatalf("container name = %q, want %q", c.Name, otelCollectorContainerName) @@ -367,33 +366,21 @@ func TestNewOtelCollectorSidecar_Hardened(t *testing.T) { } } -// TestNewOtelCollectorSidecar_MonitorSecret asserts the sidecar sources its -// PostgreSQL credentials from the dedicated least-privilege monitoring secret -// (not the app secret), so the collector connects as the pg_monitor-only role. -func TestNewOtelCollectorSidecar_MonitorSecret(t *testing.T) { - const secretName = "demo-otel-monitor" - c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test", secretName) - - want := map[string]string{"PGUSER": "username", "PGPASSWORD": "password"} - for envName, key := range want { - var found bool - for _, e := range c.Env { - if e.Name != envName { - continue - } - found = true - if e.ValueFrom == nil || e.ValueFrom.SecretKeyRef == nil { - t.Fatalf("%s must be sourced from a secret key ref", envName) - } - if got := e.ValueFrom.SecretKeyRef.Name; got != secretName { - t.Errorf("%s secret = %q, want %q", envName, got, secretName) - } - if got := e.ValueFrom.SecretKeyRef.Key; got != key { - t.Errorf("%s key = %q, want %q", envName, got, key) - } +// TestNewOtelCollectorSidecar_MonitorUser asserts the sidecar connects using +// the dedicated monitoring identity without injecting an unused password. +func TestNewOtelCollectorSidecar_MonitorUser(t *testing.T) { + c := newOtelCollectorSidecar("otel/opentelemetry-collector-contrib:test") + + for _, env := range c.Env { + if env.Name == "PGPASSWORD" { + t.Fatal("PGPASSWORD must not be injected while PostgreSQL uses trust authentication") } - if !found { - t.Errorf("missing %s env var", envName) + if env.Name == "PGUSER" { + if env.Value != otelMonitorRoleName { + t.Fatalf("PGUSER = %q, want %q", env.Value, otelMonitorRoleName) + } + return } } + t.Fatal("missing PGUSER env var") } diff --git a/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go index bb66519db..0c63b44d2 100644 --- a/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go +++ b/operator/cnpg-plugins/sidecar-injector/internal/operator/validation_test.go @@ -48,7 +48,7 @@ func TestValidateClusterChangeRetainsParameterErrors(t *testing.T) { if err != nil { t.Fatalf("ValidateClusterChange() error: %v", err) } - if got, want := len(result.ValidationErrors), 2; got != want { + if got, want := len(result.ValidationErrors), 1; got != want { t.Fatalf("validation errors = %d, want %d: %v", got, want, result.ValidationErrors) } } diff --git a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml index 2bc157c72..b94fff5b3 100644 --- a/operator/documentdb-helm-chart/templates/05_clusterrole.yaml +++ b/operator/documentdb-helm-chart/templates/05_clusterrole.yaml @@ -31,11 +31,10 @@ rules: resources: ["serviceexports", "multiclusterservices", "serviceimports", "internalserviceexports"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] # Secrets: certificate_controller reads cert-manager-issued TLS secrets to -# stamp into Cluster spec; the DocumentDB controller also generates and manages -# the per-cluster OTel monitoring credential secret (-otel-monitor). +# stamp into Cluster spec. No controller writes Secrets. - apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + verbs: ["get", "list", "watch"] - apiGroups: ["postgresql.cnpg.io"] resources: ["clusters", "publications", "subscriptions", "clusters/status"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml index 89524a637..6512b0623 100644 --- a/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml +++ b/operator/documentdb-helm-chart/tests/05_clusterrole_test.yaml @@ -102,14 +102,14 @@ tests: resources: ["serviceexports", "multiclusterservices", "serviceimports", "internalserviceexports"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - - it: should include secrets permissions for OTel credential management + - it: should include read-only secrets permissions asserts: - contains: path: rules content: apiGroups: [""] resources: ["secrets"] - verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + verbs: ["get", "list", "watch"] - it: should include CNPG backup permissions asserts: diff --git a/operator/src/config/rbac/role.yaml b/operator/src/config/rbac/role.yaml index c5569f7d8..38de72b77 100644 --- a/operator/src/config/rbac/role.yaml +++ b/operator/src/config/rbac/role.yaml @@ -17,10 +17,8 @@ rules: - apiGroups: - "" resources: - - secrets + - persistentvolumes verbs: - - create - - delete - get - list - patch @@ -29,12 +27,10 @@ rules: - apiGroups: - "" resources: - - persistentvolumes + - secrets verbs: - get - list - - patch - - update - watch - apiGroups: - cert-manager.io diff --git a/operator/src/internal/cnpg/cnpg_cluster.go b/operator/src/internal/cnpg/cnpg_cluster.go index 4c4065db3..2ef09a95d 100644 --- a/operator/src/internal/cnpg/cnpg_cluster.go +++ b/operator/src/internal/cnpg/cnpg_cluster.go @@ -101,9 +101,6 @@ func GetCnpgClusterSpec(req ctrl.Request, documentdb *dbpreview.DocumentDB, docu if split.MonitoringEnabled { params["otelCollectorImage"] = util.DEFAULT_OTEL_COLLECTOR_IMAGE params["otelConfigMapName"] = otelcfg.ConfigMapName(documentdb.Name) - // Sidecar sources PGUSER/PGPASSWORD from the dedicated - // least-privilege monitoring secret rather than the app secret. - params["otelMonitorSecret"] = otelcfg.MonitorSecretName(documentdb.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) @@ -376,38 +373,47 @@ func applyIOUringSeccomp(spec *cnpgv1.ClusterSpec, documentdb *dbpreview.Documen } } -// applyOtelMonitorRole declares a dedicated least-privilege PostgreSQL role for -// the OTel Collector sidecar via CNPG's managed-roles reconciler. The role has -// LOGIN and is a member of the built-in pg_monitor role (read access to all -// pg_stat_* views) and nothing else — following the principle of least -// privilege, the monitoring sidecar cannot read or modify application data. +// applyOtelMonitorRole declares the PostgreSQL identity used by the OTel +// Collector sidecar. When monitoring is disabled, the role remains declared +// with ensure=absent so CNPG removes any role left by an earlier configuration. // -// The role password is sourced from the operator-generated basic-auth secret -// (-otel-monitor); CNPG requires the secret's "username" to match the -// role name exactly. No-op when monitoring is disabled, so clusters without -// monitoring keep no managed roles. +// 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 || documentdb.Spec.Monitoring == nil || !documentdb.Spec.Monitoring.Enabled { + if documentdb == nil { return } if spec.Managed == nil { spec.Managed = &cnpgv1.ManagedConfiguration{} } - spec.Managed.Roles = append(spec.Managed.Roles, cnpgv1.RoleConfiguration{ - Name: otelcfg.MonitorRoleName, - Comment: "Least-privilege role for the OTel Collector monitoring sidecar", - Ensure: cnpgv1.EnsurePresent, - Login: true, - InRoles: []string{"pg_monitor"}, - PasswordSecret: &cnpgv1.LocalObjectReference{ - Name: otelcfg.MonitorSecretName(documentdb.Name), - }, - // Set the CNPG/CRD-defaulted fields explicitly so the desired role - // matches the API-server-defaulted form stored on the live cluster, - // keeping SyncCnpgCluster's diff stable (no perpetual re-patching). + role := absentOtelMonitorRole() + if documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled { + // The current health query is SELECT 1, so the role needs LOGIN only + // and is not granted broad monitoring memberships. + role = cnpgv1.RoleConfiguration{ + Name: otelcfg.MonitorRoleName, + Comment: "Dedicated role for the OTel Collector monitoring sidecar", + Ensure: cnpgv1.EnsurePresent, + Login: true, + DisablePassword: true, + // Set the CNPG/CRD-defaulted fields explicitly so the desired role + // matches the API-server-defaulted form stored on the live cluster, + // keeping SyncCnpgCluster's diff stable (no perpetual re-patching). + ConnectionLimit: -1, + Inherit: pointer.Bool(true), + } + } + spec.Managed.Roles = append(spec.Managed.Roles, role) +} + +func absentOtelMonitorRole() cnpgv1.RoleConfiguration { + return cnpgv1.RoleConfiguration{ + Name: otelcfg.MonitorRoleName, + Ensure: cnpgv1.EnsureAbsent, ConnectionLimit: -1, Inherit: pointer.Bool(true), - }) + } } // for the cluster. diff --git a/operator/src/internal/cnpg/cnpg_cluster_test.go b/operator/src/internal/cnpg/cnpg_cluster_test.go index c985aa472..df7167c18 100644 --- a/operator/src/internal/cnpg/cnpg_cluster_test.go +++ b/operator/src/internal/cnpg/cnpg_cluster_test.go @@ -770,7 +770,7 @@ var _ = Describe("GetCnpgClusterSpec", func() { Expect(pluginParams).NotTo(HaveKey("otelConfigMapName")) }) - It("declares the otel_monitor managed role and passes otelMonitorSecret when monitoring is enabled", func() { + It("declares a password-disabled otel_monitor role when monitoring is enabled", func() { req := ctrl.Request{} req.Name = "test-cluster" req.Namespace = "default" @@ -793,24 +793,21 @@ var _ = Describe("GetCnpgClusterSpec", func() { cluster := GetCnpgClusterSpec(req, documentdb, "test-image:latest", "test-sa", "", true, log) - pluginParams := cluster.Spec.Plugins[0].Parameters - Expect(pluginParams).To(HaveKeyWithValue("otelMonitorSecret", "test-cluster-otel-monitor")) - Expect(cluster.Spec.Managed).NotTo(BeNil()) Expect(cluster.Spec.Managed.Roles).To(HaveLen(1)) role := cluster.Spec.Managed.Roles[0] Expect(role.Name).To(Equal("otel_monitor")) Expect(role.Login).To(BeTrue()) Expect(role.Ensure).To(Equal(cnpgv1.EnsurePresent)) - Expect(role.InRoles).To(ConsistOf("pg_monitor")) - Expect(role.PasswordSecret).NotTo(BeNil()) - Expect(role.PasswordSecret.Name).To(Equal("test-cluster-otel-monitor")) + Expect(role.InRoles).To(BeEmpty()) + Expect(role.PasswordSecret).To(BeNil()) + Expect(role.DisablePassword).To(BeTrue()) Expect(role.Superuser).To(BeFalse()) Expect(role.CreateDB).To(BeFalse()) Expect(role.CreateRole).To(BeFalse()) }) - It("does not declare managed roles when monitoring is disabled", func() { + It("declares the otel_monitor role absent when monitoring is disabled", func() { req := ctrl.Request{} req.Name = "test-cluster" req.Namespace = "default" @@ -828,10 +825,8 @@ var _ = Describe("GetCnpgClusterSpec", func() { cluster := GetCnpgClusterSpec(req, documentdb, "test-image:latest", "test-sa", "", true, log) - Expect(cluster.Spec.Plugins[0].Parameters).NotTo(HaveKey("otelMonitorSecret")) - if cluster.Spec.Managed != nil { - Expect(cluster.Spec.Managed.Roles).To(BeEmpty()) - } + Expect(cluster.Spec.Managed).NotTo(BeNil()) + Expect(cluster.Spec.Managed.Roles).To(Equal([]cnpgv1.RoleConfiguration{absentOtelMonitorRole()})) }) It("propagates spec.imagePullSecrets to the CNPG cluster spec", func() { diff --git a/operator/src/internal/cnpg/cnpg_sync.go b/operator/src/internal/cnpg/cnpg_sync.go index 0dd64793a..899676b03 100644 --- a/operator/src/internal/cnpg/cnpg_sync.go +++ b/operator/src/internal/cnpg/cnpg_sync.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" + otelcfg "github.com/documentdb/documentdb-operator/internal/otel" util "github.com/documentdb/documentdb-operator/internal/utils" ) @@ -115,7 +116,6 @@ func SyncCnpgCluster( "otelConfigMapName", "prometheusPort", "otelConfigHash", - "otelMonitorSecret", util.PLUGIN_PARAM_OTEL_MEMORY_REQUEST, util.PLUGIN_PARAM_OTEL_MEMORY_LIMIT, util.PLUGIN_PARAM_OTEL_CPU_REQUEST, @@ -325,36 +325,54 @@ func managedRoles(cluster *cnpgv1.Cluster) []cnpgv1.RoleConfiguration { } // managedRolesPatch returns the JSON Patch operation needed to reconcile -// spec.managed.roles. Removing the field when no roles are desired avoids -// assigning JSON null to the CRD array field, which Kubernetes validation can -// reject. Other managed configuration, such as services, remains untouched. +// spec.managed.roles. Only the operator-owned OTel role is changed; unrelated +// managed roles and managed services remain untouched. func managedRolesPatch(current, desired *cnpgv1.Cluster) *JSONPatch { currentRoles := managedRoles(current) desiredRoles := managedRoles(desired) - // Treat nil and empty role lists as equivalent. - if len(currentRoles) == 0 && len(desiredRoles) == 0 { - return nil - } - if reflect.DeepEqual(currentRoles, desiredRoles) { - return nil + desiredRole := absentOtelMonitorRole() + for _, role := range desiredRoles { + if role.Name == otelcfg.MonitorRoleName { + desiredRole = role + break + } } - if len(desiredRoles) == 0 { - return &JSONPatch{ - Op: PatchOpRemove, - Path: PatchPathManagedRoles, + + roles := make([]cnpgv1.RoleConfiguration, 0, len(currentRoles)+1) + found := false + for _, role := range currentRoles { + if role.Name == otelcfg.MonitorRoleName { + if !found { + roles = append(roles, desiredRole) + found = true + } + continue } + roles = append(roles, role) + } + if !found { + roles = append(roles, desiredRole) + } + + if reflect.DeepEqual(currentRoles, roles) { + return nil } if current.Spec.Managed == nil { + managed := cnpgv1.ManagedConfiguration{} + if desired.Spec.Managed != nil { + managed = *desired.Spec.Managed + } + managed.Roles = roles return &JSONPatch{ Op: PatchOpAdd, Path: PatchPathManaged, - Value: desired.Spec.Managed, + Value: &managed, } } return &JSONPatch{ Op: PatchOpAdd, Path: PatchPathManagedRoles, - Value: desiredRoles, + Value: roles, } } diff --git a/operator/src/internal/cnpg/cnpg_sync_test.go b/operator/src/internal/cnpg/cnpg_sync_test.go index 9769f5c01..a04750a09 100644 --- a/operator/src/internal/cnpg/cnpg_sync_test.go +++ b/operator/src/internal/cnpg/cnpg_sync_test.go @@ -528,13 +528,20 @@ var _ = Describe("SyncCnpgCluster - mutable spec fields", func() { var _ = Describe("SyncCnpgCluster - managed roles", func() { const namespace = "test-ns" - otelRole := func(secret string) cnpgv1.RoleConfiguration { + otelRole := func() cnpgv1.RoleConfiguration { return cnpgv1.RoleConfiguration{ Name: "otel_monitor", Ensure: cnpgv1.EnsurePresent, Login: true, - InRoles: []string{"pg_monitor"}, - PasswordSecret: &cnpgv1.LocalObjectReference{Name: secret}, + DisablePassword: true, + ConnectionLimit: -1, + Inherit: pointer.Bool(true), + } + } + absentOtelRole := func() cnpgv1.RoleConfiguration { + return cnpgv1.RoleConfiguration{ + Name: "otel_monitor", + Ensure: cnpgv1.EnsureAbsent, ConnectionLimit: -1, Inherit: pointer.Bool(true), } @@ -545,7 +552,7 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { Expect(current.Spec.Managed).To(BeNil()) desired := current.DeepCopy() desired.Spec.Managed = &cnpgv1.ManagedConfiguration{ - Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + Roles: []cnpgv1.RoleConfiguration{otelRole()}, } c := buildFakeClient(current).Build() @@ -556,29 +563,71 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { Expect(updated.Spec.Managed).NotTo(BeNil()) Expect(updated.Spec.Managed.Roles).To(HaveLen(1)) Expect(updated.Spec.Managed.Roles[0].Name).To(Equal("otel_monitor")) - Expect(updated.Spec.Managed.Roles[0].PasswordSecret.Name).To(Equal("test-cluster-otel-monitor")) + Expect(updated.Spec.Managed.Roles[0].DisablePassword).To(BeTrue()) }) - It("removes the managed role when monitoring is disabled", func() { + It("marks the managed role absent when monitoring is disabled", func() { current := baseCluster("test-cluster", namespace) current.Spec.Managed = &cnpgv1.ManagedConfiguration{ - Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + Roles: []cnpgv1.RoleConfiguration{otelRole()}, } desired := current.DeepCopy() - desired.Spec.Managed = nil + desired.Spec.Managed.Roles = []cnpgv1.RoleConfiguration{absentOtelRole()} patch := managedRolesPatch(current, desired) Expect(patch).NotTo(BeNil()) - Expect(patch.Op).To(Equal(PatchOpRemove)) + Expect(patch.Op).To(Equal(PatchOpAdd)) Expect(patch.Path).To(Equal(PatchPathManagedRoles)) - Expect(patch.Value).To(BeNil()) + Expect(patch.Value).To(Equal([]cnpgv1.RoleConfiguration{absentOtelRole()})) + + c := buildFakeClient(current).Build() + Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) + + updated := &cnpgv1.Cluster{} + Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) + Expect(managedRoles(updated)).To(Equal([]cnpgv1.RoleConfiguration{absentOtelRole()})) + }) + + It("keeps the absent role declaration until monitoring is re-enabled", func() { + current := baseCluster("test-cluster", namespace) + current.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{absentOtelRole()}, + } + desired := current.DeepCopy() + + Expect(managedRolesPatch(current, desired)).To(BeNil()) + }) + + It("adds an absent declaration to clean up an orphaned database role", func() { + current := baseCluster("test-cluster", namespace) + desired := current.DeepCopy() + desired.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{absentOtelRole()}, + } c := buildFakeClient(current).Build() Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) updated := &cnpgv1.Cluster{} Expect(c.Get(context.Background(), types.NamespacedName{Name: "test-cluster", Namespace: namespace}, updated)).To(Succeed()) - Expect(managedRoles(updated)).To(BeEmpty()) + Expect(updated.Spec.Managed.Roles).To(Equal([]cnpgv1.RoleConfiguration{absentOtelRole()})) + }) + + It("preserves other managed roles when marking the monitor role absent", func() { + current := baseCluster("test-cluster", namespace) + customRole := cnpgv1.RoleConfiguration{Name: "custom_role", Login: true} + current.Spec.Managed = &cnpgv1.ManagedConfiguration{ + Roles: []cnpgv1.RoleConfiguration{customRole, otelRole()}, + } + desired := current.DeepCopy() + desired.Spec.Managed.Roles = []cnpgv1.RoleConfiguration{absentOtelRole()} + + patch := managedRolesPatch(current, desired) + Expect(patch).NotTo(BeNil()) + Expect(patch.Value).To(Equal([]cnpgv1.RoleConfiguration{ + customRole, + absentOtelRole(), + })) }) It("preserves managed.services when patching only roles", func() { @@ -596,7 +645,7 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { }, } desired := current.DeepCopy() - desired.Spec.Managed.Roles = []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")} + desired.Spec.Managed.Roles = []cnpgv1.RoleConfiguration{otelRole()} c := buildFakeClient(current).Build() Expect(SyncCnpgCluster(context.Background(), c, current, desired, nil)).To(Succeed()) @@ -612,7 +661,7 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { It("does not patch when managed roles are unchanged", func() { current := baseCluster("test-cluster", namespace) current.Spec.Managed = &cnpgv1.ManagedConfiguration{ - Roles: []cnpgv1.RoleConfiguration{otelRole("test-cluster-otel-monitor")}, + Roles: []cnpgv1.RoleConfiguration{otelRole()}, } desired := current.DeepCopy() @@ -626,14 +675,13 @@ var _ = Describe("SyncCnpgCluster - managed roles", func() { Expect(updated.Annotations).ToNot(HaveKey("kubectl.kubernetes.io/restartedAt")) }) - It("treats nil and empty managed role lists as equivalent", func() { + It("defaults a missing desired monitor role to absent", func() { current := baseCluster("test-cluster", namespace) desired := current.DeepCopy() - desired.Spec.Managed = &cnpgv1.ManagedConfiguration{ - Roles: []cnpgv1.RoleConfiguration{}, - } - Expect(managedRolesPatch(current, desired)).To(BeNil()) + patch := managedRolesPatch(current, desired) + Expect(patch).NotTo(BeNil()) + Expect(patch.Path).To(Equal(PatchPathManaged)) }) }) diff --git a/operator/src/internal/controller/documentdb_controller.go b/operator/src/internal/controller/documentdb_controller.go index ed2ad6d4c..45f8e4ac8 100644 --- a/operator/src/internal/controller/documentdb_controller.go +++ b/operator/src/internal/controller/documentdb_controller.go @@ -6,10 +6,7 @@ package controller import ( "bytes" "context" - "crypto/rand" - "encoding/base64" "fmt" - "io" "slices" "strconv" "strings" @@ -77,7 +74,6 @@ var reconcileMutex sync.Mutex // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { reconcileMutex.Lock() defer reconcileMutex.Unlock() @@ -183,10 +179,6 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) // and CNPG manages the pod rollout. monitoringEnabled := documentdb.Spec.Monitoring != nil && documentdb.Spec.Monitoring.Enabled if monitoringEnabled { - if err := r.reconcileOtelMonitorSecret(ctx, documentdb, req.Namespace); err != nil { - logger.Error(err, "Failed to reconcile OTel monitoring secret") - return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil - } if err := r.reconcileOtelConfigMap(ctx, documentdb, req.Namespace); err != nil { logger.Error(err, "Failed to reconcile OTel ConfigMap") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil @@ -222,19 +214,14 @@ func (r *DocumentDBReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil } - // Delete the OTel resources only after the CNPG Cluster spec no longer - // references them. If cluster sync fails, retaining these objects keeps the - // existing managed role and sidecar configuration functional and allows a - // subsequent reconcile to retry safely. + // Delete the OTel ConfigMap only after the CNPG Cluster spec no longer + // references it. If cluster sync fails, retaining it keeps the existing + // sidecar configuration functional and allows a subsequent reconcile to retry. if !monitoringEnabled { if err := r.deleteOtelConfigMap(ctx, documentdb.Name, req.Namespace); err != nil { logger.Error(err, "Failed to clean up OTel ConfigMap") return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil } - if err := r.deleteOtelMonitorSecret(ctx, documentdb.Name, req.Namespace); err != nil { - logger.Error(err, "Failed to clean up OTel monitoring secret") - return ctrl.Result{RequeueAfter: RequeueAfterShort}, nil - } } if slices.Contains(currentCnpgCluster.Status.InstancesStatus[cnpgv1.PodHealthy], currentCnpgCluster.Status.CurrentPrimary) && replicationContext.IsPrimary() { @@ -1123,78 +1110,3 @@ func (r *DocumentDBReconciler) deleteOtelConfigMap(ctx context.Context, clusterN logger.Info("OTel ConfigMap deleted", "name", cmName) return nil } - -// reconcileOtelMonitorSecret ensures the dedicated basic-auth Secret holding the -// least-privilege OTel monitoring role's credentials exists. CNPG consumes it as -// the managed role's passwordSecret and the OTel Collector sidecar sources -// PGUSER/PGPASSWORD from it. The password is generated once and preserved across -// reconciles so it stays stable for the managed role and the running sidecar. -func (r *DocumentDBReconciler) reconcileOtelMonitorSecret(ctx context.Context, documentdb *dbpreview.DocumentDB, namespace string) error { - logger := log.FromContext(ctx) - secretName := otelcfg.MonitorSecretName(documentdb.Name) - - secret := &corev1.Secret{} - secret.Name = secretName - secret.Namespace = namespace - - result, err := controllerutil.CreateOrUpdate(ctx, r.Client, secret, func() error { - // Owner reference so the Secret is garbage-collected with the DocumentDB CR. - if err := controllerutil.SetControllerReference(documentdb, secret, r.Scheme); err != nil { - return fmt.Errorf("failed to set owner reference: %w", err) - } - secret.Type = corev1.SecretTypeBasicAuth - if secret.Data == nil { - secret.Data = map[string][]byte{} - } - // CNPG requires the secret username to match the managed role name exactly. - secret.Data[corev1.BasicAuthUsernameKey] = []byte(otelcfg.MonitorRoleName) - // Generate the password only once; preserve it on subsequent reconciles so - // the managed role's password and the sidecar's cached env stay in sync. - if len(secret.Data[corev1.BasicAuthPasswordKey]) == 0 { - password, genErr := generateRandomPassword(rand.Reader) - if genErr != nil { - return fmt.Errorf("failed to generate monitoring password: %w", genErr) - } - secret.Data[corev1.BasicAuthPasswordKey] = []byte(password) - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to reconcile OTel monitoring secret %s: %w", secretName, err) - } - if result != controllerutil.OperationResultNone { - logger.Info("OTel monitoring secret reconciled", "name", secretName, "operation", result) - } - return nil -} - -// deleteOtelMonitorSecret removes the OTel monitoring Secret when monitoring is -// no longer configured. -func (r *DocumentDBReconciler) deleteOtelMonitorSecret(ctx context.Context, clusterName, namespace string) error { - logger := log.FromContext(ctx) - secretName := otelcfg.MonitorSecretName(clusterName) - - secret := &corev1.Secret{} - secret.Name = secretName - secret.Namespace = namespace - - err := r.Client.Delete(ctx, secret) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return fmt.Errorf("failed to delete OTel monitoring secret %s: %w", secretName, err) - } - logger.Info("OTel monitoring secret deleted", "name", secretName) - return nil -} - -// generateRandomPassword returns a cryptographically random, URL-safe password -// suitable for a PostgreSQL role. -func generateRandomPassword(reader io.Reader) (string, error) { - buf := make([]byte, 24) - if _, err := io.ReadFull(reader, buf); err != nil { - return "", err - } - return base64.RawURLEncoding.EncodeToString(buf), nil -} diff --git a/operator/src/internal/controller/documentdb_controller_test.go b/operator/src/internal/controller/documentdb_controller_test.go index 03f892363..c77531fa3 100644 --- a/operator/src/internal/controller/documentdb_controller_test.go +++ b/operator/src/internal/controller/documentdb_controller_test.go @@ -2925,7 +2925,7 @@ var _ = Describe("DocumentDB Controller", func() { Expect(result.Requeue).To(BeFalse()) }) - It("retains OTel resources when cluster sync fails while disabling monitoring", func() { + It("retains the OTel ConfigMap when cluster sync fails while disabling monitoring", func() { Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) documentdb := &dbpreview.DocumentDB{ @@ -2941,7 +2941,6 @@ var _ = Describe("DocumentDB Controller", func() { }, }, } - monitorSecretName := documentDBName + "-otel-monitor" configMapName := documentDBName + "-otel-config" cnpgCluster := &cnpgv1.Cluster{ ObjectMeta: metav1.ObjectMeta{ @@ -2969,7 +2968,6 @@ var _ = Describe("DocumentDB Controller", func() { "documentDbCredentialSecret": util.DEFAULT_DOCUMENTDB_CREDENTIALS_SECRET, "otelCollectorImage": util.DEFAULT_OTEL_COLLECTOR_IMAGE, "otelConfigMapName": configMapName, - "otelMonitorSecret": monitorSecretName, }, }, }, @@ -2979,8 +2977,7 @@ var _ = Describe("DocumentDB Controller", func() { Name: "otel_monitor", Ensure: cnpgv1.EnsurePresent, Login: true, - InRoles: []string{"pg_monitor"}, - PasswordSecret: &cnpgv1.LocalObjectReference{Name: monitorSecretName}, + DisablePassword: true, ConnectionLimit: -1, Inherit: ptr.To(true), }, @@ -2988,17 +2985,13 @@ var _ = Describe("DocumentDB Controller", func() { }, }, } - monitorSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: monitorSecretName, Namespace: documentDBNamespace}, - Type: corev1.SecretTypeBasicAuth, - } configMap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: configMapName, Namespace: documentDBNamespace}, } fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(documentdb, cnpgCluster, monitorSecret, configMap). + WithObjects(documentdb, cnpgCluster, configMap). WithStatusSubresource(&dbpreview.DocumentDB{}). WithInterceptorFuncs(interceptor.Funcs{ Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { @@ -3021,54 +3014,9 @@ var _ = Describe("DocumentDB Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.RequeueAfter).To(Equal(RequeueAfterShort)) - Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, &corev1.Secret{})).To(Succeed()) Expect(fakeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: documentDBNamespace}, &corev1.ConfigMap{})).To(Succeed()) }) - It("requeues when the OTel monitoring secret cannot be created", func() { - Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) - - documentdb := &dbpreview.DocumentDB{ - ObjectMeta: metav1.ObjectMeta{ - Name: documentDBName, - Namespace: documentDBNamespace, - Finalizers: []string{documentDBFinalizer}, - }, - Spec: dbpreview.DocumentDBSpec{ - InstancesPerNode: 1, - Resource: dbpreview.Resource{ - Storage: dbpreview.StorageConfiguration{PvcSize: "1Gi"}, - }, - Monitoring: &dbpreview.MonitoringSpec{Enabled: true}, - }, - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(documentdb). - WithStatusSubresource(&dbpreview.DocumentDB{}). - WithInterceptorFuncs(interceptor.Funcs{ - Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { - if _, ok := obj.(*corev1.Secret); ok { - return fmt.Errorf("simulated secret create failure") - } - return c.Create(ctx, obj, opts...) - }, - }). - Build() - reconciler := &DocumentDBReconciler{ - Client: fakeClient, - Scheme: scheme, - Recorder: recorder, - } - - result, err := reconciler.Reconcile(ctx, ctrl.Request{ - NamespacedName: types.NamespacedName{Name: documentDBName, Namespace: documentDBNamespace}, - }) - - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(RequeueAfterShort)) - }) - It("should add restart annotation when TLS secret name changes", func() { Expect(rbacv1.AddToScheme(scheme)).To(Succeed()) @@ -3488,110 +3436,4 @@ var _ = Describe("DocumentDB Controller", func() { }) }) - Describe("reconcileOtelMonitorSecret", func() { - monitorSecretName := documentDBName + "-otel-monitor" - - newDocumentDB := func() *dbpreview.DocumentDB { - return &dbpreview.DocumentDB{ - ObjectMeta: metav1.ObjectMeta{ - Name: documentDBName, - Namespace: documentDBNamespace, - }, - Spec: dbpreview.DocumentDBSpec{ - Monitoring: &dbpreview.MonitoringSpec{Enabled: true}, - }, - } - } - - It("creates a basic-auth secret with the otel_monitor username and a generated password", func() { - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - - Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) - - secret := &corev1.Secret{} - Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, secret)).To(Succeed()) - Expect(secret.Type).To(Equal(corev1.SecretTypeBasicAuth)) - Expect(string(secret.Data[corev1.BasicAuthUsernameKey])).To(Equal("otel_monitor")) - Expect(secret.Data[corev1.BasicAuthPasswordKey]).ToNot(BeEmpty()) - Expect(secret.OwnerReferences).To(HaveLen(1)) - Expect(secret.OwnerReferences[0].Name).To(Equal(documentDBName)) - }) - - It("preserves the existing password across reconciles", func() { - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - - Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) - first := &corev1.Secret{} - Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, first)).To(Succeed()) - original := append([]byte(nil), first.Data[corev1.BasicAuthPasswordKey]...) - - Expect(reconciler.reconcileOtelMonitorSecret(ctx, newDocumentDB(), documentDBNamespace)).To(Succeed()) - second := &corev1.Secret{} - Expect(fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, second)).To(Succeed()) - Expect(second.Data[corev1.BasicAuthPasswordKey]).To(Equal(original)) - }) - - It("returns an error when the owner reference cannot be set", func() { - documentdb := newDocumentDB() - documentdb.Namespace = "different-namespace" - documentdb.UID = "test-uid" - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - - err := reconciler.reconcileOtelMonitorSecret(ctx, documentdb, documentDBNamespace) - - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("owner reference")) - }) - }) - - Describe("deleteOtelMonitorSecret", func() { - monitorSecretName := documentDBName + "-otel-monitor" - - It("deletes the monitoring secret when present", func() { - existing := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: monitorSecretName, Namespace: documentDBNamespace}, - Type: corev1.SecretTypeBasicAuth, - } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - - Expect(reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace)).To(Succeed()) - secret := &corev1.Secret{} - err := fakeClient.Get(ctx, types.NamespacedName{Name: monitorSecretName, Namespace: documentDBNamespace}, secret) - Expect(errors.IsNotFound(err)).To(BeTrue()) - }) - - It("is a no-op when the monitoring secret does not exist", func() { - fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - Expect(reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace)).To(Succeed()) - }) - - It("returns an error when deletion fails", func() { - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithInterceptorFuncs(interceptor.Funcs{ - Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { - return fmt.Errorf("simulated delete failure") - }, - }). - Build() - reconciler := &DocumentDBReconciler{Client: fakeClient, Scheme: scheme} - - err := reconciler.deleteOtelMonitorSecret(ctx, documentDBName, documentDBNamespace) - - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to delete OTel monitoring secret")) - }) - }) - - Describe("generateRandomPassword", func() { - It("returns an error when the entropy source fails", func() { - _, err := generateRandomPassword(strings.NewReader("")) - Expect(err).To(HaveOccurred()) - }) - }) }) diff --git a/operator/src/internal/controller/physical_replication_test.go b/operator/src/internal/controller/physical_replication_test.go index 87a2e0f54..b0c4a6344 100644 --- a/operator/src/internal/controller/physical_replication_test.go +++ b/operator/src/internal/controller/physical_replication_test.go @@ -682,7 +682,7 @@ var _ = Describe("addAzureFleetManagedServices", func() { Spec: cnpgv1.ClusterSpec{ Managed: &cnpgv1.ManagedConfiguration{ Roles: []cnpgv1.RoleConfiguration{ - {Name: "otel_monitor", Login: true, InRoles: []string{"pg_monitor"}}, + {Name: "otel_monitor", Login: true, DisablePassword: true}, }, }, }, diff --git a/operator/src/internal/controller/pv_controller.go b/operator/src/internal/controller/pv_controller.go index 7b23feca0..fda27942b 100644 --- a/operator/src/internal/controller/pv_controller.go +++ b/operator/src/internal/controller/pv_controller.go @@ -72,8 +72,10 @@ type PersistentVolumeReconciler struct { client.Client } +// Package-level RBAC also covers PVC lifecycle operations performed by the +// DocumentDB reconciler. // +kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups=storage.k8s.io,resources=storageclasses,verbs=get;list;watch func (r *PersistentVolumeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { diff --git a/operator/src/internal/otel/base_config.yaml b/operator/src/internal/otel/base_config.yaml index 0b82af385..1ef124baf 100644 --- a/operator/src/internal/otel/base_config.yaml +++ b/operator/src/internal/otel/base_config.yaml @@ -7,7 +7,7 @@ receivers: # no Go code changes needed. sqlquery: driver: postgres - datasource: "host=localhost port=5432 user=${env:PGUSER} password=${env:PGPASSWORD} dbname=postgres sslmode=disable" + datasource: "host=localhost port=5432 user=${env:PGUSER} dbname=postgres sslmode=disable" collection_interval: "30s" queries: - sql: "SELECT 1 as up" diff --git a/operator/src/internal/otel/config.go b/operator/src/internal/otel/config.go index 473b4bb90..e87eb7e65 100644 --- a/operator/src/internal/otel/config.go +++ b/operator/src/internal/otel/config.go @@ -19,21 +19,10 @@ var baseConfigYAML []byte const defaultPrometheusPort = 8888 -// MonitorRoleName is the dedicated least-privilege PostgreSQL role the OTel -// Collector sidecar uses to run health-check queries and read pg_stat_* views. -// It is granted membership in the built-in pg_monitor role and nothing else. -// CNPG's managed-roles reconciler requires the username stored in the password -// secret to match this name exactly. +// MonitorRoleName is the dedicated PostgreSQL identity the OTel Collector +// sidecar uses for its health-check query. const MonitorRoleName = "otel_monitor" -// MonitorSecretName returns the name of the basic-auth Secret holding the OTel -// monitoring role's credentials for a given DocumentDB cluster. The operator -// generates and owns this Secret; CNPG reads it (as the managed role's -// passwordSecret) and the sidecar sources PGUSER/PGPASSWORD from it. -func MonitorSecretName(clusterName string) string { - return fmt.Sprintf("%s-otel-monitor", clusterName) -} - // collectorConfig represents the OTel Collector configuration structure. type collectorConfig struct { Receivers map[string]any `yaml:"receivers,omitempty"` diff --git a/operator/src/internal/otel/config_test.go b/operator/src/internal/otel/config_test.go index 0433b5be7..0032c3312 100644 --- a/operator/src/internal/otel/config_test.go +++ b/operator/src/internal/otel/config_test.go @@ -25,12 +25,6 @@ var _ = Describe("ConfigMapName", func() { }) }) -var _ = Describe("MonitorSecretName", func() { - It("returns the expected monitoring secret name", func() { - Expect(MonitorSecretName("my-cluster")).To(Equal("my-cluster-otel-monitor")) - }) -}) - var _ = Describe("MonitorRoleName", func() { It("is the dedicated least-privilege monitoring role", func() { Expect(MonitorRoleName).To(Equal("otel_monitor")) @@ -56,6 +50,11 @@ var _ = Describe("base_config.yaml embed", func() { Expect(cfg.Exporters).To(BeEmpty()) }) + It("does not configure a password while PostgreSQL uses trust authentication", func() { + Expect(string(baseConfigYAML)).To(ContainSubstring("user=${env:PGUSER}")) + Expect(string(baseConfigYAML)).NotTo(ContainSubstring("PGPASSWORD")) + }) + It("declares a cgroup-aware memory_limiter processor", func() { var cfg collectorConfig Expect(yaml.Unmarshal(baseConfigYAML, &cfg)).To(Succeed())