-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add namespace enforcing wrapper for client.Client #1088
✨ Add namespace enforcing wrapper for client.Client #1088
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/assign @alvaroaleman |
/ok-to-test |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
What about having a method on the main For example:
|
@vincepri one drawback of that is that we break existing wrappers |
That would be a breaking change yes |
a758028
to
d0dc121
Compare
pkg/client/namespaced_client.go
Outdated
return err | ||
} | ||
|
||
if n.namespace != "" && metaObj.GetNamespace() != n.namespace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related #1058
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vincepri and @alvaroaleman . Will update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've merged https://github.com/kubernetes-sigs/controller-runtime/pull/1109/files, which brings in support to get a RESTMapper from a client
Re-reviewed this PR, and I'd like to propose to go down the |
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. |
d0dc121
to
4d7e627
Compare
4d7e627
to
35adc25
Compare
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 |
35adc25
to
063de40
Compare
Addressed review comments. |
This now checks if the namespace matches the client twice, basically two strcmps where I think you only need one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
063de40
to
1700206
Compare
I think I understand the flow now, it looks like if the object has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, after that it should be good to go. Thanks for your work on this!
This helps while dealing with namespace-scoped objects, where the namespace value need not be specified in every operation.
1700206
to
6ca7808
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/label tide/merge-method-squash
/retitle ✨ Add namespace enforcing wrapper for client.Client
/lgtm
/approve
Thanks for your work!
[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:
Approvers can indicate their approval by writing |
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