fix: skip merge patch in normalizeTargetResources for resources without matching ignoreDifferences rules

Fixes #26588

Signed-off-by: Eugene Babichev <eugene.babichev@clickhouse.com>

Author: BEvgeniyS

controller/sync.go

@@ -429,6 +429,7 @@ func normalizeTargetResources(cr *comparisonResult) ([]*unstructured.Unstructure
 	if err != nil {
 		return nil, err
 	}
+	idc := diff.NewIgnoreDiffConfig(cr.diffConfig.Ignores(), cr.diffConfig.Overrides())
 	patchedTargets := []*unstructured.Unstructured{}
 	for idx, live := range cr.reconciliationResult.Live {
 		normalizedTarget := normalized.Targets[idx]
@@ -442,6 +443,20 @@ func normalizeTargetResources(cr *comparisonResult) ([]*unstructured.Unstructure
 			continue
 		}
 
+		// Only compute and apply the merge patch if this resource has matching
+		// ignoreDifferences rules. The merge patch copies ignored field values
+		// from the live object into the target so they are not changed during
+		// sync. Without matching rules there is nothing to copy, and applying
+		// the patch anyway corrupts fields that use patchStrategy:"replace"
+		// (e.g. PodDisruptionBudget.spec.selector) or arrays in CRDs that
+		// fall back to JSON merge patch.
+		gvk := normalizedTarget.GroupVersionKind()
+		hasIgnore, _ := idc.HasIgnoreDifference(gvk.Group, gvk.Kind, normalizedTarget.GetName(), normalizedTarget.GetNamespace())
+		if !hasIgnore {
+			patchedTargets = append(patchedTargets, originalTarget)
+			continue
+		}
+
 		var lookupPatchMeta *strategicpatch.PatchMetaFromStruct
 		versionedObject, err := scheme.Scheme.New(normalizedTarget.GroupVersionKind())
 		if err == nil {

controller/sync_test.go

@@ -485,6 +485,208 @@ func TestNormalizeTargetResources(t *testing.T) {
 	})
 }
 
+func TestNormalizeTargetResourcesPDB(t *testing.T) {
+	type fixture struct {
+		comparisonResult *comparisonResult
+	}
+	setup := func(t *testing.T, ignores []v1alpha1.ResourceIgnoreDifferences) *fixture {
+		t.Helper()
+		dc, err := diff.NewDiffConfigBuilder().
+			WithDiffSettings(ignores, nil, true, normalizers.IgnoreNormalizerOpts{}).
+			WithNoCache().
+			Build()
+		require.NoError(t, err)
+		live := test.YamlToUnstructured(testdata.LivePDBYaml)
+		target := test.YamlToUnstructured(testdata.TargetPDBYaml)
+		return &fixture{
+			&comparisonResult{
+				reconciliationResult: sync.ReconciliationResult{
+					Live:   []*unstructured.Unstructured{live},
+					Target: []*unstructured.Unstructured{target},
+				},
+				diffConfig: dc,
+			},
+		}
+	}
+	t.Run("will not corrupt PDB selector when no ignore rules match", func(t *testing.T) {
+		// PDB.spec.selector has patchStrategy:"replace" which causes
+		// strategic merge patch to always include the selector in the patch.
+		// Without the fix, normalizeTargetResources overwrites the target
+		// selector with the live selector, making sync a permanent no-op.
+		f := setup(t, []v1alpha1.ResourceIgnoreDifferences{})
+
+		// when
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		// then
+		require.NoError(t, err)
+		require.Len(t, targets, 1)
+		selector, ok, err := unstructured.NestedStringMap(targets[0].Object, "spec", "selector", "matchLabels")
+		require.NoError(t, err)
+		require.True(t, ok)
+		// Target selector must be preserved, not overwritten with live values
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/instance"])
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/name"])
+		assert.Equal(t, "kube-dns", selector["k8s-app"])
+		// Must NOT contain the live-only label
+		_, hasEksLabel := selector["eks.amazonaws.com/component"]
+		assert.False(t, hasEksLabel, "target selector should not contain live-only label eks.amazonaws.com/component")
+	})
+	t.Run("will not corrupt PDB selector when unrelated ignore rules exist", func(t *testing.T) {
+		// Even with ignore rules present, if they don't match the PDB,
+		// the target must not be modified.
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:        "apps",
+				Kind:         "Deployment",
+				JSONPointers: []string{"/spec/replicas"},
+			},
+		}
+		f := setup(t, ignores)
+
+		// when
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		// then
+		require.NoError(t, err)
+		require.Len(t, targets, 1)
+		selector, ok, err := unstructured.NestedStringMap(targets[0].Object, "spec", "selector", "matchLabels")
+		require.NoError(t, err)
+		require.True(t, ok)
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/instance"])
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/name"])
+		assert.Equal(t, "kube-dns", selector["k8s-app"])
+		_, hasEksLabel := selector["eks.amazonaws.com/component"]
+		assert.False(t, hasEksLabel, "target selector should not contain live-only label eks.amazonaws.com/component")
+	})
+	t.Run("will return nil for target when target is nil (prune)", func(t *testing.T) {
+		f := setup(t, []v1alpha1.ResourceIgnoreDifferences{})
+		// Simulate a resource that exists live but has no target (will be pruned)
+		f.comparisonResult.reconciliationResult.Target = []*unstructured.Unstructured{nil}
+
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		require.NoError(t, err)
+		require.Len(t, targets, 1)
+		assert.Nil(t, targets[0])
+	})
+	t.Run("will preserve both resources in mixed list with partial ignore match", func(t *testing.T) {
+		// A Deployment with matching ignore rules should get normalized,
+		// while a PDB without matching rules should pass through unchanged.
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:                 "apps",
+				Kind:                  "Deployment",
+				ManagedFieldsManagers: []string{"janitor"},
+			},
+		}
+		dc, err := diff.NewDiffConfigBuilder().
+			WithDiffSettings(ignores, nil, true, normalizers.IgnoreNormalizerOpts{}).
+			WithNoCache().
+			Build()
+		require.NoError(t, err)
+		liveDeployment := test.YamlToUnstructured(testdata.LiveDeploymentYaml)
+		targetDeployment := test.YamlToUnstructured(testdata.TargetDeploymentYaml)
+		livePDB := test.YamlToUnstructured(testdata.LivePDBYaml)
+		targetPDB := test.YamlToUnstructured(testdata.TargetPDBYaml)
+
+		cr := &comparisonResult{
+			reconciliationResult: sync.ReconciliationResult{
+				Live:   []*unstructured.Unstructured{liveDeployment, livePDB},
+				Target: []*unstructured.Unstructured{targetDeployment, targetPDB},
+			},
+			diffConfig: dc,
+		}
+
+		targets, err := normalizeTargetResources(cr)
+
+		require.NoError(t, err)
+		require.Len(t, targets, 2)
+
+		// Deployment (idx 0): has matching ignore rule, gets normalized
+		iksmVersion := targets[0].GetAnnotations()["iksm-version"]
+		assert.Equal(t, "2.0", iksmVersion)
+
+		// PDB (idx 1): no matching ignore rule, selector must be preserved
+		selector, ok, err := unstructured.NestedStringMap(targets[1].Object, "spec", "selector", "matchLabels")
+		require.NoError(t, err)
+		require.True(t, ok)
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/instance"])
+		_, hasEksLabel := selector["eks.amazonaws.com/component"]
+		assert.False(t, hasEksLabel, "PDB selector should not be corrupted by unrelated ignore rules")
+	})
+	t.Run("known limitation: PDB selector corrupted when ignore rules match the PDB", func(t *testing.T) {
+		t.Skip("known limitation: merge patch overwrites patchStrategy:replace fields even when they are not in the ignore rule")
+		// If the PDB has matching ignore rules (e.g. on annotations), the
+		// merge patch runs and drags in spec.selector as collateral damage
+		// due to patchStrategy:"replace". This documents the problem for a
+		// follow-up fix.
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:        "policy",
+				Kind:         "PodDisruptionBudget",
+				JSONPointers: []string{"/metadata/annotations"},
+			},
+		}
+		f := setup(t, ignores)
+		livePDB := test.YamlToUnstructured(testdata.LivePDBYaml)
+		targetPDB := test.YamlToUnstructured(testdata.TargetPDBYaml)
+		f.comparisonResult.reconciliationResult.Live = []*unstructured.Unstructured{livePDB}
+		f.comparisonResult.reconciliationResult.Target = []*unstructured.Unstructured{targetPDB}
+
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		require.NoError(t, err)
+		require.Len(t, targets, 1)
+		selector, ok, err := unstructured.NestedStringMap(targets[0].Object, "spec", "selector", "matchLabels")
+		require.NoError(t, err)
+		require.True(t, ok)
+		// Ideally the selector should be preserved, but the merge patch
+		// overwrites it because patchStrategy:"replace" forces inclusion.
+		assert.Equal(t, "coredns", selector["app.kubernetes.io/instance"])
+	})
+}
+
+func TestNormalizeTargetResourcesCRD(t *testing.T) {
+	// CRDs are not in the k8s scheme, so normalizeTargetResources falls back
+	// to JSON merge patch which replaces arrays wholesale. Without matching
+	// ignore rules the merge patch must be skipped entirely.
+	setup := func(t *testing.T, ignores []v1alpha1.ResourceIgnoreDifferences) *comparisonResult {
+		t.Helper()
+		dc, err := diff.NewDiffConfigBuilder().
+			WithDiffSettings(ignores, nil, true, normalizers.IgnoreNormalizerOpts{}).
+			WithNoCache().
+			Build()
+		require.NoError(t, err)
+		live := test.YamlToUnstructured(testdata.LiveHTTPProxy)
+		target := test.YamlToUnstructured(testdata.TargetHTTPProxy)
+		return &comparisonResult{
+			reconciliationResult: sync.ReconciliationResult{
+				Live:   []*unstructured.Unstructured{live},
+				Target: []*unstructured.Unstructured{target},
+			},
+			diffConfig: dc,
+		}
+	}
+	t.Run("will not corrupt CRD arrays when no ignore rules match", func(t *testing.T) {
+		cr := setup(t, []v1alpha1.ResourceIgnoreDifferences{})
+
+		targets, err := normalizeTargetResources(cr)
+
+		require.NoError(t, err)
+		require.Len(t, targets, 1)
+		// Target HTTPProxy has 2 descriptors; live has 1.
+		// Without fix the JSON merge patch would replace the array with live's single entry.
+		descriptors, ok, err := unstructured.NestedSlice(targets[0].Object, "spec", "routes")
+		require.NoError(t, err)
+		require.True(t, ok)
+		route := descriptors[0].(map[string]any)
+		global := route["rateLimitPolicy"].(map[string]any)["global"].(map[string]any)
+		descs := global["descriptors"].([]any)
+		assert.Len(t, descs, 2, "target should retain both descriptors, not be overwritten with live's single entry")
+	})
+}
+
 func TestNormalizeTargetResourcesWithList(t *testing.T) {
 	type fixture struct {
 		comparisonResult *comparisonResult

controller/testdata/data.go

@@ -32,4 +32,10 @@ var (
 
 	//go:embed additional-image-replicas-deployment.yaml
 	AdditionalImageReplicaDeploymentYaml string
+
+	//go:embed live-pdb.yaml
+	LivePDBYaml string
+
+	//go:embed target-pdb.yaml
+	TargetPDBYaml string
 )

controller/testdata/live-pdb.yaml

@@ -0,0 +1,15 @@
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: coredns
+  namespace: kube-system
+  labels:
+    eks.amazonaws.com/component: coredns
+  annotations:
+    argocd.argoproj.io/tracking-id: "coredns-wrapper:policy/PodDisruptionBudget:kube-system/coredns"
+spec:
+  maxUnavailable: 1
+  selector:
+    matchLabels:
+      eks.amazonaws.com/component: coredns
+      k8s-app: kube-dns

controller/testdata/target-pdb.yaml

@@ -0,0 +1,18 @@
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: coredns
+  namespace: kube-system
+  labels:
+    app.kubernetes.io/instance: coredns
+    app.kubernetes.io/managed-by: Helm
+    app.kubernetes.io/name: coredns
+  annotations:
+    argocd.argoproj.io/tracking-id: "coredns-wrapper:policy/PodDisruptionBudget:kube-system/coredns"
+spec:
+  maxUnavailable: 1
+  selector:
+    matchLabels:
+      app.kubernetes.io/instance: coredns
+      app.kubernetes.io/name: coredns
+      k8s-app: kube-dns