Merge pull request #63525 from enj/enj/i/flatten_transform_requests

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Correctly apply request transforms with flattened resource builder

This change moves the NewClientWithOptions call into Builder.getClient.  Since getClient is the only way for Builder and its visitors to create a RESTClient, we can reasonably guarantee that the request transforms will be honored.  Previously, it was possible for a call to NewFlattenListVisitor to return resource Info objects whose Client field did not honor the request transforms.

Signed-off-by: Monis Khan <mkhan@redhat.com>

/kind bug
@kubernetes/sig-cli-maintainers
/assign @smarterclayton @deads2k
Builds on #63318

```release-note
NONE
```

pkg/kubectl/genericclioptions/resource/builder.go

@@ -49,9 +49,7 @@ type Builder struct {
 	categoryExpander restmapper.CategoryExpander
 
 	// mapper is set explicitly by resource builders
-	mapper       *mapper
-	internal     *mapper
-	unstructured *mapper
+	mapper *mapper
 
 	// clientConfigFn is a function to produce a client, *if* you need one
 	clientConfigFn ClientConfigFunc
@@ -829,7 +827,6 @@ func (b *Builder) visitBySelector() *Result {
 			result.err = err
 			return result
 		}
-		client = NewClientWithOptions(client, b.requestTransforms...)
 		selectorNamespace := b.namespace
 		if mapping.Scope.Name() != meta.RESTScopeNameNamespace {
 			selectorNamespace = ""
@@ -846,15 +843,25 @@ func (b *Builder) visitBySelector() *Result {
 }
 
 func (b *Builder) getClient(gv schema.GroupVersion) (RESTClient, error) {
-	if b.fakeClientFn != nil {
-		return b.fakeClientFn(gv)
+	var (
+		client RESTClient
+		err    error
+	)
+
+	switch {
+	case b.fakeClientFn != nil:
+		client, err = b.fakeClientFn(gv)
+	case b.negotiatedSerializer != nil:
+		client, err = b.clientConfigFn.clientForGroupVersion(gv, b.negotiatedSerializer)
+	default:
+		client, err = b.clientConfigFn.unstructuredClientForGroupVersion(gv)
 	}
 
-	if b.negotiatedSerializer != nil {
-		return b.clientConfigFn.clientForGroupVersion(gv, b.negotiatedSerializer)
+	if err != nil {
+		return nil, err
 	}
 
-	return b.clientConfigFn.unstructuredClientForGroupVersion(gv)
+	return NewClientWithOptions(client, b.requestTransforms...), nil
 }
 
 func (b *Builder) visitByResource() *Result {
@@ -891,7 +898,6 @@ func (b *Builder) visitByResource() *Result {
 			result.err = err
 			return result
 		}
-		client = NewClientWithOptions(client, b.requestTransforms...)
 		clients[s] = client
 	}
 

pkg/kubectl/genericclioptions/resource/builder_test.go

@@ -655,22 +655,50 @@ func TestMultipleResourceByTheSameName(t *testing.T) {
 }
 
 func TestRequestModifier(t *testing.T) {
-	var got *rest.Request
-	b := newDefaultBuilderWith(fakeClientWith("test", t, nil)).
-		NamespaceParam("foo").
-		TransformRequests(func(req *rest.Request) {
-			got = req
-		}).
-		ResourceNames("", "services/baz").
-		RequireObject(false)
-
-	i, err := b.Do().Infos()
-	if err != nil {
-		t.Fatal(err)
-	}
-	req := i[0].Client.Get()
-	if got != req {
-		t.Fatalf("request was not received by modifier: %#v", req)
+	for _, tc := range []struct {
+		name string
+		f    func(t *testing.T, got **rest.Request) *Builder
+	}{
+		{
+			name: "simple",
+			f: func(t *testing.T, got **rest.Request) *Builder {
+				return newDefaultBuilderWith(fakeClientWith(t.Name(), t, nil)).
+					NamespaceParam("foo").
+					TransformRequests(func(req *rest.Request) {
+						*got = req
+					}).
+					ResourceNames("", "services/baz").
+					RequireObject(false)
+			},
+		},
+		{
+			name: "flatten",
+			f: func(t *testing.T, got **rest.Request) *Builder {
+				pods, _ := testData()
+				return newDefaultBuilderWith(fakeClientWith(t.Name(), t, map[string]string{
+					"/namespaces/foo/pods": runtime.EncodeOrDie(corev1Codec, pods),
+				})).
+					NamespaceParam("foo").
+					TransformRequests(func(req *rest.Request) {
+						*got = req
+					}).
+					ResourceTypeOrNameArgs(true, "pods").
+					Flatten()
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			var got *rest.Request
+			b := tc.f(t, &got)
+			i, err := b.Do().Infos()
+			if err != nil {
+				t.Fatal(err)
+			}
+			req := i[0].Client.Get()
+			if got != req {
+				t.Fatalf("request was not received by modifier: %#v", req)
+			}
+		})
 	}
 }
 

pkg/kubectl/genericclioptions/resource/interfaces.go

@@ -47,6 +47,9 @@ type RequestTransform func(*rest.Request)
 // NewClientWithOptions wraps the provided RESTClient and invokes each transform on each
 // newly created request.
 func NewClientWithOptions(c RESTClient, transforms ...RequestTransform) RESTClient {
+	if len(transforms) == 0 {
+		return c
+	}
 	return &clientOptions{c: c, transforms: transforms}
 }