Skip to content

Commit

Permalink
Remove obj parameter from MutateFn
Browse files Browse the repository at this point in the history
  • Loading branch information
@calind
calind committed Feb 22, 2019
1 parent adbc7af commit 3683b64
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 54 deletions.
40 changes: 19 additions & 21 deletions pkg/controller/controllerutil/controllerutil.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,47 +114,45 @@ 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
}

if reflect.DeepEqual(existing, obj) {
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")
}

Expand All @@ -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
73 changes: 45 additions & 28 deletions pkg/controller/controllerutil/controllerutil_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -93,7 +93,7 @@ var _ = Describe("Controllerutil", func() {
f := false
t := true
rsOwners := []metav1.OwnerReference{
metav1.OwnerReference{
{
Name: "foo",
Kind: "Deployment",
APIVersion: "extensions/v1beta1",
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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())
Expand All @@ -170,19 +173,19 @@ 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))
})

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())

Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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
})
Expand All @@ -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
}
Expand Down
8 changes: 3 additions & 5 deletions pkg/controller/controllerutil/example_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -59,7 +57,7 @@ func ExampleCreateOrUpdate() {
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
corev1.Container{
{
Name: "busybox",
Image: "busybox",
},
Expand Down

0 comments on commit 3683b64

Please sign in to comment.