From 4497f36656928b092aff532233838023bd9a8dd2 Mon Sep 17 00:00:00 2001 From: Axel Christ Date: Thu, 31 Jan 2019 10:22:12 +0100 Subject: [PATCH] :bug: cleanup and fix CreateOrUpdate cleanup the difficult if/else structure fix an issue with the MutateFn of `CreateOrUpdate` being run even if the initial `Get` fails --- .../controllerutil/controllerutil.go | 61 +++++++------------ .../controllerutil/controllerutil_test.go | 28 ++++++++- 2 files changed, 48 insertions(+), 41 deletions(-) diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index d918eeaa42..3edc98e541 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -121,57 +121,42 @@ const ( // They should complete the sentence "Deployment default/foo has been .. // // It returns the executed operation and an error. func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) { - // op is the operation we are going to attempt - op := OperationResultNone - - // get the existing object meta - metaObj, ok := obj.(v1.Object) - if !ok { - return OperationResultNone, fmt.Errorf("%T does not implement metav1.Object interface", obj) + key, err := client.ObjectKeyFromObject(obj) + if err != nil { + return OperationResultNone, err } - // retrieve the existing object - key := client.ObjectKey{ - Name: metaObj.GetName(), - Namespace: metaObj.GetNamespace(), + if err := c.Get(ctx, key, obj); err != nil { + if errors.IsNotFound(err) { + if err := c.Create(ctx, obj); err != nil { + return OperationResultNone, err + } + return OperationResultCreated, nil + } + return OperationResultNone, err } - err := c.Get(ctx, key, obj) - // reconcile the existing object existing := obj.DeepCopyObject() - existingObjMeta := existing.(v1.Object) - existingObjMeta.SetName(metaObj.GetName()) - existingObjMeta.SetNamespace(metaObj.GetNamespace()) - - if e := f(obj); e != nil { - return OperationResultNone, e - } - - if metaObj.GetName() != existingObjMeta.GetName() { - return OperationResultNone, fmt.Errorf("ReconcileFn cannot mutate objects name") + if err := f(obj); err != nil { + return OperationResultNone, err } - if metaObj.GetNamespace() != existingObjMeta.GetNamespace() { - return OperationResultNone, fmt.Errorf("ReconcileFn cannot mutate objects namespace") + if reflect.DeepEqual(existing, obj) { + return OperationResultNone, nil } - if errors.IsNotFound(err) { - err = c.Create(ctx, obj) - op = OperationResultCreated - } else if err == nil { - if reflect.DeepEqual(existing, obj) { - return OperationResultNone, nil - } - err = c.Update(ctx, obj) - op = OperationResultUpdated - } else { + newKey, err := client.ObjectKeyFromObject(obj) + if err != nil { return OperationResultNone, err } + if key != newKey { + return OperationResultNone, fmt.Errorf("MutateFn cannot mutate object namespace and/or object name") + } - if err != nil { - op = OperationResultNone + if err := c.Update(ctx, obj); err != nil { + return OperationResultNone, err } - return op, err + return OperationResultUpdated, nil } // MutateFn is a function which mutates the existing object into it's desired state. diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index 89bde6329b..0018d9c9de 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/pkg/controller/controllerutil/controllerutil_test.go @@ -21,6 +21,8 @@ import ( "fmt" "math/rand" + "sigs.k8s.io/controller-runtime/pkg/client" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -140,7 +142,7 @@ var _ = Describe("Controllerutil", func() { }, Spec: corev1.PodSpec{ Containers: []corev1.Container{ - corev1.Container{ + { Name: "busybox", Image: "busybox", }, @@ -149,6 +151,8 @@ var _ = Describe("Controllerutil", func() { }, } + deploy.Spec = deplSpec + deplKey = types.NamespacedName{ Name: deploy.Name, Namespace: deploy.Namespace, @@ -158,7 +162,7 @@ var _ = Describe("Controllerutil", func() { It("creates a new object if one doesn't exists", func() { op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec)) - By("returning OperationResultCreatedd") + By("returning OperationResultCreated") Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated)) By("returning no error") @@ -176,7 +180,7 @@ var _ = Describe("Controllerutil", func() { Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated)) op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentScaler(scale)) - By("returning OperationResultUpdatedd") + By("returning OperationResultUpdated") Expect(op).To(BeEquivalentTo(controllerutil.OperationResultUpdated)) By("returning no error") @@ -232,6 +236,16 @@ var _ = Describe("Controllerutil", func() { By("returning error") Expect(err).To(HaveOccurred()) }) + + It("aborts immediately if there was an error initially retrieving the object", func() { + op, err := controllerutil.CreateOrUpdate(context.TODO(), errorReader{c}, deploy, func(_ runtime.Object) error { + Fail("Mutation method should not run") + return nil + }) + + Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone)) + Expect(err).To(HaveOccurred()) + }) }) }) @@ -273,3 +287,11 @@ func deploymentScaler(replicas int32) controllerutil.MutateFn { } return fn } + +type errorReader struct { + client.Client +} + +func (e errorReader) Get(ctx context.Context, key client.ObjectKey, into runtime.Object) error { + return fmt.Errorf("unexpected error") +}