Merge pull request #33136 from smarterclayton/default_config

Automatic merge from submit-queue

When client config is default or default is invalid, check ICC

Alternative fix to #33019

pkg/client/unversioned/clientcmd/client_config.go

@@ -34,16 +34,25 @@ import (
 )
 
 var (
-	// DefaultCluster is the cluster config used when no other config is specified
+	// ClusterDefaults has the same behavior as the old EnvVar and DefaultCluster fields
-	// TODO: eventually apiserver should start on 443 and be secure by default
+	// DEPRECATED will be replaced
-	DefaultCluster = clientcmdapi.Cluster{Server: "http://localhost:8080"}
+	ClusterDefaults = clientcmdapi.Cluster{Server: getDefaultServer()}
-
+	// DefaultClientConfig represents the legacy behavior of this package for defaulting
-	// EnvVarCluster allows overriding the DefaultCluster using an envvar for the server name
+	// DEPRECATED will be replace
-	EnvVarCluster = clientcmdapi.Cluster{Server: os.Getenv("KUBERNETES_MASTER")}
+	DefaultClientConfig = DirectClientConfig{*clientcmdapi.NewConfig(), "", &ConfigOverrides{
-
+		ClusterDefaults: ClusterDefaults,
-	DefaultClientConfig = DirectClientConfig{*clientcmdapi.NewConfig(), "", &ConfigOverrides{}, nil, NewDefaultClientConfigLoadingRules()}
+	}, nil, NewDefaultClientConfigLoadingRules()}
 )
 
