From 915b85dcace934feadffe80ba8f0d8fd0a7fa0b6 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Mon, 30 Jan 2023 12:59:16 -0800 Subject: [PATCH] Refactor cache.Options, deprecate MultiNamespacedCacheBuilder This changeset refactors the entire cache.Options struct to provide a legible and clear way to configure parameters when a cache is created. In addition, it handles under the hood the automatic multi-namespace cache support we currently offer through the MultiNamespacedCacheBuilder, which is now deprecated. Signed-off-by: Vince Prignano --- pkg/cache/cache.go | 112 +++++++++++++-------- pkg/cache/cache_test.go | 92 ++++++++++------- pkg/cache/cache_unit_test.go | 156 ++++++++++++++--------------- pkg/cache/internal/informers.go | 16 ++- pkg/cache/multi_namespace_cache.go | 46 +++++---- pkg/cluster/cluster.go | 10 +- pkg/manager/example_test.go | 10 +- 7 files changed, 253 insertions(+), 189 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 28fd32c259..13cc87a297 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -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" @@ -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 +} - // 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 - // 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 +} - // 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 } var defaultResyncTime = 10 * time.Hour @@ -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 } @@ -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, @@ -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 } @@ -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 { return override } return def @@ -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 } @@ -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 } @@ -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 } @@ -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 } diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 37999ccac0..4e882f0f8f 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -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() { @@ -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 @@ -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()) @@ -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") @@ -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") @@ -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") @@ -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(), + }, + }, }, }, }, @@ -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 diff --git a/pkg/cache/cache_unit_test.go b/pkg/cache/cache_unit_test.go index c5e2ab7763..8f74e2407f 100644 --- a/pkg/cache/cache_unit_test.go +++ b/pkg/cache/cache_unit_test.go @@ -93,73 +93,73 @@ var _ = Describe("cache.inheritFrom", func() { }) Context("Resync", func() { It("is nil when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).Resync).To(BeNil()) + Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(BeNil()) }) It("is unchanged when only specified is set", func() { - specified.Resync = pointer.Duration(time.Second) - Expect(checkError(specified.inheritFrom(inherited)).Resync).To(Equal(specified.Resync)) + specified.ResyncEvery = pointer.Duration(time.Second) + Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(specified.ResyncEvery)) }) It("is inherited when only inherited is set", func() { - inherited.Resync = pointer.Duration(time.Second) - Expect(checkError(specified.inheritFrom(inherited)).Resync).To(Equal(inherited.Resync)) + inherited.ResyncEvery = pointer.Duration(time.Second) + Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(inherited.ResyncEvery)) }) It("is unchanged when both inherited and specified are set", func() { - specified.Resync = pointer.Duration(time.Second) - inherited.Resync = pointer.Duration(time.Minute) - Expect(checkError(specified.inheritFrom(inherited)).Resync).To(Equal(specified.Resync)) + specified.ResyncEvery = pointer.Duration(time.Second) + inherited.ResyncEvery = pointer.Duration(time.Minute) + Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(specified.ResyncEvery)) }) }) Context("Namespace", func() { - It("is NamespaceAll when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).Namespace).To(Equal(corev1.NamespaceAll)) + It("has zero length when View.Namespaces specified and inherited are unset", func() { + Expect(checkError(specified.inheritFrom(inherited)).View.Namespaces).To(HaveLen(0)) }) It("is unchanged when only specified is set", func() { - specified.Namespace = "specified" - Expect(checkError(specified.inheritFrom(inherited)).Namespace).To(Equal(specified.Namespace)) + specified.View.Namespaces = []string{"specified"} + Expect(checkError(specified.inheritFrom(inherited)).View.Namespaces).To(Equal(specified.View.Namespaces)) }) It("is inherited when only inherited is set", func() { - inherited.Namespace = "inherited" - Expect(checkError(specified.inheritFrom(inherited)).Namespace).To(Equal(inherited.Namespace)) + inherited.View.Namespaces = []string{"inherited"} + Expect(checkError(specified.inheritFrom(inherited)).View.Namespaces).To(Equal(inherited.View.Namespaces)) }) It("in unchanged when both inherited and specified are set", func() { - specified.Namespace = "specified" - inherited.Namespace = "inherited" - Expect(checkError(specified.inheritFrom(inherited)).Namespace).To(Equal(specified.Namespace)) + specified.View.Namespaces = []string{"specified"} + inherited.View.Namespaces = []string{"inherited"} + Expect(checkError(specified.inheritFrom(inherited)).View.Namespaces).To(Equal(specified.View.Namespaces)) }) }) Context("SelectorsByObject", func() { It("is unchanged when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(BeNil()) + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(BeNil()) }) It("is unchanged when only specified is set", func() { specified.Scheme = coreScheme - specified.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.Pod{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(1)) + specified.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.Pod{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(1)) }) It("is inherited when only inherited is set", func() { inherited.Scheme = coreScheme - inherited.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(1)) + inherited.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(1)) }) It("is combined when both inherited and specified are set", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.Pod{}: {}} - inherited.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(2)) + specified.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.Pod{}: {}} + inherited.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(2)) }) It("combines selectors if specified and inherited specify selectors for the same object", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.Pod{}: { + specified.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.Pod{}: { Label: labels.Set{"specified": "true"}.AsSelector(), Field: fields.Set{"metadata.name": "specified"}.AsSelector(), }} - inherited.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.Pod{}: { + inherited.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.Pod{}: { Label: labels.Set{"inherited": "true"}.AsSelector(), Field: fields.Set{"metadata.namespace": "inherited"}.AsSelector(), }} - combined := checkError(specified.inheritFrom(inherited)).SelectorsByObject + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors Expect(combined).To(HaveLen(1)) var ( obj client.Object @@ -179,51 +179,51 @@ var _ = Describe("cache.inheritFrom", func() { }) It("uses inherited scheme for inherited selectors", func() { inherited.Scheme = coreScheme - inherited.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(1)) + inherited.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(1)) }) It("does not use specified scheme for inherited selectors", func() { inherited.Scheme = runtime.NewScheme() specified.Scheme = coreScheme - inherited.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + inherited.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} _, err := specified.inheritFrom(inherited) Expect(err).To(WithTransform(runtime.IsNotRegisteredError, BeTrue())) }) It("uses inherited scheme for specified selectors", func() { inherited.Scheme = coreScheme - specified.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(1)) + specified.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(1)) }) It("uses specified scheme for specified selectors", func() { specified.Scheme = coreScheme - specified.SelectorsByObject = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} - Expect(checkError(specified.inheritFrom(inherited)).SelectorsByObject).To(HaveLen(1)) + specified.View.ByObject.Selectors = map[client.Object]ObjectSelector{&corev1.ConfigMap{}: {}} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Selectors).To(HaveLen(1)) }) }) Context("DefaultSelector", func() { It("is unchanged when specified and inherited are unset", func() { - Expect(specified.DefaultSelector).To(Equal(ObjectSelector{})) - Expect(inherited.DefaultSelector).To(Equal(ObjectSelector{})) - Expect(checkError(specified.inheritFrom(inherited)).DefaultSelector).To(Equal(ObjectSelector{})) + Expect(specified.View.DefaultSelector).To(Equal(ObjectSelector{})) + Expect(inherited.View.DefaultSelector).To(Equal(ObjectSelector{})) + Expect(checkError(specified.inheritFrom(inherited)).View.DefaultSelector).To(Equal(ObjectSelector{})) }) It("is unchanged when only specified is set", func() { - specified.DefaultSelector = ObjectSelector{Label: labels.Set{"specified": "true"}.AsSelector()} - Expect(checkError(specified.inheritFrom(inherited)).DefaultSelector).To(Equal(specified.DefaultSelector)) + specified.View.DefaultSelector = ObjectSelector{Label: labels.Set{"specified": "true"}.AsSelector()} + Expect(checkError(specified.inheritFrom(inherited)).View.DefaultSelector).To(Equal(specified.View.DefaultSelector)) }) It("is inherited when only inherited is set", func() { - inherited.DefaultSelector = ObjectSelector{Label: labels.Set{"inherited": "true"}.AsSelector()} - Expect(checkError(specified.inheritFrom(inherited)).DefaultSelector).To(Equal(inherited.DefaultSelector)) + inherited.View.DefaultSelector = ObjectSelector{Label: labels.Set{"inherited": "true"}.AsSelector()} + Expect(checkError(specified.inheritFrom(inherited)).View.DefaultSelector).To(Equal(inherited.View.DefaultSelector)) }) It("is combined when both inherited and specified are set", func() { - specified.DefaultSelector = ObjectSelector{ + specified.View.DefaultSelector = ObjectSelector{ Label: labels.Set{"specified": "true"}.AsSelector(), Field: fields.Set{"metadata.name": "specified"}.AsSelector(), } - inherited.DefaultSelector = ObjectSelector{ + inherited.View.DefaultSelector = ObjectSelector{ Label: labels.Set{"inherited": "true"}.AsSelector(), Field: fields.Set{"metadata.namespace": "inherited"}.AsSelector(), } - combined := checkError(specified.inheritFrom(inherited)).DefaultSelector + combined := checkError(specified.inheritFrom(inherited)).View.DefaultSelector Expect(combined).NotTo(BeNil()) Expect(combined.Label.Matches(labels.Set{"specified": "true"})).To(BeFalse()) Expect(combined.Label.Matches(labels.Set{"inherited": "true"})).To(BeFalse()) @@ -236,31 +236,31 @@ var _ = Describe("cache.inheritFrom", func() { }) Context("UnsafeDisableDeepCopyByObject", func() { It("is unchanged when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject).To(BeNil()) + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy).To(BeNil()) }) It("is unchanged when only specified is set", func() { specified.Scheme = coreScheme - specified.UnsafeDisableDeepCopyByObject = map[client.Object]bool{ObjectAll{}: true} - Expect(checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject).To(HaveLen(1)) + specified.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{ObjectAll{}: true} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy).To(HaveLen(1)) }) It("is inherited when only inherited is set", func() { inherited.Scheme = coreScheme - inherited.UnsafeDisableDeepCopyByObject = map[client.Object]bool{ObjectAll{}: true} - Expect(checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject).To(HaveLen(1)) + inherited.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{ObjectAll{}: true} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy).To(HaveLen(1)) }) It("is combined when both inherited and specified are set for different keys", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.Pod{}: true} - inherited.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.ConfigMap{}: true} - Expect(checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject).To(HaveLen(2)) + specified.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.Pod{}: true} + inherited.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.ConfigMap{}: true} + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy).To(HaveLen(2)) }) It("is true when inherited=false and specified=true for the same key", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.Pod{}: true} - inherited.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.Pod{}: false} - combined := checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject + specified.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.Pod{}: true} + inherited.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.Pod{}: false} + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy Expect(combined).To(HaveLen(1)) var ( @@ -275,9 +275,9 @@ var _ = Describe("cache.inheritFrom", func() { It("is false when inherited=true and specified=false for the same key", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.Pod{}: false} - inherited.UnsafeDisableDeepCopyByObject = map[client.Object]bool{&corev1.Pod{}: true} - combined := checkError(specified.inheritFrom(inherited)).UnsafeDisableDeepCopyByObject + specified.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.Pod{}: false} + inherited.View.ByObject.UnsafeDisableDeepCopy = map[client.Object]bool{&corev1.Pod{}: true} + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.UnsafeDisableDeepCopy Expect(combined).To(HaveLen(1)) var ( @@ -302,16 +302,16 @@ var _ = Describe("cache.inheritFrom", func() { tx = transformed{} }) It("is unchanged when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).TransformByObject).To(BeNil()) + Expect(checkError(specified.inheritFrom(inherited)).View.ByObject.Transform).To(BeNil()) }) It("is unchanged when only specified is set", func() { specified.Scheme = coreScheme - specified.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { + specified.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.podSpecified = true return ti, nil }} - combined := checkError(specified.inheritFrom(inherited)).TransformByObject + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.Transform Expect(combined).To(HaveLen(1)) for obj, fn := range combined { Expect(obj).To(BeAssignableToTypeOf(&corev1.Pod{})) @@ -325,12 +325,12 @@ var _ = Describe("cache.inheritFrom", func() { }) It("is inherited when only inherited is set", func() { inherited.Scheme = coreScheme - inherited.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { + inherited.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.podInherited = true return ti, nil }} - combined := checkError(specified.inheritFrom(inherited)).TransformByObject + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.Transform Expect(combined).To(HaveLen(1)) for obj, fn := range combined { Expect(obj).To(BeAssignableToTypeOf(&corev1.Pod{})) @@ -345,17 +345,17 @@ var _ = Describe("cache.inheritFrom", func() { It("is combined when both inherited and specified are set for different keys", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { + specified.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.podSpecified = true return ti, nil }} - inherited.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.ConfigMap{}: func(i interface{}) (interface{}, error) { + inherited.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.ConfigMap{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.configmapInherited = true return ti, nil }} - combined := checkError(specified.inheritFrom(inherited)).TransformByObject + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.Transform Expect(combined).To(HaveLen(2)) for obj, fn := range combined { out, _ := fn(tx) @@ -382,17 +382,17 @@ var _ = Describe("cache.inheritFrom", func() { It("is combined into a single transform function when both inherited and specified are set for the same key", func() { specified.Scheme = coreScheme inherited.Scheme = coreScheme - specified.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { + specified.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.podSpecified = true return ti, nil }} - inherited.TransformByObject = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { + inherited.View.ByObject.Transform = map[client.Object]cache.TransformFunc{&corev1.Pod{}: func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.podInherited = true return ti, nil }} - combined := checkError(specified.inheritFrom(inherited)).TransformByObject + combined := checkError(specified.inheritFrom(inherited)).View.ByObject.Transform Expect(combined).To(HaveLen(1)) for obj, fn := range combined { Expect(obj).To(BeAssignableToTypeOf(&corev1.Pod{})) @@ -417,15 +417,15 @@ var _ = Describe("cache.inheritFrom", func() { tx = transformed{} }) It("is unchanged when specified and inherited are unset", func() { - Expect(checkError(specified.inheritFrom(inherited)).DefaultTransform).To(BeNil()) + Expect(checkError(specified.inheritFrom(inherited)).View.DefaultTransform).To(BeNil()) }) It("is unchanged when only specified is set", func() { - specified.DefaultTransform = func(i interface{}) (interface{}, error) { + specified.View.DefaultTransform = func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.specified = true return ti, nil } - combined := checkError(specified.inheritFrom(inherited)).DefaultTransform + combined := checkError(specified.inheritFrom(inherited)).View.DefaultTransform out, _ := combined(tx) Expect(out).To(And( BeAssignableToTypeOf(tx), @@ -434,12 +434,12 @@ var _ = Describe("cache.inheritFrom", func() { )) }) It("is inherited when only inherited is set", func() { - inherited.DefaultTransform = func(i interface{}) (interface{}, error) { + inherited.View.DefaultTransform = func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.inherited = true return ti, nil } - combined := checkError(specified.inheritFrom(inherited)).DefaultTransform + combined := checkError(specified.inheritFrom(inherited)).View.DefaultTransform out, _ := combined(tx) Expect(out).To(And( BeAssignableToTypeOf(tx), @@ -448,17 +448,17 @@ var _ = Describe("cache.inheritFrom", func() { )) }) It("is combined when the transform function is defined in both inherited and specified", func() { - specified.DefaultTransform = func(i interface{}) (interface{}, error) { + specified.View.DefaultTransform = func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.specified = true return ti, nil } - inherited.DefaultTransform = func(i interface{}) (interface{}, error) { + inherited.View.DefaultTransform = func(i interface{}) (interface{}, error) { ti := i.(transformed) ti.inherited = true return ti, nil } - combined := checkError(specified.inheritFrom(inherited)).DefaultTransform + combined := checkError(specified.inheritFrom(inherited)).View.DefaultTransform Expect(combined).NotTo(BeNil()) out, _ := combined(tx) Expect(out).To(And( diff --git a/pkg/cache/internal/informers.go b/pkg/cache/internal/informers.go index 0005847dba..c69c4985ef 100644 --- a/pkg/cache/internal/informers.go +++ b/pkg/cache/internal/informers.go @@ -313,7 +313,7 @@ func (ip *Informers) addInformerToMap(gvk schema.GroupVersionKind, obj runtime.O opts.Watch = true // Watch needs to be set to true separately return listWatcher.WatchFunc(opts) }, - }, obj, resyncPeriod(ip.resync)(), cache.Indexers{ + }, obj, calculateResyncPeriod(ip.resync), cache.Indexers{ cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, }) @@ -506,15 +506,13 @@ func newGVKFixupWatcher(gvk schema.GroupVersionKind, watcher watch.Interface) wa ) } -// resyncPeriod returns a function which generates a duration each time it is -// invoked; this is so that multiple controllers don't get into lock-step and all +// calculateResyncPeriod returns a duration based on the desired input +// this is so that multiple controllers don't get into lock-step and all // hammer the apiserver with list requests simultaneously. -func resyncPeriod(resync time.Duration) func() time.Duration { - return func() time.Duration { - // the factor will fall into [0.9, 1.1) - factor := rand.Float64()/5.0 + 0.9 //nolint:gosec - return time.Duration(float64(resync.Nanoseconds()) * factor) - } +func calculateResyncPeriod(resync time.Duration) time.Duration { + // the factor will fall into [0.9, 1.1) + factor := rand.Float64()/5.0 + 0.9 //nolint:gosec + return time.Duration(float64(resync.Nanoseconds()) * factor) } // restrictNamespaceBySelector returns either a global restriction for all ListWatches diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 1d9d75af54..1bd2a8c96b 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -43,31 +43,43 @@ const globalCache = "_cluster-scope" // a global cache for cluster scoped resource. Note that this is not intended // to be used for excluding namespaces, this is better done via a Predicate. Also note that // you may face performance issues when using this with a high number of namespaces. +// +// Deprecated: Use cache.Options.View.Namespaces instead. func MultiNamespacedCacheBuilder(namespaces []string) NewCacheFunc { return func(config *rest.Config, opts Options) (Cache, error) { - opts, err := defaultOpts(config, opts) - if err != nil { - return nil, err - } + opts.View.Namespaces = namespaces + return newMultiNamespaceCache(config, opts) + } +} - caches := map[string]Cache{} +func newMultiNamespaceCache(config *rest.Config, opts Options) (Cache, error) { + if len(opts.View.Namespaces) < 2 { + return nil, fmt.Errorf("must specify more than one namespace to use multi-namespace cache") + } + opts, err := defaultOpts(config, opts) + if err != nil { + return nil, err + } - // create a cache for cluster scoped resources - gCache, err := New(config, opts) + // Create every namespace cache. + caches := map[string]Cache{} + for _, ns := range opts.View.Namespaces { + opts.View.Namespaces = []string{ns} + c, err := New(config, opts) if err != nil { - return nil, fmt.Errorf("error creating global cache: %w", err) + return nil, err } + caches[ns] = c + } - for _, ns := range namespaces { - opts.Namespace = ns - c, err := New(config, opts) - if err != nil { - return nil, err - } - caches[ns] = c - } - return &multiNamespaceCache{namespaceToCache: caches, Scheme: opts.Scheme, RESTMapper: opts.Mapper, clusterCache: gCache}, nil + // Create a cache for cluster scoped resources. + opts.View.Namespaces = []string{} + gCache, err := New(config, opts) + if err != nil { + return nil, fmt.Errorf("error creating global cache: %w", err) } + + return &multiNamespaceCache{namespaceToCache: caches, Scheme: opts.Scheme, RESTMapper: opts.Mapper, clusterCache: gCache}, nil } // multiNamespaceCache knows how to handle multiple namespaced caches diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index b95ee19e56..85f8821c9a 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -171,7 +171,15 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) { } // Create the cache for the cached read client and registering informers - cache, err := options.NewCache(config, cache.Options{HTTPClient: options.HTTPClient, Scheme: options.Scheme, Mapper: mapper, Resync: options.SyncPeriod, Namespace: options.Namespace}) + cache, err := options.NewCache(config, cache.Options{ + HTTPClient: options.HTTPClient, + Scheme: options.Scheme, + Mapper: mapper, + ResyncEvery: options.SyncPeriod, + View: cache.ViewOptions{ + Namespaces: []string{options.Namespace}, + }, + }) if err != nil { return nil, err } diff --git a/pkg/manager/example_test.go b/pkg/manager/example_test.go index 3d54ca3144..12c421bb83 100644 --- a/pkg/manager/example_test.go +++ b/pkg/manager/example_test.go @@ -20,6 +20,7 @@ import ( "context" "os" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client/config" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -50,7 +51,7 @@ func ExampleNew() { } // This example creates a new Manager that has a cache scoped to a list of namespaces. -func ExampleNew_multinamespaceCache() { +func ExampleNew_limitToNamespaces() { cfg, err := config.GetConfig() if err != nil { log.Error(err, "unable to get kubeconfig") @@ -58,8 +59,11 @@ func ExampleNew_multinamespaceCache() { } mgr, err := manager.New(cfg, manager.Options{ - NewCache: cache.MultiNamespacedCacheBuilder([]string{"namespace1", "namespace2"}), - }) + NewCache: func(config *rest.Config, opts cache.Options) (cache.Cache, error) { + opts.View.Namespaces = []string{"namespace1", "namespace2"} + return cache.New(config, opts) + }}, + ) if err != nil { log.Error(err, "unable to set up manager") os.Exit(1)