Skip to content

Commit 6ca7808

Browse files
varshaprasad96varshaprasad96
committed
Address review comments
1 parent fca0592 commit 6ca7808

File tree

2 files changed

+101
-74
lines changed

2 files changed

+101
-74
lines changed

‎pkg/client/namespaced_client.go

Lines changed: 59 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package client
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223

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

8687
if scope == "" {
87-
return false, fmt.Errorf("Scope cannot be identified. Empty scope returned")
88+
return false, errors.New("Scope cannot be identified. Empty scope returned")
8889
}
8990

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

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

103-
isNamespaceScoped, err := isNamespaced(n.client, obj)
104-
if err != nil {
105-
return fmt.Errorf("error finding the scope of the object %v", err)
104+
objectNamespace := obj.GetNamespace()
105+
if objectNamespace != n.namespace && objectNamespace != "" {
106+
return fmt.Errorf("Namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
106107
}
107-
if isNamespaceScoped {
108-
metaObj.SetNamespace(n.namespace)
108+
109+
if isNamespaceScoped && objectNamespace == "" {
110+
obj.SetNamespace(n.namespace)
109111
}
110112
return n.client.Create(ctx, obj, opts...)
111113
}
112114

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

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

125-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
126-
metaObj.SetNamespace(n.namespace)
127+
if isNamespaceScoped && objectNamespace == "" {
128+
obj.SetNamespace(n.namespace)
127129
}
128130
return n.client.Update(ctx, obj, opts...)
129131
}
130132

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

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

143-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
144-
metaObj.SetNamespace(n.namespace)
145+
if isNamespaceScoped && objectNamespace == "" {
146+
obj.SetNamespace(n.namespace)
145147
}
146148
return n.client.Delete(ctx, obj, opts...)
147149
}
148150

149151
// DeleteAllOf implements client.Client
150-
func (n *namespacedClient) DeleteAllOf(ctx context.Context, obj runtime.Object, opts ...DeleteAllOfOption) error {
152+
func (n *namespacedClient) DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllOfOption) error {
151153
isNamespaceScoped, err := isNamespaced(n.client, obj)
152154
if err != nil {
153-
return fmt.Errorf("error finding the scope of the object %v", err)
155+
return fmt.Errorf("error finding the scope of the object: %v", err)
154156
}
155157

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

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

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

174-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
175-
metaObj.SetNamespace(n.namespace)
176+
if isNamespaceScoped && objectNamespace == "" {
177+
obj.SetNamespace(n.namespace)
176178
}
177179
return n.client.Patch(ctx, obj, patch, opts...)
178180
}
179181

180182
// Get implements client.Client
181-
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj runtime.Object) error {
183+
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
182184
isNamespaceScoped, err := isNamespaced(n.client, obj)
183185
if err != nil {
184-
return fmt.Errorf("error finding the scope of the object %v", err)
186+
return fmt.Errorf("error finding the scope of the object: %v", err)
185187
}
186188
if isNamespaceScoped {
189+
if key.Namespace != "" && key.Namespace != n.namespace {
190+
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)
191+
}
187192
key.Namespace = n.namespace
188193
}
189194
return n.client.Get(ctx, key, obj)
190195
}
191196

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

214219
// Update implements client.StatusWriter
215-
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
216-
metaObj, err := meta.Accessor(obj)
220+
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
221+
isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
217222
if err != nil {
218-
return err
223+
return fmt.Errorf("error finding the scope of the object: %v", err)
219224
}
220225

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

226-
if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
227-
metaObj.SetNamespace(nsw.namespace)
231+
if isNamespaceScoped && objectNamespace == "" {
232+
obj.SetNamespace(nsw.namespace)
228233
}
229234
return nsw.StatusClient.Update(ctx, obj, opts...)
230235
}
231236

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

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

244-
if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
245-
metaObj.SetNamespace(nsw.namespace)
249+
if isNamespaceScoped && objectNamespace == "" {
250+
obj.SetNamespace(nsw.namespace)
246251
}
247252
return nsw.StatusClient.Patch(ctx, obj, patch, opts...)
248253
}

‎pkg/client/namespaced_client_test.go

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,12 @@ var _ = Describe("NamespacedClient", func() {
9696
Expect(result).To(BeEquivalentTo(dep))
9797
})
9898

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

103-
Expect(getClient().Get(ctx, name, result)).NotTo(HaveOccurred())
104-
Expect(result).To(BeEquivalentTo(dep))
104+
Expect(getClient().Get(ctx, name, result)).To(HaveOccurred())
105105
})
106106
})
107107

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