+// getDefaultServer returns a default setting for DefaultClientConfig
+// DEPRECATED
+func getDefaultServer() string {
+	if server := os.Getenv("KUBERNETES_MASTER"); len(server) > 0 {
+		return server
+	}
+	return "http://localhost:8080"
+}
+
 // ClientConfig is used to make it easy to get an api server client
 type ClientConfig interface {
 	// RawConfig returns the merged result of all overrides
@@ -347,7 +356,6 @@ func (config *DirectClientConfig) getCluster() clientcmdapi.Cluster {
 
 	var mergedClusterInfo clientcmdapi.Cluster
 	mergo.Merge(&mergedClusterInfo, config.overrides.ClusterDefaults)
-	mergo.Merge(&mergedClusterInfo, EnvVarCluster)
 	if configClusterInfo, exists := clusterInfos[clusterInfoName]; exists {
 		mergo.Merge(&mergedClusterInfo, configClusterInfo)
 	}

pkg/client/unversioned/clientcmd/client_config_test.go

@@ -293,8 +293,6 @@ func TestCreateCleanWithPrefix(t *testing.T) {
 		{"anything", "anything"},
 	}
 
-	// WARNING: EnvVarCluster.Server is set during package loading time and can not be overridden by os.Setenv inside this test
-	EnvVarCluster.Server = ""
 	tt = append(tt, struct{ server, host string }{"", "http://localhost:8080"})
 
 	for _, tc := range tt {
@@ -305,7 +303,7 @@ func TestCreateCleanWithPrefix(t *testing.T) {
 		config.Clusters["clean"] = cleanConfig
 
 		clientBuilder := NewNonInteractiveClientConfig(*config, "clean", &ConfigOverrides{
-			ClusterDefaults: DefaultCluster,
+			ClusterDefaults: clientcmdapi.Cluster{Server: "http://localhost:8080"},
 		}, nil)
 
 		clientConfig, err := clientBuilder.ClientConfig()
@@ -334,7 +332,7 @@ func TestCreateCleanDefault(t *testing.T) {
 func TestCreateCleanDefaultCluster(t *testing.T) {
 	config := createValidTestConfig()
 	clientBuilder := NewDefaultClientConfig(*config, &ConfigOverrides{
-		ClusterDefaults: DefaultCluster,
+		ClusterDefaults: clientcmdapi.Cluster{Server: "http://localhost:8080"},
 	})
 
 	clientConfig, err := clientBuilder.ClientConfig()
@@ -362,7 +360,7 @@ func TestCreateMissingContext(t *testing.T) {
 	const expectedErrorContains = "context was not found for specified context: not-present"
 	config := createValidTestConfig()
 	clientBuilder := NewNonInteractiveClientConfig(*config, "not-present", &ConfigOverrides{
-		ClusterDefaults: DefaultCluster,
+		ClusterDefaults: clientcmdapi.Cluster{Server: "http://localhost:8080"},
 	}, nil)
 
 	_, err := clientBuilder.ClientConfig()

pkg/client/unversioned/clientcmd/loader.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"reflect"
 	goruntime "runtime"
 	"strings"
 
@@ -30,6 +31,7 @@ import (
 	"github.com/imdario/mergo"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
+	"k8s.io/kubernetes/pkg/client/restclient"
 	clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
 	clientcmdlatest "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api/latest"
 	"k8s.io/kubernetes/pkg/runtime"
@@ -65,6 +67,9 @@ func currentMigrationRules() map[string]string {
 
 type ClientConfigLoader interface {
 	ConfigAccess
+	// IsDefaultConfig returns true if the returned config matches the defaults.
+	IsDefaultConfig(*restclient.Config) bool
+	// Load returns the latest config
 	Load() (*clientcmdapi.Config, error)
 }
 
@@ -96,6 +101,9 @@ func (g *ClientConfigGetter) IsExplicitFile() bool {
 func (g *ClientConfigGetter) GetExplicitFile() string {
 	return ""
 }
+func (g *ClientConfigGetter) IsDefaultConfig(config *restclient.Config) bool {
+	return false
+}
 
 // ClientConfigLoadingRules is an ExplicitPath and string slice of specific locations that are used for merging together a Config
 // Callers can put the chain together however they want, but we'd recommend:
@@ -112,6 +120,10 @@ type ClientConfigLoadingRules struct {
 	// DoNotResolvePaths indicates whether or not to resolve paths with respect to the originating files.  This is phrased as a negative so
 	// that a default object that doesn't set this will usually get the behavior it wants.
 	DoNotResolvePaths bool
+
+	// DefaultClientConfig is an optional field indicating what rules to use to calculate a default configuration.
+	// This should match the overrides passed in to ClientConfig loader.
+	DefaultClientConfig ClientConfig
 }
 
 // ClientConfigLoadingRules implements the ClientConfigLoader interface.
@@ -192,6 +204,7 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) {
 
 	// first merge all of our maps
 	mapConfig := clientcmdapi.NewConfig()
+
 	for _, kubeconfig := range kubeconfigs {
 		mergo.Merge(mapConfig, kubeconfig)
 	}
@@ -316,6 +329,18 @@ func (rules *ClientConfigLoadingRules) GetExplicitFile() string {
 	return rules.ExplicitPath
 }
 
+// IsDefaultConfig returns true if the provided configuration matches the default
+func (rules *ClientConfigLoadingRules) IsDefaultConfig(config *restclient.Config) bool {
+	if rules.DefaultClientConfig == nil {
+		return false
+	}
+	defaultConfig, err := rules.DefaultClientConfig.ClientConfig()
+	if err != nil {
+		return false
+	}
+	return reflect.DeepEqual(config, defaultConfig)
+}
+
 // LoadFromFile takes a filename and deserializes the contents into Config object
 func LoadFromFile(filename string) (*clientcmdapi.Config, error) {
 	kubeconfigBytes, err := ioutil.ReadFile(filename)

pkg/client/unversioned/clientcmd/merged_client_builder.go

@@ -18,7 +18,6 @@ package clientcmd
 
 import (
 	"io"
-	"reflect"
 	"sync"
 
 	"github.com/golang/glog"
@@ -105,20 +104,15 @@ func (config *DeferredLoadingClientConfig) ClientConfig() (*restclient.Config, e
 	// content differs from the default config
 	mergedConfig, err := mergedClientConfig.ClientConfig()
 	switch {
-	case err != nil && !IsEmptyConfig(err):
+	case err != nil:
-		// return on any error except empty config
+		if !IsEmptyConfig(err) {
-		return nil, err
+			// return on any error except empty config
-	case mergedConfig != nil:
+			return nil, err
-		// if the configuration has any settings at all, we cannot use ICC
-		// TODO: we need to discriminate better between "empty due to env" and
-		//   "empty due to defaults"
-		// TODO: this shouldn't be a global - the client config rules should be
-		//   handling this.
-		defaultConfig, defErr := DefaultClientConfig.ClientConfig()
-		if IsConfigurationInvalid(defErr) && !IsEmptyConfig(err) {
-			return mergedConfig, nil
 		}
-		if defErr == nil && !reflect.DeepEqual(mergedConfig, defaultConfig) {
+	case mergedConfig != nil:
+		// the configuration is valid, but if this is equal to the defaults we should try
+		// in-cluster configuration
+		if !config.loader.IsDefaultConfig(mergedConfig) {
 			return mergedConfig, nil
 		}
 	}

pkg/client/unversioned/clientcmd/merged_client_builder_test.go

@@ -70,20 +70,27 @@ func (icc *testICC) Possible() bool {
 }
 
 func TestInClusterConfig(t *testing.T) {
-	// override direct client config for this run
-	originalDefault := DefaultClientConfig
-	defer func() {
-		DefaultClientConfig = originalDefault
-	}()
-
-	baseDefault := &DirectClientConfig{
-		overrides: &ConfigOverrides{},
-	}
 	default1 := &DirectClientConfig{
 		config:      *createValidTestConfig(),
 		contextName: "clean",
 		overrides:   &ConfigOverrides{},
 	}
+	invalidDefaultConfig := clientcmdapi.NewConfig()
+	invalidDefaultConfig.Clusters["clean"] = &clientcmdapi.Cluster{
+		Server: "http://localhost:8080",
+	}
+	invalidDefaultConfig.Contexts["other"] = &clientcmdapi.Context{
+		Cluster: "clean",
+	}
+	invalidDefaultConfig.CurrentContext = "clean"
+
+	defaultInvalid := &DirectClientConfig{
+		config:    *invalidDefaultConfig,
+		overrides: &ConfigOverrides{},
+	}
+	if _, err := defaultInvalid.ClientConfig(); err == nil || !IsConfigurationInvalid(err) {
+		t.Fatal(err)
+	}
 	config1, err := default1.ClientConfig()
 	if err != nil {
 		t.Fatal(err)
@@ -128,6 +135,16 @@ func TestInClusterConfig(t *testing.T) {
 			err:        nil,
 		},
 
+		"in-cluster not checked when default config is invalid": {
+			defaultConfig: defaultInvalid,
+			clientConfig:  &testClientConfig{config: config1},
+			icc:           &testICC{},
+
+			checkedICC: false,
+			result:     config1,
+			err:        nil,
+		},
+
 		"in-cluster not checked when config is not equal to default": {
 			defaultConfig: default1,
 			clientConfig:  &testClientConfig{config: config2},
@@ -175,7 +192,7 @@ func TestInClusterConfig(t *testing.T) {
 			err:        nil,
 		},
 
-		"in-cluster not checked when default is invalid": {
+		"in-cluster not checked when standard default is invalid": {
 			defaultConfig: &DefaultClientConfig,
 			clientConfig:  &testClientConfig{config: config2},
 			icc:           &testICC{},
@@ -187,12 +204,8 @@ func TestInClusterConfig(t *testing.T) {
 	}
 
 	for name, test := range testCases {
-		if test.defaultConfig != nil {
-			DefaultClientConfig = *test.defaultConfig
-		} else {
-			DefaultClientConfig = *baseDefault
-		}
 		c := &DeferredLoadingClientConfig{icc: test.icc}
+		c.loader = &ClientConfigLoadingRules{DefaultClientConfig: test.defaultConfig}
 		c.clientConfig = test.clientConfig
 
 		cfg, err := c.ClientConfig()

pkg/kubectl/cmd/util/factory.go

@@ -34,7 +34,6 @@ import (
 
 	"github.com/blang/semver"
 	"github.com/emicklei/go-restful/swagger"
-	"github.com/imdario/mergo"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
@@ -1161,11 +1160,13 @@ func (c *clientSwaggerSchema) ValidateBytes(data []byte) error {
 //     exists and is not a directory.
 func DefaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig {
 	loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
+	// use the standard defaults for this client command
+	// DEPRECATED: remove and replace with something more accurate
+	loadingRules.DefaultClientConfig = &clientcmd.DefaultClientConfig
+
 	flags.StringVar(&loadingRules.ExplicitPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests.")
 
-	overrides := &clientcmd.ConfigOverrides{}
+	overrides := &clientcmd.ConfigOverrides{ClusterDefaults: clientcmd.ClusterDefaults}
-	// use the standard defaults for this client config
-	mergo.Merge(&overrides.ClusterDefaults, clientcmd.DefaultCluster)
 
 	flagNames := clientcmd.RecommendedConfigOverrideFlags("")
 	// short flagnames are disabled by default.  These are here for compatibility with existing scripts