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.

Sign up for GitHub

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

Jump to bottom

⚠ Refactor cache.Options, deprecate MultiNamespacedCacheBuilder #2157

Merged
merged 1 commit into from
Jan 31, 2023
Merged

⚠ Refactor cache.Options, deprecate MultiNamespacedCacheBuilder #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
112 changes: 68 additions & 44 deletions pkg/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"time"

"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -118,44 +119,59 @@ type Options struct {
// Mapper is the RESTMapper to use for mapping GroupVersionKinds to Resources
Mapper meta.RESTMapper

// Resync is the base frequency the informers are resynced.
// ResyncEvery is the base frequency the informers are resynced.
// Defaults to defaultResyncTime.
// A 10 percent jitter will be added to the Resync period between informers
// A 10 percent jitter will be added to the ResyncEvery period between informers
// So that all informers will not send list requests simultaneously.
Resync *time.Duration
ResyncEvery *time.Duration

// Namespace restricts the cache's ListWatch to the desired namespace
// Default watches all namespaces
Namespace string
// View restricts the cache's ListWatch to the desired fields per GVK
// Default watches all fields and all namespaces.
View ViewOptions
Copy link
Member

Choose a reason for hiding this comment

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

I am really, really not a fan of moving things into substructs. It is much easier to find what options exist if they are on the top level.

Copy link
Member Author

@vincepri vincepri Feb 1, 2023 •
edited

Choose a reason for hiding this comment

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

Do we have a better name? Having all of the selectors and transformers at the top level IMO it's super confusing for new users. It's also not clear at all that all Get() and List() requests would be limited to those for example

}

// SelectorsByObject restricts the cache's ListWatch to the desired
// fields per GVK at the specified object, the map's value must implement
// Selector [1] using for example a Set [2]
// [1] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Selector
// [2] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Set
SelectorsByObject SelectorsByObject
// ViewOptions are the optional arguments for creating a cache view.
// A cache view restricts the Get(), List(), and internal ListWatch operations
// to the desired parameter.
type ViewOptions struct {
// Namespaces restricts the cache's ListWatch to the desired namespaces
// Default watches all namespaces
Namespaces []string

// DefaultSelector will be used as selectors for all object types
// that do not have a selector in SelectorsByObject defined.
// unless they have a more specific selector set in ByObject.
DefaultSelector ObjectSelector
vincepri marked this conversation as resolved.
Show resolved Hide resolved

// UnsafeDisableDeepCopyByObject indicates not to deep copy objects during get or
// list objects per GVK at the specified object.
// Be very careful with this, when enabled you must DeepCopy any object before mutating it,
// otherwise you will mutate the object in the cache.
UnsafeDisableDeepCopyByObject DisableDeepCopyByObject
// DefaultTransform will be used as transform for all object types
// unless they have a more specific transform set in ByObject.
DefaultTransform toolscache.TransformFunc

// ByObject restricts the cache's ListWatch to the desired fields per GVK at the specified object.
ByObject ViewByObject
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand how it improves anything to move this into a substruct, IMHO this just makes it harder to find what options exist.

What might have improved things would be if this was the object -> object-level options map rather than a struct that embedds a bunch of settings as maps.

}

// TransformByObject is a map from GVKs to transformer functions which
// ViewByObject offers more fine-grained control over the cache's ListWatch by object.
type ViewByObject struct {
// Selectors restricts the cache's ListWatch to the desired
// fields per GVK at the specified object, the map's value must implement
// Selectors [1] using for example a Set [2]
// [1] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Selectors
// [2] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Set
Selectors SelectorsByObject

// Transform is a map from objects to transformer functions which
// get applied when objects of the transformation are about to be committed
// to cache.
//
// This function is called both for new objects to enter the cache,
// and for updated objects.
TransformByObject TransformByObject
// and for updated objects.
Transform TransformByObject

// DefaultTransform is the transform used for all GVKs which do
// not have an explicit transform func set in TransformByObject
DefaultTransform toolscache.TransformFunc
// UnsafeDisableDeepCopy indicates not to deep copy objects during get or
// list objects per GVK at the specified object.
// Be very careful with this, when enabled you must DeepCopy any object before mutating it,
// otherwise you will mutate the object in the cache.
UnsafeDisableDeepCopy DisableDeepCopyByObject
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really have anything to do with viewing objects

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'll follow-up and move this one out

}

