diff --git a/controller/state.go b/controller/state.go index 80678b74790e7..3e3eba340819f 100644 --- a/controller/state.go +++ b/controller/state.go @@ -904,9 +904,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 } @@ -915,6 +913,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 a371a30baddce..2ff91ce19f68d 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -1432,6 +1432,8 @@ func TestIsLiveResourceManaged(t *testing.T) { } func TestUseDiffCache(t *testing.T) { + t.Parallel() + type fixture struct { testName string noCache bool @@ -1692,6 +1694,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 e8d9dcb3f36bf..27faddf9fc8a3 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -981,15 +981,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 }