fix(sync): auto-sync loop when FailOnSharedResource (#23357)

controller/sync.go

@@ -108,15 +108,6 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	}
 	syncOp = *state.Operation.Sync
 
-	// validates if it should fail the sync if it finds shared resources
-	hasSharedResource, sharedResourceMessage := hasSharedResourceCondition(app)
-	if syncOp.SyncOptions.HasOption("FailOnSharedResource=true") &&
-		hasSharedResource {
-		state.Phase = common.OperationFailed
-		state.Message = "Shared resource found: " + sharedResourceMessage
-		return
-	}
-
 	isMultiSourceRevision := app.Spec.HasMultipleSources()
 	rollback := len(syncOp.Sources) > 0 || syncOp.Source != nil
 	if rollback {
@@ -207,6 +198,15 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	syncRes.Revision = compareResult.syncStatus.Revision
 	syncRes.Revisions = compareResult.syncStatus.Revisions
 
+	// validates if it should fail the sync if it finds shared resources
+	hasSharedResource, sharedResourceMessage := hasSharedResourceCondition(app)
+	if syncOp.SyncOptions.HasOption("FailOnSharedResource=true") &&
+		hasSharedResource {
+		state.Phase = common.OperationFailed
+		state.Message = "Shared resource found: %s" + sharedResourceMessage
+		return
+	}
+
 	// If there are any comparison or spec errors error conditions do not perform the operation
 	if errConditions := app.Status.GetConditions(map[v1alpha1.ApplicationConditionType]bool{
 		v1alpha1.ApplicationConditionComparisonError:  true,

controller/sync_test.go

@@ -5,7 +5,7 @@ import (
 	"testing"
 
 	"github.com/argoproj/gitops-engine/pkg/sync"
-	"github.com/argoproj/gitops-engine/pkg/sync/common"
+	synccommon "github.com/argoproj/gitops-engine/pkg/sync/common"
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -14,6 +14,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 
+	"github.com/argoproj/argo-cd/v3/common"
 	"github.com/argoproj/argo-cd/v3/controller/testdata"
 	"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v3/reposerver/apiclient"
@@ -196,18 +197,28 @@ func TestAppStateManager_SyncAppState(t *testing.T) {
 		controller  *ApplicationController
 	}
 
-	setup := func() *fixture {
+	setup := func(liveObjects map[kube.ResourceKey]*unstructured.Unstructured) *fixture {
 		app := newFakeApp()
 		app.Status.OperationState = nil
 		app.Status.History = nil
 
+		if liveObjects == nil {
+			liveObjects = make(map[kube.ResourceKey]*unstructured.Unstructured)
+		}
+
 		project := &v1alpha1.AppProject{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: test.FakeArgoCDNamespace,
 				Name:      "default",
 			},
 			Spec: v1alpha1.AppProjectSpec{
 				SignatureKeys: []v1alpha1.SignatureKey{{KeyID: "test"}},
+				Destinations: []v1alpha1.ApplicationDestination{
+					{
+						Namespace: "*",
+						Server:    "*",
+					},
+				},
 			},
 		}
 		data := fakeData{
@@ -218,7 +229,7 @@ func TestAppStateManager_SyncAppState(t *testing.T) {
 				Server:    test.FakeClusterURL,
 				Revision:  "abc123",
 			},
-			managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
+			managedLiveObjs: liveObjects,
 		}
 		ctrl := newFakeController(&data, nil)
 
@@ -231,13 +242,23 @@ func TestAppStateManager_SyncAppState(t *testing.T) {
 	t.Run("will fail the sync if finds shared resources", func(t *testing.T) {
 		// given
 		t.Parallel()
-		f := setup()
-		syncErrorMsg := "deployment already applied by another application"
-		condition := v1alpha1.ApplicationCondition{
-			Type:    v1alpha1.ApplicationConditionSharedResourceWarning,
-			Message: syncErrorMsg,
-		}
-		f.application.Status.Conditions = append(f.application.Status.Conditions, condition)
+
+		sharedObject := kube.MustToUnstructured(&corev1.ConfigMap{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "v1",
+				Kind:       "ConfigMap",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "configmap1",
+				Namespace: "default",
+				Annotations: map[string]string{
+					common.AnnotationKeyAppInstance: "guestbook:/ConfigMap:default/configmap1",
+				},
+			},
+		})
+		liveObjects := make(map[kube.ResourceKey]*unstructured.Unstructured)
+		liveObjects[kube.GetResourceKey(sharedObject)] = sharedObject
+		f := setup(liveObjects)
 
 		// Sync with source unspecified
 		opState := &v1alpha1.OperationState{Operation: v1alpha1.Operation{
@@ -251,8 +272,8 @@ func TestAppStateManager_SyncAppState(t *testing.T) {
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then
-		assert.Equal(t, common.OperationFailed, opState.Phase)
-		assert.Contains(t, opState.Message, syncErrorMsg)
+		assert.Equal(t, synccommon.OperationFailed, opState.Phase)
+		assert.Contains(t, opState.Message, "ConfigMap/configmap1 is part of applications fake-argocd-ns/my-app and guestbook")
 	})
 }
 
@@ -315,13 +336,13 @@ func TestSyncWindowDeniesSync(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 		// when
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then
-		assert.Equal(t, common.OperationRunning, opState.Phase)
+		assert.Equal(t, synccommon.OperationRunning, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 }
@@ -1405,13 +1426,13 @@ func TestSyncWithImpersonate(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 		// when
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then, app sync should fail with expected error message in operation state
-		assert.Equal(t, common.OperationError, opState.Phase)
+		assert.Equal(t, synccommon.OperationError, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 
@@ -1426,13 +1447,13 @@ func TestSyncWithImpersonate(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 		// when
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then app sync should fail with expected error message in operation state
-		assert.Equal(t, common.OperationError, opState.Phase)
+		assert.Equal(t, synccommon.OperationError, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 
@@ -1447,13 +1468,13 @@ func TestSyncWithImpersonate(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 		// when
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then app sync should not fail
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 
@@ -1468,13 +1489,13 @@ func TestSyncWithImpersonate(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 		// when
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then application sync should pass using the control plane service account
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 
@@ -1489,7 +1510,7 @@ func TestSyncWithImpersonate(t *testing.T) {
 					Source: &v1alpha1.ApplicationSource{},
 				},
 			},
-			Phase: common.OperationRunning,
+			Phase: synccommon.OperationRunning,
 		}
 
 		f.application.Spec.Destination.Server = ""
@@ -1499,7 +1520,7 @@ func TestSyncWithImpersonate(t *testing.T) {
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then app sync should not fail
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, opMessage)
 	})
 }
@@ -1567,7 +1588,7 @@ func TestClientSideApplyMigration(t *testing.T) {
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, "successfully synced")
 	})
 
@@ -1585,7 +1606,7 @@ func TestClientSideApplyMigration(t *testing.T) {
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, "successfully synced")
 	})
 
@@ -1603,7 +1624,7 @@ func TestClientSideApplyMigration(t *testing.T) {
 		f.controller.appStateManager.SyncAppState(f.application, opState)
 
 		// then
-		assert.Equal(t, common.OperationSucceeded, opState.Phase)
+		assert.Equal(t, synccommon.OperationSucceeded, opState.Phase)
 		assert.Contains(t, opState.Message, "successfully synced")
 	})
 }