From 6ca011134391e795164e5a50309ef430f3d2fbe9 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 7 May 2023 16:40:39 -0400 Subject: [PATCH] :warning: Inteceptor: Unify Client and Subresource Client At the moment, the inteceptor has two different constructors for a client interceptor and a subresource interceptor. This means that in order to intercept subresource requests, one has to set up a normal interceptor, set the `SubResource` func to return a subresource interceptor and then configure the desired behavior on the subresource interceptor. This is needlessly complex, unify the two and add the parameters for the subresource funcs into the top-level `Funcs` struct. Also, change the signature of the subresource funcs to contain a `Client` instead of a `SubResourceClient` and the name of the subresource. This makes more sense, as for example if someone were to implement `Scale` this way, they likely want to check if the object to be scaled actually exists in the client and then modify it. --- pkg/client/interceptor/intercept.go | 84 +++++++------ pkg/client/interceptor/intercept_test.go | 112 ++++++------------ .../interceptor/interceptor_suite_test.go | 36 ++++++ 3 files changed, 113 insertions(+), 119 deletions(-) create mode 100644 pkg/client/interceptor/interceptor_suite_test.go diff --git a/pkg/client/interceptor/intercept.go b/pkg/client/interceptor/intercept.go index 986f7ea163..3d3f3cb011 100644 --- a/pkg/client/interceptor/intercept.go +++ b/pkg/client/interceptor/intercept.go @@ -10,38 +10,29 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -type ( - - // Funcs contains functions that are called instead of the underlying client's methods. - Funcs struct { - Get func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error - List func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error - Create func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error - Delete func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error - DeleteAllOf func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error - Update func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error - Patch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error - Watch func(ctx context.Context, client client.WithWatch, obj client.ObjectList, opts ...client.ListOption) (watch.Interface, error) - SubResource func(client client.WithWatch, subResource string) client.SubResourceClient - } - - // SubResourceFuncs is a set of functions that can be used to intercept calls to a SubResourceClient. - SubResourceFuncs struct { - Get func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error - Create func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error - Update func(ctx context.Context, client client.SubResourceClient, obj client.Object, opts ...client.SubResourceUpdateOption) error - Patch func(ctx context.Context, client client.SubResourceClient, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error - } -) +// Funcs contains functions that are called instead of the underlying client's methods. +type Funcs struct { + Get func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error + List func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error + Create func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error + Delete func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error + DeleteAllOf func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error + Update func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error + Patch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error + Watch func(ctx context.Context, client client.WithWatch, obj client.ObjectList, opts ...client.ListOption) (watch.Interface, error) + SubResource func(client client.WithWatch, subResource string) client.SubResourceClient + SubResourceGet func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error + SubResourceCreate func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error + SubResourceUpdate func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error + SubResourcePatch func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error +} // NewClient returns a new interceptor client that calls the functions in funcs instead of the underlying client's methods, if they are not nil. func NewClient(interceptedClient client.WithWatch, funcs Funcs) client.WithWatch { - return interceptor{client: interceptedClient, funcs: funcs} -} - -// NewSubResourceClient returns a SubResourceClient that intercepts calls to the provided client with the provided functions. -func NewSubResourceClient(interceptedClient client.SubResourceClient, funcs SubResourceFuncs) client.SubResourceClient { - return subResourceInterceptor{client: interceptedClient, funcs: funcs} + return interceptor{ + client: interceptedClient, + funcs: funcs, + } } type interceptor struct { @@ -116,7 +107,11 @@ func (c interceptor) SubResource(subResource string) client.SubResourceClient { if c.funcs.SubResource != nil { return c.funcs.SubResource(c.client, subResource) } - return c.client.SubResource(subResource) + return subResourceInterceptor{ + subResourceName: subResource, + client: c.client, + funcs: c.funcs, + } } func (c interceptor) Scheme() *runtime.Scheme { @@ -135,36 +130,37 @@ func (c interceptor) Watch(ctx context.Context, obj client.ObjectList, opts ...c } type subResourceInterceptor struct { - client client.SubResourceClient - funcs SubResourceFuncs + subResourceName string + client client.Client + funcs Funcs } var _ client.SubResourceClient = &subResourceInterceptor{} func (s subResourceInterceptor) Get(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error { - if s.funcs.Get != nil { - return s.funcs.Get(ctx, s.client, obj, subResource, opts...) + if s.funcs.SubResourceGet != nil { + return s.funcs.SubResourceGet(ctx, s.client, s.subResourceName, obj, subResource, opts...) } - return s.client.Get(ctx, obj, subResource, opts...) + return s.client.SubResource(s.subResourceName).Get(ctx, obj, subResource, opts...) } func (s subResourceInterceptor) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { - if s.funcs.Create != nil { - return s.funcs.Create(ctx, s.client, obj, subResource, opts...) + if s.funcs.SubResourceCreate != nil { + return s.funcs.SubResourceCreate(ctx, s.client, s.subResourceName, obj, subResource, opts...) } - return s.client.Create(ctx, obj, subResource, opts...) + return s.client.SubResource(s.subResourceName).Create(ctx, obj, subResource, opts...) } func (s subResourceInterceptor) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - if s.funcs.Update != nil { - return s.funcs.Update(ctx, s.client, obj, opts...) + if s.funcs.SubResourceUpdate != nil { + return s.funcs.SubResourceUpdate(ctx, s.client, s.subResourceName, obj, opts...) } - return s.client.Update(ctx, obj, opts...) + return s.client.SubResource(s.subResourceName).Update(ctx, obj, opts...) } func (s subResourceInterceptor) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { - if s.funcs.Patch != nil { - return s.funcs.Patch(ctx, s.client, obj, patch, opts...) + if s.funcs.SubResourcePatch != nil { + return s.funcs.SubResourcePatch(ctx, s.client, s.subResourceName, obj, patch, opts...) } - return s.client.Patch(ctx, obj, patch, opts...) + return s.client.SubResource(s.subResourceName).Patch(ctx, obj, patch, opts...) } diff --git a/pkg/client/interceptor/intercept_test.go b/pkg/client/interceptor/intercept_test.go index 9e88f0837a..a0536789b1 100644 --- a/pkg/client/interceptor/intercept_test.go +++ b/pkg/client/interceptor/intercept_test.go @@ -212,18 +212,6 @@ var _ = Describe("NewClient", func() { _ = client.SubResource("") Expect(called).To(BeTrue()) }) - It("should call the underlying client if the provided SubResource function is nil", func() { - var called bool - client1 := NewClient(wrappedClient, Funcs{ - SubResource: func(client client.WithWatch, subResource string) client.SubResourceClient { - called = true - return nil - }, - }) - client2 := NewClient(client1, Funcs{}) - _ = client2.SubResource("") - Expect(called).To(BeTrue()) - }) It("should call the provided SubResource function with 'status' when calling Status()", func() { var called bool client := NewClient(wrappedClient, Funcs{ @@ -237,115 +225,109 @@ var _ = Describe("NewClient", func() { _ = client.Status() Expect(called).To(BeTrue()) }) - It("should call the underlying client if the provided SubResource function is nil when calling Status", func() { - var called bool - client1 := NewClient(wrappedClient, Funcs{ - SubResource: func(client client.WithWatch, subResource string) client.SubResourceClient { - if subResource == "status" { - called = true - } - return nil - }, - }) - client2 := NewClient(client1, Funcs{}) - _ = client2.Status() - Expect(called).To(BeTrue()) - }) }) var _ = Describe("NewSubResourceClient", func() { - srClient := dummySubResourceClient{} + c := dummyClient{} ctx := context.Background() It("should call the provided Get function", func() { var called bool - client := NewSubResourceClient(srClient, SubResourceFuncs{ - Get: func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error { + c := NewClient(c, Funcs{ + SubResourceGet: func(_ context.Context, client client.Client, subResourceName string, obj, subResource client.Object, opts ...client.SubResourceGetOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - _ = client.Get(ctx, nil, nil) + _ = c.SubResource("foo").Get(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the underlying client if the provided Get function is nil", func() { var called bool - client1 := NewSubResourceClient(srClient, SubResourceFuncs{ - Get: func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error { + client1 := NewClient(c, Funcs{ + SubResourceGet: func(_ context.Context, client client.Client, subResourceName string, obj, subResource client.Object, opts ...client.SubResourceGetOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - client2 := NewSubResourceClient(client1, SubResourceFuncs{}) - _ = client2.Get(ctx, nil, nil) + client2 := NewClient(client1, Funcs{}) + _ = client2.SubResource("foo").Get(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the provided Update function", func() { var called bool - client := NewSubResourceClient(srClient, SubResourceFuncs{ - Update: func(ctx context.Context, client client.SubResourceClient, obj client.Object, opts ...client.SubResourceUpdateOption) error { + client := NewClient(c, Funcs{ + SubResourceUpdate: func(_ context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - _ = client.Update(ctx, nil, nil) + _ = client.SubResource("foo").Update(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the underlying client if the provided Update function is nil", func() { var called bool - client1 := NewSubResourceClient(srClient, SubResourceFuncs{ - Update: func(ctx context.Context, client client.SubResourceClient, obj client.Object, opts ...client.SubResourceUpdateOption) error { + client1 := NewClient(c, Funcs{ + SubResourceUpdate: func(_ context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - client2 := NewSubResourceClient(client1, SubResourceFuncs{}) - _ = client2.Update(ctx, nil, nil) + client2 := NewClient(client1, Funcs{}) + _ = client2.SubResource("foo").Update(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the provided Patch function", func() { var called bool - client := NewSubResourceClient(srClient, SubResourceFuncs{ - Patch: func(ctx context.Context, client client.SubResourceClient, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + client := NewClient(c, Funcs{ + SubResourcePatch: func(_ context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - _ = client.Patch(ctx, nil, nil) + _ = client.SubResource("foo").Patch(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the underlying client if the provided Patch function is nil", func() { var called bool - client1 := NewSubResourceClient(srClient, SubResourceFuncs{ - Patch: func(ctx context.Context, client client.SubResourceClient, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + client1 := NewClient(c, Funcs{ + SubResourcePatch: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - client2 := NewSubResourceClient(client1, SubResourceFuncs{}) - _ = client2.Patch(ctx, nil, nil) + client2 := NewClient(client1, Funcs{}) + _ = client2.SubResource("foo").Patch(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the provided Create function", func() { var called bool - client := NewSubResourceClient(srClient, SubResourceFuncs{ - Create: func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { + client := NewClient(c, Funcs{ + SubResourceCreate: func(_ context.Context, client client.Client, subResourceName string, obj, subResource client.Object, opts ...client.SubResourceCreateOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - _ = client.Create(ctx, nil, nil) + _ = client.SubResource("foo").Create(ctx, nil, nil) Expect(called).To(BeTrue()) }) It("should call the underlying client if the provided Create function is nil", func() { var called bool - client1 := NewSubResourceClient(srClient, SubResourceFuncs{ - Create: func(ctx context.Context, client client.SubResourceClient, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { + client1 := NewClient(c, Funcs{ + SubResourceCreate: func(_ context.Context, client client.Client, subResourceName string, obj, subResource client.Object, opts ...client.SubResourceCreateOption) error { called = true + Expect(subResourceName).To(BeEquivalentTo("foo")) return nil }, }) - client2 := NewSubResourceClient(client1, SubResourceFuncs{}) - _ = client2.Create(ctx, nil, nil) + client2 := NewClient(client1, Funcs{}) + _ = client2.SubResource("foo").Create(ctx, nil, nil) Expect(called).To(BeTrue()) }) }) @@ -409,23 +391,3 @@ func (d dummyClient) IsObjectNamespaced(obj runtime.Object) (bool, error) { func (d dummyClient) Watch(ctx context.Context, obj client.ObjectList, opts ...client.ListOption) (watch.Interface, error) { return nil, nil } - -type dummySubResourceClient struct{} - -var _ client.SubResourceClient = &dummySubResourceClient{} - -func (d dummySubResourceClient) Get(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error { - return nil -} - -func (d dummySubResourceClient) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { - return nil -} - -func (d dummySubResourceClient) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - return nil -} - -func (d dummySubResourceClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { - return nil -} diff --git a/pkg/client/interceptor/interceptor_suite_test.go b/pkg/client/interceptor/interceptor_suite_test.go new file mode 100644 index 0000000000..08d9fe2281 --- /dev/null +++ b/pkg/client/interceptor/interceptor_suite_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2022 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 interceptor + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestInterceptor(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Fake client Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) +})