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

nixos/acme: Disable bash tracing #147498

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Nov 26, 2021

Motivation for this change

This is horrible if you want to debug failures that happened during
system switches but your 30-ish acme clients spam the log with the same
messages over and over again.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@dasJ dasJ requested review from ajs124, m1cr0man and a team November 26, 2021 13:00
@github-actions github-actions 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/` labels Nov 26, 2021
@ofborg ofborg bot added 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 Nov 26, 2021
@m1cr0man
Copy link
Contributor

Honestly I'm not too keen on disabling this. When it was introduced there was quite some discussion about it (see here for some of that). The main reason it is there is that ACME failures tend to be really hard to reproduce, and having the script's logs has always been really helpful in debugging.

With the module working reliably for quite some time now, I can understand that the value of having it enabled is probably being outweighed by the log spam. I'm still undecided though, I'd be keen to hear what other people think (@NixOS/acme & co).

@dasJ
Copy link
Member Author

dasJ commented Nov 26, 2021

It's probably easier to opt in than to opt out because one can just do:

script = lib.mkBefore ''
  set -x
'';

This is not possible the other way around

@m1cr0man
Copy link
Contributor

Ok well for a middle ground, how do you feel about adding security.acme.enableDebugLogs (or similar), and defaulting it to true? That way users who run into issues on their first few runs will have clear logs of what went wrong, and those with working systems with lots of logs can disable it.

@dasJ dasJ force-pushed the feat/disable-acme-debugging branch from 5013663 to fb63dd4 Compare December 7, 2021 13:02
@dasJ
Copy link
Member Author

dasJ commented Dec 7, 2021

I hope this is close to what you had in mind

Copy link
Contributor

@m1cr0man m1cr0man left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks good! Lets see if someone can merge it so I can rebase my PR for acme

@@ -616,6 +617,8 @@ in {
options = {
security.acme = {

enableDebugLogs = mkEnableOption "debug logging in the acme script" // { default = true; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works yeah? I'm always adding brackets in an abundance of caution

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've been doing this for years now ;)

@dasJ dasJ requested a review from roberth December 7, 2021 13:11
@dasJ
Copy link
Member Author

dasJ commented Dec 7, 2021

@m1cr0man Do you want me to remove the backport label or are you fine with backporting this change?

@m1cr0man
Copy link
Contributor

m1cr0man commented Dec 7, 2021

Actually.. I hate to be indecisive like this but maybe this is better as an option under security.acme.certs? In your PR description you point out the difficulty debugging if theres many acme clients. If its under the submodule options, that can be solved here too :)

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 7, 2021
@m1cr0man
Copy link
Contributor

m1cr0man commented Dec 7, 2021

@m1cr0man Do you want me to remove the backport label or are you fine with backporting this change?

Keep it for now. I'm not entirely sure if #147784 will be backportable yet.

This is horrible if you want to debug failures that happened during
system switches but your 30-ish acme clients spam the log with the same
messages over and over again.
@dasJ dasJ force-pushed the feat/disable-acme-debugging branch from fb63dd4 to e37aab2 Compare December 7, 2021 13:18
@m1cr0man
Copy link
Contributor

m1cr0man commented Dec 7, 2021

Looks good now!

@dasJ
Copy link
Member Author

dasJ commented Dec 7, 2021

@GrahamcOfBorg test acme

@roberth roberth merged commit e8862a9 into NixOS:master Dec 7, 2021
@dasJ dasJ deleted the feat/disable-acme-debugging branch December 7, 2021 14:16
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2021

Successfully created backport PR #149371 for release-21.11.

@m1cr0man m1cr0man mentioned this pull request Dec 8, 2021
13 tasks
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: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants