From bb5db090231c3bb2a3cac6a65b985caeb99c82d8 Mon Sep 17 00:00:00 2001 From: Andrii Korotkov Date: Sat, 23 Nov 2024 08:23:50 -0800 Subject: [PATCH] fix: Set operation state to nil if new operation has been initiated by a direct object update (#20875) Fixes #20875 Some systems like Kargo update the application operation directly to initiate a new sync, bypassing the server. However, they can't update the status operation state to become nil, resulting in discrepancy, see https://github.com/akuity/kargo/issues/2985 and the discussion. Handle this case on the app controller side. Let's cherry-pick to v2.13. Signed-off-by: Andrii Korotkov --- common/common.go | 5 ++++ controller/appcontroller.go | 26 +++++++++++++++++ server/application/application.go | 6 ++-- server/application/application_test.go | 33 +++++++++++++++++++++ test/e2e/app_management_test.go | 40 ++++++++++++++++++++++++++ test/e2e/fixture/app/expectation.go | 7 +++++ 6 files changed, 115 insertions(+), 2 deletions(-) diff --git a/common/common.go b/common/common.go index eea862644d166..830bc1c01bbee 100644 --- a/common/common.go +++ b/common/common.go @@ -189,6 +189,11 @@ const ( // can be disregarded. AnnotationIgnoreHealthCheck = "argocd.argoproj.io/ignore-healthcheck" + // AnnotationAllowPatchingOperationTestOnly when set the patch operations also can update the operation field. + // Intended for use in tests only. Keep in sync with tests using argocd.argoproj.io~1allow-patching-operation-test-only or + // argocd.argoproj.io/allow-patching-operation-test-only directly. + AnnotationAllowPatchingOperationTestOnly = "argocd.argoproj.io/allow-patching-operation-test-only" + // AnnotationKeyManagedBy is annotation name which indicates that k8s resource is managed by an application. AnnotationKeyManagedBy = "managed-by" // AnnotationValueManagedByArgoCD is a 'managed-by' annotation value for resources managed by Argo CD diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 5a48f1d41cb09..3659e2f598e2f 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1927,6 +1927,27 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new return patchMs } +// resetAppStatusOperationState updates the app status operation state to be nil. If no changes were made, it is a no-op +func (ctrl *ApplicationController) resetAppStatusOperationState(orig *appv1.Application) { + logCtx := getAppLog(orig).WithField("method", "resetAppStatusOperationState") + if orig.Status.OperationState == nil { + return + } + newStatus := orig.Status.DeepCopy() + newStatus.OperationState = nil + patch, _, err := createMergePatch( + &appv1.Application{Status: orig.Status}, + &appv1.Application{Status: *newStatus}) + if err != nil { + logCtx.Errorf("Error constructing app status patch: %v", err) + return + } + _, err = ctrl.PatchAppWithWriteBack(context.Background(), orig.Name, orig.Namespace, types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + logCtx.Warnf("Error updating application: %v", err) + } +} + // autoSync will initiate a sync operation for an application configured with automated sync func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *appv1.SyncStatus, resources []appv1.ResourceStatus, revisionUpdated bool) (*appv1.ApplicationCondition, time.Duration) { logCtx := getAppLog(app) @@ -2318,6 +2339,11 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar jitter := time.Duration(float64(ctrl.statusRefreshJitter) * rand.Float64()) delay = &jitter } + // Something updated application operation state bypassing the controller and + // operation state in status still reflects the old sync. Reset it. + if oldApp.Operation == nil && newApp.Operation != nil && newApp.Status.OperationState != nil { + ctrl.resetAppStatusOperationState(newApp) + } } ctrl.requestAppRefresh(newApp.QualifiedName(), compareWith, delay) diff --git a/server/application/application.go b/server/application/application.go index 838110c388e1d..253f680a41f09 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -937,12 +937,14 @@ func (s *Server) updateApp(app *appv1.Application, newApp *appv1.Application, ct app.Labels = newApp.Labels app.Annotations = newApp.Annotations } - + if app.Annotations[argocommon.AnnotationAllowPatchingOperationTestOnly] == "true" { + app.Operation = newApp.Operation + } app.Finalizers = newApp.Finalizers res, err := s.appclientset.ArgoprojV1alpha1().Applications(app.Namespace).Update(ctx, app, metav1.UpdateOptions{}) if err == nil { - s.logAppEvent(app, ctx, argo.EventReasonResourceUpdated, "updated application spec") + s.logAppEvent(app, ctx, argo.EventReasonResourceUpdated, "updated application") s.waitSync(res) return res, nil } diff --git a/server/application/application_test.go b/server/application/application_test.go index 0c409357995fe..89d3fed36ec7b 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1972,6 +1972,39 @@ func TestAppJsonPatch(t *testing.T) { app, err = appServer.Patch(ctx, &application.ApplicationPatchRequest{Name: &testApp.Name, Patch: ptr.To(`[{"op": "remove", "path": "/metadata/annotations/test.annotation"}]`)}) require.NoError(t, err) assert.NotContains(t, app.Annotations, "test.annotation") + + app, err = appServer.Patch( + ctx, + &application.ApplicationPatchRequest{ + Name: &testApp.Name, + Patch: ptr.To(fmt.Sprintf( + `[ + { + "op": "add", + "path": "/metadata/annotations", + "value": { + "%s": "true" + } + }, + { + "op": "add", + "path": "/operation", + "value": { + "sync": { + "revisions": ["test"] + }, + "initiatedBy": { + "username": "test" + } + } + } + ]`, + common.AnnotationAllowPatchingOperationTestOnly, + )), + }, + ) + require.NoError(t, err) + assert.NotNil(t, app.Operation) } func TestAppMergePatch(t *testing.T) { diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 113d18b5969b7..c81f4cd80d34e 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -3001,3 +3001,43 @@ func TestDeletionConfirmation(t *testing.T) { When().ConfirmDeletion(). Then().Expect(DoesNotExist()) } + +func TestNewOperationTriggeredByUpdatingAppObject(t *testing.T) { + Given(t). + Path(guestbookPath). + When(). + CreateApp(). + Sync(). + Then(). + Expect(OperationPhaseIs(OperationSucceeded)). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(HealthIs(health.HealthStatusHealthy)). + When(). + // Keep annotation value in sync with common.AnnotationAllowPatchingOperationTestOnly + PatchApp(fmt.Sprintf( + `[ + { + "op": "add", + "path": "/metadata/annotations", + "value": { + "%s": "true" + } + }, + { + "op": "add", + "path": "/operation", + "value": { + "sync": { + "revisions": ["test"] + }, + "initiatedBy": { + "username": "test" + } + } + } + ]`, + common.AnnotationAllowPatchingOperationTestOnly, + )). + Then(). + Expect(OperationStateIsNil()) +} diff --git a/test/e2e/fixture/app/expectation.go b/test/e2e/fixture/app/expectation.go index b5e83a664085c..080a0648c9df0 100644 --- a/test/e2e/fixture/app/expectation.go +++ b/test/e2e/fixture/app/expectation.go @@ -50,6 +50,13 @@ func OperationMessageContains(text string) Expectation { } } +func OperationStateIsNil() Expectation { + return func(c *Consequences) (state, string) { + actual := c.app().Status.OperationState + return simple(actual == nil, fmt.Sprintf("operation state should be nil, is %s", actual)) + } +} + func simple(success bool, message string) (state, string) { if success { return succeeded, message