⚠️ Allow setting NewClientFunc w/o implementing an interface

When we introduced the ClientBuilder we lost the ability to easily
overwrite the NewClient func, as interfaces can not be implemented
inline. This change reverts us back to using a NewClientFunc but with a
slightly changed signature so objects that should not be cached can
still be configured. In order to not provide two ways to do the same
thing, the ClientBuilder is removed.

Author: alvaroaleman

pkg/cluster/client_builder.go

@@ -109,10 +109,10 @@ type Options struct {
 	// by the manager. If not set this will use the default new cache function.
 	NewCache cache.NewCacheFunc
 
-	// ClientBuilder is the builder that creates the client to be used by the manager.
+	// NewClient is the func that creates the client to be used by the manager.
 	// If not set this will create the default DelegatingClient that will
 	// use the cache for reads and the client for writes.
-	ClientBuilder ClientBuilder
+	NewClient NewClientFunc
 
 	// ClientDisableCacheFor tells the client that, if any cache is used, to bypass it
 	// for the given objects.
@@ -174,9 +174,7 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
 		return nil, err
 	}
 
-	writeObj, err := options.ClientBuilder.
-		WithUncached(options.ClientDisableCacheFor...).
-		Build(cache, config, clientOptions)
+	writeObj, err := options.NewClient(cache, config, clientOptions, options.ClientDisableCacheFor...)
 	if err != nil {
 		return nil, err
 	}
@@ -219,9 +217,9 @@ func setOptionsDefaults(options Options) Options {
 		}
 	}
 
-	// Allow the client builder to be mocked
-	if options.ClientBuilder == nil {
-		options.ClientBuilder = NewClientBuilder()
+	// Allow users to define how to create a new client
+	if options.NewClient == nil {
+		options.NewClient = DefaultNewClient
 	}
 
 	// Allow newCache to be mocked
@@ -253,3 +251,20 @@ func setOptionsDefaults(options Options) Options {
 
 	return options
 }
+
+// NewClientFunc allows a user to define how to create a client
+type NewClientFunc func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error)
+
+// DefaultNewClient creates the default caching client
+func DefaultNewClient(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+	c, err := client.New(config, options)
+	if err != nil {
+		return nil, err
+	}
+
+	return client.NewDelegatingClient(client.NewDelegatingClientInput{
+		CacheReader:     cache,
+		Client:          c,
+		UncachedObjects: uncachedObjects,
+	})
+}

pkg/cluster/cluster.go

@@ -18,6 +18,7 @@ package cluster
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/go-logr/logr"
@@ -35,18 +36,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
 )
 
-type fakeClientBuilder struct {
-	err error
-}
-
-func (e *fakeClientBuilder) WithUncached(objs ...client.Object) ClientBuilder {
-	return e
-}
-
-func (e *fakeClientBuilder) Build(cache cache.Cache, config *rest.Config, options client.Options) (client.Client, error) {
-	return nil, e.err
-}
-
 var _ = Describe("cluster.Cluster", func() {
 	Describe("New", func() {
 		It("should return an error if there is no Config", func() {
@@ -68,7 +57,9 @@ var _ = Describe("cluster.Cluster", func() {
 
 		It("should return an error it can't create a client.Client", func(done Done) {
 			c, err := New(cfg, func(o *Options) {
-				o.ClientBuilder = &fakeClientBuilder{err: fmt.Errorf("expected error")}
+				o.NewClient = func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+					return nil, errors.New("expected error")
+				}
 			})
 			Expect(c).To(BeNil())
 			Expect(err).To(HaveOccurred())
@@ -92,7 +83,9 @@ var _ = Describe("cluster.Cluster", func() {
 
 		It("should create a client defined in by the new client function", func(done Done) {
 			c, err := New(cfg, func(o *Options) {
-				o.ClientBuilder = &fakeClientBuilder{}
+				o.NewClient = func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+					return nil, nil
+				}
 			})
 			Expect(c).ToNot(BeNil())
 			Expect(err).ToNot(HaveOccurred())

pkg/cluster/cluster_test.go

@@ -207,10 +207,10 @@ type Options struct {
 	// by the manager. If not set this will use the default new cache function.
 	NewCache cache.NewCacheFunc
 
-	// ClientBuilder is the builder that creates the client to be used by the manager.
+	// NewClient is the func that creates the client to be used by the manager.
 	// If not set this will create the default DelegatingClient that will
 	// use the cache for reads and the client for writes.
-	ClientBuilder ClientBuilder
+	NewClient cluster.NewClientFunc
 
 	// ClientDisableCacheFor tells the client that, if any cache is used, to bypass it
 	// for the given objects.
@@ -290,7 +290,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		clusterOptions.SyncPeriod = options.SyncPeriod
 		clusterOptions.Namespace = options.Namespace
 		clusterOptions.NewCache = options.NewCache
-		clusterOptions.ClientBuilder = options.ClientBuilder
+		clusterOptions.NewClient = options.NewClient
 		clusterOptions.ClientDisableCacheFor = options.ClientDisableCacheFor
 		clusterOptions.DryRunClient = options.DryRunClient
 		clusterOptions.EventBroadcaster = options.EventBroadcaster

pkg/manager/client_builder.go

@@ -55,18 +55,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
 )
 
-type fakeClientBuilder struct {
-	err error
-}
-
-func (e *fakeClientBuilder) WithUncached(objs ...client.Object) ClientBuilder {
-	return e
-}
-
-func (e *fakeClientBuilder) Build(cache cache.Cache, config *rest.Config, options client.Options) (client.Client, error) {
-	return nil, e.err
-}
-
 var _ = Describe("manger.Manager", func() {
 	Describe("New", func() {
 		It("should return an error if there is no Config", func() {
@@ -88,7 +76,9 @@ var _ = Describe("manger.Manager", func() {
 
 		It("should return an error it can't create a client.Client", func(done Done) {
 			m, err := New(cfg, Options{
-				ClientBuilder: &fakeClientBuilder{err: fmt.Errorf("expected error")},
+				NewClient: func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+					return nil, errors.New("expected error")
+				},
 			})
 			Expect(m).To(BeNil())
 			Expect(err).To(HaveOccurred())
@@ -112,7 +102,9 @@ var _ = Describe("manger.Manager", func() {
 
 		It("should create a client defined in by the new client function", func(done Done) {
 			m, err := New(cfg, Options{
-				ClientBuilder: &fakeClientBuilder{},
+				NewClient: func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+					return nil, nil
+				},
 			})
 			Expect(m).ToNot(BeNil())
 			Expect(err).ToNot(HaveOccurred())