🐛 Fix sorting of v1beta2 conditions when patching

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)
 }

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
 }

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,
 		},
 	}

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))
 			}
 		}
 	}

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))
 		})