kubeadm: make "alpha kubeconfig user" accept --config #94879

knight42 · 2020-09-18T08:02:22Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: make the command "kubeadm alpha kubeconfig user" accept a "--config" flag and remove the following flags:
- apiserver-advertise-address / apiserver-bind-port: use either localAPIEndpoint from InitConfiguration or controlPlaneEndpoint from ClusterConfiguration.
- cluster-name: use clusterName from ClusterConfiguration
- cert-dir: use certificatesDir from ClusterConfiguration

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

knight42 · 2020-09-18T08:04:19Z

/assign @neolit123
/priority important-soon

knight42 · 2020-09-18T08:07:45Z

cmd/kubeadm/app/cmd/alpha/kubeconfig.go

-	// Add InitConfiguration backed flags to the command
-	cmd.Flags().StringVar(&initCfg.LocalAPIEndpoint.AdvertiseAddress, options.APIServerAdvertiseAddress, initCfg.LocalAPIEndpoint.AdvertiseAddress, "The IP address the API server is accessible on")
-	cmd.Flags().Int32Var(&initCfg.LocalAPIEndpoint.BindPort, options.APIServerBindPort, initCfg.LocalAPIEndpoint.BindPort, "The port the API server is accessible on")
+	options.AddConfigFlag(cmd.Flags(), &cfgPath)


@neolit123 Currently this command only accept local kubeadm config, should we also consider the config saved in cluster?

thanks for the PR, i can have a deeper look next week.

that is a good question. we had previous discussions whether certain kubeadm commands should use the kubeadm-config CM. my take is that if a command has potential to not operate on the running cluster it should not fetch the CM.

for example, if the command is currently being run on a Node part of a cluster and if a kubeconfig to access the cluster is in the user home dir, the user might still want to create a kubeadm for a completely different cluster, but the command will use the configuration from the cluster, which might not be desired.

i had the same argument for the command "kubeadm config images...". "kubeadm alpha certs renew" on the other hand has CM fetch, but the idea of the command is to renew certs for this Node / cluster.

so my preference here would be the following:

the user should feed --config and the command should fail without it.

if the user wants to feed the --config for the current cluster they can fetch the ClusterConfiguration from the CM.

I got it, thanks for your detailed clarification!

knight42 · 2020-09-18T08:49:05Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

neolit123 · 2020-09-21T17:49:01Z

doing a minor retitle:

/retitle kubeadm: make "alpha kubeconfig user" accept --config

neolit123 · 2020-09-21T17:54:20Z

please change the release note to:

kubeadm: make the command "kubeadm alpha kubeconfig user" accept a "--config" flag and remove the following flags:
- apiserver-advertise-address / apiserver-bind-port: use either localAPIEndpoint from InitConfiguration or controlPlaneEndpoint from ClusterConfiguration.
- cluster-name: use clusterName from ClusterConfiguration
- cert-dir: use certificatesDir from ClusterConfiguration

neolit123 · 2020-09-21T17:55:17Z

cmd/kubeadm/app/cmd/alpha/kubeconfig.go


 	// Add command specific flags
 	cmd.Flags().StringVar(&token, options.TokenStr, token, "The token that should be used as the authentication mechanism for this kubeconfig, instead of client certificates")
 	cmd.Flags().StringVar(&clientName, "client-name", clientName, "The name of user. It will be used as the CN if client certificates are created")
 	cmd.Flags().StringSliceVar(&organizations, "org", organizations, "The orgnizations of the client certificate. It will be used as the O if client certificates are created")

+	_ = cmd.MarkFlagRequired(options.CfgPath)
+	_ = cmd.MarkFlagRequired("client-name")


these can be without the leading _ =, unless linters complain?

yeah just want to make goland happy.

neolit123 · 2020-09-21T17:56:35Z

cmd/kubeadm/app/cmd/alpha/kubeconfig.go

@@ -79,36 +75,35 @@ func newCmdUserKubeConfig(out io.Writer) *cobra.Command {
 			if clientName == "" {
 				return errors.New("missing required argument --client-name")
 			}
+			if cfgPath == "" {
+				return errors.New("missing required argument --config")
+			}


we can remove the above if checks if cmd.MarkFlagRequired is used.

neolit123 · 2020-09-21T17:57:12Z

cmd/kubeadm/app/cmd/alpha/kubeconfig_test.go

+		},
+		NodeRegistration: kubeadmapiv1beta2.NodeRegistrationOptions{
+			CRISocket: "/path/to/dockershim.sock",
+		},


i think the NodeRegistration above can be removed

neolit123 · 2020-09-21T18:00:08Z

cmd/kubeadm/app/cmd/alpha/kubeconfig_test.go

 	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
 	"k8s.io/kubernetes/cmd/kubeadm/app/util/pkiutil"
 	testutil "k8s.io/kubernetes/cmd/kubeadm/test"
 	kubeconfigtestutil "k8s.io/kubernetes/cmd/kubeadm/test/kubeconfig"
 )

+func generateKubeadmConfig(dir, id, certDir, clusterName string) (string, error) {


possibly best to indicate this is used for tests (package scope wise) - e.g. name the function generateTestKubeadmConfig

neolit123 · 2020-09-21T18:03:13Z

cmd/kubeadm/app/cmd/alpha/kubeconfig_test.go

+	}
+	buf.Write(data)
+
+	err = ioutil.WriteFile(cfgPath, buf.Bytes(), 0o644)


should be 0644 for consistency with the rest of kubeadm code.

neolit123 · 2020-09-21T18:04:22Z

cmd/kubeadm/app/cmd/alpha/kubeconfig_test.go

 	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
 	"k8s.io/kubernetes/cmd/kubeadm/app/util/pkiutil"
 	testutil "k8s.io/kubernetes/cmd/kubeadm/test"
 	kubeconfigtestutil "k8s.io/kubernetes/cmd/kubeadm/test/kubeconfig"
 )

+func generateKubeadmConfig(dir, id, certDir, clusterName string) (string, error) {
+	id = strings.ReplaceAll(id, " ", "_") + ".conf"


i think we can leave "id" here as is, without replacing " " and adding the extension - i.e. the file will be loaded even if has spaces and no ext.

cmd/kubeadm/app/cmd/alpha/kubeconfig.go

neolit123

@knight42 thanks for the PR, i've added some minor comments.
i think we can keep the commits squashed to 1 here.

LMK if you have any questions.

knight42 · 2020-09-22T05:33:56Z

/test pull-kubernetes-bazel-test

neolit123 · 2020-09-22T14:48:29Z

cmd/kubeadm/app/cmd/alpha/kubeconfig.go

@@ -40,7 +39,7 @@ var (

 	userKubeconfigExample = cmdutil.Examples(`
 	# Output a kubeconfig file for an additional user named foo


probably best to indicate that --config is the kubeadm configuration, as mentioned here:
https://github.com/kubernetes/kubernetes/pull/94879/files#r492257006

neolit123

added one final comment. i think this is good to go after that.
/approve

k8s-ci-robot · 2020-09-22T14:50:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [neolit123]

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

Signed-off-by: knight42 <anonymousknight96@gmail.com>

neolit123 · 2020-09-22T15:13:54Z

thank you @knight42
if someone objects to the change made here we can amend before 1.20 is released.
/lgtm
/milestone v1.20

knight42 force-pushed the refactor/kubeadm-alpha-kubeconfig branch from c46b7b5 to ae0959f Compare September 18, 2020 08:02

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2020

k8s-ci-robot assigned neolit123 Sep 18, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 18, 2020

k8s-ci-robot requested review from ereslibre and SataQiu September 18, 2020 08:04

knight42 commented Sep 18, 2020

View reviewed changes

k8s-ci-robot changed the title ~~refactor(kubeadm): make kubeconfig user command accpet kubeadm config~~ kubeadm: make "alpha kubeconfig user" accept --config Sep 21, 2020

neolit123 reviewed Sep 21, 2020

View reviewed changes

cmd/kubeadm/app/cmd/alpha/kubeconfig.go Show resolved Hide resolved

neolit123 reviewed Sep 21, 2020

View reviewed changes

neolit123 mentioned this pull request Sep 21, 2020

prepare "alpha kubeconfig user" for graduation kubernetes/kubeadm#2292

Closed

4 tasks

knight42 force-pushed the refactor/kubeadm-alpha-kubeconfig branch from ebcb128 to 90dd499 Compare September 22, 2020 03:45

knight42 requested a review from neolit123 September 22, 2020 03:46

neolit123 reviewed Sep 22, 2020

View reviewed changes

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

refactor(kubeadm): make alpha kubeconfig user command accpet --config

36eb74a

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 requested a review from neolit123 September 22, 2020 15:11

knight42 force-pushed the refactor/kubeadm-alpha-kubeconfig branch from 90dd499 to 36eb74a Compare September 22, 2020 15:11

k8s-ci-robot added this to the v1.20 milestone Sep 22, 2020

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

k8s-ci-robot merged commit 9f32854 into kubernetes:master Sep 22, 2020

knight42 deleted the refactor/kubeadm-alpha-kubeconfig branch September 22, 2020 16:17

wilsonehusin mentioned this pull request Dec 3, 2020

Add release notes JSON draft kubernetes/sig-release#1373

Merged

SataQiu mentioned this pull request Feb 27, 2023

kubeadm: modify '--config' flag from required to optional for 'kubeadm kubeconfig user' #116074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: make "alpha kubeconfig user" accept --config #94879

kubeadm: make "alpha kubeconfig user" accept --config #94879

knight42 commented Sep 18, 2020 •

edited

knight42 commented Sep 18, 2020

knight42 Sep 18, 2020

neolit123 Sep 19, 2020

knight42 Sep 20, 2020 •

edited

knight42 commented Sep 18, 2020

neolit123 commented Sep 21, 2020

neolit123 commented Sep 21, 2020

neolit123 Sep 21, 2020

knight42 Sep 22, 2020

neolit123 Sep 21, 2020

neolit123 Sep 21, 2020

neolit123 Sep 21, 2020 •

edited

neolit123 Sep 21, 2020

neolit123 Sep 21, 2020 •

edited

neolit123 left a comment

knight42 commented Sep 22, 2020

neolit123 Sep 22, 2020

neolit123 left a comment

k8s-ci-robot commented Sep 22, 2020

neolit123 commented Sep 22, 2020

		@@ -40,7 +39,7 @@ var (

		userKubeconfigExample = cmdutil.Examples(`
		# Output a kubeconfig file for an additional user named foo

kubeadm: make "alpha kubeconfig user" accept --config #94879

kubeadm: make "alpha kubeconfig user" accept --config #94879

Conversation

knight42 commented Sep 18, 2020 • edited

knight42 commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knight42 Sep 20, 2020 • edited

Choose a reason for hiding this comment

knight42 commented Sep 18, 2020

neolit123 commented Sep 21, 2020

neolit123 commented Sep 21, 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

neolit123 Sep 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Sep 21, 2020 • edited

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

knight42 commented Sep 22, 2020

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 22, 2020

neolit123 commented Sep 22, 2020

knight42 commented Sep 18, 2020 •

edited

knight42 Sep 20, 2020 •

edited

neolit123 Sep 21, 2020 •

edited

neolit123 Sep 21, 2020 •

edited