kubeadm: Enable `dry-run` mode for phase of `control-plane-prepare certs` #113005

chendave · 2022-10-12T09:27:02Z

All certs will be created under the folder of /etc/kubernetes/tmp/kubeadm-join-dryrunxxx if the dry-run mode is enabled.
Try to make each phase idempotent by resetting the cert dir with dry-run mode

Signed-off-by: Dave Chen dave.chen@arm.com

What type of PR is this?

/kind feature
/sig cluster-lifecycle

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #kubernetes/kubeadm#2543

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: command `kubeadm join phase control-plane-prepare certs` is now supporting to run with `dry-run` mode on it's own

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

k8s-ci-robot · 2022-10-12T09:27:09Z

@chendave: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

pacoxu · 2022-10-12T10:39:43Z

cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

@@ -230,10 +231,10 @@ func runControlPlanePrepareDownloadCertsPhaseLocal(c workflow.RunData) error {
 		return err
 	}

-	// If we're dry-running, download certs to tmp dir
-	if data.DryRun() {


Why remove if dryRun here?

data.CertificateWriteDir() already did the check,
pls see:

kubernetes/cmd/kubeadm/app/cmd/join.go

Lines 518 to 521 in 4f2faa2

func (j *joinData) CertificateWriteDir() string {

if j.dryRun {

return j.dryRunDir

}

pacoxu · 2022-10-13T03:10:31Z

cmd/kubeadm/app/phases/copycerts/copycerts.go

@@ -234,6 +234,8 @@ func DownloadCerts(client clientset.Interface, cfg *kubeadmapi.InitConfiguration
 		return errors.Wrap(err, "error decoding secret data with provided key")
 	}

+	fmt.Printf("[download-certs] Saving the certificates to the folder: %q\n", cfg.CertificatesDir)


Suggested change

fmt.Printf("[download-certs] Saving the certificates to the folder: %q\n", cfg.CertificatesDir)

fmt.Printf("[download-certs] Download the certificates to the %q directory\n", cfg.CertificatesDir)

Or s/Saving/Save/g.

let's use "folder" instead, as I found this word is preferred in this project:

kubernetes/cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

Line 193 in 9b4b1c0

fmt.Printf("[control-plane] Using manifest folder %q\n", data.ManifestDir())

pacoxu · 2022-10-13T03:17:29Z

cmd/kubeadm/app/phases/certs/certlist.go

@@ -151,6 +152,7 @@ func (t CertificateTree) CreateTree(ic *kubeadmapi.InitConfiguration) error {
 				continue
 			}
 			// CA key exists; just use that to create new certificates.
+			fmt.Printf("[certs] Using the existing CA certificate %q and key %q\n", filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.crt", ca.BaseName)), filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.key", ca.BaseName)))


Suggested change

fmt.Printf("[certs] Using the existing CA certificate %q and key %q\n", filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.crt", ca.BaseName)), filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.key", ca.BaseName)))

fmt.Printf("[certs] Using existing %s certificate authority\n", ca.BaseName)

as cert dir is already printed later in below.

kubernetes/cmd/kubeadm/app/phases/certs/certs.go

Lines 65 to 69 in 335fd41

if err := certTree.CreateTree(cfg); err != nil {

return errors.Wrap(err, "error creating PKI assets")

}

fmt.Printf("[certs] Valid certificates and keys now exist in %q\n", cfg.CertificatesDir)

I added this line mostly want to let us know where is the CA certs exist, it's using the default location "/etc/kubernetes/pki/" or the dry-run directory "/etc/kubernetes/tmp/"?

It was using "/etc/kubernetes/pki/" and this is why this is not enabled along with other sub-phases (#112945).

The full path will help us to debug, without this there won't give any hint where the CA files are located if the following actions to create certs failed for some reason.

./kubeadm -v 4 join phase control-plane-prepare all is like what I said.

The tmp folder path would be printed in
1.[download-certs] Download the certificates
2. every [certs] Using the existing CA certificate: I prefer to remove the absolute path here. (a little cleaner.)
3. [certs] Valid certificates and keys now exist

[download-certs] Downloading the certificates in Secret "kubeadm-certs" in the "kube-system" Namespace [download-certs] Download the certificates to the folder: "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268" [certs] Using certificateDir folder "/etc/kubernetes/pki" I1013 14:31:43.122073 3706 certs.go:47] creating PKI assets [certs] Using the existing CA certificate "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/front-proxy-ca.crt" and key "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/front-proxy-ca.key" [certs] Generating "front-proxy-client" certificate and key [certs] Using the existing CA certificate "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/etcd/ca.crt" and key "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/etcd/ca.key" [certs] Generating "etcd/peer" certificate and key [certs] etcd/peer serving cert is signed for DNS names [localhost paco] and IPs [10.6.177.41 127.0.0.1 ::1] [certs] Generating "apiserver-etcd-client" certificate and key [certs] Generating "etcd/server" certificate and key [certs] etcd/server serving cert is signed for DNS names [localhost paco] and IPs [10.6.177.41 127.0.0.1 ::1] [certs] Generating "etcd/healthcheck-client" certificate and key [certs] Using the existing CA certificate "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/ca.crt" and key "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268/ca.key" [certs] Generating "apiserver" certificate and key [certs] apiserver serving cert is signed for DNS names [kubernetes kubernetes.default kubernetes.default.svc kubernetes.default.svc.cluster.local paco] and IPs [10.96.0.1 10.6.177.41] [certs] Generating "apiserver-kubelet-client" certificate and key [certs] Valid certificates and keys now exist in "/etc/kubernetes/tmp/kubeadm-join-dryrun2327321268" I1013 14:31:44.366518 3706 certs.go:78] creating new public/private key files for signing service account users [certs] Using the existing "sa" key

