Skip to content
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

Cache selectors should allow setting a default #1708

Closed
alvaroaleman opened this issue Oct 29, 2021 · 5 comments · Fixed by #1710
Closed

Cache selectors should allow setting a default #1708

alvaroaleman opened this issue Oct 29, 2021 · 5 comments · Fixed by #1710

Comments

@alvaroaleman
Copy link
Member

It is currently possible to set a selector in the cache per GVK, however it is not possible to set a default selector which might be very useful for scenarios like "I want all caches in this namespace except the cache for footype, which should be in a different namespace".

@joelanford
Copy link
Member

I definitely agree that a default selector would be useful in many cases (e.g. label my.domain/managedBy == "my-operator"), but I'm not sure a namespace selector is appropriate at this layer.

If I'm understanding this correctly, it would result in the informers using a cluster-wide list/watch and the apiserver receiving all objects from etcd before filtering down to the desired namespace.

Clients using this setup would require cluster-wide RBAC list/watch for all of the watched kinds and (barring any gaps in my understanding of apiserver <-> etcd communication) would cause unnecessary resource usage from objects not in the selected namespaces being unnecessarily sent from etcd to the apiserver.

It seems like the GVKCaches approach in #1591 would be better from an RBAC and resource utilization perspective.

@alvaroaleman
Copy link
Member Author

alvaroaleman commented Nov 1, 2021

If I'm understanding this correctly, it would result in the informers using a cluster-wide list/watch and the apiserver receiving all objects from etcd before filtering down to the desired namespace.

No, the selector can be used to not even receive the objects and make the apiserver filter for us. Using a default namespace was just an example btw, could also be a default label selector. What exactly makes sense depends on what you want to do :)

@joelanford
Copy link
Member

No, the selector can be used to not even receive the objects and make the apiserver filter for us.

Understood, but if cache.Options.Namespace is unset, the underlying apiserver call is going to be cluster-scoped regardless of the selectors. So it would generally be a bad idea to use a cluster-scoped informer with a namespace selector for the reasons I stated.

Nonetheless, I agree a default selector is valuable.

@alvaroaleman
Copy link
Member Author

alvaroaleman commented Nov 1, 2021

No, we had a change a while back that made us do a namespace-scoped list+watch request and only requires rbac for that namespace if a fieldselector for a namespace is set: #1602

@joelanford
Copy link
Member

Ah ha! That was the piece I was missing. Very cool!

akalenyu added a commit to akalenyu/containerized-data-importer that referenced this issue Jul 25, 2022
This gives us flexibility to cache only exactly what we need.
The error that led me to this was that we were attempting to Watch()
Routes/Ingresses which is basically caching all namespaces. We only want to cache the CDI namespace for those.
Source/feature from kubernetes-sigs/controller-runtime#1708

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
akalenyu added a commit to akalenyu/containerized-data-importer that referenced this issue Jul 26, 2022
This gives us flexibility to cache only exactly what we need.
The error that led me to this was that we were attempting to Watch()
Routes/Ingresses which is basically caching all namespaces. We only want to cache the CDI namespace for those.
Source/feature from kubernetes-sigs/controller-runtime#1708

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot pushed a commit to kubevirt/containerized-data-importer that referenced this issue Aug 1, 2022
…2371)

* Only list Ingresses/Routes in CDI namespace instead of cluster level

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Change the way we initialize cache for cdi controller

This gives us flexibility to cache only exactly what we need.
The error that led me to this was that we were attempting to Watch()
Routes/Ingresses which is basically caching all namespaces. We only want to cache the CDI namespace for those.
Source/feature from kubernetes-sigs/controller-runtime#1708

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants