nixos/spamassassin: Avoid network dependency on boot #103902
Conversation
pkern
changed the title
|
@alyssais Hey Alyssa, would you mind having a look at this? It looks like you touched spamassassin last. ;-) |
|
@ofborg eval |
|
Gentle ping. :) |
|
Do we need the spamd-init service at all any more, then? All it does is create a state directory and do an update, so couldn’t it be replaced by a systemd |
|
@alyssais That's a good point. I think I made it work, PTAL. For spamd itself I did not see how you'd properly run it as non-root, though, so the systemd bits do escalation to root for starting and reloading but need to be set to user/group=spamd to ensure proper permissions on the state directory. I tested that initial startup and updates worked fine and did reload spamd without hanging. While I was on it I also added sa-compile support post-update. |
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-linux: 1-10
and removed
10.rebuild-darwin: 0
10.rebuild-linux: 0
labels
|
Although to be fair it might make sense to split out the sa-compile bit. It currently runs too often, i.e. not just when sa-update signals an update. But I guess I don't have a way to hook into that without using a script again or another service (which makes things more messy). |
|
I re-added a script to only call sa-compile if updates have been installed. So now we are down to just a superfluous spamd reload in that case (because ExecStartPost cannot distinguish the two cases and we require elevation to do the systemctl invocation). |
|
@alyssais That's a good point. I think I made it work, PTAL. For spamd
itself I did not see how you'd properly run it as non-root, though, so
the systemd bits do escalation to root for starting and reloading but
need to be set to user/group=spamd to ensure proper permissions on the
state directory. I tested that initial startup and updates worked fine
and did reload spamd without hanging.
I was able to get it to work, but only if I gave it CAP_NET_BIND_SERVICE
so it could bind to port 783. But I don't think it would be better than
what we have here, because with that approach spamd would be able to
bind to a low port at any time, whereas currently it can't do that after
priveleges have been dropped. Ideally, of course, spamd would support
socket activation, but that is not the world we live in at the moment.
|
|
Philipp Kern <notifications@github.com> writes:
I re-added a script to only call sa-compile if updates have been
installed.
Should we exit $rc at the end of that script?
So now we are down to just a superfluous spamd reload in
that case (because ExecStartPost cannot distinguish the two cases and
we require elevation to do the systemctl invocation).
Could we use a path unit to do this instead of an ExecStartPost?
(systemd.path(5))
|
|
> @alyssais That's a good point. I think I made it work, PTAL. For spamd
> itself I did not see how you'd properly run it as non-root, though, so
> the systemd bits do escalation to root for starting and reloading but
> need to be set to user/group=spamd to ensure proper permissions on the
> state directory. I tested that initial startup and updates worked fine
> and did reload spamd without hanging.
I was able to get it to work, but only if I gave it CAP_NET_BIND_SERVICE
so it could bind to port 783. But I don't think it would be better than
what we have here, because with that approach spamd would be able to
bind to a low port at any time, whereas currently it can't do that after
priveleges have been dropped. Ideally, of course, spamd would support
socket activation, but that is not the world we live in at the moment.
<systemd/systemd#17655> would solve this, so we
might be able to revisit this in the future even if SpamAssassin doesn't
implement socket activation.
|
sa-compile is only called when rc=0 and then the script returns the exit code of that. That's probably unfortunate in case sa-compile fails and systemd treats this as acceptable per
I naively don't see how given that we'd need some notion that the updating and compiling is done. The last action sa-compile (currently) performs is copying to |
|
> Should we exit $rc at the end of that script?
sa-compile is only called when rc=0 and then the script returns the
exit code of that. That's probably unfortunate in case sa-compile
fails and systemd treats this as acceptable per
`SuccessExitStatus`. Maybe it'd make sense to run sa-compile in its
own unit somehow. This all seems messy, but "exit $rc" would not
help. :/
I don't understand, then. As I see it, there are the desirable
outcomes:
sa-update exits 0 -> success
sa-update exits 1 -> sa-compile exits 0 -> success
sa-update exits 1 -> sa-compile something else -> fail
sa-update exits something else -> fail
But the code you have here does not get this right, in the case where
sa-update exits 1, and sa-compile fails with exit 1, because then the
script will exit 1, and systemd will interpret that to be a success.
I think it should look like this (with the SuccessExitStatus line
dropped):
${pkgs.spamassassin}/bin/sa-update --verbose --gpghomedir=%S/spamassassin/sa-update-keys/ || [[ $? -eq 1 ]]
${pkgs.spamassassin}/bin/sa-compile
I think that's easier to read, too, because every step either returns
non-zero and exits, or returns 0 and carries on. There's no need to
follow conditionals or think about different non-zero exit codes, except
where required to indicate that a 1 is also acceptable.
|
|
I think it should look like this (with the SuccessExitStatus line
dropped):
${pkgs.spamassassin}/bin/sa-update --verbose --gpghomedir=%S/spamassassin/sa-update-keys/ || [[ $? -eq 1 ]]
${pkgs.spamassassin}/bin/sa-compile
I think that's easier to read, too, because every step either returns
non-zero and exits, or returns 0 and carries on. There's no need to
follow conditionals or think about different non-zero exit codes, except
where required to indicate that a 1 is also acceptable.
Sorry, just realised I made an error of my own here, which was not
differentiating between the sa-update exit codes, so my suggestion
wouldn't work.
So I think your script is fine, but we should drop the SuccessExitStatus
line. What do you think?
|
|
Hm, I guess in terms of states we want:
And then we remove |
|
PTAL. Testing both with updates available and subsequently no updates available worked fine for me. |
|
Resolved the merge conflict. Is there anything else left to do for me? :) |
|
@pkern can you please remove the last 2 commits and do a rebase instead? |
sa-update currently runs as part of the pre-start script of spamd. The network is not guaranteed to be online at that point and even if we were to depend on that, it makes the bootup brittle, as there is a reliance on SpamAssassin's update server as a startup dependency on boot. Refactor the setup to move the pre-start script into its own unit. This allows to perform the setup task only once. Continuous updates are already done by sa-update.service triggered by sa-update.timer. Only run sa-update in case /var/lib/spamassassin is empty. While we are on it, let sa-update.service depend on the network being online.
Let systemd create SpamAssassin's state directory and populate it using the regular updater service. Depend on the updater service on boot but do not propagate failure to the main service. spamd's commands to start and reload the service are still executed as root but user/group are set to properly chown the state directory to the target user. spamd drops privileges itself for its runner children but preserves root on the main daemon (to listen and re-exec).
sa-compile requires re2c, gcc and gnumake to compile the expressions to performant C code. This compilation is done post-installation after every ruleset update and stored as local state.
sa-compile speeds up processing the rules by compiling them from Perl to C. This needs to be run after every update and is saved in the local state directory by Perl and SpamAssassin version.
For sa-update we care about two successful codes: * 1 -> no updates available: exit successfully * 0 -> updates have been installed: run sa-compile and pass through its return code
|
@aanderse Done. |
|
Well... in that case... "ping" to any motivated parties wanting to merge 😄 |
Motivation for this change
sa-update currently runs as part of the pre-start script of spamd. The network is not guaranteed to be online at that point and even if we were to depend on that, it makes the bootup brittle, as there is a reliance on SpamAssassin's update server as a startup dependency on boot.
Refactor the setup to move the pre-start script into its own oneshot unit (
spamd-init.service). This allows to perform the setup task only once. Continuous updates are already done bysa-update.servicetriggered bysa-update.timer. Only runsa-updatein case/var/lib/spamassassinis empty.While we are on it, let sa-update.service depend on the network being online.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)