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.

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

Scheduling renewals #79

Merged
merged 3 commits into from
Apr 8, 2023
Merged

Conversation

jcgruenhage
Copy link
Contributor

@jcgruenhage jcgruenhage commented Apr 6, 2023

Resolves #74.

I've split the work here into three commits for easier reviewing/reasoning:

  1. Sleep until certificate is due instead of looping and rechecking. This
    simplifies the code a bit, and makes the changes required in the next commit
    a ton easier. Should also perform a tiny bit better.
  2. Randomized early renew, the "big" feature here, which I actually wanted to
    work on.
  3. Switch to a default renew_delay of 30d, as recommended by Let's Encrypt.

EDIT: @breard-r I've only checked the contribution guide again after opening this PR, and noticed one thing I've not done "right": I've added a dependency, on rand. I'd assume that's okay though?

If adding a dependency is not okay, it'd be possible to start gating some of the features behind cargo feature flags and disable them by default. Where this would also make sense is #81, which will definitely pull in a lot more dependencies.

@jcgruenhage jcgruenhage force-pushed the renewal-scheduling branch 3 times, most recently from d3b0804 to c56faeb Compare April 7, 2023 10:26
acmed/src/main_event_loop.rs Show resolved Hide resolved
acmed/src/main.rs Outdated Show resolved Hide resolved
@breard-r
Copy link
Owner

breard-r commented Apr 8, 2023

I've split the work here into three commits for easier reviewing/reasoning:

1. Sleep until certificate is due instead of looping and rechecking. This
   simplifies the code a bit, and makes the changes required in the next commit
   a ton easier. Should also perform a tiny bit better.

2. Randomized early renew, the "big" feature here, which I actually wanted to
   work on.

3. Switch to a default renew_delay of 30d, as recommended by Let's Encrypt.

Thank you! 😄

EDIT: @breard-r I've only checked the contribution guide again after opening this PR, and noticed one thing I've not done "right": I've added a dependency, on rand. I'd assume that's okay though?

If adding a dependency is not okay, it'd be possible to start gating some of the features behind cargo feature flags and disable them by default. Where this would also make sense is #81, which will definitely pull in a lot more dependencies.

rand is ok, and obviously adding telemetry will require additional dependencies. It's more of a "don't end up like the average NPM project" than an absolute rule. Furthermore, I still have the left-pad affair in mind.

Let's Encrypt suggests in the integration guide, that for spacing out
renewals after issueing a lot of new certificates in a batch, you renew
a few of them a bit early until it's evened out. This adds a config
option that allows to set a timeframe in which early random delays are
attempted.
This is in line with the recommendations of the Let's Encrypt
integration guide, and the default most other clients implement as well.
@breard-r breard-r merged commit b41f116 into breard-r:main Apr 8, 2023
@jcgruenhage jcgruenhage deleted the renewal-scheduling branch April 8, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduling renewals
2 participants