New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kubeadm: Enable dry-run
mode for phase of control-plane-prepare certs
#113005
Conversation
@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 The 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. |
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove if dryRun
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | |
} |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use "folder" instead, as I found this word is preferred in this project:
fmt.Printf("[control-plane] Using manifest folder %q\n", data.ManifestDir()) |
@@ -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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reach here
fmt.Printf("[certs] Valid certificates and keys now exist in %q\n", cfg.CertificatesDir) |
If any errors thrown from here:
func (k *KubeadmCert) CreateFromCA(ic *kubeadmapi.InitConfiguration, caCert *x509.Certificate, caKey crypto.Signer) error { |
If yes, I agree with you.
33af754
to
59f6748
Compare
@pacoxu thanks for the review! answered with inline comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
overall
for the print details, leave them to @neolit123 @SataQiu to confirm. See #113005 (comment).
/assign @SataQiu |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see: #113005 (comment) 😄
I think either of the word are okay to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the 'ing' verb is better here. Saving / Downloading doesn't matter much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use klog.V(1)
here? Given that our current output already illustrates what is being done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
59f6748
to
0697402
Compare
@SataQiu Did some minor adjustment, use |
Thanks @chendave |
can we also close this issue with this PR? 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>
0697402
to
b3f91f0
Compare
I believe so, updated the PR description to close that issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[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:
Approvers can indicate their approval by writing |
/lgtm |
All certs will be created under the folder of
/etc/kubernetes/tmp/kubeadm-join-dryrunxxx
if thedry-run
mode is enabled.Try to make each phase idempotent by resetting the cert dir with
dry-run
modeSigned-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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: