⚠️ 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

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

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

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