puppet: Use certbot package timer, not our own cron job. #20512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexmv
force-pushed
the
certbot-race
branch
3 times, most recently
from
e5b63c4
to
cc2321d
Compare
timabbott
reviewed
Dec 9, 2021
timabbott
reviewed
Dec 9, 2021
The certbot package installs its own systemd timer (and cron job, which disabled itself if systemd is enabled) which updates certificates. This process races with the cron job which Zulip installs -- the only difference being that Zulip respects the `certbot.auto_renew` setting, and that it passes the deploy hook. This means that occasionally nginx would not be reloaded, when the systemd timer caught the expiration first. Remove the custom cron job and `certbot-maybe-renew` script, and reconfigure certbot to always reload nginx after deploying, using certbot directory hooks. Since `certbot.auto_renew` can't have an effect, remove the setting. In turn, this removes the need for `--no-zulip-conf` to `setup-certbot`. `--deploy-hook` is similarly removed, as running deploy hooks to restart nginx is now the default; pass `--no-directory-hooks` in standalone mode to not attempt to reload nginx. The other property of `--deploy-hook`, of skipping symlinking into place, is given its own flog.
|
lgtm. I think ideally we'd add a small "Upgrade notes" entry saying that the |
|
Are we planning to backport this to 4.x? If so, we probably want that upgrade notes entry to just be in the 4.9 section. |
|
This does seem important enough to backport. |
alexmv
added
the
backport candidate
alexmv
added a commit
to zulip/docker-zulip
that referenced
this pull request
Dec 10, 2021
The change in flag name is necessary after zulip/zulip#20512.
andersk
removed
the
backport candidate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The certbot package installs its own systemd timer (and cron job,
which disabled itself if systemd is enabled) which updates
certificates. This process races with the cron job which Zulip
installs -- the only difference being that Zulip respects the
certbot.auto_renewsetting, and that it passes the deploy hook.This means that occasionally nginx would not be reloaded, when the
systemd timer caught the expiration first.
Remove the custom cron job and
certbot-maybe-renewscript, andreconfigure certbot to always reload nginx after deploying, using
certbot directory hooks.
Since
certbot.auto_renewcan't have an effect, remove the setting.In turn, this removes the need for
--no-zulip-conftosetup-certbot.--deploy-hookis similarly removed, as runningdeploy hooks to restart nginx is now the default; pass
--no-directory-hooksin standalone mode to not attempt to reloadnginx. The other property of
--deploy-hook, of skipping symlinkinginto place, is given its own flog.
Testing plan: Untested.