From ce5c00ebea98ca1abc8137ea33152ec036d8f12b Mon Sep 17 00:00:00 2001
From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Date: Fri, 18 Oct 2024 09:36:42 -0400
Subject: [PATCH] fix(diff): avoid cache miss in server-side diff (#20423)
 (#20424)

* fix(diff): avoid cache miss in server-side diff (#20423)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix silly mistakes

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Adrian Aneci <aneci@adobe.com>
---
 controller/state.go                    | 23 +++++++++++++--
 controller/state_test.go               | 40 ++++++++++++++++++++++++++
 pkg/apis/application/v1alpha1/types.go | 12 ++++----
 3 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/controller/state.go b/controller/state.go
index 3dbe157e88379..6e847783b5b4f 100644
--- a/controller/state.go
+++ b/controller/state.go
@@ -936,9 +936,7 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou
 		return false
 	}
 
-	currentSpec := app.BuildComparedToStatus()
-	specChanged := !reflect.DeepEqual(app.Status.Sync.ComparedTo, currentSpec)
-	if specChanged {
+	if !specEqualsCompareTo(app.Spec, app.Status.Sync.ComparedTo) {
 		log.WithField("useDiffCache", "false").Debug("specChanged")
 		return false
 	}
@@ -947,6 +945,25 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou
 	return true
 }
 
+// specEqualsCompareTo compares the application spec to the comparedTo status. It normalizes the destination to match
+// the comparedTo destination before comparing. It does not mutate the original spec or comparedTo.
+func specEqualsCompareTo(spec v1alpha1.ApplicationSpec, comparedTo v1alpha1.ComparedTo) bool {
+	// Make a copy to be sure we don't mutate the original.
+	specCopy := spec.DeepCopy()
+	currentSpec := specCopy.BuildComparedToStatus()
+
+	// The spec might have been augmented to include both server and name, so change it to match the comparedTo before
+	// comparing.
+	if comparedTo.Destination.Server == "" {
+		currentSpec.Destination.Server = ""
+	}
+	if comparedTo.Destination.Name == "" {
+		currentSpec.Destination.Name = ""
+	}
+
+	return reflect.DeepEqual(comparedTo, currentSpec)
+}
+
 func (m *appStateManager) persistRevisionHistory(
 	app *v1alpha1.Application,
 	revision string,
diff --git a/controller/state_test.go b/controller/state_test.go
index 5f93393b241c4..971b7ac751273 100644
--- a/controller/state_test.go
+++ b/controller/state_test.go
@@ -1431,6 +1431,8 @@ func TestIsLiveResourceManaged(t *testing.T) {
 }
 
 func TestUseDiffCache(t *testing.T) {
+	t.Parallel()
+
 	type fixture struct {
 		testName             string
 		noCache              bool
@@ -1691,6 +1693,44 @@ func TestUseDiffCache(t *testing.T) {
 			expectedUseCache:     false,
 			serverSideDiff:       false,
 		},
+		{
+			// There are code paths that modify the ApplicationSpec and augment the destination field with both the
+			// destination server and name. Since both fields are populated in the app spec but not in the comparedTo,
+			// we need to make sure we correctly compare the fields and don't miss the cache.
+			testName:      "will return true if the app spec destination contains both server and name, but otherwise matches comparedTo",
+			noCache:       false,
+			manifestInfos: manifestInfos("rev1"),
+			sources:       sources(),
+			app: app("httpbin", "rev1", false, &argoappv1.Application{
+				Spec: argoappv1.ApplicationSpec{
+					Destination: argoappv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Name:      "httpbin",
+						Namespace: "httpbin",
+					},
+				},
+				Status: argoappv1.ApplicationStatus{
+					Resources: []argoappv1.ResourceStatus{},
+					Sync: argoappv1.SyncStatus{
+						Status: argoappv1.SyncStatusCodeSynced,
+						ComparedTo: argoappv1.ComparedTo{
+							Destination: argoappv1.ApplicationDestination{
+								Server:    "https://kubernetes.default.svc",
+								Namespace: "httpbin",
+							},
+						},
+						Revision: "rev1",
+					},
+					ReconciledAt: &metav1.Time{
+						Time: time.Now().Add(-time.Hour),
+					},
+				},
+			}),
+			manifestRevisions:    []string{"rev1"},
+			statusRefreshTimeout: time.Hour * 24,
+			expectedUseCache:     true,
+			serverSideDiff:       true,
+		},
 	}
 
 	for _, tc := range cases {
diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
index c6f3810fb20fa..62b45a27b314a 100644
--- a/pkg/apis/application/v1alpha1/types.go
+++ b/pkg/apis/application/v1alpha1/types.go
@@ -1056,15 +1056,15 @@ func (a *ApplicationStatus) GetRevisions() []string {
 
 // BuildComparedToStatus will build a ComparedTo object based on the current
 // Application state.
-func (app *Application) BuildComparedToStatus() ComparedTo {
+func (spec *ApplicationSpec) BuildComparedToStatus() ComparedTo {
 	ct := ComparedTo{
-		Destination:       app.Spec.Destination,
-		IgnoreDifferences: app.Spec.IgnoreDifferences,
+		Destination:       spec.Destination,
+		IgnoreDifferences: spec.IgnoreDifferences,
 	}
-	if app.Spec.HasMultipleSources() {
-		ct.Sources = app.Spec.Sources
+	if spec.HasMultipleSources() {
+		ct.Sources = spec.Sources
 	} else {
-		ct.Source = app.Spec.GetSource()
+		ct.Source = spec.GetSource()
 	}
 	return ct
 }