Merge pull request #63613 from deads2k/cli-52-builderclient

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

 construct resource.Builder from kubeconfig flags

updates the resource.Builder to be constructed from the kubeconfig flags struct/interface that we now have.

@kubernetes/sig-cli-maintainers 
@juanvallejo @soltysh 

```release-note
NONE
```

pkg/kubectl/cmd/cmd.go

@@ -276,11 +276,12 @@ func NewKubectlCommand(in io.Reader, out, err io.Writer) *cobra.Command {
 
 	kubeConfigFlags := cmdutil.NewConfigFlags()
 	kubeConfigFlags.AddFlags(cmds.PersistentFlags())
+	matchVersionKubeConfigFlags := cmdutil.NewMatchVersionFlags(kubeConfigFlags)
+	matchVersionKubeConfigFlags.AddFlags(cmds.PersistentFlags())
 
 	cmds.PersistentFlags().AddGoFlagSet(flag.CommandLine)
 
-	f := cmdutil.NewFactory(kubeConfigFlags)
-	f.BindFlags(cmds.PersistentFlags())
+	f := cmdutil.NewFactory(matchVersionKubeConfigFlags)
 
 	// Sending in 'nil' for the getLanguageFn() results in using
 	// the LANG environment variable.

pkg/kubectl/cmd/util/BUILD

@@ -11,6 +11,7 @@ go_library(
         "factory_client_access.go",
         "factory_object_mapping.go",
         "helpers.go",
+        "kubectl_match_version.go",
         "printing.go",
     ],
     importpath = "k8s.io/kubernetes/pkg/kubectl/cmd/util",

pkg/kubectl/cmd/util/config_flags.go

@@ -53,6 +53,7 @@ const (
 	flagUsername         = "username"
 	flagPassword         = "password"
 	flagTimeout          = "request-timeout"
+	flagHTTPCacheDir     = "cache-dir"
 )
 
 // TODO(juanvallejo): move to pkg/kubectl/genericclioptions once
@@ -225,7 +226,7 @@ func (f *ConfigFlags) AddFlags(flags *pflag.FlagSet) {
 		flags.StringVar(f.KubeConfig, "kubeconfig", *f.KubeConfig, "Path to the kubeconfig file to use for CLI requests.")
 	}
 	if f.CacheDir != nil {
-		flags.StringVar(f.CacheDir, FlagHTTPCacheDir, *f.CacheDir, "Default HTTP cache directory")
+		flags.StringVar(f.CacheDir, flagHTTPCacheDir, *f.CacheDir, "Default HTTP cache directory")
 	}
 
 	// add config options

pkg/kubectl/cmd/util/factory.go

@@ -50,14 +50,6 @@ import (
 	"k8s.io/kubernetes/pkg/printers"
 )
 
-const (
-	FlagMatchBinaryVersion = "match-server-version"
-)
-
-var (
-	FlagHTTPCacheDir = "cache-dir"
-)
-
 // Factory provides abstractions that allow the Kubectl command to be extended across multiple types
 // of resources and different API sets.
 // The rings are here for a reason. In order for composers to be able to provide alternative factory implementations
@@ -109,6 +101,10 @@ type ClientAccessFactory interface {
 	// just directions to the server. People use this to build RESTMappers on top of
 	BareClientConfig() (*restclient.Config, error)
 
+	// NewBuilder returns an object that assists in loading objects from both disk and the server
+	// and which implements the common patterns for CLI interactions with generic resources.
+	NewBuilder() *resource.Builder
+
 	// UpdatePodSpecForObject will call the provided function on the pod spec this object supports,
 	// return false if no pod spec is supported, or return an error.
 	UpdatePodSpecForObject(obj runtime.Object, fn func(*v1.PodSpec) error) (bool, error)
@@ -128,9 +124,6 @@ type ClientAccessFactory interface {
 	// using the factory.
 	Command(cmd *cobra.Command, showSecrets bool) string
 
-	// BindFlags adds any flags that are common to all kubectl sub commands.
-	BindFlags(flags *pflag.FlagSet)
-
 	// SuggestedPodTemplateResources returns a list of resource types that declare a pod template
 	SuggestedPodTemplateResources() []schema.GroupResource
 
@@ -204,9 +197,6 @@ type ObjectMappingFactory interface {
 // BuilderFactory holds the third level of factory methods. These functions depend upon ObjectMappingFactory and ClientAccessFactory methods.
 // Generally they depend upon client mapper functions
 type BuilderFactory interface {
-	// NewBuilder returns an object that assists in loading objects from both disk and the server
-	// and which implements the common patterns for CLI interactions with generic resources.
-	NewBuilder() *resource.Builder
 	// PluginLoader provides the implementation to be used to load cli plugins.
 	PluginLoader() plugins.PluginLoader
 	// PluginRunner provides the implementation to be used to run cli plugins.

pkg/kubectl/cmd/util/factory_builder.go

@@ -26,7 +26,6 @@ import (
 	scaleclient "k8s.io/client-go/scale"
 	"k8s.io/kubernetes/pkg/kubectl"
 	"k8s.io/kubernetes/pkg/kubectl/plugins"
-	"k8s.io/kubernetes/pkg/kubectl/resource"
 )
 
 type ring2Factory struct {
@@ -43,20 +42,6 @@ func NewBuilderFactory(clientAccessFactory ClientAccessFactory, objectMappingFac
 	return f
 }
 
-// NewBuilder returns a new resource builder for structured api objects.
-func (f *ring2Factory) NewBuilder() *resource.Builder {
-	mapper, mapperErr := f.clientAccessFactory.RESTMapper()
-	categoryExpander, categoryExpanderError := f.objectMappingFactory.CategoryExpander()
-
-	return resource.NewBuilder(
-		f.clientAccessFactory.ClientConfig,
-		mapper,
-		categoryExpander,
-	).
-		AddError(mapperErr).
-		AddError(categoryExpanderError)
-}
-
 // PluginLoader loads plugins from a path set by the KUBECTL_PLUGINS_PATH env var.
 // If this env var is not set, it defaults to
 //   "~/.kube/plugins", plus

pkg/kubectl/cmd/util/factory_client_access.go

@@ -26,7 +26,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-	"sync"
 
 	"k8s.io/api/core/v1"
 
@@ -58,7 +57,6 @@ import (
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/kubectl"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
-	"k8s.io/kubernetes/pkg/version"
 )
 
 type RESTClientGetter interface {
@@ -70,10 +68,6 @@ type RESTClientGetter interface {
 
 type ring0Factory struct {
 	clientGetter RESTClientGetter
-
-	requireMatchedServerVersion bool
-	checkServerVersion          sync.Once
-	matchesServerVersionErr     error
 }
 
 func NewClientAccessFactory(clientGetter RESTClientGetter) ClientAccessFactory {
@@ -88,6 +82,18 @@ func NewClientAccessFactory(clientGetter RESTClientGetter) ClientAccessFactory {
 	return f
 }
 
+func (f *ring0Factory) ClientConfig() (*restclient.Config, error) {
+	return f.clientGetter.ToRESTConfig()
+}
+
+func (f *ring0Factory) RESTMapper() (meta.RESTMapper, error) {
+	return f.clientGetter.ToRESTMapper()
+}
+
+func (f *ring0Factory) BareClientConfig() (*restclient.Config, error) {
+	return f.clientGetter.ToRESTConfig()
+}
+
 func (f *ring0Factory) DiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
 	return f.clientGetter.ToDiscoveryClient()
 }
@@ -115,40 +121,10 @@ func (f *ring0Factory) DynamicClient() (dynamic.DynamicInterface, error) {
 	}
 	return dynamic.NewForConfig(clientConfig)
 }
-func (f *ring0Factory) checkMatchingServerVersion() error {
-	f.checkServerVersion.Do(func() {
-		if !f.requireMatchedServerVersion {
-			return
-		}
-		discoveryClient, err := f.DiscoveryClient()
-		if err != nil {
-			f.matchesServerVersionErr = err
-			return
-		}
-		f.matchesServerVersionErr = discovery.MatchesServerVersion(version.Get(), discoveryClient)
-	})
 
-	return f.matchesServerVersionErr
-}
-
-func (f *ring0Factory) ClientConfig() (*restclient.Config, error) {
-	if err := f.checkMatchingServerVersion(); err != nil {
-		return nil, err
-	}
-	clientConfig, err := f.clientGetter.ToRESTConfig()
-	if err != nil {
-		return nil, err
-	}
-	setKubernetesDefaults(clientConfig)
-	return clientConfig, nil
-}
-
-func (f *ring0Factory) RESTMapper() (meta.RESTMapper, error) {
-	return f.clientGetter.ToRESTMapper()
-}
-
-func (f *ring0Factory) BareClientConfig() (*restclient.Config, error) {
-	return f.clientGetter.ToRESTConfig()
+// NewBuilder returns a new resource builder for structured api objects.
+func (f *ring0Factory) NewBuilder() *resource.Builder {
+	return resource.NewBuilder(f.clientGetter)
 }
 
 func (f *ring0Factory) RESTClient() (*restclient.RESTClient, error) {
@@ -156,6 +132,7 @@ func (f *ring0Factory) RESTClient() (*restclient.RESTClient, error) {
 	if err != nil {
 		return nil, err
 	}
+	setKubernetesDefaults(clientConfig)
 	return restclient.RESTClientFor(clientConfig)
 }
 
@@ -325,14 +302,6 @@ func (f *ring0Factory) Command(cmd *cobra.Command, showSecrets bool) string {
 	return base + args + flags
 }
 
-func (f *ring0Factory) BindFlags(flags *pflag.FlagSet) {
-	// Globally persistent flags across all subcommands.
-	// TODO Change flag names to consts to allow safer lookup from subcommands.
-	// TODO Add a verbose flag that turns on glog logging. Probably need a way
-	// to do that automatically for every subcommand.
-	flags.BoolVar(&f.requireMatchedServerVersion, FlagMatchBinaryVersion, false, "Require server version to match client version")
-}
-
 func (f *ring0Factory) SuggestedPodTemplateResources() []schema.GroupResource {
 	return []schema.GroupResource{
 		{Resource: "replicationcontroller"},
@@ -635,19 +604,3 @@ func InternalVersionJSONEncoder() runtime.Encoder {
 	encoder := legacyscheme.Codecs.LegacyCodec(legacyscheme.Scheme.PrioritizedVersionsAllGroups()...)
 	return unstructured.JSONFallbackEncoder{Encoder: encoder}
 }
-
-// setKubernetesDefaults sets default values on the provided client config for accessing the
-// Kubernetes API or returns an error if any of the defaults are impossible or invalid.
-// TODO this isn't what we want.  Each clientset should be setting defaults as it sees fit.
-func setKubernetesDefaults(config *restclient.Config) error {
-	// TODO remove this hack.  This is allowing the GetOptions to be serialized.
-	config.GroupVersion = &schema.GroupVersion{Group: "", Version: "v1"}
-
-	if config.APIPath == "" {
-		config.APIPath = "/api"
-	}
-	if config.NegotiatedSerializer == nil {
-		config.NegotiatedSerializer = legacyscheme.Codecs
-	}
-	return restclient.SetKubernetesDefaults(config)
-}

pkg/kubectl/cmd/util/factory_object_mapping.go

@@ -81,7 +81,7 @@ func (f *ring1Factory) CategoryExpander() (restmapper.CategoryExpander, error) {
 		return nil, err
 	}
 
-	return restmapper.NewDiscoveryCategoryExpander(discoveryClient)
+	return restmapper.NewDiscoveryCategoryExpander(discoveryClient), nil
 }
 
 func (f *ring1Factory) ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {

pkg/kubectl/cmd/util/kubectl_match_version.go

@@ -0,0 +1,125 @@
+/*
+Copyright 2018 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 util
+
+import (
+	"sync"
+
+	"github.com/spf13/pflag"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/clientcmd"
+
+	"k8s.io/kubernetes/pkg/api/legacyscheme"
+	"k8s.io/kubernetes/pkg/version"
+)
+
+const (
+	flagMatchBinaryVersion = "match-server-version"
+)
+
+// MatchVersionFlags is for setting the "match server version" function.
+type MatchVersionFlags struct {
+	Delegate RESTClientGetter
+
+	RequireMatchedServerVersion bool
+	checkServerVersion          sync.Once
+	matchesServerVersionErr     error
+}
+
+var _ RESTClientGetter = &MatchVersionFlags{}
+
+func (f *MatchVersionFlags) checkMatchingServerVersion() error {
+	f.checkServerVersion.Do(func() {
+		if !f.RequireMatchedServerVersion {
+			return
+		}
+		discoveryClient, err := f.Delegate.ToDiscoveryClient()
+		if err != nil {
+			f.matchesServerVersionErr = err
+			return
+		}
+		f.matchesServerVersionErr = discovery.MatchesServerVersion(version.Get(), discoveryClient)
+	})
+
+	return f.matchesServerVersionErr
+}
+
+// ToRESTConfig implements RESTClientGetter.
+// Returns a REST client configuration based on a provided path
+// to a .kubeconfig file, loading rules, and config flag overrides.
+// Expects the AddFlags method to have been called.
+func (f *MatchVersionFlags) ToRESTConfig() (*rest.Config, error) {
+	if err := f.checkMatchingServerVersion(); err != nil {
+		return nil, err
+	}
+	clientConfig, err := f.Delegate.ToRESTConfig()
+	if err != nil {
+		return nil, err
+	}
+	// TODO we should not have to do this.  It smacks of something going wrong.
+	setKubernetesDefaults(clientConfig)
+	return clientConfig, nil
+}
+
+func (f *MatchVersionFlags) ToRawKubeConfigLoader() clientcmd.ClientConfig {
+	return f.Delegate.ToRawKubeConfigLoader()
+}
+
+func (f *MatchVersionFlags) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
+	if err := f.checkMatchingServerVersion(); err != nil {
+		return nil, err
+	}
+	return f.Delegate.ToDiscoveryClient()
+}
+
+// RESTMapper returns a mapper.
+func (f *MatchVersionFlags) ToRESTMapper() (meta.RESTMapper, error) {
+	if err := f.checkMatchingServerVersion(); err != nil {
+		return nil, err
+	}
+	return f.Delegate.ToRESTMapper()
+}
+
+func (f *MatchVersionFlags) AddFlags(flags *pflag.FlagSet) {
+	flags.BoolVar(&f.RequireMatchedServerVersion, flagMatchBinaryVersion, f.RequireMatchedServerVersion, "Require server version to match client version")
+}
+
+func NewMatchVersionFlags(delegate RESTClientGetter) *MatchVersionFlags {
+	return &MatchVersionFlags{
+		Delegate: delegate,
+	}
+}
+
+// setKubernetesDefaults sets default values on the provided client config for accessing the
+// Kubernetes API or returns an error if any of the defaults are impossible or invalid.
+// TODO this isn't what we want.  Each clientset should be setting defaults as it sees fit.
+func setKubernetesDefaults(config *rest.Config) error {
+	// TODO remove this hack.  This is allowing the GetOptions to be serialized.
+	config.GroupVersion = &schema.GroupVersion{Group: "", Version: "v1"}
+
+	if config.APIPath == "" {
+		config.APIPath = "/api"
+	}
+	if config.NegotiatedSerializer == nil {
+		config.NegotiatedSerializer = legacyscheme.Codecs
+	}
+	return rest.SetKubernetesDefaults(config)
+}

pkg/kubectl/resource/BUILD

@@ -42,6 +42,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/yaml:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
+        "//vendor/k8s.io/client-go/discovery:go_default_library",
         "//vendor/k8s.io/client-go/dynamic:go_default_library",
         "//vendor/k8s.io/client-go/rest:go_default_library",
         "//vendor/k8s.io/client-go/restmapper:go_default_library",

pkg/kubectl/resource/builder.go

@@ -144,7 +144,7 @@ type resourceTuple struct {
 type FakeClientFunc func(version schema.GroupVersion) (RESTClient, error)
 
 func NewFakeBuilder(fakeClientFn FakeClientFunc, restMapper meta.RESTMapper, categoryExpander restmapper.CategoryExpander) *Builder {
-	ret := NewBuilder(nil, restMapper, categoryExpander)
+	ret := newBuilder(nil, restMapper, categoryExpander)
 	ret.fakeClientFn = fakeClientFn
 	return ret
 }
@@ -153,7 +153,7 @@ 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 meta.RESTMapper, categoryExpander restmapper.CategoryExpander) *Builder {
 	return &Builder{
 		clientConfigFn:   clientConfigFn,
 		restMapper:       restMapper,
@@ -162,6 +162,21 @@ func NewBuilder(clientConfigFn ClientConfigFunc, restMapper meta.RESTMapper, cat
 	}
 }
 
+func NewBuilder(restClientGetter RESTClientGetter) *Builder {
+	restMapper, mapperErr := restClientGetter.ToRESTMapper()
+	discoveryClient, discoveryErr := restClientGetter.ToDiscoveryClient()
+	var categoryExpander restmapper.CategoryExpander
+	if discoveryErr == nil {
+		categoryExpander = restmapper.NewDiscoveryCategoryExpander(discoveryClient)
+	}
+
+	return newBuilder(
+		restClientGetter.ToRESTConfig,
+		restMapper,
+		categoryExpander,
+	).AddError(mapperErr).AddError(discoveryErr)
+}
+
 func (b *Builder) Schema(schema validation.Schema) *Builder {
 	b.schema = schema
 	return b

pkg/kubectl/resource/interfaces.go

@@ -17,10 +17,18 @@ limitations under the License.
 package resource
 
 import (
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
 )
 
+type RESTClientGetter interface {
+	ToRESTConfig() (*rest.Config, error)
+	ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error)
+	ToRESTMapper() (meta.RESTMapper, error)
+}
+
 type ClientConfigFunc func() (*rest.Config, error)
 
 // RESTClient is a client helper for dealing with RESTful resources

staging/src/k8s.io/client-go/restmapper/category_expansion.go

@@ -48,11 +48,11 @@ type discoveryCategoryExpander struct {
 // NewDiscoveryCategoryExpander returns a category expander that makes use of the "categories" fields from
 // the API, found through the discovery client. In case of any error or no category found (which likely
 // means we're at a cluster prior to categories support, fallback to the expander provided.
-func NewDiscoveryCategoryExpander(client discovery.DiscoveryInterface) (CategoryExpander, error) {
+func NewDiscoveryCategoryExpander(client discovery.DiscoveryInterface) CategoryExpander {
 	if client == nil {
 		panic("Please provide discovery client to shortcut expander")
 	}
-	return discoveryCategoryExpander{discoveryClient: client}, nil
+	return discoveryCategoryExpander{discoveryClient: client}
 }
 
 // Expand fulfills CategoryExpander

staging/src/k8s.io/client-go/restmapper/category_expansion_test.go

@@ -135,10 +135,7 @@ func TestDiscoveryCategoryExpander(t *testing.T) {
 		dc.serverResourcesHandler = func() ([]*metav1.APIResourceList, error) {
 			return test.serverResponse, nil
 		}
-		expander, err := NewDiscoveryCategoryExpander(dc)
-		if err != nil {
-			t.Fatalf("unexpected error %v", err)
-		}
+		expander := NewDiscoveryCategoryExpander(dc)
 		expanded, _ := expander.Expand(test.category)
 		if !reflect.DeepEqual(expanded, test.expected) {
 			t.Errorf("expected %v, got %v", test.expected, expanded)