add unit test for syncSecrets + new updateSecret func

docs/administrator.md

@@ -297,13 +297,14 @@ For Helm deployments setting `rbac.createAggregateClusterRoles: true` adds these
 
 The operator regularly updates credentials in the K8s secrets if the
 `enable_password_rotation` option is set to `true` in the configuration.
-It happens only for LOGIN roles with an associated secret (manifest roles,
+It happens only for `LOGIN` roles with an associated secret (manifest roles,
-default users from `preparedDatabases`, system users). Furthermore, there
+default users from `preparedDatabases`). Furthermore, there are the following
-are the following exceptions:
+exceptions:
 
 1. Infrastructure role secrets since rotation should happen by the infrastructure.
-2. Team API roles that connect via OAuth2 and JWT token. Rotation should be provided by the infrastructure + there is even no secret for these roles
+2. Team API roles that connect via OAuth2 and JWT token (no secrets to these roles anyway).
-3. Database owners and direct members of owners, since ownership on database objects can not be inherited.
+3. Database owners since ownership on database objects can not be inherited.
+4. System users such as `postgres`, `standby` and `pooler` user.
 
 The interval of days can be set with `password_rotation_interval` (default
 `90` = 90 days, minimum 1). On each rotation the user name and password values
@@ -317,7 +318,7 @@ length of the interval.
 
 Pods still using the previous secret values which they keep in memory continue
 to connect to the database since the password of the corresponding user is not
-replaced. However, a retention policy can be configured for roles created by
+replaced. However, a retention policy can be configured for users created by
 the password rotation feature with `password_rotation_user_retention`. The
 operator will ensure that this period is at least twice as long as the
 configured rotation interval, hence the default of `180` = 180 days. When
@@ -326,7 +327,7 @@ might not get removed immediately. Only on the next user rotation it is checked
 if users can get removed. Therefore, you might want to configure the retention
 to be a multiple of the rotation interval.
 
