-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
general: update base image to Debian 12 ("Bookworm") #3403
Conversation
f597f21
to
69d3402
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
6d2cc16
to
8812a9e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
8812a9e
to
f276853
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bulk of this review is just notes for my benefit, and optional / future changes.
-
/etc/cron.daily/spamassassin
(maintenance run ofsa-update
) will fail to run properly with Debian 12:- Requires
/etc/spamassassin/skip-timer-conversion
(empty file that is checked for existence), or the prevention ofsystemd
subpackage being installed (byamavisd-new
), which creates/run/systemd
.- UPDATE: Actually, we can avoid this provided no other packages introduce
/run/systemd
by installingsystemd-standalone-sysusers
explictly. We should probably include a test along with that to avoid problems in future.
- UPDATE: Actually, we can avoid this provided no other packages introduce
-
Dockerfile
should include a comment referencing my PR feedback for details of the'CRON=1' > /etc/default/spamassassin
+ any additional added to resolve the above Debian 12 compatibility concern. - Drop this run-time change to
/etc/cron.daily/spamassassin
, it's no longer applicable or useful.
- Requires
-
Dockerfile
should include contextual comment for change to/usr/lib/rsyslog/rsyslog-rotate
. Easier to grok that it's for our alternative process manager than part of a fix related to the change preceding it. - Add back the two python packages that support Fail2Ban (see the Fail2Ban project README for more info, we lack any test coverage to catch this).
- Optional:
packages.sh
has feedback to consider. - Optional: Changelog notes. You may want to directly update the changelog via the PR? See the
compatibility_level 3.6
comment for a suggestion. - Optional: SA related test-case updates have suggested improvements to consider.
- Deferred: Rspamd may need an
sa-update
hook to reload the service, similar to the one for Amavis.
I don't mind applying these myself, but I'll let the feedback available for discussion for a day or so prior. I have some other tasks to pursue and get PRs opened for, this review was a bit of a time sink detour due to SA 😅
test/tests/parallel/set1/spam_virus/disabled_clamav_spamassassin.bats
Outdated
Show resolved
Hide resolved
_log 'debug' 'Installing Fail2ban' | ||
apt-get "${QUIET}" --no-install-recommends install python3-pyinotify python3-dnspython |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost forgot about this 😅
You've skipped these two packages, but they're added for a reason to support Fail2Ban better, they're still relevant, check git blame
(commit message below, see relevant section of PR #3032 for more details):
fix: Install optional python packages for
fail2ban
These have barely any overhead in layer weight. The DNS package may provide some QoL improvements, while the
pyinotify
is a better alternative than polling logs to check for updates._We have
gamin
package installed butfail2ban
would complain in the log that it was not able to initialize the module for it. There only appears to be apython-gamin
dependent on EOL python 2, no longer available from Debian Bullseye.
python3-pyinotify
is also listed as a recommended package in Debian fail2ban
, as is nftables
, whois
, bsd-mailx
(elsewhere in packages.sh
but specifically for fail2ban
support).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this back.
Review context regarding version changes in Debian 12 packages, plus some related history/notes for my benefit 😅 Major version updates should be included in changelog.
Regarding base image change consideration for Alpine vs Fedora:
I still have reasons for cautioning against Alpine as the base image for DMS, but lacking time to do a write-up going into detail 😅 EDIT: Presently there is a bug with QEMU that's being triggered by ARM builds that affects images using Alpine musl, stalling due to timeout. Thought I'd mention that as it's affecting quite a few CI users building docker images that use Alpine. As mentioned before, there is a variety of problems you can experience adopting Alpine and I'd strongly discourage that for DMS. Also as an additional reference, there was a recent discussion about base image change and migrating from bash to a language like rust. While neither is likely to happen (as per the discussion), it does highlight various problems known with Debian + bash. |
I will come back to this PR. @polarathene thank you for your elaborate review; I will make sure to go all of it as soon as I have time again. Probably October. |
I have finished the review. Looks overall good to me, just a few questions/comments. |
- removed `source /etc/os-release` and used `VERSION_CODENAME` manually - adding PPAs is now done in a separate function - one invocation of `curl` was streamlined - manually applied suggestions from @polarathene (I could not find them on GitHub, I don't know why...) - brought back removal of `/etc/postsrsd.secret` Co-authored-by: Casper <casperklein@users.noreply.github.com> Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
I've resolved both of these for you directly.
Should be a fairly obvious one to fix considering the related changes in this PR. Amavis process name changed, it needs to be adjusted in the test accordingly. The test only fails now from the revision because the coverage / specificity improved to catch it 😅 Should just be changing the check from
This one is odd. The logs found in the output to match have Perhaps a bug with the updated |
Should be ok for the most part, `OAUTHBEARER` is roughly equivalent in coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all feedback / concerns have now been addressed 👍
Let's finally merge this! 💪 🥳 (EDIT: After releasing v13.3.1, which I thought had already been done 😬 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional - Neither need to be addressed by this PR though.
target/scripts/build/packages.sh
Outdated
@@ -1,10 +1,13 @@ | |||
#!/bin/bash | |||
|
|||
# -eE :: exit on error (do this in functions as well) | |||
# -u :: show (and exit) when using unset variables | |||
# -eE :: exit on error (do this in functions as well) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this was what @casperklein had in mind 🤷♂️
# -eE :: exit on error (do this in functions as well) | |
# -e :: exit on error (do this in functions as well) | |
# -E :: inherit the ERR trap to functions, command substitutions and sub-shells |
This comment was marked as outdated.
This comment was marked as outdated.
I have applied your latest review comments about the Rspamd installation and will merge this PR now when tests are passing :) |
Finally 🚀🚀🚀 Thank you guys for the nice feedback and review! |
I noticed, that the timestamp format in Old:
New:
It's not only dovecot, postfix etc. uses the same new format. Any idea how to revert this back to the old format? PS: This is a breaking change for someone parsing mail.log entrys. |
The change of the time format ist mentioned here. There are other worth mentioning changes, e.g. the files /var/log/mail.{info,warn,err} are no longer created.
Adding |
It seems like rsyslog finally adopted ISO8601 as a time stamp format. I actually really appreciate that (I always thought the rsyslog format was weird and most-notable non-standard)! We should provide an entry in the changelog though, indeed. |
That's probably the newer syslog RFC format not just the timestamp change, whereas before it was BSD syslog which was less formalized in spec. |
I really like the "weird" format, because it's very easily human readable, compared to the high precision format now used. Having lots of servers and a central logging, the new format seems to be the better choice. But for a single server I prefer it simple and easy accessible. |
I understand that :) Can you additionally provide a PR that updates the CHANGELOG @casperklein? |
Description
Update to Debian 12 ("Bookworm") as the base image and fix/adjust all parts affected by this change.
Superseeds #3402
Type of change
Checklist:
docs/
)