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

[20.03] Revert "nixos/acme: Fix allowKeysForGroup not applying immediately" #85333

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

arianvp
Copy link
Member

@arianvp arianvp commented 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 #76052
I wrongly assumed that PR fixed renewal too, which it didn't!

testing renewals is hard, as we need to sleep in tests.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 15, 2020
@infinisil
Copy link
Member

I messed up really bad then, sorry for that!

@arianvp arianvp force-pushed the revert-acme-backport branch from 4f7eb53 to 6507c2c Compare April 16, 2020 08:35
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 arianvp force-pushed the revert-acme-backport branch from 6507c2c to c51c677 Compare April 16, 2020 08:36
@arianvp
Copy link
Member Author

arianvp commented Apr 16, 2020

@GrahamcOfBorg test acme

@worldofpeace worldofpeace merged commit 2ee6b5c into NixOS:release-20.03 Apr 16, 2020
@emilazy emilazy mentioned this pull request Apr 16, 2020
10 tasks
@infinisil
Copy link
Member

Was also fixed in master: #85332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants