fix: fix calculating patch for respect ignore diff feature

controller/sync.go

@@ -2,7 +2,6 @@ package controller
 
 import (
 	"context"
-	"encoding/json"
 	goerrors "errors"
 	"fmt"
 	"os"
@@ -11,6 +10,7 @@ import (
 	"time"
 
 	cdcommon "github.com/argoproj/argo-cd/v2/common"
+	"k8s.io/apimachinery/pkg/util/strategicpatch"
 
 	"github.com/argoproj/gitops-engine/pkg/sync"
 	"github.com/argoproj/gitops-engine/pkg/sync/common"
@@ -21,6 +21,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/managedfields"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/kubectl/pkg/util/openapi"
 
 	"github.com/argoproj/argo-cd/v2/controller/metrics"
@@ -405,11 +406,10 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	}
 }
 
-// normalizeTargetResources will apply the diff normalization in all live and target resources.
-// Then it calculates the merge patch between the normalized live and the current live resources.
-// Finally it applies the merge patch in the normalized target resources. This is done to ensure
-// that target resources have the same ignored diff fields values from live ones to avoid them to
-// be applied in the cluster. Returns the list of normalized target resources.
+// normalizeTargetResources modifies target resources to ensure ignored fields are not touched during synchronization:
+//   - applies normalization to the target resources based on the live resources
+//   - copies ignored fields from the matching live resources: apply normalizer to the live resource,
+//     calculates the patch performed by normalizer and applies the patch to the target resource
 func normalizeTargetResources(cr *comparisonResult) ([]*unstructured.Unstructured, error) {
 	// normalize live and target resources
 	normalized, err := diff.Normalize(cr.reconciliationResult.Live, cr.reconciliationResult.Target, cr.diffConfig)
@@ -428,94 +428,35 @@ func normalizeTargetResources(cr *comparisonResult) ([]*unstructured.Unstructure
 			patchedTargets = append(patchedTargets, originalTarget)
 			continue
 		}
-		// calculate targetPatch between normalized and target resource
-		targetPatch, err := getMergePatch(normalizedTarget, originalTarget)
-		if err != nil {
-			return nil, err
-		}
 
-		// check if there is a patch to apply. An empty patch is identified by a '{}' string.
-		if len(targetPatch) > 2 {
-			livePatch, err := getMergePatch(normalized.Lives[idx], live)
-			if err != nil {
-				return nil, err
-			}
-			// generate a minimal patch that uses the fields from targetPatch (template)
-			// with livePatch values
-			patch, err := compilePatch(targetPatch, livePatch)
+		var lookupPatchMeta *strategicpatch.PatchMetaFromStruct
+		versionedObject, err := scheme.Scheme.New(normalizedTarget.GroupVersionKind())
+		if err == nil {
+			meta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject)
 			if err != nil {
 				return nil, err
 			}
-			normalizedTarget, err = applyMergePatch(normalizedTarget, patch)
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			// if there is no patch just use the original target
-			normalizedTarget = originalTarget
+			lookupPatchMeta = &meta
 		}
-		patchedTargets = append(patchedTargets, normalizedTarget)
-	}
-	return patchedTargets, nil
-}
 
-// compilePatch will generate a patch using the fields from templatePatch with
-// the values from valuePatch.
-func compilePatch(templatePatch, valuePatch []byte) ([]byte, error) {
-	templateMap := make(map[string]interface{})
-	err := json.Unmarshal(templatePatch, &templateMap)
-	if err != nil {
-		return nil, err
-	}
-	valueMap := make(map[string]interface{})
-	err = json.Unmarshal(valuePatch, &valueMap)
-	if err != nil {
-		return nil, err
-	}
-	resultMap := intersectMap(templateMap, valueMap)
-	return json.Marshal(resultMap)
-}
+		livePatch, err := getMergePatch(normalized.Lives[idx], live, lookupPatchMeta)
+		if err != nil {
+			return nil, err
+		}
 
