Updating spec mutation logic of daemonset, statefulset, and deployment with mergeWithOverwriteWithEmptyValue

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

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# 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: "Improve 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

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

controllers/reconcile_test.go

@@ -196,9 +196,8 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 							// confirm the strategy has been changed
 							assert.Equal(t, d.Spec.Strategy.RollingUpdate.MaxUnavailable.IntVal, int32(1))
 							assert.Equal(t, d.Spec.Strategy.RollingUpdate.MaxSurge.IntVal, int32(1))
-							// confirm that we don't remove annotations and labels even if we don't set them
+							// confirm that we don't remove annotations from metadata even if we don't set them
 							assert.Contains(t, d.Annotations, annotationName)
-							assert.Contains(t, d.Labels, labelName)
 							actual := v1.Service{}
 							exists, err = populateObjectIfExists(t, &actual, namespacedObjectName(naming.Service(params.Name), params.Namespace))
 							assert.NoError(t, err)
@@ -757,9 +756,6 @@ func TestOpAMPBridgeReconciler_Reconcile(t *testing.T) {
 							exists, err := populateObjectIfExists(t, &d, namespacedObjectName(naming.OpAMPBridge(params.Name), params.Namespace))
 							assert.NoError(t, err)
 							assert.True(t, exists)
-							// confirm that we don't remove annotations and labels even if we don't set them
-							assert.Contains(t, d.Spec.Template.Annotations, annotationName)
-							assert.Contains(t, d.Labels, labelName)
 							actual := v1.Service{}
 							exists, err = populateObjectIfExists(t, &actual, namespacedObjectName(naming.OpAMPBridgeService(params.Name), params.Namespace))
 							assert.NoError(t, err)

internal/manifests/mutate.go

@@ -15,7 +15,6 @@
 package manifests
 
 import (
-	"errors"
 	"fmt"
 	"reflect"
 
@@ -33,8 +32,16 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
+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
@@ -71,7 +78,7 @@ func MutateFuncFor(existing, desired client.Object) controllerutil.MutateFn {
 		// Get the existing labels and override any conflicts with the desired labels
 		// This will preserve any labels on the existing set.
 		existingLabels := existing.GetLabels()
-		if err := mergeWithOverride(&existingLabels, desired.GetLabels()); err != nil {
+		if err := mergeWithOverwriteWithEmptyValue(&existingLabels, desired.GetLabels()); err != nil {
 			return err
 		}
 		existing.SetLabels(existingLabels)
@@ -265,84 +272,53 @@ 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 err := mergeWithOverwriteWithEmptyValue(&existing.Spec.Template.Spec.NodeSelector, desired.Spec.Template.Spec.NodeSelector); 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
+		}
 	}
-	return nil
+
+	return mergeWithOverwriteWithEmptyValue(&existing.Spec, desired.Spec)
 }
 
 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
-	}
-	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 {
-		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
+		}
 	}
-	return nil
+	return mergeWithOverwriteWithEmptyValue(&existing.Spec, desired.Spec)
 }
 
 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.PodManagementPolicy = desired.Spec.PodManagementPolicy
-	existing.Spec.Replicas = desired.Spec.Replicas
 
-	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 {
-		return err
-	}
-	return nil
+	return mergeWithOverwriteWithEmptyValue(&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)
-	}
-
-	if hasVolumeClaimsTemplatesChanged(existing, desired) {
-		return true, "Spec.VolumeClaimTemplates"
+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 false, ""
+	return nil
 }
 
 // hasVolumeClaimsTemplatesChanged if volume claims template change has been detected.