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

📖 Add a design for cache options configuration #2261

Merged
merged 1 commit into from Jun 12, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
226 changes: 226 additions & 0 deletions designs/cache_options.md
@@ -0,0 +1,226 @@
Cache Options
===================

This document describes how we imagine the cache options to look in
the future.

## Goals

* Align everyone on what settings on the cache we want to support and
their configuration surface
* Ensure that we support both complicated cache setups and provide an
intuitive configuration UX

## Non-Goals

* Describe the design and implementation of the cache itself.
The assumption is that the most granular level we will end up with is
"per-object multiple namespaces with distinct selectors" and that this
can be implemented using a "meta cache" that delegates per object and by
extending the current multi-namespace cache
* Outline any kind of timeline for when these settings will be implemented.
Implementation will happen gradually over time whenever someone steps up
to do the actual work

## Proposal


```
const (
AllNamespaces = corev1.NamespaceAll
)

type Config struct {
// LabelSelector specifies a label selector. A nil value allows to
// default this.
LabelSelector labels.Selector

// FieldSelector specifics a field selector. A nil value allows to
// default this.
FieldSelector fields.Selector

// Transform specifies a transform func. A nil value allows to default
// this.
Transform toolscache.TransformFunc

// UnsafeDisableDeepCopy specifies if List and Get requests against the
// cache should not DeepCopy. A nil value allows to default this.
UnsafeDisableDeepCopy *bool
alvaroaleman marked this conversation as resolved.
Show resolved Hide resolved
}


type ByObject struct {
// Namespaces maps a namespace name to cache setting. If set, only the
// namespaces in this map will be cached.
//
// Settings in the map value that are unset because either the value as a
// whole is nil or because the specific setting is nil will be defaulted.
// Use an empty value for the specific setting to prevent that.
//
// It is possible to have specific Config for just some namespaces
// but cache all namespaces by using the AllNamespaces const as the map key.
// This wil then include all namespaces that do not have a more specific
// setting.
//
// A nil map allows to default this to the cache's DefaultNamespaces setting.
// An empty map prevents this.
//
// This must be unset for cluster-scoped objects.
Namespaces map[string]*Config
Copy link
Member

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?

Copy link
Member Author

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.

Copy link

@irbekrm irbekrm May 31, 2023

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.

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

Copy link
Member Author

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:

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.

Copy link

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 👍🏼


// 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
Comment on lines +71 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 🤔

Copy link
Member Author

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:

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

}

type Options struct {
FillZpp marked this conversation as resolved.
Show resolved Hide resolved
// ByObject specifies per-object cache settings. If unset for a given
// object, this will fall through to Default* settings.
ByObject map[client.Object]*ByObject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Maybe?

Copy link
Member Author

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 confusion


// DefaultNamespaces maps namespace names to cache settings. If set, it
// will be used for all objects that have a nil Namespaces setting.
//
// It is possible to have a specific Config for just some namespaces
// but cache all namespaces by using the `AllNamespaces` const as the map
// key. This wil then include all namespaces that do not have a more
// specific setting.
//
// The options in the Config that are nil will be defaulted from
// the respective Default* settings.
DefaultNamespaces map[string]*Config

// DefaultLabelSelector is the label selector that will be used as
// the default field label selector for everything that doesn't
// have one configured.
DefaultLabelSelector labels.Selector

// DefaultFieldSelector is the field selector that will be used as
// the default field selector for everything that doesn't have
// one configured.
DefaultFieldSelector fields.Selector

// DefaultUnsafeDisableDeepCopy is the default for UnsafeDisableDeepCopy
// for everything that doesn't specify this.
DefaultUnsafeDisableDeepCopy *bool

// DefaultTransform will be used as transform for all object types
// unless they have a more specific transform set in ByObject.
DefaultTransform toolscache.TransformFunc
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +84 to +112
Copy link
Member

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 the ByObject seems clear that it overrides these imo

Copy link
Member Author

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


// HTTPClient is the http client to use for the REST client
HTTPClient *http.Client

// Scheme is the scheme to use for mapping objects to GroupVersionKinds
Scheme *runtime.Scheme

// Mapper is the RESTMapper to use for mapping GroupVersionKinds to Resources
Mapper meta.RESTMapper

// SyncPeriod determines the minimum frequency at which watched resources are
// reconciled. A lower period will correct entropy more quickly, but reduce
// responsiveness to change if there are many watched resources. Change this
// value only if you know what you are doing. Defaults to 10 hours if unset.
// there will a 10 percent jitter between the SyncPeriod of all controllers
// so that all controllers will not send list requests simultaneously.
//
// This applies to all controllers.
//
// A period sync happens for two reasons:
// 1. To insure against a bug in the controller that causes an object to not
// be requeued, when it otherwise should be requeued.
// 2. To insure against an unknown bug in controller-runtime, or its dependencies,
// that causes an object to not be requeued, when it otherwise should be
// requeued, or to be removed from the queue, when it otherwise should not
// be removed.
//
// If you want
// 1. to insure against missed watch events, or
// 2. to poll services that cannot be watched,
// then we recommend that, instead of changing the default period, the
// controller requeue, with a constant duration `t`, whenever the controller
// is "done" with an object, and would otherwise not requeue it, i.e., we
// recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`,
// instead of `reconcile.Result{}`.
SyncPeriod *time.Duration

}
```


## Example usages

### Cache ConfigMaps in the `public` and `kube-system` namespaces and Secrets in the `operator` Namespace


```
cache.Options{
ByObject: map[client.Object]*cache.ByObject{
&corev1.ConfigMap{}: {
Namespaces: map[string]*cache.Config{
"public": {},
"kube-system": {},
},
},
&corev1.Secret{}: {Namespaces: map[string]*Config{
"operator": {},
}},
},
}
```

### Cache ConfigMaps in all namespaces without selector, but have a selector for the `operator` Namespace

```
cache.Options{
ByObject: map[client.Object]*cache.ByObject{
&corev1.ConfigMap{}: {
Namespaces: map[string]*cache.Config{
cache.AllNamespaces: nil, // No selector for all namespaces...
"operator": {LabelSelector: labelSelector}, // except for the operator namespace
},
},
},
}
```


### Only cache the `operator` namespace for namespaced objects and all namespaces for Deployments

```
cache.Options{
ByObject: map[client.Object]*cache.ByObject{
&appsv1.Deployment: {Namespaces: map[string]*cache.Config{
cache.AllNamespaces: nil,
}},
},
DefaultNamespaces: map[string]*cache.Config{
"operator": nil,
},
}
```

### Use a LabelSelector for everything except Nodes

```
cache.Options{
ByObject: map[client.Object]*cache.ByObject{
&corev1.Node: {LabelSelector: labels.Everything()},
},
DefaultLabelSelector: myLabelSelector,
vincepri marked this conversation as resolved.
Show resolved Hide resolved
}
```

### Only cache namespaced objects in the `foo` and `bar` namespace

```
cache.Options{
DefaultNamespaces: map[string]*cache.Config{
"foo": nil,
"bar": nil,
}
}
```