Merge pull request #258 from zalando-incubator/always-create-replica-service

[WIP] Always create replica service

.travis.yml

@@ -17,4 +17,4 @@ install:
   - make deps
 
 script:
-  - travis_wait 20 goveralls -service=travis-ci -package ./pkg/... -package ./cmd/... -v
+  - travis_wait 20 goveralls -service=travis-ci -package ./pkg/... -v

README.md

@@ -242,6 +242,14 @@ As a preventive measure, one can restrict the minimum and the maximum number of
 If either `min_instances` or `max_instances` is set to a non-zero value, the operator may adjust the number of instances specified in the cluster manifest to match either the min or the max boundary.
 For instance, of a cluster manifest has 1 instance and the min_instances is set to 3, the cluster will be created with 3 instances. By default, both parameters are set to -1.
 
+### Load balancers 
+
+For any Postgresql/Spilo cluster an operator creates two separate k8s services: one for the master pod and one for replica pods. To expose these services to an outer network, one can attach load balancers to them by setting `enableMasterLoadBalancer` and/or `enableReplicaLoadBalancer` to `true` in the cluster manifest. In the case any of these variables is omitted from the manifest, the operator configmap's settings `enable_master_load_balancer` and `enable_replica_load_balancer` apply. Note that the operator settings affect all Postgresql services running in a namespace watched by the operator.
+
+For backward compatibility with already configured clusters we maintain in a cluster manifest older parameter names, namely `useLoadBalancer` for enabling the master service's load balancer and `replicaLoadBalancer` for the replica service. If set, these params take precedence over the newer `enableMasterLoadBalancer` and `enableReplicaLoadBalancer`. Note that in older versions of the operator (before PR #258) `replicaLoadBalancer` was responsible for both creating the replica service and attaching an LB to it; now the service is always created (since k8s service typically is free in the cloud setting), and this param only attaches an LB (that typically costs money).
+
+For the same reason of compatibility, we maintain the `enable_load_balancer` setting in the operator config map that was previously used to attach a LB to the master service. Its value is examined after the deprecated `useLoadBalancer` setting from the Postgresql manifest but before the recommended `enableMasterLoadBalancer`. There is no equivalent option for the replica service since the service used to be always created with a load balancer.
+
 # Setup development environment
 
 The following steps guide you through the setup to work on the operator itself.

delivery.yaml

@@ -6,6 +6,9 @@ pipeline:
         GOPATH: /root/go
         OPERATOR_TOP_DIR: /root/go/src/github.com/zalando-incubator
       commands:
+        - desc: 'Update'
+          cmd: |
+            apt-get update
         - desc: 'Install required build software'
           cmd: |
             apt-get install -y make git apt-transport-https ca-certificates curl

manifests/complete-postgres-manifest.yaml

@@ -12,8 +12,9 @@ spec:
     zalando:
     - superuser
     - createdb
-  useLoadBalancer: true
+  enableMasterLoadBalancer: true
-  allowedSourceRanges: #Load balancer source ranges
+  enableReplicaLoadBalancer: true 
+  allowedSourceRanges: # load balancers' source ranges for both master and replica services
   - 127.0.0.1/32
   databases:
     foo: zalando

manifests/configmap.yaml

@@ -37,7 +37,10 @@ data:
   team_admin_role: "admin"
   teams_api_url: http://fake-teams-api.default.svc.cluster.local
   workers: "4"
-  enable_load_balancer: "true"
+  # turn on/off load balancers for all Postgres clusters managed by the operator
+  # LB settings in cluster manifests take priority over these settings
+  enable_master_load_balancer: "true"
+  enable_replica_load_balancer: "false"
   api_port: "8080"
   ring_log_lines: "100"
   cluster_history_entries: "1000"

pkg/cluster/cluster.go

@@ -217,9 +217,7 @@ func (c *Cluster) Create() error {
 	c.setStatus(spec.ClusterStatusCreating)
 
 	for _, role := range []PostgresRole{Master, Replica} {
-		if role == Replica && !c.Spec.ReplicaLoadBalancer {
+
-			continue
-		}
 		if c.Endpoints[role] != nil {
 			return fmt.Errorf("%s endpoint already exists in the cluster", role)
 		}
@@ -480,8 +478,7 @@ func (c *Cluster) Update(oldSpec, newSpec *spec.Postgresql) error {
 
 	// Service
 	if !reflect.DeepEqual(c.generateService(Master, &oldSpec.Spec), c.generateService(Master, &newSpec.Spec)) ||
-		!reflect.DeepEqual(c.generateService(Replica, &oldSpec.Spec), c.generateService(Replica, &newSpec.Spec)) ||
+		!reflect.DeepEqual(c.generateService(Replica, &oldSpec.Spec), c.generateService(Replica, &newSpec.Spec)) {
-		oldSpec.Spec.ReplicaLoadBalancer != newSpec.Spec.ReplicaLoadBalancer {
 		c.logger.Debugf("syncing services")
 		if err := c.syncServices(); err != nil {
 			c.logger.Errorf("could not sync services: %v", err)
@@ -589,9 +586,6 @@ func (c *Cluster) Delete() error {
 	}
 
 	for _, role := range []PostgresRole{Master, Replica} {
-		if role == Replica && !c.Spec.ReplicaLoadBalancer {
-			continue
-		}
 
 		if err := c.deleteEndpoint(role); err != nil {
 			return fmt.Errorf("could not delete %s endpoint: %v", role, err)

pkg/cluster/k8sres.go

@@ -669,6 +669,48 @@ func (c *Cluster) generateSingleUserSecret(namespace string, pgUser spec.PgUser)
 	return &secret
 }
 
+func (c *Cluster) shouldCreateLoadBalancerForService(role PostgresRole, spec *spec.PostgresSpec) bool {
+
+	switch role {
+
+	case Replica:
+
+		// deprecated option takes priority for backward compatibility
+		if spec.ReplicaLoadBalancer != nil {
+			c.logger.Debugf("The Postgres manifest for the cluster %v sets the deprecated `replicaLoadBalancer` param. Consider using the `enableReplicaLoadBalancer` instead.", c.Name)
+			return *spec.ReplicaLoadBalancer
+		}
+
+		// if the value is explicitly set in a Postgresql manifest, follow this setting
+		if spec.EnableReplicaLoadBalancer != nil {
+			return *spec.EnableReplicaLoadBalancer
+		}
+
+		// otherwise, follow the operator configuration
+		return c.OpConfig.EnableReplicaLoadBalancer
+
+	case Master:
+
+		if spec.UseLoadBalancer != nil {
+			c.logger.Debugf("The Postgres manifest for the cluster %v sets the deprecated `useLoadBalancer` param. Consider using the `enableMasterLoadBalancer` instead.", c.Name)
+			return *spec.UseLoadBalancer
+		}
+
+		// `enable_load_balancer`` governs LB for a master service
+		// there is no equivalent deprecated operator option for the replica LB
+		if c.OpConfig.EnableLoadBalancer != nil {
+			c.logger.Debugf("The operator configmap sets the deprecated `enable_load_balancer` param. Consider using the `enable_master_load_balancer` or `enable_replica_load_balancer` instead.", c.Name)
+			return *c.OpConfig.EnableLoadBalancer
+		}
+
+		return c.OpConfig.EnableMasterLoadBalancer
+
+	default:
+		panic(fmt.Sprintf("Unknown role %v", role))
+	}
+
+}
+
 func (c *Cluster) generateService(role PostgresRole, spec *spec.PostgresSpec) *v1.Service {
 	var dnsName string
 
@@ -689,12 +731,11 @@ func (c *Cluster) generateService(role PostgresRole, spec *spec.PostgresSpec) *v
 
 	var annotations map[string]string
 
-	// Examine the per-cluster load balancer setting, if it is not defined - check the operator configuration.
+	if c.shouldCreateLoadBalancerForService(role, spec) {
-	if (spec.UseLoadBalancer != nil && *spec.UseLoadBalancer) ||
-		(spec.UseLoadBalancer == nil && c.OpConfig.EnableLoadBalancer) {
 
 		// safe default value: lock load balancer to only local address unless overridden explicitly.
 		sourceRanges := []string{localHost}
+
 		allowedSourceRanges := spec.AllowedSourceRanges
 		if len(allowedSourceRanges) >= 0 {
 			sourceRanges = allowedSourceRanges
@@ -707,6 +748,10 @@ func (c *Cluster) generateService(role PostgresRole, spec *spec.PostgresSpec) *v
 			constants.ZalandoDNSNameAnnotation: dnsName,
 			constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
 		}
+	} else if role == Replica {
+		// before PR #258, the replica service was only created if allocated a LB
+		// now we always create the service but warn if the LB is absent
+		c.logger.Debugf("No load balancer created for the replica service")
 	}
 
 	service := &v1.Service{

pkg/cluster/k8sres_test.go

@@ -0,0 +1,94 @@
+package cluster
+
+import (
+	"github.com/zalando-incubator/postgres-operator/pkg/spec"
+	"github.com/zalando-incubator/postgres-operator/pkg/util/config"
+	"github.com/zalando-incubator/postgres-operator/pkg/util/k8sutil"
+	"testing"
+)
+
+func True() *bool {
+	b := true
+	return &b
+}
+
+func False() *bool {
+	b := false
+	return &b
+}
+
+func TestCreateLoadBalancerLogic(t *testing.T) {
+	var cluster = New(
+		Config{
+			OpConfig: config.Config{
+				ProtectedRoles: []string{"admin"},
+				Auth: config.Auth{
+					SuperUsername:       superUserName,
+					ReplicationUsername: replicationUserName,
+				},
+			},
+		}, k8sutil.KubernetesClient{}, spec.Postgresql{}, logger)
+
+	testName := "TestCreateLoadBalancerLogic"
+	tests := []struct {
+		subtest  string
+		role     PostgresRole
+		spec     *spec.PostgresSpec
+		opConfig config.Config
+		result   bool
+	}{
+		{
+			subtest:  "new format, load balancer is enabled for replica",
+			role:     Replica,
+			spec:     &spec.PostgresSpec{EnableReplicaLoadBalancer: True()},
+			opConfig: config.Config{},
+			result:   true,
+		},
+		{
+			subtest:  "new format, load balancer is disabled for replica",
+			role:     Replica,
+			spec:     &spec.PostgresSpec{EnableReplicaLoadBalancer: False()},
+			opConfig: config.Config{},
+			result:   false,
+		},
+		{
+			subtest:  "new format, load balancer isn't specified for replica",
+			role:     Replica,
+			spec:     &spec.PostgresSpec{EnableReplicaLoadBalancer: nil},
+			opConfig: config.Config{EnableReplicaLoadBalancer: true},
+			result:   true,
+		},
+		{
+			subtest:  "new format, load balancer isn't specified for replica",
+			role:     Replica,
+			spec:     &spec.PostgresSpec{EnableReplicaLoadBalancer: nil},
+			opConfig: config.Config{EnableReplicaLoadBalancer: false},
+			result:   false,
+		},
+		{
+			subtest:  "old format, load balancer is enabled for replica",
+			role:     Replica,
+			spec:     &spec.PostgresSpec{ReplicaLoadBalancer: True()},
+			opConfig: config.Config{},
+			result:   true,
+		},
+		{
+			subtest: "old format has priority",
+			role:    Replica,
+			spec: &spec.PostgresSpec{
+				ReplicaLoadBalancer:       True(),
+				EnableReplicaLoadBalancer: False(),
+			},
+			opConfig: config.Config{},
+			result:   true,
+		},
+	}
+	for _, tt := range tests {
+		cluster.OpConfig = tt.opConfig
+		result := cluster.shouldCreateLoadBalancerForService(tt.role, tt.spec)
+		if tt.result != result {
+			t.Errorf("%s %s: Load balancer is %t, expect %t for role %#v and spec %#v",
+				testName, tt.subtest, result, tt.result, tt.role, tt.spec)
+		}
+	}
+}

pkg/cluster/sync.go

@@ -108,11 +108,6 @@ func (c *Cluster) syncService(role PostgresRole) error {
 
 	svc, err := c.KubeClient.Services(c.Namespace).Get(c.serviceName(role), metav1.GetOptions{})
 	if err == nil {
-		if role == Replica && !c.Spec.ReplicaLoadBalancer {
-			if err := c.deleteService(role); err != nil {
-				return fmt.Errorf("could not delete %s service", role)
-			}
-		}
 
 		desiredSvc := c.generateService(role, &c.Spec)
 		match, reason := k8sutil.SameService(svc, desiredSvc)
@@ -133,11 +128,6 @@ func (c *Cluster) syncService(role PostgresRole) error {
 	}
 	c.Services[role] = nil
 
-	// Service does not exist
-	if role == Replica && !c.Spec.ReplicaLoadBalancer {
-		return nil
-	}
-
 	c.logger.Infof("could not find the cluster's %s service", role)
 
 	if svc, err := c.createService(role); err != nil {
@@ -165,11 +155,6 @@ func (c *Cluster) syncEndpoint(role PostgresRole) error {
 
 	ep, err := c.KubeClient.Endpoints(c.Namespace).Get(c.endpointName(role), metav1.GetOptions{})
 	if err == nil {
-		if role == Replica && !c.Spec.ReplicaLoadBalancer {
-			if err := c.deleteEndpoint(role); err != nil {
-				return fmt.Errorf("could not delete %s endpoint", role)
-			}
-		}
 
 		c.Endpoints[role] = ep
 		return nil
@@ -178,10 +163,6 @@ func (c *Cluster) syncEndpoint(role PostgresRole) error {
 	}
 	c.Endpoints[role] = nil
 
-	if role == Replica && !c.Spec.ReplicaLoadBalancer {
-		return nil
-	}
-
 	c.logger.Infof("could not find the cluster's %s endpoint", role)
 
 	if ep, err := c.createEndpoint(role); err != nil {

pkg/spec/postgresql.go

@@ -93,19 +93,29 @@ type PostgresSpec struct {
 	Patroni         `json:"patroni,omitempty"`
 	Resources       `json:"resources,omitempty"`
 
-	TeamID              string   `json:"teamId"`
+	TeamID      string `json:"teamId"`
+	DockerImage string `json:"dockerImage,omitempty"`
+
+	// vars that enable load balancers are pointers because it is important to know if any of them is omitted from the Postgres manifest
+	// in that case the var evaluates to nil and the value is taken from the operator config
+	EnableMasterLoadBalancer  *bool `json:"enableMasterLoadBalancer,omitempty"`
+	EnableReplicaLoadBalancer *bool `json:"enableReplicaLoadBalancer,omitempty"`
+
+	// deprecated load balancer settings mantained for backward compatibility
+	// see "Load balancers" operator docs
+	UseLoadBalancer     *bool `json:"useLoadBalancer,omitempty"`
+	ReplicaLoadBalancer *bool `json:"replicaLoadBalancer,omitempty"`
+
+	// load balancers' source ranges are the same for master and replica services
 	AllowedSourceRanges []string `json:"allowedSourceRanges"`
-	DockerImage         string   `json:"dockerImage,omitempty"`
+
-	// EnableLoadBalancer  is a pointer, since it is important to know if that parameters is omitted from the manifest
+	NumberOfInstances  int32                `json:"numberOfInstances"`
-	UseLoadBalancer     *bool                `json:"useLoadBalancer,omitempty"`
+	Users              map[string]UserFlags `json:"users"`
-	ReplicaLoadBalancer bool                 `json:"replicaLoadBalancer,omitempty"`
+	MaintenanceWindows []MaintenanceWindow  `json:"maintenanceWindows,omitempty"`
-	NumberOfInstances   int32                `json:"numberOfInstances"`
+	Clone              CloneDescription     `json:"clone"`
-	Users               map[string]UserFlags `json:"users"`
+	ClusterName        string               `json:"-"`
-	MaintenanceWindows  []MaintenanceWindow  `json:"maintenanceWindows,omitempty"`
+	Databases          map[string]string    `json:"databases,omitempty"`
-	Clone               CloneDescription     `json:"clone"`
+	Tolerations        []v1.Toleration      `json:"tolerations,omitempty"`
-	ClusterName         string               `json:"-"`
-	Databases           map[string]string    `json:"databases,omitempty"`
-	Tolerations         []v1.Toleration      `json:"tolerations,omitempty"`
 }
 
 // PostgresqlList defines a list of PostgreSQL clusters.

pkg/util/config/config.go

@@ -67,20 +67,23 @@ type Config struct {
 	Resources
 	Auth
 	Scalyr
-	WatchedNamespace         string            `name:"watched_namespace"` // special values: "*" means 'watch all namespaces', the empty string "" means 'watch a namespace where operator is deployed to'
+	WatchedNamespace          string `name:"watched_namespace"` // special values: "*" means 'watch all namespaces', the empty string "" means 'watch a namespace where operator is deployed to'
-	EtcdHost                 string            `name:"etcd_host" default:"etcd-client.default.svc.cluster.local:2379"`
+	EtcdHost                  string `name:"etcd_host" default:"etcd-client.default.svc.cluster.local:2379"`
-	DockerImage              string            `name:"docker_image" default:"registry.opensource.zalan.do/acid/spiloprivate-9.6:1.2-p4"`
+	DockerImage               string `name:"docker_image" default:"registry.opensource.zalan.do/acid/spiloprivate-9.6:1.2-p4"`
-	ServiceAccountName       string            `name:"service_account_name" default:"operator"`
+	ServiceAccountName        string `name:"service_account_name" default:"operator"`
-	DbHostedZone             string            `name:"db_hosted_zone" default:"db.example.com"`
+	DbHostedZone              string `name:"db_hosted_zone" default:"db.example.com"`
-	EtcdScope                string            `name:"etcd_scope" default:"service"`
+	EtcdScope                 string `name:"etcd_scope" default:"service"`
-	WALES3Bucket             string            `name:"wal_s3_bucket"`
+	WALES3Bucket              string `name:"wal_s3_bucket"`
-	KubeIAMRole              string            `name:"kube_iam_role"`
+	KubeIAMRole               string `name:"kube_iam_role"`
-	DebugLogging             bool              `name:"debug_logging" default:"true"`
+	DebugLogging              bool   `name:"debug_logging" default:"true"`
-	EnableDBAccess           bool              `name:"enable_database_access" default:"true"`
+	EnableDBAccess            bool   `name:"enable_database_access" default:"true"`
-	EnableTeamsAPI           bool              `name:"enable_teams_api" default:"true"`
+	EnableTeamsAPI            bool   `name:"enable_teams_api" default:"true"`
-	EnableTeamSuperuser      bool              `name:"enable_team_superuser" default:"false"`
+	EnableTeamSuperuser       bool   `name:"enable_team_superuser" default:"false"`
-	TeamAdminRole            string            `name:"team_admin_role" default:"admin"`
+	TeamAdminRole             string `name:"team_admin_role" default:"admin"`
-	EnableLoadBalancer       bool              `name:"enable_load_balancer" default:"true"`
+	EnableMasterLoadBalancer  bool   `name:"enable_master_load_balancer" default:"true"`
+	EnableReplicaLoadBalancer bool   `name:"enable_replica_load_balancer" default:"false"`
+	// deprecated and kept for backward compatibility
+	EnableLoadBalancer       *bool             `name:"enable_load_balancer" default:"true"`
 	MasterDNSNameFormat      stringTemplate    `name:"master_dns_name_format" default:"{cluster}.{team}.{hostedzone}"`
 	ReplicaDNSNameFormat     stringTemplate    `name:"replica_dns_name_format" default:"{cluster}-repl.{team}.{hostedzone}"`
 	PDBNameFormat            stringTemplate    `name:"pdb_name_format" default:"postgres-{cluster}-pdb"`