diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index 4a4a87a7e6..2740b3544a 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/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, - resource.FakeCategoryExpander, - ).AddError(err) + f.ToRESTMapper, + func() (restmapper.CategoryExpander, error) { + return resource.FakeCategoryExpander, nil + }, + ) } func (f *TestFactory) KubernetesClientSet() (*kubernetes.Clientset, error) { diff --git a/pkg/kubectl/genericclioptions/resource/BUILD b/pkg/kubectl/genericclioptions/resource/BUILD index 12316b63e6..67dd53b748 100644 --- a/pkg/kubectl/genericclioptions/resource/BUILD +++ b/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", diff --git a/pkg/kubectl/genericclioptions/resource/builder.go b/pkg/kubectl/genericclioptions/resource/builder.go index b67a6976b4..22680c517c 100644 --- a/pkg/kubectl/genericclioptions/resource/builder.go +++ b/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, - restMapper: restMapper, - categoryExpander: categoryExpander, - requireObject: true, - labelSelector: nil, - fieldSelector: nil, + clientConfigFn: clientConfigFn, + restMapperFn: restMapper, + categoryExpanderFn: categoryExpander, + requireObject: true, } } func NewBuilder(restClientGetter RESTClientGetter) *Builder { - restMapper, mapperErr := restClientGetter.ToRESTMapper() - discoveryClient, discoveryErr := restClientGetter.ToDiscoveryClient() - var categoryExpander restmapper.CategoryExpander - if discoveryErr == nil { - categoryExpander = restmapper.NewDiscoveryCategoryExpander(discoveryClient) + categoryExpanderFn := func() (restmapper.CategoryExpander, error) { + discoveryClient, err := restClientGetter.ToDiscoveryClient() + if err != nil { + return nil, err + } + return restmapper.NewDiscoveryCategoryExpander(discoveryClient), err } return newBuilder( restClientGetter.ToRESTConfig, - restMapper, - categoryExpander, - ).AddError(mapperErr).AddError(discoveryErr) + (&cachingRESTMapperFunc{delegate: restClientGetter.ToRESTMapper}).ToRESTMapper, + (&cachingCategoryExpanderFunc{delegate: categoryExpanderFn}).ToCategoryExpander, + ) } 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, - restMapper: b.restMapper, - clientFn: b.getClient, - decoder: unstructured.UnstructuredJSONScheme, + localFn: b.isLocal, + restMapperFn: b.restMapperFn, + clientFn: b.getClient, + 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, - restMapper: b.restMapper, - clientFn: b.getClient, - decoder: codecFactory.UniversalDecoder(decodingVersions...), + localFn: b.isLocal, + restMapperFn: b.restMapperFn, + clientFn: b.getClient, + 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 +} diff --git a/pkg/kubectl/genericclioptions/resource/builder_test.go b/pkg/kubectl/genericclioptions/resource/builder_test.go index c6963e115d..07f62198ab 100644 --- a/pkg/kubectl/genericclioptions/resource/builder_test.go +++ b/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()...) } diff --git a/pkg/kubectl/genericclioptions/resource/interfaces.go b/pkg/kubectl/genericclioptions/resource/interfaces.go index 508d4d6b5e..29d7b34ab6 100644 --- a/pkg/kubectl/genericclioptions/resource/interfaces.go +++ b/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. diff --git a/pkg/kubectl/genericclioptions/resource/mapper.go b/pkg/kubectl/genericclioptions/resource/mapper.go index 0453e54237..962f37711f 100644 --- a/pkg/kubectl/genericclioptions/resource/mapper.go +++ b/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 - clientFn func(version schema.GroupVersion) (RESTClient, error) - decoder runtime.Decoder + restMapperFn RESTMapperFunc + clientFn func(version schema.GroupVersion) (RESTClient, error) + 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) }