✨ Add namespace enforcing wrapper for client.Client #1088

varshaprasad96 · 2020-07-30T15:51:47Z

This PR adds a namespace enforcing wrapper for client.Client.
This helps while dealing with namespace-scoped objects, where the
namespace value need not be specified in every operation.

This PR addresses the issue: #1073

k8s-ci-robot · 2020-07-30T15:51:56Z

Hi @varshaprasad96. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

varshaprasad96 · 2020-07-30T15:52:37Z

/assign @alvaroaleman

alvaroaleman · 2020-07-30T16:31:19Z

/ok-to-test

varshaprasad96 · 2020-07-30T17:23:19Z

/retest

alvaroaleman

Thanks for your work! I think this doesn't need a warning but just a ✨ prefix as its a completely backwards-compatible change.

Have you checked what happens if the namespaced client is used for non-namespaced objects like e.G. a Namespace? Does the api server error in that case or is the namespace argument just silently dropped?

Ideally it would be great if this client works for both namespaced and non-namespaced objects.

pkg/client/namespaced_client.go

vincepri · 2020-07-30T17:50:32Z

What about having a method on the main Client interface that returns the namespaced-client implementation underneath?

For example:

client.WithNamespace().Get()

or

client.ForNamespace().Get()

etc.

alvaroaleman · 2020-07-30T17:52:47Z

@vincepri one drawback of that is that we break existing wrappers

vincepri · 2020-07-30T17:53:04Z

That would be a breaking change yes

alvaroaleman · 2020-07-30T22:10:57Z

pkg/client/namespaced_client.go

