Merge pull request #319 from presslabs/fix-create-or-update

⚠️ Call MutateFn on CreateOrUpdate regardless of the operation

Author: k8s-ci-robot

pkg/controller/controllerutil/controllerutil.go

@@ -114,10 +114,11 @@ const ( // They should complete the sentence "Deployment default/foo has been ..
 	OperationResultUpdated OperationResult = "updated"
 )
 
-// CreateOrUpdate creates or updates the given object obj in the Kubernetes
-// cluster. The object's desired state should be reconciled with the existing
-// state using the passed in ReconcileFn. obj must be a struct pointer so that
-// obj can be updated with the content returned by the Server.
+// CreateOrUpdate creates or updates the given object in the Kubernetes
+// cluster. The object's desired state must be reconciled with the existing
+// state inside the passed in callback MutateFn.
+//
+// The MutateFn is called regardless of creating or updating an object.
 //
 // It returns the executed operation and an error.
 func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) {
@@ -127,37 +128,43 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f
 	}
 
 	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
+		if !errors.IsNotFound(err) {
+			return OperationResultNone, err
 		}
-		return OperationResultNone, err
+		if err := mutate(f, key, obj); err != nil {
+			return OperationResultNone, err
+		}
+		if err := c.Create(ctx, obj); err != nil {
+			return OperationResultNone, err
+		}
+		return OperationResultCreated, nil
 	}
 
 	existing := obj.DeepCopyObject()
-	if err := f(obj); err != nil {
+	if err := mutate(f, key, obj); err != nil {
 		return OperationResultNone, err
 	}
 
 	if reflect.DeepEqual(existing, obj) {
 		return OperationResultNone, nil
 	}
 
-	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 := c.Update(ctx, obj); err != nil {
 		return OperationResultNone, err
 	}
 	return OperationResultUpdated, nil
 }
 
+// mutate wraps a MutateFn and applies validation to its result
+func mutate(f MutateFn, key client.ObjectKey, obj runtime.Object) error {
+	if err := f(); err != nil {
+		return err
+	}
+	if newKey, err := client.ObjectKeyFromObject(obj); err != nil || key != newKey {
+		return fmt.Errorf("MutateFn cannot mutate object name and/or object namespace")
+	}
+	return nil
+}
+
 // MutateFn is a function which mutates the existing object into it's desired state.
-type MutateFn func(existing runtime.Object) error
+type MutateFn func() error

pkg/controller/controllerutil/controllerutil_test.go

@@ -71,7 +71,7 @@ var _ = Describe("Controllerutil", func() {
 		It("should return an error if object is already owned by another controller", func() {
 			t := true
 			rsOwners := []metav1.OwnerReference{
-				metav1.OwnerReference{
+				{
 					Name:               "bar",
 					Kind:               "Deployment",
 					APIVersion:         "extensions/v1beta1",
@@ -93,7 +93,7 @@ var _ = Describe("Controllerutil", func() {
 			f := false
 			t := true
 			rsOwners := []metav1.OwnerReference{
-				metav1.OwnerReference{
+				{
 					Name:               "foo",
 					Kind:               "Deployment",
 					APIVersion:         "extensions/v1beta1",
@@ -121,6 +121,7 @@ var _ = Describe("Controllerutil", func() {
 		var deploy *appsv1.Deployment
 		var deplSpec appsv1.DeploymentSpec
 		var deplKey types.NamespacedName
+		var specr controllerutil.MutateFn
 
 		BeforeEach(func() {
 			deploy = &appsv1.Deployment{
@@ -151,94 +152,111 @@ var _ = Describe("Controllerutil", func() {
 				},
 			}
 
-			deploy.Spec = deplSpec
-
 			deplKey = types.NamespacedName{
 				Name:      deploy.Name,
 				Namespace: deploy.Namespace,
 			}
+
+			specr = deploymentSpecr(deploy, deplSpec)
 		})
 
 		It("creates a new object if one doesn't exists", func() {
-			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec))
-
-			By("returning OperationResultCreated")
-			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
 
 			By("returning no error")
 			Expect(err).NotTo(HaveOccurred())
 
+			By("returning OperationResultCreated")
+			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
+
 			By("actually having the deployment created")
 			fetched := &appsv1.Deployment{}
 			Expect(c.Get(context.TODO(), deplKey, fetched)).To(Succeed())
+
+			By("being mutated by MutateFn")
+			Expect(fetched.Spec.Template.Spec.Containers).To(HaveLen(1))
+			Expect(fetched.Spec.Template.Spec.Containers[0].Name).To(Equal(deplSpec.Template.Spec.Containers[0].Name))
+			Expect(fetched.Spec.Template.Spec.Containers[0].Image).To(Equal(deplSpec.Template.Spec.Containers[0].Image))
 		})
 
 		It("updates existing object", func() {
 			var scale int32 = 2
-			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec))
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
 
-			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentScaler(scale))
-			By("returning OperationResultUpdated")
-			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultUpdated))
-
+			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentScaler(deploy, scale))
 			By("returning no error")
 			Expect(err).NotTo(HaveOccurred())
 
+			By("returning OperationResultUpdated")
+			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultUpdated))
+
 			By("actually having the deployment scaled")
 			fetched := &appsv1.Deployment{}
 			Expect(c.Get(context.TODO(), deplKey, fetched)).To(Succeed())
 			Expect(*fetched.Spec.Replicas).To(Equal(scale))
 		})
 
 		It("updates only changed objects", func() {
-			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec))
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
 
 			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
 			Expect(err).NotTo(HaveOccurred())
 
 			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentIdentity)
+			By("returning no error")
+			Expect(err).NotTo(HaveOccurred())
 
 			By("returning OperationResultNone")
 			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
+		})
 
-			By("returning no error")
-			Expect(err).NotTo(HaveOccurred())
+		It("errors when MutateFn changes objct name on creation", func() {
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, func() error {
+				specr()
+				return deploymentRenamer(deploy)()
+			})
+
+			By("returning error")
+			Expect(err).To(HaveOccurred())
+
+			By("returning OperationResultNone")
+			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
 		})
 
-		It("errors when reconcile renames an object", func() {
-			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec))
+		It("errors when MutateFn renames an object", func() {
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
 
 			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
 			Expect(err).NotTo(HaveOccurred())
 
-			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentRenamer)
-
-			By("returning OperationResultNone")
-			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
+			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentRenamer(deploy))
 
 			By("returning error")
 			Expect(err).To(HaveOccurred())
+
+			By("returning OperationResultNone")
+			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
 		})
 
 		It("errors when object namespace changes", func() {
-			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentSpecr(deplSpec))
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
 
 			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
 			Expect(err).NotTo(HaveOccurred())
 
-			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentNamespaceChanger)
-
-			By("returning OperationResultNone")
-			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
+			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentNamespaceChanger(deploy))
 
 			By("returning error")
 			Expect(err).To(HaveOccurred())
