fix: Set operation state to nil if new operation has been initiated b…

…y a direct object update (argoproj#20875)

Fixes argoproj#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 akuity/kargo#2985 and the discussion. Handle this case on the app controller side.

Let's cherry-pick to v2.13.

Signed-off-by: Andrii Korotkov <[email protected]>

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

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)

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
 		}

server/application/application_test.go

@@ -1972,6 +1972,37 @@ 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,
+			// Keep annotation value in sync with common.AnnotationAllowPatchingOperationTestOnly
+			Patch: ptr.To(`[
+				{
+					"op": "add",
+					"path": "/metadata/annotations",
+					"value": {
+						"argocd.argoproj.io/allow-patching-operation-test-only": "true"
+					}
+				},
+				{
+					"op": "add",
+					"path": "/operation",
+					"value": {
+						"sync": {
+							"revisions": ["test"]
+						},
+						"initiatedBy": {
+							"username": "test"
+						}
+					}
+				}
+			]`),
+		},
+	)
+	require.NoError(t, err)
+	assert.NotNil(t, app.Operation)
 }
 
 func TestAppMergePatch(t *testing.T) {

test/e2e/app_management_test.go

@@ -3001,3 +3001,40 @@ 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(`[
+			{
+				"op": "add",
+				"path": "/metadata/annotations",
+				"value": {
+					"argocd.argoproj.io/allow-patching-operation-test-only": "true"
+				}
+			},
+			{
+				"op": "add",
+				"path": "/operation",
+				"value": {
+					"sync": {
+						"revisions": "test"
+					},
+					"initiatedBy": {
+						"username": "test"
+					}
+				}
+			}
+		]`).
+		Then().
+		Expect(OperationStateIsNil())
+}

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