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 <[email protected]>

Author: a-buck

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
+	if sc.dryRun || dryRun {
+		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.

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())
+		})
+	}
+}

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