Merge pull request kubernetes-sigs#6401 from killianmuldoon/fix/nil-condition-patch

🐛 Fix nil pointers in conditions patch utils

Author: k8s-ci-robot

docs/book/src/developer/providers/v1.1-to-v1.2.md

@@ -45,6 +45,7 @@ in ClusterAPI are kept in sync with the versions used by `sigs.k8s.io/controller
 
 - `util.ClusterToInfrastructureMapFuncWithExternallyManagedCheck` was removed and the externally managed check was added to `util.ClusterToInfrastructureMapFunc`, which required changing its signature.
    Users of the former simply need to start using the latter and users of the latter need to add the new arguments to their call.
+- `conditions.NewPatch` from the "sigs.k8s.io/cluster-api/util/conditions" package has had its return type modified. Previously the function returned `Patch`. It now returns `(Patch, error)`. Users of `NewPatch` need to be update usages to handle the error.
 
 ### Required API Changes for providers
 

internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go

@@ -28,6 +28,7 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/contract"
+	"sigs.k8s.io/cluster-api/util"
 )
 
 // TopologyManagerName is the manager name in managed fields for the topology controller.
@@ -49,7 +50,7 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client,
 	// not relevant for the topology controller.
 
 	var originalUnstructured *unstructured.Unstructured
-	if !isNil(original) {
+	if !util.IsNil(original) {
 		originalUnstructured = &unstructured.Unstructured{}
 		switch original.(type) {
 		case *unstructured.Unstructured:
@@ -93,7 +94,7 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client,
 	// an actual change when running server side apply, and if this change might impact the object spec or not.
 	var hasChanges, hasSpecChanges bool
 	switch {
-	case isNil(original):
+	case util.IsNil(original):
 		hasChanges, hasSpecChanges = true, true
 	default:
 		hasChanges, hasSpecChanges = dryRunPatch(&dryRunInput{

internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go

@@ -20,7 +20,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"reflect"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
 	"github.com/pkg/errors"
@@ -30,6 +29,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"sigs.k8s.io/cluster-api/internal/contract"
+	"sigs.k8s.io/cluster-api/util"
 )
 
 // TwoWaysPatchHelper helps with a patch that yields the modified document when applied to the original document.
@@ -73,7 +73,7 @@ func NewTwoWaysPatchHelper(original, modified client.Object, c client.Client, op
 	// In case we are creating an object, we extend the set of allowed fields adding apiVersion, Kind
 	// metadata.name, metadata.namespace (who are required by the API server) and metadata.ownerReferences
 	// that gets set to avoid orphaned objects.
-	if isNil(original) {
+	if util.IsNil(original) {
 		helperOptions.allowedPaths = append(helperOptions.allowedPaths,
 			contract.Path{"apiVersion"},
 			contract.Path{"kind"},
@@ -89,7 +89,7 @@ func NewTwoWaysPatchHelper(original, modified client.Object, c client.Client, op
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to marshal original object to json")
 	}
-	if isNil(original) {
+	if util.IsNil(original) {
 		originalJSON = []byte("{}")
 	}
 
@@ -210,7 +210,7 @@ func (h *TwoWaysPatchHelper) Patch(ctx context.Context) error {
 	}
 	log := ctrl.LoggerFrom(ctx)
 
-	if isNil(h.original) {
+	if util.IsNil(h.original) {
 		modifiedMap := make(map[string]interface{})
 		if err := json.Unmarshal(h.patch, &modifiedMap); err != nil {
 			return errors.Wrap(err, "failed to unmarshal two way merge patch")
@@ -226,14 +226,3 @@ func (h *TwoWaysPatchHelper) Patch(ctx context.Context) error {
 	log.V(5).Info("Patching object", "Patch", string(h.patch))
 	return h.client.Patch(ctx, h.original.DeepCopyObject().(client.Object), client.RawPatch(types.MergePatchType, h.patch))
 }
-
-func isNil(i interface{}) bool {
-	if i == nil {
-		return true
-	}
-	switch reflect.TypeOf(i).Kind() {
-	case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice:
-		return reflect.ValueOf(i).IsValid() && reflect.ValueOf(i).IsNil()
-	}
-	return false
-}

internal/test/matchers/matchers.go

@@ -20,12 +20,13 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"reflect"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
 	"github.com/onsi/gomega/format"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime"
+
+	"sigs.k8s.io/cluster-api/util"
 )
 
 // This code is adappted from the mergePatch code at controllers/topology/internal/mergepatch pkg.
@@ -82,13 +83,11 @@ func (m *Matcher) Match(actual interface{}) (success bool, err error) {
 	// Nil checks required first here for:
 	//     1) Nil equality which returns true
 	//     2) One object nil which returns an error
-	actualIsNil := reflect.ValueOf(actual).IsNil()
-	originalIsNil := reflect.ValueOf(m.original).IsNil()
 
-	if actualIsNil && originalIsNil {
+	if util.IsNil(actual) && util.IsNil(m.original) {
 		return true, nil
 	}
-	if actualIsNil || originalIsNil {
+	if util.IsNil(actual) || util.IsNil(m.original) {
 		return false, fmt.Errorf("can not compare an object with a nil. original %v , actual %v", m.original, actual)
 	}
 

util/conditions/getter_test.go

@@ -287,6 +287,11 @@ func getterWithConditions(conditions ...*clusterv1.Condition) Getter {
 	return obj
 }
 
+func nilGetter() Getter {
+	var obj *clusterv1.Cluster
+	return obj
+}
+
 func conditionList(conditions ...*clusterv1.Condition) clusterv1.Conditions {
 	cs := clusterv1.Conditions{}
 	for _, x := range conditions {

util/conditions/patch.go

@@ -23,6 +23,7 @@ import (
 	"github.com/pkg/errors"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
 )
 
 // Patch defines a list of operations to change a list of conditions into another.
@@ -49,10 +50,17 @@ const (
 	RemoveConditionPatch PatchOperationType = "Remove"
 )
 
-// NewPatch returns the list of Patch required to align source conditions to after conditions.
-func NewPatch(before Getter, after Getter) Patch {
+// NewPatch returns the Patch required to align source conditions to after conditions.
+func NewPatch(before Getter, after Getter) (Patch, error) {
 	var patch Patch
 
+	if util.IsNil(before) {
+		return nil, errors.New("error creating patch: before object is nil")
+	}
+	if util.IsNil(after) {
+		return nil, errors.New("error creating patch: after object is nil")
+	}
+
 	// Identify AddCondition and ModifyCondition changes.
 	targetConditions := after.GetConditions()
 	for i := range targetConditions {
@@ -77,7 +85,7 @@ func NewPatch(before Getter, after Getter) Patch {
 			patch = append(patch, PatchOperation{Op: RemoveConditionPatch, Before: &baseCondition})
 		}
 	}
-	return patch
+	return patch, nil
 }
 
 // applyOptions allows to set strategies for patch apply.
@@ -116,10 +124,14 @@ func WithForceOverwrite(v bool) ApplyOption {
 // 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 {
-	if len(p) == 0 {
+	if p.IsZero() {
 		return nil
 	}
 
+	if util.IsNil(latest) {
+		return errors.New("error patching conditions: latest object was nil")
+	}
+
 	applyOpt := &applyOptions{}
 	for _, o := range options {
 		o(applyOpt)
@@ -195,7 +207,10 @@ func (p Patch) Apply(latest Setter, options ...ApplyOption) error {
 	return nil
 }
 
-// IsZero returns true if the patch has no changes.
+// IsZero returns true if the patch is nil or has no changes.
 func (p Patch) IsZero() bool {
+	if p == nil {
+		return true
+	}
 	return len(p) == 0
 }

util/conditions/patch_test.go

@@ -32,17 +32,44 @@ func TestNewPatch(t *testing.T) {
 	fooFalse := FalseCondition("foo", "reason foo", clusterv1.ConditionSeverityInfo, "message foo")
 
 	tests := []struct {
-		name   string
-		before Getter
-		after  Getter
-		want   Patch
+		name    string
+		before  Getter
+		after   Getter
+		want    Patch
+		wantErr bool
 	}{
 		{
-			name:   "No changes return empty patch",
-			before: getterWithConditions(),
-			after:  getterWithConditions(),
-			want:   nil,
+			name:    "nil before return error",
+			before:  nil,
+			after:   getterWithConditions(),
+			wantErr: true,
+		},
+		{
+			name:    "nil after return error",
+			before:  getterWithConditions(),
+			after:   nil,
+			wantErr: true,
+		},
+		{
+			name:    "nil Interface before return error",
+			before:  nilGetter(),
+			after:   getterWithConditions(),
+			wantErr: true,
 		},
+		{
+			name:    "nil Interface after return error",
+			before:  getterWithConditions(),
+			after:   nilGetter(),
+			wantErr: true,
+		},
+		{
+			name:    "No changes return empty patch",
+			before:  getterWithConditions(),
+			after:   getterWithConditions(),
+			want:    nil,
+			wantErr: false,
+		},
+
 		{
 			name:   "No changes return empty patch",
 			before: getterWithConditions(fooTrue),
@@ -91,8 +118,12 @@ func TestNewPatch(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got := NewPatch(tt.before, tt.after)
-
+			got, err := NewPatch(tt.before, tt.after)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
+			}
+			g.Expect(err).To(Not(HaveOccurred()))
 			g.Expect(got).To(Equal(tt.want))
 		})
 	}
@@ -112,6 +143,22 @@ func TestApply(t *testing.T) {
 		want    clusterv1.Conditions
 		wantErr bool
 	}{
+		{
+			name:    "error with nil interface Setter",
+			before:  getterWithConditions(fooTrue),
+			after:   getterWithConditions(fooFalse),
+			latest:  nilSetter(),
+			want:    conditionList(fooTrue),
+			wantErr: true,
+		},
+		{
+			name:    "error with nil Setter",
+			before:  getterWithConditions(fooTrue),
+			after:   getterWithConditions(fooFalse),
+			latest:  nil,
+			want:    conditionList(fooTrue),
+			wantErr: true,
+		},
 		{
 			name:    "No patch return same list",
 			before:  getterWithConditions(fooTrue),
@@ -242,7 +289,8 @@ func TestApply(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			patch := NewPatch(tt.before, tt.after)
+			// Ignore the error here to allow testing of patch.Apply with a nil patch
+			patch, _ := NewPatch(tt.before, tt.after)
 
 			err := patch.Apply(tt.latest, tt.options...)
 			if tt.wantErr {
@@ -276,8 +324,9 @@ func TestApplyDoesNotAlterLastTransitionTime(t *testing.T) {
 	// latest has no conditions, so we are actually adding the condition but in this case we should not set the LastTransition Time
 	// but we should preserve the LastTransition set in after
 
-	diff := NewPatch(before, after)
-	err := diff.Apply(latest)
+	diff, err := NewPatch(before, after)
+	g.Expect(err).ToNot(HaveOccurred())
+	err = diff.Apply(latest)
 
 	g.Expect(err).ToNot(HaveOccurred())
 	g.Expect(latest.GetConditions()).To(Equal(after.GetConditions()))

util/conditions/setter_test.go

@@ -261,6 +261,11 @@ func setterWithConditions(conditions ...*clusterv1.Condition) Setter {
 	return obj
 }
 
+func nilSetter() Setter {
+	var obj *clusterv1.Cluster
+	return obj
+}
+
 func haveSameConditionsOf(expected clusterv1.Conditions) types.GomegaMatcher {
 	return &ConditionsMatcher{
 		Expected: expected,

util/patch/patch.go

@@ -19,7 +19,6 @@ package patch
 import (
 	"context"
 	"encoding/json"
-	"reflect"
 	"time"
 
 	"github.com/pkg/errors"
@@ -33,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 )
 
@@ -51,8 +51,8 @@ type Helper struct {
 // NewHelper returns an initialized Helper.
 func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) {
 	// Return early if the object is nil.
-	if err := checkNilObject(obj); err != nil {
-		return nil, err
+	if util.IsNil(obj) {
+		return nil, errors.New("helper could not be created: object is nil")
 	}
 
 	// Get the GroupVersionKind of the object,
@@ -83,8 +83,8 @@ func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) {
 // Patch will attempt to patch the given object, including its status.
 func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) error {
 	// Return early if the object is nil.
-	if err := checkNilObject(obj); err != nil {
-		return err
+	if util.IsNil(obj) {
+		return errors.New("Patch could not be completed: object is nil")
 	}
 
 	// Get the GroupVersionKind of the object that we want to patch.
@@ -197,10 +197,13 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 	}
 
 	// Store the diff from the before/after object, and return early if there are no changes.
-	diff := conditions.NewPatch(
+	diff, err := conditions.NewPatch(
 		before,
 		after,
 	)
+	if err != nil {
+		return errors.Wrapf(err, "object can not be patched")
+	}
 	if diff.IsZero() {
 		return nil
 	}
@@ -298,12 +301,3 @@ func (h *Helper) calculateChanges(after client.Object) (map[string]bool, error)
 	}
 	return res, nil
 }
-
-func checkNilObject(obj client.Object) error {
-	// If you're wondering why we need reflection to do this check, see https://golang.org/doc/faq#nil_error.
-	// TODO(vincepri): Remove this check and let it panic if used improperly in a future minor release.
-	if obj == nil || (reflect.ValueOf(obj).IsValid() && reflect.ValueOf(obj).IsNil()) {
-		return errors.Errorf("expected non-nil object")
-	}
-	return nil
-}