Skip to content

Commit ef64625

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #11326 from fabriziopandini/fix-v1beta2-condition-sorting
🐛 Fix sorting of v1beta2 conditions when patching
2 parents cf5979e + a1d751c commit ef64625

File tree

5 files changed

+120
-35
lines changed

5 files changed

+120
-35
lines changed

‎util/conditions/v1beta2/options.go

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ func (f ConditionSortFunc) ApplyToSet(opts *SetOptions) {
2626
opts.conditionSortFunc = f
2727
}
2828

29+
// ApplyToPatchApply applies this configuration to the given patch apply options.
30+
func (f ConditionSortFunc) ApplyToPatchApply(opts *PatchApplyOptions) {
31+
opts.conditionSortFunc = f
32+
}
33+
2934
// TargetConditionType allows to specify the type of new mirror or aggregate conditions.
3035
type TargetConditionType string
3136

@@ -105,15 +110,17 @@ func (t CustomMergeStrategy) ApplyToAggregate(opts *AggregateOptions) {
105110

106111
// OwnedConditionTypes allows to define condition types owned by the controller when performing patch apply.
107112
// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
108-
func OwnedConditionTypes(conditionTypes ...string) ApplyOption {
109-
return func(c *applyOptions) {
110-
c.ownedConditionTypes = conditionTypes
111-
}
113+
type OwnedConditionTypes []string
114+
115+
// ApplyToPatchApply applies this configuration to the given patch apply options.
116+
func (o OwnedConditionTypes) ApplyToPatchApply(opts *PatchApplyOptions) {
117+
opts.ownedConditionTypes = o
112118
}
113119

114120
// ForceOverwrite instructs patch apply to always use the value provided by the controller (no matter of what value exists currently).
115-
func ForceOverwrite(v bool) ApplyOption {
116-
return func(c *applyOptions) {
117-
c.forceOverwrite = v
118-
}
121+
type ForceOverwrite bool
122+
123+
// ApplyToPatchApply applies this configuration to the given patch apply options.
124+
func (f ForceOverwrite) ApplyToPatchApply(opts *PatchApplyOptions) {
125+
opts.forceOverwrite = bool(f)
119126
}

‎util/conditions/v1beta2/patch.go

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package v1beta2
1818

1919
import (
2020
"reflect"
21+
"sort"
2122

2223
"github.com/google/go-cmp/cmp"
2324
"github.com/pkg/errors"
@@ -90,13 +91,32 @@ func NewPatch(before, after Getter) (Patch, error) {
9091
return patch, nil
9192
}
9293

93-
// applyOptions allows to set strategies for patch apply.
94-
type applyOptions struct {
94+
// PatchApplyOption is some configuration that modifies options for a patch apply call.
95+
type PatchApplyOption interface {
96+
// ApplyToPatchApply applies this configuration to the given patch apply options.
97+
ApplyToPatchApply(option *PatchApplyOptions)
98+
}
99+
100+
// PatchApplyOptions allows to set strategies for patch apply.
101+
type PatchApplyOptions struct {
95102
ownedConditionTypes []string
96103
forceOverwrite bool
104+
conditionSortFunc ConditionSortFunc
97105
}
98106

99-
func (o *applyOptions) isOwnedConditionType(conditionType string) bool {
107+
// ApplyOptions applies the given list options on these options,
108+
// and then returns itself (for convenient chaining).
109+
func (o *PatchApplyOptions) ApplyOptions(opts []PatchApplyOption) *PatchApplyOptions {
110+
for _, opt := range opts {
111+
if util.IsNil(opt) {
112+
continue
113+
}
114+
opt.ApplyToPatchApply(o)
115+
}
116+
return o
117+
}
118+
119+
func (o *PatchApplyOptions) isOwnedConditionType(conditionType string) bool {
100120
for _, i := range o.ownedConditionTypes {
101121
if i == conditionType {
102122
return true
@@ -105,12 +125,9 @@ func (o *applyOptions) isOwnedConditionType(conditionType string) bool {
105125
return false
106126
}
107127

108-
// ApplyOption defines an option for applying a condition patch.
109-
type ApplyOption func(*applyOptions)
110-
111128
// Apply executes a three-way merge of a list of Patch.
112129
// When merge conflicts are detected (latest deviated from before in an incompatible way), an error is returned.
113-
func (p Patch) Apply(latest Setter, options ...ApplyOption) error {
130+
func (p Patch) Apply(latest Setter, opts ...PatchApplyOption) error {
114131
if p.IsZero() {
115132
return nil
116133
}
@@ -120,13 +137,11 @@ func (p Patch) Apply(latest Setter, options ...ApplyOption) error {
120137
}
121138
latestConditions := latest.GetV1Beta2Conditions()
122139

123-
applyOpt := &applyOptions{}
124-
for _, o := range options {
125-
if util.IsNil(o) {
126-
return errors.New("error patching conditions: ApplyOption is nil")
127-
}
128-
o(applyOpt)
140+
applyOpt := &PatchApplyOptions{
141+
// 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.
142+
conditionSortFunc: defaultSortLessFunc,
129143
}
144+
applyOpt.ApplyOptions(opts)
130145

131146
for _, conditionPatch := range p {
132147
switch conditionPatch.Op {
@@ -201,6 +216,12 @@ func (p Patch) Apply(latest Setter, options ...ApplyOption) error {
201216
}
202217
}
203218

219+
if applyOpt.conditionSortFunc != nil {
220+
sort.SliceStable(latestConditions, func(i, j int) bool {
221+
return applyOpt.conditionSortFunc(latestConditions[i], latestConditions[j])
222+
})
223+
}
224+
204225
latest.SetV1Beta2Conditions(latestConditions)
205226
return nil
206227
}

‎util/conditions/v1beta2/patch_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func TestApply(t *testing.T) {
138138
before Setter
139139
after Setter
140140
latest Setter
141-
options []ApplyOption
141+
options []PatchApplyOption
142142
want []metav1.Condition
143143
wantErr bool
144144
}{
@@ -195,7 +195,7 @@ func TestApply(t *testing.T) {
195195
before: objectWithConditions(),
196196
after: objectWithConditions(fooTrue),
197197
latest: objectWithConditions(fooFalse),
198-
options: []ApplyOption{ForceOverwrite(true)},
198+
options: []PatchApplyOption{ForceOverwrite(true)},
199199
want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error
200200
wantErr: false,
201201
},
@@ -204,7 +204,7 @@ func TestApply(t *testing.T) {
204204
before: objectWithConditions(),
205205
after: objectWithConditions(fooTrue),
206206
latest: objectWithConditions(fooFalse),
207-
options: []ApplyOption{OwnedConditionTypes("foo")},
207+
options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
208208
want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error
209209
wantErr: false,
210210
},
@@ -237,7 +237,7 @@ func TestApply(t *testing.T) {
237237
before: objectWithConditions(fooTrue),
238238
after: objectWithConditions(),
239239
latest: objectWithConditions(fooFalse),
240-
options: []ApplyOption{ForceOverwrite(true)},
240+
options: []PatchApplyOption{ForceOverwrite(true)},
241241
want: []metav1.Condition{},
242242
wantErr: false,
243243
},
@@ -246,7 +246,7 @@ func TestApply(t *testing.T) {
246246
before: objectWithConditions(fooTrue),
247247
after: objectWithConditions(),
248248
latest: objectWithConditions(fooFalse),
249-
options: []ApplyOption{OwnedConditionTypes("foo")},
249+
options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
250250
want: []metav1.Condition{},
251251
wantErr: false,
252252
},
@@ -279,7 +279,7 @@ func TestApply(t *testing.T) {
279279
before: objectWithConditions(fooFalse),
280280
after: objectWithConditions(fooFalse2),
281281
latest: objectWithConditions(fooTrue),
282-
options: []ApplyOption{ForceOverwrite(true)},
282+
options: []PatchApplyOption{ForceOverwrite(true)},
283283
want: []metav1.Condition{fooFalse2},
284284
wantErr: false,
285285
},
@@ -288,7 +288,7 @@ func TestApply(t *testing.T) {
288288
before: objectWithConditions(fooFalse),
289289
after: objectWithConditions(fooFalse2),
290290
latest: objectWithConditions(fooTrue),
291-
options: []ApplyOption{OwnedConditionTypes("foo")},
291+
options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
292292
want: []metav1.Condition{fooFalse2},
293293
wantErr: false,
294294
},
@@ -305,7 +305,7 @@ func TestApply(t *testing.T) {
305305
before: objectWithConditions(fooTrue),
306306
after: objectWithConditions(fooFalse),
307307
latest: objectWithConditions(),
308-
options: []ApplyOption{ForceOverwrite(true)},
308+
options: []PatchApplyOption{ForceOverwrite(true)},
309309
want: []metav1.Condition{fooFalse},
310310
wantErr: false,
311311
},
@@ -314,7 +314,7 @@ func TestApply(t *testing.T) {
314314
before: objectWithConditions(fooTrue),
315315
after: objectWithConditions(fooFalse),
316316
latest: objectWithConditions(),
317-
options: []ApplyOption{OwnedConditionTypes("foo")},
317+
options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
318318
want: []metav1.Condition{fooFalse},
319319
wantErr: false,
320320
},
@@ -323,7 +323,7 @@ func TestApply(t *testing.T) {
323323
before: objectWithConditions(fooTrue),
324324
after: objectWithConditions(fooFalse),
325325
latest: objectWithConditions(),
326-
options: []ApplyOption{nil},
326+
options: []PatchApplyOption{nil},
327327
wantErr: true,
328328
},
329329
}

‎util/patch/patch.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
297297
return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, klog.KObj(latest))
298298
}
299299

300-
return diff.Apply(latestSetter, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...))
300+
return diff.Apply(latestSetter, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions))
301301
}
302302
}
303303
}

‎util/patch/patch_test.go

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1378,7 +1378,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
13781378
},
13791379
}
13801380

1381-
t.Run("should mark it ready", func(t *testing.T) {
1381+
t.Run("should mark it ready and sort conditions", func(t *testing.T) {
13821382
g := NewWithT(t)
13831383

13841384
obj := obj.DeepCopy()
@@ -1397,6 +1397,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
13971397
return env.Get(ctx, key, obj)
13981398
}).Should(Succeed())
13991399

1400+
// Adding Ready first
14001401
t.Log("Creating a new patch helper")
14011402
patcher, err := NewHelper(obj, env)
14021403
g.Expect(err).ToNot(HaveOccurred())
@@ -1408,6 +1409,17 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
14081409
t.Log("Patching the object")
14091410
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
14101411

1412+
// Adding Available as a second condition, but it should be sorted as first
1413+
t.Log("Creating a new patch helper")
1414+
patcher, err = NewHelper(obj, env)
1415+
g.Expect(err).ToNot(HaveOccurred())
1416+
1417+
t.Log("Marking metav1.conditions Available=True")
1418+
v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now})
1419+
1420+
t.Log("Patching the object")
1421+
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
1422+
14111423
t.Log("Validating the object has been updated")
14121424
g.Eventually(func() clusterv1.Conditions {
14131425
objAfter := obj.DeepCopy()
@@ -1421,6 +1433,13 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
14211433
if err := env.Get(ctx, key, objAfter); err != nil {
14221434
return nil
14231435
}
1436+
if len(objAfter.Status.V1Beta2.Conditions) != 2 {
1437+
return nil
1438+
}
1439+
if objAfter.Status.V1Beta2.Conditions[0].Type != "Available" || objAfter.Status.V1Beta2.Conditions[1].Type != "Ready" {
1440+
return nil
1441+
}
1442+
14241443
return objAfter.Status.V1Beta2.Conditions
14251444
}, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.V1Beta2.Conditions))
14261445
})
@@ -1866,7 +1885,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
18661885
},
18671886
}
18681887

