From 3683b64c616f87457d32cced65022cb24a4ef258 Mon Sep 17 00:00:00 2001 From: Calin Don Date: Fri, 15 Feb 2019 21:30:17 +0200 Subject: [PATCH] Remove obj parameter from MutateFn --- .../controllerutil/controllerutil.go | 40 +++++----- .../controllerutil/controllerutil_test.go | 73 ++++++++++++------- pkg/controller/controllerutil/example_test.go | 8 +- 3 files changed, 67 insertions(+), 54 deletions(-) diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index 14252f1b4d..ac0c2207b6 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -114,35 +114,37 @@ 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 MutateFn. obj is just 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) { // nolint: gocyclo +func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) { key, err := client.ObjectKeyFromObject(obj) if err != nil { return OperationResultNone, err } if err := c.Get(ctx, key, obj); err != nil { - if errors.IsNotFound(err) { - if err := f(obj); err != nil { - return OperationResultNone, 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 := f(); err != nil { + return OperationResultNone, err + } + if newKey, err := client.ObjectKeyFromObject(obj); err != nil || key != newKey { + return OperationResultNone, fmt.Errorf("MutateFn cannot mutate object namespace and/or object name") + } + 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 := f(); err != nil { return OperationResultNone, err } @@ -150,11 +152,7 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f return OperationResultNone, nil } - newKey, err := client.ObjectKeyFromObject(obj) - if err != nil { - return OperationResultNone, err - } - if key != newKey { + if newKey, err := client.ObjectKeyFromObject(obj); err != nil || key != newKey { return OperationResultNone, fmt.Errorf("MutateFn cannot mutate object namespace and/or object name") } @@ -165,4 +163,4 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f } // 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 diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index 851d02c3be..15fed67f60 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/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{ @@ -155,10 +156,12 @@ var _ = Describe("Controllerutil", func() { 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)) + op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr) By("returning no error") Expect(err).NotTo(HaveOccurred()) @@ -170,7 +173,7 @@ var _ = Describe("Controllerutil", func() { fetched := &appsv1.Deployment{} Expect(c.Get(context.TODO(), deplKey, fetched)).To(Succeed()) - By("applying MutateFn") + 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)) @@ -178,11 +181,11 @@ var _ = Describe("Controllerutil", func() { 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)) + op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentScaler(deploy, scale)) By("returning no error") Expect(err).NotTo(HaveOccurred()) @@ -196,7 +199,7 @@ var _ = Describe("Controllerutil", func() { }) 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()) @@ -209,13 +212,26 @@ var _ = Describe("Controllerutil", func() { 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 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 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) + op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentRenamer(deploy)) By("returning error") Expect(err).To(HaveOccurred()) @@ -225,12 +241,12 @@ var _ = Describe("Controllerutil", func() { }) 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) + op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, deploymentNamespaceChanger(deploy)) By("returning error") Expect(err).To(HaveOccurred()) @@ -240,7 +256,7 @@ var _ = Describe("Controllerutil", func() { }) 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 }) @@ -257,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 } diff --git a/pkg/controller/controllerutil/example_test.go b/pkg/controller/controllerutil/example_test.go index f340f623fc..7670dd1852 100644 --- a/pkg/controller/controllerutil/example_test.go +++ b/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", },