Edited. Still prefer to remove the folder here.

Can you reach here

kubernetes/cmd/kubeadm/app/phases/certs/certs.go

Line 69 in 335fd41

fmt.Printf("[certs] Valid certificates and keys now exist in %q\n", cfg.CertificatesDir)

If any errors thrown from here:

kubernetes/cmd/kubeadm/app/phases/certs/certlist.go

Line 66 in 335fd41

func (k *KubeadmCert) CreateFromCA(ic *kubeadmapi.InitConfiguration, caCert *x509.Certificate, caKey crypto.Signer) error {

If yes, I agree with you.

chendave · 2022-10-13T05:53:19Z

@pacoxu thanks for the review! answered with inline comments.

pacoxu

/lgtm
overall
for the print details, leave them to @neolit123 @SataQiu to confirm. See #113005 (comment).

chendave · 2022-10-14T01:36:44Z

/assign @SataQiu
for approval, per the suggest from bot :)

SataQiu · 2022-10-14T03:36:40Z

cmd/kubeadm/app/phases/copycerts/copycerts.go

@@ -234,6 +234,8 @@ func DownloadCerts(client clientset.Interface, cfg *kubeadmapi.InitConfiguration
 		return errors.Wrap(err, "error decoding secret data with provided key")
 	}

+	fmt.Printf("[download-certs] Download the certificates to the folder: %q\n", cfg.CertificatesDir)


Suggested change

fmt.Printf("[download-certs] Download the certificates to the folder: %q\n", cfg.CertificatesDir)

fmt.Printf("[download-certs] Saving the certificates to folder: %q\n", cfg.CertificatesDir)

pls see: #113005 (comment) 😄

I think either of the word are okay to me.

Having the 'ing' verb is better here. Saving / Downloading doesn't matter much.

SataQiu · 2022-10-14T03:59:20Z

cmd/kubeadm/app/phases/certs/certlist.go

@@ -151,6 +152,7 @@ func (t CertificateTree) CreateTree(ic *kubeadmapi.InitConfiguration) error {
 				continue
 			}
 			// CA key exists; just use that to create new certificates.
+			fmt.Printf("[certs] Using the existing CA certificate %q and key %q\n", filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.crt", ca.BaseName)), filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.key", ca.BaseName)))


Can we just use klog.V(1) here? Given that our current output already illustrates what is being done.

What I want is a way to show that the directory here is indeed pointing to a dry-run dir, either print or log is fine for me.

chendave · 2022-10-14T07:00:14Z

@SataQiu Did some minor adjustment, use klog.V(1) instead, could you take another look?

SataQiu · 2022-10-17T09:41:16Z

Thanks @chendave
/lgtm
defer @neolit123 for final approve

neolit123 · 2022-10-17T09:45:39Z

can we also close this issue with this PR?
kubernetes/kubeadm#2543

IIRC, the main problem there was that dryrun overrides the certs dir field. this PR fixes it with deffer.

