From 654d22d04a1863d7ade4a61829379b68ba7b276c Mon Sep 17 00:00:00 2001 From: Felix Kunde <felix-kunde@gmx.de> Date: Thu, 24 Mar 2022 18:38:37 +0100 Subject: [PATCH] Configure annotations to be ignored in comparisons during sync (#1823) * feat: add ignored annotations when comparing during sync Co-authored-by: Felix Kunde <felix-kunde@gmx.de> Co-authored-by: Moshe Immerman <moshe@flanksource.com> --- .../crds/operatorconfigurations.yaml | 4 + charts/postgres-operator/values.yaml | 5 + cmd/main.go | 3 +- docs/reference/operator_parameters.md | 6 + e2e/tests/test_e2e.py | 43 +++ manifests/configmap.yaml | 1 + manifests/operatorconfiguration.crd.yaml | 4 + ...gresql-operator-default-configuration.yaml | 2 + pkg/apis/acid.zalan.do/v1/crds.go | 8 + .../v1/operator_configuration_type.go | 1 + .../acid.zalan.do/v1/zz_generated.deepcopy.go | 5 + pkg/cluster/cluster.go | 67 +++- pkg/cluster/cluster_test.go | 331 ++++++++++++++++++ pkg/cluster/connection_pooler.go | 2 +- pkg/cluster/sync.go | 2 +- pkg/controller/operator_config.go | 1 + pkg/spec/types.go | 1 + pkg/util/config/config.go | 1 + pkg/util/k8sutil/k8sutil.go | 51 --- pkg/util/k8sutil/k8sutil_test.go | 311 ---------------- 20 files changed, 479 insertions(+), 370 deletions(-) delete mode 100644 pkg/util/k8sutil/k8sutil_test.go diff --git a/charts/postgres-operator/crds/operatorconfigurations.yaml b/charts/postgres-operator/crds/operatorconfigurations.yaml index 5d76b954..283acee2 100644 --- a/charts/postgres-operator/crds/operatorconfigurations.yaml +++ b/charts/postgres-operator/crds/operatorconfigurations.yaml @@ -212,6 +212,10 @@ spec: enable_sidecars: type: boolean default: true + ignored_annotations: + type: array + items: + type: string infrastructure_roles_secret_name: type: string infrastructure_roles_secrets: diff --git a/charts/postgres-operator/values.yaml b/charts/postgres-operator/values.yaml index 4c373ac8..2ce26d18 100644 --- a/charts/postgres-operator/values.yaml +++ b/charts/postgres-operator/values.yaml @@ -124,6 +124,11 @@ configKubernetes: enable_pod_disruption_budget: true # enables sidecar containers to run alongside Spilo in the same pod enable_sidecars: true + + # annotations to be ignored when comparing statefulsets, services etc. + # ignored_annotations: + # - k8s.v1.cni.cncf.io/network-status + # namespaced name of the secret containing infrastructure roles names and passwords # infrastructure_roles_secret_name: postgresql-infrastructure-roles diff --git a/cmd/main.go b/cmd/main.go index 376df0ba..0b48ac86 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -2,13 +2,14 @@ package main import ( "flag" - log "github.com/sirupsen/logrus" "os" "os/signal" "sync" "syscall" "time" + log "github.com/sirupsen/logrus" + "github.com/zalando/postgres-operator/pkg/controller" "github.com/zalando/postgres-operator/pkg/spec" "github.com/zalando/postgres-operator/pkg/util/k8sutil" diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index 4e0d0d2e..d245be58 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -287,6 +287,12 @@ configuration they are grouped under the `kubernetes` key. Regular expressions like `downscaler/*` etc. are also accepted. Can be used with [kube-downscaler](https://github.com/hjacobs/kube-downscaler). +* **ignored_annotations** + Some K8s tools inject and update annotations out of the Postgres Operator + control. This can cause rolling updates on each cluster sync cycle. With + this option you can specify an array of annotation keys that should be + ignored when comparing K8s resources on sync. The default is empty. + * **watched_namespace** The operator watches for Postgres objects in the given namespace. If not specified, the value is taken from the operator namespace. A special `*` diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 44c72cb4..b2977b68 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -704,6 +704,49 @@ class EndToEndTestCase(unittest.TestCase): print('Operator log: {}'.format(k8s.get_operator_log())) raise + @timeout_decorator.timeout(TEST_TIMEOUT_SEC) + def test_ignored_annotations(self): + ''' + Test if injected annotation does not cause replacement of resources when listed under ignored_annotations + ''' + k8s = self.k8s + + annotation_patch = { + "metadata": { + "annotations": { + "k8s-status": "healthy" + }, + } + } + + try: + sts = k8s.api.apps_v1.read_namespaced_stateful_set('acid-minimal-cluster', 'default') + old_sts_creation_timestamp = sts.metadata.creation_timestamp + k8s.api.apps_v1.patch_namespaced_stateful_set(sts.metadata.name, sts.metadata.namespace, annotation_patch) + svc = k8s.api.core_v1.read_namespaced_service('acid-minimal-cluster', 'default') + old_svc_creation_timestamp = svc.metadata.creation_timestamp + k8s.api.core_v1.patch_namespaced_service(svc.metadata.name, svc.metadata.namespace, annotation_patch) + + patch_config_ignored_annotations = { + "data": { + "ignored_annotations": "k8s-status", + } + } + k8s.update_config(patch_config_ignored_annotations) + self.eventuallyEqual(lambda: k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync") + + sts = k8s.api.apps_v1.read_namespaced_stateful_set('acid-minimal-cluster', 'default') + new_sts_creation_timestamp = sts.metadata.creation_timestamp + svc = k8s.api.core_v1.read_namespaced_service('acid-minimal-cluster', 'default') + new_svc_creation_timestamp = svc.metadata.creation_timestamp + + self.assertEqual(old_sts_creation_timestamp, new_sts_creation_timestamp, "unexpected replacement of statefulset on sync") + self.assertEqual(old_svc_creation_timestamp, new_svc_creation_timestamp, "unexpected replacement of master service on sync") + + except timeout_decorator.TimeoutError: + print('Operator log: {}'.format(k8s.get_operator_log())) + raise + @timeout_decorator.timeout(TEST_TIMEOUT_SEC) def test_infrastructure_roles(self): ''' diff --git a/manifests/configmap.yaml b/manifests/configmap.yaml index bc1fe6ff..130a3517 100644 --- a/manifests/configmap.yaml +++ b/manifests/configmap.yaml @@ -64,6 +64,7 @@ data: external_traffic_policy: "Cluster" # gcp_credentials: "" # kubernetes_use_configmaps: "false" + # ignored_annotations: "" # infrastructure_roles_secret_name: "postgresql-infrastructure-roles" # infrastructure_roles_secrets: "secretname:monitoring-roles,userkey:user,passwordkey:password,rolekey:inrole" # inherited_annotations: owned-by diff --git a/manifests/operatorconfiguration.crd.yaml b/manifests/operatorconfiguration.crd.yaml index c67d4f19..173e83c5 100644 --- a/manifests/operatorconfiguration.crd.yaml +++ b/manifests/operatorconfiguration.crd.yaml @@ -210,6 +210,10 @@ spec: enable_sidecars: type: boolean default: true + ignored_annotations: + type: array + items: + type: string infrastructure_roles_secret_name: type: string infrastructure_roles_secrets: diff --git a/manifests/postgresql-operator-default-configuration.yaml b/manifests/postgresql-operator-default-configuration.yaml index ea5032e3..a5ceb8d2 100644 --- a/manifests/postgresql-operator-default-configuration.yaml +++ b/manifests/postgresql-operator-default-configuration.yaml @@ -59,6 +59,8 @@ configuration: enable_pod_antiaffinity: false enable_pod_disruption_budget: true enable_sidecars: true + # ignored_annotations: + # - k8s.v1.cni.cncf.io/network-status # infrastructure_roles_secret_name: "postgresql-infrastructure-roles" # infrastructure_roles_secrets: # - secretname: "monitoring-roles" diff --git a/pkg/apis/acid.zalan.do/v1/crds.go b/pkg/apis/acid.zalan.do/v1/crds.go index fe436534..77828255 100644 --- a/pkg/apis/acid.zalan.do/v1/crds.go +++ b/pkg/apis/acid.zalan.do/v1/crds.go @@ -1251,6 +1251,14 @@ var OperatorConfigCRDResourceValidation = apiextv1.CustomResourceValidation{ "enable_sidecars": { Type: "boolean", }, + "ignored_annotations": { + Type: "array", + Items: &apiextv1.JSONSchemaPropsOrArray{ + Schema: &apiextv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, "infrastructure_roles_secret_name": { Type: "string", }, diff --git a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go index 3d94ae03..0be27555 100644 --- a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go +++ b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go @@ -82,6 +82,7 @@ type KubernetesMetaConfiguration struct { InheritedLabels []string `json:"inherited_labels,omitempty"` InheritedAnnotations []string `json:"inherited_annotations,omitempty"` DownscalerAnnotations []string `json:"downscaler_annotations,omitempty"` + IgnoredAnnotations []string `json:"ignored_annotations,omitempty"` ClusterNameLabel string `json:"cluster_name_label,omitempty"` DeleteAnnotationDateKey string `json:"delete_annotation_date_key,omitempty"` DeleteAnnotationNameKey string `json:"delete_annotation_name_key,omitempty"` 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 c4f5f077..26d930f2 100644 --- a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go +++ b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go @@ -228,6 +228,11 @@ func (in *KubernetesMetaConfiguration) DeepCopyInto(out *KubernetesMetaConfigura *out = make([]string, len(*in)) copy(*out, *in) } + if in.IgnoredAnnotations != nil { + in, out := &in.IgnoredAnnotations, &out.IgnoredAnnotations + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.NodeReadinessLabel != nil { in, out := &in.NodeReadinessLabel, &out.NodeReadinessLabel *out = make(map[string]string, len(*in)) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index eec7d77d..e411c66e 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -378,9 +378,10 @@ func (c *Cluster) compareStatefulSetWith(statefulSet *appsv1.StatefulSet) *compa match = false reasons = append(reasons, "new statefulset's number of replicas does not match the current one") } - if !reflect.DeepEqual(c.Statefulset.Annotations, statefulSet.Annotations) { + if changed, reason := c.compareAnnotations(c.Statefulset.Annotations, statefulSet.Annotations); changed { + match = false needsReplace = true - reasons = append(reasons, "new statefulset's annotations do not match the current one") + reasons = append(reasons, "new statefulset's annotations do not match: "+reason) } needsRollUpdate, reasons = c.compareContainers("initContainers", c.Statefulset.Spec.Template.Spec.InitContainers, statefulSet.Spec.Template.Spec.InitContainers, needsRollUpdate, reasons) @@ -434,10 +435,11 @@ func (c *Cluster) compareStatefulSetWith(statefulSet *appsv1.StatefulSet) *compa } } - if !reflect.DeepEqual(c.Statefulset.Spec.Template.Annotations, statefulSet.Spec.Template.Annotations) { + if changed, reason := c.compareAnnotations(c.Statefulset.Spec.Template.Annotations, statefulSet.Spec.Template.Annotations); changed { + match = false needsReplace = true needsRollUpdate = true - reasons = append(reasons, "new statefulset's pod template metadata annotations does not match the current one") + reasons = append(reasons, "new statefulset's pod template metadata annotations does not match "+reason) } if !reflect.DeepEqual(c.Statefulset.Spec.Template.Spec.SecurityContext, statefulSet.Spec.Template.Spec.SecurityContext) { needsReplace = true @@ -672,6 +674,61 @@ func comparePorts(a, b []v1.ContainerPort) bool { return true } +func (c *Cluster) compareAnnotations(old, new map[string]string) (bool, string) { + reason := "" + ignoredAnnotations := make(map[string]bool) + for _, ignore := range c.OpConfig.IgnoredAnnotations { + ignoredAnnotations[ignore] = true + } + + for key := range old { + if _, ok := ignoredAnnotations[key]; ok { + continue + } + if _, ok := new[key]; !ok { + reason += fmt.Sprintf(" Removed %q.", key) + } + } + + for key := range new { + if _, ok := ignoredAnnotations[key]; ok { + continue + } + v, ok := old[key] + if !ok { + reason += fmt.Sprintf(" Added %q with value %q.", key, new[key]) + } else if v != new[key] { + reason += fmt.Sprintf(" %q changed from %q to %q.", key, v, new[key]) + } + } + + return reason != "", reason + +} + +func (c *Cluster) compareServices(old, new *v1.Service) (bool, string) { + if old.Spec.Type != new.Spec.Type { + return false, fmt.Sprintf("new service's type %q does not match the current one %q", + new.Spec.Type, old.Spec.Type) + } + + oldSourceRanges := old.Spec.LoadBalancerSourceRanges + newSourceRanges := new.Spec.LoadBalancerSourceRanges + + /* work around Kubernetes 1.6 serializing [] as nil. See https://github.com/kubernetes/kubernetes/issues/43203 */ + if (len(oldSourceRanges) != 0) || (len(newSourceRanges) != 0) { + if !util.IsEqualIgnoreOrder(oldSourceRanges, newSourceRanges) { + return false, "new service's LoadBalancerSourceRange does not match the current one" + } + } + + if changed, reason := c.compareAnnotations(old.Annotations, new.Annotations); changed { + return !changed, "new service's annotations does not match the current one:" + reason + } + + return true, "" +} + // Update changes Kubernetes objects according to the new specification. Unlike the sync case, the missing object // (i.e. service) is treated as an error // logical backup cron jobs are an exception: a user-initiated Update can enable a logical backup job @@ -764,7 +821,7 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error { updateFailed = true return } - if syncStatefulSet || !reflect.DeepEqual(oldSs, newSs) || !reflect.DeepEqual(oldSpec.Annotations, newSpec.Annotations) { + if syncStatefulSet || !reflect.DeepEqual(oldSs, newSs) { c.logger.Debugf("syncing statefulsets") syncStatefulSet = false // TODO: avoid generating the StatefulSet object twice by passing it to syncStatefulSet diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 401b1bc9..acfecc16 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -3,6 +3,7 @@ package cluster import ( "fmt" "reflect" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -1054,6 +1055,336 @@ func TestCompareEnv(t *testing.T) { } } +func newService(ann map[string]string, svcT v1.ServiceType, lbSr []string) *v1.Service { + svc := &v1.Service{ + Spec: v1.ServiceSpec{ + Type: svcT, + LoadBalancerSourceRanges: lbSr, + }, + } + svc.Annotations = ann + return svc +} + +func TestCompareServices(t *testing.T) { + testName := "TestCompareServices" + cluster := Cluster{ + Config: Config{ + OpConfig: config.Config{ + Resources: config.Resources{ + IgnoredAnnotations: []string{ + "k8s.v1.cni.cncf.io/network-status", + }, + }, + }, + }, + } + + tests := []struct { + about string + current *v1.Service + new *v1.Service + reason string + match bool + }{ + { + about: "two equal services", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeClusterIP, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeClusterIP, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: true, + }, + { + about: "services differ on service type", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeClusterIP, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's type "LoadBalancer" does not match the current one "ClusterIP"`, + }, + { + about: "services differ on lb source ranges", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"185.249.56.0/22"}), + match: false, + reason: `new service's LoadBalancerSourceRange does not match the current one`, + }, + { + about: "new service doesn't have lb source ranges", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{}), + match: false, + reason: `new service's LoadBalancerSourceRange does not match the current one`, + }, + { + about: "services differ on DNS annotation", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "new_clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: "external-dns.alpha.kubernetes.io/hostname" changed from "clstr.acid.zalan.do" to "new_clstr.acid.zalan.do".`, + }, + { + about: "services differ on AWS ELB annotation", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: "1800", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout" changed from "3600" to "1800".`, + }, + { + about: "service changes existing annotation", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "baz", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: "foo" changed from "bar" to "baz".`, + }, + { + about: "service changes multiple existing annotations", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + "bar": "foo", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "baz", + "bar": "fooz", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + // Test just the prefix to avoid flakiness and map sorting + reason: `new service's annotations does not match the current one:`, + }, + { + about: "service adds a new custom annotation", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: Added "foo" with value "bar".`, + }, + { + about: "service removes a custom annotation", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: Removed "foo".`, + }, + { + about: "service removes a custom annotation and adds a new one", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "bar": "foo", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + reason: `new service's annotations does not match the current one: Removed "foo". Added "bar" with value "foo".`, + }, + { + about: "service removes a custom annotation, adds a new one and change another", + current: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "foo": "bar", + "zalan": "do", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + "bar": "foo", + "zalan": "do.com", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + // Test just the prefix to avoid flakiness and map sorting + reason: `new service's annotations does not match the current one: Removed "foo".`, + }, + { + about: "service add annotations", + current: newService( + map[string]string{}, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", + constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: false, + // Test just the prefix to avoid flakiness and map sorting + reason: `new service's annotations does not match the current one: Added `, + }, + { + about: "ignored annotations", + current: newService( + map[string]string{}, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + new: newService( + map[string]string{ + "k8s.v1.cni.cncf.io/network-status": "up", + }, + v1.ServiceTypeLoadBalancer, + []string{"128.141.0.0/16", "137.138.0.0/16"}), + match: true, + }, + } + + for _, tt := range tests { + t.Run(tt.about, func(t *testing.T) { + match, reason := cluster.compareServices(tt.current, tt.new) + if match && !tt.match { + t.Logf("match=%v current=%v, old=%v reason=%s", match, tt.current.Annotations, tt.new.Annotations, reason) + t.Errorf("%s - expected services to do not match: %q and %q", testName, tt.current, tt.new) + return + } + if !match && tt.match { + t.Errorf("%s - expected services to be the same: %q and %q", testName, tt.current, tt.new) + return + } + if !match && !tt.match { + if !strings.HasPrefix(reason, tt.reason) { + t.Errorf("%s - expected reason prefix %s, found %s", testName, tt.reason, reason) + return + } + } + }) + } +} + func TestCrossNamespacedSecrets(t *testing.T) { testName := "test secrets in different namespace" clientSet := fake.NewSimpleClientset() diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 238e9d43..1fba2eed 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -912,7 +912,7 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql if service, err = c.KubeClient.Services(c.Namespace).Get(context.TODO(), c.connectionPoolerName(role), metav1.GetOptions{}); err == nil { c.ConnectionPooler[role].Service = service desiredSvc := c.generateConnectionPoolerService(c.ConnectionPooler[role]) - if match, reason := k8sutil.SameService(service, desiredSvc); !match { + if match, reason := c.compareServices(service, desiredSvc); !match { syncReason = append(syncReason, reason) c.logServiceChanges(role, service, desiredSvc, false, reason) newService, err = c.updateService(role, service, desiredSvc) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 1c011426..7ff021ce 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -167,7 +167,7 @@ func (c *Cluster) syncService(role PostgresRole) error { if svc, err = c.KubeClient.Services(c.Namespace).Get(context.TODO(), c.serviceName(role), metav1.GetOptions{}); err == nil { c.Services[role] = svc desiredSvc := c.generateService(role, &c.Spec) - if match, reason := k8sutil.SameService(svc, desiredSvc); !match { + if match, reason := c.compareServices(svc, desiredSvc); !match { c.logServiceChanges(role, svc, desiredSvc, false, reason) updatedSvc, err := c.updateService(role, svc, desiredSvc) if err != nil { diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go index be7118d9..8e4ad1f6 100644 --- a/pkg/controller/operator_config.go +++ b/pkg/controller/operator_config.go @@ -108,6 +108,7 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur result.InheritedLabels = fromCRD.Kubernetes.InheritedLabels result.InheritedAnnotations = fromCRD.Kubernetes.InheritedAnnotations result.DownscalerAnnotations = fromCRD.Kubernetes.DownscalerAnnotations + result.IgnoredAnnotations = fromCRD.Kubernetes.IgnoredAnnotations result.ClusterNameLabel = util.Coalesce(fromCRD.Kubernetes.ClusterNameLabel, "cluster-name") result.DeleteAnnotationDateKey = fromCRD.Kubernetes.DeleteAnnotationDateKey result.DeleteAnnotationNameKey = fromCRD.Kubernetes.DeleteAnnotationNameKey diff --git a/pkg/spec/types.go b/pkg/spec/types.go index 428bcbdc..e594a797 100644 --- a/pkg/spec/types.go +++ b/pkg/spec/types.go @@ -119,6 +119,7 @@ type ControllerConfig struct { CRDReadyWaitTimeout time.Duration ConfigMapName NamespacedName Namespace string + IgnoredAnnotations []string EnableJsonLogging bool } diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go index e6b2c03c..eb2eb07c 100644 --- a/pkg/util/config/config.go +++ b/pkg/util/config/config.go @@ -42,6 +42,7 @@ type Resources struct { InheritedLabels []string `name:"inherited_labels" default:""` InheritedAnnotations []string `name:"inherited_annotations" default:""` DownscalerAnnotations []string `name:"downscaler_annotations"` + IgnoredAnnotations []string `name:"ignored_annotations"` ClusterNameLabel string `name:"cluster_name_label" default:"cluster-name"` DeleteAnnotationDateKey string `name:"delete_annotation_date_key"` DeleteAnnotationNameKey string `name:"delete_annotation_name_key"` diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index fd5de819..f4ea014f 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -213,57 +213,6 @@ func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.Namespaced return pg, nil } -// SameService compares the Services -func SameService(cur, new *v1.Service) (match bool, reason string) { - //TODO: improve comparison - if cur.Spec.Type != new.Spec.Type { - return false, fmt.Sprintf("new service's type %q does not match the current one %q", - new.Spec.Type, cur.Spec.Type) - } - - oldSourceRanges := cur.Spec.LoadBalancerSourceRanges - newSourceRanges := new.Spec.LoadBalancerSourceRanges - - /* work around Kubernetes 1.6 serializing [] as nil. See https://github.com/kubernetes/kubernetes/issues/43203 */ - if (len(oldSourceRanges) != 0) || (len(newSourceRanges) != 0) { - if !reflect.DeepEqual(oldSourceRanges, newSourceRanges) { - return false, "new service's LoadBalancerSourceRange does not match the current one" - } - } - - match = true - - reasonPrefix := "new service's annotations does not match the current one:" - for ann := range cur.Annotations { - if _, ok := new.Annotations[ann]; !ok { - match = false - if len(reason) == 0 { - reason = reasonPrefix - } - reason += fmt.Sprintf(" Removed '%s'.", ann) - } - } - - for ann := range new.Annotations { - v, ok := cur.Annotations[ann] - if !ok { - if len(reason) == 0 { - reason = reasonPrefix - } - reason += fmt.Sprintf(" Added '%s' with value '%s'.", ann, new.Annotations[ann]) - match = false - } else if v != new.Annotations[ann] { - if len(reason) == 0 { - reason = reasonPrefix - } - reason += fmt.Sprintf(" '%s' changed from '%s' to '%s'.", ann, v, new.Annotations[ann]) - match = false - } - } - - return match, reason -} - // SamePDB compares the PodDisruptionBudgets func SamePDB(cur, new *policybeta1.PodDisruptionBudget) (match bool, reason string) { //TODO: improve comparison diff --git a/pkg/util/k8sutil/k8sutil_test.go b/pkg/util/k8sutil/k8sutil_test.go deleted file mode 100644 index b3e76850..00000000 --- a/pkg/util/k8sutil/k8sutil_test.go +++ /dev/null @@ -1,311 +0,0 @@ -package k8sutil - -import ( - "strings" - "testing" - - "github.com/zalando/postgres-operator/pkg/util/constants" - - v1 "k8s.io/api/core/v1" -) - -func newsService(ann map[string]string, svcT v1.ServiceType, lbSr []string) *v1.Service { - svc := &v1.Service{ - Spec: v1.ServiceSpec{ - Type: svcT, - LoadBalancerSourceRanges: lbSr, - }, - } - svc.Annotations = ann - return svc -} - -func TestSameService(t *testing.T) { - tests := []struct { - about string - current *v1.Service - new *v1.Service - reason string - match bool - }{ - { - about: "two equal services", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeClusterIP, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeClusterIP, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: true, - }, - { - about: "services differ on service type", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeClusterIP, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's type "LoadBalancer" does not match the current one "ClusterIP"`, - }, - { - about: "services differ on lb source ranges", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"185.249.56.0/22"}), - match: false, - reason: `new service's LoadBalancerSourceRange does not match the current one`, - }, - { - about: "new service doesn't have lb source ranges", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{}), - match: false, - reason: `new service's LoadBalancerSourceRange does not match the current one`, - }, - { - about: "services differ on DNS annotation", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "new_clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: 'external-dns.alpha.kubernetes.io/hostname' changed from 'clstr.acid.zalan.do' to 'new_clstr.acid.zalan.do'.`, - }, - { - about: "services differ on AWS ELB annotation", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: "1800", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: 'service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout' changed from '3600' to '1800'.`, - }, - { - about: "service changes existing annotation", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "baz", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: 'foo' changed from 'bar' to 'baz'.`, - }, - { - about: "service changes multiple existing annotations", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - "bar": "foo", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "baz", - "bar": "fooz", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - // Test just the prefix to avoid flakiness and map sorting - reason: `new service's annotations does not match the current one:`, - }, - { - about: "service adds a new custom annotation", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: Added 'foo' with value 'bar'.`, - }, - { - about: "service removes a custom annotation", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: Removed 'foo'.`, - }, - { - about: "service removes a custom annotation and adds a new one", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "bar": "foo", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - reason: `new service's annotations does not match the current one: Removed 'foo'. Added 'bar' with value 'foo'.`, - }, - { - about: "service removes a custom annotation, adds a new one and change another", - current: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "foo": "bar", - "zalan": "do", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - "bar": "foo", - "zalan": "do.com", - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - // Test just the prefix to avoid flakiness and map sorting - reason: `new service's annotations does not match the current one: Removed 'foo'.`, - }, - { - about: "service add annotations", - current: newsService( - map[string]string{}, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - new: newsService( - map[string]string{ - constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do", - constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue, - }, - v1.ServiceTypeLoadBalancer, - []string{"128.141.0.0/16", "137.138.0.0/16"}), - match: false, - // Test just the prefix to avoid flakiness and map sorting - reason: `new service's annotations does not match the current one: Added `, - }, - } - for _, tt := range tests { - t.Run(tt.about, func(t *testing.T) { - match, reason := SameService(tt.current, tt.new) - if match && !tt.match { - t.Errorf("expected services to do not match: '%q' and '%q'", tt.current, tt.new) - return - } - if !match && tt.match { - t.Errorf("expected services to be the same: '%q' and '%q'", tt.current, tt.new) - return - } - if !match && !tt.match { - if !strings.HasPrefix(reason, tt.reason) { - t.Errorf("expected reason prefix '%s', found '%s'", tt.reason, reason) - return - } - } - }) - } -} -- GitLab