From ba4e09464cc7e2b92d4a639aee4193358a6a534d Mon Sep 17 00:00:00 2001 From: Jonathan Juares Beber Date: Fri, 24 Jan 2020 19:11:30 +0100 Subject: [PATCH] Add ServiceAnnotations cluster config The [operator parameters][1] already support the `custom_service_annotations` config.With this parameter is possible to define custom annotations that will be used on the services created by the operator. The `custom_service_annotations` as all the other [operator parameters][1] are defined on the operator level and do not allow customization on the cluster level. A cluster may require different service annotations, as for example, set up different cloud load balancers timeouts, different ingress annotations, and/or enable more customizable environments. This commit introduces a new parameter on the cluster level, called `serviceAnnotations`, responsible for defining custom annotations just for the services created by the operator to the specifically defined cluster. It allows a mix of configuration between `custom_service_annotations` and `serviceAnnotations` where the latest one will have priority. In order to allow custom service annotations to be used on services without LoadBalancers (as for example, service mesh services annotations) both `custom_service_annotations` and `serviceAnnotations` are applied independently of load-balancing configuration. For retro-compatibility purposes, `custom_service_annotations` is still under [Load balancer related options][2]. The two default annotations when using LoadBalancer services, `external-dns.alpha.kubernetes.io/hostname` and `service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout` are still defined by the operator. `service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout` can be overridden by `custom_service_annotations` or `serviceAnnotations`, allowing a more customizable environment. `external-dns.alpha.kubernetes.io/hostname` can not be overridden once there is no differentiation between custom service annotations for replicas and masters. It updates the documentation and creates the necessary unit and e2e tests to the above-described feature too. [1]: https://github.com/zalando/postgres-operator/blob/master/docs/reference/operator_parameters.md [2]: https://github.com/zalando/postgres-operator/blob/master/docs/reference/operator_parameters.md#load-balancer-related-options --- .../postgres-operator/crds/postgresqls.yaml | 4 + docs/administrator.md | 11 + docs/reference/cluster_manifest.md | 5 + docs/reference/operator_parameters.md | 5 +- e2e/README.md | 1 + e2e/tests/test_e2e.py | 51 ++- manifests/complete-postgres-manifest.yaml | 2 + ...res-manifest-with-service-annotations.yaml | 20 ++ manifests/postgresql.crd.yaml | 4 + pkg/apis/acid.zalan.do/v1/crds.go | 8 + pkg/apis/acid.zalan.do/v1/postgresql_type.go | 1 + pkg/apis/acid.zalan.do/v1/util_test.go | 101 +++++- .../acid.zalan.do/v1/zz_generated.deepcopy.go | 7 + pkg/cluster/cluster_test.go | 322 ++++++++++++++++++ pkg/cluster/k8sres.go | 60 ++-- 15 files changed, 565 insertions(+), 37 deletions(-) create mode 100644 manifests/postgres-manifest-with-service-annotations.yaml diff --git a/charts/postgres-operator/crds/postgresqls.yaml b/charts/postgres-operator/crds/postgresqls.yaml index 198afe119..b4b676236 100644 --- a/charts/postgres-operator/crds/postgresqls.yaml +++ b/charts/postgres-operator/crds/postgresqls.yaml @@ -266,6 +266,10 @@ spec: pattern: '^(\d+(e\d+)?|\d+(\.\d+)?(e\d+)?[EPTGMK]i?)$' # Note: the value specified here must not be zero or be higher # than the corresponding limit. + serviceAnnotations: + type: object + additionalProperties: + type: string sidecars: type: array nullable: true diff --git a/docs/administrator.md b/docs/administrator.md index 5b8769edb..2e86193c0 100644 --- a/docs/administrator.md +++ b/docs/administrator.md @@ -376,6 +376,17 @@ cluster manifest. In the case any of these variables are omitted from the manifest, the operator configuration settings `enable_master_load_balancer` and `enable_replica_load_balancer` apply. Note that the operator settings affect all Postgresql services running in all namespaces watched by the operator. +If load balancing is enabled two default annotations will be applied to its +services: + +- `external-dns.alpha.kubernetes.io/hostname` with the value defined by the + operator configs `master_dns_name_format` and `replica_dns_name_format`. + This value can't be overwritten. If any changing in its value is needed, it + MUST be done changing the DNS format operator config parameters; and +- `service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout` with + a default value of "3600". This value can be overwritten with the operator + config parameter `custom_service_annotations` or the cluster parameter + `serviceAnnotations`. To limit the range of IP addresses that can reach a load balancer, specify the desired ranges in the `allowedSourceRanges` field (applies to both master and diff --git a/docs/reference/cluster_manifest.md b/docs/reference/cluster_manifest.md index bf6df681b..7b049b6fa 100644 --- a/docs/reference/cluster_manifest.md +++ b/docs/reference/cluster_manifest.md @@ -122,6 +122,11 @@ These parameters are grouped directly under the `spec` key in the manifest. A map of key value pairs that gets attached as [annotations](https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/) to each pod created for the database. +* **serviceAnnotations** + A map of key value pairs that gets attached as [annotations](https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/) + to the services created for the database cluster. Check the + [administrator docs](https://github.com/zalando/postgres-operator/blob/master/docs/administrator.md#load-balancers-and-allowed-ip-ranges) + for more information regarding default values and overwrite rules. * **enableShmVolume** Start a database pod without limitations on shm memory. By default Docker diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index 1055d89b6..b0e148289 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -380,8 +380,9 @@ In the CRD-based configuration they are grouped under the `load_balancer` key. `false`. * **custom_service_annotations** - when load balancing is enabled, LoadBalancer service is created and - this parameter takes service annotations that are applied to service. + This key/value map provides a list of annotations that get attached to each + service of a cluster created by the operator. If the annotation key is also + provided by the cluster definition, the manifest value is used. Optional. * **master_dns_name_format** defines the DNS name string template for the diff --git a/e2e/README.md b/e2e/README.md index 1d611bcd0..f1bc5f9ed 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -44,3 +44,4 @@ The current tests are all bundled in [`test_e2e.py`](tests/test_e2e.py): * taint-based eviction of Postgres pods * invoking logical backup cron job * uniqueness of master pod +* custom service annotations diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 88a7f1f34..3ffdb746a 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -161,8 +161,8 @@ def test_logical_backup_cron_job(self): schedule = "7 7 7 7 *" pg_patch_enable_backup = { "spec": { - "enableLogicalBackup": True, - "logicalBackupSchedule": schedule + "enableLogicalBackup": True, + "logicalBackupSchedule": schedule } } k8s.api.custom_objects_api.patch_namespaced_custom_object( @@ -184,7 +184,7 @@ def test_logical_backup_cron_job(self): image = "test-image-name" patch_logical_backup_image = { "data": { - "logical_backup_docker_image": image, + "logical_backup_docker_image": image, } } k8s.update_config(patch_logical_backup_image) @@ -197,7 +197,7 @@ def test_logical_backup_cron_job(self): # delete the logical backup cron job pg_patch_disable_backup = { "spec": { - "enableLogicalBackup": False, + "enableLogicalBackup": False, } } k8s.api.custom_objects_api.patch_namespaced_custom_object( @@ -207,6 +207,37 @@ def test_logical_backup_cron_job(self): self.assertEqual(0, len(jobs), "Expected 0 logical backup jobs, found {}".format(len(jobs))) + @timeout_decorator.timeout(TEST_TIMEOUT_SEC) + def test_service_annotations(self): + ''' + Create a Postgres cluster with service annotations and check them. + ''' + k8s = self.k8s + patch_custom_service_annotations = { + "data": { + "custom_service_annotations": "foo:bar", + } + } + k8s.update_config(patch_custom_service_annotations) + + k8s.create_with_kubectl("manifests/postgres-manifest-with-service-annotations.yaml") + annotations = { + "annotation.key": "value", + "foo": "bar", + } + self.assertTrue(k8s.check_service_annotations( + "version=acid-service-annotations,spilo-role=master", annotations)) + self.assertTrue(k8s.check_service_annotations( + "version=acid-service-annotations,spilo-role=replica", annotations)) + + # clean up + unpatch_custom_service_annotations = { + "data": { + "custom_service_annotations": "", + } + } + k8s.update_config(unpatch_custom_service_annotations) + def assert_master_is_unique(self, namespace='default', version="acid-minimal-cluster"): """ Check that there is a single pod in the k8s cluster with the label "spilo-role=master" @@ -272,6 +303,16 @@ def wait_for_pod_start(self, pod_labels, namespace='default'): pod_phase = pods[0].status.phase time.sleep(self.RETRY_TIMEOUT_SEC) + def check_service_annotations(self, svc_labels, annotations, namespace='default'): + svcs = self.api.core_v1.list_namespaced_service(namespace, label_selector=svc_labels, limit=1).items + for svc in svcs: + if len(svc.metadata.annotations) != len(annotations): + return False + for key in svc.metadata.annotations: + if svc.metadata.annotations[key] != annotations[key]: + return False + return True + def wait_for_pg_to_scale(self, number_of_instances, namespace='default'): body = { @@ -280,7 +321,7 @@ def wait_for_pg_to_scale(self, number_of_instances, namespace='default'): } } _ = self.api.custom_objects_api.patch_namespaced_custom_object( - "acid.zalan.do", "v1", namespace, "postgresqls", "acid-minimal-cluster", body) + "acid.zalan.do", "v1", namespace, "postgresqls", "acid-minimal-cluster", body) labels = 'version=acid-minimal-cluster' while self.count_pods_with_label(labels) != number_of_instances: diff --git a/manifests/complete-postgres-manifest.yaml b/manifests/complete-postgres-manifest.yaml index cf450ef94..bfd4b7da7 100644 --- a/manifests/complete-postgres-manifest.yaml +++ b/manifests/complete-postgres-manifest.yaml @@ -32,6 +32,8 @@ spec: # spiloFSGroup: 103 # podAnnotations: # annotation.key: value +# serviceAnnotations: +# annotation.key: value # podPriorityClassName: "spilo-pod-priority" # tolerations: # - key: postgres diff --git a/manifests/postgres-manifest-with-service-annotations.yaml b/manifests/postgres-manifest-with-service-annotations.yaml new file mode 100644 index 000000000..ab3096740 --- /dev/null +++ b/manifests/postgres-manifest-with-service-annotations.yaml @@ -0,0 +1,20 @@ +apiVersion: "acid.zalan.do/v1" +kind: postgresql +metadata: + name: acid-service-annotations +spec: + teamId: "acid" + volume: + size: 1Gi + numberOfInstances: 2 + users: + zalando: # database owner + - superuser + - createdb + foo_user: [] # role for application foo + databases: + foo: zalando # dbname: owner + postgresql: + version: "11" + serviceAnnotations: + annotation.key: value diff --git a/manifests/postgresql.crd.yaml b/manifests/postgresql.crd.yaml index 3b0f652ea..276bc94b8 100644 --- a/manifests/postgresql.crd.yaml +++ b/manifests/postgresql.crd.yaml @@ -230,6 +230,10 @@ spec: pattern: '^(\d+(e\d+)?|\d+(\.\d+)?(e\d+)?[EPTGMK]i?)$' # Note: the value specified here must not be zero or be higher # than the corresponding limit. + serviceAnnotations: + type: object + additionalProperties: + type: string sidecars: type: array nullable: true diff --git a/pkg/apis/acid.zalan.do/v1/crds.go b/pkg/apis/acid.zalan.do/v1/crds.go index 20fa37138..df378515f 100644 --- a/pkg/apis/acid.zalan.do/v1/crds.go +++ b/pkg/apis/acid.zalan.do/v1/crds.go @@ -383,6 +383,14 @@ var PostgresCRDResourceValidation = apiextv1beta1.CustomResourceValidation{ }, }, }, + "serviceAnnotations": { + Type: "object", + AdditionalProperties: &apiextv1beta1.JSONSchemaPropsOrBool{ + Schema: &apiextv1beta1.JSONSchemaProps{ + Type: "string", + }, + }, + }, "sidecars": { Type: "array", Items: &apiextv1beta1.JSONSchemaPropsOrArray{ diff --git a/pkg/apis/acid.zalan.do/v1/postgresql_type.go b/pkg/apis/acid.zalan.do/v1/postgresql_type.go index 515a73ff0..07b42d4d4 100644 --- a/pkg/apis/acid.zalan.do/v1/postgresql_type.go +++ b/pkg/apis/acid.zalan.do/v1/postgresql_type.go @@ -60,6 +60,7 @@ type PostgresSpec struct { LogicalBackupSchedule string `json:"logicalBackupSchedule,omitempty"` StandbyCluster *StandbyDescription `json:"standby"` PodAnnotations map[string]string `json:"podAnnotations"` + ServiceAnnotations map[string]string `json:"serviceAnnotations"` // deprecated json tags InitContainersOld []v1.Container `json:"init_containers,omitempty"` diff --git a/pkg/apis/acid.zalan.do/v1/util_test.go b/pkg/apis/acid.zalan.do/v1/util_test.go index a1e01825f..28e9e8ca4 100644 --- a/pkg/apis/acid.zalan.do/v1/util_test.go +++ b/pkg/apis/acid.zalan.do/v1/util_test.go @@ -456,18 +456,84 @@ var postgresqlList = []struct { PostgresqlList{}, errors.New("unexpected end of JSON input")}} -var annotations = []struct { +var podAnnotations = []struct { about string in []byte annotations map[string]string err error }{{ - about: "common annotations", - in: []byte(`{"kind": "Postgresql","apiVersion": "acid.zalan.do/v1","metadata": {"name": "acid-testcluster1"}, "spec": {"podAnnotations": {"foo": "bar"},"teamId": "acid", "clone": {"cluster": "team-batman"}}}`), + about: "common annotations", + in: []byte(`{ + "kind": "Postgresql", + "apiVersion": "acid.zalan.do/v1", + "metadata": { + "name": "acid-testcluster1" + }, + "spec": { + "podAnnotations": { + "foo": "bar" + }, + "teamId": "acid", + "clone": { + "cluster": "team-batman" + } + } + }`), annotations: map[string]string{"foo": "bar"}, err: nil}, } +var serviceAnnotations = []struct { + about string + in []byte + annotations map[string]string + err error +}{ + { + about: "common single annotation", + in: []byte(`{ + "kind": "Postgresql", + "apiVersion": "acid.zalan.do/v1", + "metadata": { + "name": "acid-testcluster1" + }, + "spec": { + "serviceAnnotations": { + "foo": "bar" + }, + "teamId": "acid", + "clone": { + "cluster": "team-batman" + } + } + }`), + annotations: map[string]string{"foo": "bar"}, + err: nil, + }, + { + about: "common two annotations", + in: []byte(`{ + "kind": "Postgresql", + "apiVersion": "acid.zalan.do/v1", + "metadata": { + "name": "acid-testcluster1" + }, + "spec": { + "serviceAnnotations": { + "foo": "bar", + "post": "gres" + }, + "teamId": "acid", + "clone": { + "cluster": "team-batman" + } + } + }`), + annotations: map[string]string{"foo": "bar", "post": "gres"}, + err: nil, + }, +} + func mustParseTime(s string) metav1.Time { v, err := time.Parse("15:04", s) if err != nil { @@ -517,21 +583,42 @@ func TestWeekdayTime(t *testing.T) { } } -func TestClusterAnnotations(t *testing.T) { - for _, tt := range annotations { +func TestPodAnnotations(t *testing.T) { + for _, tt := range podAnnotations { t.Run(tt.about, func(t *testing.T) { var cluster Postgresql err := cluster.UnmarshalJSON(tt.in) if err != nil { if tt.err == nil || err.Error() != tt.err.Error() { - t.Errorf("Unable to marshal cluster with annotations: expected %v got %v", tt.err, err) + t.Errorf("Unable to marshal cluster with podAnnotations: expected %v got %v", tt.err, err) } return } for k, v := range cluster.Spec.PodAnnotations { found, expected := v, tt.annotations[k] if found != expected { - t.Errorf("Didn't find correct value for key %v in for podAnnotations: Expected %v found %v", k, expected, found) + t.Errorf("Didn't find correct value for key %v in for podAnnotations: Expected %v found %v", k, expected, found) + } + } + }) + } +} + +func TestServiceAnnotations(t *testing.T) { + for _, tt := range serviceAnnotations { + t.Run(tt.about, func(t *testing.T) { + var cluster Postgresql + err := cluster.UnmarshalJSON(tt.in) + if err != nil { + if tt.err == nil || err.Error() != tt.err.Error() { + t.Errorf("Unable to marshal cluster with serviceAnnotations: expected %v got %v", tt.err, err) + } + return + } + for k, v := range cluster.Spec.ServiceAnnotations { + found, expected := v, tt.annotations[k] + if found != expected { + t.Errorf("Didn't find correct value for key %v in for serviceAnnotations: Expected %v found %v", k, expected, found) } } }) diff --git a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go index dc07aa2cf..aaae1f04b 100644 --- a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go +++ b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go @@ -514,6 +514,13 @@ func (in *PostgresSpec) DeepCopyInto(out *PostgresSpec) { (*out)[key] = val } } + if in.ServiceAnnotations != nil { + in, out := &in.ServiceAnnotations, &out.ServiceAnnotations + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.InitContainersOld != nil { in, out := &in.InitContainersOld, &out.InitContainersOld *out = make([]corev1.Container, len(*in)) diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 85d014d8a..9efbc51c6 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -355,6 +355,12 @@ func TestPodAnnotations(t *testing.T) { database: map[string]string{"foo": "bar"}, merged: map[string]string{"foo": "bar"}, }, + { + subTest: "Both Annotations", + operator: map[string]string{"foo": "bar"}, + database: map[string]string{"post": "gres"}, + merged: map[string]string{"foo": "bar", "post": "gres"}, + }, { subTest: "Database Config overrides Operator Config Annotations", operator: map[string]string{"foo": "bar", "global": "foo"}, @@ -382,3 +388,319 @@ func TestPodAnnotations(t *testing.T) { } } } + +func TestServiceAnnotations(t *testing.T) { + enabled := true + disabled := false + tests := []struct { + about string + role PostgresRole + enableMasterLoadBalancerSpec *bool + enableMasterLoadBalancerOC bool + enableReplicaLoadBalancerSpec *bool + enableReplicaLoadBalancerOC bool + operatorAnnotations map[string]string + clusterAnnotations map[string]string + expect map[string]string + }{ + //MASTER + { + about: "Master with no annotations and EnableMasterLoadBalancer disabled on spec and OperatorConfig", + role: "master", + enableMasterLoadBalancerSpec: &disabled, + enableMasterLoadBalancerOC: false, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: make(map[string]string), + }, + { + about: "Master with no annotations and EnableMasterLoadBalancer enabled on spec", + role: "master", + enableMasterLoadBalancerSpec: &enabled, + enableMasterLoadBalancerOC: false, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Master with no annotations and EnableMasterLoadBalancer enabled only on operator config", + role: "master", + enableMasterLoadBalancerSpec: &disabled, + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: make(map[string]string), + }, + { + about: "Master with no annotations and EnableMasterLoadBalancer defined only on operator config", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Master with cluster annotations and load balancer enabled", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{"foo": "bar"}, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + "foo": "bar", + }, + }, + { + about: "Master with cluster annotations and load balancer disabled", + role: "master", + enableMasterLoadBalancerSpec: &disabled, + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{"foo": "bar"}, + expect: map[string]string{"foo": "bar"}, + }, + { + about: "Master with operator annotations and load balancer enabled", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: map[string]string{"foo": "bar"}, + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + "foo": "bar", + }, + }, + { + about: "Master with operator annotations override default annotations", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + }, + { + about: "Master with cluster annotations override default annotations", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + }, + { + about: "Master with cluster annotations do not override external-dns annotations", + role: "master", + enableMasterLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Master with operator annotations do not override external-dns annotations", + role: "master", + enableMasterLoadBalancerOC: true, + clusterAnnotations: make(map[string]string), + operatorAnnotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + // REPLICA + { + about: "Replica with no annotations and EnableReplicaLoadBalancer disabled on spec and OperatorConfig", + role: "replica", + enableReplicaLoadBalancerSpec: &disabled, + enableReplicaLoadBalancerOC: false, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: make(map[string]string), + }, + { + about: "Replica with no annotations and EnableReplicaLoadBalancer enabled on spec", + role: "replica", + enableReplicaLoadBalancerSpec: &enabled, + enableReplicaLoadBalancerOC: false, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Replica with no annotations and EnableReplicaLoadBalancer enabled only on operator config", + role: "replica", + enableReplicaLoadBalancerSpec: &disabled, + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: make(map[string]string), + }, + { + about: "Replica with no annotations and EnableReplicaLoadBalancer defined only on operator config", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Replica with cluster annotations and load balancer enabled", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{"foo": "bar"}, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + "foo": "bar", + }, + }, + { + about: "Replica with cluster annotations and load balancer disabled", + role: "replica", + enableReplicaLoadBalancerSpec: &disabled, + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{"foo": "bar"}, + expect: map[string]string{"foo": "bar"}, + }, + { + about: "Replica with operator annotations and load balancer enabled", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: map[string]string{"foo": "bar"}, + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + "foo": "bar", + }, + }, + { + about: "Replica with operator annotations override default annotations", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + clusterAnnotations: make(map[string]string), + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + }, + { + about: "Replica with cluster annotations override default annotations", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "1800", + }, + }, + { + about: "Replica with cluster annotations do not override external-dns annotations", + role: "replica", + enableReplicaLoadBalancerOC: true, + operatorAnnotations: make(map[string]string), + clusterAnnotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + { + about: "Replica with operator annotations do not override external-dns annotations", + role: "replica", + enableReplicaLoadBalancerOC: true, + clusterAnnotations: make(map[string]string), + operatorAnnotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "wrong.external-dns-name.example.com", + }, + expect: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test-repl.acid.db.example.com", + "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "3600", + }, + }, + // COMMON + { + about: "cluster annotations append to operator annotations", + role: "replica", + enableReplicaLoadBalancerOC: false, + operatorAnnotations: map[string]string{"foo": "bar"}, + clusterAnnotations: map[string]string{"post": "gres"}, + expect: map[string]string{"foo": "bar", "post": "gres"}, + }, + { + about: "cluster annotations override operator annotations", + role: "replica", + enableReplicaLoadBalancerOC: false, + operatorAnnotations: map[string]string{"foo": "bar", "post": "gres"}, + clusterAnnotations: map[string]string{"post": "greSQL"}, + expect: map[string]string{"foo": "bar", "post": "greSQL"}, + }, + } + + for _, tt := range tests { + t.Run(tt.about, func(t *testing.T) { + cl.OpConfig.CustomServiceAnnotations = tt.operatorAnnotations + cl.OpConfig.EnableMasterLoadBalancer = tt.enableMasterLoadBalancerOC + cl.OpConfig.EnableReplicaLoadBalancer = tt.enableReplicaLoadBalancerOC + cl.OpConfig.MasterDNSNameFormat = "{cluster}.{team}.{hostedzone}" + cl.OpConfig.ReplicaDNSNameFormat = "{cluster}-repl.{team}.{hostedzone}" + cl.OpConfig.DbHostedZone = "db.example.com" + + cl.Postgresql.Spec.ClusterName = "test" + cl.Postgresql.Spec.TeamID = "acid" + cl.Postgresql.Spec.ServiceAnnotations = tt.clusterAnnotations + cl.Postgresql.Spec.EnableMasterLoadBalancer = tt.enableMasterLoadBalancerSpec + cl.Postgresql.Spec.EnableReplicaLoadBalancer = tt.enableReplicaLoadBalancerSpec + + got := cl.generateServiceAnnotations(tt.role, &cl.Postgresql.Spec) + if len(tt.expect) != len(got) { + t.Errorf("expected %d annotation(s), got %d", len(tt.expect), len(got)) + return + } + for k, v := range got { + if tt.expect[k] != v { + t.Errorf("expected annotation '%v' with value '%v', got value '%v'", k, tt.expect[k], v) + } + } + }) + } +} diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index aed0c6e83..c9c0362be 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1230,14 +1230,6 @@ func (c *Cluster) shouldCreateLoadBalancerForService(role PostgresRole, spec *ac } func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) *v1.Service { - var dnsName string - - if role == Master { - dnsName = c.masterDNSName() - } else { - dnsName = c.replicaDNSName() - } - serviceSpec := v1.ServiceSpec{ Ports: []v1.ServicePort{{Name: "postgresql", Port: 5432, TargetPort: intstr.IntOrString{IntVal: 5432}}}, Type: v1.ServiceTypeClusterIP, @@ -1247,8 +1239,6 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) serviceSpec.Selector = c.roleLabelsSet(false, role) } - var annotations map[string]string - if c.shouldCreateLoadBalancerForService(role, spec) { // spec.AllowedSourceRanges evaluates to the empty slice of zero length @@ -1262,18 +1252,6 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) c.logger.Debugf("final load balancer source ranges as seen in a service spec (not necessarily applied): %q", serviceSpec.LoadBalancerSourceRanges) serviceSpec.Type = v1.ServiceTypeLoadBalancer - - annotations = map[string]string{ - constants.ZalandoDNSNameAnnotation: dnsName, - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - } - - if len(c.OpConfig.CustomServiceAnnotations) != 0 { - c.logger.Debugf("There are custom annotations defined, creating them.") - for customAnnotationKey, customAnnotationValue := range c.OpConfig.CustomServiceAnnotations { - annotations[customAnnotationKey] = customAnnotationValue - } - } } 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 @@ -1285,7 +1263,7 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) Name: c.serviceName(role), Namespace: c.Namespace, Labels: c.roleLabelsSet(true, role), - Annotations: annotations, + Annotations: c.generateServiceAnnotations(role, spec), }, Spec: serviceSpec, } @@ -1293,6 +1271,42 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) return service } +func (c *Cluster) generateServiceAnnotations(role PostgresRole, spec *acidv1.PostgresSpec) map[string]string { + annotations := make(map[string]string) + + 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 c.shouldCreateLoadBalancerForService(role, spec) { + var dnsName string + if role == Master { + dnsName = c.masterDNSName() + } else { + dnsName = c.replicaDNSName() + } + + // Just set ELB Timeout annotation with default value, if it does not + // have a cutom value + if _, ok := annotations[constants.ElbTimeoutAnnotationName]; !ok { + annotations[constants.ElbTimeoutAnnotationName] = constants.ElbTimeoutAnnotationValue + } + // External DNS name annotation is not customizable + annotations[constants.ZalandoDNSNameAnnotation] = dnsName + } + + if len(annotations) == 0 { + return nil + } + + return annotations +} + func (c *Cluster) generateEndpoint(role PostgresRole, subsets []v1.EndpointSubset) *v1.Endpoints { endpoints := &v1.Endpoints{ ObjectMeta: metav1.ObjectMeta{