Skip to content

Commit 1700206

Browse files
varshaprasad96varshaprasad96
committed
Address review comments
1 parent 8185464 commit 1700206

File tree

2 files changed

+97
-59
lines changed

2 files changed

+97
-59
lines changed

‎pkg/client/namespaced_client.go

Lines changed: 54 additions & 52 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,60 +95,61 @@ 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)
99-
if err != nil {
100-
return err
101-
}
102-
98+
func (n *namespacedClient) Create(ctx context.Context, obj Object, opts ...CreateOption) error {
10399
isNamespaceScoped, err := isNamespaced(n.client, obj)
104100
if err != nil {
105101
return fmt.Errorf("error finding the scope of the object %v", err)
106102
}
107-
if isNamespaceScoped {
108-
metaObj.SetNamespace(n.namespace)
103+
104+
objectNamespace := obj.GetNamespace()
105+
if objectNamespace != n.namespace && objectNamespace != "" {
106+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), n.namespace)
107+
}
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-
if err != nil {
117-
return err
118-
}
119-
116+
func (n *namespacedClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
120117
isNamespaceScoped, err := isNamespaced(n.client, obj)
121118
if err != nil {
122119
return fmt.Errorf("error finding the scope of the object %v", err)
123120
}
124121

125-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
126-
metaObj.SetNamespace(n.namespace)
122+
objectNamespace := obj.GetNamespace()
123+
if objectNamespace != n.namespace && objectNamespace != "" {
124+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), n.namespace)
125+
}
126+
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-
if err != nil {
135-
return err
136-
}
137-
134+
func (n *namespacedClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error {
138135
isNamespaceScoped, err := isNamespaced(n.client, obj)
139136
if err != nil {
140137
return fmt.Errorf("error finding the scope of the object %v", err)
141138
}
142139

143-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
144-
metaObj.SetNamespace(n.namespace)
140+
objectNamespace := obj.GetNamespace()
141+
if objectNamespace != n.namespace && objectNamespace != "" {
142+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), n.namespace)
143+
}
144+
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 {
153155
return fmt.Errorf("error finding the scope of the object %v", err)
@@ -160,25 +162,25 @@ 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-
if err != nil {
166-
return err
167-
}
168-
165+
func (n *namespacedClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
169166
isNamespaceScoped, err := isNamespaced(n.client, obj)
170167
if err != nil {
171168
return fmt.Errorf("error finding the scope of the object %v", err)
172169
}
173170

174-
if isNamespaceScoped && metaObj.GetNamespace() != n.namespace {
175-
metaObj.SetNamespace(n.namespace)
171+
objectNamespace := obj.GetNamespace()
172+
if objectNamespace != n.namespace && objectNamespace != "" {
173+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), n.namespace)
174+
}
175+
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 {
184186
return fmt.Errorf("error finding the scope of the object %v", err)
@@ -190,7 +192,7 @@ func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj runtime.O
190192
}
191193

192194
// List implements client.Client
193-
func (n *namespacedClient) List(ctx context.Context, obj runtime.Object, opts ...ListOption) error {
195+
func (n *namespacedClient) List(ctx context.Context, obj ObjectList, opts ...ListOption) error {
194196
if n.namespace != "" {
195197
opts = append(opts, InNamespace(n.namespace))
196198
}
@@ -212,37 +214,37 @@ type namespacedClientStatusWriter struct {
212214
}
213215

214216
// Update implements client.StatusWriter
215-
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
216-
metaObj, err := meta.Accessor(obj)
217-
if err != nil {
218-
return err
219-
}
220-
217+
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
221218
isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
222219
if err != nil {
223220
return fmt.Errorf("error finding the scope of the object %v", err)
224221
}
225222

226-
if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
227-
metaObj.SetNamespace(nsw.namespace)
223+
objectNamespace := obj.GetNamespace()
224+
if objectNamespace != nsw.namespace && objectNamespace != "" {
225+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), nsw.namespace)
226+
}
227+
228+
if isNamespaceScoped && objectNamespace == "" {
229+
obj.SetNamespace(nsw.namespace)
228230
}
229231
return nsw.StatusClient.Update(ctx, obj, opts...)
230232
}
231233

