From e257b59f3f4d6ab30fffd210c432b681aaab8d8d Mon Sep 17 00:00:00 2001 From: fabriziopandini <fpandini@vmware.com> Date: Mon, 16 Sep 2024 11:26:42 +0200 Subject: [PATCH] Address comments --- internal/test/builder/v1beta2_transition.go | 9 ++ util/conditions/v1beta2/options.go | 15 +++ util/conditions/v1beta2/patch.go | 33 ++--- util/conditions/v1beta2/patch_test.go | 37 ++--- util/conditions/v1beta2/setter.go | 10 +- util/conditions/v1beta2/setter_test.go | 43 +++++- util/patch/options.go | 12 +- util/patch/patch.go | 21 ++- util/patch/patch_test.go | 142 ++++++++++---------- util/patch/utils.go | 60 +++++++-- util/patch/utils_test.go | 38 ++++-- 11 files changed, 266 insertions(+), 154 deletions(-) diff --git a/internal/test/builder/v1beta2_transition.go b/internal/test/builder/v1beta2_transition.go index 1c838b8653ec..37ad1a677c28 100644 --- a/internal/test/builder/v1beta2_transition.go +++ b/internal/test/builder/v1beta2_transition.go @@ -207,11 +207,20 @@ type Phase2ObjStatusDeprecatedV1Beta1 struct { // GetConditions returns the set of conditions for this object. func (o *Phase2Obj) GetConditions() clusterv1.Conditions { + if o.Status.Deprecated == nil || o.Status.Deprecated.V1Beta1 == nil { + return nil + } return o.Status.Deprecated.V1Beta1.Conditions } // SetConditions sets the conditions on this object. func (o *Phase2Obj) SetConditions(conditions clusterv1.Conditions) { + if o.Status.Deprecated == nil && conditions != nil { + o.Status.Deprecated = &Phase2ObjStatusDeprecated{V1Beta1: &Phase2ObjStatusDeprecatedV1Beta1{}} + } + if o.Status.Deprecated.V1Beta1 == nil && conditions != nil { + o.Status.Deprecated.V1Beta1 = &Phase2ObjStatusDeprecatedV1Beta1{} + } o.Status.Deprecated.V1Beta1.Conditions = conditions } diff --git a/util/conditions/v1beta2/options.go b/util/conditions/v1beta2/options.go index 855c9c9de118..8ec2b7d81765 100644 --- a/util/conditions/v1beta2/options.go +++ b/util/conditions/v1beta2/options.go @@ -86,3 +86,18 @@ type StepCounter bool func (t StepCounter) ApplyToSummary(opts *SummaryOptions) { opts.stepCounter = bool(t) } + +// 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 + } +} + +// 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 + } +} diff --git a/util/conditions/v1beta2/patch.go b/util/conditions/v1beta2/patch.go index 85a21c919ec4..1be0f63f4d8f 100644 --- a/util/conditions/v1beta2/patch.go +++ b/util/conditions/v1beta2/patch.go @@ -99,13 +99,13 @@ func NewPatch(before, after runtime.Object) (Patch, error) { // applyOptions allows to set strategies for patch apply. type applyOptions struct { - ownedConditions []string - forceOverwrite bool + ownedConditionTypes []string + forceOverwrite bool } -func (o *applyOptions) isOwnedCondition(t string) bool { - for _, i := range o.ownedConditions { - if i == t { +func (o *applyOptions) isOwnedConditionType(conditionType string) bool { + for _, i := range o.ownedConditionTypes { + if i == conditionType { return true } } @@ -115,21 +115,6 @@ func (o *applyOptions) isOwnedCondition(t string) bool { // ApplyOption defines an option for applying a condition patch. type ApplyOption func(*applyOptions) -// WithOwnedConditions allows to define condition types owned by the controller. -// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. -func WithOwnedConditions(t ...string) ApplyOption { - return func(c *applyOptions) { - c.ownedConditions = t - } -} - -// WithForceOverwrite In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. -func WithForceOverwrite(v bool) ApplyOption { - return func(c *applyOptions) { - c.forceOverwrite = v - } -} - // 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 runtime.Object, options ...ApplyOption) error { @@ -157,14 +142,14 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error { switch conditionPatch.Op { case AddConditionPatch: // If the conditions is owned, always keep the after value. - if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.After.Type) { + if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) { meta.SetStatusCondition(&latestConditions, *conditionPatch.After) continue } // If the condition is already on latest, check if latest and after agree on the change; if not, this is a conflict. if latestCondition := meta.FindStatusCondition(latestConditions, conditionPatch.After.Type); latestCondition != nil { - // If latest and after agree on the change, then it is a conflict. + // If latest and after disagreeWithForceOverwrite on the change, then it is a conflict. if !hasSameState(latestCondition, conditionPatch.After) { return errors.Errorf("error patching conditions: The condition %q was modified by a different process and this caused a merge/AddCondition conflict: %v", conditionPatch.After.Type, cmp.Diff(latestCondition, conditionPatch.After)) } @@ -177,7 +162,7 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error { case ChangeConditionPatch: // If the conditions is owned, always keep the after value. - if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.After.Type) { + if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) { meta.SetStatusCondition(&latestConditions, *conditionPatch.After) continue } @@ -209,7 +194,7 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error { } // If the conditions is owned, always keep the after value (condition should be deleted). - if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.Before.Type) { + if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.Before.Type) { meta.RemoveStatusCondition(&latestConditions, conditionPatch.Before.Type) continue } diff --git a/util/conditions/v1beta2/patch_test.go b/util/conditions/v1beta2/patch_test.go index 23b9efb70513..38b61a8886d3 100644 --- a/util/conditions/v1beta2/patch_test.go +++ b/util/conditions/v1beta2/patch_test.go @@ -129,9 +129,10 @@ func TestNewPatch(t *testing.T) { } func TestApply(t *testing.T) { - fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue} - fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse} - fooFalse2 := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, Reason: "Something else"} + now := metav1.Now().Rfc3339Copy() + fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue, LastTransitionTime: now} + fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, LastTransitionTime: now} + fooFalse2 := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, Reason: "Something else", LastTransitionTime: now} tests := []struct { name string @@ -195,7 +196,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(), after: objectWithConditions(fooTrue), latest: objectWithConditions(fooFalse), - options: []ApplyOption{WithForceOverwrite(true)}, + options: []ApplyOption{ForceOverwrite(true)}, want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error wantErr: false, }, @@ -204,7 +205,7 @@ func TestApply(t *testing.T) { before: objectWithConditions(), after: objectWithConditions(fooTrue), latest: objectWithConditions(fooFalse), - options: []ApplyOption{WithOwnedConditions("foo")}, + options: []ApplyOption{OwnedConditionTypes("foo")}, want: []metav1.Condition{fooTrue}, // after condition should be kept in case of error wantErr: false, }, @@ -237,8 +238,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(), latest: objectWithConditions(fooFalse), - options: []ApplyOption{WithForceOverwrite(true)}, - want: []metav1.Condition{}, // after condition should be kept in case of error + options: []ApplyOption{ForceOverwrite(true)}, + want: []metav1.Condition{}, wantErr: false, }, { @@ -246,8 +247,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(), latest: objectWithConditions(fooFalse), - options: []ApplyOption{WithOwnedConditions("foo")}, - want: []metav1.Condition{}, // after condition should be kept in case of error + options: []ApplyOption{OwnedConditionTypes("foo")}, + want: []metav1.Condition{}, wantErr: false, }, { @@ -279,8 +280,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooFalse), after: objectWithConditions(fooFalse2), latest: objectWithConditions(fooTrue), - options: []ApplyOption{WithForceOverwrite(true)}, - want: []metav1.Condition{fooFalse2}, // after condition should be kept in case of error + options: []ApplyOption{ForceOverwrite(true)}, + want: []metav1.Condition{fooFalse2}, wantErr: false, }, { @@ -288,8 +289,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooFalse), after: objectWithConditions(fooFalse2), latest: objectWithConditions(fooTrue), - options: []ApplyOption{WithOwnedConditions("foo")}, - want: []metav1.Condition{fooFalse2}, // after condition should be kept in case of error + options: []ApplyOption{OwnedConditionTypes("foo")}, + want: []metav1.Condition{fooFalse2}, wantErr: false, }, { @@ -305,8 +306,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(fooFalse), latest: objectWithConditions(), - options: []ApplyOption{WithForceOverwrite(true)}, - want: []metav1.Condition{fooFalse}, // after condition should be kept in case of error + options: []ApplyOption{ForceOverwrite(true)}, + want: []metav1.Condition{fooFalse}, wantErr: false, }, { @@ -314,8 +315,8 @@ func TestApply(t *testing.T) { before: objectWithConditions(fooTrue), after: objectWithConditions(fooFalse), latest: objectWithConditions(), - options: []ApplyOption{WithOwnedConditions("foo")}, - want: []metav1.Condition{fooFalse}, // after condition should be kept in case of error + options: []ApplyOption{OwnedConditionTypes("foo")}, + want: []metav1.Condition{fooFalse}, wantErr: false, }, { @@ -345,7 +346,7 @@ func TestApply(t *testing.T) { gotConditions, err := GetAll(tt.latest) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(gotConditions).To(MatchConditions(tt.want, IgnoreLastTransitionTime(true))) + g.Expect(gotConditions).To(MatchConditions(tt.want)) }) } } diff --git a/util/conditions/v1beta2/setter.go b/util/conditions/v1beta2/setter.go index 39ea72a0cb59..dcafb8e5f0e2 100644 --- a/util/conditions/v1beta2/setter.go +++ b/util/conditions/v1beta2/setter.go @@ -152,8 +152,16 @@ func setToTypedObject(obj runtime.Object, conditions []metav1.Condition) error { } v1beta2Elem := v1beta2Field.Elem() + + // If v1beta2Elem is nil (not a valid value), initialize it only if the condition list is not nil. + // NOTE: this makes the UX simpler (the caller should not take care of initializing the v1beta2 field before calling set). if !v1beta2Elem.IsValid() { - return errors.Errorf("%s.status.v1beta2 must be a valid value (non zero value of its type)", ownerInfo) + if conditions == nil { + return nil + } + v1beta2Value := reflect.New(v1beta2Field.Type().Elem()) + v1beta2Field.Set(v1beta2Value) + v1beta2Elem = v1beta2Field.Elem() } if conditionField := v1beta2Elem.FieldByName("Conditions"); conditionField != (reflect.Value{}) { diff --git a/util/conditions/v1beta2/setter_test.go b/util/conditions/v1beta2/setter_test.go index e678cd085890..5cd0e82b9371 100644 --- a/util/conditions/v1beta2/setter_test.go +++ b/util/conditions/v1beta2/setter_test.go @@ -86,7 +86,48 @@ func TestSetAll(t *testing.T) { g.Expect(err).To(HaveOccurred()) // Can't set legacy conditions. }) - t.Run("v1beta1 object with both legacy and v1beta2 conditions", func(t *testing.T) { + t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 nil)", func(t *testing.T) { + g := NewWithT(t) + foo := &builder.Phase1Obj{ + Status: builder.Phase1ObjStatus{ + Conditions: clusterv1.Conditions{ + { + Type: "barCondition", + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + // V1Beta2 is nil, but it should be automatically initialized + }, + } + + conditions := cloneConditions() + err := SetAll(foo, conditions) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(foo.Status.V1Beta2.Conditions).To(Equal(conditions), cmp.Diff(foo.Status.V1Beta2.Conditions, conditions)) + }) + + t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 nil should remain nil if nil conditions is passed)", func(t *testing.T) { + g := NewWithT(t) + foo := &builder.Phase1Obj{ + Status: builder.Phase1ObjStatus{ + Conditions: clusterv1.Conditions{ + { + Type: "barCondition", + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + // V1Beta2 is nil, but it should be automatically initialized + }, + } + + err := SetAll(foo, nil) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(foo.Status.V1Beta2).To(BeNil()) + }) + + t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 not nil)", func(t *testing.T) { g := NewWithT(t) foo := &builder.Phase1Obj{ Status: builder.Phase1ObjStatus{ diff --git a/util/patch/options.go b/util/patch/options.go index 7086bb6404ef..640446681ed1 100644 --- a/util/patch/options.go +++ b/util/patch/options.go @@ -38,9 +38,9 @@ type HelperOptions struct { // In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. OwnedConditions []clusterv1.ConditionType - // OwnedV1Beta1Conditions defines condition types owned by the controller. + // OwnedV1Beta2Conditions defines condition types owned by the controller. // In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. - OwnedV1Beta1Conditions []string + OwnedV1Beta2Conditions []string } // WithForceOverwriteConditions allows the patch helper to overwrite conditions in case of conflicts. @@ -72,13 +72,13 @@ func (w WithOwnedConditions) ApplyToHelper(in *HelperOptions) { in.OwnedConditions = w.Conditions } -// WithOwnedV1Beta1Conditions allows to define condition types owned by the controller. +// WithOwnedV1Beta2Conditions allows to define condition types owned by the controller. // In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller. -type WithOwnedV1Beta1Conditions struct { +type WithOwnedV1Beta2Conditions struct { Conditions []string } // ApplyToHelper applies this configuration to the given HelperOptions. -func (w WithOwnedV1Beta1Conditions) ApplyToHelper(in *HelperOptions) { - in.OwnedV1Beta1Conditions = w.Conditions +func (w WithOwnedV1Beta2Conditions) ApplyToHelper(in *HelperOptions) { + in.OwnedV1Beta2Conditions = w.Conditions } diff --git a/util/patch/patch.go b/util/patch/patch.go index 2d9a061f8965..26a87a13882d 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -152,7 +152,7 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e // Given that we pass in metadata.resourceVersion to perform a 3-way-merge conflict resolution, // patching conditions first avoids an extra loop if spec or status patch succeeds first // given that causes the resourceVersion to mutate. - if err := h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions, options.OwnedV1Beta1Conditions); err != nil { + if err := h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions, options.OwnedV1Beta2Conditions); err != nil { errs = append(errs, err) } // Then proceed to patch the rest of the object. @@ -206,7 +206,7 @@ func (h *Helper) patchStatus(ctx context.Context, obj client.Object) error { // Condition changes are then applied to the latest version of the object, and if there are // no unresolvable conflicts, the patch is sent again. func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, forceOverwrite bool, ownedConditions []clusterv1.ConditionType, ownedV1beta2Conditions []string) error { - // Nothing to do if the object doesn't have conditions. + // Nothing to do if the object doesn't have conditions (doesn't have conditions identified as needing a special treatment). if len(h.clusterv1ConditionsFields) == 0 && len(h.metav1ConditionsFields) == 0 { return nil } @@ -220,26 +220,25 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f // interface any longer, although this shouldn't happen because we already check when creating the patcher. before, ok := h.beforeObject.(conditions.Getter) if !ok { - return errors.Errorf("object %s doesn't satisfy conditions.Getter, cannot patch", before.GetObjectKind()) + return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot patch", h.gvk.Kind, klog.KObj(before)) } after, ok := obj.(conditions.Getter) if !ok { - return errors.Errorf("object %s doesn't satisfy conditions.Getter, cannot patch", after.GetObjectKind()) + return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot compute patch", h.gvk.Kind, after.GetObjectKind()) } - // Store the diff from the before/after object, and return early if there are no changes. diff, err := conditions.NewPatch( before, after, ) if err != nil { - return errors.Wrapf(err, "object can not be patched") + return errors.Wrapf(err, "%s can not be patched", h.gvk.Kind) } if !diff.IsZero() { clusterv1ApplyPatch = func(latest client.Object) error { latestSetter, ok := latest.(conditions.Setter) if !ok { - return errors.Errorf("object %s doesn't satisfy conditions.Setter, cannot patch", before.GetObjectKind()) + return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, before.GetObjectKind()) } return diff.Apply(latestSetter, conditions.WithForceOverwrite(forceOverwrite), conditions.WithOwnedConditions(ownedConditions...)) @@ -250,22 +249,22 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f // If the object has metav1 conditions, create a function applying corresponding changes if any. var metav1ApplyPatch func(client.Object) error if len(h.metav1ConditionsFields) > 0 { - // Store the diff from the before/after object, and return early if there are no changes. diff, err := v1beta2conditions.NewPatch( h.beforeObject, obj, ) if err != nil { - return errors.Wrapf(err, "object can not be patched") + return errors.Wrapf(err, "%s can not be patched", h.gvk.Kind) } if !diff.IsZero() { metav1ApplyPatch = func(latest client.Object) error { - return diff.Apply(latest, v1beta2conditions.WithForceOverwrite(forceOverwrite), v1beta2conditions.WithOwnedConditions(ownedV1beta2Conditions...)) + return diff.Apply(latest, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...)) } } } + // No changes to apply, return early. if clusterv1ApplyPatch == nil && metav1ApplyPatch == nil { return nil } @@ -287,7 +286,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f return wait.ExponentialBackoff(backoff, func() (bool, error) { latest, ok := h.beforeObject.DeepCopyObject().(client.Object) if !ok { - return false, errors.Errorf("object %s doesn't satisfy conditions.Setter, cannot patch", latest.GetObjectKind()) + return false, errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot patch", h.gvk.Kind, latest.GetObjectKind()) } // Get a new copy of the object. diff --git a/util/patch/patch_test.go b/util/patch/patch_test.go index bc13b71e329b..49271930970d 100644 --- a/util/patch/patch_test.go +++ b/util/patch/patch_test.go @@ -237,7 +237,7 @@ func TestPatchHelper(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking Ready=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("TestCondition"), "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -299,7 +299,7 @@ func TestPatchHelper(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking Ready=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("TestCondition"), "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -367,7 +367,7 @@ func TestPatchHelper(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking ") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -422,7 +422,7 @@ func TestPatchHelper(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking ") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -471,7 +471,7 @@ func TestPatchHelper(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking ") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1001,6 +1001,8 @@ func TestNewHelperNil(t *testing.T) { } func TestPatchHelperForV1beta2Transition(t *testing.T) { + now := metav1.Now().Rfc3339Copy() + t.Run("Should patch conditions on a v1beta1 object with conditions (phase 0)", func(t *testing.T) { obj := &builder.Phase0Obj{ ObjectMeta: metav1.ObjectMeta{ @@ -1195,7 +1197,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking Ready=False") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1230,7 +1232,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { }, timeout).Should(BeTrue()) }) - t.Run("should not return an error if there is an unresolvable conflict but the conditions is owned by the controller", func(t *testing.T) { + t.Run("should not return an error if there is an unresolvable conflict but the condition is owned by the controller", func(t *testing.T) { g := NewWithT(t) obj := obj.DeepCopy() @@ -1250,7 +1252,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking Ready=False") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1299,7 +1301,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() - t.Log("Marking a custom condition to be false") + t.Log("Marking Ready=False") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1362,7 +1364,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking clusterv1.conditions and metav1.conditions Ready=True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1382,7 +1384,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return nil } return objAfter.Status.V1Beta2.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.V1Beta2.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.V1Beta2.Conditions)) }) t.Run("should recover if there is a resolvable conflict", func(t *testing.T) { @@ -1407,7 +1409,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking clusterv1.conditions and metav1.conditions Test=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("Test"), "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1420,7 +1422,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking clusterv1.conditions and metav1.conditions Ready=True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1468,7 +1470,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyV1Beta2After) + ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before).Match(*readyV1Beta2After) if err != nil || !ok { return false } @@ -1499,7 +1501,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking clusterv1.conditions and metav1.conditions Test=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("Test"), "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1514,7 +1516,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { obj.Spec.Foo = "foo" obj.Status.Bar = "bat" conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1550,7 +1552,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*testV1Beta2ConditionCopy, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*testV1Beta2ConditionAfter) + ok, err = v1beta2conditions.MatchCondition(*testV1Beta2ConditionCopy).Match(*testV1Beta2ConditionAfter) if err != nil || !ok { return false } @@ -1563,7 +1565,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyV1Beta2After) + ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before).Match(*readyV1Beta2After) if err != nil || !ok { return false } @@ -1641,7 +1643,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking a Ready condition to be false") - err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1653,20 +1655,20 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking condition Ready=True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") g.Expect(patcher.Patch(ctx, obj)).NotTo(Succeed()) t.Log("Validating the object has not been updated") - g.Eventually(func() []metav1.Condition { + g.Eventually(func() *builder.Phase1ObjStatusV1Beta2 { objAfter := obj.DeepCopy() if err := env.Get(ctx, key, objAfter); err != nil { return nil } - return objAfter.Status.V1Beta2.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(objCopy.Status.V1Beta2.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + return objAfter.Status.V1Beta2 + }, timeout).Should(Equal(objCopy.Status.V1Beta2)) }) t.Run("should not return an error if there is an unresolvable conflict but the conditions is owned by the controller", func(t *testing.T) { @@ -1691,7 +1693,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking a Ready clusterv1.condition and metav1.conditions to be false") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1704,11 +1706,11 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking Ready clusterv1.condition and metav1.conditions True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") - g.Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}}, WithOwnedV1Beta1Conditions{Conditions: []string{"Ready"}})).To(Succeed()) + g.Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}}, WithOwnedV1Beta2Conditions{Conditions: []string{"Ready"}})).To(Succeed()) t.Log("Validating the object has been updated") g.Eventually(func() bool { @@ -1732,7 +1734,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyV1Beta2After) + ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before).Match(*readyV1Beta2After) if err != nil || !ok { return false } @@ -1763,7 +1765,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking a Ready clusterv1.condition and metav1.conditions to be false") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1776,7 +1778,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking Ready clusterv1.condition and metav1.conditions True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1804,7 +1806,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyV1Beta2After) + ok, err = v1beta2conditions.MatchCondition(*readyV1Beta2Before).Match(*readyV1Beta2After) if err != nil || !ok { return false } @@ -1847,7 +1849,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking condition and back compatibility condition Ready=True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1867,7 +1869,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return nil } return objAfter.Status.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions)) }) t.Run("should recover if there is a resolvable conflict", func(t *testing.T) { @@ -1892,7 +1894,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking condition and back compatibility condition Test=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("Test"), "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1905,7 +1907,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking condition and back compatibility condition Ready=True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -1940,7 +1942,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*testConditionCopy, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*testConditionAfter) + ok, err = v1beta2conditions.MatchCondition(*testConditionCopy).Match(*testConditionAfter) if err != nil || !ok { return false } @@ -1953,7 +1955,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -1984,7 +1986,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking condition and back compatibility condition Test=False") conditions.MarkFalse(objCopy, clusterv1.ConditionType("Test"), "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -1999,7 +2001,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { obj.Spec.Foo = "foo" obj.Status.Bar = "bat" conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2035,7 +2037,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*testConditionCopy, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*testConditionAfter) + ok, err = v1beta2conditions.MatchCondition(*testConditionCopy).Match(*testConditionAfter) if err != nil || !ok { return false } @@ -2048,7 +2050,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2126,7 +2128,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking a Ready condition to be false") - err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2138,7 +2140,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking condition Ready=True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2151,7 +2153,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return nil } return objAfter.Status.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(objCopy.Status.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + }, timeout).Should(v1beta2conditions.MatchConditions(objCopy.Status.Conditions)) }) t.Run("should not return an error if there is an unresolvable conflict but the conditions is owned by the controller", func(t *testing.T) { @@ -2176,7 +2178,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking a Ready condition and back compatibility condition to be false") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2189,11 +2191,11 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking Ready condition and back compatibility condition True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") - g.Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}}, WithOwnedV1Beta1Conditions{Conditions: []string{"Ready"}})).To(Succeed()) + g.Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}}, WithOwnedV1Beta2Conditions{Conditions: []string{"Ready"}})).To(Succeed()) t.Log("Validating the object has been updated") g.Eventually(func() bool { @@ -2217,7 +2219,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2248,7 +2250,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking a Ready condition and back compatibility condition to be false") conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2261,7 +2263,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Marking Ready condition and back compatibility condition True") conditions.MarkTrue(obj, clusterv1.ReadyCondition) - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2289,7 +2291,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2331,7 +2333,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking condition Ready=True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2344,7 +2346,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return nil } return objAfter.Status.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + }, timeout).Should(v1beta2conditions.MatchConditions(obj.Status.Conditions)) }) t.Run("should recover if there is a resolvable conflict", func(t *testing.T) { @@ -2368,7 +2370,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking condition Test=False") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2380,7 +2382,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking condition Ready=True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2401,7 +2403,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err := v1beta2conditions.MatchCondition(*testConditionCopy, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*testConditionAfter) + ok, err := v1beta2conditions.MatchCondition(*testConditionCopy).Match(*testConditionAfter) if err != nil || !ok { return false } @@ -2414,7 +2416,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2444,7 +2446,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking condition Test=False") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Test", Status: metav1.ConditionFalse, Reason: "reason", Message: "message", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2458,7 +2460,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { t.Log("Changing the object spec, status, and marking condition Ready=True") obj.Spec.Foo = "foo" obj.Status.Bar = "bat" - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2480,7 +2482,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err := v1beta2conditions.MatchCondition(*testConditionCopy, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*testConditionAfter) + ok, err := v1beta2conditions.MatchCondition(*testConditionCopy).Match(*testConditionAfter) if err != nil || !ok { return false } @@ -2493,7 +2495,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err = v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err = v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2524,7 +2526,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking a Ready condition to be false") - err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2536,7 +2538,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking condition Ready=True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2549,7 +2551,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { return nil } return objAfter.Status.Conditions - }, timeout).Should(v1beta2conditions.MatchConditions(objCopy.Status.Conditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + }, timeout).Should(v1beta2conditions.MatchConditions(objCopy.Status.Conditions)) }) t.Run("should not return an error if there is an unresolvable conflict but the conditions is owned by the controller", func(t *testing.T) { @@ -2573,7 +2575,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking a Ready condition to be false") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2585,11 +2587,11 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking Ready condition True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") - g.Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}}, WithOwnedV1Beta1Conditions{Conditions: []string{"Ready"}})).To(Succeed()) + g.Expect(patcher.Patch(ctx, obj, WithOwnedV1Beta2Conditions{Conditions: []string{"Ready"}})).To(Succeed()) t.Log("Validating the object has been updated") g.Eventually(func() bool { @@ -2606,7 +2608,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err := v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err := v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } @@ -2636,7 +2638,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { objCopy := obj.DeepCopy() t.Log("Marking a Ready condition to be false") - err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood"}) + err := v1beta2conditions.Set(objCopy, metav1.Condition{Type: "Ready", Status: metav1.ConditionFalse, Reason: "NotGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(env.Status().Update(ctx, objCopy)).To(Succeed()) @@ -2648,7 +2650,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Log("Marking Ready condition True") - err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood"}) + err = v1beta2conditions.Set(obj, metav1.Condition{Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllGood", LastTransitionTime: now}) g.Expect(err).ToNot(HaveOccurred()) t.Log("Patching the object") @@ -2669,7 +2671,7 @@ func TestPatchHelperForV1beta2Transition(t *testing.T) { if err != nil { return false } - ok, err := v1beta2conditions.MatchCondition(*readyBefore, v1beta2conditions.IgnoreLastTransitionTime(true)).Match(*readyAfter) + ok, err := v1beta2conditions.MatchCondition(*readyBefore).Match(*readyAfter) if err != nil || !ok { return false } diff --git a/util/patch/utils.go b/util/patch/utils.go index 92046b89628e..b056d4a9d138 100644 --- a/util/patch/utils.go +++ b/util/patch/utils.go @@ -97,16 +97,17 @@ func unsafeUnstructuredCopy(obj *unstructured.Unstructured, focus patchType, met if preserve || preserveUnstructuredKeys[key] { res.Object[key] = value } + } - // If we've determined that new or old condition must be set, - // when dealing with the status patch, remove corresponding sub-fields from the object. - // NOTE: Removing conditions sub-fields changes the incoming object! This is safe because the condition patch - // doesn't use the unstructured fields, and it runs before any other patch. - // - // If we want to be 100% safe, we could make a copy of the incoming object before modifying it, although - // copies have a high cpu and high memory usage, therefore we intentionally choose to avoid extra copies - // given that the ordering of operations and safety is handled internally by the patch helper. - + // If we've determined that new or old condition must be set, + // when dealing with the status patch, remove corresponding sub-fields from the object. + // NOTE: Removing conditions sub-fields changes the incoming object! This is safe because the condition patch + // doesn't use the unstructured fields, and it runs before any other patch. + // + // If we want to be 100% safe, we could make a copy of the incoming object before modifying it, although + // copies have a high cpu and high memory usage, therefore we intentionally choose to avoid extra copies + // given that the ordering of operations and safety is handled internally by the patch helper. + if focus == statusPatch { if len(metav1ConditionsFields) > 0 { unstructured.RemoveNestedField(res.Object, metav1ConditionsFields...) } @@ -151,8 +152,19 @@ func identifySupportedConditions(obj runtime.Object) ([]string, []string, error) var clusterv1ConditionsFields []string if v1beta2Field := statusField.FieldByName("V1Beta2"); v1beta2Field != (reflect.Value{}) { - if conditionField := v1beta2Field.FieldByName("Conditions"); conditionField != (reflect.Value{}) { + if v1beta2Field.Kind() != reflect.Pointer { + return nil, nil, errors.New("obj.status.v1beta2 must be a pointer") + } + + v1beta2Elem := v1beta2Field.Elem() + if !v1beta2Elem.IsValid() { + // If the field is a zero value of its type, we can't further investigate type struct. + // We assume the type is implemented according to transition guidelines metav1ConditionsFields = []string{"status", "v1beta2", "conditions"} + } else { + if conditionField := v1beta2Elem.FieldByName("Conditions"); conditionField != (reflect.Value{}) { + metav1ConditionsFields = []string{"status", "v1beta2", "conditions"} + } } } @@ -166,9 +178,31 @@ func identifySupportedConditions(obj runtime.Object) ([]string, []string, error) } if deprecatedField := statusField.FieldByName("Deprecated"); deprecatedField != (reflect.Value{}) { - if v1beta1Field := deprecatedField.FieldByName("V1Beta1"); v1beta1Field != (reflect.Value{}) { - if conditionField := v1beta1Field.FieldByName("Conditions"); conditionField != (reflect.Value{}) { - clusterv1ConditionsFields = []string{"status", "deprecated", "v1beta1", "conditions"} + if deprecatedField.Kind() != reflect.Pointer { + return nil, nil, errors.New("obj.status.deprecated must be a pointer") + } + + deprecatedElem := deprecatedField.Elem() + if !deprecatedElem.IsValid() { + // If the field is a zero value of its type, we can't further investigate type struct. + // We assume the type is implemented according to transition guidelines + clusterv1ConditionsFields = []string{"status", "deprecated", "v1beta1", "conditions"} + } else { + if v1Beta1Field := deprecatedElem.FieldByName("V1Beta1"); deprecatedField != (reflect.Value{}) { + if v1Beta1Field.Kind() != reflect.Pointer { + return nil, nil, errors.New("obj.status.deprecated.v1beta1 must be a pointer") + } + + v1Beta1Elem := v1Beta1Field.Elem() + if !v1Beta1Elem.IsValid() { + // If the field is a zero value of its type, we can't further investigate type struct. + // We assume the type is implemented according to transition guidelines + clusterv1ConditionsFields = []string{"status", "deprecated", "v1beta1", "conditions"} + } else { + if conditionField := v1Beta1Elem.FieldByName("Conditions"); conditionField != (reflect.Value{}) { + clusterv1ConditionsFields = []string{"status", "deprecated", "v1beta1", "conditions"} + } + } } } } diff --git a/util/patch/utils_test.go b/util/patch/utils_test.go index 1dc6c2519380..c89a6e640046 100644 --- a/util/patch/utils_test.go +++ b/util/patch/utils_test.go @@ -22,6 +22,7 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -256,20 +257,37 @@ func TestIdentifySupportedConditions(t *testing.T) { t.Run("v1beta1 object with both clusterv1.conditions and metav1.conditions (phase 1)", func(t *testing.T) { g := NewWithT(t) - obj := &builder.Phase1Obj{} - metav1ConditionsFields, clusterv1ConditionsFields, err := identifySupportedConditions(obj) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(metav1ConditionsFields).To(Equal([]string{"status", "v1beta2", "conditions"})) - g.Expect(clusterv1ConditionsFields).To(Equal([]string{"status", "conditions"})) + tests := []struct { + obj runtime.Object + }{ + {obj: &builder.Phase1Obj{}}, + {obj: &builder.Phase1Obj{Status: builder.Phase1ObjStatus{V1Beta2: nil}}}, + {obj: &builder.Phase1Obj{Status: builder.Phase1ObjStatus{V1Beta2: &builder.Phase1ObjStatusV1Beta2{Conditions: nil}}}}, + } + for _, tt := range tests { + metav1ConditionsFields, clusterv1ConditionsFields, err := identifySupportedConditions(tt.obj) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(metav1ConditionsFields).To(Equal([]string{"status", "v1beta2", "conditions"})) + g.Expect(clusterv1ConditionsFields).To(Equal([]string{"status", "conditions"})) + } }) t.Run("v1beta2 object with both clusterv1.conditions and metav1.conditions conditions (phase 2)", func(t *testing.T) { g := NewWithT(t) - obj := &builder.Phase2Obj{} - metav1ConditionsFields, clusterv1ConditionsFields, err := identifySupportedConditions(obj) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(metav1ConditionsFields).To(Equal([]string{"status", "conditions"})) - g.Expect(clusterv1ConditionsFields).To(Equal([]string{"status", "deprecated", "v1beta1", "conditions"})) + tests := []struct { + obj runtime.Object + }{ + {obj: &builder.Phase2Obj{}}, + {obj: &builder.Phase2Obj{Status: builder.Phase2ObjStatus{Deprecated: nil}}}, + {obj: &builder.Phase2Obj{Status: builder.Phase2ObjStatus{Deprecated: &builder.Phase2ObjStatusDeprecated{V1Beta1: nil}}}}, + {obj: &builder.Phase2Obj{Status: builder.Phase2ObjStatus{Deprecated: &builder.Phase2ObjStatusDeprecated{V1Beta1: &builder.Phase2ObjStatusDeprecatedV1Beta1{Conditions: nil}}}}}, + } + for _, tt := range tests { + metav1ConditionsFields, clusterv1ConditionsFields, err := identifySupportedConditions(tt.obj) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(metav1ConditionsFields).To(Equal([]string{"status", "conditions"})) + g.Expect(clusterv1ConditionsFields).To(Equal([]string{"status", "deprecated", "v1beta1", "conditions"})) + } }) t.Run("v1beta2 object with metav1.conditions (phase 3)", func(t *testing.T) { g := NewWithT(t)