fix: Server-Side diff removed fields missing in diff (#722)

* fix: Server-Side diff removed fields missing in diff

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

* add unit test to cover deleted field

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

---------

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

pkg/diff/diff.go

@@ -248,12 +248,28 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
 	// Remove fields from predicted live that are not managed by the provided manager
 	nonArgoFieldsSet := predictedLiveFieldSet.Difference(managerFieldsSet)
 
+	// 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)
+	}
+
+	if comparison.Removed != nil && !comparison.Removed.Empty() {
+		// exclude the removed fields not owned by this manager from the comparison
+		comparison.Removed = comparison.Removed.Difference(nonArgoFieldsSet)
+	}
+
 	// 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)
+
+	// After applying the predicted live to live state, this would cause any removed fields to be restored.
+	// We need to re-remove these from predicted live.
+	typedPredictedLive = typedPredictedLive.RemoveItems(comparison.Removed)
+
 	if err != nil {
 		return nil, fmt.Errorf("error applying predicted live to live state: %w", err)
 	}

pkg/diff/diff_test.go

@@ -1030,6 +1030,31 @@ func TestServerSideDiff(t *testing.T) {
 		assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig])
 		assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig])
 	})
+
+	t.Run("will reflect deletion of labels in predicted live", func(t *testing.T) {
+		// given
+		t.Parallel()
+		liveState := StrToUnstructured(testdata.ServiceLiveLabelYAMLSSD)
+		desiredState := StrToUnstructured(testdata.ServiceConfigNoLabelYAMLSSD)
+		opts := buildOpts(testdata.ServicePredictedLiveNoLabelJSONSSD)
+
+		// when
+		result, err := serverSideDiff(desiredState, liveState, opts...)
+
+		// then
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+		assert.True(t, result.Modified)
+
+		predictedSvc := YamlToSvc(t, result.PredictedLive)
+		liveSvc := YamlToSvc(t, result.NormalizedLive)
+
+		// Ensure that the deleted label is not present in predicted and exists in live
+		_, predictedLabelExists := predictedSvc.Labels["delete-me"]
+		_, liveLabelExists := liveSvc.Labels["delete-me"]
+		assert.False(t, predictedLabelExists)
+		assert.True(t, liveLabelExists)
+	})
 }
 
 func createSecret(data map[string]string) *unstructured.Unstructured {

pkg/diff/testdata/data.go

@@ -68,4 +68,13 @@ var (
 
 	//go:embed ssd-deploy-with-manual-apply-predicted-live.json
 	DeploymentApplyPredictedLiveJSONSSD string
+
+	//go:embed ssd-svc-label-live.yaml
+	ServiceLiveLabelYAMLSSD string
+
+	//go:embed ssd-svc-no-label-config.yaml
+	ServiceConfigNoLabelYAMLSSD string
+
+	//go:embed ssd-svc-no-label-predicted-live.json
+	ServicePredictedLiveNoLabelJSONSSD string
 )

pkg/diff/testdata/ssd-svc-label-live.yaml

@@ -0,0 +1,50 @@
+apiVersion: v1
+kind: Service
+metadata:
+  creationTimestamp: "2025-05-16T19:01:22Z"
+  labels:
+    app.kubernetes.io/instance: httpbin
+    delete-me: delete-value
+  managedFields:
+    - apiVersion: v1
+      fieldsType: FieldsV1
+      fieldsV1:
+        f:metadata:
+          f:labels:
+            f:app.kubernetes.io/instance: {}
+            f:delete-me: {}
+        f:spec:
+          f:ports:
+            k:{"port":7777,"protocol":"TCP"}:
+              .: {}
+              f:name: {}
+              f:port: {}
+              f:protocol: {}
+              f:targetPort: {}
+          f:selector: {}
+      manager: argocd-controller
+      operation: Apply
+      time: "2025-05-16T19:01:22Z"
+  name: httpbin-svc
+  namespace: httpbin
+  resourceVersion: "159005"
+  uid: 61a7a0c2-d973-4333-bbd6-c06ba1c00190
+spec:
+  clusterIP: 10.96.59.144
+  clusterIPs:
+    - 10.96.59.144
+  internalTrafficPolicy: Cluster
+  ipFamilies:
+    - IPv4
+  ipFamilyPolicy: SingleStack
+  ports:
+    - name: http-port
+      port: 7777
+      protocol: TCP
+      targetPort: 80
+  selector:
+    app: httpbin
+  sessionAffinity: None
+  type: ClusterIP
+status:
+  loadBalancer: {}

pkg/diff/testdata/ssd-svc-no-label-config.yaml

@@ -0,0 +1,15 @@
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    app.kubernetes.io/instance: httpbin
+  name: httpbin-svc
+  namespace: httpbin
+spec:
+  ports:
+    - name: http-port
+      port: 7777
+      protocol: TCP
+      targetPort: 80
+  selector:
+    app: httpbin

pkg/diff/testdata/ssd-svc-no-label-predicted-live.json

@@ -0,0 +1,69 @@
+{
+  "apiVersion": "v1",
+  "kind": "Service",
+  "metadata": {
+    "creationTimestamp": "2025-05-16T19:01:22Z",
+    "labels": {
+      "app.kubernetes.io/instance": "httpbin"
+    },
+    "managedFields": [
+      {
+        "apiVersion": "v1",
+        "fieldsType": "FieldsV1",
+        "fieldsV1": {
+          "f:metadata": {
+            "f:labels": {
+              "f:app.kubernetes.io/instance": {}
+            }
+          },
+          "f:spec": {
+            "f:ports": {
+              "k:{\"port\":7777,\"protocol\":\"TCP\"}": {
+                ".": {},
+                "f:name": {},
+                "f:port": {},
+                "f:protocol": {},
+                "f:targetPort": {}
+              }
+            },
+            "f:selector": {}
+          }
+        },
+        "manager": "argocd-controller",
+        "operation": "Apply",
+        "time": "2025-05-16T19:02:57Z"
+      }
+    ],
+    "name": "httpbin-svc",
+    "namespace": "httpbin",
+    "resourceVersion": "159005",
+    "uid": "61a7a0c2-d973-4333-bbd6-c06ba1c00190"
+  },
+  "spec": {
+    "clusterIP": "10.96.59.144",
+    "clusterIPs": [
+      "10.96.59.144"
+    ],
+    "internalTrafficPolicy": "Cluster",
+    "ipFamilies": [
+      "IPv4"
+    ],
+    "ipFamilyPolicy": "SingleStack",
+    "ports": [
+      {
+        "name": "http-port",
+        "port": 7777,
+        "protocol": "TCP",
+        "targetPort": 80
+      }
+    ],
+    "selector": {
+      "app": "httpbin"
+    },
+    "sessionAffinity": "None",
+    "type": "ClusterIP"
+  },
+  "status": {
+    "loadBalancer": {}
+  }
+}