Skip to content

Commit 48b4d6d

Browse files
smarterclaytonsmarterclayton
committed
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).
1 parent 3da5d78 commit 48b4d6d

File tree

3 files changed

+233
-16
lines changed

3 files changed

+233
-16
lines changed

‎pkg/client/unversioned/clientcmd/client_config.go

+6-4
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,8 @@ func (config *DirectClientConfig) getCluster() clientcmdapi.Cluster {
350350
// inClusterClientConfig makes a config that will work from within a kubernetes cluster container environment.
351351
type inClusterClientConfig struct{}
352352

353+
var _ ClientConfig = inClusterClientConfig{}
354+
353355
func (inClusterClientConfig) RawConfig() (clientcmdapi.Config, error) {
354356
return clientcmdapi.Config{}, fmt.Errorf("inCluster environment config doesn't support multiple clusters")
355357
}
@@ -358,21 +360,21 @@ func (inClusterClientConfig) ClientConfig() (*restclient.Config, error) {
358360
return restclient.InClusterConfig()
359361
}
360362

361-
func (inClusterClientConfig) Namespace() (string, error) {
363+
func (inClusterClientConfig) Namespace() (string, bool, error) {
362364
// This way assumes you've set the POD_NAMESPACE environment variable using the downward API.
363365
// This check has to be done first for backwards compatibility with the way InClusterConfig was originally set up
364366
if ns := os.Getenv("POD_NAMESPACE"); ns != "" {
365-
return ns, nil
367+
return ns, true, nil
366368
}
367369

368370
// Fall back to the namespace associated with the service account token, if available
369371
if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
370372
if ns := strings.TrimSpace(string(data)); len(ns) > 0 {
371-
return ns, nil
373+
return ns, true, nil
372374
}
373375
}
374376

375-
return "default", nil
377+
return "default", false, nil
376378
}
377379

378380
func (inClusterClientConfig) ConfigAccess() ConfigAccess {

‎pkg/client/unversioned/clientcmd/merged_client_builder.go

+33-12
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import (
2121
"reflect"
2222
"sync"
2323

24-
"github.com/golang/glog"
25-
2624
"k8s.io/kubernetes/pkg/client/restclient"
2725
clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
2826
)
@@ -39,16 +37,25 @@ type DeferredLoadingClientConfig struct {
3937

4038
clientConfig ClientConfig
4139
loadingLock sync.Mutex
40+
41+
// provided for testing
42+
icc InClusterConfig
43+
}
44+
45+
// InClusterConfig abstracts details of whether the client is running in a cluster for testing.
46+
type InClusterConfig interface {
47+
ClientConfig
48+
Possible() bool
4249
}
4350

4451
// NewNonInteractiveDeferredLoadingClientConfig creates a ConfigClientClientConfig using the passed context name
4552
func NewNonInteractiveDeferredLoadingClientConfig(loader ClientConfigLoader, overrides *ConfigOverrides) ClientConfig {
46-
return &DeferredLoadingClientConfig{loader: loader, overrides: overrides}
53+
return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, icc: inClusterClientConfig{}}
4754
}
4855

4956
// NewInteractiveDeferredLoadingClientConfig creates a ConfigClientClientConfig using the passed context name and the fallback auth reader
5057
func NewInteractiveDeferredLoadingClientConfig(loader ClientConfigLoader, overrides *ConfigOverrides, fallbackReader io.Reader) ClientConfig {
51-
return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, fallbackReader: fallbackReader}
58+
return &DeferredLoadingClientConfig{loader: loader, overrides: overrides, icc: inClusterClientConfig{}, fallbackReader: fallbackReader}
5259
}
5360

