From 3edd1a55abaaf953fb80b1f2229ced7086315c45 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 21 Apr 2022 15:00:19 +0200 Subject: [PATCH] reverse membership for additional owner roles remove type RoleOriginSpilo --- docs/reference/operator_parameters.md | 4 ++-- e2e/tests/test_e2e.py | 6 +++--- pkg/cluster/cluster.go | 29 ++++++++------------------- pkg/cluster/cluster_test.go | 29 +++++++++------------------ pkg/cluster/k8sres.go | 2 +- pkg/spec/types.go | 1 - pkg/util/users/users.go | 19 ++++++++---------- 7 files changed, 31 insertions(+), 59 deletions(-) diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index e4e64cc32..4ec777c7f 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -178,8 +178,8 @@ under the `users` key. `standby`. * **additional_owner_roles** - Specifies database roles that will become members of all database owners. - Then owners can use `SET ROLE` to obtain privileges of these roles to e.g. + Specifies database roles that will granted to all database owners. Owners + can then use `SET ROLE` to obtain privileges of these roles to e.g. create/update functionality from extensions as part of a migration script. Note, that roles listed here should be preconfigured in the docker image and already exist in the database cluster on startup. One such role can be diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 9b1cf50b4..5b324fda4 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -161,7 +161,7 @@ class EndToEndTestCase(unittest.TestCase): @timeout_decorator.timeout(TEST_TIMEOUT_SEC) def test_additional_owner_roles(self): ''' - Test adding additional member roles to existing database owner roles + Test granting additional roles to existing database owners ''' k8s = self.k8s @@ -181,10 +181,10 @@ class EndToEndTestCase(unittest.TestCase): FROM pg_catalog.pg_authid a JOIN pg_catalog.pg_auth_members am ON a.oid = am.member - AND a.rolname = 'cron_admin' + AND a.rolname IN ('zalando', 'bar_owner', 'bar_data_owner') JOIN pg_catalog.pg_authid a2 ON a2.oid = am.roleid - WHERE a2.rolname IN ('zalando', 'bar_owner', 'bar_data_owner'); + WHERE a2.rolname = 'cron_admin'; """ self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "postgres", owner_query)), 3, "Not all additional users found in database", 10, 5) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index a92b09094..66ec0dc60 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -1308,28 +1308,15 @@ func (c *Cluster) initRobotUsers() error { } func (c *Cluster) initAdditionalOwnerRoles() { - for _, additionalOwner := range c.OpConfig.AdditionalOwnerRoles { - // fetch all database owners the additional should become a member of - memberOf := make([]string, 0) - for username, pgUser := range c.pgUsers { - if pgUser.IsDbOwner { - memberOf = append(memberOf, username) - } - } + if len(c.OpConfig.AdditionalOwnerRoles) == 0 { + return + } - if len(memberOf) > 0 { - namespace := c.Namespace - additionalOwnerPgUser := spec.PgUser{ - Origin: spec.RoleOriginSpilo, - MemberOf: memberOf, - Name: additionalOwner, - Namespace: namespace, - } - if currentRole, present := c.pgUsers[additionalOwner]; present { - c.pgUsers[additionalOwner] = c.resolveNameConflict(¤tRole, &additionalOwnerPgUser) - } else { - c.pgUsers[additionalOwner] = additionalOwnerPgUser - } + // fetch database owners and assign additional owner roles + for username, pgUser := range c.pgUsers { + if pgUser.IsDbOwner { + pgUser.MemberOf = append(pgUser.MemberOf, c.OpConfig.AdditionalOwnerRoles...) + c.pgUsers[username] = pgUser } } } diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 7581d1473..bb4d17072 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -148,11 +148,9 @@ func TestInitAdditionalOwnerRoles(t *testing.T) { manifestUsers := map[string]acidv1.UserFlags{"foo_owner": {}, "bar_owner": {}, "app_user": {}} expectedUsers := map[string]spec.PgUser{ - "foo_owner": {Origin: spec.RoleOriginManifest, Name: "foo_owner", Namespace: cl.Namespace, Password: "f123", Flags: []string{"LOGIN"}, IsDbOwner: true}, - "bar_owner": {Origin: spec.RoleOriginManifest, Name: "bar_owner", Namespace: cl.Namespace, Password: "b123", Flags: []string{"LOGIN"}, IsDbOwner: true}, - "app_user": {Origin: spec.RoleOriginManifest, Name: "app_user", Namespace: cl.Namespace, Password: "a123", Flags: []string{"LOGIN"}, IsDbOwner: false}, - "cron_admin": {Origin: spec.RoleOriginSpilo, Name: "cron_admin", Namespace: cl.Namespace, MemberOf: []string{"foo_owner", "bar_owner"}}, - "part_man": {Origin: spec.RoleOriginSpilo, Name: "part_man", Namespace: cl.Namespace, MemberOf: []string{"foo_owner", "bar_owner"}}, + "foo_owner": {Origin: spec.RoleOriginManifest, Name: "foo_owner", Namespace: cl.Namespace, Password: "f123", Flags: []string{"LOGIN"}, IsDbOwner: true, MemberOf: []string{"cron_admin", "part_man"}}, + "bar_owner": {Origin: spec.RoleOriginManifest, Name: "bar_owner", Namespace: cl.Namespace, Password: "b123", Flags: []string{"LOGIN"}, IsDbOwner: true, MemberOf: []string{"cron_admin", "part_man"}}, + "app_user": {Origin: spec.RoleOriginManifest, Name: "app_user", Namespace: cl.Namespace, Password: "a123", Flags: []string{"LOGIN"}, IsDbOwner: false}, } cl.Spec.Databases = map[string]string{"foo_db": "foo_owner", "bar_db": "bar_owner"} @@ -163,24 +161,15 @@ func TestInitAdditionalOwnerRoles(t *testing.T) { t.Errorf("%s could not init manifest users", testName) } - // update passwords to compare with result - for manifestUser := range manifestUsers { - pgUser := cl.pgUsers[manifestUser] - pgUser.Password = manifestUser[0:1] + "123" - cl.pgUsers[manifestUser] = pgUser - } - + // now assign additional roles to owners cl.initAdditionalOwnerRoles() - for _, additionalOwnerRole := range cl.Config.OpConfig.AdditionalOwnerRoles { - expectedPgUser := expectedUsers[additionalOwnerRole] - existingPgUser, exists := cl.pgUsers[additionalOwnerRole] - if !exists { - t.Errorf("%s additional owner role %q not initilaized", testName, additionalOwnerRole) - } + // update passwords to compare with result + for username, existingPgUser := range cl.pgUsers { + expectedPgUser := expectedUsers[username] if !util.IsEqualIgnoreOrder(expectedPgUser.MemberOf, existingPgUser.MemberOf) { - t.Errorf("%s unexpected membership of additional owner role %q: expected member of %#v, got member of %#v", - testName, additionalOwnerRole, expectedPgUser.MemberOf, existingPgUser.MemberOf) + t.Errorf("%s unexpected membership of user %q: expected member of %#v, got member of %#v", + testName, username, expectedPgUser.MemberOf, existingPgUser.MemberOf) } } } diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 6cdf379bd..49c474d5d 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1649,7 +1649,7 @@ func (c *Cluster) generateUserSecrets() map[string]*v1.Secret { func (c *Cluster) generateSingleUserSecret(namespace string, pgUser spec.PgUser) *v1.Secret { //Skip users with no password i.e. human users (they'll be authenticated using pam) if pgUser.Password == "" { - if pgUser.Origin != spec.RoleOriginTeamsAPI && pgUser.Origin != spec.RoleOriginSpilo { + if pgUser.Origin != spec.RoleOriginTeamsAPI { c.logger.Warningf("could not generate secret for a non-teamsAPI role %q: role has no password", pgUser.Name) } diff --git a/pkg/spec/types.go b/pkg/spec/types.go index e594a7978..02f67d253 100644 --- a/pkg/spec/types.go +++ b/pkg/spec/types.go @@ -30,7 +30,6 @@ const ( RoleOriginManifest RoleOriginInfrastructure RoleOriginTeamsAPI - RoleOriginSpilo RoleOriginSystem RoleOriginBootstrap RoleConnectionPooler diff --git a/pkg/util/users/users.go b/pkg/util/users/users.go index 5007268d2..d09a148b4 100644 --- a/pkg/util/users/users.go +++ b/pkg/util/users/users.go @@ -57,15 +57,13 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM newMD5Password := util.NewEncryptor(strategy.PasswordEncryption).PGUserPassword(newUser) // do not compare for roles coming from docker image - if newUser.Origin != spec.RoleOriginSpilo { - if dbUser.Password != newMD5Password { - r.User.Password = newMD5Password - r.Kind = spec.PGsyncUserAlter - } - if addNewFlags, equal := util.SubstractStringSlices(newUser.Flags, dbUser.Flags); !equal { - r.User.Flags = addNewFlags - r.Kind = spec.PGsyncUserAlter - } + if dbUser.Password != newMD5Password { + r.User.Password = newMD5Password + r.Kind = spec.PGsyncUserAlter + } + if addNewFlags, equal := util.SubstractStringSlices(newUser.Flags, dbUser.Flags); !equal { + r.User.Flags = addNewFlags + r.Kind = spec.PGsyncUserAlter } if addNewRoles, equal := util.SubstractStringSlices(newUser.MemberOf, dbUser.MemberOf); !equal { r.User.MemberOf = addNewRoles @@ -75,8 +73,7 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM r.User.Name = newUser.Name reqs = append(reqs, r) } - if newUser.Origin != spec.RoleOriginSpilo && - len(newUser.Parameters) > 0 && + if len(newUser.Parameters) > 0 && !reflect.DeepEqual(dbUser.Parameters, newUser.Parameters) { reqs = append(reqs, spec.PgSyncUserRequest{Kind: spec.PGSyncAlterSet, User: newUser}) }