✨ Add support for creating namespaced watches

[lulf]

Another variant of #692 . Extend source.Kind with taking ListOptions, and pass these on to the informer map. In this PR, only namespace is used, but I think it could be extended to labels as well.

• If the informer cache is configured to handle all namespaces (i.e. "") - use it as key in all lookups to avoid cache duplication. This preserves existing lookup behavior.
• If the informer cache is configured to handle a specific namespace - create separate cache which will be used for lookups in those cases. This allows creating watches for multiple namespaces dynamically.

I don't know yet if this works, its just a prototype of an API that I as a user would like to have supported. Will add an integration test, but early feedback is welcomed.

[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]

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 droot
You can assign the PR to them by writing /assign @droot 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]

So generally this looks like it will work and I like the overall idea of "use specific cache unless configured to use global one" :)

There are however some things to consider:

• If I read this correctly, using a source.Kind with a Namespace setting and a non-namespaced specificInformersMap will result in getting events from all namespaces, which is not expected
• The current approach means that as soon as any namespace is configured, caches will get established for all namespaces an object was ever requested from which is also unexpected
• When thinking about this approach in the context of label selectors I think just a binary "cache for everything if global cache, else distinct cache for every label selector ever used" is not enough, as it may lead to quite a bit of duplication again. Maybe a list of label selectors for which we want a distinct cache and then have the client internally either use the specific cache or do the label-selecting after receiving the objects from the cache?
• More of a nit, but I think it will be better to not re-use client.ListOptions for the Get on the cache map, as that forces us to keep their feature set on par

At the end of the day thought, this is the point where I can not give you any further feedback if the approach is feasible or has more issues I didn't consider, we will have to wait for @DirectXMan12 to chime in.

But thanks a lot for pushing this forward, its really great to see some movement here :)

[shawn-hurley]

@lulf would this solve your issue?

https://godoc.org/sigs.k8s.io/controller-runtime/pkg/manager#example-New--MultinamespaceCache

I think that this is a large enough feature with enough gotchas that it might be nice to write down the design for the proposed changes and maybe another option so that we can compare? I would use the 2 ok's here as a starting point.

[lulf]

@shawn-hurley What I need specifically is a way to use a global cache for some objects, and a specific namespace for other resources. The multi-namespace cache would not give me the ability to select which cache to use when creating a watch for a particular resource.

That being said, I have attempted another approach to solve my use case, where I create a 'delegating cache', which delegates either to global or a default namespaced cache based on the object type : https://github.com/EnMasseProject/enmasse/blob/2db9469529254274c90117b1f6fad360b8081857/pkg/cache/delegate_cache.go

So, I think I have a workaround for now. I could generalize the delegate cache and submit a PR for that if you think that would be interesting.

[shawn-hurley]

That delegating cache starts to sound like the first option in the list of OK's to me.

We would have to add more scenarios to cover because dynamic watches could change what is being watched during run time. We also would have to handle the case where 1 controller asks for a namespaced watch/cache BUT we already had that in the global lists. I think you would want to add a predicate for that at that time to do the correct filter for that watch.

I think there are some other scenarios where we need to protect folks from scenarios where the behavior maybe not obvious. But it seems like it could be a good start! I second what @alvaroaleman said, thank you for driving this forward!

Is there any way to add what that code does to a proposal that we can iterate on in words to make sure we can cover these different scenarios?

[lulf]

I think there are some other scenarios where we need to protect folks from scenarios where the behavior maybe not obvious. But it seems like it could be a good start! I second what @alvaroaleman said, thank you for driving this forward!

I've been focusing our own needs for this functionality so far, as we had to investigate whether or not something like this could be done in controller-runtime and how much time it would take. With the delegate cache, we have a solution that allows us to simply implement the cache interface until a more generalized approach is available in controller-runtime, so I'm happy with that. However, I'd love to improve on that and make it into something that could be included in controller-runtime itself.

Is there any way to add what that code does to a proposal that we can iterate on in words to make sure we can cover these different scenarios?

Is that best driven through #244 ? I can make a more general design document as a starting point, if desired.

[shawn-hurley]

@lulf Sorry I missed the last comment.

I think that #244 might be the best place and a more general design document will really help me think through the problem :)

Thanks!

[lulf]

Closing this for now based on feedback, thanks!