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

@@ -178,13 +178,18 @@ under the `users` key.
   `standby`.
 
 * **additional_owner_roles**
-  Specifies database roles that will become members of all database owners.
+  Specifies database roles that will be granted to all database owners. Owners
-  Then owners can use `SET ROLE` to obtain privileges of these roles to e.g.
+  can then use `SET ROLE` to obtain privileges of these roles to e.g. create
-  create/update functionality from extensions as part of a migration script.
+  or update functionality from extensions as part of a migration script. One
-  Note, that roles listed here should be preconfigured in the docker image
+  such role can be `cron_admin` which is provided by the Spilo docker image to
-  and already exist in the database cluster on startup. One such role can be
+  set up cron jobs inside the `postgres` database. In general, roles listed
-  `cron_admin` which is provided by the Spilo docker image to set up cron
+  here should be preconfigured in the docker image and already exist in the
-  jobs inside the `postgres` database. Default is `empty`.
+  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

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_CURRENT = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.3"
-SPILO_LAZY = "registry.opensource.zalan.do/acid/spilo-14-e2e:0.2"
+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)

pkg/cluster/cluster.go

@@ -134,7 +134,9 @@ func New(cfg Config, kubeClient k8sutil.KubernetesClient, pgSpec acidv1.Postgres
 			Endpoints: make(map[PostgresRole]*v1.Endpoints)},
 		userSyncStrategy: users.DefaultUserSyncStrategy{
 			PasswordEncryption:   passwordEncryption,
-			RoleDeletionSuffix: cfg.OpConfig.RoleDeletionSuffix},
+			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 {
+	if len(c.OpConfig.AdditionalOwnerRoles) == 0 {
-		// fetch all database owners the additional should become a member of
+		return
-		memberOf := make([]string, 0)
-		for username, pgUser := range c.pgUsers {
-			if pgUser.IsDbOwner {
-				memberOf = append(memberOf, username)
-			}
 	}
 
-		if len(memberOf) > 0 {
+	// fetch database owners and assign additional owner roles
-			namespace := c.Namespace
+	for username, pgUser := range c.pgUsers {
-			additionalOwnerPgUser := spec.PgUser{
+		if pgUser.IsDbOwner {
-				Origin:    spec.RoleOriginSpilo,
+			pgUser.MemberOf = append(pgUser.MemberOf, c.OpConfig.AdditionalOwnerRoles...)
-				MemberOf:  memberOf,
+			c.pgUsers[username] = pgUser
-				Name:      additionalOwner,
-				Namespace: namespace,
-			}
-			if currentRole, present := c.pgUsers[additionalOwner]; present {
-				c.pgUsers[additionalOwner] = c.resolveNameConflict(&currentRole, &additionalOwnerPgUser)
-			} else {
-				c.pgUsers[additionalOwner] = additionalOwnerPgUser
-			}
 		}
 	}
 }

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},
+		"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},
+		"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},
-		"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"}},
 	}
 
 	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
+	// now assign additional roles to owners
-	for manifestUser := range manifestUsers {
-		pgUser := cl.pgUsers[manifestUser]
-		pgUser.Password = manifestUser[0:1] + "123"
-		cl.pgUsers[manifestUser] = pgUser
-	}
-
 	cl.initAdditionalOwnerRoles()
 
-	for _, additionalOwnerRole := range cl.Config.OpConfig.AdditionalOwnerRoles {
+	// update passwords to compare with result
-		expectedPgUser := expectedUsers[additionalOwnerRole]
+	for username, existingPgUser := range cl.pgUsers {
-		existingPgUser, exists := cl.pgUsers[additionalOwnerRole]
+		expectedPgUser := expectedUsers[username]
-		if !exists {
-			t.Errorf("%s additional owner role %q not initilaized", testName, additionalOwnerRole)
-		}
 		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",
+			t.Errorf("%s unexpected membership of user %q: expected member of %#v, got member of %#v",
-				testName, additionalOwnerRole, expectedPgUser.MemberOf, existingPgUser.MemberOf)
+				testName, username, expectedPgUser.MemberOf, existingPgUser.MemberOf)
 		}
 	}
 }

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)
 		}

pkg/spec/types.go

@@ -30,7 +30,6 @@ const (
 	RoleOriginManifest
 	RoleOriginInfrastructure
 	RoleOriginTeamsAPI
-	RoleOriginSpilo
 	RoleOriginSystem
 	RoleOriginBootstrap
 	RoleConnectionPooler

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`
@@ -33,6 +34,7 @@ const (
 type DefaultUserSyncStrategy struct {
 	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 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 addNewRoles, equal := util.SubstractStringSlices(newUser.MemberOf, dbUser.MemberOf); !equal {
-				r.User.MemberOf = addNewRoles
-				r.Kind = spec.PGsyncUserAlter
-			}
 			if r.Kind == spec.PGsyncUserAlter {
 				r.User.Name = newUser.Name
 				reqs = append(reqs, r)
 			}
-			if newUser.Origin != spec.RoleOriginSpilo &&
+			if len(newUser.Parameters) > 0 &&
-				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]