Merge pull request #313 from adracus/fix.create-or-update

🐛 cleanup and fix CreateOrUpdate

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"
@@ -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")
+}