1869-
t.Run("should mark it ready", func(t *testing.T) {
1888+
t.Run("should mark it ready and sort conditions", func(t *testing.T) {
18701889
g := NewWithT(t)
18711890

18721891
obj := obj.DeepCopy()
@@ -1885,6 +1904,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
18851904
return env.Get(ctx, key, obj)
18861905
}).Should(Succeed())
18871906

1907+
// Adding Ready first
18881908
t.Log("Creating a new patch helper")
18891909
patcher, err := NewHelper(obj, env)
18901910
g.Expect(err).ToNot(HaveOccurred())
@@ -1896,6 +1916,17 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
18961916
t.Log("Patching the object")
18971917
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
18981918

1919+
// Adding Available as a second condition, but it should be sorted as first
1920+
t.Log("Creating a new patch helper")
1921+
patcher, err = NewHelper(obj, env)
1922+
g.Expect(err).ToNot(HaveOccurred())
1923+
1924+
t.Log("Marking condition Available=True")
1925+
v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now})
1926+
1927+
t.Log("Patching the object")
1928+
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
1929+
18991930
t.Log("Validating the object has been updated")
19001931
g.Eventually(func() clusterv1.Conditions {
19011932
objAfter := obj.DeepCopy()
@@ -1909,6 +1940,13 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
19091940
if err := env.Get(ctx, key, objAfter); err != nil {
19101941
return nil
19111942
}
1943+
if len(objAfter.Status.Conditions) != 2 {
1944+
return nil
1945+
}
1946+
if objAfter.Status.Conditions[0].Type != "Available" || objAfter.Status.Conditions[1].Type != "Ready" {
1947+
return nil
1948+
}
1949+
19121950
return objAfter.Status.Conditions
19131951
}, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions))
19141952
})
@@ -2351,7 +2389,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
23512389
},
23522390
}
23532391

