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's got 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).

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{}
-	defaultConfig, err := DefaultClientConfig.ClientConfig()
-	if icc.Possible() && err == nil && reflect.DeepEqual(mergedConfig, defaultConfig) {
-		glog.V(2).Info("No kubeconfig could be created, falling back to service account.")
-		return icc.ClientConfig()
+
+	// check for in-cluster configuration and use it
+	if config.icc.Possible() {
+		return config.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)
+		}
+	}
+}