Add optional loading from a config.d directory to kubectl. #38861

brendandburns · 2016-12-16T06:48:30Z

Addresses #38791

@justinsb @kubernetes/sig-cli-misc @kubernetes/sig-cli-pr-reviews

k8s-reviewable · 2016-12-16T06:48:38Z

This change is

justinsb

I like it; a few suggestions for your consideration, but otherwise I'm excited to see it. (PS ... fast work, sir!)

justinsb · 2016-12-16T14:39:02Z

pkg/client/unversioned/clientcmd/loader.go

+		return []string{}
+	}
+	if !stat.IsDir() {
+		glog.Warningf("Expected %s to be a directory", dir)


should we return []string{} now - I guess otherwise we'll print another error?

justinsb · 2016-12-16T14:43:50Z

pkg/client/unversioned/clientcmd/config.go

-
-	return []string{o.GlobalFile}
+	result := []string{o.GlobalFile}
+	result = append(result, loadConfigDir(o.GlobalFile+".d")...)


This sets off a spidey sense for me... Should we have a separate field GlobalFileDir or similar? And then we would set it to RecommendedHomeFileDir in NewDefaultPathOptions?

I agree with @justinsb

justinsb · 2016-12-16T14:45:11Z

pkg/client/unversioned/clientcmd/loader.go

@@ -148,6 +149,29 @@ func NewDefaultClientConfigLoadingRules() *ClientConfigLoadingRules {
 	}
 }

