From bd6cd7c21a11cc67b995ec4f2214feef1425dfb1 Mon Sep 17 00:00:00 2001 From: abuck Date: Wed, 21 May 2025 22:16:25 +0100 Subject: [PATCH] feat: use server side apply for dry runs where possible Previously dry runs would always use client side apply. This is because server side apply is not compatible with the namespace auto creation feature - since the server side validation fails in this case (as expected). This change uses server side apply for dry runs where ServerSideApply=true option is set, but not in the case where the CreateNamespace=true option is set. An alternative would be to return an error if both auto-create namespace and server side apply options were set for dry runs. However this would be more of a breaking change Signed-off-by: abuck --- pkg/sync/sync_context.go | 38 ++++++---- pkg/sync/sync_context_test.go | 76 ++++++++++++++++++- .../kube/kubetest/mock_resource_operations.go | 17 ++++- 3 files changed, 110 insertions(+), 21 deletions(-) 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