From a7e6f9c2c7ef92e0385817867c88b0964c757770 Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Thu, 12 Jun 2025 11:58:04 -0400 Subject: [PATCH 1/5] fix(sync): auto-sync loop when FailOnSharedResource (#23357) Signed-off-by: Alexandre Gaudreault --- controller/sync.go | 18 ++++++++--------- controller/sync_test.go | 43 +++++++++++++++++++++++++++++++---------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index b455bd2860fb8..232d3e4ea0ecf 100644 --- a/controller/sync.go +++ b/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 = fmt.Sprintf("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, diff --git a/controller/sync_test.go b/controller/sync_test.go index bfb54e77e880f..17fe5fc7eb0ae 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + argocommon "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" @@ -193,14 +194,19 @@ func TestAppStateManager_SyncAppState(t *testing.T) { type fixture struct { application *v1alpha1.Application + project *v1alpha1.AppProject 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, @@ -208,6 +214,12 @@ func TestAppStateManager_SyncAppState(t *testing.T) { }, Spec: v1alpha1.AppProjectSpec{ SignatureKeys: []v1alpha1.SignatureKey{{KeyID: "test"}}, + Destinations: []v1alpha1.ApplicationDestination{ + { + Namespace: "*", + Server: "*", + }, + }, }, } data := fakeData{ @@ -218,12 +230,13 @@ func TestAppStateManager_SyncAppState(t *testing.T) { Server: test.FakeClusterURL, Revision: "abc123", }, - managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured), + managedLiveObjs: liveObjects, } ctrl := newFakeController(&data, nil) return &fixture{ application: app, + project: project, controller: ctrl, } } @@ -231,13 +244,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: v1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "configmap1", + Namespace: "default", + Labels: map[string]string{ + argocommon.LabelKeyAppInstance: "another-app", + }, + }, + }) + 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{ @@ -252,7 +275,7 @@ func TestAppStateManager_SyncAppState(t *testing.T) { // then assert.Equal(t, common.OperationFailed, opState.Phase) - assert.Contains(t, opState.Message, syncErrorMsg) + assert.Contains(t, opState.Message, "ConfigMap/configmap1 is part of applications fake-argocd-ns/my-app and another-app") }) } From a8dc7ac0c559dc91293b99b9ae7cec60d30327d2 Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Wed, 2 Jul 2025 11:12:33 -0400 Subject: [PATCH 2/5] fix cherry-pick Signed-off-by: Alexandre Gaudreault --- controller/sync_test.go | 42 ++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 17fe5fc7eb0ae..f371051f808c3 100644 --- a/controller/sync_test.go +++ b/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,7 +14,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - argocommon "github.com/argoproj/argo-cd/v3/common" + "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" @@ -246,15 +246,15 @@ func TestAppStateManager_SyncAppState(t *testing.T) { t.Parallel() sharedObject := kube.MustToUnstructured(&corev1.ConfigMap{ - TypeMeta: v1.TypeMeta{ + TypeMeta: metav1.TypeMeta{ APIVersion: "v1", Kind: "ConfigMap", }, - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: "configmap1", Namespace: "default", - Labels: map[string]string{ - argocommon.LabelKeyAppInstance: "another-app", + Annotations: map[string]string{ + common.AnnotationKeyAppInstance: "guestbook:/ConfigMap:default/configmap1", }, }, }) @@ -274,8 +274,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, "ConfigMap/configmap1 is part of applications fake-argocd-ns/my-app and another-app") + 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") }) } @@ -338,13 +338,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) }) } @@ -1360,13 +1360,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) }) @@ -1381,13 +1381,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) }) @@ -1402,13 +1402,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) }) @@ -1423,13 +1423,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) }) } @@ -1497,7 +1497,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") }) @@ -1515,7 +1515,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") }) @@ -1533,7 +1533,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") }) } From 494b5be097995a53c5a5636076c86a6a6566413a Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Wed, 2 Jul 2025 11:22:04 -0400 Subject: [PATCH 3/5] lint Signed-off-by: Alexandre Gaudreault --- controller/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/sync.go b/controller/sync.go index 232d3e4ea0ecf..a130900cc95cf 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -203,7 +203,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha if syncOp.SyncOptions.HasOption("FailOnSharedResource=true") && hasSharedResource { state.Phase = common.OperationFailed - state.Message = fmt.Sprintf("Shared resource found: %s", sharedResourceMessage) + state.Message = "Shared resource found: %s" + sharedResourceMessage return } From 3a795d0a36ff5a6be4718ffcc70f465d14cb7630 Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Wed, 2 Jul 2025 11:58:21 -0400 Subject: [PATCH 4/5] fix merge Signed-off-by: Alexandre Gaudreault --- controller/sync_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 525440e7c9b96..7a2f8b8467960 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1512,7 +1512,7 @@ func TestSyncWithImpersonate(t *testing.T) { Source: &v1alpha1.ApplicationSource{}, }, }, - Phase: common.OperationRunning, + Phase: synccommon.OperationRunning, } f.application.Spec.Destination.Server = "" @@ -1522,7 +1522,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) }) } From 56c918bd847dfd769d6f87af8e2bfc8ee152f9ac Mon Sep 17 00:00:00 2001 From: Alexandre Gaudreault Date: Wed, 2 Jul 2025 12:09:41 -0400 Subject: [PATCH 5/5] lint Signed-off-by: Alexandre Gaudreault --- controller/sync_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 7a2f8b8467960..815babaea395a 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -194,7 +194,6 @@ func TestAppStateManager_SyncAppState(t *testing.T) { type fixture struct { application *v1alpha1.Application - project *v1alpha1.AppProject controller *ApplicationController } @@ -236,7 +235,6 @@ func TestAppStateManager_SyncAppState(t *testing.T) { return &fixture{ application: app, - project: project, controller: ctrl, } }