232234
// 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)
235-
if err != nil {
236-
return err
237-
}
238-
235+
func (nsw *namespacedClientStatusWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
239236
isNamespaceScoped, err := isNamespaced(nsw.namespacedclient, obj)
240237
if err != nil {
241238
return fmt.Errorf("error finding the scope of the object %v", err)
242239
}
243240

244-
if isNamespaceScoped && metaObj.GetNamespace() != nsw.namespace {
245-
metaObj.SetNamespace(nsw.namespace)
241+
objectNamespace := obj.GetNamespace()
242+
if objectNamespace != nsw.namespace && objectNamespace != "" {
243+
return fmt.Errorf("Namespace %s of the object %s does not match %s", objectNamespace, obj.GetName(), nsw.namespace)
244+
}
245+
246+
if isNamespaceScoped && objectNamespace == "" {
247+
obj.SetNamespace(nsw.namespace)
246248
}
247249
return nsw.StatusClient.Patch(ctx, obj, patch, opts...)
248250
}

‎pkg/client/namespaced_client_test.go

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,13 @@ var _ = Describe("NamespacedClient", func() {
151151
Expect(res.GetNamespace()).To(BeEquivalentTo(ns))
152152
})
153153

154+
It("should not create object if the namespace of the object is different", func() {
155+
By("creating the object initially")
156+
dep.SetNamespace("non-default")
157+
err := getClient().Create(ctx, dep)
158+
Expect(err).To(HaveOccurred())
159+
})
160+
154161
It("should create an object in the namespace specified with the client", func() {
155162
By("creating the object initially")
156163

@@ -214,9 +221,9 @@ var _ = Describe("NamespacedClient", func() {
214221
Expect(actual.Annotations["foo"]).To(Equal("bar"))
215222
})
216223

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

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

238+
It("should not update when object namespace is different", func() {
239+
By("updating the Deployment")
240+
dep.SetNamespace("non-default")
241+
err = getClient().Update(ctx, dep)
242+
Expect(err).To(HaveOccurred())
243+
})
244+
231245
It("should not update any object from other namespace", func() {
232246
By("creating a new namespace")
233247
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-1"}}
@@ -321,9 +335,10 @@ var _ = Describe("NamespacedClient", func() {
321335
Expect(actual.Annotations["foo"]).To(Equal("bar"))
322336
Expect(actual.GetNamespace()).To(Equal(ns))
323337
})
324-
It("should successfully modify the object with namespace specified in the client", func() {
325-
dep.SetNamespace("non-default")
338+
339+
It("should successfully modify the object using Patch when namespace is not provided", func() {
326340
By("Applying Patch")
341+
dep.SetNamespace("")
327342
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
328343
Expect(err).NotTo(HaveOccurred())
329344

@@ -334,6 +349,13 @@ var _ = Describe("NamespacedClient", func() {
334349
Expect(actual.GetNamespace()).To(Equal(ns))
335350
})
336351

352+
It("should not modify the object when namespace of the object is different", func() {
353+
dep.SetNamespace("non-default")
354+
By("Applying Patch")
355+
err = getClient().Patch(ctx, dep, client.RawPatch(types.MergePatchType, generatePatch()))
356+
Expect(err).To(HaveOccurred())
357+
})
358+
337359
It("should not modify an object from a different namespace", func() {
338360
By("creating a new namespace")
339361
tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "non-default-2"}}
@@ -485,7 +507,6 @@ var _ = Describe("NamespacedClient", func() {
485507

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

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

521+
It("should not change objects via update status when object namespace is different", func() {
522+
changedDep := dep.DeepCopy()
523+
changedDep.SetNamespace("test")
524+
changedDep.Status.Replicas = 99
525+
526+
Expect(getClient().Status().Update(ctx, changedDep)).To(HaveOccurred())
527+
})
528+
500529
It("should change objects via status patch", func() {
501530
changedDep := dep.DeepCopy()
502531
changedDep.Status.Replicas = 99
503-
changedDep.SetNamespace("test")
504532

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

507535
actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
508536
Expect(err).NotTo(HaveOccurred())
509537
Expect(actual).NotTo(BeNil())
510538
Expect(actual.GetNamespace()).To(BeEquivalentTo(ns))
511539
Expect(actual.Status.Replicas).To(BeEquivalentTo(99))
512540
})
541+
542+
It("should not change objects via status patch when object namespace is different", func() {
543+
changedDep := dep.DeepCopy()
544+
changedDep.Status.Replicas = 99
545+
changedDep.SetNamespace("test")
546+
547+
Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).To(HaveOccurred())
548+
})
513549
})
514550

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

0 commit comments

Comments
 (0)