From 36018c54a177a826d474caa64dee257a52d358a8 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 25 Nov 2025 12:14:35 +0100 Subject: [PATCH 1/5] stop retention user cleanup early again when DB connection attempt fails --- pkg/cluster/database.go | 20 +++++++++++++++++--- pkg/cluster/sync.go | 9 +-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/pkg/cluster/database.go b/pkg/cluster/database.go index aac877bcf..56b5f3638 100644 --- a/pkg/cluster/database.go +++ b/pkg/cluster/database.go @@ -281,9 +281,23 @@ func findUsersFromRotation(rotatedUsers []string, db *sql.DB) (map[string]string return extraUsers, nil } -func (c *Cluster) cleanupRotatedUsers(rotatedUsers []string, db *sql.DB) error { +func (c *Cluster) cleanupRotatedUsers(rotatedUsers []string) error { c.setProcessName("checking for rotated users to remove from the database due to configured retention") - extraUsers, err := findUsersFromRotation(rotatedUsers, db) + + err := c.initDbConn() + if err != nil { + return fmt.Errorf("could not init db connection: %v", err) + } + defer func() { + if c.connectionIsClosed() { + return + } + if err := c.closeDbConn(); err != nil { + c.logger.Errorf("could not close database connection after removing users exceeding configured retention interval: %v", err) + } + }() + + extraUsers, err := findUsersFromRotation(rotatedUsers, c.pgDb) if err != nil { return fmt.Errorf("error when querying for deprecated users from password rotation: %v", err) } @@ -304,7 +318,7 @@ func (c *Cluster) cleanupRotatedUsers(rotatedUsers []string, db *sql.DB) error { } if retentionDate.After(userCreationDate) { c.logger.Infof("dropping user %q due to configured days in password_rotation_user_retention", rotatedUser) - if err = users.DropPgUser(rotatedUser, db); err != nil { + if err = users.DropPgUser(rotatedUser, c.pgDb); err != nil { c.logger.Errorf("could not drop role %q: %v", rotatedUser, err) continue } diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index a210790b3..b16d32d45 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -1089,16 +1089,9 @@ func (c *Cluster) syncSecrets() error { // remove rotation users that exceed the retention interval if len(retentionUsers) > 0 { - err := c.initDbConn() - if err != nil { - errors = append(errors, fmt.Sprintf("could not init db connection: %v", err)) - } - if err = c.cleanupRotatedUsers(retentionUsers, c.pgDb); err != nil { + if err := c.cleanupRotatedUsers(retentionUsers); err != nil { errors = append(errors, fmt.Sprintf("error removing users exceeding configured retention interval: %v", err)) } - if err := c.closeDbConn(); err != nil { - errors = append(errors, fmt.Sprintf("could not close database connection after removing users exceeding configured retention interval: %v", err)) - } } if len(errors) > 0 { From 807dd9294cfc0aa281326685a1d9c9c3c1b7411b Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 1 Dec 2025 12:50:48 +0100 Subject: [PATCH 2/5] add unit test and new returned error from updateSecret --- pkg/cluster/sync.go | 22 +++++++++++++--------- pkg/cluster/sync_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index b16d32d45..8968da7d0 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -1181,12 +1181,16 @@ func (c *Cluster) updateSecret( } else { // username might not match if password rotation has been disabled again if secretUsername != string(secret.Data["username"]) { - *retentionUsers = append(*retentionUsers, secretUsername) - secret.Data["username"] = []byte(secretUsername) - secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) - secret.Data["nextRotation"] = []byte{} - updateSecret = true - updateSecretMsg = fmt.Sprintf("secret %s does not contain the role %s - updating username and resetting password", secretName, secretUsername) + if len(string(secret.Data["username"])) != len(secretUsername) { + *retentionUsers = append(*retentionUsers, secretUsername) + secret.Data["username"] = []byte(secretUsername) + secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) + secret.Data["nextRotation"] = []byte{} + updateSecret = true + updateSecretMsg = fmt.Sprintf("secret does not contain the role %s - updating username and resetting password", secretUsername) + } else { + return secret, fmt.Errorf("could not update secret because of user name mismatch: expected: %s, got: %s", secretUsername, string(secret.Data["username"])) + } } } @@ -1216,18 +1220,18 @@ func (c *Cluster) updateSecret( if updateSecret { c.logger.Infof("%s", updateSecretMsg) if secret, err = c.KubeClient.Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}); err != nil { - return secret, fmt.Errorf("could not update secret %s: %v", secretName, err) + return secret, fmt.Errorf("could not update secret: %v", err) } } if changed, _ := c.compareAnnotations(secret.Annotations, generatedSecret.Annotations, nil); changed { patchData, err := metaAnnotationsPatch(generatedSecret.Annotations) if err != nil { - return secret, fmt.Errorf("could not form patch for secret %q annotations: %v", secret.Name, err) + return secret, fmt.Errorf("could not form patch for secret annotations: %v", err) } secret, err = c.KubeClient.Secrets(secret.Namespace).Patch(context.TODO(), secret.Name, types.MergePatchType, []byte(patchData), metav1.PatchOptions{}) if err != nil { - return secret, fmt.Errorf("could not patch annotations for secret %q: %v", secret.Name, err) + return secret, fmt.Errorf("could not patch annotations for secret: %v", err) } } diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index d5bad341c..f574cd5ca 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -963,4 +963,37 @@ func TestUpdateSecret(t *testing.T) { if currentUsername != appUser { t.Errorf("%s: updated secret does not contain expected username: expected %s, got %s", testName, appUser, currentUsername) } + + // test error cases + pg.Spec.Users["prepared-owner-user"] = acidv1.UserFlags{} + pg.Spec.PreparedDatabases = map[string]acidv1.PreparedDatabase{"prepared": {DefaultUsers: true}} + + var errCluster = New( + Config{ + OpConfig: config.Config{ + Auth: config.Auth{ + SuperUsername: "postgres", + ReplicationUsername: "standby", + SecretNameTemplate: secretTemplate, + }, + Resources: config.Resources{ + ClusterLabels: map[string]string{"application": "spilo"}, + ClusterNameLabel: "cluster-name", + }, + }, + }, client, pg, logger, eventRecorder) + + errCluster.Name = clusterName + errCluster.Namespace = namespace + errCluster.pgUsers = map[string]spec.PgUser{} + + // init all users + errCluster.initUsers() + // create secrets and fail because of user name mismatch + err = errCluster.syncSecrets() + assert.Error(t, err) + + // the order of secrets to sync is not deterministic, check only first part of the error message + expectedError := fmt.Sprintf("syncing secret %s failed: could not update secret because of user name mismatch", "default/prepared-owner-user.acid-test-cluster.credentials") + assert.Contains(t, err.Error(), expectedError) } From 1601f82e22305da780ed786f4e277deea1b51e38 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 9 Dec 2025 13:32:53 +0100 Subject: [PATCH 3/5] reflect feedback from code review --- pkg/cluster/sync.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 8968da7d0..ecf692702 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -1078,7 +1078,7 @@ func (c *Cluster) syncSecrets() error { c.Secrets[updatedSecret.UID] = updatedSecret continue } - errors = append(errors, fmt.Sprintf("syncing secret %s failed: %v", util.NameFromMeta(updatedSecret.ObjectMeta), err)) + errors = append(errors, fmt.Sprintf("syncing secret %s failed: %v", util.NameFromMeta(generatedSecret.ObjectMeta), err)) pgUserDegraded = true } else { errors = append(errors, fmt.Sprintf("could not create secret for user %s: in namespace %s: %v", secretUsername, generatedSecret.Namespace, err)) @@ -1180,17 +1180,18 @@ func (c *Cluster) updateSecret( } } else { // username might not match if password rotation has been disabled again - if secretUsername != string(secret.Data["username"]) { - if len(string(secret.Data["username"])) != len(secretUsername) { - *retentionUsers = append(*retentionUsers, secretUsername) - secret.Data["username"] = []byte(secretUsername) - secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) - secret.Data["nextRotation"] = []byte{} - updateSecret = true - updateSecretMsg = fmt.Sprintf("secret does not contain the role %s - updating username and resetting password", secretUsername) - } else { - return secret, fmt.Errorf("could not update secret because of user name mismatch: expected: %s, got: %s", secretUsername, string(secret.Data["username"])) + usernameFromSecret := string(secret.Data["username"]) + if secretUsername != usernameFromSecret { + // handle edge case when manifest user conflicts with a user from prepared databases + if strings.Replace(usernameFromSecret, "-", "_", -1) == strings.Replace(secretUsername, "-", "_", -1) { + return nil, fmt.Errorf("could not update secret because of user name mismatch: expected: %s, got: %s", secretUsername, usernameFromSecret) } + *retentionUsers = append(*retentionUsers, secretUsername) + secret.Data["username"] = []byte(secretUsername) + secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength)) + secret.Data["nextRotation"] = []byte{} + updateSecret = true + updateSecretMsg = fmt.Sprintf("secret does not contain the role %s - updating username and resetting password", secretUsername) } } @@ -1220,18 +1221,18 @@ func (c *Cluster) updateSecret( if updateSecret { c.logger.Infof("%s", updateSecretMsg) if secret, err = c.KubeClient.Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}); err != nil { - return secret, fmt.Errorf("could not update secret: %v", err) + return nil, fmt.Errorf("could not update secret: %v", err) } } if changed, _ := c.compareAnnotations(secret.Annotations, generatedSecret.Annotations, nil); changed { patchData, err := metaAnnotationsPatch(generatedSecret.Annotations) if err != nil { - return secret, fmt.Errorf("could not form patch for secret annotations: %v", err) + return nil, fmt.Errorf("could not form patch for secret annotations: %v", err) } secret, err = c.KubeClient.Secrets(secret.Namespace).Patch(context.TODO(), secret.Name, types.MergePatchType, []byte(patchData), metav1.PatchOptions{}) if err != nil { - return secret, fmt.Errorf("could not patch annotations for secret: %v", err) + return nil, fmt.Errorf("could not patch annotations for secret: %v", err) } } From b74a67c93c1e853e122aa1b6d221951643781a58 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 9 Dec 2025 13:41:23 +0100 Subject: [PATCH 4/5] create a separate unit test for syncSecret name conflict error --- pkg/cluster/sync_test.go | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index f574cd5ca..5ffbb7081 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -963,12 +963,32 @@ func TestUpdateSecret(t *testing.T) { if currentUsername != appUser { t.Errorf("%s: updated secret does not contain expected username: expected %s, got %s", testName, appUser, currentUsername) } +} - // test error cases - pg.Spec.Users["prepared-owner-user"] = acidv1.UserFlags{} - pg.Spec.PreparedDatabases = map[string]acidv1.PreparedDatabase{"prepared": {DefaultUsers: true}} +func TestUpdateSecretNameConflict(t *testing.T) { + client, _ := newFakeK8sSyncSecretsClient() - var errCluster = New( + clusterName := "acid-test-cluster" + namespace := "default" + secretTemplate := config.StringTemplate("{username}.{cluster}.credentials") + + // define manifest users and enable rotation for dbowner + pg := acidv1.Postgresql{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: acidv1.PostgresSpec{ + PreparedDatabases: map[string]acidv1.PreparedDatabase{"prepared": {DefaultUsers: true}}, + Users: map[string]acidv1.UserFlags{"prepared-owner-user": {}}, + Volume: acidv1.Volume{ + Size: "1Gi", + }, + }, + } + + // new cluster with enabled password rotation + var cluster = New( Config{ OpConfig: config.Config{ Auth: config.Auth{ @@ -983,14 +1003,14 @@ func TestUpdateSecret(t *testing.T) { }, }, client, pg, logger, eventRecorder) - errCluster.Name = clusterName - errCluster.Namespace = namespace - errCluster.pgUsers = map[string]spec.PgUser{} + cluster.Name = clusterName + cluster.Namespace = namespace + cluster.pgUsers = map[string]spec.PgUser{} // init all users - errCluster.initUsers() + cluster.initUsers() // create secrets and fail because of user name mismatch - err = errCluster.syncSecrets() + err := cluster.syncSecrets() assert.Error(t, err) // the order of secrets to sync is not deterministic, check only first part of the error message From f7c7d8a67d6870d9b150b55915b84befca48155c Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 10 Dec 2025 09:56:53 +0100 Subject: [PATCH 5/5] update comments in new unit test --- pkg/cluster/sync_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index 5ffbb7081..87e9dc8a5 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -972,7 +972,8 @@ func TestUpdateSecretNameConflict(t *testing.T) { namespace := "default" secretTemplate := config.StringTemplate("{username}.{cluster}.credentials") - // define manifest users and enable rotation for dbowner + // define manifest user that has the same name as a prepared database owner user except for dashes vs underscores + // because of this the operator cannot create both secrets because underscores are not allowed in k8s secret names pg := acidv1.Postgresql{ ObjectMeta: metav1.ObjectMeta{ Name: clusterName, @@ -987,7 +988,6 @@ func TestUpdateSecretNameConflict(t *testing.T) { }, } - // new cluster with enabled password rotation var cluster = New( Config{ OpConfig: config.Config{ @@ -1010,6 +1010,7 @@ func TestUpdateSecretNameConflict(t *testing.T) { // init all users cluster.initUsers() // create secrets and fail because of user name mismatch + // prepared-owner-user from manifest vs prepared_owner_user from prepared database err := cluster.syncSecrets() assert.Error(t, err)