-// intersectMap will return map with the fields intersection from the 2 provided
-// maps populated with the valueMap values.
-func intersectMap(templateMap, valueMap map[string]interface{}) map[string]interface{} {
-	result := make(map[string]interface{})
-	for k, v := range templateMap {
-		if innerTMap, ok := v.(map[string]interface{}); ok {
-			if innerVMap, ok := valueMap[k].(map[string]interface{}); ok {
-				result[k] = intersectMap(innerTMap, innerVMap)
-			}
-		} else if innerTSlice, ok := v.([]interface{}); ok {
-			if innerVSlice, ok := valueMap[k].([]interface{}); ok {
-				items := []interface{}{}
-				for idx, innerTSliceValue := range innerTSlice {
-					if idx < len(innerVSlice) {
-						if tSliceValueMap, ok := innerTSliceValue.(map[string]interface{}); ok {
-							if vSliceValueMap, ok := innerVSlice[idx].(map[string]interface{}); ok {
-								item := intersectMap(tSliceValueMap, vSliceValueMap)
-								items = append(items, item)
-							}
-						} else {
-							items = append(items, innerVSlice[idx])
-						}
-					}
-				}
-				if len(items) > 0 {
-					result[k] = items
-				}
-			}
-		} else {
-			if _, ok := valueMap[k]; ok {
-				result[k] = valueMap[k]
-			}
+		normalizedTarget, err = applyMergePatch(normalizedTarget, livePatch, versionedObject)
+		if err != nil {
+			return nil, err
 		}
+
+		patchedTargets = append(patchedTargets, normalizedTarget)
 	}
-	return result
+	return patchedTargets, nil
 }
 
 // getMergePatch calculates and returns the patch between the original and the
 // modified unstructures.
-func getMergePatch(original, modified *unstructured.Unstructured) ([]byte, error) {
+func getMergePatch(original, modified *unstructured.Unstructured, lookupPatchMeta *strategicpatch.PatchMetaFromStruct) ([]byte, error) {
 	originalJSON, err := original.MarshalJSON()
 	if err != nil {
 		return nil, err
@@ -524,20 +465,30 @@ func getMergePatch(original, modified *unstructured.Unstructured) ([]byte, error
 	if err != nil {
 		return nil, err
 	}
+	if lookupPatchMeta != nil {
+		return strategicpatch.CreateThreeWayMergePatch(modifiedJSON, modifiedJSON, originalJSON, lookupPatchMeta, true)
+	}
+
 	return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)
 }
 
 // applyMergePatch will apply the given patch in the obj and return the patched
 // unstructure.
-func applyMergePatch(obj *unstructured.Unstructured, patch []byte) (*unstructured.Unstructured, error) {
+func applyMergePatch(obj *unstructured.Unstructured, patch []byte, versionedObject interface{}) (*unstructured.Unstructured, error) {
 	originalJSON, err := obj.MarshalJSON()
 	if err != nil {
 		return nil, err
 	}
-	patchedJSON, err := jsonpatch.MergePatch(originalJSON, patch)
+	var patchedJSON []byte
+	if versionedObject == nil {
+		patchedJSON, err = jsonpatch.MergePatch(originalJSON, patch)
+	} else {
+		patchedJSON, err = strategicpatch.StrategicMergePatch(originalJSON, patch, versionedObject)
+	}
 	if err != nil {
 		return nil, err
 	}
+
 	patchedObj := &unstructured.Unstructured{}
 	_, _, err = unstructured.UnstructuredJSONScheme.Decode(patchedJSON, nil, patchedObj)
 	if err != nil {

controller/sync_test.go

@@ -455,3 +455,207 @@ func TestNormalizeTargetResources(t *testing.T) {
 		assert.Equal(t, 2, len(containers))
 	})
 }
+
+func TestNormalizeTargetResourcesWithList(t *testing.T) {
+	type fixture struct {
+		comparisonResult *comparisonResult
+	}
+	setupHttpProxy := func(t *testing.T, ignores []v1alpha1.ResourceIgnoreDifferences) *fixture {
+		t.Helper()
+		dc, err := diff.NewDiffConfigBuilder().
+			WithDiffSettings(ignores, nil, true).
+			WithNoCache().
+			Build()
+		require.NoError(t, err)
+		live := test.YamlToUnstructured(testdata.LiveHTTPProxy)
+		target := test.YamlToUnstructured(testdata.TargetHTTPProxy)
+		return &fixture{
+			&comparisonResult{
+				reconciliationResult: sync.ReconciliationResult{
+					Live:   []*unstructured.Unstructured{live},
+					Target: []*unstructured.Unstructured{target},
+				},
+				diffConfig: dc,
+			},
+		}
+	}
+
+	t.Run("will properly ignore nested fields within arrays", func(t *testing.T) {
+		// given
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:             "projectcontour.io",
+				Kind:              "HTTPProxy",
+				JQPathExpressions: []string{".spec.routes[]"},
+				//JSONPointers: []string{"/spec/routes"},
+			},
+		}
+		f := setupHttpProxy(t, ignores)
+		target := test.YamlToUnstructured(testdata.TargetHTTPProxy)
+		f.comparisonResult.reconciliationResult.Target = []*unstructured.Unstructured{target}
+
+		// when
+		patchedTargets, err := normalizeTargetResources(f.comparisonResult)
+
+		// then
+		require.NoError(t, err)
+		require.Equal(t, 1, len(f.comparisonResult.reconciliationResult.Live))
+		require.Equal(t, 1, len(f.comparisonResult.reconciliationResult.Target))
+		require.Equal(t, 1, len(patchedTargets))
+
+		// live should have 1 entry
+		require.Equal(t, 1, len(dig[[]any](f.comparisonResult.reconciliationResult.Live[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors"})))
+		// assert some arbitrary field to show `entries[0]` is not an empty object
+		require.Equal(t, "sample-header", dig[string](f.comparisonResult.reconciliationResult.Live[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors", 0, "entries", 0, "requestHeader", "headerName"}))
+
+		// target has 2 entries
+		require.Equal(t, 2, len(dig[[]any](f.comparisonResult.reconciliationResult.Target[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors", 0, "entries"})))
+		// assert some arbitrary field to show `entries[0]` is not an empty object
+		require.Equal(t, "sample-header", dig[string](f.comparisonResult.reconciliationResult.Target[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors", 0, "entries", 0, "requestHeaderValueMatch", "headers", 0, "name"}))
+
+		// It should be *1* entries in the array
+		require.Equal(t, 1, len(dig[[]any](patchedTargets[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors"})))
+		// and it should NOT equal an empty object
+		require.Len(t, dig[any](patchedTargets[0].Object, []interface{}{"spec", "routes", 0, "rateLimitPolicy", "global", "descriptors", 0, "entries", 0}), 1)
+
+	})
+	t.Run("will correctly set array entries if new entries have been added", func(t *testing.T) {
+		// given
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:             "apps",
+				Kind:              "Deployment",
+				JQPathExpressions: []string{".spec.template.spec.containers[].env[] | select(.name == \"SOME_ENV_VAR\")"},
+			},
+		}
+		f := setupHttpProxy(t, ignores)
+		live := test.YamlToUnstructured(testdata.LiveDeploymentEnvVarsYaml)
+		target := test.YamlToUnstructured(testdata.TargetDeploymentEnvVarsYaml)
+		f.comparisonResult.reconciliationResult.Live = []*unstructured.Unstructured{live}
+		f.comparisonResult.reconciliationResult.Target = []*unstructured.Unstructured{target}
+
+		// when
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		// then
+		require.NoError(t, err)
+		require.Equal(t, 1, len(targets))
+		containers, ok, err := unstructured.NestedSlice(targets[0].Object, "spec", "template", "spec", "containers")
+		require.NoError(t, err)
+		require.True(t, ok)
+		assert.Equal(t, 1, len(containers))
+
+		ports := containers[0].(map[string]interface{})["ports"].([]interface{})
+		assert.Equal(t, 1, len(ports))
+
+		env := containers[0].(map[string]interface{})["env"].([]interface{})
+		assert.Equal(t, 3, len(env))
+
+		first := env[0]
+		second := env[1]
+		third := env[2]
+
+		// Currently the defined order at this time is the insertion order of the target manifest.
+		assert.Equal(t, "SOME_ENV_VAR", first.(map[string]interface{})["name"])
+		assert.Equal(t, "some_value", first.(map[string]interface{})["value"])
+
+		assert.Equal(t, "SOME_OTHER_ENV_VAR", second.(map[string]interface{})["name"])
+		assert.Equal(t, "some_other_value", second.(map[string]interface{})["value"])
+
+		assert.Equal(t, "YET_ANOTHER_ENV_VAR", third.(map[string]interface{})["name"])
+		assert.Equal(t, "yet_another_value", third.(map[string]interface{})["value"])
+	})
+
+	t.Run("ignore-deployment-image-replicas-changes-additive", func(t *testing.T) {
+		// given
+
+		ignores := []v1alpha1.ResourceIgnoreDifferences{
+			{
+				Group:        "apps",
+				Kind:         "Deployment",
+				JSONPointers: []string{"/spec/replicas"},
+			}, {
+				Group:             "apps",
+				Kind:              "Deployment",
+				JQPathExpressions: []string{".spec.template.spec.containers[].image"},
+			},
+		}
+		f := setupHttpProxy(t, ignores)
+		live := test.YamlToUnstructured(testdata.MinimalImageReplicaDeploymentYaml)
+		target := test.YamlToUnstructured(testdata.AdditionalImageReplicaDeploymentYaml)
+		f.comparisonResult.reconciliationResult.Live = []*unstructured.Unstructured{live}
+		f.comparisonResult.reconciliationResult.Target = []*unstructured.Unstructured{target}
+
+		// when
+		targets, err := normalizeTargetResources(f.comparisonResult)
+
+		// then
+		require.NoError(t, err)
+		require.Equal(t, 1, len(targets))
+		metadata, ok, err := unstructured.NestedMap(targets[0].Object, "metadata")
+		require.NoError(t, err)
+		require.True(t, ok)
+		labels, ok := metadata["labels"].(map[string]interface{})
+		require.True(t, ok)
+		assert.Equal(t, 2, len(labels))
+		assert.Equal(t, "web", labels["appProcess"])
+
+		spec, ok, err := unstructured.NestedMap(targets[0].Object, "spec")
+		require.NoError(t, err)
+		require.True(t, ok)
+
+		assert.Equal(t, int64(1), spec["replicas"])
+
+		template, ok := spec["template"].(map[string]interface{})
+		require.True(t, ok)
+
+		tMetadata, ok := template["metadata"].(map[string]interface{})
+		require.True(t, ok)
+		tLabels, ok := tMetadata["labels"].(map[string]interface{})
+		require.True(t, ok)
+		assert.Equal(t, 2, len(tLabels))
+		assert.Equal(t, "web", tLabels["appProcess"])
+
+		tSpec, ok := template["spec"].(map[string]interface{})
+		require.True(t, ok)
+		containers, ok, err := unstructured.NestedSlice(tSpec, "containers")
+		require.NoError(t, err)
+		require.True(t, ok)
+		assert.Equal(t, 1, len(containers))
+
+		first := containers[0].(map[string]interface{})
+		assert.Equal(t, "alpine:3", first["image"])
+
+		resources, ok := first["resources"].(map[string]interface{})
+		require.True(t, ok)
+		requests, ok := resources["requests"].(map[string]interface{})
+		require.True(t, ok)
+		assert.Equal(t, "400m", requests["cpu"])
+
+		env, ok, err := unstructured.NestedSlice(first, "env")
+		require.NoError(t, err)
+		require.True(t, ok)
+		assert.Equal(t, 1, len(env))
+
+		env0 := env[0].(map[string]interface{})
+		assert.Equal(t, "EV", env0["name"])
+		assert.Equal(t, "here", env0["value"])
+	})
+}
+
+func dig[T any](obj interface{}, path []interface{}) T {
+	i := obj
+
+	for _, segment := range path {
+		switch segment.(type) {
+		case int:
+			i = i.([]interface{})[segment.(int)]
+		case string:
+			i = i.(map[string]interface{})[segment.(string)]
+		default:
+			panic("invalid path for object")
+		}
+	}
+
+	return i.(T)
+}