📖 Add a design for cache options configuration

[alvaroaleman]

Prior art

• ✨ multiNamespaceCache: support custom newCache funcs per namespace #1962

/cc @joelanford @vincepri @sbueringer @FillZpp

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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:

• OWNERS [alvaroaleman]

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

[FillZpp]

/lgtm
/hold
see if there is more discussion @vincepri @joelanford @sbueringer

[vincepri]

Suggested change

	DefaultNamespaces: map[string]*cache.CacheSetting{
	ByNamespace: map[string]*cache.CacheSetting{

Maybe? The DefaultNamespaces doesn't really give away what it's doing

[alvaroaleman]

The reason I used DefaultNamespaces is to clarify that this will only be used if ByObject has no namespace settings

[alvaroaleman]

/retest

[sbueringer]

Thank you very much!

/lgtm

/hold
in case you want to wait for more feedback

[inteon]

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?

[alvaroaleman]

What is cached is what is being watched, so both.

[irbekrm]

What is cached is what is being watched, so both.

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 controller

[alvaroaleman]

Yes, it will be. Something like your use-case is what we want to support. Your config would look something like:

cache.Options{
ByObject: map[client.Object]*cache.ByObject{
  &corev1.Secrets{}: *cache.ByObject{
    Namespaces: map[string]*cache.Config{"the-namespace-for-secrets": nil},
  }
}

The above config would watch secrets in one namespace and everything else in all namespaces. That also applies to rbac.

[irbekrm]

Thank you- for us this would be very valuable 👍🏼

[vincepri]

Would it be better or worse to drop the Default prefix? It seems redundant, given that the ByObject seems clear that it overrides these imo

[alvaroaleman]

I like it because it clarifies that this is the last layer of defaulting

[vincepri]

Suggested change

	ByObject map[client.Object]*ByObject
	ByObject map[client.Object]ByObject

Maybe?

[alvaroaleman]

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 confusion

[vincepri]

Suggested change

	// Config will be used for cluster-scoped objects and to default
	// Config in the Namespaces field.
	//
	// It gets defaulted from the cache'sDefaultLabelSelector, DefaultFieldSelector,
	// DefaultUnsafeDisableDeepCopy and DefaultTransform.
	Config *Config
	// Overrides will be used for cluster-scoped objects and to default
	// Config in the Namespaces field.
	//
	// It gets defaulted from the cache'sDefaultLabelSelector, DefaultFieldSelector,
	// DefaultUnsafeDisableDeepCopy and DefaultTransform.
	Overrides *Overrides

Maybe? Could be a bit more readable, 🤔

[alvaroaleman]

I think that might be confusing because with an override, ppl expect the value in there to be the final setting but:

• Its not the top-most setting for namespaced resources
• It gets defaulted itself

[sbueringer]

/lgtm

[alvaroaleman]

/hold cancel