From a77d5df158593ce1f10afec5de3caec5698422f7 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 28 Apr 2022 11:15:40 +0200 Subject: [PATCH] reverse membership for additional owner roles (#1862) * reverse membership for additional owner roles * remove type RoleOriginSpilo * use e2e images with cron_admin inside * let operator resolve reversed membership * make additional owner roles part of the sync user strategy * add more context in the docs about additional_owner_roles --- docs/reference/operator_parameters.md | 19 ++++++--- e2e/tests/test_e2e.py | 22 +++++++--- pkg/cluster/cluster.go | 35 ++++++--------- pkg/cluster/cluster_test.go | 29 ++++--------- pkg/cluster/k8sres.go | 2 +- pkg/spec/types.go | 1 - pkg/util/users/users.go | 61 +++++++++++++++++++++------ 7 files changed, 97 insertions(+), 72 deletions(-) diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index e4e64cc32..fa5fb0f52 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -178,13 +178,18 @@ 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. - 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 - `cron_admin` which is provided by the Spilo docker image to set up cron - jobs inside the `postgres` database. Default is `empty`. + Specifies database roles that will be granted to all database owners. Owners + can then use `SET ROLE` to obtain privileges of these roles to e.g. create + or update functionality from extensions as part of a migration script. One + such role can be `cron_admin` which is provided by the Spilo docker image to + set up cron jobs inside the `postgres` database. In general, roles listed + here should be preconfigured in the docker image and already exist in the + database cluster on startup. Otherwise, syncing roles will return an error + on each cluster sync process. Alternatively, you have to create the role and + do the GRANT manually. Note, the operator will not allow additional owner + roles to be members of database owners because it should be vice versa. If + the operator cannot set up the correct membership it tries to revoke all + additional owner roles from database owners. Default is `empty`. * **enable_password_rotation** For all `LOGIN` roles that are not database owners the operator can rotate diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 9b1cf50b4..9e990e013 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -12,8 +12,8 @@ from kubernetes import client from tests.k8s_api import K8s from kubernetes.client.rest import ApiException -SPILO_CURRENT = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.1" -SPILO_LAZY = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.2" +SPILO_CURRENT = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.3" +SPILO_LAZY = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.4" def to_selector(labels): @@ -161,10 +161,21 @@ 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 + # first test - wait for the operator to get in sync and set everything up + self.eventuallyEqual(lambda: k8s.get_operator_state(), {"0": "idle"}, + "Operator does not get in sync") + leader = k8s.get_cluster_leader_pod() + + # produce wrong membership for cron_admin + grant_dbowner = """ + GRANT bar_owner TO cron_admin; + """ + self.query_database(leader.metadata.name, "postgres", grant_dbowner) + # enable PostgresTeam CRD and lower resync owner_roles = { "data": { @@ -175,16 +186,15 @@ class EndToEndTestCase(unittest.TestCase): self.eventuallyEqual(lambda: k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync") - leader = k8s.get_cluster_leader_pod() owner_query = """ SELECT a2.rolname 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..85f60b601 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -133,8 +133,10 @@ func New(cfg Config, kubeClient k8sutil.KubernetesClient, pgSpec acidv1.Postgres Services: make(map[PostgresRole]*v1.Service), Endpoints: make(map[PostgresRole]*v1.Endpoints)}, userSyncStrategy: users.DefaultUserSyncStrategy{ - PasswordEncryption: passwordEncryption, - RoleDeletionSuffix: cfg.OpConfig.RoleDeletionSuffix}, + PasswordEncryption: passwordEncryption, + RoleDeletionSuffix: cfg.OpConfig.RoleDeletionSuffix, + AdditionalOwnerRoles: cfg.OpConfig.AdditionalOwnerRoles, + }, deleteOptions: metav1.DeleteOptions{PropagationPolicy: &deletePropagationPolicy}, podEventsQueue: podEventsQueue, KubeClient: kubeClient, @@ -1308,28 +1310,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 29e0914c8..5b2f4c745 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1650,7 +1650,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..e6364c39c 100644 --- a/pkg/util/users/users.go +++ b/pkg/util/users/users.go @@ -20,6 +20,7 @@ const ( alterRoleSetSQL = `ALTER ROLE "%s" SET %s TO %s` dropUserSQL = `SET LOCAL synchronous_commit = 'local'; DROP ROLE "%s";` grantToUserSQL = `GRANT %s TO "%s"` + revokeFromUserSQL = `REVOKE %s FROM %s` doBlockStmt = `SET LOCAL synchronous_commit = 'local'; DO $$ BEGIN %s; END;$$;` passwordTemplate = "ENCRYPTED PASSWORD '%s'" inRoleTemplate = `IN ROLE %s` @@ -31,8 +32,9 @@ const ( // an existing roles of another role membership, nor it removes the already assigned flag // (except for the NOLOGIN). TODO: process other NOflags, i.e. NOSUPERUSER correctly. type DefaultUserSyncStrategy struct { - PasswordEncryption string - RoleDeletionSuffix string + PasswordEncryption string + RoleDeletionSuffix string + AdditionalOwnerRoles []string } // ProduceSyncRequests figures out the types of changes that need to happen with the given users. @@ -53,30 +55,27 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM } } else { r := spec.PgSyncUserRequest{} - r.User = dbUser 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 addNewRoles, equal := util.SubstractStringSlices(newUser.MemberOf, dbUser.MemberOf); !equal { r.User.MemberOf = addNewRoles + r.User.IsDbOwner = newUser.IsDbOwner + r.Kind = spec.PGsyncUserAlter + } + if addNewFlags, equal := util.SubstractStringSlices(newUser.Flags, dbUser.Flags); !equal { + r.User.Flags = addNewFlags r.Kind = spec.PGsyncUserAlter } if r.Kind == spec.PGsyncUserAlter { 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}) } @@ -120,6 +119,15 @@ func (strategy DefaultUserSyncStrategy) ExecuteSyncRequests(requests []spec.PgSy if err := strategy.alterPgUser(request.User, db); err != nil { reqretries = append(reqretries, request) errors = append(errors, fmt.Sprintf("could not alter user %q: %v", request.User.Name, err)) + // XXX: we do not allow additional owner roles to be members of database owners + // if ALTER fails it could be because of the wrong memberhip (check #1862 for details) + // so in any case try to revoke the database owner from the additional owner roles + // the initial ALTER statement will be retried once and should work then + if request.User.IsDbOwner && len(strategy.AdditionalOwnerRoles) > 0 { + if err := resolveOwnerMembership(request.User, strategy.AdditionalOwnerRoles, db); err != nil { + errors = append(errors, fmt.Sprintf("could not resolve owner membership for %q: %v", request.User.Name, err)) + } + } } case spec.PGSyncAlterSet: if err := strategy.alterPgUserSet(request.User, db); err != nil { @@ -152,6 +160,21 @@ func (strategy DefaultUserSyncStrategy) ExecuteSyncRequests(requests []spec.PgSy return nil } +func resolveOwnerMembership(dbOwner spec.PgUser, additionalOwners []string, db *sql.DB) error { + errors := make([]string, 0) + for _, additionalOwner := range additionalOwners { + if err := revokeRole(dbOwner.Name, additionalOwner, db); err != nil { + errors = append(errors, fmt.Sprintf("could not revoke %q from %q: %v", dbOwner.Name, additionalOwner, err)) + } + } + + if len(errors) > 0 { + return fmt.Errorf("could not resolve membership between %q and additional owner roles: %v", dbOwner.Name, strings.Join(errors, `', '`)) + } + + return nil +} + func (strategy DefaultUserSyncStrategy) alterPgUserSet(user spec.PgUser, db *sql.DB) error { queries := produceAlterRoleSetStmts(user) query := fmt.Sprintf(doBlockStmt, strings.Join(queries, ";")) @@ -272,6 +295,16 @@ func quoteMemberList(user spec.PgUser) string { return strings.Join(memberof, ",") } +func revokeRole(groupRole, role string, db *sql.DB) error { + revokeStmt := fmt.Sprintf(revokeFromUserSQL, groupRole, role) + + if _, err := db.Exec(fmt.Sprintf(doBlockStmt, revokeStmt)); err != nil { + return err + } + + return nil +} + // quoteVal quotes values to be used at ALTER ROLE SET param = value if necessary func quoteParameterValue(name, val string) string { start := val[0]