✨ Add support for creating namespaced watches

[lulf]

Extends the Source type with an optional Namespace parameter, which will override the default namespace by adding the namespace as a key in the informer map. Existing usage will remain as is, sharing the same key as before, whereas if namespace is specified, a new informer will be created for that case.

This allows creating an operator watching resources in multiple namespaces without requiring cluster-wide privileges for that resource, as desired in #687 .

Simple tests suggest that it works for me, but there are probably pitfalls I have not considered.

I'd love some feedback on this approach and if you think this is the right or wrong direction.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

[k8s-ci-robot]

Welcome @lulf!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @lulf. 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lulf
To complete the pull request process, please assign pwittrock
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[alvaroaleman]

Looks good, but it would be nice if we had some testing for this, e.G. in pkg/controller/controller_integration_test.go.

What I am not 100% sure about is if it makes sense to make something as specific as GetInformerForKindInNamespace part of our public API, because there are more ways to scope watches (e.G. LabelSelector and FieldSelector) and with the current approach we end up with a dedicated func for each of those.

An alternate approach could be to use the functional opts pattern to allow passing in additional opts for the watches.

Assigning shawn as I heard he knows a lot about the cache :)
/assign @shawn-hurley
/ok-to-test

Edit: Also thinking about this, I am not sure if this really make sense because the client will still create a non-namespaced watch once you start using it for the given GVK. This means that you end up with both a namespaced and a non-namespaced watch and use the former for the client and the latter for the watch. This effectively makes the whole thing have the same effect as a Predicate except that you create another cache.

[lulf]

@alvaroaleman Thanks for taking a look.

Edit: Also thinking about this, I am not sure if this really make sense because the client will still create a non-namespaced watch once you start using it for the given GVK. This means that you end up with both a namespaced and a non-namespaced watch and use the former for the client and the latter for the watch. This effectively makes the whole thing have the same effect as a Predicate except that you create another cache.

I'm very open to alternative approaches. The main motivation is the ability to create watches in both the 'controller namespace' as well as watches for other namespaces, without requiring cluster-wide access to a particular resource. For example, with this PR, I'm able to create a manager watching deployments in any namespace, but for secrets, I can create a watch in a specific namespace (because I don't want to give my controller access to secrets all over the cluster).

Its not 100% clear to me what you mean by a non-namespaced watch. It doesn't seem to create a non-namespaced watch in my testing (at least it doesn't throw any permission error), could you point me to where this happens?

Happy to add some tests for this, just want to get some feedback on the approach and suggestions.

Thanks again!

[alvaroaleman]

Its not 100% clear to me what you mean by a non-namespaced watch. It doesn't seem to create a non-namespaced watch in my testing (at least it doesn't throw any permission error), could you point me to where this happens?

Seems I was too quick with the comment, as you updated *informerCache.Get to use the namespace when getting an informer. However that means that we create an informer for every namespace from which we requested something if the object is namespaced and never use a non-namespaced informer. This results in a lot of cache duplication when someone uses a non-namespaced version of source.Kind.

Setting the namespace on the mgr does not work for your use-case?

[lulf]

@alvaroaleman Yes, it would lead to cache duplication, that is the 'cost' of the added flexibility I guess, but I think it only impact users of this functionality, and existing usage should remain as is. Maybe if there was a smarter way to share cache between informers for different namespaces...

Setting namespace on the mgr does not cover my use case, because it allows only watching resources in a single namespace. The specific flexibility I need is the ability to watch some resources (that the controller "owns", i.e. CRD) in all namespaces, and then watch resources created by the controller (such as secrets, deployments etc) in the same namespace as the controller. The main point here is that I don't want my controller to read secrets in other namespaces than the same as where the controller is running.

I'm looking at your initial idea, maybe changing GetInformers to

GetInformers(runtime.Object, opts...client.ListOption) and somehow base the informer lookup key on those options. In the same way, if options are not set, as today, no cache duplication is done, its only for the cases that actually set those options.

I don't know the code that well, so apologies if what I'm saying is incorrect!

[alvaroaleman]

Yes, it would lead to cache duplication, that is the 'cost' of the added flexibility I guess, but I think it only impact users of this functionality, and existing usage should remain as is

I think it impacts everyone because whenever a namespaced object is retrieved, we create a namespaced cache for the given type even if we already have a non-namespaced one.

[lulf]

I think it impacts everyone because whenever a namespaced object is retrieved, we create a namespaced cache for the given type even if we already have a non-namespaced one.

Just pushed a change so that informer_cache.Get() passes nil as the namespace argument instead of object.key, which should make cache behavior identical to earlier.

What I was thinking is that the existing usages of cache.GetInformerForKind* pass nil as the namespace argument, which should give the exact same key in the informer map, as if the map was only keyed by GVK as before. The only code path that will cause cache duplication, is if namespace is set to something non-nil (i.e. in this case source.Namespace is set when creating the watch).

@alvaroaleman Does this address that particular concern?

[alvaroaleman]

Yeah it addresses the concern but it unfortunately means this PR won't solve your problem anymore as it now uses the non-namespaced cache for reading again :grimace:

This is unfortunate a tricky problem, IMHO it would be best to continue discussing possible approaches in #244

[lulf]

Good point. I will close this for now, but might have another go (and add a test for it) 😉 thanks for spending time looking at it, @alvaroaleman!

[lulf]

@alvaroaleman I was unable to reopen this due to force-pushing, but I have raised #694 which takes a slighly different approach, and overrides the namespace if cache is configured to watch all namespace. Also, the interface is a bit different.