🐛 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

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.

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"
@@ -232,6 +234,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 +285,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")
+}