+		return err
+	}
+
+	if n.namespace != "" && metaObj.GetNamespace() != n.namespace {


I think rather than trying to infer from the object if its namespaced by checking if any namespace is set, it would be more logical to take a RESTMapper in the constructor of this wrapper and use its RESTMapping method to figure out if a given object is namespaced or not (You can use GVKForObject to get the gvk)

Thanks for the review @alvaroaleman. Had a doubt regarding this. GVKForObject requires runtime.Scheme, which is useful in RestMapping method too. Currently we don't have a method to obtain the scheme from client Options, when it is generated. If we go ahead with creating a new scheme in the implementation, we would have to register the object Kind manually using AddToScheme. Is there any another way to go about this ?

Related #1058

I think for now the constructor needs to take both a RESTMapper and a Scheme. Ideally we could just obtain both of those from the Client, but that is not the case today. Once #1058 is in, we can at least obtain the Scheme from the Client.

Thanks @vincepri and @alvaroaleman . Will update the PR.

I see that in the implementation of client.New(..), runtime.Scheme will not be used for unstructured objects. Hence, even when we pass a scheme with the client, GVK is not registered - similar to test case present here. If this is the case, then wouldn't RESTMapping not be useful for unstructured objects, since for them GVK is extracted from Object itself ?

With unstructured objects, the user is required to set the GVK before passing in to a client, so you should be able to use GetObjectKind() on those.

That said, exposing a RESTMapper from a Client is probably a good thing. We can follow the approach used in #1058 to work out something similar

We've merged https://github.com/kubernetes-sigs/controller-runtime/pull/1109/files, which brings in support to get a RESTMapper from a client

vincepri · 2020-08-04T02:20:27Z

Re-reviewed this PR, and I'd like to propose to go down the client.ForNamespace() (or similar) route, which is flexible in a lot of ways and transparent to users.

alvaroaleman · 2020-08-04T13:33:07Z

Re-reviewed this PR, and I'd like to propose to go down the client.ForNamespace() (or similar) route, which is flexible in a lot of ways and transparent to users.

Well, since we have at least three implementations (delegating, normal and fake) and how this works is identical for all of them, this needs to be implemented as a wrapper anyways. We can probably debate in a follow-up if we want an interface change.

pkg/client/namespaced_client.go

coderanger · 2020-09-26T22:32:30Z

It seems a little weird to silently overwrite a namespace on an object. I could see defaulting it (set only if == ""), or erroring if set to the wrong thing, but this flavor seems more likely to create bugs by redirecting calls to the wrong object.

alvaroaleman · 2020-10-01T15:42:08Z

It seems a little weird to silently overwrite a namespace on an object. I could see defaulting it (set only if == ""), or erroring if set to the wrong thing, but this flavor seems more likely to create bugs by redirecting calls to the wrong object.

Yeah agree on that, if its set to a different NS we should error out instead of overwriting it

varshaprasad96 · 2020-10-18T22:35:04Z

Addressed review comments.

coderanger · 2020-10-19T03:48:59Z

This now checks if the namespace matches the client twice, basically two strcmps where I think you only need one?

alvaroaleman

This now checks if the namespace matches the client twice, basically two strcmps where I think you only need one?

@coderanger What exactly do you mean by that, could you comment on the code where you see this?

pkg/client/namespaced_client.go

coderanger · 2020-10-20T02:25:51Z

This now checks if the namespace matches the client twice, basically two strcmps where I think you only need one?

@coderanger What exactly do you mean by that, could you comment on the code where you see this?

I think I understand the flow now, it looks like if the object has Namespace == "" it will silently apply the requested namespace. If the object has any other value (other than the bound namespace itself) it will error.

alvaroaleman

Some comments, after that it should be good to go. Thanks for your work on this!

pkg/client/namespaced_client.go

pkg/client/namespaced_client_test.go

This helps while dealing with namespace-scoped objects, where the namespace value need not be specified in every operation.

…bject.

alvaroaleman

/label tide/merge-method-squash
/retitle ✨ Add namespace enforcing wrapper for client.Client
/lgtm
/approve

Thanks for your work!

k8s-ci-robot · 2020-10-22T14:47:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, varshaprasad96

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 30, 2020

k8s-ci-robot requested review from alenkacz and joelanford July 30, 2020 15:51

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 30, 2020

k8s-ci-robot assigned alvaroaleman Jul 30, 2020

varshaprasad96 changed the title ~~(:warning:) Add a namespace enforcing wrapper for client.Client~~ (:warning:) Add namespace enforcing wrapper for client.Client Jul 30, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2020

alvaroaleman reviewed Jul 30, 2020

View reviewed changes

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from a758028 to d0dc121 Compare July 30, 2020 21:25

alvaroaleman reviewed Jul 30, 2020

View reviewed changes

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from d0dc121 to 4d7e627 Compare August 25, 2020 22:40

varshaprasad96 requested a review from alvaroaleman August 25, 2020 22:41

alvaroaleman reviewed Aug 25, 2020

View reviewed changes

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from 4d7e627 to 35adc25 Compare September 26, 2020 19:18

varshaprasad96 requested a review from alvaroaleman September 26, 2020 19:20

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from 35adc25 to 063de40 Compare October 18, 2020 22:34

alvaroaleman reviewed Oct 19, 2020

View reviewed changes

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

pkg/client/namespaced_client.go Outdated Show resolved Hide resolved

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from 063de40 to 1700206 Compare October 20, 2020 01:19

varshaprasad96 requested a review from alvaroaleman October 20, 2020 01:22

varshaprasad96 changed the title ~~(:warning:) Add namespace enforcing wrapper for client.Client~~ (:sparkle:) Add namespace enforcing wrapper for client.Client Oct 20, 2020

varshaprasad96 changed the title ~~(:sparkle:) Add namespace enforcing wrapper for client.Client~~ (:sparkles:) Add namespace enforcing wrapper for client.Client Oct 20, 2020

alvaroaleman reviewed Oct 21, 2020

View reviewed changes

varshaprasad96 added 3 commits October 21, 2020 16:02

This commit adds a namespace enforcing wrapper for client.Client.

9260be6

This helps while dealing with namespace-scoped objects, where the namespace value need not be specified in every operation.

Modify namespaced client to use RestMapper for finding the scope of o…

fca0592

…bject.

Address review comments

6ca7808

varshaprasad96 force-pushed the client/add-namespace-wrapper branch from 1700206 to 6ca7808 Compare October 21, 2020 23:08

varshaprasad96 requested a review from alvaroaleman October 21, 2020 23:14

alvaroaleman approved these changes Oct 22, 2020

View reviewed changes

k8s-ci-robot changed the title ~~(:sparkles:) Add namespace enforcing wrapper for client.Client~~ ✨ Add namespace enforcing wrapper for client.Client Oct 22, 2020

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 22, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020

k8s-ci-robot merged commit 32e94b6 into kubernetes-sigs:master Oct 22, 2020

alvaroaleman mentioned this pull request Oct 26, 2020

Namespace enforcing wrapper for client.Client #1073

Closed

✨ Add namespace enforcing wrapper for client.Client #1088

✨ Add namespace enforcing wrapper for client.Client #1088

Uh oh!

Conversation

varshaprasad96 commented Jul 30, 2020

Uh oh!

k8s-ci-robot commented Jul 30, 2020

Uh oh!

varshaprasad96 commented Jul 30, 2020

Uh oh!

alvaroaleman commented Jul 30, 2020

Uh oh!

varshaprasad96 commented Jul 30, 2020

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincepri commented Jul 30, 2020

Uh oh!

alvaroaleman commented Jul 30, 2020

Uh oh!

vincepri commented Jul 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincepri commented Aug 4, 2020

Uh oh!

alvaroaleman commented Aug 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderanger commented Sep 26, 2020

Uh oh!

alvaroaleman commented Oct 1, 2020

Uh oh!

varshaprasad96 commented Oct 18, 2020

Uh oh!

coderanger commented Oct 19, 2020

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderanger commented Oct 20, 2020

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 22, 2020

Uh oh!

Uh oh!