5461
func (config *DeferredLoadingClientConfig) createClientConfig() (ClientConfig, error) {
@@ -92,18 +99,32 @@ func (config *DeferredLoadingClientConfig) ClientConfig() (*restclient.Config, e
9299
return nil, err
93100
}
94101

102+
// load the configuration and return on non-empty errors and if the
103+
// content differs from the default config
95104
mergedConfig, err := mergedClientConfig.ClientConfig()
96-
if err != nil {
105+
switch {
106+
case err != nil && !IsEmptyConfig(err):
107+
// return on any error except empty config
97108
return nil, err
109+
case mergedConfig != nil:
110+
// if the configuration has any settings at all, we cannot use ICC
111+
// TODO: we need to discriminate better between "empty due to env" and
112+
// "empty due to defaults"
113+
// TODO: this shouldn't be a global - the client config rules should be
114+
// handling this.
115+
defaultConfig, err := DefaultClientConfig.ClientConfig()
116+
if err == nil && !reflect.DeepEqual(mergedConfig, defaultConfig) {
117+
return mergedConfig, nil
118+
}
98119
}
99-
// Are we running in a cluster and were no other configs found? If so, use the in-cluster-config.
100-
icc := inClusterClientConfig{}
101-
defaultConfig, err := DefaultClientConfig.ClientConfig()
102-
if icc.Possible() && err == nil && reflect.DeepEqual(mergedConfig, defaultConfig) {
103-
glog.V(2).Info("No kubeconfig could be created, falling back to service account.")
104-
return icc.ClientConfig()
120+
121+
// check for in-cluster configuration and use it
122+
if config.icc.Possible() {
123+
return config.icc.ClientConfig()
105124
}
106-
return mergedConfig, nil
125+
126+
// return the result of the merged client config
127+
return mergedConfig, err
107128
}
108129

109130
// Namespace implements KubeConfig

‎pkg/client/unversioned/clientcmd/merged_client_builder_test.go

+194
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/*
2+
Copyright 2014 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package clientcmd
18+
19+
import (
20+
"fmt"
21+
"testing"
22+
23+
"k8s.io/kubernetes/pkg/client/restclient"
24+
clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
25+
)
26+
27+
type testLoader struct {
28+
ClientConfigLoader
29+
30+
called bool
31+
config *clientcmdapi.Config
32+
err error
33+
}
34+
35+
func (l *testLoader) Load() (*clientcmdapi.Config, error) {
36+
l.called = true
37+
return l.config, l.err
38+
}
39+
40+
type testClientConfig struct {
41+
config *restclient.Config
42+
err error
43+
}
44+
45+
func (c *testClientConfig) RawConfig() (clientcmdapi.Config, error) {
46+
return clientcmdapi.Config{}, fmt.Errorf("unexpected call")
47+
}
48+
func (c *testClientConfig) ClientConfig() (*restclient.Config, error) {
49+
return c.config, c.err
50+
}
51+
func (c *testClientConfig) Namespace() (string, bool, error) {
52+
return "", false, fmt.Errorf("unexpected call")
53+
}
54+
func (c *testClientConfig) ConfigAccess() ConfigAccess {
55+
return nil
56+
}
57+
58+
type testICC struct {
59+
testClientConfig
60+
61+
possible bool
62+
called bool
63+
}
64+
65+
func (icc *testICC) Possible() bool {
66+
icc.called = true
67+
return icc.possible
68+
}
69+
70+
func TestInClusterConfig(t *testing.T) {
71+
// override direct client config for this run
72+
originalDefault := DefaultClientConfig
73+
defer func() {
74+
DefaultClientConfig = originalDefault
75+
}()
76+
77+
baseDefault := &DirectClientConfig{
78+
overrides: &ConfigOverrides{},
79+
}
80+
default1 := &DirectClientConfig{
81+
config: *createValidTestConfig(),
82+
contextName: "clean",
83+
overrides: &ConfigOverrides{},
84+
}
85+
config1, err := default1.ClientConfig()
86+
if err != nil {
87+
t.Fatal(err)
88+
}
89+
config2 := &restclient.Config{Host: "config2"}
90+
err1 := fmt.Errorf("unique error")
91+
92+
testCases := map[string]struct {
93+
clientConfig *testClientConfig
94+
icc *testICC
95+
defaultConfig *DirectClientConfig
96+
97+
checkedICC bool
98+
result *restclient.Config
99+
err error
100+
}{
101+
"in-cluster checked on other error": {
102+
clientConfig: &testClientConfig{err: ErrEmptyConfig},
103+
icc: &testICC{},
104+
105+
checkedICC: true,
106+
result: nil,
107+
err: ErrEmptyConfig,
108+
},
109+
110+
"in-cluster not checked on non-empty error": {
111+
clientConfig: &testClientConfig{err: ErrEmptyCluster},
112+
icc: &testICC{},
113+
114+
checkedICC: false,
115+
result: nil,
116+
err: ErrEmptyCluster,
117+
},
118+
119+
"in-cluster checked when config is default": {
120+
defaultConfig: default1,
121+
clientConfig: &testClientConfig{config: config1},
122+
icc: &testICC{},
123+
124+
checkedICC: true,
125+
result: config1,
126+
err: nil,
127+
},
128+
129+
"in-cluster not checked when config is not equal to default": {
130+
defaultConfig: default1,
131+
clientConfig: &testClientConfig{config: config2},
132+
icc: &testICC{},
133+
134+
checkedICC: false,
135+
result: config2,
136+
err: nil,
137+
},
138+
139+
"in-cluster checked when config is not equal to default and error is empty": {
140+
clientConfig: &testClientConfig{config: config2, err: ErrEmptyConfig},
141+
icc: &testICC{},
142+
143+
checkedICC: true,
144+
result: config2,
145+
err: ErrEmptyConfig,
146+
},
147+
148+
"in-cluster error returned when config is empty": {
149+
clientConfig: &testClientConfig{err: ErrEmptyConfig},
150+
icc: &testICC{
151+
possible: true,
152+
testClientConfig: testClientConfig{
153+
err: err1,
154+
},
155+
},
156+
157+
checkedICC: true,
158+
result: nil,
159+
err: err1,
160+
},
161+
162+
"in-cluster config returned when config is empty": {
163+
clientConfig: &testClientConfig{err: ErrEmptyConfig},
164+
icc: &testICC{
165+
possible: true,
166+
testClientConfig: testClientConfig{
167+
config: config2,
168+
},
169+
},
170+
171+
checkedICC: true,
172+
result: config2,
173+
err: nil,
174+
},
175+
}
176+
177+
for name, test := range testCases {
178+
if test.defaultConfig != nil {
179+
DefaultClientConfig = *test.defaultConfig
180+
} else {
181+
DefaultClientConfig = *baseDefault
182+
}
183+
c := &DeferredLoadingClientConfig{icc: test.icc}
184+
c.clientConfig = test.clientConfig
185+
186+
cfg, err := c.ClientConfig()
187+
if test.icc.called != test.checkedICC {
188+
t.Errorf("%s: unexpected in-cluster-config call %t", name, test.icc.called)
189+
}
190+
if err != test.err || cfg != test.result {
191+
t.Errorf("%s: unexpected result: %v %#v", name, err, cfg)
192+
}
193+
}
194+
}

0 commit comments

Comments
 (0)