🏃 Add options to patch helper for ObservedGeneration

Signed-off-by: Vince Prignano <[email protected]>

go.mod

@@ -39,7 +39,7 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/kube-openapi v0.0.0-20200121204235-bf4fb3bd569c // indirect
 	k8s.io/utils v0.0.0-20200229041039-0a110f9eb7ab
-	sigs.k8s.io/controller-runtime v0.5.3
+	sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e
 	sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802
 	sigs.k8s.io/yaml v1.2.0
 )

go.sum

@@ -653,8 +653,8 @@ modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=
 modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs=
 modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I=
-sigs.k8s.io/controller-runtime v0.5.3 h1:HGL1kpWxe6WiiN4yCmYA5SFtvUMQAkOwV+Ylh7M87E4=
-sigs.k8s.io/controller-runtime v0.5.3/go.mod h1:JZUwSMVbxDupo0lTJSSFP5pimEyxGynROImSsqIOx1A=
+sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e h1:X/iF0awmtZnNud5vm+LNgPRzZsjyCWqGJtbluTBX9x8=
+sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e/go.mod h1:JZUwSMVbxDupo0lTJSSFP5pimEyxGynROImSsqIOx1A=
 sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802 h1:L6/8hETA7jvdx3xBcbDifrIN2xaYHE7tA58n+Kdp2Zw=
 sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802/go.mod h1:HIZ3PWUezpklcjkqpFbnYOqaqsAE1JeCTEwkgvPLXjk=
 sigs.k8s.io/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI=

test/infrastructure/docker/go.mod

@@ -11,7 +11,7 @@ require (
 	k8s.io/client-go v0.17.2
 	k8s.io/klog v1.0.0
 	sigs.k8s.io/cluster-api v0.3.3
-	sigs.k8s.io/controller-runtime v0.5.3
+	sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e
 	sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802
 	sigs.k8s.io/yaml v1.2.0
 )

test/infrastructure/docker/go.sum

@@ -580,8 +580,8 @@ modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=
 modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs=
 modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I=
-sigs.k8s.io/controller-runtime v0.5.3 h1:HGL1kpWxe6WiiN4yCmYA5SFtvUMQAkOwV+Ylh7M87E4=
-sigs.k8s.io/controller-runtime v0.5.3/go.mod h1:JZUwSMVbxDupo0lTJSSFP5pimEyxGynROImSsqIOx1A=
+sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e h1:X/iF0awmtZnNud5vm+LNgPRzZsjyCWqGJtbluTBX9x8=
+sigs.k8s.io/controller-runtime v0.5.5-0.20200529152116-5291db5ab70e/go.mod h1:JZUwSMVbxDupo0lTJSSFP5pimEyxGynROImSsqIOx1A=
 sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802 h1:L6/8hETA7jvdx3xBcbDifrIN2xaYHE7tA58n+Kdp2Zw=
 sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802/go.mod h1:HIZ3PWUezpklcjkqpFbnYOqaqsAE1JeCTEwkgvPLXjk=
 sigs.k8s.io/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI=

util/conditions/patch.go

@@ -139,3 +139,8 @@ func (p Patch) Apply(source Setter) error {
 	}
 	return nil
 }
+
+// IsZero returns true if the patch has no changes.
+func (p Patch) IsZero() bool {
+	return len(p) == 0
+}

util/patch/options.go

@@ -0,0 +1,37 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package patch
+
+// Option is some configuration that modifies options for a patch request.
+type Option interface {
+	// ApplyToHelper applies this configuration to the given Helper options.
+	ApplyToHelper(*HelperOptions)
+}
+
+// HelperOptions contains options for patch options.
+type HelperOptions struct {
+	IncludeStatusObservedGeneration bool
+}
+
+// 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.
+func (w WithStatusObservedGeneration) ApplyToHelper(in *HelperOptions) {
+	in.IncludeStatusObservedGeneration = true
+}

