rspamd: add feature for adjusting options with a file parsed by DMS#3059
rspamd: add feature for adjusting options with a file parsed by DMS#3059georglauterbach merged 29 commits intomasterfrom
Conversation
georglauterbach
added
kind/new feature
georglauterbach
requested review from
casperklein,
polarathene and
wernerfred
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
Looks fine :)
The docs could probably do with some revision, but I lack the time to go through that. They're good enough.
I'm not sure about rspamd-commands as the config file name. Is it just for managing modules? Why not something like rspamd.mod, rspamd-modules.conf (potentially gets syntax highlighting for comments in an editor), or something else that looks more like a text file rather than a binary or folder at a glance? 😅
Likewise, config/override-configs was for a specific test to use. With the intent of having a cleaner separation of what files belong to what tests. I haven't given that too much thought, so it might make sense for some tests to share other config files that aren't in the common default config in tests.
|
Had some trouble with arm64 vs amd64 but this should be properly resolved now. Also applied PR feedback. UPDATE: And corrected a stupid typo. |
| MAIL_ID=$(_exec_in_container tac /var/log/mail.log \ | ||
| | grep -E -m 1 'postfix/smtpd.*: [A-Z0-9]+: client=localhost' \ | ||
| | grep -E -o '[A-Z0-9]{11}') | ||
| | grep -E -o '[A-Z0-9]{9,12}' || true) |
There was a problem hiding this comment.
Forgot to mention this (FYI): On aarch64, the ID postfix uses might not be 11 characters long, so I provide a bit of range here.
There was a problem hiding this comment.
Can you add an inline comment about that just above line 57 then please? That sort of information is probably going to be hard to remember 😅
Do you know why that might be btw? Sounds a bit odd 🤔
There was a problem hiding this comment.
I don't have the slightest idea.. but this is what I saw today on my arm64 machine.
| MAIL_ID=$(_exec_in_container tac /var/log/mail.log \ | ||
| | grep -E -m 1 'postfix/smtpd.*: [A-Z0-9]+: client=localhost' \ | ||
| | grep -E -o '[A-Z0-9]{11}') | ||
| | grep -E -o '[A-Z0-9]{9,12}' || true) |
There was a problem hiding this comment.
Can you add an inline comment about that just above line 57 then please? That sort of information is probably going to be hard to remember 😅
Do you know why that might be btw? Sounds a bit odd 🤔
| # We try getting the most recent version of Rspamd for aarch64 (from an official source, which | ||
| # is the backports repository). The version for aarch64 is 3.2; the most recent version for amd64 | ||
| # that we get with the official PPA is 3.4. | ||
| # | ||
| # Not removing it later is fine as you have to explicitly opt into installing a backports package | ||
| # which is not something you could be doing by accident. | ||
| if [[ $(uname --machine) == 'aarch64' ]] |
There was a problem hiding this comment.
Not a big fan of this due to version difference, and the official rspamd advice is to not use the debian packages 😅
Hopefully the version disparity doesn't cause issues going forward if we take the approach you've chosen to support ARM64.
I had a browse of the rspamd download docs, and noticed they have support for Fedora (and OpenSUSE via OBS, but TW isn't a good idea, and Leap releases are slow - not that Debian is any better).
At one point I wanted to consider Alpine as a base image offering, especially for the regular releases and newer packages, but I'm more reluctant now with all the musl compatibility and performance issues I've seen with it 😅
Fedora might be interesting, they have a minimal base image (slightly bigger than Debian), but hard to say if it's worthwhile or a smooth transition to change to. Those rspamd docs made it seem like it was supported but it's been broken for a while.
Maybe OpenSUSE Leap image base would be worth looking into. It's fairly small at a glance, but I understand there is more familiarity with Debian 😅 (Leap is not likely to stay around either AFAIK as they're working towards a new container focused distro/spin)
One other option is building rspamd ourselves with a multi-stage build, but that has it's own drawbacks (maintenance + build time + CI cache?).
There was a problem hiding this comment.
I agree, it's not very nice, but as of now, ARM64 users would have to use v2.7. With this change, they can use v3.2 which is only 2 minor versions behind the official PPA. I will rework this as soon as a better emerges :) The current (hopefully temporary solution) just makes the version disparity smaller.
About the base image change: we should put this into our backlog for now I think :)
There was a problem hiding this comment.
About the base image change: we should put this into our backlog for now I think :)
I am in no rush, that's not a fun change to tackle and really needs to be worthwhile to bother with 😅
My main motivations were attempting to reduce image weight and switch to something with more frequent release cadence for us to not rely on all these external builds for newer versions, as reliance on that seems to be growing.
Debian Bookworm release should bump up the rspamd package release at least? :)
I raised a request to get official ARM64 builds supported, but given the history of maintainer stances on that, I'm not sure if it'll be approved on their end.
I've also further investigated the Fedora situation further and no fedora release going back 5 years has support to install rspamd based on their instructions 😕 Their docs are terribly out of date for that support it seems.
There was a problem hiding this comment.
This is going to be a tough one indeed. Ubuntu probably wont help here either as it's basically Debian.. We should discuss about this first on a maintainers discussion board or with discussions for this repository so users can post too.
I thought Fedora was actually quite nice, while I don't have a great experience with OpenSUSE, but I'm open to listening to all proposals.
There was a problem hiding this comment.
No rush, we'll likely update to Bookworm before we'd consider properly discussing to change distro :)
|
Added a comment about the mail ID in e5ab05f. |
This addition allows users to mount a single file with commands to adjust Rspamd modules. The commands are pretty easy and meant for simple adjustments.
For Rspamd, the test showed an error which stated that Bash could not write to `/etc/opendkim/trustedHosts`. The directory is not created as the test disables OpenDKIM & OpenDMARC. Rspamd setup can run without them as Rspamd can do DKIM/DMARC itself. I added a proper guard before the `echo` lines that were causing the error to appear.
During some tests I did on an aarch64 machine, I noticed weird behaviour. After investigating, I found that the PPA I added initially only supports amd64. I was looking for a good alternative and the Debinan packports seemed like a good choice. I added comments explaining the whole process in the `packages.sh` script. Now, aarch64 users can enjoy v3.2 too instead of running v2.7 ..
This was coming from PR feedback, and I think the feedback was pretty good so I went ahead and changed the file's name.
So, because commands like `set-option-for-controller` only usually take 2 arguments, previously the value (argument 2) was broken in case it contained a space (as the `while read` loop splits at whitespaces). The other part of value after the space would go into argument 3. I corrected this behaviour. Now, we pass in argument 3 as well and cut off the last whitespace if it exists.
|
1056876 goes to show what I added to the docs a short while ago: "Program testing can be used to show the presence of bugs, but never to show their absence!" -- Dijkstra |
😉
Interesting! Never thought about hearing something linke this though.. actually a bit scary 🙈 |
I resolved the conflicts and after looking at the code, I decided against the proposed solution of using an array/string that is appended to Postfix's milters at the end due to unnecessarry complexity it would add IMO. The current solution is absolutely fine when proper comments are in place (which I also added). Moreover, I added a warning to the log that enabling OpenDMARC but disabling OpenDKIM is not compliant with RFC 7489 (see #3082 (comment)).
I extended the docs a bit (corrected minor issues) and added a section about the current state of AMD64 vs ARM64. I also adjusted the tests a bit (dropping the `|| exit 1` for example, since I adjusted the `_send_mail_and_get_id` function to abort the setup if it fails). I also noticed issues when running Rspamd tests on ARM64, which lead me to disabling the DKIM signing module (specific for ARM64) and the RBL module (not specific to ARM64; in general we do not want users to also require a proper DNS setup for DNSBLs when testing DMS). I was seeing segfaults though with DKIM signing, very very strange (only on ARM64..). The setup script has only seen doc updates or very minor improvements (better regex, etc.).
|
Alright, after dismissing @polarathene's review for the one millionth time, I promise to not do it again (unless required by resolving another conflict - I do hope I don't need to..). I added proper commit messages to the last two commits to make the final review as easy as possible. |
There was a problem hiding this comment.
I decided against the proposed solution of using an array/string that is appended to Postfix's milters at the end due to unnecessarry complexity it would add IMO.
I'm guessing the complication was with expanding the array values within the sed command? Here's two different ways that it could work:
local DKIM_DMARC=()
# In conditionals:
DKIM_DMARC+='$dkim_milter'
DKIM_DMARC+='$dmarc_milter'
# After conditionals retrieve setting value and expand array into it
# (Note querying settings will probably delay without calling our utility method to avoid that):
postconf "smtpd_milters = $(postconf -h smtpd_milters) ${DKIM_DMARC[@]}"
# Alternatively, create a custom setting to use as an interpolated variable (by Postfix):
postconf "dms_dkim_dmarc = ${DKIM_DMARC[@]}"
sed -i -E 's|^(smtpd_milters =.*)|\1 \$dms_dkim_dmarc|g' /etc/postfix/main.cf
# If you ever need to retrieve the value
# Query the custom setting for value (-h) directly:
postfix -h dms_dkim_dmarc
# Or expand (-x) to the interpolated value where it's used:
postfix -hx smtpd_miltersI won't block on it, but it seemed a bit more DRY, in that the related logic between the two ENV conditions handled that shared setting together in one place afterwards 😅
1. adjusted a comment (wording)
2. adding milter variables to Postfix's config is now done with `postconf`
instead of uncommenting them (they are also removed from `main.cf`
too)
3. The command that displays keys was adjusted as well
1. The global option `maxdepth` belongs before local options (like
`type`
2. The files will now be printed properly in one line and without the
whole path prefix
4. simplified some parts (creating of files, etc.9
5. removed a warning that is not valid yet (but will be when SPF is
configurable); added a note (TODO)
For a discussion on why no array is currently used for the milters, see
#3059.
Actually, no :) I think your solution adds lines of code that are not necessary, since in your example you don't account for the extra calls to
I do like some of the parts; especially adding things via
I actually think it is really DRY now, and the current setup is easy to understand. |
Co-authored-by: Brennan Kinney <[email protected]>
|
@polarathene this is a rather big PR, but I actually want to have this merged anyway. Do you think this PR is fine to merge with 1 review? |
|
Documentation preview for this PR is ready! 🎉 Built with commit: fff5d16 |
I have been reasonably thorough with reviews. I know I sometimes miss things though. Pretty much our only other reviewer is Casper and he seems rather busy lately, probably swamped with notifications from this project to wade through too. I don't see any harm in merging it, no release has been made, so there is still time to review even after a merge and provide feedback that can be addressed via a follow-up PR. |
|
Alright, then I will go ahead :D |
Absolutely! lol |
3 participants
Description
Users should be able to do some easy adjustments with our help. With this addition, they can mount a file
rspamd-modules.confand our setup will evaluate it. You can read the documentation for this when looking at the docs preview for this PR :)Type of change
Checklist:
docs/)