From aa1bfee605cea988a4a79e9021ad1d4334b09eb8 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Wed, 3 Mar 2021 15:01:02 -0500 Subject: [PATCH] :warning: 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. --- pkg/cluster/client_builder.go | 62 ----------------------------------- pkg/cluster/cluster.go | 31 +++++++++++++----- pkg/cluster/cluster_test.go | 21 ++++-------- pkg/manager/client_builder.go | 29 ---------------- pkg/manager/manager.go | 6 ++-- pkg/manager/manager_test.go | 20 ++++------- 6 files changed, 39 insertions(+), 130 deletions(-) delete mode 100644 pkg/cluster/client_builder.go delete mode 100644 pkg/manager/client_builder.go diff --git a/pkg/cluster/client_builder.go b/pkg/cluster/client_builder.go deleted file mode 100644 index 791ce16061..0000000000 --- a/pkg/cluster/client_builder.go +++ /dev/null @@ -1,62 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package cluster - -import ( - "k8s.io/client-go/rest" - - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// ClientBuilder builder is the interface for the client builder. -type ClientBuilder interface { - // WithUncached takes a list of runtime objects (plain or lists) that users don't want to cache - // for this client. This function can be called multiple times, it should append to an internal slice. - WithUncached(objs ...client.Object) ClientBuilder - - // Build returns a new client. - Build(cache cache.Cache, config *rest.Config, options client.Options) (client.Client, error) -} - -// NewClientBuilder returns a builder to build new clients to be passed when creating a Manager. -func NewClientBuilder() ClientBuilder { - return &newClientBuilder{} -} - -type newClientBuilder struct { - uncached []client.Object -} - -func (n *newClientBuilder) WithUncached(objs ...client.Object) ClientBuilder { - n.uncached = append(n.uncached, objs...) - return n -} - -func (n *newClientBuilder) Build(cache cache.Cache, config *rest.Config, options client.Options) (client.Client, error) { - // Create the Client for Write operations. - c, err := client.New(config, options) - if err != nil { - return nil, err - } - - return client.NewDelegatingClient(client.NewDelegatingClientInput{ - CacheReader: cache, - Client: c, - UncachedObjects: n.uncached, - }) -} diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 1d0ad40537..76fa72ad76 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.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, + }) +} diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index c407869fa0..d0b03785c3 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.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()) diff --git a/pkg/manager/client_builder.go b/pkg/manager/client_builder.go deleted file mode 100644 index e2fea8d1f7..0000000000 --- a/pkg/manager/client_builder.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package manager - -import ( - "sigs.k8s.io/controller-runtime/pkg/cluster" -) - -// ClientBuilder builder is the interface for the client builder. -type ClientBuilder = cluster.ClientBuilder - -// NewClientBuilder returns a builder to build new clients to be passed when creating a Manager. -func NewClientBuilder() ClientBuilder { - return cluster.NewClientBuilder() -} diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 686dca1a57..9504cadf4f 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.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 diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go index 8161ad2a70..954f27377d 100644 --- a/pkg/manager/manager_test.go +++ b/pkg/manager/manager_test.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())