From 08ae1a6b7aaf0e672056b85ed4e1ff68b8e74602 Mon Sep 17 00:00:00 2001
From: Andrii Korotkov <andrii.korotkov@verkada.com>
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 <andrii.korotkov@verkada.com>
---
 controller/appcontroller.go         | 26 ++++++++++++++++++++++++++
 test/e2e/app_management_test.go     | 20 ++++++++++++++++++++
 test/e2e/fixture/app/expectation.go |  7 +++++++
 3 files changed, 53 insertions(+)

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/test/e2e/app_management_test.go b/test/e2e/app_management_test.go
index 113d18b5969b7..765f411a02cf1 100644
--- a/test/e2e/app_management_test.go
+++ b/test/e2e/app_management_test.go
@@ -3001,3 +3001,23 @@ 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().
+		PatchApp(`[{
+			"op": "add",
+			"path": "/operation",
+			"value": { "sync": { "revisions": [] }, "initiatedBy": { "username": "test" } }
+			}]`).
+		Then().
+		Expect(OperationStateIsNil())
+}
diff --git a/test/e2e/fixture/app/expectation.go b/test/e2e/fixture/app/expectation.go
index b5e83a664085c..af6786c0fdb2a 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) {
+		operationState := c.app().Status.OperationState
+		return simple(operationState == nil, fmt.Sprintf("operation state should be nil, is %v", operationState))
+	}
+}
+
 func simple(success bool, message string) (state, string) {
 	if success {
 		return succeeded, message