k8s-certs-renew: derive next run from calendar to stop forced monthly renewal#13362
k8s-certs-renew: derive next run from calendar to stop forced monthly renewal#13362polarAli wants to merge 1 commit into
Conversation
|
|
|
Welcome @polarAli! |
|
Hi @polarAli. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: polarAli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
5720442 to
6586453
Compare
6586453 to
8559838
Compare
… renewal The renew script read the next scheduled run from the timer's NextElapseUSecRealtime property. systemd clears that property while the service the timer triggered is still running, so it was always empty during the script's own execution. The empty value made the script skip the certificate-expiry comparison and run `kubeadm certs renew all` on every timer fire, effectively renewing all control-plane certificates monthly instead of only near expiry. Compute the next elapse from the calendar spec with `systemd-analyze calendar` instead, which is independent of the timer's activation state. Fall back to the previous NextElapseUSecRealtime lookup if systemd-analyze cannot resolve it, so the existing skip-comparison safety net is preserved. Also guard the date parse: systemd-analyze prints "Next elapse: never" for a calendar with no future run (e.g. a bounded one-shot override), and any unparseable next_time must fall through to a direct renewal too. Without the guard an empty target_time yields a large negative expiry threshold and the script exits without renewing at all, which is worse than the bug being fixed. Approach suggested by zhenliangliang in the issue discussion. Signed-off-by: Ali Ghotbizadeh <ali.ghotbizadeh@neor.pro>
8559838 to
479c0f8
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
This PR fixes k8s-certs-renew.sh renewing control-plane certificates on every timer fire by avoiding reliance on NextElapseUSecRealtime (which can be empty while the triggered service is running). It derives the next run time from the systemd calendar spec via systemd-analyze calendar, with a fallback to the previous timer-property approach, and guards date parsing so unparseable schedules degrade to the existing “renew directly” behavior.
Changes:
- Compute
next_timeusingsystemd-analyze calendar "{{ auto_renew_certificates_systemd_calendar }}"and parseNext elapse:output. - Fall back to
systemctl show ... NextElapseUSecRealtimewhensystemd-analyzecan’t resolve the calendar. - Add guarded
dateparsing to prevent an unparseable next time from causing a “never renew” outcome.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| if [ "${target_time}" == "" ]; then | ||
| echo "## Skip expiry comparison due to fail to parse next elapse from systemd calendar,do renewal directly ##" |
| # the timer property if systemd-analyze cannot resolve the calendar. | ||
| next_time=$(LC_ALL=C systemd-analyze calendar "{{ auto_renew_certificates_systemd_calendar }}" 2>/dev/null | sed -n 's/^[[:space:]]*Next elapse:[[:space:]]*//p') | ||
| if [ "${next_time}" == "" ]; then | ||
| next_time=$(systemctl show k8s-certs-renew.timer -p NextElapseUSecRealtime --value) |
There was a problem hiding this comment.
Why do we need this fallback if NextElapseUSecRealtime is empty during runtime?
Also, please don't just dump an issue number into an AI tool and ask it to write a fix. It's really important to manually read the issue, investigate the related files, and properly test the changes yourself.
What type of PR is this?
/kind bug
What this PR does / why we need it:
k8s-certs-renew.shdecided whether to renew control-plane certificates by reading the next scheduled run from the timer'sNextElapseUSecRealtimeproperty. systemd clears that property while the service the timer triggered is still running, so it is always empty during the script's own execution. The empty value made the script take the "skip expiry comparison, renew directly" branch and runkubeadm certs renew allon every timer fire — effectively renewing all control-plane certificates monthly (and restarting the control-plane pods) instead of only when they are close to expiry.This computes the next elapse from the calendar spec with
systemd-analyze calendarinstead, which is independent of the timer's activation state, so the expiry comparison works as intended. Ifsystemd-analyzecannot resolve the calendar, it falls back to the previousNextElapseUSecRealtimelookup.It also guards the date parse:
systemd-analyzeprintsNext elapse: neverfor a calendar with no future run (e.g. a bounded one-shot override), and any unparseablenext_timemust fall through to a direct renewal as well. Without that guard an emptytarget_timeproduces a large negative expiry threshold and the script would exit without renewing at all — worse than the original bug. With the guard, an unresolvable schedule degrades to the pre-existing "renew directly" behavior.Which issue(s) this PR fixes:
Fixes #13072
Special notes for your reviewer:
This restores the intended behavior. The expiry-comparison path was effectively dead before this change (it was always skipped), so in practice renewals become roughly annual instead of monthly and the monthly control-plane pod restart stops.
systemd-analyze calendarhas existed since systemd v235; theNext elapse:label is unchanged from v239 through current. On systemd too old to have the verb (e.g. Amazon Linux 2 / systemd 219), the command produces no output,next_timeis empty, and the script falls back exactly to today's behavior — no regression.Verified on a systemd 252 host. The single quoted calendar spec always yields exactly one
Next elapse:line,sedextracts it, anddate -d "${next_time} + ${days_buffer} days"parses it (including a trailing timezone abbreviation and numeric offsets such as+0330). Branch behavior with a mockedkubeadm:Next elapse: never)One user-visible side effect worth calling out: because of the bug,
systemctl start k8s-certs-renew.serviceused to be a guaranteed force-renew. It is now conditional on expiry. The explicit force path remainskubeadm certs renew all.These templated shell scripts don't have a unit-test harness in the repo (shellcheck CI matches
*.sh, not*.sh.j2), so this was verified manually as above; happy to add a test if reviewers would like one in a particular form.Approach suggested by @zhenliangliang in the issue discussion.
Parts of this change were drafted with the assistance of generative AI; I have reviewed, understand, and tested all of it.
Does this PR introduce a user-facing change?: