nixos/sshguard: fix syslog identifiers and pid file #54495
Conversation
|
Looks good to me. |
Mic92
changed the title
| unitConfig.Documentation = "man:sshguard(8)"; | ||
|
|
||
| serviceConfig = { | ||
| Type = "simple"; |
There was a problem hiding this comment.
"simple" is the default, so this can be removed
There was a problem hiding this comment.
I left this in on purpose, as systemd will be moving towards Type = "exec"; in one of the upcoming versions which will eventually become the default.
There was a problem hiding this comment.
Is there a reason "exec" wouldn't work though? I doubt they'll make a change that breaks every such service.
There was a problem hiding this comment.
There probably is no reason that exec shouldn't work so this is just to make things explicit. I'm fully with you - there is normally no point to specify what's already the default but in this case, I think it makes sense to be explicit.
There was a problem hiding this comment.
I don't really agree, we shouldn't make changes already for a future version that only potentially (with a low chance) could break it.
But I don't want to hold this PR back just because of this
peterhoeg
force-pushed
the
f/sshguard
branch
2 times, most recently
from
767a6f4
to
3281964
Compare
1. Allow syslog identifiers with special characters 2. Do not write a pid file as we are running in foreground anyway 3. Clean up the module for readability Without this, when deploying using nixops, restarting sshguard would make nixops show an error about restarting the service although the service is actually being restarted.
| ###### implementation | ||
|
|
||
| config = mkIf cfg.enable { | ||
|
|
||
| environment.systemPackages = [ pkgs.sshguard pkgs.iptables pkgs.ipset ]; |
There was a problem hiding this comment.
Is sshguard useless on the command line? Is that the reason for the removal of this?
There was a problem hiding this comment.
Correct, you don't interact with sshguard. You could argue that ipset should be there in case you want to manually inspect the ipset that sshguard manipulates but for all normal uses, neither of these are needed.
Motivation for this change
sshguardruns in the foreground, so if we give thePIDFiledirective to systemd, it thinks the service should double-fork making service restarts when deploying with nixops fail.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)Cc @sargon