Add certificate-key to kubeadm upload-certs phase, and improve init output #74671

yagonobre · 2019-02-27T18:06:01Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1408

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: add certificate-key and skip-certificate-key-print flags to kubeadm init

/priority important-longterm
@kubernetes/sig-cluster-lifecycle-pr-reviews

yagonobre · 2019-02-27T18:06:31Z

/remote-priority important-longterm
/priority important-soon

yagonobre · 2019-02-27T18:07:37Z

/remove-priority important-longterm

neolit123

thanks @yagonobre !

i've added a coupe of comments.

cmd/kubeadm/app/cmd/init.go

cmd/kubeadm/app/cmd/phases/init/uploadcerts.go

neolit123 · 2019-02-27T18:35:06Z

Does this PR introduce a user-facing change?:

@fabriziopandini should we add a release note here as well? i'm sort of +1 as the PR is extending the upload certs feature that has a separate release note.

fabriziopandini

@yagonobre thanks for the PR!
Everything seems ok, pending some refinements to the final init message. I'm looping in more people to gather feedback
/approve

fabriziopandini · 2019-02-28T07:12:10Z

cmd/kubeadm/app/cmd/util/join.go

@@ -30,12 +30,16 @@ import (
 )

 var joinCommandTemplate = template.Must(template.New("join").Parse(`` +
-	`kubeadm join {{.ControlPlaneHostPort}} --token {{.Token}}{{range $h := .CAPubKeyPins}} --discovery-token-ca-cert-hash {{$h}}{{end}}{{if .UploadCerts}} --certificate-key {{.CertificateKey}}{{end}}`,
+	`{{if .UploadCerts}}To join a node:


That's the tricky part of this PR.
These are the case that should be managed IMO

without controlPlaneEndpoint --> you can only join worker nodes

with controlPlaneEndpoint --> you can join both control-plane and worker nodes

without certificate key -> you have to copy certs manually

with certificate key -> you can copy certs manually or download certs

A proposal for converting all the above cases into a short and concise command output:

only join worker nodes

You can now join any number of worker nodes by running the following on each as root: kubeadm join <master-ip>:<master-port> --token <token> --discovery-token-ca-cert-hash sha256:<hash>

i. both control-plane (with manual copy) and worker nodes

You can now join any number of control-plane node by copying the required certificate authorities on each node and then running the following as root: To join a control-plane node you should copy required certificates to the node and then run the following command as a root: kubeadm join <master-ip>:<master-port> --experimental-control-plane --token <token> --discovery-token-ca-cert-hash sha256:<hash> Then you can join any number of worker nodes by running the following on each as root: kubeadm join <master-ip>:<master-port> --token <token> --discovery-token-ca-cert-hash sha256:<hash>

ii. both control-plane (with automatic/manual) and worker nodes

To join a control-plane node run the following command on each as a root: kubeadm join <master-ip>:<master-port> --experimental-control-plane --certificate-key <key> --token <token> --discovery-token-ca-cert-hash sha256:<hash> Please note that the certificate-key gives access to cluster sensitive data, keep it secret! As a safeguard, uploaded-certs will be deleted in two hours; If necessary, you can use `kubeadm init phase upload-certs` to reload certs afterward. Then you can join any number of worker nodes by running the following on each as root: kubeadm join <master-ip>:<master-port> --token <token> --discovery-token-ca-cert-hash sha256:<hash>

Possible further improvements:

detail required certificates in case 2.i

break join commands on multiple lines using \ and move the common (--token <token> ...) part on a second line

@timothysc @neolit123 @rosti opinions?

@fabriziopandini I like your ideas. We can extend the security warning to cover tokens too.
@yagonobre I have a particular distaste for having flow control structures (like if statements) in templates. Please, avoid their use. I am OK with for loops though.

the new messages SGTM. i forgot that we now have to handle them.

2.i looks weird repeating the two first sentences in different ways, apart from that messages look fine and clear.

@rosti what do you recommend to avoid ifs on this template?

@yagonobre I may be misunderstanding your question. Correct me if I am wrong.
You can use different templates for the different use cases in the above example.
Something along the lines of:

var templateToUse *template.Template if uploadCerts { templateToUse = uploadCertsJoinTemplate } else { templateToUse = controlPlaneJoinTemplate } ... err = templateToUse.Execute(&out, ctx)

Yes, but I'll need to duplicate some text, I'm not sure if it;s better than have a if in the template.

rosti

Thanks @yagonobre !

cmd/kubeadm/app/cmd/init.go

rosti · 2019-02-28T09:34:00Z

cmd/kubeadm/app/cmd/util/join.go

@@ -30,12 +30,16 @@ import (
 )

 var joinCommandTemplate = template.Must(template.New("join").Parse(`` +
-	`kubeadm join {{.ControlPlaneHostPort}} --token {{.Token}}{{range $h := .CAPubKeyPins}} --discovery-token-ca-cert-hash {{$h}}{{end}}{{if .UploadCerts}} --certificate-key {{.CertificateKey}}{{end}}`,
+	`{{if .UploadCerts}}To join a node:


@fabriziopandini I like your ideas. We can extend the security warning to cover tokens too.
@yagonobre I have a particular distaste for having flow control structures (like if statements) in templates. Please, avoid their use. I am OK with for loops though.

rosti · 2019-02-28T09:43:58Z

cmd/kubeadm/app/cmd/util/join.go

@@ -81,6 +85,9 @@ func GetJoinCommand(kubeConfigFile, token, key string, skipTokenPrint, uploadCer
 	if skipTokenPrint {
 		ctx["Token"] = template.HTML("<value withheld>")
 	}
+	if skipCertificateKeyPrint {
+		ctx["CertificateKey"] = template.HTML("<value withheld>")


Oddly enough, we use html/template here. This might cause problems when we try to feed a template with non-alphanumeric characters (< is turned into <, etc.).
But anyway, this again is out of the scope of this PR.

neolit123 · 2019-03-02T00:01:53Z

@yagonobre please let me know if you don't have the time for this.
i'd be happy to help (create new PR based on this one), because we only have 4 more working days before feature freeze next Thursday. ❄️

…nit output

yagonobre · 2019-03-05T17:39:10Z

@neolit123 should I add a release note?

neolit123 · 2019-03-05T18:23:30Z

@neolit123 should I add a release note?

@yagonobre
yes, please. only the user facing part e.g. kubeadm: add .... flag ....

fabriziopandini · 2019-03-05T19:33:52Z

@yagonobre this looks great!
/approve
I leave final lgtm to someone else (or I will unblock this myself tomorrow during office hours)

k8s-ci-robot · 2019-03-05T19:34:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, yagonobre

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

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

neolit123

/lgtm

given feature freeze is soon -> merge this now, send bug fixes later if needed.

neolit123 · 2019-03-05T19:38:08Z

tide is currently borked, so let's see if this will merge..

timothysc · 2019-03-05T19:46:08Z

We need to draw a cut-line on anything non-bug related.

yagonobre · 2019-03-05T23:06:11Z

/test pull-kubernetes-e2e-gce

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. area/kubeadm labels Feb 27, 2019

k8s-ci-robot requested review from rosti and timothysc February 27, 2019 18:07

k8s-ci-robot removed the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 27, 2019

neolit123 approved these changes Feb 27, 2019

View reviewed changes

cmd/kubeadm/app/cmd/init.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/init.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/init.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/phases/init/uploadcerts.go Show resolved Hide resolved

fabriziopandini approved these changes Feb 28, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2019

rosti reviewed Feb 28, 2019

View reviewed changes

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

fabriziopandini mentioned this pull request Mar 5, 2019

Tracking issue for Certificates copy for join --control-plane kubernetes/kubeadm#1373

Closed

25 tasks

yagonobre force-pushed the certificate-key branch from b163184 to 205efc0 Compare March 5, 2019 15:45

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

Add certificate-key to kubeadm init upload-certs phase, and improve i…

b193685

…nit output

yagonobre force-pushed the certificate-key branch from 205efc0 to b193685 Compare March 5, 2019 17:36

k8s-ci-robot assigned neolit123 Mar 5, 2019

neolit123 reviewed Mar 5, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2019

k8s-ci-robot merged commit 2b63efc into kubernetes:master Mar 6, 2019

yagonobre deleted the certificate-key branch March 6, 2019 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add certificate-key to kubeadm upload-certs phase, and improve init output #74671

Add certificate-key to kubeadm upload-certs phase, and improve init output #74671

yagonobre commented Feb 27, 2019 •

edited

Loading

yagonobre commented Feb 27, 2019

yagonobre commented Feb 27, 2019

neolit123 left a comment

neolit123 commented Feb 27, 2019

fabriziopandini left a comment

fabriziopandini Feb 28, 2019 •

edited

Loading

rosti Feb 28, 2019

neolit123 Feb 28, 2019

ereslibre Feb 28, 2019

yagonobre Mar 5, 2019

rosti Mar 5, 2019

yagonobre Mar 5, 2019

rosti left a comment

rosti Feb 28, 2019

rosti Feb 28, 2019

neolit123 commented Mar 2, 2019

yagonobre commented Mar 5, 2019

neolit123 commented Mar 5, 2019

fabriziopandini commented Mar 5, 2019

k8s-ci-robot commented Mar 5, 2019

neolit123 left a comment

neolit123 commented Mar 5, 2019

timothysc commented Mar 5, 2019

yagonobre commented Mar 5, 2019

Add certificate-key to kubeadm upload-certs phase, and improve init output #74671

Add certificate-key to kubeadm upload-certs phase, and improve init output #74671

Conversation

yagonobre commented Feb 27, 2019 • edited Loading

yagonobre commented Feb 27, 2019

yagonobre commented Feb 27, 2019

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 27, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Feb 28, 2019 • edited Loading

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

rosti 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 Mar 2, 2019

yagonobre commented Mar 5, 2019

neolit123 commented Mar 5, 2019

fabriziopandini commented Mar 5, 2019

k8s-ci-robot commented Mar 5, 2019

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Mar 5, 2019

timothysc commented Mar 5, 2019

yagonobre commented Mar 5, 2019

yagonobre commented Feb 27, 2019 •

edited

Loading

fabriziopandini Feb 28, 2019 •

edited

Loading