-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for EmptyConfig errors when trying to use in-cluster config #31947
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is going to have priority, why don't we do it first? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hrm, I see. It doesn't have priority. And it can't go into the switch above because of the error handling down the non-nil patch. What sticky little bit of code. |
||
return config.icc.ClientConfig() | ||
} | ||
return mergedConfig, nil | ||
|
||
// return the result of the merged client config | ||
return mergedConfig, err | ||
} | ||
|
||
// Namespace implements KubeConfig | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function never actually returns an error. Remove the error parameter or use it instead of the bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's implementing an interface. Other implementors do return errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a couple type assertions. this dropped out of the
ClientConfig
interface at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's part of an interface, then change the interface, too-- (string, bool, error) is a confusing return set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is correct - this implementation just happens not to return an error. The bool is "was this specified" by the user. The string is the value. We use the bool all over the code to determine between user passed "--namespace" on the CLI vs getting the "default" namespace.