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

…y a direct object update (#20875)

May fix #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]>

controller/appcontroller.go

@@ -1927,6 +1927,27 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 	return patchMs
 }
 
+// resetAppStatus 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)

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": "replace",
+			"path": "/operation",
+			"value": { "Sync": {} }
+			}]`).
+		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) {
+		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