Skip to content

Commit 855b7b5

Browse files
andrii-korotkov-verkadaandrii-korotkov-verkada
committed
feat: New sync flags for replace and force
Gitops-engine part of argoproj/argo-cd#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 <[email protected]>
1 parent 8d65e80 commit 855b7b5

File tree

3 files changed

+151
-29
lines changed

3 files changed

+151
-29
lines changed

‎pkg/sync/common/types.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,32 @@ const (
2828
SyncOptionPruneLast = "PruneLast=true"
2929
// Sync option that enables use of replace or create command instead of apply
3030
SyncOptionReplace = "Replace=true"
31+
// Sync option that disables use of replace or create command instead of apply
32+
SyncOptionReplaceFalse = "Replace=false"
33+
// Sync option that enables use of replace or create command instead of apply
34+
SyncOptionReplaceAlways = "Replace=always"
35+
// Sync option that disables use of replace or create command instead of apply
36+
SyncOptionReplaceNever = "Replace=never"
37+
// Sync option that enables use of replace or create command instead of apply if API requested replace
38+
SyncOptionReplaceIfRequested = "Replace=if-requested"
39+
// Sync option that enables use of replace or create command instead of apply if immutable fields were updated
40+
SyncOptionReplaceIfImmutableFieldsUpdated = "Replace=if-immutable-fields-updated"
41+
// Sync option that enables use of replace or create command instead of apply if apply failed
42+
SyncOptionReplaceIfApplyFailed = "Replace=if-apply-failed"
3143
// Sync option that enables use of --force flag, delete and re-create
3244
SyncOptionForce = "Force=true"
45+
// Sync option that disables use of --force flag, delete and re-create
46+
SyncOptionForceFalse = "Force=false"
47+
// Sync option that enables use of --force flag, delete and re-create
48+
SyncOptionForceAlways = "Force=always"
49+
// Sync option that disables use of --force flag, delete and re-create
50+
SyncOptionForceNever = "Force=never"
51+
// Sync option that enables use of --force flag, delete and re-create if API requested replace
52+
SyncOptionForceIfRequested = "Force=if-requested"
53+
// Sync option that enables use of --force flag, delete and re-createif immutable fields were updated
54+
SyncOptionForceIfImmutableFieldsUpdated = "Force=if-immutable-fields-updated"
55+
// Sync option that enables use of --force flag, delete and re-create if apply failed
56+
SyncOptionForceIfApplyFailed = "Force=if-apply-failed"
3357
// Sync option that enables use of --server-side flag instead of client-side
3458
SyncOptionServerSideApply = "ServerSideApply=true"
3559
// Sync option that disables use of --server-side flag instead of client-side

‎pkg/sync/sync_context.go

Lines changed: 99 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
v1extensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1515
"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
1616
apierr "k8s.io/apimachinery/pkg/api/errors"
17+
"k8s.io/apimachinery/pkg/api/validation"
1718
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1819
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1920
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -23,6 +24,7 @@ import (
2324
"k8s.io/client-go/rest"
2425
"k8s.io/client-go/util/retry"
2526
"k8s.io/klog/v2/textlogger"
27+
"k8s.io/kubectl/pkg/cmd/util"
2628
cmdutil "k8s.io/kubectl/pkg/cmd/util"
2729
"k8s.io/kubectl/pkg/util/openapi"
2830

@@ -123,7 +125,15 @@ func WithPruneConfirmed(confirmed bool) SyncOpt {
123125
}
124126
}
125127

128+
// WithDryRun sets dry run setting
129+
func WithDryRun(dryRun bool) SyncOpt {
130+
return func(ctx *syncContext) {
131+
ctx.dryRun = dryRun
132+
}
133+
}
134+
126135
// WithOperationSettings allows to set sync operation settings
136+
// Deprecated, use individual setters
127137
func WithOperationSettings(dryRun bool, prune bool, force bool, skipHooks bool) SyncOpt {
128138
return func(ctx *syncContext) {
129139
ctx.prune = prune
@@ -183,12 +193,30 @@ func WithSyncWaveHook(syncWaveHook common.SyncWaveHook) SyncOpt {
183193
}
184194
}
185195

196+
// WithReplace sets a replace to a given value
197+
// Deprecated, prefer using WithReplaceOption
186198
func WithReplace(replace bool) SyncOpt {
187199
return func(ctx *syncContext) {
188200
ctx.replace = replace
189201
}
190202
}
191203

204+
// WithReplaceOptions sets replace options
205+
func WithReplaceOptions(replaceOption string, replaceRequested bool) SyncOpt {
206+
return func(ctx *syncContext) {
207+
ctx.replaceOption = replaceOption
208+
ctx.replaceRequested = replaceRequested
209+
}
210+
}
211+
212+
// WithReplaceOption sets force options
213+
func WithForceOptions(forceOption string, forceRequested bool) SyncOpt {
214+
return func(ctx *syncContext) {
215+
ctx.forceOption = forceOption
216+
ctx.forceRequested = forceRequested
217+
}
218+
}
219+
192220
func WithServerSideApply(serverSideApply bool) SyncOpt {
193221
return func(ctx *syncContext) {
194222
ctx.serverSideApply = serverSideApply
@@ -337,11 +365,15 @@ type syncContext struct {
337365

338366
dryRun bool
339367
force bool
368+
forceOption string
369+
forceRequested bool
340370
validate bool
341371
skipHooks bool
342372
resourcesFilter func(key kube.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool
343373
prune bool
344374
replace bool
375+
replaceOption string
376+
replaceRequested bool
345377
serverSideApply bool
346378
serverSideApplyManager string
347379
pruneLast bool
@@ -965,6 +997,64 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct
965997
return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply)
966998
}
967999

1000+
func (sc *syncContext) replaceObject(t *syncTask, dryRunStrategy util.DryRunStrategy, force bool, validate bool) (message string, err error) {
1001+
if t.liveObj != nil {
1002+
// Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
1003+
// The same thing applies for namespaces, which would delete the namespace as well as everything within it,
1004+
// so we want to avoid using `kubectl replace` in that case as well.
1005+
if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind {
1006+
update := t.targetObj.DeepCopy()
1007+
update.SetResourceVersion(t.liveObj.GetResourceVersion())
1008+
_, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy)
1009+
if err == nil {
1010+
message = fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName())
1011+
} else {
1012+
message = fmt.Sprintf("error when updating: %v", err.Error())
1013+
}
1014+
} else {
1015+
message, err = sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force)
1016+
}
1017+
} else {
1018+
message, err = sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate)
1019+
}
1020+
return message, err
1021+
}
1022+
1023+
func (sc *syncContext) shouldReplaceByDefault(t *syncTask) bool {
1024+
return (sc.replace ||
1025+
resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace) ||
1026+
sc.replaceOption == common.SyncOptionReplace ||
1027+
sc.replaceOption == common.SyncOptionReplaceAlways ||
1028+
resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceAlways) ||
1029+
sc.replaceRequested && sc.replaceOption == common.SyncOptionReplaceIfRequested ||
1030+
sc.replaceRequested && resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfRequested)) &&
1031+
!(sc.replaceOption == common.SyncOptionReplaceNever || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceNever))
1032+
}
1033+
1034+
func (sc *syncContext) shouldForceByDefault(t *syncTask) bool {
1035+
return (sc.force ||
1036+
resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForce) ||
1037+
sc.forceOption == common.SyncOptionForce ||
1038+
sc.forceOption == common.SyncOptionForceAlways ||
1039+
resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceAlways) ||
1040+
sc.forceRequested && sc.replaceOption == common.SyncOptionForceIfRequested ||
1041+
sc.forceRequested && resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfRequested)) &&
1042+
!(sc.forceOption == common.SyncOptionForceNever || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceNever))
1043+
}
1044+
1045+
func (sc *syncContext) shouldRetryWithReplace(t *syncTask, err error) bool {
1046+
return strings.Contains(err.Error(), validation.FieldImmutableErrorMsg) &&
1047+
(sc.replaceOption == common.SyncOptionReplaceIfImmutableFieldsUpdated || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfImmutableFieldsUpdated)) ||
1048+
(sc.replaceOption == common.SyncOptionReplaceIfApplyFailed || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplaceIfApplyFailed))
1049+
}
1050+
1051+
func (sc *syncContext) shouldForceWhenRetrying(t *syncTask, err error, forceByDefault bool) bool {
1052+
return forceByDefault ||
1053+
(strings.Contains(err.Error(), validation.FieldImmutableErrorMsg) &&
1054+
(sc.forceOption == common.SyncOptionForceIfImmutableFieldsUpdated || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfImmutableFieldsUpdated)) ||
1055+
(sc.forceOption == common.SyncOptionForceIfApplyFailed || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForceIfApplyFailed)))
1056+
}
1057+
9681058
func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) {
9691059
dryRunStrategy := cmdutil.DryRunNone
9701060
if dryRun {
@@ -978,31 +1068,18 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R
9781068

9791069
var err error
9801070
var message string
981-
shouldReplace := sc.replace || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionReplace)
982-
force := sc.force || resourceutil.HasAnnotationOption(t.targetObj, common.AnnotationSyncOptions, common.SyncOptionForce)
1071+
replaceByDefault := sc.shouldReplaceByDefault(t)
1072+
forceByDefault := sc.shouldForceByDefault(t)
9831073
serverSideApply := sc.shouldUseServerSideApply(t.targetObj)
984-
if shouldReplace {
985-
if t.liveObj != nil {
986-
// Avoid using `kubectl replace` for CRDs since 'replace' might recreate resource and so delete all CRD instances.
987-
// The same thing applies for namespaces, which would delete the namespace as well as everything within it,
988-
// so we want to avoid using `kubectl replace` in that case as well.
989-
if kube.IsCRD(t.targetObj) || t.targetObj.GetKind() == kubeutil.NamespaceKind {
990-
update := t.targetObj.DeepCopy()
991-
update.SetResourceVersion(t.liveObj.GetResourceVersion())
992-
_, err = sc.resourceOps.UpdateResource(context.TODO(), update, dryRunStrategy)
993-
if err == nil {
994-
message = fmt.Sprintf("%s/%s updated", t.targetObj.GetKind(), t.targetObj.GetName())
995-
} else {
996-
message = fmt.Sprintf("error when updating: %v", err.Error())
997-
}
998-
} else {
999-
message, err = sc.resourceOps.ReplaceResource(context.TODO(), t.targetObj, dryRunStrategy, force)
1074+
if replaceByDefault {
1075+
message, err = sc.replaceObject(t, dryRunStrategy, forceByDefault, validate)
1076+
} else {
1077+
message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, forceByDefault, validate, serverSideApply, sc.serverSideApplyManager, false)
1078+
if err != nil {
1079+
if sc.shouldRetryWithReplace(t, err) {
1080+
message, err = sc.replaceObject(t, dryRunStrategy, sc.shouldForceWhenRetrying(t, err, forceByDefault), validate)
10001081
}
1001-
} else {
1002-
message, err = sc.resourceOps.CreateResource(context.TODO(), t.targetObj, dryRunStrategy, validate)
10031082
}
1004-
} else {
1005-
message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager, false)
10061083
}
10071084
if err != nil {
10081085
return common.ResultCodeSyncFailed, err.Error()

‎pkg/sync/sync_context_test.go

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -753,16 +753,28 @@ func withReplaceAnnotation(un *unstructured.Unstructured) *unstructured.Unstruct
753753
return un
754754
}
755755

756+
func withReplaceOptionAnnotation(un *unstructured.Unstructured, option string) *unstructured.Unstructured {
757+
un.SetAnnotations(map[string]string{synccommon.AnnotationSyncOptions: option})
758+
return un
759+
}
760+
756761
func TestSync_Replace(t *testing.T) {
757762
testCases := []struct {
758763
name string
759764
target *unstructured.Unstructured
760765
live *unstructured.Unstructured
766+
requested bool
761767
commandUsed string
762768
}{
763-
{"NoAnnotation", NewPod(), NewPod(), "apply"},
764-
{"AnnotationIsSet", withReplaceAnnotation(NewPod()), NewPod(), "replace"},
765-
{"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create"},
769+
{"NoAnnotation", NewPod(), NewPod(), false, "apply"},
770+
{"AnnotationIsSet", withReplaceAnnotation(NewPod()), NewPod(), false, "replace"},
771+
{"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, false, "create"},
772+
{"AnnotationAlways", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceAlways), NewPod(), false, "replace"},
773+
{"AnnotationNever", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceNever), NewPod(), false, "apply"},
774+
{"AnnotationIfRequestedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfRequested), NewPod(), false, "apply"},
775+
{"AnnotationIfRequestedTrue", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfRequested), NewPod(), true, "replace"},
776+
{"AnnotationIfApplyFailedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfApplyFailed), NewPod(), false, "apply"},
777+
{"AnnotationIfImmutableFieldsUpdatedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionReplaceIfImmutableFieldsUpdated), NewPod(), false, "apply"},
766778
}
767779

768780
for _, tc := range testCases {
@@ -777,6 +789,7 @@ func TestSync_Replace(t *testing.T) {
777789
Live: []*unstructured.Unstructured{tc.live},
778790
Target: []*unstructured.Unstructured{tc.target},
779791
})
792+
syncCtx.replaceRequested = tc.requested
780793

781794
syncCtx.Sync()
782795

@@ -862,12 +875,19 @@ func TestSync_Force(t *testing.T) {
862875
target *unstructured.Unstructured
863876
live *unstructured.Unstructured
864877
commandUsed string
878+
requested bool
865879
force bool
866880
}{
867-
{"NoAnnotation", NewPod(), NewPod(), "apply", false},
868-
{"ForceApplyAnnotationIsSet", withForceAnnotation(NewPod()), NewPod(), "apply", true},
869-
{"ForceReplaceAnnotationIsSet", withForceAndReplaceAnnotations(NewPod()), NewPod(), "replace", true},
870-
{"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create", false},
881+
{"NoAnnotation", NewPod(), NewPod(), "apply", false, false},
882+
{"ForceApplyAnnotationIsSet", withForceAnnotation(NewPod()), NewPod(), "apply", false, true},
883+
{"ForceReplaceAnnotationIsSet", withForceAndReplaceAnnotations(NewPod()), NewPod(), "replace", false, true},
884+
{"LiveObjectMissing", withReplaceAnnotation(NewPod()), nil, "create", false, false},
885+
{"AnnotationAlways", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceAlways), NewPod(), "apply", false, true},
886+
{"AnnotationNever", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceNever), NewPod(), "apply", false, false},
887+
{"AnnotationIfRequestedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfRequested), NewPod(), "apply", false, false},
888+
{"AnnotationIfRequestedTrue", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfRequested), NewPod(), "apply", true, true},
889+
{"AnnotationIfApplyFailedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfApplyFailed), NewPod(), "apply", false, false},
890+
{"AnnotationIfImmutableFieldsUpdatedFalse", withReplaceOptionAnnotation(NewPod(), synccommon.SyncOptionForceIfImmutableFieldsUpdated), NewPod(), "apply", false, false},
871891
}
872892

873893
for _, tc := range testCases {
@@ -882,6 +902,7 @@ func TestSync_Force(t *testing.T) {
882902
Live: []*unstructured.Unstructured{tc.live},
883903
Target: []*unstructured.Unstructured{tc.target},
884904
})
905+
syncCtx.forceRequested = tc.requested
885906

886907
syncCtx.Sync()
887908

0 commit comments

Comments
 (0)