From 631124cde4989ec7233d9b885a9d140a0ed0bb89 Mon Sep 17 00:00:00 2001
From: Monis Khan <mkhan@redhat.com>
Date: Mon, 7 May 2018 14:14:23 -0400
Subject: [PATCH] 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>
---
 .../genericclioptions/resource/builder.go     | 26 ++++----
 .../resource/builder_test.go                  | 60 ++++++++++++++-----
 .../genericclioptions/resource/interfaces.go  |  3 +
 3 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/pkg/kubectl/genericclioptions/resource/builder.go b/pkg/kubectl/genericclioptions/resource/builder.go
index 9f4e02d02b2..09e21827f7e 100644
--- a/pkg/kubectl/genericclioptions/resource/builder.go
+++ b/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
 	}
 
diff --git a/pkg/kubectl/genericclioptions/resource/builder_test.go b/pkg/kubectl/genericclioptions/resource/builder_test.go
index e6308a56687..ecf12529a19 100644
--- a/pkg/kubectl/genericclioptions/resource/builder_test.go
+++ b/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)
+			}
+		})
 	}
 }
 
diff --git a/pkg/kubectl/genericclioptions/resource/interfaces.go b/pkg/kubectl/genericclioptions/resource/interfaces.go
index 6179481a5d8..508d4d6b5e4 100644
--- a/pkg/kubectl/genericclioptions/resource/interfaces.go
+++ b/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}
 }