make builder tolerant of restmapper failures when it doesn't need the answer

pkg/kubectl/cmd/testing/fake.go

@@ -322,8 +322,6 @@ func (f *TestFactory) OpenAPISchema() (openapi.Resources, error) {
 }
 
 func (f *TestFactory) NewBuilder() *resource.Builder {
-	mapper, err := f.ToRESTMapper()
-
 	return resource.NewFakeBuilder(
 		func(version schema.GroupVersion) (resource.RESTClient, error) {
 			if f.UnstructuredClientForMappingFunc != nil {
@@ -334,9 +332,11 @@ func (f *TestFactory) NewBuilder() *resource.Builder {
 			}
 			return f.Client, nil
 		},
-		mapper,
+		f.ToRESTMapper,
-		resource.FakeCategoryExpander,
+		func() (restmapper.CategoryExpander, error) {
-	).AddError(err)
+			return resource.FakeCategoryExpander, nil
+		},
+	)
 }
 
 func (f *TestFactory) KubernetesClientSet() (*kubernetes.Clientset, error) {

pkg/kubectl/genericclioptions/resource/BUILD

@@ -73,6 +73,7 @@ go_test(
         "//staging/src/k8s.io/client-go/rest:go_default_library",
         "//staging/src/k8s.io/client-go/rest/fake:go_default_library",
         "//staging/src/k8s.io/client-go/rest/watch:go_default_library",
+        "//staging/src/k8s.io/client-go/restmapper:go_default_library",
         "//staging/src/k8s.io/client-go/util/testing:go_default_library",
         "//vendor/github.com/davecgh/go-spew/spew:go_default_library",
         "//vendor/github.com/ghodss/yaml:go_default_library",

pkg/kubectl/genericclioptions/resource/builder.go

@@ -23,6 +23,7 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -46,7 +47,7 @@ const defaultHttpGetAttempts int = 3
 // from the command line and converting them to a list of resources to iterate
 // over using the Visitor interface.
 type Builder struct {
-	categoryExpander restmapper.CategoryExpander
+	categoryExpanderFn CategoryExpanderFunc
 
 	// mapper is set explicitly by resource builders
 	mapper *mapper
@@ -54,7 +55,7 @@ type Builder struct {
 	// clientConfigFn is a function to produce a client, *if* you need one
 	clientConfigFn ClientConfigFunc
 
-	restMapper meta.RESTMapper
+	restMapperFn RESTMapperFunc
 
 	// objectTyper is statically determinant per-command invocation based on your internal or unstructured choice
 	// it does not ever need to rely upon discovery.
@@ -140,7 +141,7 @@ type resourceTuple struct {
 
 type FakeClientFunc func(version schema.GroupVersion) (RESTClient, error)
 
-func NewFakeBuilder(fakeClientFn FakeClientFunc, restMapper meta.RESTMapper, categoryExpander restmapper.CategoryExpander) *Builder {
+func NewFakeBuilder(fakeClientFn FakeClientFunc, restMapper RESTMapperFunc, categoryExpander CategoryExpanderFunc) *Builder {
 	ret := newBuilder(nil, restMapper, categoryExpander)
 	ret.fakeClientFn = fakeClientFn
 	return ret
@@ -150,30 +151,29 @@ func NewFakeBuilder(fakeClientFn FakeClientFunc, restMapper meta.RESTMapper, cat
 // internal or unstructured must be specified.
 // TODO: Add versioned client (although versioned is still lossy)
 // TODO remove internal and unstructured mapper and instead have them set the negotiated serializer for use in the client
-func newBuilder(clientConfigFn ClientConfigFunc, restMapper meta.RESTMapper, categoryExpander restmapper.CategoryExpander) *Builder {
+func newBuilder(clientConfigFn ClientConfigFunc, restMapper RESTMapperFunc, categoryExpander CategoryExpanderFunc) *Builder {
 	return &Builder{
-		clientConfigFn:   clientConfigFn,
+		clientConfigFn:     clientConfigFn,
-		restMapper:       restMapper,
+		restMapperFn:       restMapper,
-		categoryExpander: categoryExpander,
+		categoryExpanderFn: categoryExpander,
-		requireObject:    true,
+		requireObject:      true,
-		labelSelector:    nil,
-		fieldSelector:    nil,
 	}
 }
 
 func NewBuilder(restClientGetter RESTClientGetter) *Builder {
-	restMapper, mapperErr := restClientGetter.ToRESTMapper()
+	categoryExpanderFn := func() (restmapper.CategoryExpander, error) {
-	discoveryClient, discoveryErr := restClientGetter.ToDiscoveryClient()
+		discoveryClient, err := restClientGetter.ToDiscoveryClient()
-	var categoryExpander restmapper.CategoryExpander
+		if err != nil {
-	if discoveryErr == nil {
+			return nil, err
-		categoryExpander = restmapper.NewDiscoveryCategoryExpander(discoveryClient)
+		}
+		return restmapper.NewDiscoveryCategoryExpander(discoveryClient), err
 	}
 
 	return newBuilder(
 		restClientGetter.ToRESTConfig,
-		restMapper,
+		(&cachingRESTMapperFunc{delegate: restClientGetter.ToRESTMapper}).ToRESTMapper,
-		categoryExpander,
+		(&cachingCategoryExpanderFunc{delegate: categoryExpanderFn}).ToCategoryExpander,
-	).AddError(mapperErr).AddError(discoveryErr)
+	)
 }
 
 func (b *Builder) Schema(schema ContentValidator) *Builder {
@@ -236,10 +236,10 @@ func (b *Builder) Unstructured() *Builder {
 	}
 	b.objectTyper = unstructuredscheme.NewUnstructuredObjectTyper()
 	b.mapper = &mapper{
-		localFn:    b.isLocal,
+		localFn:      b.isLocal,
-		restMapper: b.restMapper,
+		restMapperFn: b.restMapperFn,
-		clientFn:   b.getClient,
+		clientFn:     b.getClient,
-		decoder:    unstructured.UnstructuredJSONScheme,
+		decoder:      unstructured.UnstructuredJSONScheme,
 	}
 
 	return b
@@ -263,10 +263,10 @@ func (b *Builder) WithScheme(scheme *runtime.Scheme, decodingVersions ...schema.
 	b.negotiatedSerializer = negotiatedSerializer
 
 	b.mapper = &mapper{
-		localFn:    b.isLocal,
+		localFn:      b.isLocal,
-		restMapper: b.restMapper,
+		restMapperFn: b.restMapperFn,
-		clientFn:   b.getClient,
+		clientFn:     b.getClient,
-		decoder:    codecFactory.UniversalDecoder(decodingVersions...),
+		decoder:      codecFactory.UniversalDecoder(decodingVersions...),
 	}
 
 	return b
@@ -557,10 +557,16 @@ func (b *Builder) ResourceTypeOrNameArgs(allowEmptySelector bool, args ...string
 func (b *Builder) ReplaceAliases(input string) string {
 	replaced := []string{}
 	for _, arg := range strings.Split(input, ",") {
-		if b.categoryExpander == nil {
+		if b.categoryExpanderFn == nil {
 			continue
 		}
-		if resources, ok := b.categoryExpander.Expand(arg); ok {
+		categoryExpander, err := b.categoryExpanderFn()
+		if err != nil {
+			b.AddError(err)
+			continue
+		}
+
+		if resources, ok := categoryExpander.Expand(arg); ok {
 			asStrings := []string{}
 			for _, resource := range resources {
 				if len(resource.Group) == 0 {
@@ -677,14 +683,19 @@ func (b *Builder) SingleResourceType() *Builder {
 func (b *Builder) mappingFor(resourceOrKindArg string) (*meta.RESTMapping, error) {
 	fullySpecifiedGVR, groupResource := schema.ParseResourceArg(resourceOrKindArg)
 	gvk := schema.GroupVersionKind{}
+	restMapper, err := b.restMapperFn()
+	if err != nil {
+		return nil, err
+	}
+
 	if fullySpecifiedGVR != nil {
-		gvk, _ = b.mapper.restMapper.KindFor(*fullySpecifiedGVR)
+		gvk, _ = restMapper.KindFor(*fullySpecifiedGVR)
 	}
 	if gvk.Empty() {
-		gvk, _ = b.mapper.restMapper.KindFor(groupResource.WithVersion(""))
+		gvk, _ = restMapper.KindFor(groupResource.WithVersion(""))
 	}
 	if !gvk.Empty() {
-		return b.mapper.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		return restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 	}
 
 	fullySpecifiedGVK, groupKind := schema.ParseKindArg(resourceOrKindArg)
@@ -694,12 +705,12 @@ func (b *Builder) mappingFor(resourceOrKindArg string) (*meta.RESTMapping, error
 	}
 
 	if !fullySpecifiedGVK.Empty() {
-		if mapping, err := b.mapper.restMapper.RESTMapping(fullySpecifiedGVK.GroupKind(), fullySpecifiedGVK.Version); err == nil {
+		if mapping, err := restMapper.RESTMapping(fullySpecifiedGVK.GroupKind(), fullySpecifiedGVK.Version); err == nil {
 			return mapping, nil
 		}
 	}
 
-	mapping, err := b.mapper.restMapper.RESTMapping(groupKind, gvk.Version)
+	mapping, err := restMapper.RESTMapping(groupKind, gvk.Version)
 	if err != nil {
 		// if we error out here, it is because we could not match a resource or a kind
 		// for the given argument. To maintain consistency with previous behavior,
@@ -1045,7 +1056,7 @@ func (b *Builder) visitByPaths() *Result {
 	if b.labelSelector != nil {
 		selector, err := labels.Parse(*b.labelSelector)
 		if err != nil {
-			return result.withError(fmt.Errorf("the provided selector %q is not valid: %v", *b.labelSelector, err))
+			return result.withError(fmt.Errorf("the provided selector %q is not valid: %v", b.labelSelector, err))
 		}
 		visitors = NewFilteredVisitor(visitors, FilterByLabelSelector(selector))
 	}
@@ -1109,3 +1120,47 @@ func HasNames(args []string) (bool, error) {
 	}
 	return hasCombinedTypes || len(args) > 1, nil
 }
+
+type cachingRESTMapperFunc struct {
+	delegate RESTMapperFunc
+
+	lock   sync.Mutex
+	cached meta.RESTMapper
+}
+
+func (c *cachingRESTMapperFunc) ToRESTMapper() (meta.RESTMapper, error) {
+	c.lock.Lock()
+	defer c.lock.Unlock()
+	if c.cached != nil {
+		return c.cached, nil
+	}
+
+	ret, err := c.delegate()
+	if err != nil {
+		return nil, err
+	}
+	c.cached = ret
+	return c.cached, nil
+}
+
+type cachingCategoryExpanderFunc struct {
+	delegate CategoryExpanderFunc
+
+	lock   sync.Mutex
+	cached restmapper.CategoryExpander
+}
+
+func (c *cachingCategoryExpanderFunc) ToCategoryExpander() (restmapper.CategoryExpander, error) {
+	c.lock.Lock()
+	defer c.lock.Unlock()
+	if c.cached != nil {
+		return c.cached, nil
+	}
+
+	ret, err := c.delegate()
+	if err != nil {
+		return nil, err
+	}
+	c.cached = ret
+	return c.cached, nil
+}

pkg/kubectl/genericclioptions/resource/builder_test.go

@@ -46,6 +46,7 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/rest/fake"
 	restclientwatch "k8s.io/client-go/rest/watch"
+	"k8s.io/client-go/restmapper"
 	utiltesting "k8s.io/client-go/util/testing"
 
 	// TODO we need to remove this linkage and create our own scheme
@@ -270,7 +271,14 @@ func newDefaultBuilder() *Builder {
 }
 
 func newDefaultBuilderWith(fakeClientFn FakeClientFunc) *Builder {
-	return NewFakeBuilder(fakeClientFn, testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme), FakeCategoryExpander).
+	return NewFakeBuilder(
+		fakeClientFn,
+		func() (meta.RESTMapper, error) {
+			return testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme), nil
+		},
+		func() (restmapper.CategoryExpander, error) {
+			return FakeCategoryExpander, nil
+		}).
 		WithScheme(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...)
 }
 

pkg/kubectl/genericclioptions/resource/interfaces.go

@@ -21,6 +21,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/restmapper"
 )
 
 type RESTClientGetter interface {
@@ -30,6 +31,8 @@ type RESTClientGetter interface {
 }
 
 type ClientConfigFunc func() (*rest.Config, error)
+type RESTMapperFunc func() (meta.RESTMapper, error)
+type CategoryExpanderFunc func() (restmapper.CategoryExpander, error)
 
 // RESTClient is a client helper for dealing with RESTful resources
 // in a generic way.

pkg/kubectl/genericclioptions/resource/mapper.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"reflect"
 
-	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -31,9 +30,9 @@ type mapper struct {
 	// localFn indicates the call can't make server requests
 	localFn func() bool
 
-	restMapper meta.RESTMapper
+	restMapperFn RESTMapperFunc
-	clientFn   func(version schema.GroupVersion) (RESTClient, error)
+	clientFn     func(version schema.GroupVersion) (RESTClient, error)
-	decoder    runtime.Decoder
+	decoder      runtime.Decoder
 }
 
 // InfoForData creates an Info object for the given data. An error is returned
@@ -59,7 +58,11 @@ func (m *mapper) infoForData(data []byte, source string) (*Info, error) {
 	}
 
 	if m.localFn == nil || !m.localFn() {
-		mapping, err := m.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		restMapper, err := m.restMapperFn()
+		if err != nil {
+			return nil, err
+		}
+		mapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 		if err != nil {
 			return nil, fmt.Errorf("unable to recognize %q: %v", source, err)
 		}
@@ -101,7 +104,11 @@ func (m *mapper) infoForObject(obj runtime.Object, typer runtime.ObjectTyper, pr
 	}
 
 	if m.localFn == nil || !m.localFn() {
-		mapping, err := m.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		restMapper, err := m.restMapperFn()
+		if err != nil {
+			return nil, err
+		}
+		mapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 		if err != nil {
 			return nil, fmt.Errorf("unable to recognize %v", err)
 		}