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: Honor cert-dir
for cert operations
#110709
Conversation
/sig cluster-lifecycle |
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.
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.
I think so, other flags (CertificatesDir, cfgPath) that are added seem like are not related.
Okay, will do as suggested. |
/hold |
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. |
IMHO override seems like the least surprising behavior for setting a flag. |
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 Besides, I print one line msg to tell the cert directory is been overridden. Is that okay for you? @neolit123 |
cmd/kubeadm/app/cmd/certs.go
Outdated
// 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) |
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 think this should be a klog.V(1).Infof()
other than that LGTM.
/approve
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, thanks!
[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 |
/triage accepted |
cmd/kubeadm/app/cmd/certs.go
Outdated
klog.V(1).Infof("Overriding the default cert dir with the value from command line flag: %s", cfg.CertificatesDir) | ||
if len(cfg.CertificatesDir) != 0 { |
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.
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:
CertificatesDir = "cert-dir" |
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.
thanks!
- `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>
/lgtm |
/hold cancel thanks for the review! |
cert-dir
could be specified to a value other than the default valueSigned-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
, thecert-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: #110681pls see #110708 for details.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @neolit123