From 744c71d16bd93b2d94b74fabc5e1f37a1eaa58c2 Mon Sep 17 00:00:00 2001 From: Jonathan Juares Beber Date: Thu, 13 Feb 2020 10:55:30 +0100 Subject: [PATCH] Allow services update when changing annotations (#818) The current implementations for `pkg.util.k8sutil.SameService` considers only service annotations change on the default annotations created by the operator. Custom annotations are not compared and consequently not applied after the first service creation. This commit introduces a complete annotations comparison between the current service created by the operator and the new one generated based on the configs. Also, it adds tests on the above-mentioned function. --- e2e/tests/test_e2e.py | 11 +- ...res-manifest-with-service-annotations.yaml | 20 -- pkg/util/k8sutil/k8sutil.go | 41 ++- pkg/util/k8sutil/k8sutil_test.go | 310 ++++++++++++++++++ 4 files changed, 348 insertions(+), 34 deletions(-) delete mode 100644 manifests/postgres-manifest-with-service-annotations.yaml create mode 100644 pkg/util/k8sutil/k8sutil_test.go diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index e3f00da9c..e92aba11f 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -270,7 +270,16 @@ class EndToEndTestCase(unittest.TestCase): } k8s.update_config(patch_custom_service_annotations) - k8s.create_with_kubectl("manifests/postgres-manifest-with-service-annotations.yaml") + pg_patch_custom_annotations = { + "spec": { + "serviceAnnotations": { + "annotation.key": "value" + } + } + } + k8s.api.custom_objects_api.patch_namespaced_custom_object( + "acid.zalan.do", "v1", "default", "postgresqls", "acid-minimal-cluster", pg_patch_custom_annotations) + annotations = { "annotation.key": "value", "foo": "bar", diff --git a/manifests/postgres-manifest-with-service-annotations.yaml b/manifests/postgres-manifest-with-service-annotations.yaml deleted file mode 100644 index ab3096740..000000000 --- a/manifests/postgres-manifest-with-service-annotations.yaml +++ /dev/null @@ -1,20 +0,0 @@ -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/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index 118d1df53..c7b2366b0 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -9,7 +9,6 @@ import ( batchv1beta1 "k8s.io/api/batch/v1beta1" clientbatchv1beta1 "k8s.io/client-go/kubernetes/typed/batch/v1beta1" - "github.com/zalando/postgres-operator/pkg/util/constants" v1 "k8s.io/api/core/v1" policybeta1 "k8s.io/api/policy/v1beta1" apiextclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" @@ -136,21 +135,37 @@ func SameService(cur, new *v1.Service) (match bool, reason string) { } } - oldDNSAnnotation := cur.Annotations[constants.ZalandoDNSNameAnnotation] - newDNSAnnotation := new.Annotations[constants.ZalandoDNSNameAnnotation] - oldELBAnnotation := cur.Annotations[constants.ElbTimeoutAnnotationName] - newELBAnnotation := new.Annotations[constants.ElbTimeoutAnnotationName] + match = true - if oldDNSAnnotation != newDNSAnnotation { - return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q", - constants.ZalandoDNSNameAnnotation, newDNSAnnotation, oldDNSAnnotation) - } - if oldELBAnnotation != newELBAnnotation { - return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q", - constants.ElbTimeoutAnnotationName, oldELBAnnotation, newELBAnnotation) + reasonPrefix := "new service's annotations doesn't 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) + } } - return true, "" + 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 diff --git a/pkg/util/k8sutil/k8sutil_test.go b/pkg/util/k8sutil/k8sutil_test.go new file mode 100644 index 000000000..12288243e --- /dev/null +++ b/pkg/util/k8sutil/k8sutil_test.go @@ -0,0 +1,310 @@ +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 TestServiceAnnotations(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" doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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 doesn't 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, + reason: `new service's annotations doesn't match the current one: Removed 'foo'. Added 'bar' with value 'foo'. 'zalan' changed from 'do' to 'do.com'`, + }, + { + 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 doesn't 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 '%s', found '%s'", tt.reason, reason) + return + } + } + }) + } +}