Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@varshaprasad96
varshaprasad96 committed Oct 21, 2020
1 parent fca0592 commit 6ca7808
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 74 deletions.
113 changes: 59 additions & 54 deletions pkg/client/namespaced_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package client

import (
"context"
"errors"
"fmt"

"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -84,7 +85,7 @@ func isNamespaced(c Client, obj runtime.Object) (bool, error) {
scope := restmapping.Scope.Name()

if scope == "" {
return false, fmt.Errorf("Scope cannot be identified. Empty scope returned")
return false, errors.New("Scope cannot be identified. Empty scope returned")
}

if scope != meta.RESTScopeNameRoot {
Expand All @@ -94,63 +95,64 @@ func isNamespaced(c Client, obj runtime.Object) (bool, error) {
}

// Create implements clinet.Client
func (n *namespacedClient) Create(ctx context.Context, obj runtime.Object, opts ...CreateOption) error {
metaObj, err := meta.Accessor(obj)
func (n *namespacedClient) Create(ctx context.Context, obj Object, opts ...CreateOption) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != n.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
}
if isNamespaceScoped {
metaObj.SetNamespace(n.namespace)

if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(n.namespace)
}
return n.client.Create(ctx, obj, opts...)
}

// Update implements client.Client
func (n *namespacedClient) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
metaObj, err := meta.Accessor(obj)
func (n *namespacedClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != n.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
}

if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
metaObj.SetNamespace(n.namespace)
if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(n.namespace)
}
return n.client.Update(ctx, obj, opts...)
}

// Delete implements client.Client
func (n *namespacedClient) Delete(ctx context.Context, obj runtime.Object, opts ...DeleteOption) error {
metaObj, err := meta.Accessor(obj)
func (n *namespacedClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != n.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
}

if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
metaObj.SetNamespace(n.namespace)
if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(n.namespace)
}
return n.client.Delete(ctx, obj, opts...)
}

// DeleteAllOf implements client.Client
func (n *namespacedClient) DeleteAllOf(ctx context.Context, obj runtime.Object, opts ...DeleteAllOfOption) error {
func (n *namespacedClient) DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllOfOption) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
return fmt.Errorf("error finding the scope of the object: %v", err)
}

if isNamespaceScoped {
Expand All @@ -160,37 +162,40 @@ func (n *namespacedClient) DeleteAllOf(ctx context.Context, obj runtime.Object,
}

// Patch implements client.Client
func (n *namespacedClient) Patch(ctx context.Context, obj runtime.Object, patch Patch, opts ...PatchOption) error {
metaObj, err := meta.Accessor(obj)
func (n *namespacedClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != n.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
}

if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
metaObj.SetNamespace(n.namespace)
if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(n.namespace)
}
return n.client.Patch(ctx, obj, patch, opts...)
}

// Get implements client.Client
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj runtime.Object) error {
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
isNamespaceScoped, err := isNamespaced(n.client, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
return fmt.Errorf("error finding the scope of the object: %v", err)
}
if isNamespaceScoped {
if key.Namespace != "" && key.Namespace != n.namespace {
return fmt.Errorf("Namespace %s provided for the object %s does not match the namesapce %s on the client", key.Namespace, obj.GetName(), n.namespace)
}
key.Namespace = n.namespace
}
return n.client.Get(ctx, key, obj)
}

// List implements client.Client
func (n *namespacedClient) List(ctx context.Context, obj runtime.Object, opts ...ListOption) error {
func (n *namespacedClient) List(ctx context.Context, obj ObjectList, opts ...ListOption) error {
if n.namespace != "" {
opts = append(opts, InNamespace(n.namespace))
}
Expand All @@ -212,37 +217,37 @@ type namespacedClientStatusWriter struct {
}

// Update implements client.StatusWriter
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
metaObj, err := meta.Accessor(obj)
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != nsw.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), nsw.namespace)
}

if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
metaObj.SetNamespace(nsw.namespace)
if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(nsw.namespace)
}
return nsw.StatusClient.Update(ctx, obj, opts...)
}

// Patch implements client.StatusWriter
func (nsw *namespacedClientStatusWriter) Patch(ctx context.Context, obj runtime.Object, patch Patch, opts ...PatchOption) error {
metaObj, err := meta.Accessor(obj)
func (nsw *namespacedClientStatusWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
if err != nil {
return err
return fmt.Errorf("error finding the scope of the object: %v", err)
}

isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object %v", err)
objectNamespace := obj.GetNamespace()
if objectNamespace != nsw.namespace && objectNamespace != "" {
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), nsw.namespace)
}

if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
metaObj.SetNamespace(nsw.namespace)
if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(nsw.namespace)
}
return nsw.StatusClient.Patch(ctx, obj, patch, opts...)
}
62 changes: 42 additions & 20 deletions pkg/client/namespaced_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,12 @@ var _ = Describe("NamespacedClient", func() {
Expect(result).To(BeEquivalentTo(dep))
})

