fix: Server-side diff shows incorrect diffs for list related changes (#688)

* fix: Server-side diff shows incorrect diffs for list related changes

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

* Update docs for removeWebHookMutation

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

* Update docs

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

* Update docs

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

---------

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

Author: pjiang-dev

pkg/diff/diff.go

@@ -193,13 +193,13 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D
 	return buildDiffResult(predictedLiveBytes, liveBytes), nil
 }
 
-// removeWebhookMutation will compare the predictedLive with live to identify
-// changes done by mutation webhooks. Webhook mutations are identified by finding
-// changes in predictedLive fields not associated with any manager in the
-// managedFields. All fields under this condition will be reverted with their state
-// from live. If the given predictedLive does not have the managedFields, an error
-// will be returned.
-func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, _ string) (*unstructured.Unstructured, error) {
+// removeWebhookMutation will compare the predictedLive with live to identify changes done by mutation webhooks.
+// Webhook mutations are removed from predictedLive by removing all fields which are not managed by the given 'manager'.
+// At this step, we will only have the fields that are managed by the given 'manager'.
+// It is then merged with the live state and re-assigned to predictedLive. This means that any
+// fields not managed by the specified manager will be reverted with their state from live, including any webhook mutations.
+// If the given predictedLive does not have the managedFields, an error will be returned.
+func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, manager string) (*unstructured.Unstructured, error) {
 	plManagedFields := predictedLive.GetManagedFields()
 	if len(plManagedFields) == 0 {
 		return nil, fmt.Errorf("predictedLive for resource %s/%s must have the managedFields", predictedLive.GetKind(), predictedLive.GetName())
@@ -220,57 +220,42 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
 		return nil, fmt.Errorf("error converting live state from unstructured to %s: %w", gvk, err)
 	}
 
-	// Compare the predicted live with the live resource
-	comparison, err := typedLive.Compare(typedPredictedLive)
-	if err != nil {
-		return nil, fmt.Errorf("error comparing predicted resource to live resource: %w", err)
-	}
+	// Initialize an empty fieldpath.Set to aggregate managed fields for the specified manager
+	managerFieldsSet := &fieldpath.Set{}
 
-	// Loop over all existing managers in predicted live resource to identify
-	// fields mutated (in predicted live) not owned by any manager.
+	// Iterate over all ManagedFields entries in predictedLive
 	for _, mfEntry := range plManagedFields {
-		mfs := &fieldpath.Set{}
-		err := mfs.FromJSON(bytes.NewReader(mfEntry.FieldsV1.Raw))
+		managedFieldsSet := &fieldpath.Set{}
+		err := managedFieldsSet.FromJSON(bytes.NewReader(mfEntry.FieldsV1.Raw))
 		if err != nil {
 			return nil, fmt.Errorf("error building managedFields set: %w", err)
 		}
-		if comparison.Added != nil && !comparison.Added.Empty() {
-			// exclude the added fields owned by this manager from the comparison
-			comparison.Added = comparison.Added.Difference(mfs)
-		}
-		if comparison.Modified != nil && !comparison.Modified.Empty() {
-			// exclude the modified fields owned by this manager from the comparison
-			comparison.Modified = comparison.Modified.Difference(mfs)
-		}
-		if comparison.Removed != nil && !comparison.Removed.Empty() {
-			// exclude the removed fields owned by this manager from the comparison
-			comparison.Removed = comparison.Removed.Difference(mfs)
+		if mfEntry.Manager == manager {
+			// Union the fields with the aggregated set
+			managerFieldsSet = managerFieldsSet.Union(managedFieldsSet)
 		}
 	}
-	// At this point, comparison holds all mutations that aren't owned by any
-	// of the existing managers.
 
-	if comparison.Added != nil && !comparison.Added.Empty() {
-		// remove added fields that aren't owned by any manager
-		typedPredictedLive = typedPredictedLive.RemoveItems(comparison.Added)
+	if managerFieldsSet.Empty() {
+		return nil, fmt.Errorf("no managed fields found for manager: %s", manager)
 	}
 
-	if comparison.Modified != nil && !comparison.Modified.Empty() {
-		liveModValues := typedLive.ExtractItems(comparison.Modified, typed.WithAppendKeyFields())
-		// revert modified fields not owned by any manager
-		typedPredictedLive, err = typedPredictedLive.Merge(liveModValues)
-		if err != nil {
-			return nil, fmt.Errorf("error reverting webhook modified fields in predicted live resource: %w", err)
-		}
+	predictedLiveFieldSet, err := typedPredictedLive.ToFieldSet()
+	if err != nil {
+		return nil, fmt.Errorf("error converting predicted live state to FieldSet: %w", err)
 	}
 
-	if comparison.Removed != nil && !comparison.Removed.Empty() {
-		liveRmValues := typedLive.ExtractItems(comparison.Removed, typed.WithAppendKeyFields())
-		// revert removed fields not owned by any manager
-		typedPredictedLive, err = typedPredictedLive.Merge(liveRmValues)
-		if err != nil {
-			return nil, fmt.Errorf("error reverting webhook removed fields in predicted live resource: %w", err)
-		}
+	// Remove fields from predicted live that are not managed by the provided manager
+	nonArgoFieldsSet := predictedLiveFieldSet.Difference(managerFieldsSet)
+
+	// In case any of the removed fields cause schema violations, we will keep those fields
+	nonArgoFieldsSet = safelyRemoveFieldsSet(typedPredictedLive, nonArgoFieldsSet)
+	typedPredictedLive = typedPredictedLive.RemoveItems(nonArgoFieldsSet)
+
+	// Apply the predicted live state to the live state to get a diff without mutation webhook fields
+	typedPredictedLive, err = typedLive.Merge(typedPredictedLive)
+	if err != nil {
+		return nil, fmt.Errorf("error applying predicted live to live state: %w", err)
 	}
 
 	plu := typedPredictedLive.AsValue().Unstructured()
@@ -281,6 +266,31 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
 	return &unstructured.Unstructured{Object: pl}, nil
 }
 
+// safelyRemoveFieldSet will validate if removing the fieldsToRemove set from predictedLive maintains
+// a valid schema. If removing a field in fieldsToRemove is invalid and breaks the schema, it is not safe
+// to remove and will be skipped from removal from predictedLive.
+func safelyRemoveFieldsSet(predictedLive *typed.TypedValue, fieldsToRemove *fieldpath.Set) *fieldpath.Set {
+	// In some cases, we cannot remove fields due to violation of the predicted live schema. In such cases we validate the removal
+	// of each field and only include it if the removal is valid.
+	testPredictedLive := predictedLive.RemoveItems(fieldsToRemove)
+	err := testPredictedLive.Validate()
+	if err != nil {
+		adjustedFieldsToRemove := fieldpath.NewSet()
+		fieldsToRemove.Iterate(func(p fieldpath.Path) {
+			singleFieldSet := fieldpath.NewSet(p)
+			testSingleRemoval := predictedLive.RemoveItems(singleFieldSet)
+			// Check if removing this single field maintains a valid schema
+			if testSingleRemoval.Validate() == nil {
+				// If valid, add this field to the adjusted set to remove
+				adjustedFieldsToRemove.Insert(p)
+			}
+		})
+		return adjustedFieldsToRemove
+	}
+	// If no violations, return the original set to remove
+	return fieldsToRemove
+}
+
 func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error) {
 	res := make(map[string]any)
 	err := json.Unmarshal([]byte(jsonString), &res)

pkg/diff/diff_test.go

@@ -972,6 +972,64 @@ func TestServerSideDiff(t *testing.T) {
 		assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig])
 		assert.NotEmpty(t, predictedSVC.Labels["event"])
 	})
+
+	t.Run("will include nested fields like ports and env", func(t *testing.T) {
+		// given
+		t.Parallel()
+		liveState := StrToUnstructured(testdata.DeploymentNestedLiveYAMLSSD)
+		desiredState := StrToUnstructured(testdata.DeploymentNestedConfigYAMLSSD)
+		opts := buildOpts(testdata.DeploymentNestedPredictedLiveJSONSSD)
+
+		// when
+		result, err := serverSideDiff(desiredState, liveState, opts...)
+
+		// then
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+		assert.True(t, result.Modified)
+
+		predictedDeploy := YamlToDeploy(t, result.PredictedLive)
+		liveDeploy := YamlToDeploy(t, result.NormalizedLive)
+
+		// Check ports
+		assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers[0].Ports, 2)
+		assert.Len(t, liveDeploy.Spec.Template.Spec.Containers[0].Ports, 1)
+		assert.Equal(t, int32(80), predictedDeploy.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort)
+		assert.Equal(t, int32(443), predictedDeploy.Spec.Template.Spec.Containers[0].Ports[1].ContainerPort)
+
+		// Check env
+		assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers[0].Env, 2)
+		assert.Len(t, liveDeploy.Spec.Template.Spec.Containers[0].Env, 1)
+		assert.Equal(t, "ENV_VAR1", predictedDeploy.Spec.Template.Spec.Containers[0].Env[0].Name)
+		assert.Equal(t, "ENV_VAR2", predictedDeploy.Spec.Template.Spec.Containers[0].Env[1].Name)
+	})
+
+	t.Run("will add an extra container using kubectl apply and include mutation webhook", func(t *testing.T) {
+		// given
+		t.Parallel()
+		liveState := StrToUnstructured(testdata.DeploymentApplyLiveYAMLSSD)
+		desiredState := StrToUnstructured(testdata.DeploymentApplyConfigYAMLSSD)
+		opts := buildOpts(testdata.DeploymentApplyPredictedLiveJSONSSD)
+
+		// when
+		result, err := serverSideDiff(desiredState, liveState, opts...)
+
+		// then
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+		assert.True(t, result.Modified)
+
+		predictedDeploy := YamlToDeploy(t, result.PredictedLive)
+		liveDeploy := YamlToDeploy(t, result.NormalizedLive)
+
+		// Check ports are shown in diff and ensure mutation webhook is not shown
+		assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers[0].Ports, 2)
+		assert.Len(t, liveDeploy.Spec.Template.Spec.Containers[0].Ports, 1)
+		assert.Equal(t, int32(80), predictedDeploy.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort)
+		assert.Equal(t, int32(40), predictedDeploy.Spec.Template.Spec.Containers[0].Ports[1].ContainerPort)
+		assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig])
+		assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig])
+	})
 }
 
 func createSecret(data map[string]string) *unstructured.Unstructured {

pkg/diff/testdata/data.go

@@ -50,4 +50,22 @@ var (
 
 	//go:embed ssd-service-predicted-live.json
 	ServicePredictedLiveJSONSSD string
+
+	//go:embed ssd-deploy-nested-config.yaml
+	DeploymentNestedConfigYAMLSSD string
+
+	//go:embed ssd-deploy-nested-live.yaml
+	DeploymentNestedLiveYAMLSSD string
+
+	//go:embed ssd-deploy-nested-predicted-live.json
+	DeploymentNestedPredictedLiveJSONSSD string
+
+	//go:embed ssd-deploy-with-manual-apply-config.yaml
+	DeploymentApplyConfigYAMLSSD string
+
+	//go:embed ssd-deploy-with-manual-apply-live.yaml
+	DeploymentApplyLiveYAMLSSD string
+
+	//go:embed ssd-deploy-with-manual-apply-predicted-live.json
+	DeploymentApplyPredictedLiveJSONSSD string
 )

pkg/diff/testdata/ssd-deploy-nested-config.yaml

@@ -0,0 +1,36 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nested-test-deployment
+  namespace: default
+  labels:
+    app: nested-test
+    applications.argoproj.io/app-name: nested-app
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: nested-test
+  template:
+    metadata:
+      labels:
+        app: nested-test
+    spec:
+      automountServiceAccountToken: false
+      containers:
+        - name: main-container
+          image: 'nginx:latest'
+          ports:
+            - containerPort: 80
+              name: http
+              protocol: TCP
+            - containerPort: 443
+              name: https
+          env:
+            - name: ENV_VAR1
+              value: "value1"
+            - name: ENV_VAR2
+              value: "value2"
+          resources:
+            limits:
+              memory: 100Mi

pkg/diff/testdata/ssd-deploy-nested-live.yaml

@@ -0,0 +1,70 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nested-test-deployment
+  namespace: default
+  labels:
+    app: nested-test
+    applications.argoproj.io/app-name: nested-app
+  annotations:
+    deployment.kubernetes.io/revision: '1'
+  managedFields:
+    - apiVersion: apps/v1
+      fieldsType: FieldsV1
+      fieldsV1:
+        f:metadata:
+          f:labels:
+            f:app: {}
+            f:applications.argoproj.io/app-name: {}
+        f:spec:
+          f:replicas: {}
+          f:selector: {}
+          f:template:
+            f:metadata:
+              f:labels:
+                f:app: {}
+            f:spec:
+              f:containers:
+                k:{"name":"main-container"}:
+                  .: {}
+                  f:image: {}
+                  f:name: {}
+                  f:ports:
+                    .: {}
+                    k:{"containerPort":80,"protocol":"TCP"}:
+                      .: {}
+                      f:containerPort: {}
+                      f:name: {}
+                      f:protocol: {}
+                  f:env:
+                    .: {}
+                    k:{"name":"ENV_VAR1"}:
+                      .: {}
+                      f:name: {}
+                      f:value: {}
+      manager: argocd-controller
+      operation: Apply
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: nested-test
+  template:
+    metadata:
+      labels:
+        app: nested-test
+    spec:
+      automountServiceAccountToken: false
+      containers:
+        - name: main-container
+          image: 'nginx:latest'
+          ports:
+            - containerPort: 80
+              name: http
+              protocol: TCP
+          env:
+            - name: ENV_VAR1
+              value: "value1"
+          resources:
+            limits:
+              memory: "100Mi"