finish ignored_annotations feature

pkg/cluster/cluster.go

@@ -382,9 +382,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)
@@ -438,10 +439,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
@@ -720,6 +722,70 @@ func (c *Cluster) enforceMinResourceLimits(spec *acidv1.PostgresSpec) error {
 	return nil
 }
 
+func (c *Cluster) compareAnnotations(old, new map[string]string) (bool, string) {
+	ignored := make(map[string]bool)
+	for _, ignore := range c.OpConfig.IgnoredAnnotations {
+		ignored[ignore] = true
+	}
+
+	// changed := false
+	reason := ""
+
+	for key := range old {
+		if _, ok := ignored[key]; ok {
+			continue
+		}
+		if _, ok := new[key]; !ok {
+			reason += fmt.Sprintf(" Removed '%s'.", key)
+		}
+	}
+
+	for key := range new {
+		if _, ok := ignored[key]; ok {
+			continue
+		}
+
+		v, ok := old[key]
+		if !ok {
+			reason += fmt.Sprintf(" Added '%s' with value '%s'.", key, new[key])
+		} else if v != new[key] {
+			reason += fmt.Sprintf(" '%s' changed from '%s' to '%s'.", key, v, new[key])
+		}
+	}
+
+	if reason != "" {
+		return true, reason
+	}
+	return false, ""
+
+}
+
+// SameService compares the Services
+func (c *Cluster) compareServices(old, new *v1.Service) (bool, string) {
+	//TODO: improve comparison
+	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
@@ -821,7 +887,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

pkg/cluster/cluster_test.go

@@ -3,6 +3,7 @@ package cluster
 import (
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -1054,6 +1055,359 @@ func TestCompareEnv(t *testing.T) {
 	}
 }
 
+func newFakeK8sServiceClient() (k8sutil.KubernetesClient, *fake.Clientset) {
+	clientSet := fake.NewSimpleClientset()
+
+	return k8sutil.KubernetesClient{
+		ServicesGetter: clientSet.CoreV1(),
+	}, clientSet
+}
+
+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 TestSameService(t *testing.T) {
+	testName := "test comparing services"
+	client, _ := newFakeK8sServiceClient()
+	namespace := "default"
+
+	pg := acidv1.Postgresql{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "acid-fake-cluster",
+			Namespace: namespace,
+			Annotations: map[string]string{
+				"deployment-time": "2022-02-02 12:00:00",
+			},
+		},
+		Spec: acidv1.PostgresSpec{
+			Volume: acidv1.Volume{
+				Size: "1Gi",
+			},
+		},
+	}
+	var cluster = New(
+		Config{
+			OpConfig: config.Config{
+				Resources: config.Resources{
+					IgnoredAnnotations: []string{
+						"k8s.v1.cni.cncf.io/network-status",
+					},
+				},
+			},
+		}, client, pg, logger, eventRecorder)
+
+	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()

pkg/cluster/connection_pooler.go

@@ -910,7 +910,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)

pkg/cluster/sync.go

@@ -172,7 +172,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 {