kubeadm: Honor `cert-dir` for cert operations #110709

chendave · 2022-06-22T03:17:25Z

cert-dir could be specified to a value other than the default value
we have tests that should be executed successfully on the working cluster

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

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #110708

Special notes for your reviewer:

This issue only exists with a working cluster since the code will fall back to use the cluster configuration when config file is not specified.

And if the cluster cannot be connected with the admin.conf, the cert-dir will be honored, this is why the test can pass with the CI jobs but fail with working cluster, pls check the log printed by this test pr: #110681

pls see #110708 for details.

Does this PR introduce a user-facing change?

`kubeadm certs renew` and   `kubeadm certs check-expiration`  now honor the `cert-dir` on a working kubernetes cluster.

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

/cc @neolit123

chendave · 2022-06-22T03:17:49Z

/sig cluster-lifecycle

neolit123

This seems like a flags vs config problem.
Is cert-dir the only flag we have to override in the incluster config?

The change seems ok, but i think the more accurate fix is to not download the incluster config if a single flag is passed, because flags are also config.

Having said that, i don't think we do it like that in orher places and we just check for the config flag "" string similarly.

chendave · 2022-06-22T10:22:39Z

Is cert-dir the only flag we have to override in the incluster config?

I think so, other flags (CertificatesDir, cfgPath) that are added seem like are not related.

The change seems ok, but i think the more accurate fix is to not download the incluster config if a single flag is passed, because flags are also config.

Okay, will do as suggested.

chendave · 2022-06-22T10:22:46Z

/hold

neolit123 · 2022-06-22T10:32:07Z

Okay, will do as suggested.

Unclear if we want the override vs don't download if a flag is passed. Ideally we should have consistency with other commands / code paths.

BenTheElder · 2022-06-22T16:55:20Z

IMHO override seems like the least surprising behavior for setting a flag.

chendave · 2022-06-23T09:31:48Z

Unclear if we want the override vs don't download if a flag is passed.

Read the code again, if we don't download if the flag is passed, then the config returned is a default config plus the customized cert directory (if cfgPath is not given here and won't be an issue if cfgPath is given), so the override sound like a little better for this case?

Besides, I print one line msg to tell the cert directory is been overridden. Is that okay for you? @neolit123

neolit123 · 2022-06-23T10:19:54Z

cmd/kubeadm/app/cmd/certs.go

+				// certificate renewal or expiration checking doesn't depend on a running cluster, which means the CertificatesDir
+				// could be set to a value other than the default value or the value fetched from the cluster.
+				// cfg.CertificatesDir could be empty if the default value is set to empty (not true today).
+				fmt.Printf("Overriding the default cert dir with the value from command line flag: %s", cfg.CertificatesDir)


i think this should be a klog.V(1).Infof()
other than that LGTM.

/approve

Done, thanks!

k8s-ci-robot · 2022-06-23T10:20:30Z

[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-06-23T10:22:54Z

/triage accepted
/priority backlog

neolit123 · 2022-06-23T11:14:33Z

cmd/kubeadm/app/cmd/certs.go

+				klog.V(1).Infof("Overriding the default cert dir with the value from command line flag: %s", cfg.CertificatesDir)
+				if len(cfg.CertificatesDir) != 0 {


Suggested change

klog.V(1).Infof("Overriding the default cert dir with the value from command line flag: %s", cfg.CertificatesDir)

if len(cfg.CertificatesDir) != 0 {

if len(cfg.CertificatesDir) != 0 {

klog.V(1).Infof("Overriding the cluster certificate directory with the value from command line flag --%s: %s", options.CertificatesDir, cfg.CertificatesDir)

flag name comes from:

kubernetes/cmd/kubeadm/app/cmd/options/constant.go

Line 33 in 0985c47

CertificatesDir = "cert-dir"

- `cert-dir` could be specified to a value other than the default value - we have tests that should be executed successfully on the working cluster Signed-off-by: Dave Chen <dave.chen@arm.com>

neolit123 · 2022-06-23T14:07:34Z

/lgtm

chendave · 2022-06-24T01:41:12Z

/hold cancel

thanks for the review!

k8s-ci-robot requested a review from neolit123 June 22, 2022 03:17

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

neolit123 reviewed Jun 22, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2022

chendave force-pushed the fix_renew branch from 805fad1 to 9ab00df Compare June 23, 2022 09:24

neolit123 reviewed Jun 23, 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 Jun 23, 2022

chendave force-pushed the fix_renew branch from 9ab00df to e4f05ff Compare June 23, 2022 10:55

neolit123 reviewed Jun 23, 2022

View reviewed changes

kubeadm: Honor cert-dir for cert operations

57646c9

- `cert-dir` could be specified to a value other than the default value - we have tests that should be executed successfully on the working cluster Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the fix_renew branch from e4f05ff to 57646c9 Compare June 23, 2022 11:33

k8s-ci-robot assigned neolit123 Jun 23, 2022

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2022

k8s-ci-robot merged commit 07dfdf0 into kubernetes:master Jun 24, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 24, 2022

chendave deleted the fix_renew branch June 24, 2022 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Honor `cert-dir` for cert operations #110709

kubeadm: Honor `cert-dir` for cert operations #110709

chendave commented Jun 22, 2022

chendave commented Jun 22, 2022

neolit123 left a comment

chendave commented Jun 22, 2022

chendave commented Jun 22, 2022

neolit123 commented Jun 22, 2022 •

edited

BenTheElder commented Jun 22, 2022

chendave commented Jun 23, 2022 •

edited

neolit123 Jun 23, 2022

chendave Jun 23, 2022

k8s-ci-robot commented Jun 23, 2022

neolit123 commented Jun 23, 2022

neolit123 Jun 23, 2022

chendave Jun 23, 2022

neolit123 commented Jun 23, 2022

chendave commented Jun 24, 2022

		klog.V(1).Infof("Overriding the default cert dir with the value from command line flag: %s", cfg.CertificatesDir)
		if len(cfg.CertificatesDir) != 0 {

kubeadm: Honor cert-dir for cert operations #110709

kubeadm: Honor cert-dir for cert operations #110709

Conversation

chendave commented Jun 22, 2022

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.:

chendave commented Jun 22, 2022

neolit123 left a comment

Choose a reason for hiding this comment

chendave commented Jun 22, 2022

chendave commented Jun 22, 2022

neolit123 commented Jun 22, 2022 • edited

BenTheElder commented Jun 22, 2022

chendave commented Jun 23, 2022 • edited

neolit123 Jun 23, 2022

Choose a reason for hiding this comment

chendave Jun 23, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 23, 2022

neolit123 commented Jun 23, 2022

neolit123 Jun 23, 2022

Choose a reason for hiding this comment

chendave Jun 23, 2022

Choose a reason for hiding this comment

neolit123 commented Jun 23, 2022

chendave commented Jun 24, 2022

kubeadm: Honor `cert-dir` for cert operations #110709

kubeadm: Honor `cert-dir` for cert operations #110709

neolit123 commented Jun 22, 2022 •

edited

chendave commented Jun 23, 2022 •

edited