Improve infrastructure role definitions (#208)

Enhance definitions of infrastructure roles by allowing membership in multiple roles, role options and per-role configuration to be specified in the infrastructure role configmap, which must have the same name as the infrastructure role secret. See manifests/infrastructure-roles-configmap.yaml for the examples and updated README for the description of different types of database roles supposed by the operator and their purposes.

Change the logic of merging infrastructure roles with the manifest roles when they have the same name, to return the infrastructure role unchanged instead of merging. Previously, we used to propagate flags from the manifest role to the resulting infrastructure one, as there were no way to define flags for the infrastructure role; however, this is not the case anymore.

Code review and tests by @erthalion

pkg/cluster/cluster.go

@@ -674,24 +674,18 @@ func (c *Cluster) initRobotUsers() error {
 		if err != nil {
 			return fmt.Errorf("invalid flags for user %q: %v", username, err)
 		}
-		if _, present := c.pgUsers[username]; !present {
-			c.pgUsers[username] = spec.PgUser{
-				Origin:   spec.RoleOriginManifest,
-				Name:     username,
-				Password: util.RandomPassword(constants.PasswordLength),
-				Flags:    flags,
-			}
+		newRole := spec.PgUser{
+			Origin:   spec.RoleOriginManifest,
+			Name:     username,
+			Password: util.RandomPassword(constants.PasswordLength),
+			Flags:    flags,
+		}
+		if currentRole, present := c.pgUsers[username]; present {
+			c.pgUsers[username] = c.resolveNameConflict(&currentRole, &newRole)
 		} else {
-			// avoid overwriting the password if the user is already there. The flags should be
-			// merged here, but since there is no mechanism to define them for non-robot roles
-			// they are assigned from the robot user.
-			c.logger.Debugf("merging manifest and infrastructure user %q data", username)
-			user := c.pgUsers[username]
-			user.Flags = flags
-			c.pgUsers[username] = user
+			c.pgUsers[username] = newRole
 		}
 	}
-
 	return nil
 }
 
@@ -715,41 +709,60 @@ func (c *Cluster) initHumanUsers() error {
 			}
 		}
 
-		if _, present := c.pgUsers[username]; present {
-			c.logger.Warnf("overwriting existing user %q with the data from the teams API", username)
-		}
-
-		c.pgUsers[username] = spec.PgUser{
+		newRole := spec.PgUser{
 			Origin:     spec.RoleOriginTeamsAPI,
 			Name:       username,
 			Flags:      flags,
 			MemberOf:   memberOf,
 			Parameters: c.OpConfig.TeamAPIRoleConfiguration,
 		}
+
+		if currentRole, present := c.pgUsers[username]; present {
+			c.pgUsers[username] = c.resolveNameConflict(&currentRole, &newRole)
+		} else {
+			c.pgUsers[username] = newRole
+		}
 	}
 
 	return nil
 }
 
 func (c *Cluster) initInfrastructureRoles() error {
-	// add infrastucture roles from the operator's definition
-	for username, data := range c.InfrastructureRoles {
+	// add infrastructure roles from the operator's definition
+	for username, newRole := range c.InfrastructureRoles {
 		if !isValidUsername(username) {
 			return fmt.Errorf("invalid username: '%v'", username)
 		}
 		if c.shouldAvoidProtectedOrSystemRole(username, "infrastructure role") {
 			continue
 		}
-		flags, err := normalizeUserFlags(data.Flags)
+		flags, err := normalizeUserFlags(newRole.Flags)
 		if err != nil {
 			return fmt.Errorf("invalid flags for user '%v': %v", username, err)
 		}
-		data.Flags = flags
-		c.pgUsers[username] = data
+		newRole.Flags = flags
+
+		if currentRole, present := c.pgUsers[username]; present {
+			c.pgUsers[username] = c.resolveNameConflict(&currentRole, &newRole)
+		} else {
+			c.pgUsers[username] = newRole
+		}
 	}
 	return nil
 }
 
+// resolves naming conflicts between existing and new roles by chosing either of them.
+func (c *Cluster) resolveNameConflict(currentRole, newRole *spec.PgUser) (result spec.PgUser) {
+	if newRole.Origin >= currentRole.Origin {
+		result = *newRole
+	} else {
+		result = *currentRole
+	}
+	c.logger.Debugf("resolved a conflict of role %q between %s and %s to %s",
+		newRole.Name, newRole.Origin, currentRole.Origin, result.Origin)
+	return
+}
+
 func (c *Cluster) shouldAvoidProtectedOrSystemRole(username, purpose string) bool {
 	if c.isProtectedUsername(username) {
 		c.logger.Warnf("cannot initialize a new %s with the name of the protected user %q", purpose, username)

pkg/cluster/cluster_test.go

@@ -33,9 +33,8 @@ func TestInitRobotUsers(t *testing.T) {
 	}{
 		{
 			manifestUsers: map[string]spec.UserFlags{"foo": {"superuser", "createdb"}},
-			infraRoles:    map[string]spec.PgUser{"foo": {Origin: spec.RoleOriginManifest, Name: "foo", Password: "bar"}},
-			result: map[string]spec.PgUser{"foo": {Origin: spec.RoleOriginManifest,
-				Name: "foo", Password: "bar", Flags: []string{"CREATEDB", "LOGIN", "SUPERUSER"}}},
+			infraRoles:    map[string]spec.PgUser{"foo": {Origin: spec.RoleOriginInfrastructure, Name: "foo", Password: "bar"}},
+			result: map[string]spec.PgUser{"foo": {Origin: spec.RoleOriginInfrastructure,  Name: "foo", Password: "bar"}},
 			err: nil,
 		},
 		{

pkg/controller/util.go

@@ -13,6 +13,7 @@ import (
 	"github.com/zalando-incubator/postgres-operator/pkg/util/config"
 	"github.com/zalando-incubator/postgres-operator/pkg/util/constants"
 	"github.com/zalando-incubator/postgres-operator/pkg/util/k8sutil"
+	"gopkg.in/yaml.v2"
 )
 
 func (c *Controller) makeClusterConfig() cluster.Config {
@@ -96,6 +97,14 @@ func (c *Controller) createCRD() error {
 	})
 }
 
+func readDecodedRole(s string) (*spec.PgUser, error) {
+	var result spec.PgUser
+	if err := yaml.Unmarshal([]byte(s), &result); err != nil {
+		return nil, fmt.Errorf("could not decode yaml role: %v", err)
+	}
+	return &result, nil
+}
+
 func (c *Controller) getInfrastructureRoles(rolesSecret *spec.NamespacedName) (result map[string]spec.PgUser, err error) {
 	if *rolesSecret == (spec.NamespacedName{}) {
 		// we don't have infrastructure roles defined, bail out
@@ -110,16 +119,16 @@ func (c *Controller) getInfrastructureRoles(rolesSecret *spec.NamespacedName) (r
 		return nil, fmt.Errorf("could not get infrastructure roles secret: %v", err)
 	}
 
-	data := infraRolesSecret.Data
+	secretData := infraRolesSecret.Data
 	result = make(map[string]spec.PgUser)
 Users:
 	// in worst case we would have one line per user
-	for i := 1; i <= len(data); i++ {
+	for i := 1; i <= len(secretData); i++ {
 		properties := []string{"user", "password", "inrole"}
 		t := spec.PgUser{Origin: spec.RoleOriginInfrastructure}
 		for _, p := range properties {
 			key := fmt.Sprintf("%s%d", p, i)
-			if val, present := data[key]; !present {
+			if val, present := secretData[key]; !present {
 				if p == "user" {
 					// exit when the user name with the next sequence id is absent
 					break Users
@@ -137,19 +146,48 @@ Users:
 					c.logger.Warningf("unknown key %q", p)
 				}
 			}
-
-			delete(data, key)
+			delete(secretData, key)
 		}
 
 		if t.Name != "" {
+			if t.Password == "" {
+				c.logger.Warningf("infrastructure role %q has no password defined and is ignored", t.Name)
+				continue
+			}
 			result[t.Name] = t
 		}
 	}
 
-	if len(data) != 0 {
-		c.logger.Warningf("%d unprocessed entries in the infrastructure roles' secret", len(data))
-		c.logger.Info(`infrastructure role entries should be in the {key}{id} format, where {key} can be either of "user", "password", "inrole" and the {id} a monotonically increasing integer starting with 1`)
-		c.logger.Debugf("unprocessed entries: %#v", data)
+	// perhaps we have some map entries with usernames, passwords, let's check if we have those users in the configmap
+	if infraRolesMap, err := c.KubeClient.ConfigMaps(rolesSecret.Namespace).Get(rolesSecret.Name, metav1.GetOptions{}); err == nil {
+		// we have a configmap with username - json description, let's read and decode it
+		for role, s := range infraRolesMap.Data {
+			if roleDescr, err := readDecodedRole(s); err != nil {
+				return nil, fmt.Errorf("could not decode role description: %v", err)
+			} else {
+				// check if we have a a password in a configmap
+				c.logger.Debugf("found role description for role %q: %+v", role, roleDescr)
+				if passwd, ok := secretData[role]; ok {
+					roleDescr.Password = string(passwd)
+					delete(secretData, role)
+				} else {
+					c.logger.Warningf("infrastructure role %q has no password defined and is ignored", role)
+					continue
+				}
+				roleDescr.Name = role
+				roleDescr.Origin = spec.RoleOriginInfrastructure
+				result[role] = *roleDescr
+			}
+		}
+	}
+
+	if len(secretData) > 0 {
+		c.logger.Warningf("%d unprocessed entries in the infrastructure roles secret,"+
+			" checking configmap %v", len(secretData), rolesSecret.Name)
+		c.logger.Info(`infrastructure role entries should be in the {key}{id} format,` +
+			` where {key} can be either of "user", "password", "inrole" and the {id}` +
+			` a monotonically increasing integer starting with 1`)
+		c.logger.Debugf("unprocessed entries: %#v", secretData)
 	}
 
 	return result, nil

pkg/controller/util_test.go

@@ -5,6 +5,7 @@ import (
 	"reflect"
 	"testing"
 
+	b64 "encoding/base64"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/pkg/api/v1"
@@ -21,6 +22,10 @@ type mockSecret struct {
 	v1core.SecretInterface
 }
 
+type mockConfigMap struct {
+	v1core.ConfigMapInterface
+}
+
 func (c *mockSecret) Get(name string, options metav1.GetOptions) (*v1.Secret, error) {
 	if name != testInfrastructureRolesSecretName {
 		return nil, fmt.Errorf("NotFound")
@@ -31,20 +36,43 @@ func (c *mockSecret) Get(name string, options metav1.GetOptions) (*v1.Secret, er
 		"user1":     []byte("testrole"),
 		"password1": []byte("testpassword"),
 		"inrole1":   []byte("testinrole"),
+		"foobar":    []byte(b64.StdEncoding.EncodeToString([]byte("password"))),
 	}
 	return secret, nil
 
 }
 
+func (c *mockConfigMap) Get(name string, options metav1.GetOptions) (*v1.ConfigMap, error) {
+	if name != testInfrastructureRolesSecretName {
+		return nil, fmt.Errorf("NotFound")
+	}
+	configmap := &v1.ConfigMap{}
+	configmap.Name = mockController.opConfig.ClusterNameLabel
+	configmap.Data = map[string]string{
+		"foobar": "{}",
+	}
+	return configmap, nil
+}
+
 type MockSecretGetter struct {
 }
 
+type MockConfigMapsGetter struct {
+}
+
 func (c *MockSecretGetter) Secrets(namespace string) v1core.SecretInterface {
 	return &mockSecret{}
 }
 
+func (c *MockConfigMapsGetter) ConfigMaps(namespace string) v1core.ConfigMapInterface {
+	return &mockConfigMap{}
+}
+
 func newMockKubernetesClient() k8sutil.KubernetesClient {
-	return k8sutil.KubernetesClient{SecretsGetter: &MockSecretGetter{}}
+	return k8sutil.KubernetesClient{
+		SecretsGetter:    &MockSecretGetter{},
+		ConfigMapsGetter: &MockConfigMapsGetter{},
+	}
 }
 
 func newMockController() *Controller {
@@ -135,6 +163,12 @@ func TestGetInfrastructureRoles(t *testing.T) {
 					Password: "testpassword",
 					MemberOf: []string{"testinrole"},
 				},
+				"foobar": {
+					Name:     "foobar",
+					Origin:   spec.RoleOriginInfrastructure,
+					Password: b64.StdEncoding.EncodeToString([]byte("password")),
+					MemberOf: nil,
+				},
 			},
 			nil,
 		},

pkg/spec/types.go

@@ -32,12 +32,14 @@ const (
 	fileWithNamespace = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"
 )
 
+// RoleOrigin contains the code of the origin of a role
 type RoleOrigin int
 
+// The rolesOrigin constant values should be sorted by the role priority.
 const (
-	RoleOriginUnknown = iota
-	RoleOriginInfrastructure
+	RoleOriginUnknown RoleOrigin = iota
 	RoleOriginManifest
+	RoleOriginInfrastructure
 	RoleOriginTeamsAPI
 	RoleOriginSystem
 )
@@ -72,12 +74,12 @@ type PodEvent struct {
 
 // PgUser contains information about a single user.
 type PgUser struct {
-	Origin     RoleOrigin
-	Name       string
-	Password   string
-	Flags      []string
-	MemberOf   []string
-	Parameters map[string]string
+	Origin     RoleOrigin        `yaml:"-"`
+	Name       string            `yaml:"-"`
+	Password   string            `yaml:"-"`
+	Flags      []string          `yaml:"user_flags"`
+	MemberOf   []string          `yaml:"inrole"`
+	Parameters map[string]string `yaml:"db_parameters"`
 }
 
 // PgUserMap maps user names to the definitions.
@@ -203,6 +205,20 @@ func (n *NamespacedName) DecodeWorker(value, operatorNamespace string) error {
 	return nil
 }
 
+func (r RoleOrigin) String() string {
+	switch r {
+	case RoleOriginManifest:
+		return "manifest role"
+	case RoleOriginInfrastructure:
+		return "infrastructure role"
+	case RoleOriginTeamsAPI:
+		return "teams API role"
+	case RoleOriginSystem:
+		return "system role"
+	}
+	return "unknown"
+}
+
 // GetOperatorNamespace assumes serviceaccount secret is mounted by kubernetes
 // Placing this func here instead of pgk/util avoids circular import
 func GetOperatorNamespace() string {