Configure pg_hba in the local postgresql configuration of Patroni. (#361)

Previously, the operator put pg_hba into the bootstrap/pg_hba key of
Patroni. That had 2 adverse effects:
 - pg_hba.conf was shadowed by Spilo default section in the local
   postgresql configuration
 - when updating pg_hba in the cluster manifest, the updated lines were
   not propagated to DCS, since the key was defined in the boostrap
   section of Patroni.

Include some minor refactoring, moving methods to unexported when
possible and commenting out usage of md5, so that gosec won't complain.

Per https://github.com/zalando-incubator/postgres-operator/issues/330

Review by @zerg-junior

pkg/cluster/cluster.go

@@ -119,7 +119,7 @@ func New(cfg Config, kubeClient k8sutil.KubernetesClient, pgSpec spec.Postgresql
 	}
 	cluster.logger = logger.WithField("pkg", "cluster").WithField("cluster-name", cluster.clusterName())
 	cluster.teamsAPIClient = teams.NewTeamsAPI(cfg.OpConfig.TeamsAPIUrl, logger)
-	cluster.oauthTokenGetter = NewSecretOauthTokenGetter(&kubeClient, cfg.OpConfig.OAuthTokenSecretName)
+	cluster.oauthTokenGetter = newSecretOauthTokenGetter(&kubeClient, cfg.OpConfig.OAuthTokenSecretName)
 	cluster.patroni = patroni.New(cluster.logger)
 
 	return cluster
@@ -404,15 +404,15 @@ func (c *Cluster) compareStatefulSetWith(statefulSet *v1beta1.StatefulSet) *comp
 	return &compareStatefulsetResult{match: match, reasons: reasons, rollingUpdate: needsRollUpdate, replace: needsReplace}
 }
 
-type ContainerCondition func(a, b v1.Container) bool
+type containerCondition func(a, b v1.Container) bool
 
-type ContainerCheck struct {
+type containerCheck struct {
-	condition ContainerCondition
+	condition containerCondition
 	reason    string
 }
 
-func NewCheck(msg string, cond ContainerCondition) ContainerCheck {
+func newCheck(msg string, cond containerCondition) containerCheck {
-	return ContainerCheck{reason: msg, condition: cond}
+	return containerCheck{reason: msg, condition: cond}
 }
 
 // compareContainers: compare containers from two stateful sets
@@ -422,18 +422,18 @@ func NewCheck(msg string, cond ContainerCondition) ContainerCheck {
 func (c *Cluster) compareContainers(setA, setB *v1beta1.StatefulSet) (bool, []string) {
 	reasons := make([]string, 0)
 	needsRollUpdate := false
-	checks := []ContainerCheck{
+	checks := []containerCheck{
-		NewCheck("new statefulset's container %d name doesn't match the current one",
+		newCheck("new statefulset's container %d name doesn't match the current one",
 			func(a, b v1.Container) bool { return a.Name != b.Name }),
-		NewCheck("new statefulset's container %d image doesn't match the current one",
+		newCheck("new statefulset's container %d image doesn't match the current one",
 			func(a, b v1.Container) bool { return a.Image != b.Image }),
-		NewCheck("new statefulset's container %d ports don't match the current one",
+		newCheck("new statefulset's container %d ports don't match the current one",
 			func(a, b v1.Container) bool { return !reflect.DeepEqual(a.Ports, b.Ports) }),
-		NewCheck("new statefulset's container %d resources don't match the current ones",
+		newCheck("new statefulset's container %d resources don't match the current ones",
 			func(a, b v1.Container) bool { return !compareResources(&a.Resources, &b.Resources) }),
-		NewCheck("new statefulset's container %d environment doesn't match the current one",
+		newCheck("new statefulset's container %d environment doesn't match the current one",
 			func(a, b v1.Container) bool { return !reflect.DeepEqual(a.Env, b.Env) }),
-		NewCheck("new statefulset's container %d environment sources don't match the current one",
+		newCheck("new statefulset's container %d environment sources don't match the current one",
 			func(a, b v1.Container) bool { return !reflect.DeepEqual(a.EnvFrom, b.EnvFrom) }),
 	}
 
@@ -630,6 +630,7 @@ func (c *Cluster) Delete() {
 	}
 }
 
+//NeedsRepair returns true if the cluster should be included in the repair scan (based on its in-memory status).
 func (c *Cluster) NeedsRepair() (bool, spec.PostgresStatus) {
 	c.specMu.RLock()
 	defer c.specMu.RUnlock()
@@ -905,9 +906,9 @@ func (c *Cluster) shouldDeleteSecret(secret *v1.Secret) (delete bool, userName s
 
 type simpleActionWithResult func() error
 
-type ClusterObjectGet func(name string) (spec.NamespacedName, error)
+type clusterObjectGet func(name string) (spec.NamespacedName, error)
 
-type ClusterObjectDelete func(name string) error
+type clusterObjectDelete func(name string) error
 
 func (c *Cluster) deletePatroniClusterObjects() error {
 	// TODO: figure out how to remove leftover patroni objects in other cases
@@ -924,8 +925,8 @@ func (c *Cluster) deletePatroniClusterObjects() error {
 }
 
 func (c *Cluster) deleteClusterObject(
-	get ClusterObjectGet,
+	get clusterObjectGet,
-	del ClusterObjectDelete,
+	del clusterObjectDelete,
 	objType string) error {
 	for _, suffix := range patroniObjectSuffixes {
 		name := fmt.Sprintf("%s-%s", c.Name, suffix)

pkg/cluster/k8sres.go

@@ -25,6 +25,7 @@ const (
 	pgBinariesLocationTemplate       = "/usr/lib/postgresql/%s/bin"
 	patroniPGBinariesParameterName   = "bin_dir"
 	patroniPGParametersParameterName = "parameters"
+	patroniPGHBAConfParameterName    = "pg_hba"
 	localHost                        = "127.0.0.1/32"
 )
 
@@ -44,7 +45,6 @@ type patroniDCS struct {
 type pgBootstrap struct {
 	Initdb []interface{}     `json:"initdb"`
 	Users  map[string]pgUser `json:"users"`
-	PgHBA  []string          `json:"pg_hba"`
 	DCS    patroniDCS        `json:"dcs,omitempty"`
 }
 
@@ -202,19 +202,6 @@ PatroniInitDBParams:
 		config.Bootstrap.Initdb = append(config.Bootstrap.Initdb, map[string]string{k: v})
 	}
 
-	// pg_hba parameters in the manifest replace the default ones. We cannot
-	// reasonably merge them automatically, because pg_hba parsing stops on
-	// a first successfully matched rule.
-	if len(patroni.PgHba) > 0 {
-		config.Bootstrap.PgHBA = patroni.PgHba
-	} else {
-		config.Bootstrap.PgHBA = []string{
-			"hostnossl all all all reject",
-			fmt.Sprintf("hostssl   all +%s all pam", pamRoleName),
-			"hostssl   all all all md5",
-		}
-	}
-
 	if patroni.MaximumLagOnFailover >= 0 {
 		config.Bootstrap.DCS.MaximumLagOnFailover = patroni.MaximumLagOnFailover
 	}
@@ -231,23 +218,23 @@ PatroniInitDBParams:
 	config.PgLocalConfiguration = make(map[string]interface{})
 	config.PgLocalConfiguration[patroniPGBinariesParameterName] = fmt.Sprintf(pgBinariesLocationTemplate, pg.PgVersion)
 	if len(pg.Parameters) > 0 {
-		localParameters := make(map[string]string)
+		local, bootstrap := getLocalAndBoostrapPostgreSQLParameters(pg.Parameters)
-		bootstrapParameters := make(map[string]string)
+
-		for param, val := range pg.Parameters {
+		if len(local) > 0 {
-			if isBootstrapOnlyParameter(param) {
+			config.PgLocalConfiguration[patroniPGParametersParameterName] = local
-				bootstrapParameters[param] = val
-			} else {
-				localParameters[param] = val
 		}
-		}
+		if len(bootstrap) > 0 {
-		if len(localParameters) > 0 {
-			config.PgLocalConfiguration[patroniPGParametersParameterName] = localParameters
-		}
-		if len(bootstrapParameters) > 0 {
 			config.Bootstrap.DCS.PGBootstrapConfiguration = make(map[string]interface{})
-			config.Bootstrap.DCS.PGBootstrapConfiguration[patroniPGParametersParameterName] = bootstrapParameters
+			config.Bootstrap.DCS.PGBootstrapConfiguration[patroniPGParametersParameterName] = bootstrap
 		}
 	}
+	// Patroni gives us a choice of writing pg_hba.conf to either the bootstrap section or to the local postgresql one.
+	// We choose the local one, because we need Patroni to change pg_hba.conf in PostgreSQL after the user changes the
+	// relevant section in the manifest.
+	if len(patroni.PgHba) > 0 {
+		config.PgLocalConfiguration[patroniPGHBAConfParameterName] = patroni.PgHba
+	}
+
 	config.Bootstrap.Users = map[string]pgUser{
 		pamRoleName: {
 			Password: "",
@@ -262,6 +249,19 @@ PatroniInitDBParams:
 	return string(result)
 }
 
+func getLocalAndBoostrapPostgreSQLParameters(parameters map[string]string) (local, bootstrap map[string]string) {
+	local = make(map[string]string)
+	bootstrap = make(map[string]string)
+	for param, val := range parameters {
+		if isBootstrapOnlyParameter(param) {
+			bootstrap[param] = val
+		} else {
+			local[param] = val
+		}
+	}
+	return
+}
+
 func nodeAffinity(nodeReadinessLabel map[string]string) *v1.Affinity {
 	matchExpressions := make([]v1.NodeSelectorRequirement, 0)
 	if len(nodeReadinessLabel) == 0 {
@@ -736,7 +736,7 @@ func (c *Cluster) generateStatefulSet(spec *spec.PostgresSpec) (*v1beta1.Statefu
 			Name:        c.statefulSetName(),
 			Namespace:   c.Namespace,
 			Labels:      c.labelsSet(true),
-			Annotations: map[string]string{RollingUpdateStatefulsetAnnotationKey: "false"},
+			Annotations: map[string]string{rollingUpdateStatefulsetAnnotationKey: "false"},
 		},
 		Spec: v1beta1.StatefulSetSpec{
 			Replicas:             &numberOfInstances,

pkg/cluster/resources.go

@@ -18,7 +18,7 @@ import (
 )
 
 const (
-	RollingUpdateStatefulsetAnnotationKey = "zalando-postgres-operator-rolling-update-required"
+	rollingUpdateStatefulsetAnnotationKey = "zalando-postgres-operator-rolling-update-required"
 )
 
 func (c *Cluster) listResources() error {
@@ -140,7 +140,7 @@ func (c *Cluster) setRollingUpdateFlagForStatefulSet(sset *v1beta1.StatefulSet,
 	if anno == nil {
 		anno = make(map[string]string)
 	}
-	anno[RollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(val)
+	anno[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(val)
 	sset.SetAnnotations(anno)
 }
 
@@ -162,12 +162,12 @@ func (c *Cluster) getRollingUpdateFlagFromStatefulSet(sset *v1beta1.StatefulSet,
 	anno := sset.GetAnnotations()
 	flag = defaultValue
 
-	stringFlag, exists := anno[RollingUpdateStatefulsetAnnotationKey]
+	stringFlag, exists := anno[rollingUpdateStatefulsetAnnotationKey]
 	if exists {
 		var err error
 		if flag, err = strconv.ParseBool(stringFlag); err != nil {
 			c.logger.Warnf("error when parsing %q annotation for the statefulset %q: expected boolean value, got %q\n",
-				RollingUpdateStatefulsetAnnotationKey,
+				rollingUpdateStatefulsetAnnotationKey,
 				types.NamespacedName{Namespace: sset.Namespace, Name: sset.Name},
 				stringFlag)
 			flag = defaultValue

pkg/cluster/util.go

@@ -35,7 +35,7 @@ type SecretOauthTokenGetter struct {
 	OAuthTokenSecretName spec.NamespacedName
 }
 
-func NewSecretOauthTokenGetter(kubeClient *k8sutil.KubernetesClient,
+func newSecretOauthTokenGetter(kubeClient *k8sutil.KubernetesClient,
 	OAuthTokenSecretName spec.NamespacedName) *SecretOauthTokenGetter {
 	return &SecretOauthTokenGetter{kubeClient, OAuthTokenSecretName}
 }

pkg/spec/postgresql.go

@@ -71,6 +71,7 @@ type Sidecar struct {
 	Env         []v1.EnvVar        `json:"env,omitempty"`
 }
 
+// UserFlags defines flags (such as superuser, nologin) that could be assigned to individual users
 type UserFlags []string
 
 // PostgresStatus contains status of the PostgreSQL cluster (running, creation failed etc.)

pkg/util/util.go

@@ -1,7 +1,7 @@
 package util
 
 import (
-	"crypto/md5"
+	"crypto/md5" // #nosec we need it to for PostgreSQL md5 passwords
 	"encoding/hex"
 	"math/rand"
 	"regexp"
@@ -48,7 +48,7 @@ func PGUserPassword(user spec.PgUser) string {
 		// Avoid processing already encrypted or empty passwords
 		return user.Password
 	}
-	s := md5.Sum([]byte(user.Password + user.Name))
+	s := md5.Sum([]byte(user.Password + user.Name)) // #nosec, using md5 since PostgreSQL uses it for hashing passwords.
 	return md5prefix + hex.EncodeToString(s[:])
 }
 
@@ -120,6 +120,7 @@ func MapContains(haystack, needle map[string]string) bool {
 	return true
 }
 
+// Coalesce returns the first argument if it is not null, otherwise the second one.
 func Coalesce(val, defaultVal string) string {
 	if val == "" {
 		return defaultVal