fix team member deprecation (#2072)

Author: FxKu

.github/workflows/run_e2e.yaml

@@ -17,6 +17,8 @@ jobs:
           go-version: "^1.17.4"
     - name: Make dependencies
       run: make deps mocks
+    - name: Code generation
+      run: make codegen
     - name: Compile
       run: make linux
     - name: Run unit tests

e2e/tests/test_e2e.py

@@ -250,6 +250,8 @@ def test_additional_teams_and_members(self):
         }
         k8s.update_config(enable_postgres_team_crd)
 
+        # add team and member to custom-team-membership
+        # contains already elephant user
         k8s.api.custom_objects_api.patch_namespaced_custom_object(
         'acid.zalan.do', 'v1', 'default',
         'postgresteams', 'custom-team-membership',
@@ -300,6 +302,13 @@ def test_additional_teams_and_members(self):
         self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 2,
             "Database role of replaced member in PostgresTeam not renamed", 10, 5)
 
+        # create fake deletion user so operator fails renaming
+        # but altering role to NOLOGIN will succeed
+        create_fake_deletion_user = """
+            CREATE USER tester_delete_me NOLOGIN;
+        """
+        self.query_database(leader.metadata.name, "postgres", create_fake_deletion_user)
+
         # re-add additional member and check if the role is renamed back
         k8s.api.custom_objects_api.patch_namespaced_custom_object(
         'acid.zalan.do', 'v1', 'default',
@@ -317,12 +326,45 @@ def test_additional_teams_and_members(self):
         user_query = """
             SELECT rolname
               FROM pg_catalog.pg_roles
-             WHERE (rolname = 'kind' AND rolcanlogin)
-                OR (rolname = 'tester_delete_me' AND NOT rolcanlogin);
+             WHERE rolname = 'kind' AND rolcanlogin;
         """
-        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 2,
+        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 1,
             "Database role of recreated member in PostgresTeam not renamed back to original name", 10, 5)
 
