Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📖 Add a design for cache options configuration #2261
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 a design for cache options configuration #2261
Changes from all commits
7ad4b4c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How does this map to RBAC? Do I only need list, get, watch permissions in the namespaces specified in this map, or do I need those permissions for all namespaces? Does this option limit what is requested/ watched from the APIServer or does it just limit what is being cached?
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.
What is cached is what is being watched, so both.
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.
At the moment the calls to the kube apiserver to LIST resources are different (this might be in client-go, not sure) depending on whether the whole cache is scoped to a namespace or whether the namespace selector is applied as a field selector.
Because of that (I think) a namespace-scoped cache allows us to also scope RBAC to that namespace whereas the selector does not.
We have a use case where we want to watch Secrets in namespace X and Configmaps in all namespaces. We create two caches for this because although we could use the existing cache options to apply namespace filter to Secrets, only namespaced cache allows us to scope RBAC down so that users only need to give our controller permissions to list Secrets in that one namespace.
For us, it would be quite useful if we were able to use this
Namespaces
option in such a way that the namespace filter is also reflected in the required RBAC for our controllerThere 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.
Yes, it will be. Something like your use-case is what we want to support. Your config would look something like:
The above config would watch secrets in one namespace and everything else in all namespaces. That also applies to rbac.
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.
Thank you- for us this would be very valuable 👍🏼
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.
Maybe? Could be a bit more readable, 🤔
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 that might be confusing because with an override, ppl expect the value in there to be the final setting but:
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.
Maybe?
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.
Not a super strong opinion here, the reason I chose the pointer type is in some contexts an empty thing has a meaning (for example an empty labelSelector is a match-everything selector) so I thought by not requireding ppl to do a
map[string]ByObject{"namespace":{}}
we can avoid a bit of confusionThere 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.
Would it be better or worse to drop the
Default
prefix? It seems redundant, given that theByObject
seems clear that it overrides these imoThere 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 like it because it clarifies that this is the last layer of defaulting