+
+			By("returning OperationResultNone")
+			Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
 		})
 
 		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 {
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), errorReader{c}, deploy, func() error {
 				Fail("Mutation method should not run")
 				return nil
 			})
@@ -255,33 +273,34 @@ type errMetaObj struct {
 	metav1.ObjectMeta
 }
 
-func deploymentSpecr(spec appsv1.DeploymentSpec) controllerutil.MutateFn {
-	return func(obj runtime.Object) error {
-		deploy := obj.(*appsv1.Deployment)
+func deploymentSpecr(deploy *appsv1.Deployment, spec appsv1.DeploymentSpec) controllerutil.MutateFn {
+	return func() error {
 		deploy.Spec = spec
 		return nil
 	}
 }
 
-var deploymentIdentity controllerutil.MutateFn = func(obj runtime.Object) error {
+var deploymentIdentity controllerutil.MutateFn = func() error {
 	return nil
 }
 
-var deploymentRenamer controllerutil.MutateFn = func(obj runtime.Object) error {
-	deploy := obj.(*appsv1.Deployment)
-	deploy.Name = fmt.Sprintf("%s-1", deploy.Name)
-	return nil
+func deploymentRenamer(deploy *appsv1.Deployment) controllerutil.MutateFn {
+	return func() error {
+		deploy.Name = fmt.Sprintf("%s-1", deploy.Name)
+		return nil
+	}
 }
 
-var deploymentNamespaceChanger controllerutil.MutateFn = func(obj runtime.Object) error {
-	deploy := obj.(*appsv1.Deployment)
-	deploy.Namespace = fmt.Sprintf("%s-1", deploy.Namespace)
-	return nil
+func deploymentNamespaceChanger(deploy *appsv1.Deployment) controllerutil.MutateFn {
+	return func() error {
+		deploy.Namespace = fmt.Sprintf("%s-1", deploy.Namespace)
+		return nil
+
+	}
 }
 
-func deploymentScaler(replicas int32) controllerutil.MutateFn {
-	fn := func(obj runtime.Object) error {
-		deploy := obj.(*appsv1.Deployment)
+func deploymentScaler(deploy *appsv1.Deployment, replicas int32) controllerutil.MutateFn {
+	fn := func() error {
 		deploy.Spec.Replicas = &replicas
 		return nil
 	}

pkg/controller/controllerutil/example_test.go

@@ -22,7 +22,6 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -37,10 +36,9 @@ func ExampleCreateOrUpdate() {
 	// c is client.Client
 
 	// Create or Update the deployment default/foo
-	deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
+	deploy := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
 
-	op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deployment, func(existing runtime.Object) error {
-		deploy := existing.(*appsv1.Deployment)
+	op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, func() error {
 
 		// Deployment selector is immutable so we set this value only if
 		// a new object is going to be created
@@ -59,7 +57,7 @@ func ExampleCreateOrUpdate() {
 			},
 			Spec: corev1.PodSpec{
 				Containers: []corev1.Container{
-					corev1.Container{
+					{
 						Name:  "busybox",
 						Image: "busybox",
 					},