Specify user on export kubecfg #9280

olemarkus · 2020-06-05T06:35:52Z

Don't export admin user by default.
Allow specifying existing user when exporting context.

cmd/kops/export_kubecfg.go

rifelpet · 2020-06-05T11:24:11Z

cmd/kops/export_kubecfg.go

@@ -71,6 +73,8 @@ func NewCmdExportKubecfg(f *util.Factory, out io.Writer) *cobra.Command {

 	cmd.Flags().StringVar(&options.KubeConfigPath, "kubeconfig", options.KubeConfigPath, "The location of the kubeconfig file to create.")
 	cmd.Flags().BoolVar(&options.all, "all", options.all, "export all clusters from the kops state store")
+	cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "also export the admin user")


Suggested change

cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "also export the admin user")

cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "export the admin user")

Suggested change

cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "also export the admin user")

cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "export the admin user and use in the cluster context")

maybe one of these? I'll keep thinking of how this could be worded...

"include the admin user and credentials"

olemarkus · 2020-06-05T12:03:14Z

Thanks for looking at this @rifelpet . The current staet is pretty much a brain dump of how I want this to work. It is in no way close to merge :) I just wanted some comments that more people agree to this approach. The final solution will probably need a bit of refactor etc, so I didn't want to do too much up front :)

Should have said so initially.Apologies.

cmd/kops/export_kubecfg.go

johngmyers · 2020-06-06T03:21:54Z

cmd/kops/export_kubecfg.go

@@ -71,6 +73,8 @@ func NewCmdExportKubecfg(f *util.Factory, out io.Writer) *cobra.Command {

 	cmd.Flags().StringVar(&options.KubeConfigPath, "kubeconfig", options.KubeConfigPath, "The location of the kubeconfig file to create.")
 	cmd.Flags().BoolVar(&options.all, "all", options.all, "export all clusters from the kops state store")
+	cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "also export the admin user")


"include the admin user and credentials"

johngmyers · 2020-06-06T03:27:28Z

pkg/kubeconfig/create_kubecfg.go