var defaultResyncTime = 10 * time.Hour
Expand All @@ -166,15 +182,15 @@ func New(config *rest.Config, opts Options) (Cache, error) {
if err != nil {
return nil, err
}
selectorsByGVK, err := convertToByGVK(opts.SelectorsByObject, opts.DefaultSelector, opts.Scheme)
selectorsByGVK, err := convertToByGVK(opts.View.ByObject.Selectors, opts.View.DefaultSelector, opts.Scheme)
if err != nil {
return nil, err
}
disableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(opts.UnsafeDisableDeepCopyByObject, opts.Scheme)
disableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(opts.View.ByObject.UnsafeDisableDeepCopy, opts.Scheme)
if err != nil {
return nil, err
}
transformers, err := convertToByGVK(opts.TransformByObject, opts.DefaultTransform, opts.Scheme)
transformers, err := convertToByGVK(opts.View.ByObject.Transform, opts.View.DefaultTransform, opts.Scheme)
if err != nil {
return nil, err
}
Expand All @@ -185,14 +201,22 @@ func New(config *rest.Config, opts Options) (Cache, error) {
internalSelectorsByGVK[gvk] = internal.Selector(selector)
}

if len(opts.View.Namespaces) == 0 {
opts.View.Namespaces = []string{metav1.NamespaceAll}
}

if len(opts.View.Namespaces) > 1 {
return newMultiNamespaceCache(config, opts)
}

return &informerCache{
scheme: opts.Scheme,
Informers: internal.NewInformers(config, &internal.InformersOpts{
HTTPClient: opts.HTTPClient,
Scheme: opts.Scheme,
Mapper: opts.Mapper,
ResyncPeriod: *opts.Resync,
Namespace: opts.Namespace,
ResyncPeriod: *opts.ResyncEvery,
Namespace: opts.View.Namespaces[0],
ByGVK: internal.InformersOptsByGVK{
Selectors: internalSelectorsByGVK,
DisableDeepCopy: disableDeepCopyByGVK,
Expand Down Expand Up @@ -235,17 +259,17 @@ func (options Options) inheritFrom(inherited Options) (*Options, error) {
)
combined.Scheme = combineScheme(inherited.Scheme, options.Scheme)
combined.Mapper = selectMapper(inherited.Mapper, options.Mapper)
combined.Resync = selectResync(inherited.Resync, options.Resync)
combined.Namespace = selectNamespace(inherited.Namespace, options.Namespace)
combined.SelectorsByObject, combined.DefaultSelector, err = combineSelectors(inherited, options, combined.Scheme)
combined.ResyncEvery = selectResync(inherited.ResyncEvery, options.ResyncEvery)
combined.View.Namespaces = selectNamespaces(inherited.View.Namespaces, options.View.Namespaces)
combined.View.ByObject.Selectors, combined.View.DefaultSelector, err = combineSelectors(inherited, options, combined.Scheme)
if err != nil {
return nil, err
}
combined.UnsafeDisableDeepCopyByObject, err = combineUnsafeDeepCopy(inherited, options, combined.Scheme)
combined.View.ByObject.UnsafeDisableDeepCopy, err = combineUnsafeDeepCopy(inherited, options, combined.Scheme)
if err != nil {
return nil, err
}
combined.TransformByObject, combined.DefaultTransform, err = combineTransforms(inherited, options, combined.Scheme)
combined.View.ByObject.Transform, combined.View.DefaultTransform, err = combineTransforms(inherited, options, combined.Scheme)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -282,8 +306,8 @@ func selectResync(def, override *time.Duration) *time.Duration {
return def
}

func selectNamespace(def, override string) string {
if override != "" {
func selectNamespaces(def, override []string) []string {
if len(override) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to check for nil here instead? Otherwise you can't overwrite with an empty slice. Or is that not a relevant use case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't both nil and 0 have the same effect?

Copy link
Member

Choose a reason for hiding this comment

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

The last time I checked this it was a difference. Let me double-check

Copy link
Member

Choose a reason for hiding this comment

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

https://go.dev/play/p/jF2-tgk93OB

Copy link
Member Author

@vincepri vincepri Jan 31, 2023 •
edited

Choose a reason for hiding this comment

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

I think that works as I would expect, the default is a nil slice, which should never overwrite anything. Only slices with at least one member should, so it works ~like before

Copy link
Member

Choose a reason for hiding this comment

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

Yup that's why I asked. Do we ever want to overwrite an existing value with an empty slice (which would then lead to us using all namespaces).

But I guess in that case folks can also just overwrite it with a slice with the All value

return override
}
return def
Expand All @@ -297,11 +321,11 @@ func combineSelectors(inherited, options Options, scheme *runtime.Scheme) (Selec
//
// There is a bunch of complexity here because we need to convert to SelectorsByGVK
// to be able to match keys between options and inherited and then convert back to SelectorsByObject
optionsSelectorsByGVK, err := convertToByGVK(options.SelectorsByObject, options.DefaultSelector, scheme)
optionsSelectorsByGVK, err := convertToByGVK(options.View.ByObject.Selectors, options.View.DefaultSelector, scheme)
if err != nil {
return nil, ObjectSelector{}, err
}
inheritedSelectorsByGVK, err := convertToByGVK(inherited.SelectorsByObject, inherited.DefaultSelector, inherited.Scheme)
inheritedSelectorsByGVK, err := convertToByGVK(inherited.View.ByObject.Selectors, inherited.View.DefaultSelector, inherited.Scheme)
if err != nil {
return nil, ObjectSelector{}, err
}
Expand Down Expand Up @@ -360,11 +384,11 @@ func combineFieldSelectors(fs ...fields.Selector) fields.Selector {
func combineUnsafeDeepCopy(inherited, options Options, scheme *runtime.Scheme) (DisableDeepCopyByObject, error) {
// UnsafeDisableDeepCopyByObject is combined via precedence. Only if a value for a particular GVK is unset
// in options will a value from inherited be used.
optionsDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(options.UnsafeDisableDeepCopyByObject, options.Scheme)
optionsDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(options.View.ByObject.UnsafeDisableDeepCopy, options.Scheme)
if err != nil {
return nil, err
}
inheritedDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(inherited.UnsafeDisableDeepCopyByObject, inherited.Scheme)
inheritedDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(inherited.View.ByObject.UnsafeDisableDeepCopy, inherited.Scheme)
if err != nil {
return nil, err
}
Expand All @@ -384,11 +408,11 @@ func combineTransforms(inherited, options Options, scheme *runtime.Scheme) (Tran
// Transform functions are combined via chaining. If both inherited and options define a transform
// function, the transform function from inherited will be called first, and the transform function from
// options will be called second.
optionsTransformByGVK, err := convertToByGVK(options.TransformByObject, options.DefaultTransform, options.Scheme)
optionsTransformByGVK, err := convertToByGVK(options.View.ByObject.Transform, options.View.DefaultTransform, options.Scheme)
if err != nil {
return nil, nil, err
}
inheritedTransformByGVK, err := convertToByGVK(inherited.TransformByObject, inherited.DefaultTransform, inherited.Scheme)
inheritedTransformByGVK, err := convertToByGVK(inherited.View.ByObject.Transform, inherited.View.DefaultTransform, inherited.Scheme)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -447,8 +471,8 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
}

// Default the resync period to 10 hours if unset
if opts.Resync == nil {
opts.Resync = &defaultResyncTime
if opts.ResyncEvery == nil {
opts.ResyncEvery = &defaultResyncTime
}
return opts, nil
}
Expand Down
92 changes: 55 additions & 37 deletions pkg/cache/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,13 @@ var _ = Describe("Multi-Namespace Informer Cache", func() {
CacheTest(cache.MultiNamespacedCacheBuilder([]string{testNamespaceOne, testNamespaceTwo, "default"}), cache.Options{})
})
var _ = Describe("Informer Cache without DeepCopy", func() {
CacheTest(cache.New, cache.Options{UnsafeDisableDeepCopyByObject: cache.DisableDeepCopyByObject{cache.ObjectAll{}: true}})
CacheTest(cache.New, cache.Options{
View: cache.ViewOptions{
ByObject: cache.ViewByObject{
UnsafeDisableDeepCopy: cache.DisableDeepCopyByObject{cache.ObjectAll{}: true},
},
},
})
})

var _ = Describe("Cache with transformers", func() {
Expand Down Expand Up @@ -184,34 +190,15 @@ var _ = Describe("Cache with transformers", func() {

By("creating the informer cache")
informerCache, err = cache.New(cfg, cache.Options{
DefaultTransform: func(i interface{}) (interface{}, error) {
obj := i.(runtime.Object)
Expect(obj).NotTo(BeNil())

accessor, err := meta.Accessor(obj)
Expect(err).To(BeNil())
annotations := accessor.GetAnnotations()

if _, exists := annotations["transformed"]; exists {
// Avoid performing transformation multiple times.
return i, nil
}

if annotations == nil {
annotations = make(map[string]string)
}
annotations["transformed"] = "default"
accessor.SetAnnotations(annotations)
return i, nil
},
TransformByObject: cache.TransformByObject{
&corev1.Pod{}: func(i interface{}) (interface{}, error) {
View: cache.ViewOptions{
DefaultTransform: func(i interface{}) (interface{}, error) {
obj := i.(runtime.Object)
Expect(obj).NotTo(BeNil())

accessor, err := meta.Accessor(obj)
Expect(err).To(BeNil())

annotations := accessor.GetAnnotations()

if _, exists := annotations["transformed"]; exists {
// Avoid performing transformation multiple times.
return i, nil
Expand All @@ -220,10 +207,33 @@ var _ = Describe("Cache with transformers", func() {
if annotations == nil {
annotations = make(map[string]string)
}
annotations["transformed"] = "explicit"
annotations["transformed"] = "default"
accessor.SetAnnotations(annotations)
return i, nil
},
ByObject: cache.ViewByObject{
Transform: cache.TransformByObject{
&corev1.Pod{}: func(i interface{}) (interface{}, error) {
obj := i.(runtime.Object)
Expect(obj).NotTo(BeNil())
accessor, err := meta.Accessor(obj)
Expect(err).To(BeNil())

annotations := accessor.GetAnnotations()
if _, exists := annotations["transformed"]; exists {
// Avoid performing transformation multiple times.
return i, nil
}

if annotations == nil {
annotations = make(map[string]string)
}
annotations["transformed"] = "explicit"
accessor.SetAnnotations(annotations)
return i, nil
},
},
},
},
})
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -360,10 +370,14 @@ var _ = Describe("Cache with selectors", func() {
}

opts := cache.Options{
SelectorsByObject: cache.SelectorsByObject{
&corev1.ServiceAccount{}: {Field: fields.OneTermEqualSelector("metadata.namespace", testNamespaceOne)},
View: cache.ViewOptions{
DefaultSelector: cache.ObjectSelector{Field: fields.OneTermEqualSelector("metadata.namespace", testNamespaceTwo)},
ByObject: cache.ViewByObject{
Selectors: cache.SelectorsByObject{
&corev1.ServiceAccount{}: {Field: fields.OneTermEqualSelector("metadata.namespace", testNamespaceOne)},
},
},
},
DefaultSelector: cache.ObjectSelector{Field: fields.OneTermEqualSelector("metadata.namespace", testNamespaceTwo)},
}

By("creating the informer cache")
Expand Down Expand Up @@ -784,7 +798,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca

It("should be able to restrict cache to a namespace", func() {
By("creating a namespaced cache")
namespacedCache, err := cache.New(cfg, cache.Options{Namespace: testNamespaceOne})
namespacedCache, err := cache.New(cfg, cache.Options{View: cache.ViewOptions{Namespaces: []string{testNamespaceOne}}})
Expect(err).NotTo(HaveOccurred())

By("running the cache and waiting for it to sync")
Expand Down Expand Up @@ -1065,7 +1079,7 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca

It("should be able to restrict cache to a namespace", func() {
By("creating a namespaced cache")
namespacedCache, err := cache.New(cfg, cache.Options{Namespace: testNamespaceOne})
namespacedCache, err := cache.New(cfg, cache.Options{View: cache.ViewOptions{Namespaces: []string{testNamespaceOne}}})
Expect(err).NotTo(HaveOccurred())

By("running the cache and waiting for it to sync")
Expand Down Expand Up @@ -1219,10 +1233,14 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
By("creating the cache")
builder := cache.BuilderWithOptions(
cache.Options{
SelectorsByObject: cache.SelectorsByObject{
&corev1.Pod{}: {
Label: labels.Set(tc.labelSelectors).AsSelector(),
Field: fields.Set(tc.fieldSelectors).AsSelector(),
View: cache.ViewOptions{
ByObject: cache.ViewByObject{
Selectors: cache.SelectorsByObject{
&corev1.Pod{}: {
Label: labels.Set(tc.labelSelectors).AsSelector(),
Field: fields.Set(tc.fieldSelectors).AsSelector(),
},
},
},
},
},
Expand Down Expand Up @@ -1804,11 +1822,11 @@ func isKubeService(svc metav1.Object) bool {
}

func isPodDisableDeepCopy(opts cache.Options) bool {
if d, ok := opts.UnsafeDisableDeepCopyByObject[&corev1.Pod{}]; ok {
if d, ok := opts.View.ByObject.UnsafeDisableDeepCopy[&corev1.Pod{}]; ok {
return d
} else if d, ok = opts.UnsafeDisableDeepCopyByObject[cache.ObjectAll{}]; ok {
} else if d, ok = opts.View.ByObject.UnsafeDisableDeepCopy[cache.ObjectAll{}]; ok {
return d
} else if d, ok = opts.UnsafeDisableDeepCopyByObject[&cache.ObjectAll{}]; ok {
} else if d, ok = opts.View.ByObject.UnsafeDisableDeepCopy[&cache.ObjectAll{}]; ok {
return d
}
return false
Expand Down