diff --git a/.github/workflows/run_e2e.yaml b/.github/workflows/run_e2e.yaml index 5dcb195d2..cdfcf9b2b 100644 --- a/.github/workflows/run_e2e.yaml +++ b/.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 diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 4c37d2629..82e9cb39c 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -250,6 +250,8 @@ class EndToEndTestCase(unittest.TestCase): } 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 @@ class EndToEndTestCase(unittest.TestCase): 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,11 +326,44 @@ class EndToEndTestCase(unittest.TestCase): 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)), 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 recreated member in PostgresTeam not renamed back to original name", 10, 5) + "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 = { @@ -1204,8 +1246,9 @@ class EndToEndTestCase(unittest.TestCase): 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 @@ class EndToEndTestCase(unittest.TestCase): return result_set if __name__ == '__main__': - unittest.main() + unittest.main() \ No newline at end of file diff --git a/pkg/cluster/database.go b/pkg/cluster/database.go index 652f0d0ae..978b570b0 100644 --- a/pkg/cluster/database.go +++ b/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 } diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 3300f8724..68f00533a 100644 --- a/pkg/cluster/sync.go +++ b/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) diff --git a/pkg/util/users/users.go b/pkg/util/users/users.go index b7f5c45c1..4d9a21f73 100644 --- a/pkg/util/users/users.go +++ b/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}) } } diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 75853c3d6..6444bb48f 100644 --- a/pkg/util/util_test.go +++ b/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 {