Refactor w/ simple boolean FailOnUnknownResource

Signed-off-by: Vince Prignano <[email protected]>

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

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

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
 }
 

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() {

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)
 	}
 }
 

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")

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,48 +284,14 @@ 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())
 			Expect(errors.As(cl.List(ctx, &appsv1.DeploymentList{}), &errNotCached)).To(BeTrue())
 			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
 }

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
-}