154-
It("should create an object in the namespace specified with the client", func() {
154+
It("should not create object if the namespace of the object is different", func() {
155155
By("creating the object initially")
156-
156+
dep.SetNamespace("non-default")
157157
err := getClient().Create(ctx, dep)
158-
Expect(err).NotTo(HaveOccurred())
159-
160-
By("checking if the object was created in the right namespace")
161-
res, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
162-
Expect(err).NotTo(HaveOccurred())
163-
Expect(res.GetNamespace()).To(BeEquivalentTo(ns))
158+
Expect(err).To(HaveOccurred())
164159
})
165160
It("should create a cluster scoped object", func() {
166161
cr := &rbacv1.ClusterRole{
@@ -169,7 +164,6 @@ var _ = Describe("NamespacedClient", func() {
169164
Labels: map[string]string{"name": fmt.Sprintf("clusterRole-%v", count)},
170165
},
171166
}
172-
173167
cr.SetGroupVersionKind(schema.GroupVersionKind{
174168
Group: "rbac.authorization.k8s.io",
175169
Version: "v1",
@@ -193,8 +187,8 @@ var _ = Describe("NamespacedClient", func() {
193187
Describe("Update", func() {
194188
var err error
195189
BeforeEach(func() {
196-
dep.Annotations = map[string]string{"foo": "bar"}
197190
dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
191+
dep.Annotations = map[string]string{"foo": "bar"}
198192
Expect(err).NotTo(HaveOccurred())
199193
})
200194
AfterEach(func() {
@@ -214,9 +208,9 @@ var _ = Describe("NamespacedClient", func() {
214208
Expect(actual.Annotations["foo"]).To(Equal("bar"))
215209
})
216210

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

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

225+
It("should not update when object namespace is different", func() {
226+
By("updating the Deployment")
227+
dep.SetNamespace("non-default")
228+
err = getClient().Update(ctx, dep)
229+
Expect(err).To(HaveOccurred())
230+
})
231+
231232
It("should not update any object from other namespace", func() {
232233
By("creating a new namespace")
233234
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-1"}}
@@ -321,9 +322,10 @@ var _ = Describe("NamespacedClient", func() {
321322
Expect(actual.Annotations["foo"]).To(Equal("bar"))
322323
Expect(actual.GetNamespace()).To(Equal(ns))
323324
})
324-
It("should successfully modify the object with namespace specified in the client", func() {
325-
dep.SetNamespace("non-default")
325+
326+
It("should successfully modify the object using Patch when namespace is not provided", func() {
326327
By("Applying Patch")
328+
dep.SetNamespace("")
327329
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
328330
Expect(err).NotTo(HaveOccurred())
329331

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

339+
It("should not modify the object when namespace of the object is different", func() {
340+
dep.SetNamespace("non-default")
341+
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
342+
Expect(err).To(HaveOccurred())
343+
})
344+
337345
It("should not modify an object from a different namespace", func() {
338346
By("creating a new namespace")
339347
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-2"}}
@@ -485,7 +493,6 @@ var _ = Describe("NamespacedClient", func() {
485493

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

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

507+
It("should not change objects via update status when object namespace is different", func() {
508+
changedDep := dep.DeepCopy()
509+
changedDep.SetNamespace("test")
510+
changedDep.Status.Replicas = 99
511+
512+
Expect(getClient().Status().Update(ctx, changedDep)).To(HaveOccurred())
513+
})
514+
500515
It("should change objects via status patch", func() {
501516
changedDep := dep.DeepCopy()
502517
changedDep.Status.Replicas = 99
503-
changedDep.SetNamespace("test")
504518

505-
Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).ToNot(HaveOccurred())
519+
Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).NotTo(HaveOccurred())
506520

507521
actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
508522
Expect(err).NotTo(HaveOccurred())
509523
Expect(actual).NotTo(BeNil())
510524
Expect(actual.GetNamespace()).To(BeEquivalentTo(ns))
511525
Expect(actual.Status.Replicas).To(BeEquivalentTo(99))
512526
})
527+
528+
It("should not change objects via status patch when object namespace is different", func() {
529+
changedDep := dep.DeepCopy()
530+
changedDep.Status.Replicas = 99
531+
changedDep.SetNamespace("test")
532+
533+
Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).To(HaveOccurred())
534+
})
513535
})
514536

515537
Describe("Test on invalid objects", func() {

0 commit comments

Comments
 (0)