stop retention user cleanup early again when DB connection attempt fails

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
 			}

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 {
@@ -1188,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"]))
+			}
 		}
 	}
 
@@ -1223,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)
 		}
 	}
 

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