util/patch/patch.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -18,65 +18,58 @@ package patch
 
 import (
 	"context"
-	"reflect"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/api/equality"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/wait"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // Helper is a utility for ensuring the proper Patching of resources
 // and their status
 type Helper struct {
-	client        client.Client
-	before        map[string]interface{}
-	hasStatus     bool
-	beforeStatus  interface{}
-	resourcePatch client.Patch
-	statusPatch   client.Patch
+	opts *HelperOptions
+
+	client          client.Client
+	before          *unstructured.Unstructured
+	after           *unstructured.Unstructured
+	resourcePatch   client.Patch
+	statusPatch     client.Patch
+	conditionsPatch client.Patch
 }
 
 // NewHelper returns an initialized Helper
-func NewHelper(resource runtime.Object, crClient client.Client) (*Helper, error) {
+func NewHelper(resource runtime.Object, crClient client.Client, opts ...Option) (*Helper, error) {
 	if resource == nil {
 		return nil, errors.Errorf("expected non-nil resource")
 	}
 
-	// If the object is already unstructured, we need to perform a deepcopy first
-	// because the `DefaultUnstructuredConverter.ToUnstructured` function returns
-	// the underlying unstructured object map without making a copy.
-	if _, ok := resource.(runtime.Unstructured); ok {
-		resource = resource.DeepCopyObject()
+	// Calculate the options to pass to the helper.
+	options := &HelperOptions{}
+	for _, opt := range opts {
+		opt.ApplyToHelper(options)
 	}
 
-	// Convert the resource to unstructured for easier comparison later.
-	before, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource)
-	if err != nil {
-		return nil, err
-	}
-
-	hasStatus := false
-	// attempt to extract the status from the resource for easier comparison later
-	beforeStatus, ok, err := unstructured.NestedFieldCopy(before, "status")
+	// Convert the resource to unstructured to compare against our before copy.
+	raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource.DeepCopyObject())
 	if err != nil {
 		return nil, err
 	}
-	if ok {
-		hasStatus = true
-		// if the resource contains a status remove it from our unstructured copy
-		// to avoid unnecessary patching later
-		unstructured.RemoveNestedField(before, "status")
-	}
+	unstructuredResource := &unstructured.Unstructured{Object: raw}
 
 	return &Helper{
-		client:        crClient,
-		before:        before,
-		beforeStatus:  beforeStatus,
-		hasStatus:     hasStatus,
-		resourcePatch: client.MergeFrom(resource.DeepCopyObject()),
-		statusPatch:   client.MergeFrom(resource.DeepCopyObject()),
+		opts:            options,
+		client:          crClient,
+		before:          unstructuredResource,
+		resourcePatch:   client.MergeFrom(resource.DeepCopyObject()),
+		statusPatch:     client.MergeFrom(resource.DeepCopyObject()),
+		conditionsPatch: client.MergeFromWithOptions(resource.DeepCopyObject(), client.MergeFromWithOptimisticLock{}),
 	}, nil
 }
 
@@ -86,49 +79,168 @@ func (h *Helper) Patch(ctx context.Context, resource runtime.Object) error {
 		return errors.Errorf("expected non-nil resource")
 	}
 
-	// If the object is already unstructured, we need to perform a deepcopy first
-	// because the `DefaultUnstructuredConverter.ToUnstructured` function returns
-	// the underlying unstructured object map without making a copy.
-	if _, ok := resource.(runtime.Unstructured); ok {
-		resource = resource.DeepCopyObject()
-	}
-
 	// Convert the resource to unstructured to compare against our before copy.
-	after, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource)
+	raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource.DeepCopyObject())
 	if err != nil {
 		return err
 	}
+	h.after = &unstructured.Unstructured{Object: raw}
+
+	// Determine if the object has status.
+	if unstructuredHasStatus(h.after) {
+		if h.opts.IncludeStatusObservedGeneration {
+			// Set status.observedGeneration if we're asked to do so.
+			if err := unstructured.SetNestedField(h.after.Object, h.after.GetGeneration(), "status", "observedGeneration"); err != nil {
+				return err
+			}
+
+			// Restore the changes back to the original object.
+			if err := runtime.DefaultUnstructuredConverter.FromUnstructured(h.after.Object, resource); err != nil {
+				return err
+			}
+		}
+	}
 
