Configure annotations to be ignored in comparisons during sync (zalando#1823)

* feat: add ignored annotations when comparing during sync

Co-authored-by: Felix Kunde <[email protected]>
Co-authored-by: Moshe Immerman <[email protected]>

Author: FxKu

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:

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
 

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"

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 `*`

e2e/tests/test_e2e.py

@@ -704,6 +704,49 @@ def test_enable_load_balancer(self):
             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):
         '''

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

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:

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"

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",
 							},

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"`

pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.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