fix: remove last-applied-configuration before diff in ssa (#460)

* fix: remove last-apply-configurations before diff in ssa

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* fix: add tests to validate expected behaviour

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

Author: leoluz

pkg/diff/diff.go

@@ -29,7 +29,10 @@ import (
 	kubescheme "github.com/argoproj/gitops-engine/pkg/utils/kube/scheme"
 )
 
-const couldNotMarshalErrMsg = "Could not unmarshal to object of type %s: %v"
+const (
+	couldNotMarshalErrMsg       = "Could not unmarshal to object of type %s: %v"
+	AnnotationLastAppliedConfig = "kubectl.kubernetes.io/last-applied-configuration"
+)
 
 // Holds diffing result of two resources
 type DiffResult struct {
@@ -173,44 +176,49 @@ func structuredMergeDiff(config, live *unstructured.Unstructured, pt *typed.Pars
 	}
 
 	// 6) Apply default values in predicted live
-	predictedLive, err := applyDefaultValues(mergedCleanLive)
+	predictedLive, err := normalizeTypedValue(mergedCleanLive)
 	if err != nil {
 		return nil, fmt.Errorf("error applying default values in predicted live: %w", err)
 	}
 
 	// 7) Apply default values in live
-	taintedLive, err := applyDefaultValues(tvLive)
+	taintedLive, err := normalizeTypedValue(tvLive)
 	if err != nil {
 		return nil, fmt.Errorf("error applying default values in live: %w", err)
 	}
 
 	return buildDiffResult(predictedLive, taintedLive), nil
 }
 
-func applyDefaultValues(result *typed.TypedValue) ([]byte, error) {
-	ru := result.AsValue().Unstructured()
+// normalizeTypedValue will prepare the given tv so it can be used in diffs by:
+// - removing last-applied-configuration annotation
+// - applying default values
+func normalizeTypedValue(tv *typed.TypedValue) ([]byte, error) {
+	ru := tv.AsValue().Unstructured()
 	r, ok := ru.(map[string]interface{})
 	if !ok {
 		return nil, fmt.Errorf("error converting result typedValue: expected map got %T", ru)
 	}
-	mergedUnstructured := &unstructured.Unstructured{Object: r}
-	mergedBytes, err := json.Marshal(mergedUnstructured)
+	resultUn := &unstructured.Unstructured{Object: r}
+	unstructured.RemoveNestedField(resultUn.Object, "metadata", "annotations", AnnotationLastAppliedConfig)
+
+	resultBytes, err := json.Marshal(resultUn)
 	if err != nil {
 		return nil, fmt.Errorf("error while marshaling merged unstructured: %w", err)
 	}
 
-	obj, err := scheme.Scheme.New(mergedUnstructured.GroupVersionKind())
+	obj, err := scheme.Scheme.New(resultUn.GroupVersionKind())
 	if err == nil {
-		err := json.Unmarshal(mergedBytes, &obj)
+		err := json.Unmarshal(resultBytes, &obj)
 		if err != nil {
 			return nil, fmt.Errorf("error unmarshaling merged bytes into object: %w", err)
 		}
-		mergedBytes, err = patchDefaultValues(mergedBytes, obj)
+		resultBytes, err = patchDefaultValues(resultBytes, obj)
 		if err != nil {
 			return nil, fmt.Errorf("error applying defaults: %w", err)
 		}
 	}
-	return mergedBytes, nil
+	return resultBytes, nil
 }
 
 func buildDiffResult(predictedBytes []byte, liveBytes []byte) *DiffResult {

pkg/diff/diff_test.go

@@ -770,14 +770,13 @@ func TestStructuredMergeDiff(t *testing.T) {
 		assert.NotNil(t, liveSVC.Spec.InternalTrafficPolicy)
 		assert.Equal(t, "Cluster", string(*predictedSVC.Spec.InternalTrafficPolicy))
 		assert.Equal(t, "Cluster", string(*liveSVC.Spec.InternalTrafficPolicy))
+		assert.Empty(t, predictedSVC.Annotations[AnnotationLastAppliedConfig])
+		assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig])
 	})
 	t.Run("will remove entries in list", func(t *testing.T) {
 		// given
 		liveState := StrToUnstructured(testdata.ServiceLiveYAML)
 		desiredState := StrToUnstructured(testdata.ServiceConfigWith2Ports)
-		expectedLiveState := StrToUnstructured(testdata.ExpectedServiceLiveWith2PortsYAML)
-		expectedLiveBytes, err := json.Marshal(expectedLiveState)
-		require.NoError(t, err)
 
 		// when
 		result, err := structuredMergeDiff(desiredState, liveState, &svcParseType, manager)
@@ -786,15 +785,13 @@ func TestStructuredMergeDiff(t *testing.T) {
 		require.NoError(t, err)
 		assert.NotNil(t, result)
 		assert.True(t, result.Modified)
-		assert.Equal(t, string(expectedLiveBytes), string(result.PredictedLive))
+		svc := YamlToSvc(t, result.PredictedLive)
+		assert.Len(t, svc.Spec.Ports, 2)
 	})
 	t.Run("will remove previously added fields not present in desired state", func(t *testing.T) {
 		// given
 		liveState := StrToUnstructured(testdata.LiveServiceWithTypeYAML)
 		desiredState := StrToUnstructured(testdata.ServiceConfigYAML)
-		expectedLiveState := StrToUnstructured(testdata.ServiceLiveNoTypeYAML)
-		expectedLiveBytes, err := json.Marshal(expectedLiveState)
-		require.NoError(t, err)
 
 		// when
 		result, err := structuredMergeDiff(desiredState, liveState, &svcParseType, manager)
@@ -803,7 +800,8 @@ func TestStructuredMergeDiff(t *testing.T) {
 		require.NoError(t, err)
 		assert.NotNil(t, result)
 		assert.True(t, result.Modified)
-		assert.Equal(t, string(expectedLiveBytes), string(result.PredictedLive))
+		svc := YamlToSvc(t, result.PredictedLive)
+		assert.Equal(t, corev1.ServiceTypeClusterIP, svc.Spec.Type)
 	})
 	t.Run("will apply service with multiple ports", func(t *testing.T) {
 		// given
@@ -818,7 +816,7 @@ func TestStructuredMergeDiff(t *testing.T) {
 		assert.NotNil(t, result)
 		assert.True(t, result.Modified)
 		svc := YamlToSvc(t, result.PredictedLive)
-		assert.Equal(t, 5, len(svc.Spec.Ports))
+		assert.Len(t, svc.Spec.Ports, 5)
 	})
 }
 

pkg/diff/testdata/data.go

@@ -9,15 +9,9 @@ var (
 	//go:embed smd-service-live.yaml
 	ServiceLiveYAML string
 
-	//go:embed smd-service-live-no-type.yaml
-	ServiceLiveNoTypeYAML string
-
 	//go:embed smd-service-config-2-ports.yaml
 	ServiceConfigWith2Ports string
 
-	//go:embed smd-service-live-expected-2-ports.yaml
-	ExpectedServiceLiveWith2PortsYAML string
-
 	//go:embed smd-service-live-with-type.yaml
 	LiveServiceWithTypeYAML string