Only patch annotations + tests refactoring

Author: hughcapet

manifests/operator-service-account-rbac.yaml

@@ -102,6 +102,7 @@ rules:
   - delete
   - get
   - update
+  - patch
 # to check nodes for node readiness label
 - apiGroups:
   - ""
@@ -173,7 +174,6 @@ rules:
   - get
   - list
   - patch
-  - update
 # to CRUD cron jobs for logical backups
 - apiGroups:
   - batch

pkg/cluster/cluster.go

@@ -764,16 +764,6 @@ func (c *Cluster) compareAnnotations(old, new map[string]string) (bool, string)
 
 }
 
-func (c *Cluster) extractIgnoredAnnotations(annoList map[string]string) map[string]string {
-	result := make(map[string]string)
-	for _, ignore := range c.OpConfig.IgnoredAnnotations {
-		if _, ok := annoList[ignore]; ok {
-			result[ignore] = annoList[ignore]
-		}
-	}
-	return result
-}
-
 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",
@@ -790,10 +780,6 @@ func (c *Cluster) compareServices(old, new *v1.Service) (bool, string) {
 		}
 	}
 
-	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, ""
 }
 

pkg/cluster/cluster_test.go

@@ -1443,205 +1443,6 @@ func TestCompareServices(t *testing.T) {
 			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 {

pkg/cluster/connection_pooler.go

@@ -10,7 +10,6 @@ import (
 	"github.com/sirupsen/logrus"
 	acidzalando "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do"
 	acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1"
-	"golang.org/x/exp/maps"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -692,6 +691,26 @@ func updateConnectionPoolerDeployment(KubeClient k8sutil.KubernetesClient, newDe
 	return deployment, nil
 }
 
+// patchConnectionPoolerAnnotations updates the annotations of connection pooler deployment
+func patchConnectionPoolerAnnotations(KubeClient k8sutil.KubernetesClient, deployment *appsv1.Deployment, annotations map[string]string) (*appsv1.Deployment, error) {
+	patchData, err := metaAnnotationsPatch(annotations)
+	if err != nil {
+		return nil, fmt.Errorf("could not form patch for the connection pooler deployment metadata: %v", err)
+	}
+	result, err := KubeClient.Deployments(deployment.Namespace).Patch(
+		context.TODO(),
+		deployment.Name,
+		types.MergePatchType,
+		[]byte(patchData),
+		metav1.PatchOptions{},
+		"")
+	if err != nil {
+		return nil, fmt.Errorf("could not patch connection pooler annotations %q: %v", patchData, err)
+	}
+	return result, nil
+
+}
+
 // Test if two connection pooler configuration needs to be synced. For simplicity
 // compare not the actual K8S objects, but the configuration itself and request
 // sync if there is any difference.
@@ -1009,18 +1028,6 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql
 			syncReason = append(syncReason, []string{"new connection pooler's pod template annotations do not match the current one: " + reason}...)
 			deployment.Spec.Template.Annotations = newPodAnnotations
 		}
-		newAnnotations := c.AnnotationsToPropagate(c.annotationsSet(nil)) // including the downscaling annotations
-		ignoredAnnotations := c.extractIgnoredAnnotations(deployment.Annotations)
-		maps.Copy(newAnnotations, ignoredAnnotations)
-		if changed, reason := c.compareAnnotations(deployment.Annotations, newAnnotations); changed {
-			deployment.Annotations = newAnnotations
-			c.logger.Infof("new connection pooler deployments's annotations does not match the current one:" + reason)
-			c.logger.Debug("updating connection pooler deployments's annotations")
-			deployment, err = c.KubeClient.Deployments(deployment.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{})
-			if err != nil {
-				return nil, fmt.Errorf("could not update connection pooler annotations %q: %v", deployment.Name, err)
-			}
-		}
 
 		defaultsSync, defaultsReason := c.needSyncConnectionPoolerDefaults(&c.Config, newConnectionPooler, deployment)
 		syncReason = append(syncReason, defaultsReason...)
@@ -1040,6 +1047,15 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql
 			}
 			c.ConnectionPooler[role].Deployment = deployment
 		}
+
+		newAnnotations := c.AnnotationsToPropagate(c.annotationsSet(nil)) // including the downscaling annotations
+		if changed, _ := c.compareAnnotations(deployment.Annotations, newAnnotations); changed {
+			deployment, err = patchConnectionPoolerAnnotations(c.KubeClient, deployment, newAnnotations)
+			if err != nil {
+				return nil, err
+			}
+			c.ConnectionPooler[role].Deployment = deployment
+		}
 	}
 
 	// check if pooler pods must be replaced due to secret update
@@ -1068,13 +1084,13 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql
 				return nil, fmt.Errorf("could not delete pooler pod: %v", err)
 			}
 		} else if changed, _ := c.compareAnnotations(pod.Annotations, deployment.Spec.Template.Annotations); changed {
-			newAnnotations := c.extractIgnoredAnnotations(pod.Annotations)
-			maps.Copy(newAnnotations, deployment.Spec.Template.Annotations)
-			pod.Annotations = newAnnotations
-			c.logger.Debugf("updating annotations for connection pooler's pod %q", pod.Name)
-			_, err := c.KubeClient.Pods(pod.Namespace).Update(context.TODO(), &pod, metav1.UpdateOptions{})
+			patchData, err := metaAnnotationsPatch(deployment.Spec.Template.Annotations)
+			if err != nil {
+				return nil, fmt.Errorf("could not form patch for pooler's pod annotations: %v", err)
+			}
+			_, err = c.KubeClient.Pods(pod.Namespace).Patch(context.TODO(), pod.Name, types.MergePatchType, []byte(patchData), metav1.PatchOptions{})
 			if err != nil {
-				return nil, fmt.Errorf("could not update annotations for pod %q: %v", pod.Name, err)
+				return nil, fmt.Errorf("could not patch annotations for pooler's pod %q: %v", pod.Name, err)
 			}
 		}
 	}