Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move control plane certs renewal "spread out" into the systemd timer #10596

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Move control plane certs renewal "spread out" into the systemd timer #10596

merged 2 commits into from
Nov 8, 2023

Conversation

VannTen
Copy link
Contributor

@VannTen VannTen commented Nov 6, 2023

What type of PR is this?
/kind bug

What this PR does / why we need it:
Instead of relying on the index for spacing out the auto renewal of certifcates, we instead use 'RandomizedDelaySec' and 'FixedRandomDelay' in the timer unit file.
This prevents having an invalid OnCalendar spec when there is more than 6 control plane nodes.

We also make the timer "Persistent" to avoid missing renewal if the node was offline during the scheduled time for the renewal.

Special notes for your reviewer:

Alternative to #10594

Two potential downsides with the random approach:

  • renewal order is not predictable (at least the first time)
  • Less customizable. We could add a var to opt-in/opt-out of that behavior if really necessary

Does this PR introduce a user-facing change?:

The auto renewal of control plane nodes certificates delay (different for each node to avoid concurrent renewal) is now randomized in a fixed way by node. This means it will happend each time with the same delay for the same node.
action required: if you were using a non-default value for auto_renew_certificates_systemd_calendar, you should review the new unit file.

@VannTen
Use RandomizedDelaySec to spread out control certificates renewal plane
c691e1e 
If the number of control plane node is superior to 6, using (index * 10
minutes) will fail (03:60:00 is not a valid timestamp).

Compared to just fixing the jinja expression (to use a modulo for
example), this should avoid having two control planes certificates
update node being triggered at the same time.
@VannTen
Make k8s-certs-renew.timer Persistent
01b17ae 
If the control plane happens to be offline during the scheduled
certificates renewal (node failure or anything like that), we still want
the renewal to happen.
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 6, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 6, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @VannTen. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2023
@VannTen VannTen mentioned this pull request Nov 6, 2023
Merged
floryut
floryut approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VannTen Looks cleaner to me 👍

@floryut
Copy link
Member

floryut commented Nov 7, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 7, 2023
MrFreezeex
MrFreezeex approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@MrFreezeex MrFreezeex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :D
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, MrFreezeex, VannTen

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit b3f6d05 into kubernetes-sigs:master Nov 8, 2023
63 checks passed
@VannTen VannTen mentioned this pull request Nov 8, 2023
Closed
@VannTen VannTen mentioned this pull request Nov 28, 2023
Closed
9 tasks
@yankay yankay mentioned this pull request Dec 15, 2023
Closed
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
@VannTen @pedromcpedro
Move control plane certs renewal "spread out" into the systemd timer (k…
a8681a7 
…ubernetes-sigs#10596)

* Use RandomizedDelaySec to spread out control certificates renewal plane

If the number of control plane node is superior to 6, using (index * 10
minutes) will fail (03:60:00 is not a valid timestamp).

Compared to just fixing the jinja expression (to use a modulo for
example), this should avoid having two control planes certificates
update node being triggered at the same time.

* Make k8s-certs-renew.timer Persistent

If the control plane happens to be offline during the scheduled
certificates renewal (node failure or anything like that), we still want
the renewal to happen.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrFreezeex MrFreezeex MrFreezeex approved these changes

@floryut floryut floryut approved these changes

@bozzo bozzo Awaiting requested review from bozzo

@yankay yankay Awaiting requested review from yankay

Assignees

@MrFreezeex MrFreezeex

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@VannTen @k8s-ci-robot @floryut @MrFreezeex