From 855b7b55bd16351639976986870e8a1879927893 Mon Sep 17 00:00:00 2001 From: Andrii Korotkov Date: Fri, 13 Dec 2024 07:11:48 -0800 Subject: [PATCH] feat: New sync flags for replace and force Gitops-engine part of https://github.com/argoproj/argo-cd/issues/20730 Add new way of specifying options and include options like always, never, if-requested, if-immutable-fields-updated, if-apply-failed. Tests can be better, but I haven't figure out how to configure apply failur with existing mocks. Signed-off-by: Andrii Korotkov --- pkg/sync/common/types.go | 24 +++++++ pkg/sync/sync_context.go | 121 +++++++++++++++++++++++++++------- pkg/sync/sync_context_test.go | 35 ++++++++-- 3 files changed, 151 insertions(+), 29 deletions(-) diff --git a/pkg/sync/common/types.go b/pkg/sync/common/types.go index b02ad8c20..86c58a6f9 100644 --- a/pkg/sync/common/types.go +++ b/pkg/sync/common/types.go @@ -28,8 +28,32 @@ const ( SyncOptionPruneLast = "PruneLast=true" // Sync option that enables use of replace or create command instead of apply SyncOptionReplace = "Replace=true" + // Sync option that disables use of replace or create command instead of apply + SyncOptionReplaceFalse = "Replace=false" + // Sync option that enables use of replace or create command instead of apply + SyncOptionReplaceAlways = "Replace=always" + // Sync option that disables use of replace or create command instead of apply + SyncOptionReplaceNever = "Replace=never" + // Sync option that enables use of replace or create command instead of apply if API requested replace + SyncOptionReplaceIfRequested = "Replace=if-requested" + // Sync option that enables use of replace or create command instead of apply if immutable fields were updated + SyncOptionReplaceIfImmutableFieldsUpdated = "Replace=if-immutable-fields-updated" + // Sync option that enables use of replace or create command instead of apply if apply failed + SyncOptionReplaceIfApplyFailed = "Replace=if-apply-failed" // Sync option that enables use of --force flag, delete and re-create SyncOptionForce = "Force=true" + // Sync option that disables use of --force flag, delete and re-create + SyncOptionForceFalse = "Force=false" + // Sync option that enables use of --force flag, delete and re-create + SyncOptionForceAlways = "Force=always" + // Sync option that disables use of --force flag, delete and re-create + SyncOptionForceNever = "Force=never" + // Sync option that enables use of --force flag, delete and re-create if API requested replace + SyncOptionForceIfRequested = "Force=if-requested" + // Sync option that enables use of --force flag, delete and re-createif immutable fields were updated + SyncOptionForceIfImmutableFieldsUpdated = "Force=if-immutable-fields-updated" + // Sync option that enables use of --force flag, delete and re-create if apply failed + SyncOptionForceIfApplyFailed = "Force=if-apply-failed" // Sync option that enables use of --server-side flag instead of client-side SyncOptionServerSideApply = "ServerSideApply=true" // Sync option that disables use of --server-side flag instead of client-side diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 35981ebaa..6dc97f0e3 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -14,6 +14,7 @@ import ( v1extensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" apierr "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" @@ -23,6 +24,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" "k8s.io/klog/v2/textlogger" + "k8s.io/kubectl/pkg/cmd/util" cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/util/openapi" @@ -123,7 +125,15 @@ func WithPruneConfirmed(confirmed bool) SyncOpt { } } +// WithDryRun sets dry run setting +func WithDryRun(dryRun bool) SyncOpt { + return func(ctx *syncContext) { + ctx.dryRun = dryRun + } +} + // WithOperationSettings allows to set sync operation settings +// Deprecated, use individual setters func WithOperationSettings(dryRun bool, prune bool, force bool, skipHooks bool) SyncOpt { return func(ctx *syncContext) { ctx.prune = prune @@ -183,12 +193,30 @@ func WithSyncWaveHook(syncWaveHook common.SyncWaveHook) SyncOpt { } } +// WithReplace sets a replace to a given value +// Deprecated, prefer using WithReplaceOption func WithReplace(replace bool) SyncOpt { return func(ctx *syncContext) { ctx.replace = replace } } +// WithReplaceOptions sets replace options +func WithReplaceOptions(replaceOption string, replaceRequested bool) SyncOpt { + return func(ctx *syncContext) { + ctx.replaceOption = replaceOption + ctx.replaceRequested = replaceRequested + } +} + +// WithReplaceOption sets force options +func WithForceOptions(forceOption string, forceRequested bool) SyncOpt { + return func(ctx *syncContext) { + ctx.forceOption = forceOption + ctx.forceRequested = forceRequested + } +} + func WithServerSideApply(serverSideApply bool) SyncOpt { return func(ctx *syncContext) { ctx.serverSideApply = serverSideApply @@ -337,11 +365,15 @@ type syncContext struct { dryRun bool force bool + forceOption string + forceRequested bool validate bool skipHooks bool resourcesFilter func(key kube.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool prune bool replace bool + replaceOption string + replaceRequested bool serverSideApply bool serverSideApplyManager string pruneLast bool @@ -965,6 +997,64 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply) } +func (sc *syncContext) replaceObject(t *syncTask, dryRunStrategy util.DryRunStrategy, force bool, validate bool) (message string, err error) { + if t.liveObj != nil { + // Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances. + // The same thing applies for namespaces, which would delete the namespace as well as everything within it, + // so we want to avoid using `kubectl replace` in that case as well. + if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind { + update := t.targetObj.DeepCopy() + update.SetResourceVersion(t.liveObj.GetResourceVersion()) + _, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy) + if err == nil { + message = fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName()) + } else { + message = fmt.Sprintf("error when updating: %v", err.Error()) + } + } else { + message, err = sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force) + } + } else { + message, err = sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate) + } + return message, err +} + +func (sc *syncContext) shouldReplaceByDefault(t *syncTask) bool { + return (sc.replace || + resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace) || + sc.replaceOption == common.SyncOptionReplace || + sc.replaceOption == common.SyncOptionReplaceAlways || + resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceAlways) || + sc.replaceRequested && sc.replaceOption == common.SyncOptionReplaceIfRequested || + sc.replaceRequested && resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfRequested)) && + !(sc.replaceOption == common.SyncOptionReplaceNever || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceNever)) +} + +func (sc *syncContext) shouldForceByDefault(t *syncTask) bool { + return (sc.force || + resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForce) || + sc.forceOption == common.SyncOptionForce || + sc.forceOption == common.SyncOptionForceAlways || + resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceAlways) || + sc.forceRequested && sc.replaceOption == common.SyncOptionForceIfRequested || + sc.forceRequested && resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfRequested)) && + !(sc.forceOption == common.SyncOptionForceNever || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceNever)) +} + +func (sc *syncContext) shouldRetryWithReplace(t *syncTask, err error) bool { + return strings.Contains(err.Error(), validation.FieldImmutableErrorMsg) && + (sc.replaceOption == common.SyncOptionReplaceIfImmutableFieldsUpdated || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfImmutableFieldsUpdated)) || + (sc.replaceOption == common.SyncOptionReplaceIfApplyFailed || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfApplyFailed)) +} + +func (sc *syncContext) shouldForceWhenRetrying(t *syncTask, err error, forceByDefault bool) bool { + return forceByDefault || + (strings.Contains(err.Error(), validation.FieldImmutableErrorMsg) && + (sc.forceOption == common.SyncOptionForceIfImmutableFieldsUpdated || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfImmutableFieldsUpdated)) || + (sc.forceOption == common.SyncOptionForceIfApplyFailed || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfApplyFailed))) +} + func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { dryRunStrategy := cmdutil.DryRunNone if dryRun { @@ -978,31 +1068,18 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R 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) + replaceByDefault := sc.shouldReplaceByDefault(t) + forceByDefault := sc.shouldForceByDefault(t) serverSideApply := sc.shouldUseServerSideApply(t.targetObj) - if shouldReplace { - if t.liveObj != nil { - // Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances. - // The same thing applies for namespaces, which would delete the namespace as well as everything within it, - // so we want to avoid using `kubectl replace` in that case as well. - if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind { - update := t.targetObj.DeepCopy() - update.SetResourceVersion(t.liveObj.GetResourceVersion()) - _, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy) - if err == nil { - message = fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName()) - } else { - message = fmt.Sprintf("error when updating: %v", err.Error()) - } - } else { - message, err = sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force) + if replaceByDefault { + message, err = sc.replaceObject(t, dryRunStrategy, forceByDefault, validate) + } else { + message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, forceByDefault, validate, serverSideApply, sc.serverSideApplyManager, false) + if err != nil { + if sc.shouldRetryWithReplace(t, err) { + message, err = sc.replaceObject(t, dryRunStrategy, sc.shouldForceWhenRetrying(t, err, forceByDefault), validate) } - } else { - message, err = sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate) } - } else { - message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager, false) } if err != nil { return common.ResultCodeSyncFailed, err.Error() diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 7e416d20b..2405bfb1f 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -753,16 +753,28 @@ func withReplaceAnnotation(un *unstructured.Unstructured) *unstructured.Unstruct return un } +func withReplaceOptionAnnotation(un *unstructured.Unstructured, option string) *unstructured.Unstructured { + un.SetAnnotations(map[string]string{synccommon.AnnotationSyncOptions: option}) + return un +} + func TestSync_Replace(t *testing.T) { testCases := []struct { name string target *unstructured.Unstructured live *unstructured.Unstructured + requested bool commandUsed string }{ - {"NoAnnotation", NewPod(), NewPod(), "apply"}, - {"AnnotationIsSet", withReplaceAnnotation(NewPod()), NewPod(), "replace"}, - {"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create"}, + {"NoAnnotation", NewPod(), NewPod(), false, "apply"}, + {"AnnotationIsSet", withReplaceAnnotation(NewPod()), NewPod(), false, "replace"}, + {"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, false, "create"}, + {"AnnotationAlways", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceAlways), NewPod(), false, "replace"}, + {"AnnotationNever", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceNever), NewPod(), false, "apply"}, + {"AnnotationIfRequestedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfRequested), NewPod(), false, "apply"}, + {"AnnotationIfRequestedTrue", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfRequested), NewPod(), true, "replace"}, + {"AnnotationIfApplyFailedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfApplyFailed), NewPod(), false, "apply"}, + {"AnnotationIfImmutableFieldsUpdatedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfImmutableFieldsUpdated), NewPod(), false, "apply"}, } for _, tc := range testCases { @@ -777,6 +789,7 @@ func TestSync_Replace(t *testing.T) { Live: []*unstructured.Unstructured{tc.live}, Target: []*unstructured.Unstructured{tc.target}, }) + syncCtx.replaceRequested = tc.requested syncCtx.Sync() @@ -862,12 +875,19 @@ func TestSync_Force(t *testing.T) { target *unstructured.Unstructured live *unstructured.Unstructured commandUsed string + requested bool force bool }{ - {"NoAnnotation", NewPod(), NewPod(), "apply", false}, - {"ForceApplyAnnotationIsSet", withForceAnnotation(NewPod()), NewPod(), "apply", true}, - {"ForceReplaceAnnotationIsSet", withForceAndReplaceAnnotations(NewPod()), NewPod(), "replace", true}, - {"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create", false}, + {"NoAnnotation", NewPod(), NewPod(), "apply", false, false}, + {"ForceApplyAnnotationIsSet", withForceAnnotation(NewPod()), NewPod(), "apply", false, true}, + {"ForceReplaceAnnotationIsSet", withForceAndReplaceAnnotations(NewPod()), NewPod(), "replace", false, true}, + {"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create", false, false}, + {"AnnotationAlways", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceAlways), NewPod(), "apply", false, true}, + {"AnnotationNever", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceNever), NewPod(), "apply", false, false}, + {"AnnotationIfRequestedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfRequested), NewPod(), "apply", false, false}, + {"AnnotationIfRequestedTrue", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfRequested), NewPod(), "apply", true, true}, + {"AnnotationIfApplyFailedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfApplyFailed), NewPod(), "apply", false, false}, + {"AnnotationIfImmutableFieldsUpdatedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfImmutableFieldsUpdated), NewPod(), "apply", false, false}, } for _, tc := range testCases { @@ -882,6 +902,7 @@ func TestSync_Force(t *testing.T) { Live: []*unstructured.Unstructured{tc.live}, Target: []*unstructured.Unstructured{tc.target}, }) + syncCtx.forceRequested = tc.requested syncCtx.Sync()