+func loadConfigDir(dir string) []string {
+	stat, err := os.Lstat(RecommendedHomeFileDir)


Perhaps skip if dir == "", although I'm not entirely sure how PathOptions is generally used.

justinsb · 2016-12-16T14:46:29Z

pkg/client/unversioned/clientcmd/loader_test.go

+	dir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+		t.FailNow()


Is this different from t.Fatalf?

Nope, fixed. (here and elswehere)

bgrant0607 · 2016-12-16T17:24:47Z

cc @deads2k

deads2k · 2016-12-16T17:33:20Z

I think this is worthy of discussion before changing. We used to merge kubeconfig files and it was difficult for users. We've also talked about splitting the kubeconfig file to separate preferences.

This pull looks like it just throws them all into a bucket, merges them, and hopes for the best. I suspect we'll end up with many unintended consequences. @kubernetes/sig-cli-misc @smarterclayton @liggitt @kubernetes/rh-ux

smarterclayton · 2016-12-16T18:42:34Z

I'd like a proposal and some design discussion before this goes any further. I see lots of folks who want to change kubeconfig, but no design docs.

brendandburns · 2016-12-17T00:06:52Z

@deads2k @smarterclayton look at the issue requesting this: #38791

Basically the idea is to allow users to keep all of their clusters in disjoint files.

I think allowing users to self-organize is going to be a better way forward than refactoring all of the merging code.

And more importantly, this is no different than what you can already do via $ENVVARS, it's just making it easier and directory based.

brendandburns · 2016-12-17T00:16:56Z

And fundamentally, this doesn't change kubeconfig. If you have an explicit kubeconfig file it still just uses that.

I'd be ok with an exclusive approach too, that being:

If ~/.kube/config is present use that, otherwise if ~/.kube/config.d is present, use the contents of that directory.

I think this is a small, incremental, useful step that will help people better organize their kubeconfig files.

bgrant0607 · 2016-12-20T17:17:09Z

Previously discussed in #9298, #10693, and #20605.

What would happen in the case of nicknames specified multiple times?

How would kubectl config commands work?

From where would context and preferences be taken?

brendandburns · 2016-12-20T18:56:40Z

@bgrant0607 this doesn't change any of the behaviors that are already built into kubectl.

This is 100% equivalent to:

KUBECONFIG=$(ls $HOME/.kube/config.d/*)
kubectl ...

But it is more convenient. Since it's not a behavior change, I don't see the problem.

It's quite possible that we should improve, change or update the merge policies, but they already exist
inside of kubectl.

This is basically putting the load on the user to ensure that the files are disjoint.

But it will enable users to copy around different kubeconfig files for specific cluster between different users and have different users be able to have a different union of clusters for their own use without having to do the manual effort of merging/separating out the configs.

Given that it doesn't introduce a new behavior, and that it is more convenient for this use case, I think that it is worth doing prior to any major refactor of config that is proposed for the future.

Let's not let the perfect be the enemy of the good here.

brendandburns · 2017-01-10T07:55:59Z

@bgrant0607 friendly new years ping on this. I believe it is worth doing, and @justinsb also is inclined to agree.

bgrant0607 · 2017-01-10T17:00:23Z

cc @jlowdermilk

smarterclayton · 2017-01-10T17:16:49Z

Can we detect the full default case?

I.e.:

If KUBECONFIG is set use that
If ~/.kube/config is present use that
If ~/.kube/config.d is present, use the contents of that directory.
Fall back to in-cluster config

I'd be ok with that.

smarterclayton · 2017-01-10T17:17:19Z

I would like to look at splitting out API token and certs into their own files, I'm not sure if this will complicate what @mikedanese and @jbeda wanted to do there.

bgrant0607

Made a few comments. Will leave more detailed review to @deads2k.

Documentation in kubernetes.github.io will also need to be updated.

bgrant0607 · 2017-01-10T17:13:40Z

pkg/client/unversioned/clientcmd/loader.go

 	} else {
 		chain = append(chain, RecommendedHomeFile)
+		chain = append(chain, loadConfigDir(RecommendedHomeFileDir)...)


It would be more user-friendly if users could also specify directories. If file => use it. If directory => expand to its files and use them.

bgrant0607 · 2017-01-10T17:19:02Z

pkg/client/unversioned/clientcmd/loader.go

+	}
+	result := []string{}
+	for _, file := range files {
+		result = append(result, path.Join(dir, file.Name()))


I'd exclude hidden files.

bgrant0607 · 2017-01-10T17:19:43Z

pkg/kubectl/cmd/config/config.go

@@ -44,7 +44,8 @@ func NewCmdConfig(pathOptions *clientcmd.PathOptions, out, errOut io.Writer) *co

 			1. If the --` + pathOptions.ExplicitFileFlag + ` flag is set, then only that file is loaded.  The flag may only be set once and no merging takes place.
 			2. If $` + pathOptions.EnvVar + ` environment variable is set, then it is used a list of paths (normal path delimitting rules for your system).  These paths are merged.  When a value is modified, it is modified in the file that defines the stanza.  When a value is created, it is created in the first file that exists.  If no files in the chain exist, then it creates the last file in the list.
-			3. Otherwise, ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + ` is used and no merging takes place.`),
+			3. Otherwise, ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + ` is used.
+			4. If ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + `.d exists, any files in that directory are also merged into the config.`),


The order of the files is significant, so please specify what the order is.

liggitt · 2017-01-10T19:38:53Z

pkg/kubectl/cmd/config/config.go

@@ -44,7 +44,8 @@ func NewCmdConfig(pathOptions *clientcmd.PathOptions, out, errOut io.Writer) *co

 			1. If the --` + pathOptions.ExplicitFileFlag + ` flag is set, then only that file is loaded.  The flag may only be set once and no merging takes place.
 			2. If $` + pathOptions.EnvVar + ` environment variable is set, then it is used a list of paths (normal path delimitting rules for your system).  These paths are merged.  When a value is modified, it is modified in the file that defines the stanza.  When a value is created, it is created in the first file that exists.  If no files in the chain exist, then it creates the last file in the list.
-			3. Otherwise, ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + ` is used and no merging takes place.`),
+			3. Otherwise, ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + ` is used.
+			4. If ` + path.Join("${HOME}", pathOptions.GlobalFileSubpath) + `.d exists, any files in that directory are also merged into the config.`),


is step 4 exclusive to step 3 (or any of the other steps)?

Clarified. (and yes)

deads2k · 2017-02-13T13:39:53Z

A couple comments on precendence, then lgtm

fabianofranz · 2017-02-17T19:44:26Z

@k8s-bot test this

k8s-github-robot · 2017-02-21T08:08:20Z

@brendandburns PR needs rebase

k8s-github-robot · 2017-05-18T22:12:14Z

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @brendandburns @deads2k @smarterclayton

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

robszumski · 2017-06-21T03:57:42Z

As @justinsb there have been a number of requests for this function, it was implemented two days after the issue was filed, but it's been sitting in code review for nearly 2 months. I'd really like to get it submitted.

Super :( :( any chance we can revive this?

k8s-ci-robot · 2017-06-21T04:27:20Z

@brendandburns: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
Jenkins unit/integration	`3c3c50f`	link	`@k8s-bot unit test this`
Jenkins kops AWS e2e	`3c3c50f`	link	`@k8s-bot kops aws e2e test this`
Jenkins GCI GCE e2e	`3c3c50f`	link	`@k8s-bot gci gce e2e test this`
Jenkins non-CRI GCE e2e	`3c3c50f`	link	`@k8s-bot non-cri e2e test this`
Jenkins GCE etcd3 e2e	`3c3c50f`	link	`@k8s-bot gce etcd3 e2e test this`
Jenkins GCE e2e	`3c3c50f`	link	`@k8s-bot cvm gce e2e test this`
Jenkins GKE smoke e2e	`3c3c50f`	link	`@k8s-bot cvm gke e2e test this`
pull-kubernetes-bazel	`3c3c50f`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-kubemark-e2e-gce	`3c3c50f`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-verify	`3c3c50f`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-gce-etcd3	`3c3c50f`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-e2e-kops-aws	`3c3c50f`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-federation-e2e-gce	`3c3c50f`	link	`/test pull-kubernetes-federation-e2e-gce`
pull-kubernetes-node-e2e	`3c3c50f`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-unit	`3c3c50f`	link	`/test pull-kubernetes-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

deads2k · 2017-06-21T12:20:29Z

Super :( :( any chance we can revive this?

Review comments from February still stand.

justinsb · 2017-06-21T15:43:17Z

@deads2k What does openshift do here? How does a user "add" a cluster?

robszumski · 2017-06-21T16:11:56Z

I'll throw out that Tectonic generates kubeconfigs from the Console that are scoped to just that cluster. It would be great UX to tells users to throw that in a config and voila they can switch to that context.

Merging two configs together is possible of course, but it's complex and you end up staring at a huge kubectl | jq | whatever mess. Mixing work and pleasure creds is also not idea.

liggitt · 2017-06-21T16:22:46Z

@deads2k What does openshift do here? How does a user "add" a cluster?

oc login https://myserver

Merging two configs together is possible of course, but it's complex and you end up staring at a huge kubectl | jq | whatever mess. Mixing work and pleasure creds is also not idea.

I agree. Combining files in the same directory as this PR does is the equivalent of merging files together automatically... you have to make sure they are non-overlapping and coherent.

bgrant0607 · 2017-08-11T19:07:21Z

cc @csbell

cblecker · 2018-02-10T17:54:50Z

Is this PR still active? There doesn't seem to be meaningful action in over 8 months.
I'm going to remove the lifecycle/frozen status and allow this to close in ~120 days if there is no further action.

/remove-lifecycle frozen

fejta-bot · 2018-05-11T18:32:51Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-06-10T19:19:23Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot · 2018-07-10T20:05:11Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 16, 2016

k8s-github-robot assigned janetkuo Dec 16, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Dec 16, 2016

justinsb reviewed Dec 16, 2016

View reviewed changes

bgrant0607 assigned fabianofranz and unassigned janetkuo Dec 16, 2016

bgrant0607 assigned deads2k and unassigned fabianofranz Jan 10, 2017

bgrant0607 mentioned this pull request Jan 10, 2017

By default, merge more .kubeconfig files #9298

Open

bgrant0607 suggested changes Jan 10, 2017

View reviewed changes

bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jan 10, 2017

liggitt reviewed Jan 10, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017

brendandburns force-pushed the config.d branch 2 times, most recently from a29cbd3 to 53240ea Compare January 15, 2017 05:57

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2017

k8s-github-robot closed this May 18, 2017

luxas mentioned this pull request May 29, 2017

kubeadm: improve quickstart instructions #44125

Merged

kongslund mentioned this pull request Jun 21, 2017

It should be easier to configure kubectl to work with multiple tectonic clusters coreos/tectonic-forum#110

Open

justinsb added the keep-open label Jun 21, 2017

justinsb reopened this Jun 21, 2017

fabianofranz mentioned this pull request Jul 14, 2017

Adds merge command #47730

Closed

k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed keep-open labels Dec 15, 2017

k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Feb 10, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 11, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 10, 2018

k8s-ci-robot closed this Jul 10, 2018

bgrant0607 mentioned this pull request Aug 12, 2019

Feature Request: Support $HOME/.kube/config.d/* kubernetes/kubectl#569

Open

Add optional loading from a config.d directory to kubectl. #38861

Add optional loading from a config.d directory to kubectl. #38861

Conversation

brendandburns commented Dec 16, 2016 • edited by k8s-github-robot Loading

k8s-reviewable commented Dec 16, 2016

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Dec 16, 2016

deads2k commented Dec 16, 2016

smarterclayton commented Dec 16, 2016

brendandburns commented Dec 17, 2016

brendandburns commented Dec 17, 2016

bgrant0607 commented Dec 20, 2016

brendandburns commented Dec 20, 2016 • edited Loading

brendandburns commented Jan 10, 2017

bgrant0607 commented Jan 10, 2017

smarterclayton commented Jan 10, 2017

smarterclayton commented Jan 10, 2017

bgrant0607 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 13, 2017

fabianofranz commented Feb 17, 2017

k8s-github-robot commented Feb 21, 2017

k8s-github-robot commented May 18, 2017

robszumski commented Jun 21, 2017

k8s-ci-robot commented Jun 21, 2017

deads2k commented Jun 21, 2017

justinsb commented Jun 21, 2017

robszumski commented Jun 21, 2017

liggitt commented Jun 21, 2017

bgrant0607 commented Aug 11, 2017

cblecker commented Feb 10, 2018

fejta-bot commented May 11, 2018

fejta-bot commented Jun 10, 2018

fejta-bot commented Jul 10, 2018

brendandburns commented Dec 16, 2016 •

edited by k8s-github-robot

Loading

brendandburns commented Dec 20, 2016 •

edited

Loading