kubeadm: print warnings on invalid cert period instead of erroring out #94504
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
neolit123
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
approved
neolit123
force-pushed
the
1.20-warning-cert-bounds-client-side
branch
3 times, most recently
from
a362772
to
564af09
Compare
neolit123
changed the title
|
/kind feature |
k8s-ci-robot
added
kind/feature
neolit123
force-pushed
the
1.20-warning-cert-bounds-client-side
branch
from
564af09
to
f991cb9
Compare
Client side period validation of certificates should not be fatal, as local clock skews are not so uncommon. The validation should be left to the running servers. - Remove this validation from TryLoadCertFromDisk(). - Add a new function ValidateCertPeriod(), that can be used for this purpose on demand. - In phases/certs add a new function CheckCertificatePeriodValidity() that will print warnings if a certificate does not pass period validation, and caches certificates that were already checked. - Use the function in a number of places where certificates are loaded from disk.
neolit123
force-pushed
the
1.20-warning-cert-bounds-client-side
branch
from
f991cb9
to
b5b9698
Compare
| // CheckCertificatePeriodValidity takes a certificate and prints a warning if its period | ||
| // is not valid related to the current time. It does so only if the certificate was not validated already | ||
| // by keeping track with a cache. | ||
| func CheckCertificatePeriodValidity(baseName string, cert *x509.Certificate) { |
There was a problem hiding this comment.
Is the cache used to not print the same warning a lot of time? Or because the validation take too long to run?
There was a problem hiding this comment.
i'm still debating whether the cache is wanted here. the only benefit is that during a "kubeadm" process execution, it can omit printing the same validation warning multiple times for the same certificate.
| @@ -124,6 +125,9 @@ func getKubeConfigSpecs(cfg *kubeadmapi.InitConfiguration) (map[string]*kubeConf | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "couldn't create a kubeconfig; the CA files couldn't be loaded") | |||
| } | |||
| // Validate period | |||
There was a problem hiding this comment.
Should we move this comment to line 129 and keep the line 128 empty?
There was a problem hiding this comment.
i think it's consistent with the other function WriteKubeConfigWithClientCert where // Validate period is printed right after a line with }
k8s-ci-robot
added
the
lgtm
neolit123
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
let's unblock this because original problem has existed for a while. |
|
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
What this PR does / why we need it:
Client side period validation of certificates should not be
fatal, as local clock skews are not so uncommon. The validation
should be left to the running servers.
purpose on demand.
that will print warnings if a certificate does not pass period
validation, and caches certificates that were already checked.
are loaded from disk.
Which issue(s) this PR fixes:
xref #76714
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: