Introduce masterServiceAnnotations & replicaServiceAnnotations (#2161)

* Introduce `masterServiceAnnotations` & `replicaServiceAnnotations`

Introduce `masterServiceAnnotations` & `replicaServiceAnnotations` to the `Postgresql` CRD.
`masterServiceAnnotations` overrides `serviceAnnotations` for master role if not empty.
`replicaServiceAnnotations` overrides `serviceAnnotations` for replica role if not empty.
Existing definition of `serviceAnnotations` continue to work for backward compatibitlity when neither `masterServiceAnnotations` nor `replicaServiceAnnotations` is defined.

This closes https://github.com/zalando/postgres-operator/issues/1927

* Accumulate service annotations

First, global config, then ServiceAnnotations overriding, then MasterServiceAnnotations and ReplicaServiceAnnotations.

This addresses
https://github.com/zalando/postgres-operator/pull/2161#discussion_r1063558711.

* Update admin doc with master & replica service annotations overrides

Addressed https://github.com/zalando/postgres-operator/pull/2161#discussion_r1064744086

Co-authored-by: Felix Kunde <felix-kunde@gmx.de>

pkg/apis/acid.zalan.do/v1/crds.go

@@ -355,6 +355,14 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
 							},
 						},
 					},
+					"masterServiceAnnotations": {
+						Type: "object",
+						AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
+							Schema: &apiextv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
 					"nodeAffinity": {
 						Type: "object",
 						Properties: map[string]apiextv1.JSONSchemaProps{
@@ -654,6 +662,14 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
 						Type:        "boolean",
 						Description: "deprecated",
 					},
+					"replicaServiceAnnotations": {
+						Type: "object",
+						AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
+							Schema: &apiextv1.JSONSchemaProps{
+								Type: "string",
+							},
+						},
+					},
 					"resources": {
 						Type: "object",
 						Properties: map[string]apiextv1.JSONSchemaProps{

pkg/apis/acid.zalan.do/v1/postgresql_type.go

@@ -79,10 +79,14 @@ type PostgresSpec struct {
 	StandbyCluster        *StandbyDescription         `json:"standby,omitempty"`
 	PodAnnotations        map[string]string           `json:"podAnnotations,omitempty"`
 	ServiceAnnotations    map[string]string           `json:"serviceAnnotations,omitempty"`
-	TLS                   *TLSDescription             `json:"tls,omitempty"`
-	AdditionalVolumes     []AdditionalVolume          `json:"additionalVolumes,omitempty"`
-	Streams               []Stream                    `json:"streams,omitempty"`
-	Env                   []v1.EnvVar                 `json:"env,omitempty"`
+	// MasterServiceAnnotations takes precedence over ServiceAnnotations for master role if not empty
+	MasterServiceAnnotations map[string]string `json:"masterServiceAnnotations,omitempty"`
+	// ReplicaServiceAnnotations takes precedence over ServiceAnnotations for replica role if not empty
+	ReplicaServiceAnnotations map[string]string  `json:"replicaServiceAnnotations,omitempty"`
+	TLS                       *TLSDescription    `json:"tls,omitempty"`
+	AdditionalVolumes         []AdditionalVolume `json:"additionalVolumes,omitempty"`
+	Streams                   []Stream           `json:"streams,omitempty"`
+	Env                       []v1.EnvVar        `json:"env,omitempty"`
 
 	// deprecated json tags
 	InitContainersOld       []v1.Container `json:"init_containers,omitempty"`

pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go

@@ -792,6 +792,20 @@ func (in *PostgresSpec) DeepCopyInto(out *PostgresSpec) {
 			(*out)[key] = val
 		}
 	}
+	if in.MasterServiceAnnotations != nil {
+		in, out := &in.MasterServiceAnnotations, &out.MasterServiceAnnotations
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
+	if in.ReplicaServiceAnnotations != nil {
+		in, out := &in.ReplicaServiceAnnotations, &out.ReplicaServiceAnnotations
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	if in.TLS != nil {
 		in, out := &in.TLS, &out.TLS
 		*out = new(TLSDescription)

pkg/cluster/cluster_test.go

@@ -524,7 +524,9 @@ func TestServiceAnnotations(t *testing.T) {
 		enableReplicaLoadBalancerOC   bool
 		enableTeamIdClusterPrefix     bool
 		operatorAnnotations           map[string]string
-		clusterAnnotations            map[string]string
+		serviceAnnotations            map[string]string
+		masterServiceAnnotations      map[string]string
+		replicaServiceAnnotations     map[string]string
 		expect                        map[string]string
 	}{
 		//MASTER
@@ -535,7 +537,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC:   false,
 			enableTeamIdClusterPrefix:    false,
 			operatorAnnotations:          make(map[string]string),
-			clusterAnnotations:           make(map[string]string),
+			serviceAnnotations:           make(map[string]string),
 			expect:                       make(map[string]string),
 		},
 		{
@@ -545,7 +547,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC:   false,
 			enableTeamIdClusterPrefix:    false,
 			operatorAnnotations:          make(map[string]string),
-			clusterAnnotations:           make(map[string]string),
+			serviceAnnotations:           make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -558,7 +560,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC:   true,
 			enableTeamIdClusterPrefix:    false,
 			operatorAnnotations:          make(map[string]string),
-			clusterAnnotations:           make(map[string]string),
+			serviceAnnotations:           make(map[string]string),
 			expect:                       make(map[string]string),
 		},
 		{
@@ -567,7 +569,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  false,
 			operatorAnnotations:        make(map[string]string),
-			clusterAnnotations:         make(map[string]string),
+			serviceAnnotations:         make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -579,7 +581,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  false,
 			operatorAnnotations:        make(map[string]string),
-			clusterAnnotations:         map[string]string{"foo": "bar"},
+			serviceAnnotations:         map[string]string{"foo": "bar"},
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -593,7 +595,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC:   true,
 			enableTeamIdClusterPrefix:    false,
 			operatorAnnotations:          make(map[string]string),
-			clusterAnnotations:           map[string]string{"foo": "bar"},
+			serviceAnnotations:           map[string]string{"foo": "bar"},
 			expect:                       map[string]string{"foo": "bar"},
 		},
 		{
@@ -602,7 +604,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  false,
 			operatorAnnotations:        map[string]string{"foo": "bar"},
-			clusterAnnotations:         make(map[string]string),
+			serviceAnnotations:         make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -617,7 +619,7 @@ func TestServiceAnnotations(t *testing.T) {
 			operatorAnnotations: map[string]string{
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
 			},
-			clusterAnnotations: make(map[string]string),
+			serviceAnnotations: make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
@@ -629,7 +631,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  false,
 			operatorAnnotations:        make(map[string]string),
-			clusterAnnotations: map[string]string{
+			serviceAnnotations: map[string]string{
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
 			},
 			expect: map[string]string{
@@ -643,7 +645,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  false,
 			operatorAnnotations:        make(map[string]string),
-			clusterAnnotations: map[string]string{
+			serviceAnnotations: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com",
 			},
 			expect: map[string]string{
@@ -656,13 +658,32 @@ func TestServiceAnnotations(t *testing.T) {
 			role:                       "master",
 			enableMasterLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:  true,
-			clusterAnnotations:         make(map[string]string),
+			serviceAnnotations:         make(map[string]string),
 			operatorAnnotations:        make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
 			},
 		},
+		{
+			about:                      "Master with master service annotations override service annotations",
+			role:                       "master",
+			enableMasterLoadBalancerOC: true,
+			enableTeamIdClusterPrefix:  false,
+			operatorAnnotations:        make(map[string]string),
+			serviceAnnotations: map[string]string{
+				"service.beta.kubernetes.io/aws-load-balancer-nlb-target-type":         "ip",
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
+			},
+			masterServiceAnnotations: map[string]string{
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "2000",
+			},
+			expect: map[string]string{
+				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test.test.db.example.com,test.acid.db.example.com",
+				"service.beta.kubernetes.io/aws-load-balancer-nlb-target-type":         "ip",
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "2000",
+			},
+		},
 		// REPLICA
 		{
 			about:                         "Replica with no annotations and EnableReplicaLoadBalancer disabled on spec and OperatorConfig",
@@ -671,7 +692,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC:   false,
 			enableTeamIdClusterPrefix:     false,
 			operatorAnnotations:           make(map[string]string),
-			clusterAnnotations:            make(map[string]string),
+			serviceAnnotations:            make(map[string]string),
 			expect:                        make(map[string]string),
 		},
 		{
@@ -681,7 +702,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC:   false,
 			enableTeamIdClusterPrefix:     false,
 			operatorAnnotations:           make(map[string]string),
-			clusterAnnotations:            make(map[string]string),
+			serviceAnnotations:            make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -694,7 +715,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC:   true,
 			enableTeamIdClusterPrefix:     false,
 			operatorAnnotations:           make(map[string]string),
-			clusterAnnotations:            make(map[string]string),
+			serviceAnnotations:            make(map[string]string),
 			expect:                        make(map[string]string),
 		},
 		{
@@ -703,7 +724,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         make(map[string]string),
-			clusterAnnotations:          make(map[string]string),
+			serviceAnnotations:          make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -715,7 +736,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         make(map[string]string),
-			clusterAnnotations:          map[string]string{"foo": "bar"},
+			serviceAnnotations:          map[string]string{"foo": "bar"},
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -729,7 +750,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC:   true,
 			enableTeamIdClusterPrefix:     false,
 			operatorAnnotations:           make(map[string]string),
-			clusterAnnotations:            map[string]string{"foo": "bar"},
+			serviceAnnotations:            map[string]string{"foo": "bar"},
 			expect:                        map[string]string{"foo": "bar"},
 		},
 		{
@@ -738,7 +759,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         map[string]string{"foo": "bar"},
-			clusterAnnotations:          make(map[string]string),
+			serviceAnnotations:          make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
@@ -753,7 +774,7 @@ func TestServiceAnnotations(t *testing.T) {
 			operatorAnnotations: map[string]string{
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
 			},
-			clusterAnnotations: make(map[string]string),
+			serviceAnnotations: make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
@@ -765,7 +786,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         make(map[string]string),
-			clusterAnnotations: map[string]string{
+			serviceAnnotations: map[string]string{
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
 			},
 			expect: map[string]string{
@@ -779,7 +800,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         make(map[string]string),
-			clusterAnnotations: map[string]string{
+			serviceAnnotations: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com",
 			},
 			expect: map[string]string{
@@ -792,13 +813,32 @@ func TestServiceAnnotations(t *testing.T) {
 			role:                        "replica",
 			enableReplicaLoadBalancerOC: true,
 			enableTeamIdClusterPrefix:   true,
-			clusterAnnotations:          make(map[string]string),
+			serviceAnnotations:          make(map[string]string),
 			operatorAnnotations:         make(map[string]string),
 			expect: map[string]string{
 				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
 				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600",
 			},
 		},
+		{
+			about:                       "Replica with replica service annotations override service annotations",
+			role:                        "replica",
+			enableReplicaLoadBalancerOC: true,
+			enableTeamIdClusterPrefix:   false,
+			operatorAnnotations:         make(map[string]string),
+			serviceAnnotations: map[string]string{
+				"service.beta.kubernetes.io/aws-load-balancer-nlb-target-type":         "ip",
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800",
+			},
+			replicaServiceAnnotations: map[string]string{
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "2000",
+			},
+			expect: map[string]string{
+				"external-dns.alpha.kubernetes.io/hostname":                            "acid-test-repl.test.db.example.com,test-repl.acid.db.example.com",
+				"service.beta.kubernetes.io/aws-load-balancer-nlb-target-type":         "ip",
+				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "2000",
+			},
+		},
 		// COMMON
 		{
 			about:                       "cluster annotations append to operator annotations",
@@ -806,7 +846,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: false,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         map[string]string{"foo": "bar"},
-			clusterAnnotations:          map[string]string{"post": "gres"},
+			serviceAnnotations:          map[string]string{"post": "gres"},
 			expect:                      map[string]string{"foo": "bar", "post": "gres"},
 		},
 		{
@@ -815,7 +855,7 @@ func TestServiceAnnotations(t *testing.T) {
 			enableReplicaLoadBalancerOC: false,
 			enableTeamIdClusterPrefix:   false,
 			operatorAnnotations:         map[string]string{"foo": "bar", "post": "gres"},
-			clusterAnnotations:          map[string]string{"post": "greSQL"},
+			serviceAnnotations:          map[string]string{"post": "greSQL"},
 			expect:                      map[string]string{"foo": "bar", "post": "greSQL"},
 		},
 	}
@@ -833,7 +873,9 @@ func TestServiceAnnotations(t *testing.T) {
 
 			cl.Postgresql.Spec.ClusterName = ""
 			cl.Postgresql.Spec.TeamID = "acid"
-			cl.Postgresql.Spec.ServiceAnnotations = tt.clusterAnnotations
+			cl.Postgresql.Spec.ServiceAnnotations = tt.serviceAnnotations
+			cl.Postgresql.Spec.MasterServiceAnnotations = tt.masterServiceAnnotations
+			cl.Postgresql.Spec.ReplicaServiceAnnotations = tt.replicaServiceAnnotations
 			cl.Postgresql.Spec.EnableMasterLoadBalancer = tt.enableMasterLoadBalancerSpec
 			cl.Postgresql.Spec.EnableReplicaLoadBalancer = tt.enableReplicaLoadBalancerSpec
 

pkg/cluster/k8sres.go

@@ -28,6 +28,7 @@ import (
 	"github.com/zalando/postgres-operator/pkg/util/k8sutil"
 	"github.com/zalando/postgres-operator/pkg/util/patroni"
 	"github.com/zalando/postgres-operator/pkg/util/retryutil"
+	"golang.org/x/exp/maps"
 	batchv1 "k8s.io/api/batch/v1"
 	"k8s.io/apimachinery/pkg/labels"
 )
@@ -1901,13 +1902,16 @@ func (c *Cluster) configureLoadBalanceService(serviceSpec *v1.ServiceSpec, sourc
 
 func (c *Cluster) generateServiceAnnotations(role PostgresRole, spec *acidv1.PostgresSpec) map[string]string {
 	annotations := make(map[string]string)
+	maps.Copy(annotations, c.OpConfig.CustomServiceAnnotations)
 
-	for k, v := range c.OpConfig.CustomServiceAnnotations {
-		annotations[k] = v
-	}
-	if spec != nil || spec.ServiceAnnotations != nil {
-		for k, v := range spec.ServiceAnnotations {
-			annotations[k] = v
+	if spec != nil {
+		maps.Copy(annotations, spec.ServiceAnnotations)
+
+		switch role {
+		case Master:
+			maps.Copy(annotations, spec.MasterServiceAnnotations)
+		case Replica:
+			maps.Copy(annotations, spec.ReplicaServiceAnnotations)
 		}
 	}