diff --git a/Makefile b/Makefile index 71ec644de0..c040931548 100644 --- a/Makefile +++ b/Makefile @@ -110,6 +110,7 @@ generate: $(CONTROLLER_GEN) ## Runs controller-gen for internal types for config .PHONY: clean clean: ## Cleanup. + $(GOLANGCI_LINT) cache clean $(MAKE) clean-bin .PHONY: clean-bin diff --git a/go.mod b/go.mod index a5d7da8406..e8456bd506 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module sigs.k8s.io/controller-runtime go 1.20 require ( + github.com/davecgh/go-spew v1.1.1 github.com/evanphx/json-patch v5.6.0+incompatible github.com/evanphx/json-patch/v5 v5.6.0 github.com/fsnotify/fsnotify v1.6.0 @@ -30,7 +31,6 @@ require ( require ( github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect - github.com/davecgh/go-spew v1.1.1 // indirect github.com/emicklei/go-restful/v3 v3.9.0 // indirect github.com/go-openapi/jsonpointer v0.19.6 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index ccb02b3a80..f4e7152c26 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -167,29 +167,14 @@ type Options struct { // This is a global setting for all objects, and can be overridden by the ByObject setting. UnsafeDisableDeepCopy *bool - // UnknownResourcePolicy determines how the cache should behave when no informers are started for the - // resource type a client is asking for in a Get or a List. See the UnknownResourcePolicies - // for documentation for each policy. The default policy is UnknownResourcePolicyBackfill. - UnknownResourcePolicy UnknownResourcePolicy -} - -// UnknownResourcePolicy determines how the cache should behave when no informers are started for the -// resource type a client is asking for in a Get or a List. -type UnknownResourcePolicy string - -const ( - // UnknownResourcePolicyBackfill configures the cache to spin up an informer for a resource when - // the first request for that resource comes in. This means that a Get or a List may take - // longer than normal to succeed on the first invocation as the cache waits until it's fully - // back-filled. - // This is the default policy. - UnknownResourcePolicyBackfill UnknownResourcePolicy = "backfill" - - // UnknownResourcePolicyFail configures the cache to return a ErrResourceNotCached error when a user + // FailOnUnknownResource configures the cache to return a ErrResourceNotCached error when a user // requests a resource the cache is not configured to hold. This error is distinct from an // errors.NotFound. - UnknownResourcePolicyFail UnknownResourcePolicy = "fail" -) + // + // Defaults to false, which means that the cache will start a new informer + // for every new requested resource. + FailOnUnknownResource bool +} // ByObject offers more fine-grained control over the cache's ListWatch by object. type ByObject struct { @@ -255,7 +240,7 @@ func New(config *rest.Config, opts Options) (Cache, error) { Namespace: opts.Namespaces[0], ByGVK: byGVK, }), - missPolicy: opts.UnknownResourcePolicy, + failOnUnknown: opts.FailOnUnknownResource, }, nil } @@ -292,10 +277,6 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) { opts.SyncPeriod = &defaultSyncPeriod } - // Backfill by default - if opts.UnknownResourcePolicy == "" { - opts.UnknownResourcePolicy = UnknownResourcePolicyBackfill - } return opts, nil } diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 65ada13af8..ce66ace332 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -121,7 +121,7 @@ var _ = Describe("Informer Cache", func() { }) var _ = Describe("Informer Cache with miss policy = fail", func() { - CacheTestMissPolicyFail(cache.New, cache.Options{UnknownResourcePolicy: cache.UnknownResourcePolicyFail}) + CacheTestMissPolicyFail(cache.New, cache.Options{FailOnUnknownResource: true}) }) var _ = Describe("Multi-Namespace Informer Cache", func() { diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index cf3b673831..836a38ade3 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -51,7 +51,7 @@ func (*ErrCacheNotStarted) Error() string { type informerCache struct { scheme *runtime.Scheme *internal.Informers - missPolicy UnknownResourcePolicy + failOnUnknown bool } // Get implements Reader. @@ -154,17 +154,15 @@ func (ic *informerCache) GetInformer(ctx context.Context, obj client.Object) (In } func (ic *informerCache) getInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, obj runtime.Object) (bool, *internal.Cache, error) { - switch ic.missPolicy { - case UnknownResourcePolicyFail: + switch { + case ic.failOnUnknown: cache, started, ok := ic.Informers.Peek(gvk, obj) if !ok { return false, nil, &errors.ErrResourceNotCached{GVK: gvk} } return started, cache, nil - case UnknownResourcePolicyBackfill: - return ic.Informers.Get(ctx, gvk, obj) default: - panic(fmt.Errorf("invalid cache miss policy %q, programmer error", ic.missPolicy)) + return ic.Informers.Get(ctx, gvk, obj) } } diff --git a/pkg/client/client.go b/pkg/client/client.go index 704789e942..a24df8ba2e 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -78,34 +78,17 @@ type CacheOptions struct { // Reader is a cache-backed reader that will be used to read objects from the cache. // +required Reader Reader - // DisableFor is a list of objects that should not be read from the cache. See the - // UncachedResourcePolicy for what the client will do when encountering requests for - // these kinds. + // DisableFor is a list of objects that should never be read from the cache. + // Objects configured here always result in a live lookup. DisableFor []Object // Unstructured is a flag that indicates whether the cache-backed client should // read unstructured objects or lists from the cache. + // If false, unstructured objects will always result in a live lookup. Unstructured bool - // UncachedResourcePolicy determines how the client should behave when the underlying cache is - // configured to fail when encountering an unknown resource type or when the user asks the client - // for resources in the DisableFor set. See the UncachedResourcePolicies for documentation for - // each policy. The default policy is UncachedResourcePolicyLiveLookup. - UncachedResourcePolicy UncachedResourcePolicy -} - -// UncachedResourcePolicy determines how the client should behave when the underlying cache is -// configured to fail on a miss. -type UncachedResourcePolicy string - -const ( - // UncachedResourcePolicyLiveLookup configures the client to issue a live client lookup to get data - // for a resource that is not cached. - // This is the default policy. - UncachedResourcePolicyLiveLookup UncachedResourcePolicy = "live-lookup" - - // UncachedResourcePolicyFail configures the client to return an errors.NotFound error when a user - // requests a resource the cache is not configured to hold. - UncachedResourcePolicyFail UncachedResourcePolicy = "fail" -) + // FailOnUnknownResource determines how the client should behave when the client + // is asked to read an object from the cache, and ErrResourceNotCached is returned. + FailOnUnknownResource bool +} // NewClientFunc allows a user to define how to create a client. type NewClientFunc func(config *rest.Config, options Options) (Client, error) @@ -208,15 +191,10 @@ func newClient(config *rest.Config, options Options) (*client, error) { return c, nil } - // Default to live lookups when missing in the cache - if options.Cache.UncachedResourcePolicy == "" { - options.Cache.UncachedResourcePolicy = UncachedResourcePolicyLiveLookup - } - // We want a cache if we're here. // Set the cache. c.cache = options.Cache.Reader - c.uncachedResourcePolicy = options.Cache.UncachedResourcePolicy + c.failOnUncached = options.Cache.FailOnUnknownResource // Load uncached GVKs. c.cacheUnstructured = options.Cache.Unstructured @@ -242,10 +220,10 @@ type client struct { scheme *runtime.Scheme mapper meta.RESTMapper - cache Reader - uncachedResourcePolicy UncachedResourcePolicy - uncachedGVKs map[schema.GroupVersionKind]struct{} - cacheUnstructured bool + cache Reader + uncachedGVKs map[schema.GroupVersionKind]struct{} + failOnUncached bool + cacheUnstructured bool } func (c *client) shouldBypassCache(obj runtime.Object) (bool, error) { @@ -263,23 +241,15 @@ func (c *client) shouldBypassCache(obj runtime.Object) (bool, error) { gvk.Kind = strings.TrimSuffix(gvk.Kind, "List") } if _, isUncached := c.uncachedGVKs[gvk]; isUncached { - bypass := true - return bypass, c.shouldBypassCacheErr(gvk, bypass) + return true, nil } if !c.cacheUnstructured { _, isUnstructured := obj.(runtime.Unstructured) - return isUnstructured, c.shouldBypassCacheErr(gvk, isUnstructured) + return isUnstructured, nil } return false, nil } -func (c *client) shouldBypassCacheErr(gvk schema.GroupVersionKind, bypass bool) error { - if bypass && c.uncachedResourcePolicy == UncachedResourcePolicyFail { - return &cacheerrors.ErrResourceNotCached{GVK: gvk} - } - return nil -} - // resetGroupVersionKind is a helper function to restore and preserve GroupVersionKind on an object. func (c *client) resetGroupVersionKind(obj runtime.Object, gvk schema.GroupVersionKind) { if gvk != schema.EmptyObjectKind.GroupVersionKind() { @@ -376,11 +346,13 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj Object, opts ...Get if isUncached, err := c.shouldBypassCache(obj); err != nil { return err } else if !isUncached { + // Attempt to get from the cache. if err := c.cache.Get(ctx, key, obj, opts...); c.shouldReturnCacheErr(err) { return err } } + // Perform a live lookup. switch obj.(type) { case runtime.Unstructured: return c.unstructuredClient.Get(ctx, key, obj, opts...) @@ -393,23 +365,18 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj Object, opts ...Get } } -// shouldReturnCacheErr determines if we should return the error we got from the cache, which we do in every case -// except for when we're configured to do live lookups and the cache returns cache.ErrResourceNotCached -func (c *client) shouldReturnCacheErr(err error) bool { - var errNotCached *cacheerrors.ErrResourceNotCached - return !(c.uncachedResourcePolicy == UncachedResourcePolicyLiveLookup && errors.As(err, &errNotCached)) -} - // List implements client.Client. func (c *client) List(ctx context.Context, obj ObjectList, opts ...ListOption) error { if isUncached, err := c.shouldBypassCache(obj); err != nil { return err } else if !isUncached { + // Attempt to get from the cache. if err := c.cache.List(ctx, obj, opts...); c.shouldReturnCacheErr(err) { return err } } + // Perform a live lookup. switch x := obj.(type) { case runtime.Unstructured: return c.unstructuredClient.List(ctx, obj, opts...) @@ -442,6 +409,16 @@ func (c *client) List(ctx context.Context, obj ObjectList, opts ...ListOption) e } } +// shouldReturnCacheErr determines if we should return the error we got from the cache. +func (c *client) shouldReturnCacheErr(err error) bool { + if !c.failOnUncached && errors.As(err, &cacheerrors.ErrResourceNotCached{}) { + // Ignore errors if we're not configured to fail on uncached resources. + return false + } + // Return in any other case. + return true +} + // Status implements client.StatusClient. func (c *client) Status() SubResourceWriter { return c.SubResource("status") diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index e242013950..86799bd217 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -25,6 +25,7 @@ import ( "sync/atomic" "time" + "github.com/davecgh/go-spew/spew" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -283,7 +284,7 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC It("should use the provided reader cache if provided, but fail when configured to on cache misses for get and list", func() { c := &fakeUncachedReader{} - cl, err := client.New(cfg, client.Options{Cache: &client.CacheOptions{Reader: c, UncachedResourcePolicy: client.UncachedResourcePolicyFail}}) + cl, err := client.New(cfg, client.Options{Cache: &client.CacheOptions{Reader: c, FailOnUnknownResource: true}}) Expect(err).NotTo(HaveOccurred()) Expect(cl).NotTo(BeNil()) Expect(errors.As(cl.Get(ctx, client.ObjectKey{Name: "test"}, &appsv1.Deployment{}), &errNotCached)).To(BeTrue()) @@ -291,40 +292,6 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC Expect(c.Called).To(Equal(2)) }) - It("should use the provided reader cache if provided, but live lookup when configured to on cache misses for get and list", func() { - cache := &fakeUncachedReader{} - cl, err := client.New(cfg, client.Options{Cache: &client.CacheOptions{Reader: cache, UncachedResourcePolicy: client.UncachedResourcePolicyLiveLookup}}) - Expect(err).NotTo(HaveOccurred()) - Expect(cl).NotTo(BeNil()) - - By("creating the object") - err = cl.Create(context.TODO(), dep) - Expect(err).NotTo(HaveOccurred()) - - By("getting the object with a get") - var actual appsv1.Deployment - Expect(cl.Get(ctx, client.ObjectKey{Name: dep.Name, Namespace: dep.Namespace}, &actual)).To(Succeed()) - Expect(dep).To(Equal(&actual)) - - By("getting the object with a list") - var list appsv1.DeploymentList - Expect(cl.List(ctx, &list)).To(Succeed()) - Expect(list.Items).To(HaveLen(1)) - Expect(dep).To(Equal(&list.Items[0])) - - Expect(cache.Called).To(Equal(2)) - }) - - It("should use the provided reader cache if provided, but forward cache errors even if configured to live lookup on cache misses for get and list", func() { - c := &fakeErrorReader{} - cl, err := client.New(cfg, client.Options{Cache: &client.CacheOptions{Reader: c, UncachedResourcePolicy: client.UncachedResourcePolicyLiveLookup}}) - Expect(err).NotTo(HaveOccurred()) - Expect(cl).NotTo(BeNil()) - Expect(cl.Get(ctx, client.ObjectKey{Name: "test"}, &appsv1.Deployment{})).To(MatchError(ContainSubstring("sentinel"))) - Expect(cl.List(ctx, &appsv1.DeploymentList{})).To(MatchError(ContainSubstring("sentinel"))) - Expect(c.Called).To(Equal(2)) - }) - It("should not use the provided reader cache if provided, on get and list for uncached GVKs", func() { cache := &fakeReader{} cl, err := client.New(cfg, client.Options{Cache: &client.CacheOptions{Reader: cache, DisableFor: []client.Object{&corev1.Namespace{}}}}) @@ -3976,6 +3943,7 @@ type fakeReader struct { } func (f *fakeReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + spew.Dump("key") f.Called++ return nil } diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index ec72ef30a7..7e65ef0c3a 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -19,7 +19,6 @@ package manager import ( "context" "crypto/tls" - "errors" "fmt" "net" "net/http" @@ -395,10 +394,6 @@ func New(config *rest.Config, options Options) (Manager, error) { // Set default values for options fields options = setOptionsDefaults(options) - if err := validateOptions(options); err != nil { - return nil, err - } - cluster, err := cluster.New(config, func(clusterOptions *cluster.Options) { clusterOptions.Scheme = options.Scheme clusterOptions.MapperProvider = options.MapperProvider @@ -749,16 +744,3 @@ func setOptionsDefaults(options Options) Options { return options } - -func validateOptions(options Options) error { - if options.Client.Cache == nil || options.Client.Cache.UncachedResourcePolicy == "" { - return nil - } - cacheConfiguredToExcludeResources := len(options.Client.Cache.DisableFor) > 0 || options.Cache.UnknownResourcePolicy == cache.UnknownResourcePolicyFail - clientConfiguredToHandleExcludedResources := options.Client.Cache.UncachedResourcePolicy != "" - if !cacheConfiguredToExcludeResources && clientConfiguredToHandleExcludedResources { - return errors.New("you've configured the client to handle uncached resource kinds, but have not configured the cache to exclude any resources") - } - - return nil -}