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

smarterclayton · 2016-09-02T05:58:19Z

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:

If loading the underlying config returns a non-empty error, fail
If the underlying config is not equal to the default config,
return that config (it has user input)
If it is possible to use in-cluster config, do so
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.

This change is

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.

smarterclayton · 2016-09-02T06:00:05Z

Note the controller and scheduler were not affected because they don't use a DeferredNonInteractiveConfig. Kubectl was not affected because it uses a Deferred*Config with defaulting. OpenShift would not have been broken since we use a wrapper (which basically was there to fix this particular bug).

smarterclayton · 2016-09-02T06:02:39Z

I don't think this needs a release note because the issue never actually manifested in any code (kube-dns was not built).

smarterclayton · 2016-09-02T16:04:00Z

@deads2k can you review?

lavalamp · 2016-09-02T22:13:43Z

pkg/client/unversioned/clientcmd/client_config.go

@@ -358,21 +358,21 @@ func (inClusterClientConfig) ClientConfig() (*restclient.Config, error) {
 	return restclient.InClusterConfig()
 }

-func (inClusterClientConfig) Namespace() (string, error) {


this function never actually returns an error. Remove the error parameter or use it instead of the bool.

this function never actually returns an error. Remove the error parameter or use it instead of the bool.

It's implementing an interface. Other implementors do return errors.

Add a couple type assertions. this dropped out of the ClientConfig interface at some point.

If it's part of an interface, then change the interface, too-- (string, bool, error) is a confusing return set.

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.

lavalamp · 2016-09-02T22:18:11Z

I still can't understand this loader code. lgtm but maybe @jlowdermilk wants to take a look?

deads2k · 2016-09-06T12:16:08Z

pkg/client/unversioned/clientcmd/merged_client_builder.go

-		return icc.ClientConfig()
+
+	// check for in-cluster configuration and use it
+	if config.icc.Possible() {


If this is going to have priority, why don't we do it first?

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.

smarterclayton · 2016-09-06T20:03:48Z

@deads2k any other comments?

deads2k · 2016-09-06T20:11:02Z

@smarterclayton no more comments from me.

lgtm, could use a squash.

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).

k8s-bot · 2016-09-06T22:25:13Z

GCE e2e build/test passed for commit 48b4d6d.

smarterclayton · 2016-09-07T03:10:38Z

Squashed

k8s-github-robot · 2016-09-07T04:26:12Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-09-07T05:02:19Z

GCE e2e build/test passed for commit 48b4d6d.

k8s-github-robot · 2016-09-07T05:02:55Z

Automatic merge from submit-queue

…32165-#31655-#31833-#31947-#30105-upstream-release-1.4 Automated cherry pick of #31794 #32165 #31655 #31833 #31947 #30105

Changes made in kubernetes#31947 cause kube-proxy to ignore `--kubeconfig` and `--master` flags and use in-cluster configuration, which is unusable due to the fact that VIP hasn't been created yet.

…32072-#32105-#32197-#32181-upstream-release-1.4 Automated cherry pick of #31947 #32072 #32105 #32197 #32181

Automatic merge from submit-queue Automated cherry pick of #31947 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). Cherry-pick of #31947

eparis · 2016-09-13T16:55:23Z

removing cherrypick-candidate as this was previously merged in release-1.4

Changes made in kubernetes#31947 cause kube-proxy to ignore `--kubeconfig` and `--master` flags and use in-cluster configuration, which is unusable due to the fact that VIP hasn't been created yet.

Check if default config is invalid before comparing it Changes made in kubernetes#31947 cause kube-proxy to ignore `--kubeconfig` and `--master` flags and use in-cluster configuration, which is unusable due to the fact that VIP hasn't been created yet.

…-of-#31794-kubernetes#32165-kubernetes#31655-kubernetes#31833-kubernetes#31947-kubernetes#30105-upstream-release-1.4 Automated cherry pick of kubernetes#31794 kubernetes#32165 kubernetes#31655 kubernetes#31833 kubernetes#31947 kubernetes#30105

…-of-#31947-kubernetes#32072-kubernetes#32105-kubernetes#32197-kubernetes#32181-upstream-release-1.4 Automated cherry pick of kubernetes#31947 kubernetes#32072 kubernetes#32105 kubernetes#32197 kubernetes#32181

…onfig_14 Automatic merge from submit-queue Automated cherry pick of kubernetes#31947 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). Cherry-pick of kubernetes#31947

googlebot added the cla: yes label Sep 2, 2016

smarterclayton assigned lavalamp and deads2k Sep 2, 2016

smarterclayton added this to the v1.4 milestone Sep 2, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 2, 2016

dims mentioned this pull request Sep 2, 2016

Set DefaultCluster in config overrides #31940

Closed

smarterclayton added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. cherrypick-candidate labels Sep 2, 2016

lavalamp reviewed Sep 2, 2016
View reviewed changes

lavalamp assigned j3ffml Sep 2, 2016

deads2k reviewed Sep 6, 2016
View reviewed changes

smarterclayton force-pushed the check_empty_config branch from de7e1c7 to 48b4d6d Compare September 6, 2016 21:46

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2016

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 7, 2016

smarterclayton mentioned this pull request Sep 7, 2016

Automated cherry pick of #31947 #32180

Merged

smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Sep 7, 2016

k8s-github-robot merged commit 9dce773 into kubernetes:master Sep 7, 2016

pwittrock added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 7, 2016

foxish mentioned this pull request Sep 7, 2016

Automated cherry pick of #31794 #32165 #31655 #31833 #31947 #30105 #32236

Merged

foxish added a commit that referenced this pull request Sep 8, 2016

Merge pull request #32236 from foxish/automated-cherry-pick-of-#31794-#…

48f725c

…32165-#31655-#31833-#31947-#30105-upstream-release-1.4 Automated cherry pick of #31794 #32165 #31655 #31833 #31947 #30105

smarterclayton mentioned this pull request Sep 9, 2016

Namespace lookup fails on in-cluster config #32352

Closed

errordeveloper mentioned this pull request Sep 10, 2016

Check if default config is invalid before comparing it #32438

Closed

foxish mentioned this pull request Sep 10, 2016

Automated cherry pick of #31947 #32072 #32105 #32197 #32181 #32443

Merged

pwittrock pushed a commit that referenced this pull request Sep 10, 2016

Merge pull request #32443 from foxish/automated-cherry-pick-of-#31947-#…

532f507

…32072-#32105-#32197-#32181-upstream-release-1.4 Automated cherry pick of #31947 #32072 #32105 #32197 #32181

eparis removed the cherrypick-candidate label Sep 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

smarterclayton commented Sep 2, 2016 •

edited

Loading

smarterclayton commented Sep 2, 2016 •

edited

Loading

smarterclayton commented Sep 2, 2016

smarterclayton commented Sep 2, 2016

lavalamp Sep 2, 2016

deads2k Sep 6, 2016

deads2k Sep 6, 2016

lavalamp Sep 6, 2016

smarterclayton Sep 6, 2016

lavalamp commented Sep 2, 2016

deads2k Sep 6, 2016

deads2k Sep 6, 2016

smarterclayton commented Sep 6, 2016

deads2k commented Sep 6, 2016

k8s-bot commented Sep 6, 2016

smarterclayton commented Sep 7, 2016

k8s-github-robot commented Sep 7, 2016

k8s-bot commented Sep 7, 2016

k8s-github-robot commented Sep 7, 2016

eparis commented Sep 13, 2016

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

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

Conversation

smarterclayton commented Sep 2, 2016 • edited Loading

smarterclayton commented Sep 2, 2016 • edited Loading

smarterclayton commented Sep 2, 2016

smarterclayton commented Sep 2, 2016

lavalamp Sep 2, 2016

Choose a reason for hiding this comment

deads2k Sep 6, 2016

Choose a reason for hiding this comment

deads2k Sep 6, 2016

Choose a reason for hiding this comment

lavalamp Sep 6, 2016

Choose a reason for hiding this comment

smarterclayton Sep 6, 2016

Choose a reason for hiding this comment

lavalamp commented Sep 2, 2016

deads2k Sep 6, 2016

Choose a reason for hiding this comment

deads2k Sep 6, 2016

Choose a reason for hiding this comment

smarterclayton commented Sep 6, 2016

deads2k commented Sep 6, 2016

k8s-bot commented Sep 6, 2016

smarterclayton commented Sep 7, 2016

k8s-github-robot commented Sep 7, 2016

k8s-bot commented Sep 7, 2016

k8s-github-robot commented Sep 7, 2016

eparis commented Sep 13, 2016

smarterclayton commented Sep 2, 2016 •

edited

Loading

smarterclayton commented Sep 2, 2016 •

edited

Loading