diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index a7b08fa5f..5badf54ac 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1042,15 +1042,7 @@ func (sc *syncContext) ensureCRDReady(name string) error { }) } -func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstructured, dryRun bool) bool { - // if it is a dry run, disable server side apply, as the goal is to validate only the - // yaml correctness of the rendered manifests. - // running dry-run in server mode breaks the auto create namespace feature - // https://github.com/argoproj/argo-cd/issues/13874 - if sc.dryRun || dryRun { - return false - } - +func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstructured) bool { resourceHasDisableSSAAnnotation := resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionDisableServerSideApply) if resourceHasDisableSSAAnnotation { return false @@ -1059,22 +1051,36 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply) } +func (sc *syncContext) getDryRunStrategy(serverSideApply bool, isAutoCreateNamespaceFeatureEnabled bool) cmdutil.DryRunStrategy { + if serverSideApply { + // if auto create namespace is not enabled (CreateNamespace option not set), + // then server apply can be used for dry runs. Users can choose whether to prioritise + // convenience of namespaces being created automatically with client side only feedback, + // or manage the namespace themselves and have rich feedback with server side apply. + // https://github.com/argoproj/argo-cd/issues/13874 + if !isAutoCreateNamespaceFeatureEnabled { + return cmdutil.DryRunServer + } + sc.log.Info("server side apply is not supported for dry run when auto create namespace is enabled. Falling back to client side dry run") + } + return cmdutil.DryRunClient +} + func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { + isResourceEligibleForServerSideApply := sc.shouldUseServerSideApply(t.targetObj) + dryRunStrategy := cmdutil.DryRunNone if dryRun { - // irrespective of the dry run mode set in the sync context, always run - // in client dry run mode as the goal is to validate only the - // yaml correctness of the rendered manifests. - // running dry-run in server mode breaks the auto create namespace feature - // https://github.com/argoproj/argo-cd/issues/13874 - dryRunStrategy = cmdutil.DryRunClient + dryRunStrategy = sc.getDryRunStrategy(isResourceEligibleForServerSideApply, sc.syncNamespace != nil) } + serverSideApply := isResourceEligibleForServerSideApply && dryRunStrategy != cmdutil.DryRunClient + var err error var message string shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace) force := sc.force || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForce) - serverSideApply := sc.shouldUseServerSideApply(t.targetObj, dryRun) + if shouldReplace { if t.liveObj != nil { // Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances. diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 5e7bc84b9..c754f83d8 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/client-go/rest" testcore "k8s.io/client-go/testing" "k8s.io/klog/v2/textlogger" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "github.com/argoproj/gitops-engine/pkg/diff" "github.com/argoproj/gitops-engine/pkg/health" @@ -860,9 +861,9 @@ func TestSyncContext_ServerSideApplyWithDryRun(t *testing.T) { objToUse func(*unstructured.Unstructured) *unstructured.Unstructured }{ {"BothFlagsFalseAnnotated", false, false, true, withServerSideApplyAnnotation}, - {"scDryRunTrueAnnotated", true, false, false, withServerSideApplyAnnotation}, - {"dryRunTrueAnnotated", false, true, false, withServerSideApplyAnnotation}, - {"BothFlagsTrueAnnotated", true, true, false, withServerSideApplyAnnotation}, + {"scDryRunTrueAnnotated", true, false, true, withServerSideApplyAnnotation}, + {"dryRunTrueAnnotated", false, true, true, withServerSideApplyAnnotation}, + {"BothFlagsTrueAnnotated", true, true, true, withServerSideApplyAnnotation}, {"AnnotatedDisabledSSA", false, false, false, withDisableServerSideApplyAnnotation}, } @@ -873,7 +874,7 @@ func TestSyncContext_ServerSideApplyWithDryRun(t *testing.T) { targetObj := tc.objToUse(testingutils.NewPod()) // Execute the shouldUseServerSideApply method and assert expectations - serverSideApply := sc.shouldUseServerSideApply(targetObj, tc.dryRun) + serverSideApply := sc.shouldUseServerSideApply(targetObj) assert.Equal(t, tc.expectedSSA, serverSideApply) }) } @@ -2180,3 +2181,70 @@ func BenchmarkSync(b *testing.B) { syncCtx.Sync() } } + +func TestSyncContext_ApplyObjectDryRun(t *testing.T) { + testCases := []struct { + name string + target *unstructured.Unstructured + live *unstructured.Unstructured + dryRun bool + syncNamespace func(*unstructured.Unstructured, *unstructured.Unstructured) (bool, error) + serverSideApply bool + expectedDryRun cmdutil.DryRunStrategy + }{ + { + name: "DryRunWithNoAutoCreateNamespace", + target: withServerSideApplyAnnotation(testingutils.NewPod()), + live: testingutils.NewPod(), + dryRun: true, + syncNamespace: nil, + serverSideApply: true, + expectedDryRun: cmdutil.DryRunServer, + }, + { + name: "DryRunWithAutoCreateNamespace", + target: withServerSideApplyAnnotation(testingutils.NewPod()), + live: testingutils.NewPod(), + dryRun: true, + syncNamespace: func(*unstructured.Unstructured, *unstructured.Unstructured) (bool, error) { return true, nil }, + serverSideApply: true, + expectedDryRun: cmdutil.DryRunClient, + }, + { + name: "NoDryRun", + target: withServerSideApplyAnnotation(testingutils.NewPod()), + live: testingutils.NewPod(), + dryRun: false, + syncNamespace: nil, + serverSideApply: true, + expectedDryRun: cmdutil.DryRunNone, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + syncCtx := newTestSyncCtx(nil) + syncCtx.serverSideApply = tc.serverSideApply + syncCtx.syncNamespace = tc.syncNamespace + + tc.target.SetNamespace(testingutils.FakeArgoCDNamespace) + if tc.live != nil { + tc.live.SetNamespace(testingutils.FakeArgoCDNamespace) + } + + task := &syncTask{ + targetObj: tc.target, + liveObj: tc.live, + } + + result, _ := syncCtx.applyObject(task, tc.dryRun, true) + + assert.Equal(t, synccommon.ResultCodeSynced, result) + + resourceOps, _ := syncCtx.resourceOps.(*kubetest.MockResourceOps) + assert.Equal(t, tc.expectedDryRun, resourceOps.GetLastDryRunStrategy()) + }) + } +} diff --git a/pkg/utils/kube/kubetest/mock_resource_operations.go b/pkg/utils/kube/kubetest/mock_resource_operations.go index a97c4d09a..10ae8e08f 100644 --- a/pkg/utils/kube/kubetest/mock_resource_operations.go +++ b/pkg/utils/kube/kubetest/mock_resource_operations.go @@ -24,6 +24,7 @@ type MockResourceOps struct { serverSideApply bool serverSideApplyManager string lastForce bool + lastDryRunStrategy cmdutil.DryRunStrategy recordLock sync.RWMutex @@ -106,12 +107,26 @@ func (r *MockResourceOps) GetLastResourceCommand(key kube.ResourceKey) string { return r.lastCommandPerResource[key] } -func (r *MockResourceOps) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, force bool, validate bool, serverSideApply bool, manager string) (string, error) { +func (r *MockResourceOps) SetLastDryRunStrategy(dryRunStrategy cmdutil.DryRunStrategy) { + r.recordLock.Lock() + r.lastDryRunStrategy = dryRunStrategy + r.recordLock.Unlock() +} + +func (r *MockResourceOps) GetLastDryRunStrategy() cmdutil.DryRunStrategy { + r.recordLock.RLock() + dryRunStrategy := r.lastDryRunStrategy + r.recordLock.RUnlock() + return dryRunStrategy +} + +func (r *MockResourceOps) ApplyResource(_ context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force bool, validate bool, serverSideApply bool, manager string) (string, error) { r.SetLastValidate(validate) r.SetLastServerSideApply(serverSideApply) r.SetLastServerSideApplyManager(manager) r.SetLastForce(force) r.SetLastResourceCommand(kube.GetResourceKey(obj), "apply") + r.SetLastDryRunStrategy(dryRunStrategy) command, ok := r.Commands[obj.GetName()] if !ok { return "", nil