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

Revert "nixos/acme: Fix allowKeysForGroup not applying immediately" #85332

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.

@arianvp
Copy link
Member Author

arianvp commented Apr 15, 2020

Backport is here #85333

@arianvp
Copy link
Member Author

arianvp commented Apr 15, 2020

@arianvp
Copy link
Member Author

arianvp commented Apr 15, 2020

Need to address #85333 (comment) was an artifact of the revert.

@emilazy
Copy link
Member

emilazy commented Apr 15, 2020

Reverting sounds good to me. Is the re-addition of SuccessExitStatus correct? Presumably it was removed by an intervening commit?

@arianvp
Copy link
Member Author

arianvp commented Apr 16, 2020

It was not. I have force-pushed now

@GrahamcOfBorg test acme

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants