Merge pull request #62876 from deads2k/cli-33-discovery

Automatic merge from submit-queue (batch tested with PRs 62876, 62733, 62827). 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>.

remove discovery injection from factory

We added this shim when cached discovery was a contentious thing to give ourselves flexibility.  It is no longer contentious and this removes a layer of complexity we no longer need.

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

```release-note
NONE
```

pkg/kubectl/cmd/util/BUILD

@@ -113,13 +113,10 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/version:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
-        "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library",
         "//vendor/k8s.io/client-go/discovery:go_default_library",
         "//vendor/k8s.io/client-go/rest:go_default_library",
         "//vendor/k8s.io/client-go/rest/fake:go_default_library",
         "//vendor/k8s.io/client-go/testing:go_default_library",
-        "//vendor/k8s.io/client-go/tools/clientcmd:go_default_library",
-        "//vendor/k8s.io/client-go/tools/clientcmd/api:go_default_library",
         "//vendor/k8s.io/utils/exec:go_default_library",
     ],
 )

pkg/kubectl/cmd/util/factory.go

@@ -118,8 +118,6 @@ type ClientAccessFactory interface {
 	// LabelsForObject returns the labels associated with the provided object
 	LabelsForObject(object runtime.Object) (map[string]string, error)
 
-	// Returns internal flagset
-	FlagSet() *pflag.FlagSet
 	// Command will stringify and return all environment arguments ie. a command run by a client
 	// using the factory.
 	Command(cmd *cobra.Command, showSecrets bool) string

pkg/kubectl/cmd/util/factory_client_access.go

@@ -68,9 +68,9 @@ import (
 )
 
 type ring0Factory struct {
-	flags            *pflag.FlagSet
+	flags             *pflag.FlagSet
-	clientConfig     clientcmd.ClientConfig
+	clientConfig      clientcmd.ClientConfig
-	discoveryFactory DiscoveryClientFactory
+	discoveryCacheDir string
 
 	requireMatchedServerVersion bool
 	checkServerVersion          sync.Once
@@ -85,62 +85,16 @@ func NewClientAccessFactory(optionalClientConfig clientcmd.ClientConfig) ClientA
 		clientConfig = DefaultClientConfig(flags)
 	}
 
-	return NewClientAccessFactoryFromDiscovery(flags, clientConfig, &discoveryFactory{clientConfig: clientConfig})
-}
-
-// NewClientAccessFactoryFromDiscovery allows an external caller to substitute a different discoveryFactory
-// Which allows for the client cache to be built in ring0, but still rely on a custom discovery client
-func NewClientAccessFactoryFromDiscovery(flags *pflag.FlagSet, clientConfig clientcmd.ClientConfig, discoveryFactory DiscoveryClientFactory) ClientAccessFactory {
 	flags.SetNormalizeFunc(utilflag.WarnWordSepNormalizeFunc) // Warn for "_" flags
 
 	f := &ring0Factory{
-		flags:            flags,
+		flags:        flags,
-		clientConfig:     clientConfig,
+		clientConfig: clientConfig,
-		discoveryFactory: discoveryFactory,
 	}
 
 	return f
 }
 
-type discoveryFactory struct {
-	clientConfig clientcmd.ClientConfig
-	cacheDir     string
-}
-
-func (f *discoveryFactory) DiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
-	cfg, err := f.clientConfig.ClientConfig()
-	if err != nil {
-		return nil, err
-	}
-
-	// The more groups you have, the more discovery requests you need to make.
-	// given 25 groups (our groups + a few custom resources) with one-ish version each, discovery needs to make 50 requests
-	// double it just so we don't end up here again for a while.  This config is only used for discovery.
-	cfg.Burst = 100
-
-	if f.cacheDir != "" {
-		wt := cfg.WrapTransport
-		cfg.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {
-			if wt != nil {
-				rt = wt(rt)
-			}
-			return transport.NewCacheRoundTripper(f.cacheDir, rt)
-		}
-	}
-
-	discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
-	if err != nil {
-		return nil, err
-	}
-	cacheDir := computeDiscoverCacheDir(filepath.Join(homedir.HomeDir(), ".kube", "cache", "discovery"), cfg.Host)
-	return NewCachedDiscoveryClient(discoveryClient, cacheDir, time.Duration(10*time.Minute)), nil
-}
-
-func (f *discoveryFactory) BindFlags(flags *pflag.FlagSet) {
-	defaultCacheDir := filepath.Join(homedir.HomeDir(), ".kube", "http-cache")
-	flags.StringVar(&f.cacheDir, FlagHTTPCacheDir, defaultCacheDir, "Default HTTP cache directory")
-}
-
 // DefaultClientConfig creates a clientcmd.ClientConfig with the following hierarchy:
 //   1.  Use the kubeconfig builder.  The number of merges and overrides here gets a little crazy.  Stay with me.
 //       1.  Merge the kubeconfig itself.  This is done with the following hierarchy rules:
@@ -201,7 +155,32 @@ func DefaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig {
 }
 
 func (f *ring0Factory) DiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
-	return f.discoveryFactory.DiscoveryClient()
+	cfg, err := f.clientConfig.ClientConfig()
+	if err != nil {
+		return nil, err
+	}
+
+	// The more groups you have, the more discovery requests you need to make.
+	// given 25 groups (our groups + a few custom resources) with one-ish version each, discovery needs to make 50 requests
+	// double it just so we don't end up here again for a while.  This config is only used for discovery.
+	cfg.Burst = 100
+
+	if f.discoveryCacheDir != "" {
+		wt := cfg.WrapTransport
+		cfg.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {
+			if wt != nil {
+				rt = wt(rt)
+			}
+			return transport.NewCacheRoundTripper(f.discoveryCacheDir, rt)
+		}
+	}
+
+	discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
+	if err != nil {
+		return nil, err
+	}
+	cacheDir := computeDiscoverCacheDir(filepath.Join(homedir.HomeDir(), ".kube", "cache", "discovery"), cfg.Host)
+	return NewCachedDiscoveryClient(discoveryClient, cacheDir, time.Duration(10*time.Minute)), nil
 }
 
 func (f *ring0Factory) KubernetesClientSet() (*kubernetes.Clientset, error) {
@@ -387,10 +366,6 @@ func (f *ring0Factory) LabelsForObject(object runtime.Object) (map[string]string
 	return meta.NewAccessor().Labels(object)
 }
 
-func (f *ring0Factory) FlagSet() *pflag.FlagSet {
-	return f.flags
-}
-
 // Set showSecrets false to filter out stuff like secrets.
 func (f *ring0Factory) Command(cmd *cobra.Command, showSecrets bool) string {
 	if len(os.Args) == 0 {
@@ -432,7 +407,8 @@ func (f *ring0Factory) BindFlags(flags *pflag.FlagSet) {
 	// to do that automatically for every subcommand.
 	flags.BoolVar(&f.requireMatchedServerVersion, FlagMatchBinaryVersion, false, "Require server version to match client version")
 
-	f.discoveryFactory.BindFlags(flags)
+	defaultCacheDir := filepath.Join(homedir.HomeDir(), ".kube", "http-cache")
+	flags.StringVar(&f.discoveryCacheDir, FlagHTTPCacheDir, defaultCacheDir, "Default HTTP cache directory")
 
 	// Normalize all flags that are coming from other packages or pre-configurations
 	// a.k.a. change all "_" to "-". e.g. glog package

pkg/kubectl/cmd/util/factory_test.go

@@ -32,11 +32,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/watch"
-	"k8s.io/apiserver/pkg/util/flag"
 	manualfake "k8s.io/client-go/rest/fake"
 	testcore "k8s.io/client-go/testing"
-	"k8s.io/client-go/tools/clientcmd"
-	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/api/testapi"
 	api "k8s.io/kubernetes/pkg/apis/core"
@@ -47,23 +44,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 )
 
-func TestNewFactoryDefaultFlagBindings(t *testing.T) {
-	factory := NewFactory(nil)
-
-	if !factory.FlagSet().HasFlags() {
-		t.Errorf("Expected flags, but didn't get any")
-	}
-}
-
-func TestNewFactoryNoFlagBindings(t *testing.T) {
-	clientConfig := clientcmd.NewDefaultClientConfig(*clientcmdapi.NewConfig(), &clientcmd.ConfigOverrides{})
-	factory := NewFactory(clientConfig)
-
-	if factory.FlagSet().HasFlags() {
-		t.Errorf("Expected zero flags, but got %v", factory.FlagSet())
-	}
-}
-
 func TestPortsForObject(t *testing.T) {
 	f := NewFactory(nil)
 
@@ -211,18 +191,6 @@ func TestCanBeExposed(t *testing.T) {
 	}
 }
 
-func TestFlagUnderscoreRenaming(t *testing.T) {
-	factory := NewFactory(nil)
-
-	factory.FlagSet().SetNormalizeFunc(flag.WordSepNormalizeFunc)
-	factory.FlagSet().Bool("valid_flag", false, "bool value")
-
-	// In case of failure of this test check this PR: spf13/pflag#23
-	if factory.FlagSet().Lookup("valid_flag").Name != "valid-flag" {
-		t.Fatalf("Expected flag name to be valid-flag, got %s", factory.FlagSet().Lookup("valid_flag").Name)
-	}
-}
-
 func newPodList(count, isUnready, isUnhealthy int, labels map[string]string) *api.PodList {
 	pods := []api.Pod{}
 	for i := 0; i < count; i++ {