Updating spec mutation logic of daemonset, statefulset, and deploymen…

…t with mergeWithOverwriteWithEmptyValue (#3324)

* e2e tests for additionalContainers added

* additionalContainers related unit tests added for mutation

* Changed apiversion to v1beta1 in nodeselector e2e test

* removed explicit zero value for additionalContainers and changed apply to update in chainsaw test

* added affinity in collector e2e tests

* affinity unit tests added for daemonset, deployment, statefulset during mutation

* collector container mutate args e2e tests added

* Unit tests added for additional args mutation of collector container

* e2e tests for changing labels in collectors

* e2e tests for changing annotations in collectors

* fix annotation change e2e test asserts

* Error and label change related unit tests added for resource mutation

* fix label change e2e tests for mutation

* mutating the spec and labels of deployment, daemonset, statefulset with mergeWithOverwriteWithEmptyValue

* Adjust reconcile tests to new mutation logic

* Added chlog entry for new mutation logic

* fix typo in mutate_test.go

* Fix G601: Implicit memory aliasing in mutate_test.go

* Revert "Adjust reconcile tests to new mutation logic"

This reverts commit 9060661.

* label and annotation changes with mergeWithOverride; adjust tests

* copy over desired.spec.template.spec to existing.spec.template.spec

* volumeClaimTemplates mutation through range

* Change type to bugfix

* Fix volume-claim-label e2e test

---------

Co-authored-by: Israel Blancas <[email protected]>

.chloggen/2947-updating-ds-sf-depl-mutation.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Fix mutation of deployments, statefulsets, and daemonsets allowing to remove fields on update"
+
+# One or more tracking issues related to the change
+issues: [2947]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:

controllers/common.go

@@ -160,7 +160,7 @@ func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logg
 			op = result
 			return createOrUpdateErr
 		})
-		if crudErr != nil && errors.Is(crudErr, manifests.ImmutableChangeErr) {
+		if crudErr != nil && errors.As(crudErr, &manifests.ImmutableChangeErr) {
 			l.Error(crudErr, "detected immutable field change, trying to delete, new object will be created on next reconcile", "existing", existing.GetName())
 			delErr := kubeClient.Delete(ctx, existing)
 			if delErr != nil {

internal/manifests/mutate.go

@@ -15,7 +15,6 @@
 package manifests
 
 import (
-	"errors"
 	"fmt"
 	"reflect"
 
@@ -36,8 +35,16 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 )
 
+type ImmutableFieldChangeErr struct {
+	Field string
+}
+
+func (e *ImmutableFieldChangeErr) Error() string {
+	return fmt.Sprintf("Immutable field change attempted: %s", e.Field)
+}
+
 var (
-	ImmutableChangeErr = errors.New("immutable field change attempted")
+	ImmutableChangeErr *ImmutableFieldChangeErr
 )
 
 // MutateFuncFor returns a mutate function based on the
@@ -197,10 +204,6 @@ func mergeWithOverride(dst, src interface{}) error {
 	return mergo.Merge(dst, src, mergo.WithOverride)
 }
 
-func mergeWithOverwriteWithEmptyValue(dst, src interface{}) error {
-	return mergo.Merge(dst, src, mergo.WithOverwriteWithEmptyValue)
-}
-
 func mutateSecret(existing, desired *corev1.Secret) {
 	existing.Labels = desired.Labels
 	existing.Annotations = desired.Annotations
@@ -290,68 +293,82 @@ func mutateService(existing, desired *corev1.Service) {
 }
 
 func mutateDaemonset(existing, desired *appsv1.DaemonSet) error {
-	if !existing.CreationTimestamp.IsZero() && !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
-		return ImmutableChangeErr
-	}
-	// Daemonset 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
-	}
-	if err := mergeWithOverride(&existing.Spec, desired.Spec); err != nil {
-		return err
+	if !existing.CreationTimestamp.IsZero() {
+		if !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
+			return &ImmutableFieldChangeErr{Field: "Spec.Selector"}
+		}
+		if err := hasImmutableLabelChange(existing.Spec.Selector.MatchLabels, desired.Spec.Template.Labels); err != nil {
+			return err
+		}
 	}
-	if err := mergeWithOverwriteWithEmptyValue(&existing.Spec.Template.Spec.NodeSelector, desired.Spec.Template.Spec.NodeSelector); err != nil {
+
+	existing.Spec.MinReadySeconds = desired.Spec.MinReadySeconds
+	existing.Spec.RevisionHistoryLimit = desired.Spec.RevisionHistoryLimit
+	existing.Spec.UpdateStrategy = desired.Spec.UpdateStrategy
+
+	if err := mutatePodTemplate(&existing.Spec.Template, &desired.Spec.Template); err != nil {
 		return err
 	}
+
 	return nil
 }
 
 func mutateDeployment(existing, desired *appsv1.Deployment) error {
-	if !existing.CreationTimestamp.IsZero() && !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
-		return ImmutableChangeErr
-	}
-	// 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
+	if !existing.CreationTimestamp.IsZero() {
+		if !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
+			return &ImmutableFieldChangeErr{Field: "Spec.Selector"}
+		}
+		if err := hasImmutableLabelChange(existing.Spec.Selector.MatchLabels, desired.Spec.Template.Labels); err != nil {
+			return err
+		}
 	}
+
+	existing.Spec.MinReadySeconds = desired.Spec.MinReadySeconds
+	existing.Spec.Paused = desired.Spec.Paused
+	existing.Spec.ProgressDeadlineSeconds = desired.Spec.ProgressDeadlineSeconds
 	existing.Spec.Replicas = desired.Spec.Replicas
-	if err := mergeWithOverride(&existing.Spec.Template, desired.Spec.Template); err != nil {
-		return err
-	}
-	if err := mergeWithOverwriteWithEmptyValue(&existing.Spec.Template.Spec.NodeSelector, desired.Spec.Template.Spec.NodeSelector); err != nil {
-		return err
-	}
-	if err := mergeWithOverride(&existing.Spec.Strategy, desired.Spec.Strategy); err != nil {
+	existing.Spec.RevisionHistoryLimit = desired.Spec.RevisionHistoryLimit
+	existing.Spec.Strategy = desired.Spec.Strategy
+
+	if err := mutatePodTemplate(&existing.Spec.Template, &desired.Spec.Template); err != nil {
 		return err
 	}
+
 	return nil
 }
 
 func mutateStatefulSet(existing, desired *appsv1.StatefulSet) error {
-	if hasChange, field := hasImmutableFieldChange(existing, desired); hasChange {
-		return fmt.Errorf("%s is being changed, %w", field, ImmutableChangeErr)
-	}
-	// 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
+	if !existing.CreationTimestamp.IsZero() {
+		if !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
+			return &ImmutableFieldChangeErr{Field: "Spec.Selector"}
+		}
+		if err := hasImmutableLabelChange(existing.Spec.Selector.MatchLabels, desired.Spec.Template.Labels); err != nil {
+			return err
+		}
+		if hasVolumeClaimsTemplatesChanged(existing, desired) {
+			return &ImmutableFieldChangeErr{Field: "Spec.VolumeClaimTemplates"}
+		}
 	}
+
+	existing.Spec.MinReadySeconds = desired.Spec.MinReadySeconds
+	existing.Spec.Ordinals = desired.Spec.Ordinals
+	existing.Spec.PersistentVolumeClaimRetentionPolicy = desired.Spec.PersistentVolumeClaimRetentionPolicy
 	existing.Spec.PodManagementPolicy = desired.Spec.PodManagementPolicy
 	existing.Spec.Replicas = desired.Spec.Replicas
+	existing.Spec.RevisionHistoryLimit = desired.Spec.RevisionHistoryLimit
+	existing.Spec.ServiceName = desired.Spec.ServiceName
+	existing.Spec.UpdateStrategy = desired.Spec.UpdateStrategy
 
 	for i := range existing.Spec.VolumeClaimTemplates {
 		existing.Spec.VolumeClaimTemplates[i].TypeMeta = desired.Spec.VolumeClaimTemplates[i].TypeMeta
 		existing.Spec.VolumeClaimTemplates[i].ObjectMeta = desired.Spec.VolumeClaimTemplates[i].ObjectMeta
 		existing.Spec.VolumeClaimTemplates[i].Spec = desired.Spec.VolumeClaimTemplates[i].Spec
 	}
-	if err := mergeWithOverride(&existing.Spec.Template, desired.Spec.Template); err != nil {
-		return err
-	}
-	if err := mergeWithOverwriteWithEmptyValue(&existing.Spec.Template.Spec.NodeSelector, desired.Spec.Template.Spec.NodeSelector); err != nil {
+
+	if err := mutatePodTemplate(&existing.Spec.Template, &desired.Spec.Template); err != nil {
 		return err
 	}
+
 	return nil
 }
 
@@ -367,19 +384,28 @@ func mutateIssuer(existing, desired *cmv1.Issuer) {
 	existing.Spec = desired.Spec
 }
 
-func hasImmutableFieldChange(existing, desired *appsv1.StatefulSet) (bool, string) {
-	if existing.CreationTimestamp.IsZero() {
-		return false, ""
-	}
-	if !apiequality.Semantic.DeepEqual(desired.Spec.Selector, existing.Spec.Selector) {
-		return true, fmt.Sprintf("Spec.Selector: desired: %s existing: %s", desired.Spec.Selector, existing.Spec.Selector)
+func mutatePodTemplate(existing, desired *corev1.PodTemplateSpec) error {
+	if err := mergeWithOverride(&existing.Labels, desired.Labels); err != nil {
+		return err
 	}
 
-	if hasVolumeClaimsTemplatesChanged(existing, desired) {
-		return true, "Spec.VolumeClaimTemplates"
+	if err := mergeWithOverride(&existing.Annotations, desired.Annotations); err != nil {
+		return err
 	}
 
-	return false, ""
+	existing.Spec = desired.Spec
+
+	return nil
+
+}
+
+func hasImmutableLabelChange(existingSelectorLabels, desiredLabels map[string]string) error {
+	for k, v := range existingSelectorLabels {
+		if vv, ok := desiredLabels[k]; !ok || vv != v {
+			return &ImmutableFieldChangeErr{Field: "Spec.Template.Metadata.Labels"}
+		}
+	}
+	return nil
 }
 
 // hasVolumeClaimsTemplatesChanged if volume claims template change has been detected.
@@ -402,6 +428,9 @@ func hasVolumeClaimsTemplatesChanged(existing, desired *appsv1.StatefulSet) bool
 		if !apiequality.Semantic.DeepEqual(desired.Spec.VolumeClaimTemplates[i].Annotations, existing.Spec.VolumeClaimTemplates[i].Annotations) {
 			return true
 		}
+		if !apiequality.Semantic.DeepEqual(desired.Spec.VolumeClaimTemplates[i].Labels, existing.Spec.VolumeClaimTemplates[i].Labels) {
+			return true
+		}
 		if !apiequality.Semantic.DeepEqual(desired.Spec.VolumeClaimTemplates[i].Spec, existing.Spec.VolumeClaimTemplates[i].Spec) {
 			return true
 		}