+        user_query = """
+            SELECT rolname
+              FROM pg_catalog.pg_roles
+             WHERE rolname IN ('tester','tester_delete_me') AND NOT rolcanlogin;
+        """
+        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 2,
+            "Database role of replaced member in PostgresTeam not denied from login", 10, 5)
+
+        # re-add other additional member, operator should grant LOGIN back to tester
+        # but nothing happens to deleted role
+        k8s.api.custom_objects_api.patch_namespaced_custom_object(
+        'acid.zalan.do', 'v1', 'default',
+        'postgresteams', 'custom-team-membership',
+        {
+            'spec': {
+                'additionalMembers': {
+                    'e2e': [
+                        'kind',
+                        'tester'
+                    ]
+                },
+            }
+        })
+
+        user_query = """
+            SELECT rolname
+              FROM pg_catalog.pg_roles
+             WHERE (rolname IN ('tester', 'kind')
+               AND rolcanlogin)
+                OR (rolname = 'tester_delete_me' AND NOT rolcanlogin);
+        """
+        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", user_query)), 3,
+            "Database role of deleted member in PostgresTeam not removed when recreated manually", 10, 5)
+
         # revert config change
         revert_resync = {
             "data": {
@@ -1204,8 +1246,9 @@ def test_node_affinity(self):
             self.eventuallyEqual(lambda: k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync")
 
             # node affinity change should cause another rolling update and relocation of replica
-            k8s.wait_for_pod_start('spilo-role=replica,' + cluster_label)
+            k8s.wait_for_pod_failover(master_nodes, 'spilo-role=replica,' + cluster_label)
             k8s.wait_for_pod_start('spilo-role=master,' + cluster_label)
+            k8s.wait_for_pod_start('spilo-role=replica,' + cluster_label)
 
         except timeout_decorator.TimeoutError:
             print('Operator log: {}'.format(k8s.get_operator_log()))
@@ -1956,4 +1999,4 @@ def query_database(self, pod_name, db_name, query):
         return result_set
 
 if __name__ == '__main__':
-    unittest.main()
+    unittest.main()

pkg/cluster/database.go

@@ -231,7 +231,8 @@ func (c *Cluster) readPgUsersFromDatabase(userNames []string) (users spec.PgUser
 			parameters[fields[0]] = fields[1]
 		}
 
-		if strings.HasSuffix(rolname, c.OpConfig.RoleDeletionSuffix) {
+		// consider NOLOGIN roles with deleted suffix as deprecated users
+		if strings.HasSuffix(rolname, c.OpConfig.RoleDeletionSuffix) && !rolcanlogin {
 			roldeleted = true
 		}
 

pkg/cluster/sync.go

@@ -104,18 +104,19 @@ func (c *Cluster) Sync(newSpec *acidv1.Postgresql) error {
 	if !(c.databaseAccessDisabled() || c.getNumberOfInstances(&newSpec.Spec) <= 0 || c.Spec.StandbyCluster != nil) {
 		c.logger.Debug("syncing roles")
 		if err = c.syncRoles(); err != nil {
-			err = fmt.Errorf("could not sync roles: %v", err)
-			return err
+			// remember all cached users in c.pgUsers
+			for cachedUserName, cachedUser := range c.pgUsersCache {
+				c.pgUsers[cachedUserName] = cachedUser
+			}
+			c.logger.Errorf("could not sync roles: %v", err)
 		}
 		c.logger.Debug("syncing databases")
 		if err = c.syncDatabases(); err != nil {
-			err = fmt.Errorf("could not sync databases: %v", err)
-			return err
+			c.logger.Errorf("could not sync databases: %v", err)
 		}
 		c.logger.Debug("syncing prepared databases with schemas")
 		if err = c.syncPreparedDatabases(); err != nil {
-			err = fmt.Errorf("could not sync prepared database: %v", err)
-			return err
+			c.logger.Errorf("could not sync prepared database: %v", err)
 		}
 	}
 
@@ -933,10 +934,7 @@ func (c *Cluster) syncRoles() (err error) {
 		}
 	}
 
-	// copy map for ProduceSyncRequests to include also system users
-	for userName, pgUser := range c.pgUsers {
-		newUsers[userName] = pgUser
-	}
+	// search also for system users
 	for _, systemUser := range c.systemUsers {
 		userNames = append(userNames, systemUser.Name)
 		newUsers[systemUser.Name] = systemUser
@@ -950,13 +948,21 @@ func (c *Cluster) syncRoles() (err error) {
 	// update pgUsers where a deleted role was found
 	// so that they are skipped in ProduceSyncRequests
 	for _, dbUser := range dbUsers {
-		if originalUser, exists := deletedUsers[dbUser.Name]; exists {
-			recreatedUser := c.pgUsers[originalUser]
+		originalUsername, foundDeletedUser := deletedUsers[dbUser.Name]
+		// check if original user does not exist in dbUsers
+		_, originalUserAlreadyExists := dbUsers[originalUsername]
+		if foundDeletedUser && !originalUserAlreadyExists {
+			recreatedUser := c.pgUsers[originalUsername]
 			recreatedUser.Deleted = true
-			c.pgUsers[originalUser] = recreatedUser
+			c.pgUsers[originalUsername] = recreatedUser
 		}
 	}
 
+	// last but not least copy pgUsers to newUsers to send to ProduceSyncRequests
+	for _, pgUser := range c.pgUsers {
+		newUsers[pgUser.Name] = pgUser
+	}
+
 	pgSyncRequests := c.userSyncStrategy.ProduceSyncRequests(dbUsers, newUsers)
 	if err = c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil {
 		return fmt.Errorf("error executing sync statements: %v", err)

pkg/util/users/users.go

@@ -43,7 +43,8 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM
 
 	var reqs []spec.PgSyncUserRequest
 	for name, newUser := range newUsers {
-		// do not create user that exists in DB with deletion suffix
+		// do not create user when there exists a user with the same name plus deletion suffix
+		// instead request a renaming of the deleted user back to the original name (see * below)
 		if newUser.Deleted {
 			continue
 		}
@@ -82,22 +83,28 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM
 		}
 	}
 
-	// No existing roles are deleted or stripped of role membership/flags
+	// no existing roles are deleted or stripped of role membership/flags
 	// but team roles will be renamed and denied from LOGIN
 	for name, dbUser := range dbUsers {
 		if _, exists := newUsers[name]; !exists {
-			// toggle LOGIN flag based on role deletion
-			userFlags := make([]string, len(dbUser.Flags))
-			userFlags = append(userFlags, dbUser.Flags...)
 			if dbUser.Deleted {
-				dbUser.Flags = util.StringSliceReplaceElement(dbUser.Flags, constants.RoleFlagNoLogin, constants.RoleFlagLogin)
+				// * user with deletion suffix and NOLOGIN found in database
+				// grant back LOGIN and rename only if original user is wanted and does not exist in database
+				originalName := strings.TrimSuffix(name, strategy.RoleDeletionSuffix)
+				_, originalUserWanted := newUsers[originalName]
+				_, originalUserAlreadyExists := dbUsers[originalName]
+				if !originalUserWanted || originalUserAlreadyExists {
+					continue
+				}
+				// a deleted dbUser has no NOLOGIN flag, so we can add the LOGIN flag
+				dbUser.Flags = append(dbUser.Flags, constants.RoleFlagLogin)
 			} else {
+				// user found in database and not wanted in newUsers - replace LOGIN flag with NOLOGIN
 				dbUser.Flags = util.StringSliceReplaceElement(dbUser.Flags, constants.RoleFlagLogin, constants.RoleFlagNoLogin)
 			}
-			if !util.IsEqualIgnoreOrder(userFlags, dbUser.Flags) {
-				reqs = append(reqs, spec.PgSyncUserRequest{Kind: spec.PGsyncUserAlter, User: dbUser})
-			}
-
+			// request ALTER ROLE to grant or revoke LOGIN
+			reqs = append(reqs, spec.PgSyncUserRequest{Kind: spec.PGsyncUserAlter, User: dbUser})
+			// request RENAME which will happen on behalf of the pgUser.Deleted field
 			reqs = append(reqs, spec.PgSyncUserRequest{Kind: spec.PGSyncUserRename, User: dbUser})
 		}
 	}

pkg/util/util_test.go

@@ -62,6 +62,8 @@ var substractTest = []struct {
 }{
 	{[]string{"a", "b", "c", "d"}, []string{"a", "b", "c", "d"}, []string{}, true},
 	{[]string{"a", "b", "c", "d"}, []string{"a", "bb", "c", "d"}, []string{"b"}, false},
+	{[]string{""}, []string{"b"}, []string{""}, false},
+	{[]string{"a"}, []string{""}, []string{"a"}, false},
 }
 
 var sliceContaintsTest = []struct {