2354-
t.Run("should mark it ready", func(t *testing.T) {
2392+
t.Run("should mark it ready and sort conditions", func(t *testing.T) {
23552393
g := NewWithT(t)
23562394

23572395
obj := obj.DeepCopy()
@@ -2370,6 +2408,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
23702408
return env.Get(ctx, key, obj)
23712409
}).Should(Succeed())
23722410

2411+
// Adding Ready first
23732412
t.Log("Creating a new patch helper")
23742413
patcher, err := NewHelper(obj, env)
23752414
g.Expect(err).ToNot(HaveOccurred())
@@ -2380,12 +2419,30 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) {
23802419
t.Log("Patching the object")
23812420
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
23822421

2422+
// Adding Available as a second condition, but it should be sorted as first
2423+
t.Log("Creating a new patch helper")
2424+
patcher, err = NewHelper(obj, env)
2425+
g.Expect(err).ToNot(HaveOccurred())
2426+
2427+
t.Log("Marking condition Available=True")
2428+
v1beta2conditions.Set(obj, metav1.Condition{Type: "Available", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now})
2429+
2430+
t.Log("Patching the object")
2431+
g.Expect(patcher.Patch(ctx, obj)).To(Succeed())
2432+
23832433
t.Log("Validating the object has been updated")
23842434
g.Eventually(func() []metav1.Condition {
23852435
objAfter := obj.DeepCopy()
23862436
if err := env.Get(ctx, key, objAfter); err != nil {
23872437
return nil
23882438
}
2439+
if len(objAfter.Status.Conditions) != 2 {
2440+
return nil
2441+
}
2442+
if objAfter.Status.Conditions[0].Type != "Available" || objAfter.Status.Conditions[1].Type != "Ready" {
2443+
return nil
2444+
}
2445+
23892446
return objAfter.Status.Conditions
23902447
}, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions))
23912448
})

0 commit comments

Comments
 (0)