It("should get the objects from namespace specified in the client", func() {
It("should error when namespace provided in the object is different than the one "+
"specified in client", func() {
name := types.NamespacedName{Name: dep.Name, Namespace: "non-default"}
result := &appsv1.Deployment{}

Expect(getClient().Get(ctx, name, result)).NotTo(HaveOccurred())
Expect(result).To(BeEquivalentTo(dep))
Expect(getClient().Get(ctx, name, result)).To(HaveOccurred())
})
})

Expand Down Expand Up @@ -151,16 +151,11 @@ var _ = Describe("NamespacedClient", func() {
Expect(res.GetNamespace()).To(BeEquivalentTo(ns))
})

It("should create an object in the namespace specified with the client", func() {
It("should not create object if the namespace of the object is different", func() {
By("creating the object initially")

dep.SetNamespace("non-default")
err := getClient().Create(ctx, dep)
Expect(err).NotTo(HaveOccurred())

By("checking if the object was created in the right namespace")
res, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(res.GetNamespace()).To(BeEquivalentTo(ns))
Expect(err).To(HaveOccurred())
})
It("should create a cluster scoped object", func() {
cr := &rbacv1.ClusterRole{
Expand All @@ -169,7 +164,6 @@ var _ = Describe("NamespacedClient", func() {
Labels: map[string]string{"name": fmt.Sprintf("clusterRole-%v", count)},
},
}

cr.SetGroupVersionKind(schema.GroupVersionKind{
Group: "rbac.authorization.k8s.io",
Version: "v1",
Expand All @@ -193,8 +187,8 @@ var _ = Describe("NamespacedClient", func() {
Describe("Update", func() {
var err error
BeforeEach(func() {
dep.Annotations = map[string]string{"foo": "bar"}
dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
dep.Annotations = map[string]string{"foo": "bar"}
Expect(err).NotTo(HaveOccurred())
})
AfterEach(func() {
Expand All @@ -214,9 +208,9 @@ var _ = Describe("NamespacedClient", func() {
Expect(actual.Annotations["foo"]).To(Equal("bar"))
})

It("should update the object in the namespace specified in the client", func() {
It("should successfully update the provided object when namespace is not provided", func() {
By("updating the Deployment")
dep.SetNamespace("non-default")
dep.SetNamespace("")
err = getClient().Update(ctx, dep)
Expect(err).NotTo(HaveOccurred())

Expand All @@ -228,6 +222,13 @@ var _ = Describe("NamespacedClient", func() {
Expect(actual.Annotations["foo"]).To(Equal("bar"))
})

It("should not update when object namespace is different", func() {
By("updating the Deployment")
dep.SetNamespace("non-default")
err = getClient().Update(ctx, dep)
Expect(err).To(HaveOccurred())
})

It("should not update any object from other namespace", func() {
By("creating a new namespace")
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-1"}}
Expand Down Expand Up @@ -321,9 +322,10 @@ var _ = Describe("NamespacedClient", func() {
Expect(actual.Annotations["foo"]).To(Equal("bar"))
Expect(actual.GetNamespace()).To(Equal(ns))
})
It("should successfully modify the object with namespace specified in the client", func() {
dep.SetNamespace("non-default")

It("should successfully modify the object using Patch when namespace is not provided", func() {
By("Applying Patch")
dep.SetNamespace("")
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
Expect(err).NotTo(HaveOccurred())

Expand All @@ -334,6 +336,12 @@ var _ = Describe("NamespacedClient", func() {
Expect(actual.GetNamespace()).To(Equal(ns))
})

It("should not modify the object when namespace of the object is different", func() {
dep.SetNamespace("non-default")
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
Expect(err).To(HaveOccurred())
})

It("should not modify an object from a different namespace", func() {
By("creating a new namespace")
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-2"}}
Expand Down Expand Up @@ -485,7 +493,6 @@ var _ = Describe("NamespacedClient", func() {

It("should change objects via update status", func() {
changedDep := dep.DeepCopy()
changedDep.SetNamespace("test")
changedDep.Status.Replicas = 99

Expect(getClient().Status().Update(ctx, changedDep)).NotTo(HaveOccurred())
Expand All @@ -497,19 +504,34 @@ var _ = Describe("NamespacedClient", func() {
Expect(actual.Status.Replicas).To(BeEquivalentTo(99))
})

It("should not change objects via update status when object namespace is different", func() {
changedDep := dep.DeepCopy()
changedDep.SetNamespace("test")
changedDep.Status.Replicas = 99

Expect(getClient().Status().Update(ctx, changedDep)).To(HaveOccurred())
})

It("should change objects via status patch", func() {
changedDep := dep.DeepCopy()
changedDep.Status.Replicas = 99
changedDep.SetNamespace("test")

Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).ToNot(HaveOccurred())
Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).NotTo(HaveOccurred())

actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(actual).NotTo(BeNil())
Expect(actual.GetNamespace()).To(BeEquivalentTo(ns))
Expect(actual.Status.Replicas).To(BeEquivalentTo(99))
})

It("should not change objects via status patch when object namespace is different", func() {
changedDep := dep.DeepCopy()
changedDep.Status.Replicas = 99
changedDep.SetNamespace("test")

Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).To(HaveOccurred())
})
})

Describe("Test on invalid objects", func() {
Expand Down

0 comments on commit 6ca7808

Please sign in to comment.