…rts` - All certs will be created under the folder of `/etc/kubernetes/tmp/kubeadm-join-dryrunxxx` if the `dry-run` mode is enabled. - Try to make each phase idempotent by resetting the cert dir with `dry-run` mode Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave · 2022-10-17T10:31:48Z

can we also close this issue with this PR?

I believe so, updated the PR description to close that issue.

neolit123

/approve

k8s-ci-robot · 2022-10-17T10:41:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, 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

neolit123 · 2022-10-17T12:51:42Z

/lgtm

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 12, 2022

k8s-ci-robot requested review from RA489 and SataQiu October 12, 2022 09:27

k8s-ci-robot added the area/kubeadm label Oct 12, 2022

pacoxu reviewed Oct 12, 2022

View reviewed changes

pacoxu reviewed Oct 13, 2022

View reviewed changes

chendave force-pushed the dry-run-prepare branch from 33af754 to 59f6748 Compare October 13, 2022 05:52

pacoxu reviewed Oct 13, 2022

View reviewed changes

k8s-ci-robot assigned pacoxu Oct 13, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2022

k8s-ci-robot assigned SataQiu Oct 14, 2022

SataQiu reviewed Oct 14, 2022

View reviewed changes

chendave force-pushed the dry-run-prepare branch from 59f6748 to 0697402 Compare October 14, 2022 06:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2022

chendave mentioned this pull request Oct 14, 2022

Decouple kubeadm phases and make each phase idempotent kubernetes/kubeadm#2769

Closed

4 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2022

chendave force-pushed the dry-run-prepare branch from 0697402 to b3f91f0 Compare October 17, 2022 10:30

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2022

neolit123 reviewed Oct 17, 2022

View reviewed changes

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

k8s-ci-robot assigned neolit123 Oct 17, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2022

k8s-ci-robot merged commit 3b8cfef into kubernetes:master Oct 17, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 17, 2022

chendave deleted the dry-run-prepare branch October 18, 2022 02:11

chendave mentioned this pull request Oct 18, 2022

Remake some func args for dry run kubernetes/kubeadm#2543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Enable `dry-run` mode for phase of `control-plane-prepare certs` #113005

kubeadm: Enable `dry-run` mode for phase of `control-plane-prepare certs` #113005

chendave commented Oct 12, 2022 •

edited

k8s-ci-robot commented Oct 12, 2022

pacoxu Oct 12, 2022

chendave Oct 12, 2022

pacoxu Oct 13, 2022

chendave Oct 13, 2022

pacoxu Oct 13, 2022

chendave Oct 13, 2022 •

edited

pacoxu Oct 13, 2022 •

edited

chendave Oct 13, 2022

chendave commented Oct 13, 2022

pacoxu left a comment •

edited

chendave commented Oct 14, 2022

SataQiu Oct 14, 2022

chendave Oct 14, 2022 •

edited

neolit123 Oct 17, 2022

chendave Oct 17, 2022

SataQiu Oct 14, 2022

chendave Oct 14, 2022 •

edited

chendave commented Oct 14, 2022

SataQiu commented Oct 17, 2022

neolit123 commented Oct 17, 2022 •

edited

chendave commented Oct 17, 2022

neolit123 left a comment

k8s-ci-robot commented Oct 17, 2022

neolit123 commented Oct 17, 2022

	func (j *joinData) CertificateWriteDir() string {
	if j.dryRun {
	return j.dryRunDir
	}

	fmt.Printf("[download-certs] Saving the certificates to the folder: %q\n", cfg.CertificatesDir)
	fmt.Printf("[download-certs] Download the certificates to the %q directory\n", cfg.CertificatesDir)

	fmt.Printf("[certs] Using the existing CA certificate %q and key %q\n", filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.crt", ca.BaseName)), filepath.Join(ic.CertificatesDir, fmt.Sprintf("%s.key", ca.BaseName)))
	fmt.Printf("[certs] Using existing %s certificate authority\n", ca.BaseName)

	if err := certTree.CreateTree(cfg); err != nil {
	return errors.Wrap(err, "error creating PKI assets")
	}

	fmt.Printf("[certs] Valid certificates and keys now exist in %q\n", cfg.CertificatesDir)

kubeadm: Enable dry-run mode for phase of control-plane-prepare certs #113005

kubeadm: Enable dry-run mode for phase of control-plane-prepare certs #113005

Conversation

chendave commented Oct 12, 2022 • edited

What type of PR is this?

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?

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

k8s-ci-robot commented Oct 12, 2022

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

chendave Oct 13, 2022 • edited

Choose a reason for hiding this comment

pacoxu Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave commented Oct 13, 2022

pacoxu left a comment • edited

Choose a reason for hiding this comment

chendave commented Oct 14, 2022

Choose a reason for hiding this comment

chendave Oct 14, 2022 • 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

chendave Oct 14, 2022 • edited

Choose a reason for hiding this comment

chendave commented Oct 14, 2022

SataQiu commented Oct 17, 2022

neolit123 commented Oct 17, 2022 • edited

chendave commented Oct 17, 2022

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 17, 2022

neolit123 commented Oct 17, 2022

kubeadm: Enable `dry-run` mode for phase of `control-plane-prepare certs` #113005

kubeadm: Enable `dry-run` mode for phase of `control-plane-prepare certs` #113005

chendave commented Oct 12, 2022 •

edited

chendave Oct 13, 2022 •

edited

pacoxu Oct 13, 2022 •

edited

pacoxu left a comment •

edited

chendave Oct 14, 2022 •

edited

chendave Oct 14, 2022 •

edited

neolit123 commented Oct 17, 2022 •

edited