SCF-857: Skipping owner reference when enable_secret_deletion is false

docs/administrator.md

@@ -281,11 +281,12 @@ will differ and trigger a rolling update of the pods.
 ## Owner References and Finalizers
 
 The Postgres Operator can set [owner references](https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/) to most of a cluster's child resources to improve
-monitoring with GitOps tools and enable cascading deletes. There are two
+monitoring with GitOps tools and enable cascading deletes. There are three
 exceptions:
 
 * Persistent Volume Claims, because they are handled by the [PV Reclaim Policy]https://kubernetes.io/docs/tasks/administer-cluster/change-pv-reclaim-policy/ of the Stateful Set
 * Cross-namespace secrets, because owner references are not allowed across namespaces by design
+* User-credential secrets when [`enable_secrets_deletion`](reference/operator_parameters.md#enable_secrets_deletion) is `false`, so Kubernetes garbage collection does not cascade-delete them after the Postgresql resource is removed (the `enable_secrets_deletion` flag alone only suppresses the operator's own delete path, not K8s GC)
 
 The operator would clean these resources up with its regular delete loop
 unless they got synced correctly. If for some reason the initial cluster sync

docs/reference/operator_parameters.md

@@ -298,8 +298,12 @@ configuration they are grouped under the `kubernetes` key.
 * **enable_owner_references**
   The operator can set owner references on its child resources (except PVCs,
   Patroni config service/endpoint, cross-namespace secrets) to improve cluster
-  monitoring and enable cascading deletion. The default is `false`. Warning,
-  enabling this option disables configured delete protection checks (see below).
+  monitoring and enable cascading deletion. User-credential secrets are also
+  excluded from controller owner references whenever
+  [enable_secrets_deletion](#enable_secrets_deletion) is `false`, so that
+  Kubernetes garbage collection does not cascade-delete them when the
+  Postgresql resource is removed. The default is `false`. Warning, enabling
+  this option disables configured delete protection checks (see below).
 
 * **delete_annotation_date_key**
   key name for annotation that compares manifest value with current date in the
@@ -386,7 +390,15 @@ configuration they are grouped under the `kubernetes` key.
 
 * **enable_secrets_deletion**
   By default, the operator deletes secrets when removing the Postgres cluster
-  manifest. To keep secrets, set this option to `false`. The default is `true`.
+  manifest. To keep secrets, set this option to `false`. Note that this only
+  guards the operator's own deletion logic; Kubernetes garbage collection can
+  still remove user-credential secrets when
+  [enable_owner_references](#enable_owner_references) is `true` because the
+  Postgresql resource acts as a controller owner. To prevent that, the
+  operator skips the controller owner reference on user-credential secrets
+  whenever `enable_secrets_deletion` is `false`, so the two settings work
+  together. This protection takes effect on the cluster's next sync after
+  the setting is applied. The default is `true`.
 
 * **enable_persistent_volume_claim_deletion**
   By default, the operator deletes persistent volume claims when removing the

pkg/cluster/k8sres.go

@@ -1942,9 +1942,15 @@ func (c *Cluster) generateSingleUserSecret(pgUser spec.PgUser) *v1.Secret {
 		lbls = c.connectionPoolerLabels("", false).MatchLabels
 	}
 
-	// if secret lives in another namespace we cannot set ownerReferences
+	// Skip a controller ownerReference on user-credential secrets when the
+	// operator is configured to keep them (enable_secrets_deletion=false);
+	// otherwise Kubernetes garbage collection would still cascade-delete them
+	// once the owning Postgresql CR is removed, defeating that setting.
+	// Cross-namespace secrets also have no ownerReference because K8s forbids
+	// cross-namespace ownerRefs by design.
 	var ownerReferences []metav1.OwnerReference
-	if c.Config.OpConfig.EnableCrossNamespaceSecret && c.Postgresql.ObjectMeta.Namespace != pgUser.Namespace {
+	secretsDeletionDisabled := c.OpConfig.EnableSecretsDeletion != nil && !*c.OpConfig.EnableSecretsDeletion
+	if secretsDeletionDisabled || (c.Config.OpConfig.EnableCrossNamespaceSecret && c.Postgresql.ObjectMeta.Namespace != pgUser.Namespace) {
 		ownerReferences = nil
 	} else {
 		ownerReferences = c.ownerReferences()

pkg/cluster/k8sres_test.go

@@ -2819,6 +2819,134 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 	}
 }
 
+func TestGenerateSingleUserSecret_OwnerReferences(t *testing.T) {
+	testName := "Test generateSingleUserSecret owner references"
+
+	newCluster := func(ownerRefs, secretsDeletion *bool, crossNamespaceSecret bool) *Cluster {
+		cfg := Config{
+			OpConfig: config.Config{
+				Resources: config.Resources{
+					ClusterNameLabel:      "cluster-name",
+					PodRoleLabel:          "spilo-role",
+					EnableOwnerReferences: ownerRefs,
+				},
+				EnableSecretsDeletion:      secretsDeletion,
+				EnableCrossNamespaceSecret: crossNamespaceSecret,
+			},
+		}
+		pg := acidv1.Postgresql{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "myapp-database",
+				Namespace: "myapp",
+				UID:       types.UID("myapp-database-uid"),
+			},
+			Spec: acidv1.PostgresSpec{TeamID: "myapp", NumberOfInstances: 1},
+		}
+		return New(cfg, k8sutil.KubernetesClient{}, pg, logger, eventRecorder)
+	}
+
+	newPgUser := func(namespace string) spec.PgUser {
+		return spec.PgUser{
+			Name:      "app_user",
+			Namespace: namespace,
+			Password:  "secret",
+		}
+	}
+
+	hasControllerOwnerRef := func(cluster *Cluster) func(*v1.Secret) error {
+		return func(secret *v1.Secret) error {
+			for _, ref := range secret.OwnerReferences {
+				if ref.UID == cluster.Postgresql.ObjectMeta.UID &&
+					ref.Name == cluster.Postgresql.ObjectMeta.Name &&
+					ref.Controller != nil && *ref.Controller {
+					return nil
+				}
+			}
+			return fmt.Errorf("expected a controller owner reference pointing at the Postgresql CR, got %#v",
+				secret.OwnerReferences)
+		}
+	}
+
+	hasNoControllerOwnerRef := func(cluster *Cluster) func(*v1.Secret) error {
+		return func(secret *v1.Secret) error {
+			for _, ref := range secret.OwnerReferences {
+				if ref.UID == cluster.Postgresql.ObjectMeta.UID && ref.Controller != nil && *ref.Controller {
+					return fmt.Errorf("expected no controller owner reference, got %#v", secret.OwnerReferences)
+				}
+			}
+			return nil
+		}
+	}
+
+	tests := []struct {
+		scenario              string
+		cluster               *Cluster
+		pgUser                spec.PgUser
+		expectControllerOwner bool
+	}{
+		{
+			scenario:              "owner refs + secrets deletion enabled (default)",
+			cluster:               newCluster(util.True(), util.True(), false),
+			pgUser:                newPgUser("myapp"),
+			expectControllerOwner: true,
+		},
+		{
+			scenario:              "owner refs enabled, secrets deletion disabled (skip owner ref)",
+			cluster:               newCluster(util.True(), util.False(), false),
+			pgUser:                newPgUser("myapp"),
+			expectControllerOwner: false,
+		},
+		{
+			scenario:              "owner refs enabled, secrets deletion unset (default true)",
+			cluster:               newCluster(util.True(), nil, false),
+			pgUser:                newPgUser("myapp"),
+			expectControllerOwner: true,
+		},
+		{
+			scenario:              "owner refs disabled, secrets deletion enabled",
+			cluster:               newCluster(util.False(), util.True(), false),
+			pgUser:                newPgUser("myapp"),
+			expectControllerOwner: false,
+		},
+		{
+			scenario:              "owner refs disabled, secrets deletion disabled",
+			cluster:               newCluster(util.False(), util.False(), false),
+			pgUser:                newPgUser("myapp"),
+			expectControllerOwner: false,
+		},
+		{
+			scenario:              "cross-namespace secret, owner refs + secrets deletion enabled",
+			cluster:               newCluster(util.True(), util.True(), true),
+			pgUser:                newPgUser("other-ns"),
+			expectControllerOwner: false,
+		},
+		{
+			scenario:              "cross-namespace secret, owner refs enabled, secrets deletion disabled",
+			cluster:               newCluster(util.True(), util.False(), true),
+			pgUser:                newPgUser("other-ns"),
+			expectControllerOwner: false,
+		},
+	}
+
+	for _, tt := range tests {
+		secret := tt.cluster.generateSingleUserSecret(tt.pgUser)
+		if secret == nil {
+			t.Errorf("%s [%s]: expected a non-nil secret", testName, tt.scenario)
+			continue
+		}
+
+		var check func(*v1.Secret) error
+		if tt.expectControllerOwner {
+			check = hasControllerOwnerRef(tt.cluster)
+		} else {
+			check = hasNoControllerOwnerRef(tt.cluster)
+		}
+		if err := check(secret); err != nil {
+			t.Errorf("%s [%s]: %+v", testName, tt.scenario, err)
+		}
+	}
+}
+
 func TestGenerateService(t *testing.T) {
 	var spec acidv1.PostgresSpec
 	var cluster *Cluster