PR review round 1

Author: calind

pkg/controller/controllerutil/controllerutil.go

@@ -114,43 +114,62 @@ const ( // They should complete the sentence "Deployment default/foo has been ..
 	OperationUpdated = "updated"
 )
 
-// CreateOrUpdate creates or updates a kuberenes resource. It takes in a key and
-// a placeholder for the existing object and returns the modified object
-func CreateOrUpdate(ctx context.Context, c client.Client, key client.ObjectKey, existing runtime.Object, t TransformFn) (runtime.Object, OperationType, error) {
+// CreateOrUpdate creates or updates a kubernetes resource. It takes in a key and
+// a placeholder for the existing object and returns the operation executed
+func CreateOrUpdate(ctx context.Context, c client.Client, key client.ObjectKey, existing runtime.Object, t TransformFn) (OperationType, error) {
 	err := c.Get(ctx, key, existing)
 	var obj runtime.Object
 
 	if errors.IsNotFound(err) {
-		obj, err = t(existing)
+		// Create a new zero value object so that the in parameter of
+		// TransformFn is always a "clean" object, with only Name and Namespace
+		// set
+		zero := reflect.New(reflect.TypeOf(existing).Elem()).Interface()
+
+		// Set Namespace and Name from the lookup key
+		zmeta, ok := zero.(v1.Object)
+		if !ok {
+			return OperationNoop, fmt.Errorf("is not a %T a metav1.Object, cannot call CreateOrUpdate", zero)
+		}
+		zmeta.SetNamespace(key.Namespace)
+		zmeta.SetName(key.Name)
+
+		// Apply the TransformFn
+		obj, err = t(zero.(runtime.Object))
 		if err != nil {
-			return nil, OperationNoop, err
+			return OperationNoop, err
 		}
 
+		// Create the new object
 		err = c.Create(ctx, obj)
-
 		if err != nil {
-			return nil, OperationNoop, err
+			return OperationNoop, err
 		}
-		return obj, OperationCreated, err
+
+		return OperationCreated, err
 	} else if err != nil {
-		return nil, OperationNoop, err
+		return OperationNoop, err
 	} else {
 		obj, err = t(existing.DeepCopyObject())
 		if err != nil {
-			return nil, OperationNoop, err
+			return OperationNoop, err
 		}
 
 		if !reflect.DeepEqual(existing, obj) {
 			err = c.Update(ctx, obj)
 			if err != nil {
-				return nil, OperationNoop, err
+				return OperationNoop, err
 			}
-			return obj, OperationUpdated, err
+
+			return OperationUpdated, err
 		}
-		return obj, OperationNoop, nil
+
+		return OperationNoop, nil
 	}
 }
 
 // TransformFn is a function which take in a kubernetes object and returns the
-// desired state of that object
+// desired state of that object.
+// It is safe to mutate the object inside this function, since it's always
+// called with an object's deep copy.
 type TransformFn func(in runtime.Object) (runtime.Object, error)

pkg/controller/controllerutil/controllerutil_suite_test.go

@@ -5,9 +5,33 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
 )
 
 func TestControllerutil(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Controllerutil Suite")
 }
+
+var t *envtest.Environment
+var cfg *rest.Config
+var c client.Client
+
+var _ = BeforeSuite(func() {
+	var err error
+
+	t = &envtest.Environment{}
+
+	cfg, err = t.Start()
+	Expect(err).NotTo(HaveOccurred())
+
+	c, err = client.New(cfg, client.Options{})
+	Expect(err).NotTo(HaveOccurred())
+})
+
+var _ = AfterSuite(func() {
+	t.Stop()
+})

pkg/controller/controllerutil/controllerutil_test.go

@@ -1,12 +1,16 @@
 package controllerutil_test
 
 import (
+	"context"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
@@ -92,10 +96,94 @@ var _ = Describe("Controllerutil", func() {
 			}))
 		})
 	})
+
+	Describe("CreateOrUpdate", func() {
+
+		It("creates a new object if one doesn't exists", func() {
+			deplKey := types.NamespacedName{Name: "test-create", Namespace: "default"}
+			depl := &appsv1.Deployment{}
+
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deplKey, depl, createDeployment)
+
+			By("returning OperationCreated")
+			Expect(op).Should(BeEquivalentTo(controllerutil.OperationCreated))
+
+			By("returning returning no error")
+			Expect(err).ShouldNot(HaveOccurred())
+
+			By("actually having the deployment created")
+			fetched := &appsv1.Deployment{}
+			Expect(c.Get(context.TODO(), deplKey, fetched)).Should(Succeed())
+		})
+
+		It("update existing object", func() {
+			deplKey := types.NamespacedName{Name: "test-update", Namespace: "default"}
+			d, _ := createDeployment(&appsv1.Deployment{})
+			depl := d.(*appsv1.Deployment)
+			depl.Name = "test-update"
+			depl.Namespace = "default"
+
+			var scale int32 = 2
+
+			Expect(c.Create(context.TODO(), depl)).Should(Succeed())
+
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deplKey, &appsv1.Deployment{}, deploymentScaler(scale))
+
+			By("returning OperationUpdated")
+			Expect(op).Should(BeEquivalentTo(controllerutil.OperationUpdated))
+
+			By("returning returning no error")
+			Expect(err).ShouldNot(HaveOccurred())
+
+			By("actually having the deployment scaled")
+			fetched := &appsv1.Deployment{}
+			Expect(c.Get(context.TODO(), deplKey, fetched)).Should(Succeed())
+			Expect(*fetched.Spec.Replicas).To(Equal(scale))
+		})
+
+		It("updates only changed objects", func() {
+			deplKey := types.NamespacedName{Name: "test-idempotency", Namespace: "default"}
+			depl := &appsv1.Deployment{}
+
+			op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deplKey, depl, createDeployment)
+			Expect(op).Should(BeEquivalentTo(controllerutil.OperationCreated))
+			Expect(err).ShouldNot(HaveOccurred())
+
+			op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deplKey, depl, deploymentIdentity)
+
+			By("returning OperationNoop")
+			Expect(op).Should(BeEquivalentTo(controllerutil.OperationNoop))
+
+			By("returning returning no error")
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+	})
 })
 
 var _ metav1.Object = &errMetaObj{}
 
 type errMetaObj struct {
 	metav1.ObjectMeta
 }
+
+var createDeployment controllerutil.TransformFn = func(in runtime.Object) (runtime.Object, error) {
+	out := in.(*appsv1.Deployment)
+	out.Spec.Selector = &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}
+	out.Spec.Template.ObjectMeta.Labels = map[string]string{"foo": "bar"}
+	out.Spec.Template.Spec.Containers = []corev1.Container{corev1.Container{Name: "foo", Image: "busybox"}}
+	return out, nil
+}
+
+var deploymentIdentity controllerutil.TransformFn = func(in runtime.Object) (runtime.Object, error) {
+	return in, nil
+}
+
+func deploymentScaler(replicas int32) controllerutil.TransformFn {
+	fn := func(in runtime.Object) (runtime.Object, error) {
+		d, _ := createDeployment(in)
+		out := d.(*appsv1.Deployment)
+		out.Spec.Replicas = &replicas
+		return out, nil
+	}
+	return fn
+}