kubeadm-init: add --copy-credentials-for-user #55901

neolit123 · 2017-11-16T20:02:52Z

What this PR does / why we need it:

Using this new parameter the administrator's credentials
for the cluster will be copied to the specified
user's home directory.

WARNING: existing files will be overwritten.

Usage:
kubeadm init --copy-credentials-for-user=

It automates a step that the user needs to perform either manually each time or by scripting. If the flag is not set the old behavior is preserved - i.e. show info how / where to copy the config.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #235
Fixes kubernetes/kubeadm#416

Special notes for your reviewer:

Release note:

Add `--copy-credentials-for-user` to `kubeadm init`. The flag can be used to automatically copy the administrator's credentials to the specified user's home directory.

neolit123 · 2017-11-16T20:03:12Z

/area kubeadm

dims · 2017-11-16T21:09:20Z

/ok-to-test

neolit123 · 2017-11-21T17:51:44Z

@timothysc i was trying to speak about this during the SIG meeting today, but looks like my MIC decided to not work for some reason. :(

timothysc

I'm generally ok with it, but we should talk with @luxas about what he thinks because it's so late in the cycle.

timothysc · 2017-11-21T21:56:53Z

cmd/kubeadm/app/cmd/init.go

+	} else {
+		ctx["KubeConfigInfo"] = ""
+		if err := copyConfigForDefaultUser(i.defaultUser, adminKubeConfigPath); err != nil {
+			return fmt.Errorf("error copying configuration for user %q: %v", i.defaultUser, err)


Shouldn't we still have the previous instructions for a failure?

seems like the correct thing to do. i will amend the commit.

neolit123 · 2017-11-22T01:28:01Z

@timothysc @luxas
updated.

timothysc

/lgtm - to get you in before the freeze, but definitely redux the logic per-comments b4 approve.

timothysc · 2017-11-22T15:43:46Z

cmd/kubeadm/app/cmd/init.go

+
+	if _, err := os.Stat(kubeDir); err != nil {
+		if os.IsNotExist(err) {
+			err = os.Mkdir(kubeDir, 0700)


You could probably just make the dir and check ignore the error if it exists.

ok, makes sense.

timothysc · 2017-11-22T15:46:03Z

cmd/kubeadm/app/cmd/init.go

+	}
+	defer dest.Close()
+
+	_, err = io.Copy(src, dest)


I don't see why you couldn't just copy vs. open.

could you please clarify this comment.

@timothysc TMK, golang does not provide means to copy files on os level with a single command. thus, one has to os.Open() and use io.Copy().

issue: reversed src / dest in the recent amend!

neolit123 · 2018-01-19T23:46:12Z

should i include the above change in my commit?

@neolit123 - yes

@timothysc ok, done.

timothysc · 2018-01-31T02:12:28Z

/retest

fabriziopandini

@neolit123 , if we are extending how kubeconfig file are created in kubeadm init, IMO it is necessary to ensure a consistent behaviour also in kubeadm alpha phase kubeconfig. WDYT?

neolit123 · 2018-02-07T14:00:00Z

@fabianofranz hi,
i'm not sure i understand how to relate --copy-credentials-for-user with kubeadm alpha phase kubeconfig. could you please elaborate?

perhaps you mean to allow alpha phase to generate directly in a user folder in the lines of "generate config files for this user" (e.g. add new flag --generate-config-for-user)?

k8s-ci-robot · 2018-02-23T18:47:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [timothysc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

neolit123 · 2018-02-23T20:13:15Z

got the bazel tests to pass but the uni test now spills a pile of warnings / errors that seem unrelated.
could someone please explain the situation, please?

neolit123 · 2018-02-24T12:29:34Z

/test pull-kubernetes-unit

k8s-github-robot · 2018-03-02T18:47:08Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@luxas @neolit123 @timothysc

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.10 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

timothysc · 2018-04-10T14:10:59Z

cmd/kubeadm/app/cmd/platform/init_unix.go

+
+	usr, err = user.Lookup(copyCredentialsForUser)
+	if err != nil {
+		return fmt.Errorf(InitErrorNoSuchUser)


I'm not a fan of recasting the errors here. Could you use the errors.Wrap utility if you want to put your output change on it.

ok, i can rebase this next Monday (AFK this week).

updated to always include the relevant error in the Errorf() format.

timothysc · 2018-04-10T14:12:57Z

cmd/kubeadm/app/cmd/platform/init_windows.go

+)
+
+// InitCopyCredentialsForUser will copy the cluster admin credentials to the home path of a non-root user
+func InitCopyCredentialsForUser(copyCredentialsForUser string, adminKubeConfigPath string) error {


Why can you use ENV-var's or some other platform agnostic library to determine $HOMEDIR.

/cc @kubernetes/sig-windows-misc

this targets any Windows user. the env. variable %HOME% can only be used for the current user. also some user might have decided to move his home dir outside of default user path on Windows - e.g. /users/.

cc @luxas
some related changes of mine got accepted in the user package of the Go standard libarary...
https://github.com/golang/go/commits?author=neolit123

^ these are going to make it in go 1.11 in August i would guess.

I'm generally not ok with this change todo the last step which folks automate to add this platform specific code which can/will be obviated.

even with my contribs to the user package in the go std lib that can eventually replace the above code, this change would still need the _unix, _windows files, because:

there is no chown on windows

the way to check if a user is root/admin differs between platforms

Using this new parameter the administrator's credentials for the cluster will be copied to the specified user's home directory. WARNING: existing files will be overwritten. Usage: kubeadm init --copy-credentials-for-user=<someuser> Windows requires a different approach for copying the credentials than Unix, thus a new package is added - `platform` in `kubeadm/app/cmd`. It contains `init_unix.go` and `init_windows.go` that define the exported function - InitCopyCredentialsForUser(). The function works differently on Unix and Windows. The same package can be re-used for other multiplatform code.

k8s-ci-robot · 2018-04-18T15:15:51Z

@neolit123: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`f67a150`	link	`/test pull-kubernetes-e2e-gce`

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.

neolit123 · 2018-04-18T15:26:56Z

/test pull-kubernetes-verify

timothysc · 2018-04-20T22:18:12Z

I'm going to close this one per comment kubernetes/kubeadm#416 (comment) .

Also, due to the other dependencies that we would drag in. If you feel like we can do this and trim it down feel free to ping me on slack.

neolit123 · 2018-04-20T23:06:17Z

i'm perfectly fine with the close in the aspect of this not being a cmd flag. having this in the configuration is better.
in terms of actually installing the kubeconfig for a certain user i don't think this can go without the os/user package and without a small OS abstraction layer.

k8s-ci-robot added the area/kubeadm label Nov 16, 2017

k8s-github-robot assigned timothysc and luxas Nov 16, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 16, 2017

neolit123 force-pushed the config branch 2 times, most recently from 71139ff to 2956298 Compare November 17, 2017 02:31

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 17, 2017

neolit123 force-pushed the config branch 2 times, most recently from cb8da28 to f0f8d7f Compare November 18, 2017 18:18

timothysc added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Nov 21, 2017

timothysc suggested changes Nov 21, 2017

View reviewed changes

neolit123 force-pushed the config branch 3 times, most recently from 3369144 to d3d0843 Compare November 22, 2017 00:04

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

neolit123 force-pushed the config branch from d3d0843 to 14c20d9 Compare November 22, 2017 14:00

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

timothysc reviewed Nov 22, 2017

View reviewed changes

timothysc added this to the v1.9 milestone Nov 22, 2017

timothysc added status/approved-for-milestone lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 22, 2017

idvoretskyi mentioned this pull request Feb 3, 2018

Dramatically Simplify Kubernetes Cluster Creation (kubeadm umbrella issue). kubernetes/enhancements#11

Closed

18 tasks

fabriziopandini reviewed Feb 7, 2018

View reviewed changes

neolit123 force-pushed the config branch from d56decd to 4bbd1f2 Compare February 23, 2018 18:47

k8s-github-robot added the milestone/incomplete-labels label Mar 2, 2018

timothysc removed this from the v1.10 milestone Mar 2, 2018

neolit123 force-pushed the config branch 4 times, most recently from 73921ce to b6d6dc5 Compare March 24, 2018 14:25

timothysc mentioned this pull request Apr 7, 2018

Ability to configure user and cluster name in AdminKubeConfigFile kubernetes/kubeadm#416

Closed

timothysc removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2018

timothysc changed the title ~~[WIP] kubeadm-init: add --copy-credentials-for-user~~ kubeadm-init: add --copy-credentials-for-user Apr 7, 2018

timothysc suggested changes Apr 10, 2018

View reviewed changes

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Apr 10, 2018

neolit123 force-pushed the config branch from b6d6dc5 to 62672c3 Compare April 17, 2018 13:55

neolit123 force-pushed the config branch from 62672c3 to f67a150 Compare April 18, 2018 14:59

timothysc removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2018

timothysc closed this Apr 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm-init: add --copy-credentials-for-user #55901

kubeadm-init: add --copy-credentials-for-user #55901

neolit123 commented Nov 16, 2017 •

edited by timothysc

neolit123 commented Nov 16, 2017

dims commented Nov 16, 2017

neolit123 commented Nov 21, 2017

timothysc left a comment

timothysc Nov 21, 2017

neolit123 Nov 21, 2017

neolit123 commented Nov 22, 2017

timothysc left a comment

timothysc Nov 22, 2017

neolit123 Nov 22, 2017

timothysc Nov 22, 2017

neolit123 Nov 22, 2017

neolit123 Nov 22, 2017

neolit123 Nov 22, 2017

neolit123 commented Jan 19, 2018

timothysc commented Jan 31, 2018

fabriziopandini left a comment

neolit123 commented Feb 7, 2018

k8s-ci-robot commented Feb 23, 2018

neolit123 commented Feb 23, 2018

neolit123 commented Feb 24, 2018

k8s-github-robot commented Mar 2, 2018

timothysc Apr 10, 2018

neolit123 Apr 11, 2018

neolit123 Apr 17, 2018

timothysc Apr 10, 2018

neolit123 Apr 11, 2018 •

edited

neolit123 Apr 11, 2018

timothysc Apr 18, 2018

neolit123 Apr 18, 2018

k8s-ci-robot commented Apr 18, 2018 •

edited

neolit123 commented Apr 18, 2018

timothysc commented Apr 20, 2018

neolit123 commented Apr 20, 2018

kubeadm-init: add --copy-credentials-for-user #55901

kubeadm-init: add --copy-credentials-for-user #55901

Conversation

neolit123 commented Nov 16, 2017 • edited by timothysc

neolit123 commented Nov 16, 2017

dims commented Nov 16, 2017

neolit123 commented Nov 21, 2017

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Nov 22, 2017

timothysc 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

neolit123 commented Jan 19, 2018

timothysc commented Jan 31, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 7, 2018

k8s-ci-robot commented Feb 23, 2018

neolit123 commented Feb 23, 2018

neolit123 commented Feb 24, 2018

k8s-github-robot commented Mar 2, 2018

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

neolit123 Apr 11, 2018 • edited

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

k8s-ci-robot commented Apr 18, 2018 • edited

neolit123 commented Apr 18, 2018

timothysc commented Apr 20, 2018

neolit123 commented Apr 20, 2018

neolit123 commented Nov 16, 2017 •

edited by timothysc

neolit123 Apr 11, 2018 •

edited

k8s-ci-robot commented Apr 18, 2018 •

edited