-	hasStatus := false
-	// attempt to extract the status from the resource to compare against our
-	// beforeStatus copy
-	afterStatus, ok, err := unstructured.NestedFieldCopy(after, "status")
+	return kerrors.NewAggregate([]error{
+		h.patch(ctx, resource),
+		h.patchStatus(ctx, resource),
+		h.patchStatusConditions(ctx, resource),
+	})
+}
+
+// patch issues a patch for everything in the object except for the status subresource.
+func (h *Helper) patch(ctx context.Context, resource runtime.Object) error {
+	before := h.before.DeepCopy()
+	unstructured.RemoveNestedField(before.Object, "status")
+	after := h.after.DeepCopy()
+	unstructured.RemoveNestedField(after.Object, "status")
+	if !shouldPatch(before, after) {
+		return nil
+	}
+	spew.Dump("AAAA")
+	return h.client.Patch(ctx, resource.DeepCopyObject(), h.resourcePatch)
+}
+
+// patchStatus issues a patch if the status has changed.
+// Conditions are ignored at this stage and handled separately.
+func (h *Helper) patchStatus(ctx context.Context, resource runtime.Object) error {
+	before, err := extractStatusWithoutConditions(h.before)
 	if err != nil {
 		return err
 	}
-	if ok {
-		hasStatus = true
-		// if the resource contains a status remove it from our unstructured copy
-		// to avoid uneccsary patching.
-		unstructured.RemoveNestedField(after, "status")
+	after, err := extractStatusWithoutConditions(h.after)
+	if err != nil {
+		return err
+	}
+	if !shouldPatch(before, after) {
+		return nil
 	}
+	spew.Dump("BBBBB")
+	return h.client.Status().Patch(ctx, resource.DeepCopyObject(), h.statusPatch)
+}
 
-	var errs []error
+// patchStatusConditions issues a patch if there is any changes to the conditions slice under
+// the status subresource. This is a special case and it's handled separately given that
+// we allow different controllers to act on conditions of the same object.
+//
+// This method has an internal backoff loop. When a conflict is detected, the method
+// asks the Client for the a new version of the resource we're trying to patch.
+//
+// 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, resource runtime.Object) error {
+	// Store the patch from the before/after object.
+	conditionPatch := conditions.NewPatch(
+		conditions.UnstructuredGetter(h.before),
+		conditions.UnstructuredGetter(h.after),
+	)
+
+	// Return early if there are no changes.
+	if conditionPatch.IsZero() {
+		return nil
+	}
 
-	if !reflect.DeepEqual(h.before, after) {
-		// only issue a Patch if the before and after resources (minus status) differ
-		if err := h.client.Patch(ctx, resource.DeepCopyObject(), h.resourcePatch); err != nil {
-			errs = append(errs, err)
-		}
+	// Make a copy of the object and store the key used if we have conflicts.
+	resourceCopy := resource.DeepCopyObject()
+	key, err := client.ObjectKeyFromObject(resourceCopy)
+	if err != nil {
+		return err
 	}
 
-	if (h.hasStatus || hasStatus) && !reflect.DeepEqual(h.beforeStatus, afterStatus) {
-		// only issue a Status Patch if the resource has a status and the beforeStatus
-		// and afterStatus copies differ
-		if err := h.client.Status().Patch(ctx, resource.DeepCopyObject(), h.statusPatch); err != nil {
-			errs = append(errs, err)
+	// Define and start a backoff loop to handle conflicts
+	// between controllers working on the same object.
+	backoff := wait.Backoff{} // TODO
+	if err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+		// Issue a patch.
+		err := h.client.Status().Patch(ctx, resourceCopy, h.conditionsPatch)
+		switch {
+		// Handle conflicts.
+		case apierrors.IsConflict(err):
+			// Get a new copy of the object.
+			if err := h.client.Get(ctx, key, resourceCopy); err != nil {
+				return false, err
+			}
+
+			// Reset the condition patch before merging conditions.
+			h.conditionsPatch = client.MergeFromWithOptions(resourceCopy.DeepCopyObject(), client.MergeFromWithOptimisticLock{})
+
+			// Convert the newly-retrieved object to unstructured.
+			raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resourceCopy.DeepCopyObject())
+			if err != nil {
+				return false, err
+			}
+			u := &unstructured.Unstructured{Object: raw}
+
+			// Set the condition patch previously creatd on the new object.
+			if err := conditionPatch.Apply(conditions.UnstructuredSetter(u)); err != nil {
+				return false, err
+			}
+
+			// Restore the changes back to the temporary copy.
+			if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, resourceCopy); err != nil {
+				return false, err
+			}
+
+			return false, nil
+		case err != nil:
+			return false, err
 		}
+
+		return true, nil
+	}); err != nil {
+		return err
 	}
 
-	return kerrors.NewAggregate(errs)
+	return err
+}
+
+func unstructuredHasStatus(resource *unstructured.Unstructured) bool {
+	_, ok := resource.Object["status"]
+	return ok
+}
+
+func extractStatusWithoutConditions(u *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	status, err := extractField(u, "status")
+	if err != nil {
+		return nil, err
+	}
+	res := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"status": status,
+		},
+	}
+	unstructured.RemoveNestedField(res.Object, "status", "conditions")
+	return res, nil
+}
+
+func extractField(u *unstructured.Unstructured, fields ...string) (interface{}, error) {
+	x, ok, err := unstructured.NestedFieldCopy(u.Object, fields...)
+	if err != nil || !ok {
+		return nil, err
+	}
+	return x, nil
+}
+
+func shouldPatch(before, after *unstructured.Unstructured) bool {
+	return !equality.Semantic.DeepEqual(before, after)
 }