Merge pull request #31947 from smarterclayton/check_empty_config

Automatic merge from submit-queue

Check for EmptyConfig errors when trying to use in-cluster config

By removing the default "localhost:8080" behavior several paths in
client config began returning err == ErrEmptyConfig rather than err ==
nil.  The code checking for in cluster config was wrong - the logic
should be:

1. If loading the underlying config returns a non-empty error, fail
2. If the underlying config is not equal to the default config,
   return that config (it has user input)
3. If it is possible to use in-cluster config, do so
4. Otherwise return the default config (and or default EmptyConfig
   error).

Fixes #31910 @thockin @lavalamp @deads2k. We introduced EmptyConfig a very long time ago, but until I removed the "localhost:8080" Kubernetes was not actually exposing it (OpenShift relies on it since we don't set an insecure default). This properly falls through to in-cluster config in the described conditions.



```release-note
Some components like kube-dns and kube-proxy could fail to load the service account token when started within a pod. Properly handle empty configurations to try loading the service account config.
```

pkg/client/unversioned/clientcmd/client_config.go

@@ -350,6 +350,8 @@ func (config *DirectClientConfig) getCluster() clientcmdapi.Cluster {
 // inClusterClientConfig makes a config that will work from within a kubernetes cluster container environment.
 type inClusterClientConfig struct{}
 
+var _ ClientConfig = inClusterClientConfig{}
+
 func (inClusterClientConfig) RawConfig() (clientcmdapi.Config, error) {
 	return clientcmdapi.Config{}, fmt.Errorf("inCluster environment config doesn't support multiple clusters")
 }
@@ -358,21 +360,21 @@ func (inClusterClientConfig) ClientConfig() (*restclient.Config, error) {
 	return restclient.InClusterConfig()
 }
 
-func (inClusterClientConfig) Namespace() (string, error) {
+func (inClusterClientConfig) Namespace() (string, bool, error) {
 	// This way assumes you've set the POD_NAMESPACE environment variable using the downward API.
 	// This check has to be done first for backwards compatibility with the way InClusterConfig was originally set up
 	if ns := os.Getenv("POD_NAMESPACE"); ns != "" {
-		return ns, nil
+		return ns, true, nil
 	}
 
 	// Fall back to the namespace associated with the service account token, if available
 	if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
 		if ns := strings.TrimSpace(string(data)); len(ns) > 0 {
-			return ns, nil
+			return ns, true, nil
 		}
 	}
 
-	return "default", nil
+	return "default", false, nil
 }
 
 func (inClusterClientConfig) ConfigAccess() ConfigAccess {

pkg/client/unversioned/clientcmd/merged_client_builder.go

@@ -21,8 +21,6 @@ import (
 	"reflect"
 	"sync"
 
-	"github.com/golang/glog"
-
 	"k8s.io/kubernetes/pkg/client/restclient"
 	clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
 )
@@ -39,16 +37,25 @@ type DeferredLoadingClientConfig struct {
 
 	clientConfig ClientConfig
 	loadingLock  sync.Mutex
+
+	// provided for testing
+	icc InClusterConfig
+}
+
+// InClusterConfig abstracts details of whether the client is running in a cluster for testing.
+type InClusterConfig interface {
+	ClientConfig
+	Possible() bool
 }
 
 // NewNonInteractiveDeferredLoadingClientConfig creates a ConfigClientClientConfig using the passed context name
 func NewNonInteractiveDeferredLoadingClientConfig(loader ClientConfigLoader, overrides *ConfigOverrides) ClientConfig {
-	return &DeferredLoadingClientConfig{loader: loader, overrides: overrides}
+	return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, icc: inClusterClientConfig{}}
 }
 
 // NewInteractiveDeferredLoadingClientConfig creates a ConfigClientClientConfig using the passed context name and the fallback auth reader
 func NewInteractiveDeferredLoadingClientConfig(loader ClientConfigLoader, overrides *ConfigOverrides, fallbackReader io.Reader) ClientConfig {
-	return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, fallbackReader: fallbackReader}
+	return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, icc: inClusterClientConfig{}, fallbackReader: fallbackReader}
 }
 
 func (config *DeferredLoadingClientConfig) createClientConfig() (ClientConfig, error) {
@@ -92,18 +99,32 @@ func (config *DeferredLoadingClientConfig) ClientConfig() (*restclient.Config, e
 		return nil, err
 	}
 
+	// load the configuration and return on non-empty errors and if the
+	// content differs from the default config
 	mergedConfig, err := mergedClientConfig.ClientConfig()
-	if err != nil {
+	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, err := DefaultClientConfig.ClientConfig()
+		if err == nil && !reflect.DeepEqual(mergedConfig, defaultConfig) {
+			return mergedConfig, nil
+		}
 	}
-	// Are we running in a cluster and were no other configs found? If so, use the in-cluster-config.
+
-	icc := inClusterClientConfig{}
+	// check for in-cluster configuration and use it
-	defaultConfig, err := DefaultClientConfig.ClientConfig()
+	if config.icc.Possible() {
-	if icc.Possible() && err == nil && reflect.DeepEqual(mergedConfig, defaultConfig) {
+		return config.icc.ClientConfig()
-		glog.V(2).Info("No kubeconfig could be created, falling back to service account.")
-		return icc.ClientConfig()
 	}
-	return mergedConfig, nil
+
+	// return the result of the merged client config
+	return mergedConfig, err
 }
 
 // Namespace implements KubeConfig

pkg/client/unversioned/clientcmd/merged_client_builder_test.go

@@ -0,0 +1,194 @@
+/*
+Copyright 2014 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 clientcmd
+
+import (
+	"fmt"
+	"testing"
+
+	"k8s.io/kubernetes/pkg/client/restclient"
+	clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
+)
+
+type testLoader struct {
+	ClientConfigLoader
+
+	called bool
+	config *clientcmdapi.Config
+	err    error
+}
+
+func (l *testLoader) Load() (*clientcmdapi.Config, error) {
+	l.called = true
+	return l.config, l.err
+}
+
+type testClientConfig struct {
+	config *restclient.Config
+	err    error
+}
+
+func (c *testClientConfig) RawConfig() (clientcmdapi.Config, error) {
+	return clientcmdapi.Config{}, fmt.Errorf("unexpected call")
+}
+func (c *testClientConfig) ClientConfig() (*restclient.Config, error) {
+	return c.config, c.err
+}
+func (c *testClientConfig) Namespace() (string, bool, error) {
+	return "", false, fmt.Errorf("unexpected call")
+}
+func (c *testClientConfig) ConfigAccess() ConfigAccess {
+	return nil
+}
+
+type testICC struct {
+	testClientConfig
+
+	possible bool
+	called   bool
+}
+
+func (icc *testICC) Possible() bool {
+	icc.called = true
+	return icc.possible
+}
+
+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{},
+	}
+	config1, err := default1.ClientConfig()
+	if err != nil {
+		t.Fatal(err)
+	}
+	config2 := &restclient.Config{Host: "config2"}
+	err1 := fmt.Errorf("unique error")
+
+	testCases := map[string]struct {
+		clientConfig  *testClientConfig
+		icc           *testICC
+		defaultConfig *DirectClientConfig
+
+		checkedICC bool
+		result     *restclient.Config
+		err        error
+	}{
+		"in-cluster checked on other error": {
+			clientConfig: &testClientConfig{err: ErrEmptyConfig},
+			icc:          &testICC{},
+
+			checkedICC: true,
+			result:     nil,
+			err:        ErrEmptyConfig,
+		},
+
+		"in-cluster not checked on non-empty error": {
+			clientConfig: &testClientConfig{err: ErrEmptyCluster},
+			icc:          &testICC{},
+
+			checkedICC: false,
+			result:     nil,
+			err:        ErrEmptyCluster,
+		},
+
+		"in-cluster checked when config is default": {
+			defaultConfig: default1,
+			clientConfig:  &testClientConfig{config: config1},
+			icc:           &testICC{},
+
+			checkedICC: true,
+			result:     config1,
+			err:        nil,
+		},
+
+		"in-cluster not checked when config is not equal to default": {
+			defaultConfig: default1,
+			clientConfig:  &testClientConfig{config: config2},
+			icc:           &testICC{},
+
+			checkedICC: false,
+			result:     config2,
+			err:        nil,
+		},
+
+		"in-cluster checked when config is not equal to default and error is empty": {
+			clientConfig: &testClientConfig{config: config2, err: ErrEmptyConfig},
+			icc:          &testICC{},
+
+			checkedICC: true,
+			result:     config2,
+			err:        ErrEmptyConfig,
+		},
+
+		"in-cluster error returned when config is empty": {
+			clientConfig: &testClientConfig{err: ErrEmptyConfig},
+			icc: &testICC{
+				possible: true,
+				testClientConfig: testClientConfig{
+					err: err1,
+				},
+			},
+
+			checkedICC: true,
+			result:     nil,
+			err:        err1,
+		},
+
+		"in-cluster config returned when config is empty": {
+			clientConfig: &testClientConfig{err: ErrEmptyConfig},
+			icc: &testICC{
+				possible: true,
+				testClientConfig: testClientConfig{
+					config: config2,
+				},
+			},
+
+			checkedICC: true,
+			result:     config2,
+			err:        nil,
+		},
+	}
+
+	for name, test := range testCases {
+		if test.defaultConfig != nil {
+			DefaultClientConfig = *test.defaultConfig
+		} else {
+			DefaultClientConfig = *baseDefault
+		}
+		c := &DeferredLoadingClientConfig{icc: test.icc}
+		c.clientConfig = test.clientConfig
+
+		cfg, err := c.ClientConfig()
+		if test.icc.called != test.checkedICC {
+			t.Errorf("%s: unexpected in-cluster-config call %t", name, test.icc.called)
+		}
+		if err != test.err || cfg != test.result {
+			t.Errorf("%s: unexpected result: %v %#v", name, err, cfg)
+		}
+	}
+}