From 07f079216ee794035dfe337f7393b793ae420fec Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 20 Sep 2016 19:00:48 -0400 Subject: [PATCH] Make clientcmd defaulting a function of ConfigOverrides and LoadingRules This commit moves away from using a global variable for default configuration checking, and instead exposes a method on LoadingRules to determine whether a particular restclient.Config should be considered "default". This allows kubectl to provide its own defaults (the same as before, KUBERNETES_MASTER and the static localhost:8080 values) while allowing other clients to avoid defining them. In-cluster config defaulting is now easier to read. --- .../unversioned/clientcmd/client_config.go | 26 +++++++---- .../clientcmd/client_config_test.go | 8 ++-- pkg/client/unversioned/clientcmd/loader.go | 25 +++++++++++ .../clientcmd/merged_client_builder.go | 22 ++++------ .../clientcmd/merged_client_builder_test.go | 43 ++++++++++++------- pkg/kubectl/cmd/util/factory.go | 9 ++-- 6 files changed, 86 insertions(+), 47 deletions(-) diff --git a/pkg/client/unversioned/clientcmd/client_config.go b/pkg/client/unversioned/clientcmd/client_config.go index cd453004717..8223bad94ef 100644 --- a/pkg/client/unversioned/clientcmd/client_config.go +++ b/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 - // TODO: eventually apiserver should start on 443 and be secure by default - DefaultCluster = clientcmdapi.Cluster{Server: "http://localhost:8080"} - - // EnvVarCluster allows overriding the DefaultCluster using an envvar for the server name - EnvVarCluster = clientcmdapi.Cluster{Server: os.Getenv("KUBERNETES_MASTER")} - - DefaultClientConfig = DirectClientConfig{*clientcmdapi.NewConfig(), "", &ConfigOverrides{}, nil, NewDefaultClientConfigLoadingRules()} + // ClusterDefaults has the same behavior as the old EnvVar and DefaultCluster fields + // DEPRECATED will be replaced + ClusterDefaults = clientcmdapi.Cluster{Server: getDefaultServer()} + // DefaultClientConfig represents the legacy behavior of this package for defaulting + // DEPRECATED will be replace + DefaultClientConfig = DirectClientConfig{*clientcmdapi.NewConfig(), "", &ConfigOverrides{ + ClusterDefaults: ClusterDefaults, + }, 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) } diff --git a/pkg/client/unversioned/clientcmd/client_config_test.go b/pkg/client/unversioned/clientcmd/client_config_test.go index 365571bb84d..232ff480d80 100644 --- a/pkg/client/unversioned/clientcmd/client_config_test.go +++ b/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() diff --git a/pkg/client/unversioned/clientcmd/loader.go b/pkg/client/unversioned/clientcmd/loader.go index c93f51aa3fe..654dfe98d07 100644 --- a/pkg/client/unversioned/clientcmd/loader.go +++ b/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) diff --git a/pkg/client/unversioned/clientcmd/merged_client_builder.go b/pkg/client/unversioned/clientcmd/merged_client_builder.go index 407230369be..4c3645121b1 100644 --- a/pkg/client/unversioned/clientcmd/merged_client_builder.go +++ b/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): - // return on any error except empty config - return nil, err - case mergedConfig != nil: - // 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 + case err != nil: + if !IsEmptyConfig(err) { + // return on any error except empty config + return nil, err } - 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 } } diff --git a/pkg/client/unversioned/clientcmd/merged_client_builder_test.go b/pkg/client/unversioned/clientcmd/merged_client_builder_test.go index b45a87f642f..ec4b4072a97 100644 --- a/pkg/client/unversioned/clientcmd/merged_client_builder_test.go +++ b/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() diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 1540b399667..29df6a61496 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/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{} - // use the standard defaults for this client config - mergo.Merge(&overrides.ClusterDefaults, clientcmd.DefaultCluster) + overrides := &clientcmd.ConfigOverrides{ClusterDefaults: clientcmd.ClusterDefaults} flagNames := clientcmd.RecommendedConfigOverrideFlags("") // short flagnames are disabled by default. These are here for compatibility with existing scripts