Deprecate old LB options, fix endpoint sync. (#287)

* Depreate old LB options, fix endpoint sync.

- deprecate useLoadBalancer, replicaLoadBalancer from the manifest
  and enable_load_balancer from the operator configuration. The old
  operator configuration options become no-op with this commit. For
  the old manifest options, `useLoadBalancer` and `replicaLoadBalancer`
  are still consulted,  but only in the absense of the new ones
  (enableMasterLoadBalancer and enableReplicaLoadBalancer).

- Make sure the endpoint being created during the sync receives proper
  addresses subset. This is more critical for the replicas, as for the
  masters Patroni will normally re-create the endpoint before the
  operator.

- Avoid creating the replica endpoint, since it will be created automatically
  by the corresponding service.
- Update the README and unit tests.

Code review by @mgomezch and @zerg-junior

README.md

@@ -276,11 +276,21 @@ For instance, of a cluster manifest has 1 instance and the min_instances is set
 
 ### 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 any Postgresql/Spilo cluster, the 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 are 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).
+###### Deprecated parameters
 
-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.
+Parameters `useLoadBalancer` and `replicaLoadBalancer` in the PostgreSQL manifest are deprecated. To retain
+compatibility with the old manifests they take affect in the absense of new `enableMasterLoadBalancer` and
+`enableReplicaLoadBalancer` parameters (that is, if either of the new ones is present - all deprecated parameters are
+ignored). The operator configuration parameter `enable_load_balancer` is ignored in all cases.
+
+`
 
 # Setup development environment
 

pkg/cluster/cluster.go

@@ -170,6 +170,10 @@ func (c *Cluster) setStatus(status spec.PostgresStatus) {
 	}
 }
 
+func (c *Cluster) isNewCluster() bool {
+	return c.Status == spec.ClusterStatusCreating
+}
+
 // initUsers populates c.systemUsers and c.pgUsers maps.
 func (c *Cluster) initUsers() error {
 	c.setProcessName("initializing users")
@@ -255,11 +259,15 @@ func (c *Cluster) Create() error {
 		if c.Endpoints[role] != nil {
 			return fmt.Errorf("%s endpoint already exists in the cluster", role)
 		}
+		if role == Master {
+			// replica endpoint will be created by the replica service. Master endpoint needs to be created by us,
+			// since the corresponding master service doesn't define any selectors.
 			ep, err = c.createEndpoint(role)
 			if err != nil {
 				return fmt.Errorf("could not create %s endpoint: %v", role, err)
 			}
 			c.logger.Infof("endpoint %q has been successfully created", util.NameFromMeta(ep.ObjectMeta))
+		}
 
 		if c.Services[role] != nil {
 			return fmt.Errorf("service already exists in the cluster")

pkg/cluster/k8sres.go

@@ -683,12 +683,6 @@ func (c *Cluster) shouldCreateLoadBalancerForService(role PostgresRole, spec *sp
 
 	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
@@ -699,16 +693,8 @@ func (c *Cluster) shouldCreateLoadBalancerForService(role PostgresRole, spec *sp
 
 	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.")
-			return *c.OpConfig.EnableLoadBalancer
+		if spec.EnableMasterLoadBalancer != nil {
+			return *spec.EnableMasterLoadBalancer
 		}
 
 		return c.OpConfig.EnableMasterLoadBalancer

pkg/cluster/k8sres_test.go

@@ -65,23 +65,6 @@ func TestCreateLoadBalancerLogic(t *testing.T) {
 			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

pkg/cluster/resources.go

@@ -344,10 +344,16 @@ func (c *Cluster) deleteService(role PostgresRole) error {
 }
 
 func (c *Cluster) createEndpoint(role PostgresRole) (*v1.Endpoints, error) {
+	var (
+		subsets []v1.EndpointSubset
+	)
 	c.setProcessName("creating endpoint")
-	subsets := make([]v1.EndpointSubset, 0)
-	if role == Master {
-		//TODO: set subsets to the master
+	if !c.isNewCluster() {
+		subsets = c.generateEndpointSubsets(role)
+	} else {
+		// Patroni will populate the master endpoint for the new cluster
+		// The replica endpoint will be filled-in by the service selector.
+		subsets = make([]v1.EndpointSubset, 0)
 	}
 	endpointsSpec := c.generateEndpoint(role, subsets)
 
@@ -361,6 +367,34 @@ func (c *Cluster) createEndpoint(role PostgresRole) (*v1.Endpoints, error) {
 	return endpoints, nil
 }
 
+func (c *Cluster) generateEndpointSubsets(role PostgresRole) []v1.EndpointSubset {
+	result := make([]v1.EndpointSubset, 0)
+	pods, err := c.getRolePods(role)
+	if err != nil {
+		if role == Master {
+			c.logger.Warningf("could not obtain the address for %s pod: %v", role, err)
+		} else {
+			c.logger.Warningf("could not obtain the addresses for %s pods: %v", role, err)
+		}
+		return result
+	}
+
+	endPointAddresses := make([]v1.EndpointAddress, 0)
+	for _, pod := range pods {
+		endPointAddresses = append(endPointAddresses, v1.EndpointAddress{IP: pod.Status.PodIP})
+	}
+	if len(endPointAddresses) > 0 {
+		result = append(result, v1.EndpointSubset{
+			Addresses: endPointAddresses,
+			Ports:     []v1.EndpointPort{{"postgresql", 5432, "TCP"}},
+		})
+	} else if role == Master {
+		c.logger.Warningf("master is not running, generated master endpoint does not contain any addresses")
+	}
+
+	return result
+}
+
 func (c *Cluster) createPodDisruptionBudget() (*policybeta1.PodDisruptionBudget, error) {
 	podDisruptionBudgetSpec := c.generatePodDisruptionBudget()
 	podDisruptionBudget, err := c.KubeClient.

pkg/controller/controller.go

@@ -111,6 +111,7 @@ func (c *Controller) initOperatorConfig() {
 	}
 
 	c.opConfig = config.NewFromMap(configMapData)
+	c.warnOnDeprecatedOperatorParameters()
 
 	scalyrAPIKey := os.Getenv("SCALYR_API_KEY")
 	if scalyrAPIKey != "" {
@@ -119,6 +120,14 @@ func (c *Controller) initOperatorConfig() {
 
 }
 
+// warningOnDeprecatedParameters emits warnings upon finding deprecated parmaters
+func (c *Controller) warnOnDeprecatedOperatorParameters() {
+	if c.opConfig.EnableLoadBalancer != nil {
+		c.logger.Warningf("Operator configuration parameter 'enable_load_balancer' is deprecated and takes no effect. " +
+			"Consider using the 'enable_master_load_balancer' or 'enable_replica_load_balancer' instead.")
+	}
+}
+
 func (c *Controller) initPodServiceAccount() {
 
 	if c.opConfig.PodServiceAccountDefinition == "" {

pkg/controller/postgresql.go

@@ -166,6 +166,17 @@ func (c *Controller) processEvent(event spec.ClusterEvent) {
 
 	defer c.curWorkerCluster.Store(event.WorkerID, nil)
 
+	if event.EventType == spec.EventAdd || event.EventType == spec.EventUpdate || event.EventType == spec.EventSync {
+		// handle deprecated parameters by possibly assigning their values to the new ones.
+		if event.OldSpec != nil {
+			c.mergeDeprecatedPostgreSQLSpecParameters(&event.OldSpec.Spec)
+		}
+		if event.NewSpec != nil {
+			c.mergeDeprecatedPostgreSQLSpecParameters(&event.NewSpec.Spec)
+		}
+		c.warnOnDeprecatedPostgreSQLSpecParameters(&event.NewSpec.Spec)
+	}
+
 	switch event.EventType {
 	case spec.EventAdd:
 		if clusterFound {
@@ -287,6 +298,46 @@ func (c *Controller) processClusterEventsQueue(idx int, stopCh <-chan struct{},
 	}
 }
 
+func (c *Controller) warnOnDeprecatedPostgreSQLSpecParameters(spec *spec.PostgresSpec) {
+
+	deprecate := func(deprecated, replacement string) {
+		c.logger.Warningf("Parameter %q is deprecated. Consider setting %q instead", deprecated, replacement)
+	}
+
+	noeffect := func(param string, explanation string) {
+		c.logger.Warningf("Parameter %q takes no effect. %s", param, explanation)
+	}
+
+	if spec.UseLoadBalancer != nil {
+		deprecate("useLoadBalancer", "enableMasterLoadBalancer")
+	}
+	if spec.ReplicaLoadBalancer != nil {
+		deprecate("replicaLoadBalancer", "enableReplicaLoadBalancer")
+	}
+
+	if len(spec.MaintenanceWindows) > 0 {
+		noeffect("maintenanceWindows", "Not implemented.")
+	}
+}
+
+func (c *Controller) mergeDeprecatedPostgreSQLSpecParameters(spec *spec.PostgresSpec) *spec.PostgresSpec {
+	if spec.UseLoadBalancer != nil || spec.ReplicaLoadBalancer != nil {
+		if spec.EnableReplicaLoadBalancer != nil || spec.EnableMasterLoadBalancer != nil {
+			c.logger.Warnf("Both old and new load balancer options are present, ignoring old ones")
+		} else {
+			if spec.UseLoadBalancer != nil {
+				spec.EnableMasterLoadBalancer = new(bool)
+				*spec.EnableMasterLoadBalancer = *spec.UseLoadBalancer
+			}
+			if spec.ReplicaLoadBalancer != nil {
+				spec.EnableReplicaLoadBalancer = new(bool)
+				*spec.EnableReplicaLoadBalancer = *spec.ReplicaLoadBalancer
+			}
+		}
+	}
+	return spec
+}
+
 func (c *Controller) queueClusterEvent(old, new *spec.Postgresql, eventType spec.EventType) {
 	var (
 		uid          types.UID

pkg/controller/postgresql_test.go

@@ -0,0 +1,43 @@
+package controller
+
+import (
+	"github.com/zalando-incubator/postgres-operator/pkg/spec"
+	"reflect"
+	"testing"
+)
+
+var (
+	True  bool = true
+	False bool = false
+)
+
+func TestMergeDeprecatedPostgreSQLSpecParameters(t *testing.T) {
+	c := NewController(&spec.ControllerConfig{})
+
+	tests := []struct {
+		name  string
+		in    *spec.PostgresSpec
+		out   *spec.PostgresSpec
+		error string
+	}{
+		{
+			"Check that old parameters propagate values to the new ones",
+			&spec.PostgresSpec{UseLoadBalancer: &True, ReplicaLoadBalancer: &True},
+			&spec.PostgresSpec{UseLoadBalancer: &True, ReplicaLoadBalancer: &True,
+				EnableMasterLoadBalancer: &True, EnableReplicaLoadBalancer: &True},
+			"New parameters should be set from the values of old ones",
+		},
+		{
+			"Check that new parameters are not set when both old and new ones are present",
+			&spec.PostgresSpec{UseLoadBalancer: &True, EnableReplicaLoadBalancer: &True},
+			&spec.PostgresSpec{UseLoadBalancer: &True, EnableReplicaLoadBalancer: &True},
+			"New parameters should remain unchanged when both old and new are present",
+		},
+	}
+	for _, tt := range tests {
+		result := c.mergeDeprecatedPostgreSQLSpecParameters(tt.in)
+		if !reflect.DeepEqual(result, tt.out) {
+			t.Errorf("%s: %v", tt.name, tt.error)
+		}
+	}
+}

pkg/util/config/config.go

@@ -88,7 +88,7 @@ type Config struct {
 	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"`
+	EnableLoadBalancer       *bool             `name:"enable_load_balancer"`
 	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"`