From a1d751c7e7f965db3cc12edc720e5e883c0402d4 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Tue, 22 Oct 2024 21:30:39 +0200 Subject: [PATCH] Fix sorting of v1beta2 conditions when patching --- util/conditions/v1beta2/options.go | 23 ++++++---- util/conditions/v1beta2/patch.go | 47 ++++++++++++++------ util/conditions/v1beta2/patch_test.go | 20 ++++----- util/patch/patch.go | 2 +- util/patch/patch_test.go | 63 +++++++++++++++++++++++++-- 5 files changed, 120 insertions(+), 35 deletions(-) diff --git a/util/conditions/v1beta2/options.go b/util/conditions/v1beta2/options.go index 98a9ea3c874e..7d02e1ab0716 100644 --- a/util/conditions/v1beta2/options.go +++ b/util/conditions/v1beta2/options.go @@ -26,6 +26,11 @@ func (f ConditionSortFunc) ApplyToSet(opts *SetOptions) { opts.conditionSortFunc = f } +// ApplyToPatchApply applies this configuration to the given patch apply options. +func (f ConditionSortFunc) ApplyToPatchApply(opts *PatchApplyOptions) { + opts.conditionSortFunc = f +} + // TargetConditionType allows to specify the type of new mirror or aggregate conditions. type TargetConditionType string @@ -105,15 +110,17 @@ func (t CustomMergeStrategy) ApplyToAggregate(opts *AggregateOptions) { // OwnedConditionTypes allows to define condition types owned by the controller when performing patch apply. // In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. -func OwnedConditionTypes(conditionTypes ...string) ApplyOption { - return func(c *applyOptions) { - c.ownedConditionTypes = conditionTypes - } +type OwnedConditionTypes []string + +// ApplyToPatchApply applies this configuration to the given patch apply options. +func (o OwnedConditionTypes) ApplyToPatchApply(opts *PatchApplyOptions) { + opts.ownedConditionTypes = o } // ForceOverwrite instructs patch apply to always use the value provided by the controller (no matter of what value exists currently). -func ForceOverwrite(v bool) ApplyOption { - return func(c *applyOptions) { - c.forceOverwrite = v - } +type ForceOverwrite bool + +// ApplyToPatchApply applies this configuration to the given patch apply options. +func (f ForceOverwrite) ApplyToPatchApply(opts *PatchApplyOptions) { + opts.forceOverwrite = bool(f) } diff --git a/util/conditions/v1beta2/patch.go b/util/conditions/v1beta2/patch.go index 19e6b7e6416d..99f17a52eb33 100644 --- a/util/conditions/v1beta2/patch.go +++ b/util/conditions/v1beta2/patch.go @@ -18,6 +18,7 @@ package v1beta2 import ( "reflect" + "sort" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" @@ -90,13 +91,32 @@ func NewPatch(before, after Getter) (Patch, error) { return patch, nil } -// applyOptions allows to set strategies for patch apply. -type applyOptions struct { +// PatchApplyOption is some configuration that modifies options for a patch apply call. +type PatchApplyOption interface { + // ApplyToPatchApply applies this configuration to the given patch apply options. + ApplyToPatchApply(option *PatchApplyOptions) +} + +// PatchApplyOptions allows to set strategies for patch apply. +type PatchApplyOptions struct { ownedConditionTypes []string forceOverwrite bool + conditionSortFunc ConditionSortFunc } -func (o *applyOptions) isOwnedConditionType(conditionType string) bool { +// ApplyOptions applies the given list options on these options, +// and then returns itself (for convenient chaining). +func (o *PatchApplyOptions) ApplyOptions(opts []PatchApplyOption) *PatchApplyOptions { + for _, opt := range opts { + if util.IsNil(opt) { + continue + } + opt.ApplyToPatchApply(o) + } + return o +} + +func (o *PatchApplyOptions) isOwnedConditionType(conditionType string) bool { for _, i := range o.ownedConditionTypes { if i == conditionType { return true @@ -105,12 +125,9 @@ func (o *applyOptions) isOwnedConditionType(conditionType string) bool { return false } -// ApplyOption defines an option for applying a condition patch. -type ApplyOption func(*applyOptions) - // Apply executes a three-way merge of a list of Patch. // When merge conflicts are detected (latest deviated from before in an incompatible way), an error is returned. -func (p Patch) Apply(latest Setter, options ...ApplyOption) error { +func (p Patch) Apply(latest Setter, opts ...PatchApplyOption) error { if p.IsZero() { return nil } @@ -120,13 +137,11 @@ func (p Patch) Apply(latest Setter, options ...ApplyOption) error { } latestConditions := latest.GetV1Beta2Conditions() - applyOpt := &applyOptions{} - for _, o := range options { - if util.IsNil(o) { - return errors.New("error patching conditions: ApplyOption is nil") - } - o(applyOpt) + applyOpt := &PatchApplyOptions{ + // By default, sort conditions by the default condition order: available and ready always first, deleting and paused always last, all the other conditions in alphabetical order. + conditionSortFunc: defaultSortLessFunc, } + applyOpt.ApplyOptions(opts) for _, conditionPatch := range p { switch conditionPatch.Op { @@ -201,6 +216,12 @@ func (p Patch) Apply(latest Setter, options ...ApplyOption) error { } } + if applyOpt.conditionSortFunc != nil { + sort.SliceStable(latestConditions, func(i, j int) bool { + return applyOpt.conditionSortFunc(latestConditions[i], latestConditions[j]) + }) + } + latest.SetV1Beta2Conditions(latestConditions) return nil } diff --git a/util/conditions/v1beta2/patch_test.go b/util/conditions/v1beta2/patch_test.go index aebfda081b14..d0b71458afea 100644 --- a/util/conditions/v1beta2/patch_test.go +++ b/util/conditions/v1beta2/patch_test.go @@ -138,7 +138,7 @@ func TestApply(t *testing.T) { before Setter after Setter latest Setter - options []ApplyOption + options []PatchApplyOption want []metav1.Condition wantErr bool }{ @@ -195,7 +195,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(), after: objectWithConditions(fooTrue), latest: objectWithConditions(fooFalse), - options: []ApplyOption{ForceOverwrite(true)}, + options: []PatchApplyOption{ForceOverwrite(true)}, want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error wantErr: false, }, @@ -204,7 +204,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(), after: objectWithConditions(fooTrue), latest: objectWithConditions(fooFalse), - options: []ApplyOption{OwnedConditionTypes("foo")}, + options: []PatchApplyOption{OwnedConditionTypes{"foo"}}, want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error wantErr: false, }, @@ -237,7 +237,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(), latest: objectWithConditions(fooFalse), - options: []ApplyOption{ForceOverwrite(true)}, + options: []PatchApplyOption{ForceOverwrite(true)}, want: []metav1.Condition{}, wantErr: false, }, @@ -246,7 +246,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(), latest: objectWithConditions(fooFalse), - options: []ApplyOption{OwnedConditionTypes("foo")}, + options: []PatchApplyOption{OwnedConditionTypes{"foo"}}, want: []metav1.Condition{}, wantErr: false, }, @@ -279,7 +279,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooFalse), after: objectWithConditions(fooFalse2), latest: objectWithConditions(fooTrue), - options: []ApplyOption{ForceOverwrite(true)}, + options: []PatchApplyOption{ForceOverwrite(true)}, want: []metav1.Condition{fooFalse2}, wantErr: false, }, @@ -288,7 +288,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooFalse), after: objectWithConditions(fooFalse2), latest: objectWithConditions(fooTrue), - options: []ApplyOption{OwnedConditionTypes("foo")}, + options: []PatchApplyOption{OwnedConditionTypes{"foo"}}, want: []metav1.Condition{fooFalse2}, wantErr: false, }, @@ -305,7 +305,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(fooFalse), latest: objectWithConditions(), - options: []ApplyOption{ForceOverwrite(true)}, + options: []PatchApplyOption{ForceOverwrite(true)}, want: []metav1.Condition{fooFalse}, wantErr: false, }, @@ -314,7 +314,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(fooFalse), latest: objectWithConditions(), - options: []ApplyOption{OwnedConditionTypes("foo")}, + options: []PatchApplyOption{OwnedConditionTypes{"foo"}}, want: []metav1.Condition{fooFalse}, wantErr: false, }, @@ -323,7 +323,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(fooFalse), latest: objectWithConditions(), - options: []ApplyOption{nil}, + options: []PatchApplyOption{nil}, wantErr: true, }, } diff --git a/util/patch/patch.go b/util/patch/patch.go index c2ffe01eef1d..ad8d0e531184 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -297,7 +297,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, klog.KObj(latest)) } - return diff.Apply(latestSetter, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...)) + return diff.Apply(latestSetter, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions)) } } } diff --git a/util/patch/patch_test.go b/util/patch/patch_test.go index 6dbc0d386c30..00d1910acb68 100644 --- a/util/patch/patch_test.go +++ b/util/patch/patch_test.go @@ -1378,7 +1378,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { }, } - t.Run("should mark it ready", func(t *testing.T) { + t.Run("should mark it ready and sort conditions", func(t *testing.T) { g := NewWithT(t) obj := obj.DeepCopy() @@ -1397,6 +1397,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return env.Get(ctx, key, obj) }).Should(Succeed()) + // Adding Ready first t.Log("Creating a new patch helper") patcher, err := NewHelper(obj, env) g.Expect(err).ToNot(HaveOccurred()) @@ -1408,6 +1409,17 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Patching the object") g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + // Adding Available as a second condition, but it should be sorted as first + t.Log("Creating a new patch helper") + patcher, err = NewHelper(obj, env) + g.Expect(err).ToNot(HaveOccurred()) + + t.Log("Marking metav1.conditions Available=True") + v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) + + t.Log("Patching the object") + g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + t.Log("Validating the object has been updated") g.Eventually(func() clusterv1.Conditions { objAfter := obj.DeepCopy() @@ -1421,6 +1433,13 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err := env.Get(ctx, key, objAfter); err != nil { return nil } + if len(objAfter.Status.V1Beta2.Conditions) != 2 { + return nil + } + if objAfter.Status.V1Beta2.Conditions[0].Type != "Available" || objAfter.Status.V1Beta2.Conditions[1].Type != "Ready" { + return nil + } + return objAfter.Status.V1Beta2.Conditions }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.V1Beta2.Conditions)) }) @@ -1866,7 +1885,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { }, } - t.Run("should mark it ready", func(t *testing.T) { + t.Run("should mark it ready and sort conditions", func(t *testing.T) { g := NewWithT(t) obj := obj.DeepCopy() @@ -1885,6 +1904,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return env.Get(ctx, key, obj) }).Should(Succeed()) + // Adding Ready first t.Log("Creating a new patch helper") patcher, err := NewHelper(obj, env) g.Expect(err).ToNot(HaveOccurred()) @@ -1896,6 +1916,17 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Patching the object") g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + // Adding Available as a second condition, but it should be sorted as first + t.Log("Creating a new patch helper") + patcher, err = NewHelper(obj, env) + g.Expect(err).ToNot(HaveOccurred()) + + t.Log("Marking condition Available=True") + v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) + + t.Log("Patching the object") + g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + t.Log("Validating the object has been updated") g.Eventually(func() clusterv1.Conditions { objAfter := obj.DeepCopy() @@ -1909,6 +1940,13 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err := env.Get(ctx, key, objAfter); err != nil { return nil } + if len(objAfter.Status.Conditions) != 2 { + return nil + } + if objAfter.Status.Conditions[0].Type != "Available" || objAfter.Status.Conditions[1].Type != "Ready" { + return nil + } + return objAfter.Status.Conditions }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions)) }) @@ -2351,7 +2389,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { }, } - t.Run("should mark it ready", func(t *testing.T) { + t.Run("should mark it ready and sort conditions", func(t *testing.T) { g := NewWithT(t) obj := obj.DeepCopy() @@ -2370,6 +2408,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return env.Get(ctx, key, obj) }).Should(Succeed()) + // Adding Ready first t.Log("Creating a new patch helper") patcher, err := NewHelper(obj, env) g.Expect(err).ToNot(HaveOccurred()) @@ -2380,12 +2419,30 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Patching the object") g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + // Adding Available as a second condition, but it should be sorted as first + t.Log("Creating a new patch helper") + patcher, err = NewHelper(obj, env) + g.Expect(err).ToNot(HaveOccurred()) + + t.Log("Marking condition Available=True") + v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) + + t.Log("Patching the object") + g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + t.Log("Validating the object has been updated") g.Eventually(func() []metav1.Condition { objAfter := obj.DeepCopy() if err := env.Get(ctx, key, objAfter); err != nil { return nil } + if len(objAfter.Status.Conditions) != 2 { + return nil + } + if objAfter.Status.Conditions[0].Type != "Available" || objAfter.Status.Conditions[1].Type != "Ready" { + return nil + } + return objAfter.Status.Conditions }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions)) })