nixos/acme: implement postRun using ExecStartPost #76052
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
In 5532065, acme was changed to be RemainAfterExit=true, but `postRun` commands are implemented as `ExecStopPost`. Systemd now considers the service to be still running after simp_le is finished, so won't run these commands (e.g. to reload certificates in a webserver). Change `postRun` to use `ExecStartPost` to ensure the commands are run in a timely manner.
ofborg
bot
added
6.topic: nixos
|
@GrahamcOfBorg test acme |
|
AH good catch. That's also what probably broke the tests on master |
|
Lgtm! |
1 task
m1cr0man
approved these changes
Dec 20, 2019
|
Thanks for fixing this! |
dtzWill
pushed a commit
to dtzWill/nixpkgs
that referenced
this pull request
Dec 24, 2019
nixos/acme: implement postRun using ExecStartPost (cherry picked from commit 749857f)
arianvp
added a commit
to arianvp/nixpkgs
that referenced
this pull request
Apr 15, 2020
This reverts commit 5532065. As far as I can tell setting RemainAfterExit=true here completely breaks certificate renewal, which is really bad! the sytemd timer will activate the service unit every OnCalendar=, however with RemainAfterExit=true the service is already active! So the timer doesn't rerun the service! The commit also broke the actual tests, (As it broke activation too) but this was fixed later in NixOS#76052 I wrongly assumed that PR fixed renewal too, which it didn't! testing renewals is hard, as we need to sleep in tests.
10 tasks
10 tasks
arianvp
added a commit
to arianvp/nixpkgs
that referenced
this pull request
Apr 16, 2020
This reverts commit 5532065. As far as I can tell setting RemainAfterExit=true here completely breaks certificate renewal, which is really bad! the sytemd timer will activate the service unit every OnCalendar=, however with RemainAfterExit=true the service is already active! So the timer doesn't rerun the service! The commit also broke the actual tests, (As it broke activation too) but this was fixed later in NixOS#76052 I wrongly assumed that PR fixed renewal too, which it didn't! testing renewals is hard, as we need to sleep in tests.
arianvp
added a commit
to arianvp/nixpkgs
that referenced
this pull request
Apr 16, 2020
This reverts commit 5532065. As far as I can tell setting RemainAfterExit=true here completely breaks certificate renewal, which is really bad! the sytemd timer will activate the service unit every OnCalendar=, however with RemainAfterExit=true the service is already active! So the timer doesn't rerun the service! The commit also broke the actual tests, (As it broke activation too) but this was fixed later in NixOS#76052 I wrongly assumed that PR fixed renewal too, which it didn't! testing renewals is hard, as we need to sleep in tests.
dtzWill
pushed a commit
to dtzWill/nixpkgs
that referenced
this pull request
May 21, 2020
This reverts commit 5532065. As far as I can tell setting RemainAfterExit=true here completely breaks certificate renewal, which is really bad! the sytemd timer will activate the service unit every OnCalendar=, however with RemainAfterExit=true the service is already active! So the timer doesn't rerun the service! The commit also broke the actual tests, (As it broke activation too) but this was fixed later in NixOS#76052 I wrongly assumed that PR fixed renewal too, which it didn't! testing renewals is hard, as we need to sleep in tests. (cherry picked from commit 5c1c642)
stigok
pushed a commit
to stigok/nixpkgs
that referenced
this pull request
Jun 12, 2020
This reverts commit 5532065. As far as I can tell setting RemainAfterExit=true here completely breaks certificate renewal, which is really bad! the sytemd timer will activate the service unit every OnCalendar=, however with RemainAfterExit=true the service is already active! So the timer doesn't rerun the service! The commit also broke the actual tests, (As it broke activation too) but this was fixed later in NixOS#76052 I wrongly assumed that PR fixed renewal too, which it didn't! testing renewals is hard, as we need to sleep in tests.
Janik-Haag
added
the
12. first-time contribution
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.
Motivation for this change
Consider
security.acme.certs.<name>.postRun = "systemctl reload nginx.service";.I would expect this to reload the certificates into nginx whenever new ones arrive.
However, it appears this does not happen after 5532065.
That commit changed the systemd service to be
Type=oneshot, RemainAfterExit=true, but thepostRuncommands are implemented asExecStopPost.Systemd believes the service is still running, because of
RemainAfterExit, and thus will not reload nginx. Moving toExecStartPostwill reload nginx when the new certificates arrive.Things done
I've built this change and deployed using NixOps, and tested it on the remote server.
I'm happy to do more tests (please yell!), but may need more detailed instructions
(in particular, my machine swiftly OOMs with nix-review, I'm not sure what I'm doing wrong here!)
sandboxinnix.confon non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)