-### Password rotation for single roles
+### Password rotation for single users
 
 From the configuration, password rotation is enabled for all secrets with the
 mentioned exceptions. If you wish to first test rotation for a single user (or
@@ -340,11 +341,11 @@ spec:
   - bar_reader_user
 ```
 
-### Password replacement without extra roles
+### Password replacement without extra users
 
 For some use cases where the secret is only used rarely - think of a `flyway`
 user running a migration script on pod start - we do not need to create extra
-database roles but can replace only the password in the K8s secret. This type
+database users but can replace only the password in the K8s secret. This type
 of rotation cannot be configured globally but specified in the cluster
 manifest:
 
@@ -367,7 +368,7 @@ with the latter. A new password is assigned and the `nextRotation` field is
 cleared. A final lookup for child (rotation) users to be removed is done but
 they will only be dropped if the retention policy allows for it. This is to
 avoid sudden connection issues in pods which still use credentials of these
-users in memory. You have to remove these child roles manually or re-enable
+users in memory. You have to remove these child users manually or re-enable
 password rotation with smaller interval so they get cleaned up.
 
 ## Use taints and tolerations for dedicated PostgreSQL nodes

docs/reference/cluster_manifest.md

@@ -129,7 +129,7 @@ These parameters are grouped directly under  the `spec` key in the manifest.
   password value will be replaced in the secrets which the operator reflects
   in the database, too. List only users here that rarely connect to the
   database, like a flyway user running a migration on Pod start. See more
-  details in the [administrator docs](https://github.com/zalando/postgres-operator/blob/master/docs/administrator.md#password-replacement-without-extra-roles).
+  details in the [administrator docs](https://github.com/zalando/postgres-operator/blob/master/docs/administrator.md#password-replacement-without-extra-users).
 
 * **databases**
   a map of database names to database owners for the databases that should be

docs/reference/operator_parameters.md

@@ -175,12 +175,12 @@ under the `users` key.
   `standby`.
 
 * **enable_password_rotation**
-  For all LOGIN roles that are not database owners the Operator can rotate
+  For all `LOGIN` roles that are not database owners the operator can rotate
   credentials in the corresponding K8s secrets by replacing the username and
   password. This means, new users will be added on each rotation inheriting
-  all priviliges from the original roles. The rotation date in the YYMMDD is
+  all priviliges from the original roles. The rotation date (in YYMMDD format)
-  appended to the new user names. The timestamp of the next rotation is
+  is appended to the names of the new user. The timestamp of the next rotation
-  written to the secret. The default is `false`.
+  is written to the secret. The default is `false`.
 
 * **password_rotation_interval**
   If password rotation is enabled (either from config or cluster manifest) the

pkg/cluster/cluster.go

@@ -1000,7 +1000,6 @@ func (c *Cluster) initSystemUsers() {
 		Name:      c.OpConfig.SuperUsername,
 		Namespace: c.Namespace,
 		Password:  util.RandomPassword(constants.PasswordLength),
-		IsDbOwner: true,
 	}
 	c.systemUsers[constants.ReplicationUserKeyName] = spec.PgUser{
 		Origin:    spec.RoleOriginSystem,

pkg/cluster/sync.go

@@ -617,32 +617,80 @@ func (c *Cluster) getNextRotationDate(currentDate time.Time) (time.Time, string)
 }
 
 func (c *Cluster) syncSecrets() error {
+
+	c.logger.Info("syncing secrets")
+	c.setProcessName("syncing secrets")
+	generatedSecrets := c.generateUserSecrets()
+	rotationUsers := make(spec.PgUserMap)
+	retentionUsers := make([]string, 0)
+	currentTime := time.Now()
+
+	for secretUsername, generatedSecretSpec := range generatedSecrets {
+		secret, err := c.KubeClient.Secrets(generatedSecretSpec.Namespace).Create(context.TODO(), generatedSecretSpec, metav1.CreateOptions{})
+		if err == nil {
+			c.Secrets[secret.UID] = secret
+			c.logger.Debugf("created new secret %s, namespace: %s, uid: %s", util.NameFromMeta(secret.ObjectMeta), generatedSecretSpec.Namespace, secret.UID)
+			continue
+		}
+		if k8sutil.ResourceAlreadyExists(err) {
+			if secret, err = c.KubeClient.Secrets(generatedSecretSpec.Namespace).Get(context.TODO(), generatedSecretSpec.Name, metav1.GetOptions{}); err != nil {
+				return fmt.Errorf("could not get current secret: %v", err)
+			}
+			c.Secrets[secret.UID] = secret
+			if err = c.updateSecret(secretUsername, generatedSecretSpec, secret, &rotationUsers, &retentionUsers, currentTime); err != nil {
+				c.logger.Warningf("syncing secret %s failed: %v", util.NameFromMeta(secret.ObjectMeta), err)
+			}
+		} else {
+			return fmt.Errorf("could not create secret for user %s: in namespace %s: %v", secretUsername, generatedSecretSpec.Namespace, err)
+		}
+	}
+
+	// add new user with date suffix and use it in the secret of the original user
+	if len(rotationUsers) > 0 {
+		err := c.initDbConn()
+		if err != nil {
+			return fmt.Errorf("could not init db connection: %v", err)
+		}
+		pgSyncRequests := c.userSyncStrategy.ProduceSyncRequests(spec.PgUserMap{}, rotationUsers)
+		if err = c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil {
+			return fmt.Errorf("error creating database roles for password rotation: %v", err)
+		}
+		if err := c.closeDbConn(); err != nil {
+			c.logger.Errorf("could not close database connection after creating users for password rotation: %v", err)
+		}
+	}
+
+	// remove rotation users that exceed the retention interval
+	if len(retentionUsers) > 0 {
+		err := c.initDbConn()
+		if err != nil {
+			return fmt.Errorf("could not init db connection: %v", err)
+		}
+		if err = c.cleanupRotatedUsers(retentionUsers, c.pgDb); err != nil {
+			return fmt.Errorf("error removing users exceeding configured retention interval: %v", err)
+		}
+		if err := c.closeDbConn(); err != nil {
+			c.logger.Errorf("could not close database connection after removing users exceeding configured retention interval: %v", err)
+		}
+	}
+
+	return nil
+}
+
+func (c *Cluster) updateSecret(
+	secretUsername string,
+	generatedSecret *v1.Secret,
+	secret *v1.Secret,
+	rotationUsers *spec.PgUserMap,
+	retentionUsers *[]string,
+	currentTime time.Time) error {
 	var (
 		err                 error
-		secret              *v1.Secret
 		updateSecret        bool
 		updateSecretMsg     string
 		nextRotationDate    time.Time
 		nextRotationDateStr string
 	)
-	c.logger.Info("syncing secrets")
-	c.setProcessName("syncing secrets")
-	secrets := c.generateUserSecrets()
-	rotationUsers := make(spec.PgUserMap)
-	retentionUsers := make([]string, 0)
-
-	for secretUsername, secretSpec := range secrets {
-		if secret, err = c.KubeClient.Secrets(secretSpec.Namespace).Create(context.TODO(), secretSpec, metav1.CreateOptions{}); err == nil {
-			c.Secrets[secret.UID] = secret
-			c.logger.Debugf("created new secret %s, namespace: %s, uid: %s", util.NameFromMeta(secret.ObjectMeta), secretSpec.Namespace, secret.UID)
-			continue
-		}
-		if k8sutil.ResourceAlreadyExists(err) {
-			if secret, err = c.KubeClient.Secrets(secretSpec.Namespace).Get(context.TODO(), secretSpec.Name, metav1.GetOptions{}); err != nil {
-				return fmt.Errorf("could not get current secret: %v", err)
-			}
-			c.Secrets[secret.UID] = secret
-			c.logger.Debugf("secret %s already exists, fetching its password", util.NameFromMeta(secret.ObjectMeta))
 
 	// fetch user map to update later
 	var userMap map[string]spec.PgUser
@@ -658,19 +706,21 @@ func (c *Cluster) syncSecrets() error {
 		userMap = c.pgUsers
 	}
 	pwdUser := userMap[userKey]
+	secretName := util.NameFromMeta(secret.ObjectMeta)
 
 	// if password rotation is enabled update password and username if rotation interval has been passed
-			if (c.OpConfig.EnablePasswordRotation && pwdUser.Origin != spec.RoleOriginInfrastructure && !pwdUser.IsDbOwner) ||
+	if (c.OpConfig.EnablePasswordRotation && !pwdUser.IsDbOwner &&
-				util.SliceContains(c.Spec.UsersWithSecretRotation, secretUsername) || util.SliceContains(c.Spec.UsersWithInPlaceSecretRotation, secretUsername) {
+		pwdUser.Origin != spec.RoleOriginInfrastructure && pwdUser.Origin != spec.RoleOriginSystem) ||
-				currentTime := time.Now()
+		util.SliceContains(c.Spec.UsersWithSecretRotation, secretUsername) ||
+		util.SliceContains(c.Spec.UsersWithInPlaceSecretRotation, secretUsername) {
 
 		// initialize password rotation setting first rotation date
 		nextRotationDateStr = string(secret.Data["nextRotation"])
-				if nextRotationDate, err = time.Parse("2006-01-02 15:04:05", nextRotationDateStr); err != nil {
+		if nextRotationDate, err = time.ParseInLocation("2006-01-02 15:04:05", nextRotationDateStr, time.Local); err != nil {
 			nextRotationDate, nextRotationDateStr = c.getNextRotationDate(currentTime)
 			secret.Data["nextRotation"] = []byte(nextRotationDateStr)
 			updateSecret = true
-					updateSecretMsg = fmt.Sprintf("rotation date not found in secret %q. Setting it to %s", secretSpec.Name, nextRotationDateStr)
+			updateSecretMsg = fmt.Sprintf("rotation date not found in secret %q. Setting it to %s", secretName, nextRotationDateStr)
 		}
 
 		// check if next rotation can happen sooner
@@ -688,11 +738,11 @@ func (c *Cluster) syncSecrets() error {
 				newRotationUsername := secretUsername + currentTime.Format("060102")
 				rotationUser.Name = newRotationUsername
 				rotationUser.MemberOf = []string{secretUsername}
-						rotationUsers[newRotationUsername] = rotationUser
+				(*rotationUsers)[newRotationUsername] = rotationUser
 				secret.Data["username"] = []byte(newRotationUsername)
 
 				// whenever there is a rotation, check if old rotation users can be deleted
-						retentionUsers = append(retentionUsers, secretUsername)
+				*retentionUsers = append(*retentionUsers, secretUsername)
 			}
 			secret.Data["password"] = []byte(util.RandomPassword(constants.PasswordLength))
 
@@ -700,25 +750,25 @@ func (c *Cluster) syncSecrets() error {
 			secret.Data["nextRotation"] = []byte(nextRotationDateStr)
 
 			updateSecret = true
-					updateSecretMsg = fmt.Sprintf("updating secret %q due to password rotation - next rotation date: %s", secretSpec.Name, nextRotationDateStr)
+			updateSecretMsg = fmt.Sprintf("updating secret %q due to password rotation - next rotation date: %s", secretName, nextRotationDateStr)
 		}
 	} else {
 		// username might not match if password rotation has been disabled again
 		if secretUsername != string(secret.Data["username"]) {
-					retentionUsers = append(retentionUsers, 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 %s does not contain the role %s - updating username and resetting password", secretSpec.Name, secretUsername)
+			updateSecretMsg = fmt.Sprintf("secret %s does not contain the role %s - updating username and resetting password", secretName, secretUsername)
 		}
 	}
 
 	// if this secret belongs to the infrastructure role and the password has changed - replace it in the secret
 	if pwdUser.Password != string(secret.Data["password"]) && pwdUser.Origin == spec.RoleOriginInfrastructure {
-				secret = secretSpec
+		secret = generatedSecret
 		updateSecret = true
-				updateSecretMsg = fmt.Sprintf("updating the secret %s from the infrastructure roles", secretSpec.Name)
+		updateSecretMsg = fmt.Sprintf("updating the secret %s from the infrastructure roles", secretName)
 	} else {
 		// for non-infrastructure role - update the role with the password from the secret
 		pwdUser.Password = string(secret.Data["password"])
@@ -727,47 +777,12 @@ func (c *Cluster) syncSecrets() error {
 
 	if updateSecret {
 		c.logger.Debugln(updateSecretMsg)
-				if _, err = c.KubeClient.Secrets(secretSpec.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}); err != nil {
+		if _, err = c.KubeClient.Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}); err != nil {
-					c.logger.Warningf("could not update secret %q: %v", secretSpec.Name, err)
+			return fmt.Errorf("could not update secret %q: %v", secretName, err)
-					continue
 		}
 		c.Secrets[secret.UID] = secret
 	}
 
-		} else {
-			return fmt.Errorf("could not create secret for user %s: in namespace %s: %v", secretUsername, secretSpec.Namespace, err)
-		}
-	}
-
-	// add new user with date suffix and use it in the secret of the original user
-	if len(rotationUsers) > 0 {
-		err = c.initDbConn()
-		if err != nil {
-			return fmt.Errorf("could not init db connection: %v", err)
-		}
-		pgSyncRequests := c.userSyncStrategy.ProduceSyncRequests(spec.PgUserMap{}, rotationUsers)
-		if err = c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil {
-			return fmt.Errorf("error creating database roles for password rotation: %v", err)
-		}
-		if err := c.closeDbConn(); err != nil {
-			c.logger.Errorf("could not close database connection after creating users for password rotation: %v", err)
-		}
-	}
-
-	// remove rotation users that exceed the retention interval
-	if len(retentionUsers) > 0 {
-		err = c.initDbConn()
-		if err != nil {
-			return fmt.Errorf("could not init db connection: %v", err)
-		}
-		if err = c.cleanupRotatedUsers(retentionUsers, c.pgDb); err != nil {
-			return fmt.Errorf("error removing users exceeding configured retention interval: %v", err)
-		}
-		if err := c.closeDbConn(); err != nil {
-			c.logger.Errorf("could not close database connection after removing users exceeding configured retention interval: %v", err)
-		}
-	}
-
 	return nil
 }
 

pkg/cluster/sync_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/zalando/postgres-operator/mocks"
 	acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1"
 	fakeacidv1 "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/fake"
+	"github.com/zalando/postgres-operator/pkg/spec"
 	"github.com/zalando/postgres-operator/pkg/util/config"
 	"github.com/zalando/postgres-operator/pkg/util/k8sutil"
 	"github.com/zalando/postgres-operator/pkg/util/patroni"
@@ -26,6 +27,8 @@ import (
 )
 
 var patroniLogger = logrus.New().WithField("test", "patroni")
+var acidClientSet = fakeacidv1.NewSimpleClientset()
+var clientSet = fake.NewSimpleClientset()
 
 func newMockPod(ip string) *v1.Pod {
 	return &v1.Pod{
@@ -36,9 +39,6 @@ func newMockPod(ip string) *v1.Pod {
 }
 
 func newFakeK8sSyncClient() (k8sutil.KubernetesClient, *fake.Clientset) {
-	acidClientSet := fakeacidv1.NewSimpleClientset()
-	clientSet := fake.NewSimpleClientset()
-
 	return k8sutil.KubernetesClient{
 		PodsGetter:         clientSet.CoreV1(),
 		PostgresqlsGetter:  acidClientSet.AcidV1(),
@@ -46,6 +46,12 @@ func newFakeK8sSyncClient() (k8sutil.KubernetesClient, *fake.Clientset) {
 	}, clientSet
 }
 
+func newFakeK8sSyncSecretsClient() (k8sutil.KubernetesClient, *fake.Clientset) {
+	return k8sutil.KubernetesClient{
+		SecretsGetter: clientSet.CoreV1(),
+	}, clientSet
+}
+
 func TestSyncStatefulSetsAnnotations(t *testing.T) {
 	testName := "test syncing statefulsets annotations"
 	client, _ := newFakeK8sSyncClient()
@@ -257,3 +263,72 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 		}
 	}
 }
+
+func TestUpdateSecret(t *testing.T) {
+	testName := "test syncing secrets"
+	client, _ := newFakeK8sSyncSecretsClient()
+
+	clusterName := "acid-test-cluster"
+	namespace := "default"
+	username := "foo"
+	secretTemplate := config.StringTemplate("{username}.{cluster}.credentials")
+	rotationUsers := make(spec.PgUserMap)
+	retentionUsers := make([]string, 0)
+	yesterday := time.Now().AddDate(0, 0, -1)
+
+	// new cluster with pvc storage resize mode and configured labels
+	var cluster = New(
+		Config{
+			OpConfig: config.Config{
+				Auth: config.Auth{
+					SecretNameTemplate:            secretTemplate,
+					EnablePasswordRotation:        true,
+					PasswordRotationInterval:      1,
+					PasswordRotationUserRetention: 3,
+				},
+				Resources: config.Resources{
+					ClusterLabels:    map[string]string{"application": "spilo"},
+					ClusterNameLabel: "cluster-name",
+				},
+			},
+		}, client, acidv1.Postgresql{}, logger, eventRecorder)
+
+	cluster.Name = clusterName
+	cluster.Namespace = namespace
+	cluster.pgUsers = map[string]spec.PgUser{}
+	cluster.Spec.Users = map[string]acidv1.UserFlags{username: {}}
+	cluster.initRobotUsers()
+
+	// create a secret for user foo
+	cluster.syncSecrets()
+
+	secret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{})
+	assert.NoError(t, err)
+	generatedSecret := cluster.Secrets[secret.UID]
+
+	// now update the secret setting next rotation date (yesterday + interval)
+	cluster.updateSecret(username, generatedSecret, secret, &rotationUsers, &retentionUsers, yesterday)
+	updatedSecret, err := cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{})
+	assert.NoError(t, err)
+
+	nextRotation := string(updatedSecret.Data["nextRotation"])
+	_, nextRotationDate := cluster.getNextRotationDate(yesterday)
+	if nextRotation != nextRotationDate {
+		t.Errorf("%s: updated secret does not contain correct rotation date: expected %s, got %s", testName, nextRotationDate, nextRotation)
+	}
+
+	// update secret again but use current time to trigger rotation
+	cluster.updateSecret(username, generatedSecret, updatedSecret, &rotationUsers, &retentionUsers, time.Now())
+	updatedSecret, err = cluster.KubeClient.Secrets(namespace).Get(context.TODO(), secretTemplate.Format("username", username, "cluster", clusterName), metav1.GetOptions{})
+	assert.NoError(t, err)
+
+	if len(rotationUsers) != 1 && len(retentionUsers) != 1 {
+		t.Errorf("%s: unexpected number of users to rotate - expected only foo, found %d", testName, len(rotationUsers))
+	}
+
+	secretUsername := string(updatedSecret.Data["username"])
+	rotatedUsername := username + time.Now().Format("060102")
+	if secretUsername != rotatedUsername {
+		t.Errorf("%s: updated secret does not contain correct username: expected %s, got %s", testName, rotatedUsername, secretUsername)
+	}
+}