diff --git a/util/conditions/patch.go b/util/conditions/patch.go index 31f91b98545c..c1bbdd5a3401 100644 --- a/util/conditions/patch.go +++ b/util/conditions/patch.go @@ -28,8 +28,8 @@ type Patch []PatchOperation // PatchOperation define an operation that changes a single condition. type PatchOperation struct { - Target *clusterv1.Condition - Base *clusterv1.Condition + After *clusterv1.Condition + Before *clusterv1.Condition Op PatchOperationType } @@ -57,12 +57,12 @@ func NewPatch(before Getter, after Getter) Patch { targetCondition := targetConditions[i] currentCondition := Get(before, targetCondition.Type) if currentCondition == nil { - patch = append(patch, PatchOperation{Op: AddConditionPatch, Target: &targetCondition}) + patch = append(patch, PatchOperation{Op: AddConditionPatch, After: &targetCondition}) continue } if !reflect.DeepEqual(&targetCondition, currentCondition) { - patch = append(patch, PatchOperation{Op: ChangeConditionPatch, Target: &targetCondition, Base: currentCondition}) + patch = append(patch, PatchOperation{Op: ChangeConditionPatch, After: &targetCondition, Before: currentCondition}) } } @@ -72,67 +72,114 @@ func NewPatch(before Getter, after Getter) Patch { baseCondition := baseConditions[i] targetCondition := Get(after, baseCondition.Type) if targetCondition == nil { - patch = append(patch, PatchOperation{Op: RemoveConditionPatch, Base: &baseCondition}) + patch = append(patch, PatchOperation{Op: RemoveConditionPatch, Before: &baseCondition}) } } return patch } +// applyOptions allows to set strategies for patch apply. +type applyOptions struct { + ownedConditions []clusterv1.ConditionType +} + +func (o *applyOptions) isOwnedCondition(t clusterv1.ConditionType) bool { + for _, i := range o.ownedConditions { + if i == t { + return true + } + } + return false +} + +// 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 ...clusterv1.ConditionType) ApplyOption { + return func(c *applyOptions) { + c.ownedConditions = t + } +} + // 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) error { +func (p Patch) Apply(latest Setter, options ...ApplyOption) error { if len(p) == 0 { return nil } + applyOpt := &applyOptions{} + for _, o := range options { + o(applyOpt) + } + for _, conditionPatch := range p { switch conditionPatch.Op { case AddConditionPatch: + // If the conditions is owned, always keep the after value. + if applyOpt.isOwnedCondition(conditionPatch.After.Type) { + Set(latest, 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 sourceCondition := Get(latest, conditionPatch.Target.Type); sourceCondition != nil { + if latestCondition := Get(latest, conditionPatch.After.Type); latestCondition != nil { // If latest and after agree on the change, then it is a conflict. - if !hasSameState(sourceCondition, conditionPatch.Target) { - return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/AddCondition conflict", conditionPatch.Target.Type) + 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", conditionPatch.After.Type) } // otherwise, the latest is already as intended. // NOTE: We are preserving LastTransitionTime from the latest in order to avoid altering the existing value. continue } // If the condition does not exists on the latest, add the new after condition. - Set(latest, conditionPatch.Target) + Set(latest, conditionPatch.After) case ChangeConditionPatch: - sourceCondition := Get(latest, conditionPatch.Target.Type) + // If the conditions is owned, always keep the after value. + if applyOpt.isOwnedCondition(conditionPatch.After.Type) { + Set(latest, conditionPatch.After) + continue + } + + latestCondition := Get(latest, conditionPatch.After.Type) // If the condition does not exist anymore on the latest, this is a conflict. - if sourceCondition == nil { - return errors.Errorf("error patching conditions: The condition %q on was deleted by a different process and this caused a merge/ChangeCondition conflict", conditionPatch.Target.Type) + if latestCondition == nil { + return errors.Errorf("error patching conditions: The condition %q was deleted by a different process and this caused a merge/ChangeCondition conflict", conditionPatch.After.Type) } // If the condition on the latest is different from the base condition, check if - // the after state corresponds to the desired value. If not this is a conflict. - if !reflect.DeepEqual(sourceCondition, conditionPatch.Base) { - if !hasSameState(sourceCondition, conditionPatch.Target) { - return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/ChangeCondition conflict", conditionPatch.Target.Type) + // the after state corresponds to the desired value. If not this is a conflict (unless we should ignore conflicts for this condition type). + if !reflect.DeepEqual(latestCondition, conditionPatch.Before) { + 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/ChangeCondition conflict", conditionPatch.After.Type) } // Otherwise the latest is already as intended. // NOTE: We are preserving LastTransitionTime from the latest in order to avoid altering the existing value. continue } - // Otherwise apply the new after condition. - Set(latest, conditionPatch.Target) + Set(latest, conditionPatch.After) case RemoveConditionPatch: + // If the conditions is owned, always keep the after value (condition should be deleted). + if applyOpt.isOwnedCondition(conditionPatch.Before.Type) { + Delete(latest, conditionPatch.Before.Type) + continue + } + // If the condition is still on the latest, check if it is changed in the meantime; // if so then this is a conflict. - if sourceCondition := Get(latest, conditionPatch.Base.Type); sourceCondition != nil { - if !hasSameState(sourceCondition, conditionPatch.Base) { - return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/RemoveCondition conflict", conditionPatch.Base.Type) + if latestCondition := Get(latest, conditionPatch.Before.Type); latestCondition != nil { + if !hasSameState(latestCondition, conditionPatch.Before) { + return errors.Errorf("error patching conditions: The condition %q was modified by a different process and this caused a merge/RemoveCondition conflict", conditionPatch.Before.Type) } } // Otherwise the latest and after agreed on the delete operation, so there's nothing to change. - Delete(latest, conditionPatch.Base.Type) + Delete(latest, conditionPatch.Before.Type) } } return nil diff --git a/util/conditions/patch_test.go b/util/conditions/patch_test.go index 5d491bc08c0a..b488a70fcf81 100644 --- a/util/conditions/patch_test.go +++ b/util/conditions/patch_test.go @@ -54,8 +54,8 @@ func TestNewPatch(t *testing.T) { after: getterWithConditions(fooTrue), want: Patch{ { - Base: nil, - Target: fooTrue, + Before: nil, + After: fooTrue, Op: AddConditionPatch, }, }, @@ -66,8 +66,8 @@ func TestNewPatch(t *testing.T) { after: getterWithConditions(fooFalse), want: Patch{ { - Base: fooTrue, - Target: fooFalse, + Before: fooTrue, + After: fooFalse, Op: ChangeConditionPatch, }, }, @@ -78,8 +78,8 @@ func TestNewPatch(t *testing.T) { after: getterWithConditions(), want: Patch{ { - Base: fooTrue, - Target: nil, + Before: fooTrue, + After: nil, Op: RemoveConditionPatch, }, }, @@ -107,6 +107,7 @@ func TestApply(t *testing.T) { before Getter after Getter latest Setter + options []ApplyOption want clusterv1.Conditions wantErr bool }{ @@ -142,6 +143,15 @@ func TestApply(t *testing.T) { want: nil, wantErr: true, }, + { + name: "Add: When a condition already exists but with conflicts, it should not error if the condition is owned", + before: getterWithConditions(), + after: getterWithConditions(fooTrue), + latest: setterWithConditions(fooFalse), + options: []ApplyOption{WithOwnedConditions("foo")}, + want: conditionList(fooTrue), // after condition should be kept in case of error + wantErr: false, + }, { name: "Remove: When a condition was already deleted, it should pass", before: getterWithConditions(fooTrue), @@ -166,6 +176,15 @@ func TestApply(t *testing.T) { want: nil, wantErr: true, }, + { + name: "Remove: When a condition already exists but with conflicts, it should not error if the condition is owned", + before: getterWithConditions(fooTrue), + after: getterWithConditions(), + latest: setterWithConditions(fooFalse), + options: []ApplyOption{WithOwnedConditions("foo")}, + want: conditionList(), // after condition should be kept in case of error + wantErr: false, + }, { name: "Change: When a condition exists without conflicts, it should change", before: getterWithConditions(fooTrue), @@ -190,6 +209,15 @@ func TestApply(t *testing.T) { want: nil, wantErr: true, }, + { + name: "Change: When a condition exists with conflicts but there is no agreement on the final state, it should not error if the condition is owned", + before: getterWithConditions(fooWarning), + after: getterWithConditions(fooFalse), + latest: setterWithConditions(fooTrue), + options: []ApplyOption{WithOwnedConditions("foo")}, + want: conditionList(fooFalse), // after condition should be kept in case of error + wantErr: false, + }, { name: "Change: When a condition was deleted, it should error", before: getterWithConditions(fooTrue), @@ -198,6 +226,15 @@ func TestApply(t *testing.T) { want: nil, wantErr: true, }, + { + name: "Change: When a condition was deleted, it should not error if the condition is owned", + before: getterWithConditions(fooTrue), + after: getterWithConditions(fooFalse), + latest: setterWithConditions(), + options: []ApplyOption{WithOwnedConditions("foo")}, + want: conditionList(fooFalse), // after condition should be kept in case of error + wantErr: false, + }, } for _, tt := range tests { @@ -206,7 +243,7 @@ func TestApply(t *testing.T) { patch := NewPatch(tt.before, tt.after) - err := patch.Apply(tt.latest) + err := patch.Apply(tt.latest, tt.options...) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return diff --git a/util/patch/options.go b/util/patch/options.go index 4b63264a38d7..95eb0010c5f0 100644 --- a/util/patch/options.go +++ b/util/patch/options.go @@ -16,6 +16,8 @@ limitations under the License. package patch +import clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" + // Option is some configuration that modifies options for a patch request. type Option interface { // ApplyToHelper applies this configuration to the given Helper options. @@ -27,13 +29,28 @@ type HelperOptions struct { // IncludeStatusObservedGeneration sets the status.observedGeneration field // on the incoming object to match metadata.generation, only if there is a change. IncludeStatusObservedGeneration bool + + // OwnedConditions 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. + OwnedConditions []clusterv1.ConditionType } // WithStatusObservedGeneration sets the status.observedGeneration field // on the incoming object to match metadata.generation, only if there is a change. type WithStatusObservedGeneration struct{} -// ApplyToHelper applies this configuration to the given an List options. +// ApplyToHelper applies this configuration to the given HelperOptions. func (w WithStatusObservedGeneration) ApplyToHelper(in *HelperOptions) { in.IncludeStatusObservedGeneration = true } + +// 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. +type WithOwnedConditions struct { + Conditions []clusterv1.ConditionType +} + +// ApplyToHelper applies this configuration to the given HelperOptions. +func (w WithOwnedConditions) ApplyToHelper(in *HelperOptions) { + in.OwnedConditions = w.Conditions +} diff --git a/util/patch/patch.go b/util/patch/patch.go index 029ae6b6d279..e0c2b65b242c 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" + clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -114,7 +115,7 @@ func (h *Helper) Patch(ctx context.Context, obj runtime.Object, opts ...Option) return kerrors.NewAggregate([]error{ h.patch(ctx, obj), h.patchStatus(ctx, obj), - h.patchStatusConditions(ctx, obj), + h.patchStatusConditions(ctx, obj, options.OwnedConditions), }) } @@ -151,7 +152,7 @@ func (h *Helper) patchStatus(ctx context.Context, obj runtime.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 runtime.Object) error { +func (h *Helper) patchStatusConditions(ctx context.Context, obj runtime.Object, ownedConditions []clusterv1.ConditionType) error { // Nothing to do if the object isn't a condition patcher. if !h.isConditionsSetter { return nil @@ -211,7 +212,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj runtime.Object) conditionsPatch := client.MergeFromWithOptions(latest.DeepCopyObject(), client.MergeFromWithOptimisticLock{}) // Set the condition patch previously created on the new object. - if err := diff.Apply(latest); err != nil { + if err := diff.Apply(latest, conditions.WithOwnedConditions(ownedConditions...)); err != nil { return false, err } diff --git a/util/patch/patch_test.go b/util/patch/patch_test.go index 8800ccc5e2f7..82c710d6847f 100644 --- a/util/patch/patch_test.go +++ b/util/patch/patch_test.go @@ -321,6 +321,48 @@ var _ = Describe("Patch Helper", func() { }, timeout).Should(BeTrue()) }) + Specify("should return not an error if there is an unresolvable conflict but the conditions is owned by the controller", func() { + obj := obj.DeepCopy() + + By("Creating the object") + Expect(testEnv.Create(ctx, obj)).ToNot(HaveOccurred()) + key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace} + defer func() { + Expect(testEnv.Delete(ctx, obj)).To(Succeed()) + }() + objCopy := obj.DeepCopy() + + By("Marking a custom condition to be false") + conditions.MarkFalse(objCopy, clusterv1.ReadyCondition, "reason", clusterv1.ConditionSeverityInfo, "message") + Expect(testEnv.Status().Update(ctx, objCopy)).To(Succeed()) + + By("Validating that the local object's resource version is behind") + Expect(obj.ResourceVersion).ToNot(Equal(objCopy.ResourceVersion)) + + By("Creating a new patch helper") + patcher, err := NewHelper(obj, testEnv) + Expect(err).NotTo(HaveOccurred()) + + By("Marking Ready=True") + conditions.MarkTrue(obj, clusterv1.ReadyCondition) + + By("Patching the object") + Expect(patcher.Patch(ctx, obj, WithOwnedConditions{Conditions: []clusterv1.ConditionType{clusterv1.ReadyCondition}})).To(Succeed()) + + By("Validating the object has been updated") + Eventually(func() bool { + objAfter := obj.DeepCopy() + if err := testEnv.Get(ctx, key, objAfter); err != nil { + return false + } + + readyBefore := conditions.Get(obj, clusterv1.ReadyCondition) + readyAfter := conditions.Get(objAfter, clusterv1.ReadyCondition) + + return cmp.Equal(readyBefore, readyAfter) + }, timeout).Should(BeTrue()) + }) + }) })