Skip to content

feat: use server side apply for dry runs where possible #725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: use server side apply for dry runs where possible #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions pkg/sync/sync_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
76 changes: 72 additions & 4 deletions pkg/sync/sync_context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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},
}

Expand All @@ -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)
})
}
Expand Down Expand Up @@ -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())
})
}
}
17 changes: 16 additions & 1 deletion pkg/utils/kube/kubetest/mock_resource_operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type MockResourceOps struct {
serverSideApply bool
serverSideApplyManager string
lastForce bool
lastDryRunStrategy cmdutil.DryRunStrategy

recordLock sync.RWMutex

Expand Down Expand Up @@ -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
Expand Down