-		}
-		if cert != nil {
-			b.CACert, err = cert.AsBytes()
+	if admin {


Why omit the certificate-authority-data when admin is false?

Yeah. That wasn't intentional. Fixed.

johngmyers · 2020-06-06T03:30:48Z

pkg/kubeconfig/create_kubecfg_test.go

@@ -225,7 +225,7 @@ func TestBuildKubecfg(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := BuildKubecfg(tt.args.cluster, tt.args.keyStore, tt.args.secretStore, tt.args.status, tt.args.configAccess)
+			got, err := BuildKubecfg(tt.args.cluster, tt.args.keyStore, tt.args.secretStore, tt.args.status, tt.args.configAccess, true)


Should add a test for false

And fixed (which caught the above-mentioned error)

johngmyers · 2020-06-06T03:43:27Z

pkg/kubeconfig/kubecfg_builder.go


 		if config.Clusters == nil {
 			config.Clusters = make(map[string]*clientcmdapi.Cluster)
 		}
 		config.Clusters[b.Context] = cluster
 	}

-	{
+	if b.KubeUser == "admin" {


Why only write out the client cert if there is a plaintext password?

It is a bit of a hack. I changed the check to something matching kops assumptions on user names.

pkg/kubeconfig/kubecfg_builder.go

cmd/kops/export_kubecfg.go

cmd/kops/update_cluster.go

johngmyers · 2020-06-08T03:58:00Z

cmd/kops/update_cluster.go

+	}
+
+	if c.CreateKubecfg && !c.admin && c.user == "" {
+		return nil, fmt.Errorf("--create-kube-config requires that either --admin or --user is set")


To some extent code related. It is not straightforward in the current code to see if there is already a context for the cluster early in the update code. I didn't want to let the user run update to provision a new cluster, only for the command to fail at the very end when exporting the user and then complain about --admin or --user not being set.

So we assume that the user only adds if they want to add the new context --create-kube-config. That is also why I defaulted this flag to false rather than true now since usually one only wants this if one runs update to create the cloud resources the first time.

The other use case I can think of is when one rotates certs, and at least if one uses the admin user, one would need to add --admin anyway.

I believe it makes sense to export a context for the cluster without either adding admin credentials or setting the context to a precreated singular user.

Our authentication setup has per-cluster tokens which are obtained by an exec hook. That means a single user shared across multiple clusters would not work.

I am a bit concerned that if one sets --create-kube-config, but does not catch that one probably should also set --admin or --user one will get a broken config. So I do agree that there are use cases for not doing this, I am wondering if this is worth it usabilitywise.

For your use case, you have to remove the user from the context once and then kops will at least leave the context alone and not keep re-adding the admin user, which is a good benefit on its own I should think. I assume that typically you would only export the cluster config once per operator per cluster so this isn't too much hassle.

Since --admin and --user imply --create-kube-config it is unlikely that --create-kube-config would be explicitly set unless the invoker is doing something special.

Our wrapper around kops has two forms of export kubecfg, one which corresponds to --admin and one which adds a per-cluster user with our local exec hook added. Since the admin credential is so powerful, I would prefer not to have that second case extract the credential, write it to disk, and then try to remove it.

I suppose I could let the current PR through and then address this case in a followup PR. I am planning on reducing the default lifetime of the admin credential to 18 hours.

I think that would be helpful. You have a much better idea of how this should work for your use case than I do.

I do agree that passing only --create-kube-config would be something special. If you log a warning about creating a context without user, I think that should be fine. It would at least give a hint to the user about why the credentials aren't working.

johngmyers · 2020-06-08T03:59:08Z

cmd/kops/update_cluster.go

+
+	if c.user != "" && !c.CreateKubecfg {
+		return nil, fmt.Errorf("--user requires that --create-kube-config is set")
+	}


Perhaps make either --admin or --user imply --create-kube-config?

That is a good idea! I'll give that a go.

The logging probably isn't necessary.

I think they could be helpful and I guess common for CLI tools. But we also do document that it is implied. If you think they should go, I can remove it.

I think most users aren't going to keep --create-kube-config in mind. They're going to know either --admin or --user, whichever fits their use case. So I think this would just be log chatter.

olemarkus · 2020-06-08T13:10:42Z

The error from the AWS job is expected since it is not setting the --admin user on kops update. Would be easier if it just ran kops create cluster --yes.

I am strongly for requiring the --admin user flag which means either fixing test-infra code or the flags we set on the jobs (can we set flags on the update command?).

rifelpet · 2020-06-08T13:16:35Z

kubetest currently only adds args to create cluster: https://github.com/kubernetes/test-infra/blob/4572dc3bf92e70f572e55e7ac1be643bdf6b2566/kubetest/kops.go#L465-L470

olemarkus · 2020-06-08T13:55:02Z

If you agree with me, I can look at a PR to kubetest to just add the admin flag

rifelpet · 2020-06-08T20:58:22Z

hm it'd be great if we could make the transition a bit smoother. I realize this is a breaking change regardless of how we implement it, but maybe we could do something like add the new cli flags in release-1.18 with defaults to the existing behavior, and then change the defaults in master. This way kubetest could test both master and release-1.18 kops builds. Thoughts?

olemarkus · 2020-06-09T06:41:34Z

We'd have to go further back than 1.18, wouldn't we? If we don't change kubetest, we'd have to backport noops of these flags as far back as we want to do any patch releases.

olemarkus · 2020-06-09T08:13:49Z

kops create cluster --yes will default to --admin. So the easiest way to solve this is to just add --yes to --kops-args in the test grid. Test infra will do a redundant kops update after, but I can do a PR to get that command removed eventually.

olemarkus · 2020-06-09T17:32:14Z

When kubernetes/test-infra#17890 goes in, this should pass.

olemarkus · 2020-06-16T16:51:40Z

/retest

johngmyers · 2020-06-16T20:16:17Z

cmd/kops/create_cluster.go

@@ -1376,7 +1374,7 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr
 				fmt.Fprintf(&sb, " * edit your master instance group: kops edit ig --name=%s %s\n", clusterName, masters[0].ObjectMeta.Name)
 			}
 			fmt.Fprintf(&sb, "\n")
-			fmt.Fprintf(&sb, "Finally configure your cluster with: kops update cluster --name %s --yes\n", clusterName)
+			fmt.Fprintf(&sb, "Finally configure your cluster with: kops update cluster --name %s --yes --admin --create-kube-config\n", clusterName)


Perhaps leave off the --create-kube-config since it's implied?

cmd/kops/export_kubecfg.go

cmd/kops/update_cluster.go

johngmyers · 2020-06-16T20:26:43Z

cmd/kops/update_cluster.go

+
+	if c.user != "" && !c.CreateKubecfg {
+		return nil, fmt.Errorf("--user requires that --create-kube-config is set")
+	}


The logging probably isn't necessary.

docs/cli/kops_export_kubecfg.md

olemarkus · 2020-06-17T19:30:20Z

Looks like tests are passing and all now!

/assign @rifelpet

k8s-ci-robot · 2020-06-17T19:30:21Z

@olemarkus: GitHub didn't allow me to assign the following users: rifelpe.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Looks like tests are passing and all now!

/assign @rifelpe

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.

olemarkus · 2020-06-17T19:31:45Z

/assign @rifelpet

cmd/kops/update_cluster.go

johngmyers · 2020-06-18T05:44:57Z

cmd/kops/update_cluster.go

+				conf.User = cluster.ObjectMeta.Name
+			} else {
+				conf.User = c.user
+			}


I believe this logic should be pushed down into kubeconfig.BuildKubecfg().

Not sure why I did not do this earlier. Much better and the logic is now tested properly too.

johngmyers · 2020-06-18T05:49:31Z

cmd/kops/update_cluster.go

 			if err != nil {
 				return nil, err
 			}
+
+			if c.admin {


Suggested change

if c.admin {

if c.user == "" {

johngmyers · 2020-06-20T04:59:37Z

Review comments on create_cluster.go and update_cluster.go still unaddressed.

docs/releases/1.19-NOTES.md

johngmyers · 2020-06-21T17:58:43Z

/lgtm
Could remove the log chatter either now or in a followup PR.

…en exporting context

johngmyers · 2020-06-24T20:21:42Z

/lgtm

rifelpet · 2020-06-28T14:31:15Z

this lgtm as well but since it's a breaking change and rather user-facing, i'd prefer if justin gives the final approval. if it doesnt get merged before next office hours we can discuss it then.

/lgtm
/assign @justinsb

rifelpet · 2020-07-17T17:20:39Z

No objections during office hours
/approve

k8s-ci-robot · 2020-07-17T17:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

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:

~~OWNERS~~ [rifelpet]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2020

k8s-ci-robot requested review from KashifSaadat and mikesplain June 5, 2020 06:36

olemarkus mentioned this pull request Jun 5, 2020

Environment variable to control exported user #8017

Closed

olemarkus force-pushed the no-admin branch from 1e3116c to c1dc4a1 Compare June 5, 2020 07:15

k8s-ci-robot added the area/documentation label Jun 5, 2020

rifelpet reviewed Jun 5, 2020

View reviewed changes

johngmyers requested changes Jun 6, 2020

View reviewed changes

johngmyers requested changes Jun 8, 2020

View reviewed changes

olemarkus changed the title ~~WIP: Specify user on export kubecfg~~ Specify user on export kubecfg Jun 8, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2020

olemarkus force-pushed the no-admin branch from bef7c22 to 4d7ab9c Compare June 8, 2020 20:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2020

johngmyers requested changes Jun 16, 2020

View reviewed changes

olemarkus force-pushed the no-admin branch 3 times, most recently from a041557 to 31a19db Compare June 17, 2020 18:33

k8s-ci-robot assigned rifelpet Jun 17, 2020

johngmyers requested changes Jun 18, 2020

View reviewed changes

johngmyers reviewed Jun 21, 2020

View reviewed changes

docs/releases/1.19-NOTES.md Outdated Show resolved Hide resolved

docs/releases/1.19-NOTES.md Outdated Show resolved Hide resolved

docs/releases/1.19-NOTES.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned johngmyers Jun 21, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 21, 2020

Don't export admin user by default. Allow specifying existing user wh…

72fd007

…en exporting context

olemarkus force-pushed the no-admin branch from 8c3e0c5 to 72fd007 Compare June 24, 2020 17:54

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 24, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020

k8s-ci-robot assigned justinsb Jun 28, 2020

olemarkus mentioned this pull request Jul 1, 2020

Control --create-kube-config with an environment variable or invert its default #9466

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2020

k8s-ci-robot merged commit 6f3c067 into kubernetes:master Jul 17, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 17, 2020

hakman mentioned this pull request Jul 19, 2020

Kops - Export the "admin" user on cluster update kubernetes/test-infra#18373

Closed

	cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "also export the admin user")
	cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "export the admin user")

Specify user on export kubecfg #9280

Specify user on export kubecfg #9280

Conversation

olemarkus commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Jun 5, 2020

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

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

olemarkus commented Jun 8, 2020

rifelpet commented Jun 8, 2020

olemarkus commented Jun 8, 2020

rifelpet commented Jun 8, 2020

olemarkus commented Jun 9, 2020

olemarkus commented Jun 9, 2020

olemarkus commented Jun 9, 2020

olemarkus commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Jun 17, 2020 • edited Loading

k8s-ci-robot commented Jun 17, 2020

olemarkus commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Jun 20, 2020

johngmyers commented Jun 21, 2020

johngmyers commented Jun 24, 2020

rifelpet commented Jun 28, 2020

rifelpet commented Jul 17, 2020

k8s-ci-robot commented Jul 17, 2020

olemarkus commented Jun 17, 2020 •

edited

Loading