Merge branch 'master' into lb-service-fix

e2e/tests/test_e2e.py

@@ -319,7 +319,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",

manifests/postgres-manifest-with-service-annotations.yaml

@@ -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

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

@@ -67,7 +67,7 @@ type KubernetesMetaConfiguration struct {
 	// TODO: use namespacedname
 	PodEnvironmentConfigMap    string        `json:"pod_environment_configmap,omitempty"`
 	PodPriorityClassName       string        `json:"pod_priority_class_name,omitempty"`
-	MasterPodMoveTimeout       time.Duration `json:"master_pod_move_timeout,omitempty"`
+	MasterPodMoveTimeout       Duration      `json:"master_pod_move_timeout,omitempty"`
 	EnablePodAntiAffinity      bool          `json:"enable_pod_antiaffinity,omitempty"`
 	PodAntiAffinityTopologyKey string        `json:"pod_antiaffinity_topology_key,omitempty"`
 	PodManagementPolicy        string        `json:"pod_management_policy,omitempty"`

pkg/controller/operator_config.go

@@ -66,7 +66,7 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
 	result.NodeReadinessLabel = fromCRD.Kubernetes.NodeReadinessLabel
 	result.PodPriorityClassName = fromCRD.Kubernetes.PodPriorityClassName
 	result.PodManagementPolicy = fromCRD.Kubernetes.PodManagementPolicy
-	result.MasterPodMoveTimeout = fromCRD.Kubernetes.MasterPodMoveTimeout
+	result.MasterPodMoveTimeout = time.Duration(fromCRD.Kubernetes.MasterPodMoveTimeout)
 	result.EnablePodAntiAffinity = fromCRD.Kubernetes.EnablePodAntiAffinity
 	result.PodAntiAffinityTopologyKey = fromCRD.Kubernetes.PodAntiAffinityTopologyKey
 

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]
+	match = true
-	newDNSAnnotation := new.Annotations[constants.ZalandoDNSNameAnnotation]
-	oldELBAnnotation := cur.Annotations[constants.ElbTimeoutAnnotationName]
-	newELBAnnotation := new.Annotations[constants.ElbTimeoutAnnotationName]
 
-	if oldDNSAnnotation != newDNSAnnotation {
+	reasonPrefix := "new service's annotations doesn't match the current one:"
-		return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
+	for ann := range cur.Annotations {
-			constants.ZalandoDNSNameAnnotation, newDNSAnnotation, oldDNSAnnotation)
+		if _, ok := new.Annotations[ann]; !ok {
-	}
+			match = false
-	if oldELBAnnotation != newELBAnnotation {
+			if len(reason) == 0 {
-		return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
+				reason = reasonPrefix
-			constants.ElbTimeoutAnnotationName, oldELBAnnotation, newELBAnnotation)
+			}
+			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

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
+				}
+			}
+		})
+	}
+}