From 86e31beb7e0b2345e33febfd8cb11168dbf53628 Mon Sep 17 00:00:00 2001 From: Robert Jacob Date: Tue, 4 Jul 2023 18:16:18 +0200 Subject: [PATCH 1/2] operator: Do not use mergo for updating existing resources --- operator/internal/manifests/mutate.go | 65 ++++++++++------------ operator/internal/manifests/mutate_test.go | 4 +- 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/operator/internal/manifests/mutate.go b/operator/internal/manifests/mutate.go index fbf234f94fa40..77f7d298354a7 100644 --- a/operator/internal/manifests/mutate.go +++ b/operator/internal/manifests/mutate.go @@ -16,14 +16,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -// MutateFuncFor returns a mutate function based on the -// existing resource's concrete type. It supports currently -// only the following types or else panics: -// - ConfigMap -// - Service -// - Deployment -// - StatefulSet -// - ServiceMonitor +// MutateFuncFor returns a mutate function based on the existing resource's concrete type. +// It currently supports the following types and will return an error for other types: +// +// - ConfigMap +// - Secret +// - Service +// - ServiceAccount +// - ClusterRole +// - ClusterRoleBinding +// - Role +// - RoleBinding +// - Deployment +// - StatefulSet +// - ServiceMonitor +// - Ingress +// - Route +// - PrometheusRule +// - PodDisruptionBudget func MutateFuncFor(existing, desired client.Object, depAnnotations map[string]string) controllerutil.MutateFn { return func() error { existingAnnotations := existing.GetAnnotations() @@ -61,7 +71,7 @@ func MutateFuncFor(existing, desired client.Object, depAnnotations map[string]st case *corev1.Service: svc := existing.(*corev1.Service) wantSvc := desired.(*corev1.Service) - return mutateService(svc, wantSvc) + mutateService(svc, wantSvc) case *corev1.ServiceAccount: sa := existing.(*corev1.ServiceAccount) @@ -91,12 +101,12 @@ func MutateFuncFor(existing, desired client.Object, depAnnotations map[string]st case *appsv1.Deployment: dpl := existing.(*appsv1.Deployment) wantDpl := desired.(*appsv1.Deployment) - return mutateDeployment(dpl, wantDpl) + mutateDeployment(dpl, wantDpl) case *appsv1.StatefulSet: sts := existing.(*appsv1.StatefulSet) wantSts := desired.(*appsv1.StatefulSet) - return mutateStatefulSet(sts, wantSts) + mutateStatefulSet(sts, wantSts) case *monitoringv1.ServiceMonitor: svcMonitor := existing.(*monitoringv1.ServiceMonitor) @@ -138,14 +148,6 @@ func mergeWithOverride(dst, src interface{}) error { return nil } -func mergeWithOverrideEmpty(dst, src interface{}) error { - err := mergo.Merge(dst, src, mergo.WithOverwriteWithEmptyValue) - if err != nil { - return kverrors.Wrap(err, "unable to mergeWithOverrideEmpty", "dst", dst, "src", src) - } - return nil -} - func mutateConfigMap(existing, desired *corev1.ConfigMap) { existing.Annotations = desired.Annotations existing.Labels = desired.Labels @@ -217,41 +219,30 @@ func mutatePrometheusRule(existing, desired *monitoringv1.PrometheusRule) { existing.Spec = desired.Spec } -func mutateService(existing, desired *corev1.Service) error { +func mutateService(existing, desired *corev1.Service) { existing.Spec.Ports = desired.Spec.Ports - if err := mergeWithOverride(&existing.Spec.Selector, desired.Spec.Selector); err != nil { - return err - } - return nil + existing.Spec.Selector = desired.Spec.Selector } -func mutateDeployment(existing, desired *appsv1.Deployment) error { +func mutateDeployment(existing, desired *appsv1.Deployment) { // Deployment selector is immutable so we set this value only if // a new object is going to be created if existing.CreationTimestamp.IsZero() { existing.Spec.Selector = desired.Spec.Selector } existing.Spec.Replicas = desired.Spec.Replicas - if err := mergeWithOverrideEmpty(&existing.Spec.Template, desired.Spec.Template); err != nil { - return err - } - if err := mergeWithOverride(&existing.Spec.Strategy, desired.Spec.Strategy); err != nil { - return err - } - return nil + existing.Spec.Template = desired.Spec.Template + existing.Spec.Strategy = desired.Spec.Strategy } -func mutateStatefulSet(existing, desired *appsv1.StatefulSet) error { +func mutateStatefulSet(existing, desired *appsv1.StatefulSet) { // StatefulSet selector is immutable so we set this value only if // a new object is going to be created if existing.CreationTimestamp.IsZero() { existing.Spec.Selector = desired.Spec.Selector } existing.Spec.Replicas = desired.Spec.Replicas - if err := mergeWithOverrideEmpty(&existing.Spec.Template, desired.Spec.Template); err != nil { - return err - } - return nil + existing.Spec.Template = desired.Spec.Template } func mutatePodDisruptionBudget(existing, desired *policyv1.PodDisruptionBudget) { diff --git a/operator/internal/manifests/mutate_test.go b/operator/internal/manifests/mutate_test.go index c58851bc956f6..eb82d68b56dfd 100644 --- a/operator/internal/manifests/mutate_test.go +++ b/operator/internal/manifests/mutate_test.go @@ -488,7 +488,7 @@ func TestGetMutateFunc_MutateRoleBinding(t *testing.T) { require.Exactly(t, got.Subjects, want.Subjects) } -func TestGeMutateFunc_MutateDeploymentSpec(t *testing.T) { +func TestMutateFuncFor_MutateDeploymentSpec(t *testing.T) { type test struct { name string got *appsv1.Deployment @@ -615,7 +615,7 @@ func TestGeMutateFunc_MutateDeploymentSpec(t *testing.T) { } } -func TestGeMutateFunc_MutateStatefulSetSpec(t *testing.T) { +func TestMutateFuncFor_MutateStatefulSetSpec(t *testing.T) { type test struct { name string got *appsv1.StatefulSet From 0150a95df30e571f0f296f5f5ebf58834b97ec0a Mon Sep 17 00:00:00 2001 From: Robert Jacob Date: Wed, 12 Jul 2023 14:40:31 +0200 Subject: [PATCH 2/2] Only overwrite certain fields of the PodSpec --- operator/internal/manifests/mutate.go | 20 ++++++- operator/internal/manifests/mutate_test.go | 66 ++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/operator/internal/manifests/mutate.go b/operator/internal/manifests/mutate.go index 77f7d298354a7..27421750bf2cc 100644 --- a/operator/internal/manifests/mutate.go +++ b/operator/internal/manifests/mutate.go @@ -231,8 +231,8 @@ func mutateDeployment(existing, desired *appsv1.Deployment) { existing.Spec.Selector = desired.Spec.Selector } existing.Spec.Replicas = desired.Spec.Replicas - existing.Spec.Template = desired.Spec.Template existing.Spec.Strategy = desired.Spec.Strategy + mutatePodTemplate(&existing.Spec.Template, &desired.Spec.Template) } func mutateStatefulSet(existing, desired *appsv1.StatefulSet) { @@ -242,7 +242,7 @@ func mutateStatefulSet(existing, desired *appsv1.StatefulSet) { existing.Spec.Selector = desired.Spec.Selector } existing.Spec.Replicas = desired.Spec.Replicas - existing.Spec.Template = desired.Spec.Template + mutatePodTemplate(&existing.Spec.Template, &desired.Spec.Template) } func mutatePodDisruptionBudget(existing, desired *policyv1.PodDisruptionBudget) { @@ -250,3 +250,19 @@ func mutatePodDisruptionBudget(existing, desired *policyv1.PodDisruptionBudget) existing.Labels = desired.Labels existing.Spec = desired.Spec } + +func mutatePodTemplate(existing, desired *corev1.PodTemplateSpec) { + existing.Annotations = desired.Annotations + existing.Labels = desired.Labels + mutatePodSpec(&existing.Spec, &desired.Spec) +} + +func mutatePodSpec(existing *corev1.PodSpec, desired *corev1.PodSpec) { + existing.Affinity = desired.Affinity + existing.Containers = desired.Containers + existing.InitContainers = desired.InitContainers + existing.NodeSelector = desired.NodeSelector + existing.Tolerations = desired.Tolerations + existing.TopologySpreadConstraints = desired.TopologySpreadConstraints + existing.Volumes = desired.Volumes +} diff --git a/operator/internal/manifests/mutate_test.go b/operator/internal/manifests/mutate_test.go index eb82d68b56dfd..4fac03e606569 100644 --- a/operator/internal/manifests/mutate_test.go +++ b/operator/internal/manifests/mutate_test.go @@ -591,6 +591,39 @@ func TestMutateFuncFor_MutateDeploymentSpec(t *testing.T) { }, }, }, + { + name: "remove extra annotations and labels on pod", + got: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "first-key": "first-value", + "second-key": "second-value", + }, + Labels: map[string]string{ + "first-key": "first-value", + "second-key": "second-value", + }, + }, + }, + }, + }, + want: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "first-key": "first-value", + }, + Labels: map[string]string{ + "first-key": "first-value", + }, + }, + }, + }, + }, + }, } for _, tst := range table { tst := tst @@ -748,6 +781,39 @@ func TestMutateFuncFor_MutateStatefulSetSpec(t *testing.T) { }, }, }, + { + name: "remove extra annotations and labels on pod", + got: &appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "first-key": "first-value", + "second-key": "second-value", + }, + Labels: map[string]string{ + "first-key": "first-value", + "second-key": "second-value", + }, + }, + }, + }, + }, + want: &appsv1.StatefulSet{ + Spec: appsv1.StatefulSetSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "first-key": "first-value", + }, + Labels: map[string]string{ + "first-key": "first-value", + }, + }, + }, + }, + }, + }, } for _, tst := range table { tst := tst