From 646f0b9738cd671b490834d8e7b06d1f64a634fe Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Fri, 8 Apr 2022 16:22:23 +0100 Subject: [PATCH] Fix nil pointers in conditions patch utils Signed-off-by: killianmuldoon --- .../src/developer/providers/v1.1-to-v1.2.md | 1 + .../structuredmerge/serversidepathhelper.go | 5 +- .../structuredmerge/twowayspatchhelper.go | 19 +---- internal/test/matchers/matchers.go | 9 +-- util/conditions/getter_test.go | 5 ++ util/conditions/patch.go | 25 +++++-- util/conditions/patch_test.go | 75 +++++++++++++++---- util/conditions/setter_test.go | 5 ++ util/patch/patch.go | 24 +++--- util/util.go | 13 ++++ 10 files changed, 126 insertions(+), 55 deletions(-) diff --git a/docs/book/src/developer/providers/v1.1-to-v1.2.md b/docs/book/src/developer/providers/v1.1-to-v1.2.md index b7c36a58071d..13d6ffb8a7bd 100644 --- a/docs/book/src/developer/providers/v1.1-to-v1.2.md +++ b/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 diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index 41f504fcb1ab..b8afc1ad32db 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/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{ diff --git a/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go b/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go index 78771290bf87..8d7f6fdf9f30 100644 --- a/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go +++ b/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 -} diff --git a/internal/test/matchers/matchers.go b/internal/test/matchers/matchers.go index 6d17630c1ff9..863ac0625893 100644 --- a/internal/test/matchers/matchers.go +++ b/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) } diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go index 0c6a0f92580d..1900480863c8 100644 --- a/util/conditions/getter_test.go +++ b/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 { diff --git a/util/conditions/patch.go b/util/conditions/patch.go index 358c538ad69c..9940bd8d01a2 100644 --- a/util/conditions/patch.go +++ b/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 } diff --git a/util/conditions/patch_test.go b/util/conditions/patch_test.go index 2a925d895538..d4276534c213 100644 --- a/util/conditions/patch_test.go +++ b/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())) diff --git a/util/conditions/setter_test.go b/util/conditions/setter_test.go index 0b8d1a530eef..60a7cba27c7e 100644 --- a/util/conditions/setter_test.go +++ b/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, diff --git a/util/patch/patch.go b/util/patch/patch.go index 8345a0cd8d74..a6559f6cd4d1 100644 --- a/util/patch/patch.go +++ b/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 -} diff --git a/util/util.go b/util/util.go index 8f3cad35734e..b33af56a609d 100644 --- a/util/util.go +++ b/util/util.go @@ -23,6 +23,7 @@ import ( "fmt" "math" "math/rand" + "reflect" "strings" "time" @@ -604,3 +605,15 @@ func LowestNonZeroResult(i, j ctrl.Result) ctrl.Result { return j } } + +// IsNil returns an error if the passed interface is equal to nil or if it has an interface value of nil. +func IsNil(i interface{}) bool { + if i == nil { + return true + } + switch reflect.TypeOf(i).Kind() { + case reflect.Ptr, reflect.Map, reflect.Chan, reflect.Slice, reflect.Interface, reflect.UnsafePointer, reflect.Func: + return reflect.ValueOf(i).IsValid() && reflect.ValueOf(i).IsNil() + } + return false +}