ci: enhance build process by georglauterbach · Pull Request #2755 · docker-mailserver/docker-mailserver

georglauterbach · 2022-08-30T14:26:21Z

Description

Enhances Dockerfile by

outsourcing packages installation
using COPY --link for ClamAV anti-virus bytecode

I am not sure whether using DOCKER_BUILDKIT is a breaking change. Technically not, because the build process is not taken into account for SemVer, right?

PS: Sorry for the noise in the commit message. Accidentally commit the wrong files first with wrong description.
PPS: Note, this reverts #1965 implicitly. That is okay, since Debian 11 now ships with the latest version of Fail2Ban

Type of change

Improvement (non-breaking change that does improve existing functionality)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

This new script is a clean way of handling the installation of packages. I think the huge `RUN` command in `Dockerfile` was hard to read and maintain. Using a script is a non-issue, as the image is rebuilt whenever the script is touched. Moreover, this commit implicitly removed the explicit Fail2Ban installation and therefore #1965, BECAUSE Debian 11 now ships with the latest and greatest Fail2Ban version (even more up-to-date than out current version it seems).

This commit should have the commit message of the previous commit. The previous commit added the appropriate envionment for building with BuildKit, and adds the `COPY --link` directive to use the official ClamAV image to copy the anti-virus bytecode. Therefore, we can drop the `frashclam` command.

polarathene

LGTM 👍

Just a few questions in the feedback, and some optional improvements.

The only change that I'd like to see is it looks like you might have forgotten to remove the FAIL2BAN_* ARGs that #1965 introduced?

All packages are accounted for still which is appreciated before dropping them in a follow-up 👍

I am not sure whether using DOCKER_BUILDKIT is a breaking change.
Technically not, because the build process is not taken into account for SemVer, right?

I don't think it is, but it should be mentioned in release notes maybe? Not sure if it affects anyone that isn't using make build or running their own build command (eg: Docker Compose can do this to extend an image, but in that case I think it just pulls our released image and there is no compatibility issue?)

appease the linting gods

Gotta love that commit title 😄

Co-authored-by: Brennan Kinney <[email protected]>

georglauterbach · 2022-08-31T09:17:50Z

I removed the left-over Fail2Ban ARGs from Dockerfile too :)

polarathene

🎉

See #2755 (comment)

casperklein

Moreover, this commit implicitly removed the explicit Fail2Ban
installation and therefore #1965, BECAUSE Debian 11 now ships with the
latest and greatest Fail2Ban version

I would like to stick with the official debian package from fail2ban. This way we ensure, to get the latest version (also in the future, when Debian might stick with an older version as happen in the past).

While we at it, don't we want to use the most recent dovecot version too? 😉

polarathene · 2022-09-03T21:45:17Z

I would like to stick with the official debian package from fail2ban

Do you mind if we bring that back in a follow-up PR that is introduced as a separate stage?

While we at it, don't we want to use the most recent dovecot version too?

Likewise, I don't mind this if we keep everything required to support it as a separate stage.

This way the changes between switching from the Debian package manager to pulling in external packages to support has cleaner history of changes.

Eventually, I'd like to evaluate using/supporting a different base image such as Alpine, so keeping such changes isolated is helpful.

georglauterbach · 2022-09-03T23:28:55Z

I would like to stick with the official debian package from fail2ban. This way we ensure, to get the latest version (also in the future, when Debian might stick with an older version as happen in the past).

I don't quiet get that. With this PR, we're basically using apt-get install fail2ban - this is what I understand as "the official Debian package". Could you elaborate, please?:)

polarathene · 2022-09-03T23:43:40Z

this is what I understand as "the official Debian package". Could you elaborate, please?:)

I think he was referring to the .deb release from the official Github releases? (aka FAIL2BAN_DEB_URL that was used to pull the release).

There's not been a new release for sometime since though, and the next Debian release (expected around mid 2023?) may not get it (depending on if a release happens before the cutoff point for Debian). There is over 350 commits of updates since 11.2 release, so if that happens we may have to install from Github releases again, instead of just the fail2ban package (currently the latest 11.2 release).

I'm not fussed either way, but if we don't use the package, it should be contained in it's own stage. @casperklein could contribute that after this PR is merged?

georglauterbach · 2022-09-09T09:21:17Z

CI seems to be working as expected with #2753 🎉 (This will now save 13 min on every PR check due to the ARM image being build in parallel 👍🏼)

polarathene · 2022-09-15T07:24:51Z

TL;DR: I don't have a strong enough opinion about the fail2ban decision. I'd rather see this PR get resolved, whatever makes that easier to happen is fine by me.

Serious question: Is this really worth it? IMO, this should be in the new introduced packages.sh and handled there.

I was preferring a separate stage because it interested me in building an image without fail2ban, or being able to change the base image to something like alpine where .deb would not be supported.

This would apply for pretty much anything in packages.sh though, and I don't have a strong need for fail2ban being isolated (it'd be more work beyond this since we have other support in scripts related to it, and tbh I doubt I'll find the time).

I'm fine with it being maintained in packages.sh as it's own function call 👍

Dockerfile is also fine, but it seems like a bunch of extra noise for something that is just a package. Upstream has had many commits, but I haven't seen a release in a while.

I don't know when a new release of f2b will happen or how important that is to the users of our image.

We could do the same for dovecot and postfix and whatever other packages too, but how many users need it, and if they do is Debian the right choice for a base image, or is something that updates packages more frequently like Alpine better? (I'm sure it has it's own share of problems too)

As a maintainer, what I care most about (and for any future maintainers if we're lucky), is that we have something well organized and scoped.

I'd like to say it'd be a git blame away, but we all know how frustrating that can be to track down the original PR for something to get more context on why the project chose to do something, and what in the project is related to a feature/change (@georglauterbach for example originally wanted to mix in changes for rspamd into this PR, which would contribute to the problem)

casperklein

Alright. I am still not 100% convinced, but I don't want to be the blocker here.

I bet, that the time will come when Debian again will offer not the latest version of f2b. Especially with security related software, I prefer the latest version with it's features.
"But then we can add it again".. Yeah. But that's just extra work in the future while now it would be an ease to keep the current f2b solution.

Dovecot/F2B are both very easy to keep up2date, as they offer .deb files or a repository. For Postfix unfortunately, there isn't such a option.

georglauterbach · 2022-09-16T19:33:16Z

Okay, alright :)

Next week, I will tackle the cache issue in our CI and take care of the Fail2Ban concern raised here.

casperklein · 2022-09-17T11:32:02Z

Not sure if I understand you correctly. Are you going to adjust this PR or merge it soon? If the later one, I'll provide a follow up with f2b included in packages.sh and optional if there is consensus also with Dovecot.

georglauterbach · 2022-09-17T15:14:00Z

Not sure if I understand you correctly. Are you going to adjust this PR or merge it soon? If the later one, I'll provide a follow up with f2b included in packages.sh and optional if there is consensus also with Dovecot.

I was about to do the former, but if you want to do the latter, I won't hinder you. I will need to resolve conflicts in either way. Adding the Dovecot package via .deb files seems like a good idea, so maybe a follow-up is a good idea after all (especially since the changes would be more coherent then)?

polarathene · 2022-09-17T21:15:11Z

I'll provide a follow up with f2b included in packages.sh and optional if there is consensus also with Dovecot.

👍 for adding Dovecot. I would appreciate if both f2b and dovecot live in their own separate function calls in packages.sh though if not too much trouble. Just a preference though, approach it however you like :)

georglauterbach · 2022-09-18T06:30:03Z

@casperklein which way do you prefer? Shall I integrate the changes into this PR or shall I or will you do a follow up PR? I prefer a follow-up 🙈

casperklein · 2022-09-18T09:33:38Z

Follow up 👍

georglauterbach · 2022-09-18T11:53:46Z

Follow up 👍

👍 Then I will provide a follow up. I will resolve conflicts for this PR on Monday and then we can merge this :)

casperklein · 2022-09-18T12:57:38Z

I meant, I'll provide a follow up. I've already something prepared 😉

georglauterbach · 2022-09-18T13:07:16Z

I meant, I'll provide a follow up. I've already something prepared 😉

Very nice 🚀

resolved merge conflicts

I saw that all other actions are using v3.1.1, and v3.1 does not actually work properly, so I updated to the latest version

georglauterbach · 2022-09-19T08:33:29Z

I resolved the merge conflicts and updated the docker/build-push action so that the version used across our repository is the same everywhere.

casperklein · 2022-09-27T18:02:10Z

There's not been a new release for sometime since though

Boom, there it is 🚀

polarathene · 2022-09-28T19:53:33Z

I bet, that the time will come when Debian again will offer not the latest version of f2b. Especially with security related software, I prefer the latest version with it's features.

Ok, I didn't expect the release to happen that soon after making such a statement 😆 😅

That just leaves postsrsd (which has been fixed quickly after reporting the bug) for the ulimit issue.

georglauterbach added 2 commits August 30, 2022 16:18

georglauterbach added area/ci area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Aug 30, 2022

georglauterbach requested review from casperklein and polarathene August 30, 2022 14:26

georglauterbach self-assigned this Aug 30, 2022

georglauterbach commented Aug 30, 2022

View reviewed changes

Comment thread target/scripts/helpers/log.sh Outdated

appease the linting gods

693c296

polarathene requested changes Aug 31, 2022

View reviewed changes

polarathene reviewed Aug 31, 2022

View reviewed changes

Comment thread target/scripts/build/packages.sh Outdated

polarathene mentioned this pull request Aug 31, 2022

Building on Ubuntu 21.10 failing to install postfix #2468

Merged

11 tasks

georglauterbach and others added 2 commits August 31, 2022 11:00

Apply suggestions from code review

059f6a8

Co-authored-by: Brennan Kinney <[email protected]>

remove unnecessary pakages / variables

c6ffbe8

georglauterbach requested a review from polarathene August 31, 2022 09:22

polarathene previously approved these changes Sep 1, 2022

View reviewed changes

restore original package installation order

cde7620

See #2755 (comment)

georglauterbach dismissed polarathene’s stale review via cde7620 September 3, 2022 10:02

Merge branch 'master' into ci/build-process

721eda0

georglauterbach requested a review from polarathene September 3, 2022 10:04

casperklein reviewed Sep 3, 2022

View reviewed changes

Comment thread target/scripts/build/packages.sh Outdated

polarathene previously approved these changes Sep 3, 2022

View reviewed changes

georglauterbach requested a review from casperklein September 5, 2022 10:14

Merge branch 'master' into ci/build-process

8262690

casperklein previously approved these changes Sep 16, 2022

View reviewed changes

georglauterbach added 2 commits September 19, 2022 10:21

Merge branch 'master' into ci/build-process

dfe7f96

resolved merge conflicts

update build-push-action to be cohrent

61cd3fa

I saw that all other actions are using v3.1.1, and v3.1 does not actually work properly, so I updated to the latest version

georglauterbach dismissed stale reviews from casperklein and polarathene via 61cd3fa September 19, 2022 08:32

georglauterbach requested review from casperklein and polarathene September 19, 2022 08:32

georglauterbach added the priority/medium label Sep 19, 2022

casperklein approved these changes Sep 19, 2022

View reviewed changes

georglauterbach added the priority/high label Sep 19, 2022

polarathene approved these changes Sep 20, 2022

View reviewed changes

Merge branch 'master' into ci/build-process

b33eec3

georglauterbach merged commit 32c508a into master Sep 21, 2022

georglauterbach deleted the ci/build-process branch September 21, 2022 07:31

casperklein mentioned this pull request Sep 25, 2022

#2755 follow up; bring back fail2ban package installation #2796

Merged

11 tasks

casperklein added a commit that referenced this pull request Sep 29, 2022

#2755 follow up; bring back fail2ban package installation (#2796)

a09d46a

polarathene mentioned this pull request Jan 19, 2023

fix: Ensure state persisted to /var/mail-state retains correct group #3011

Merged

6 tasks

Uh oh!

Conversation

georglauterbach commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

Uh oh!

polarathene left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

casperklein left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

polarathene commented Sep 3, 2022

Uh oh!

georglauterbach commented Sep 3, 2022

Uh oh!

polarathene commented Sep 3, 2022

Uh oh!

georglauterbach commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Sep 15, 2022

Uh oh!

casperklein left a comment

Choose a reason for hiding this comment

Uh oh!

georglauterbach commented Sep 16, 2022

Uh oh!

casperklein commented Sep 17, 2022

Uh oh!

georglauterbach commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Sep 17, 2022

Uh oh!

georglauterbach commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperklein commented Sep 18, 2022

Uh oh!

georglauterbach commented Sep 18, 2022

Uh oh!

casperklein commented Sep 18, 2022

Uh oh!

georglauterbach commented Sep 18, 2022

Uh oh!

georglauterbach commented Sep 19, 2022

Uh oh!

casperklein commented Sep 27, 2022

Uh oh!

polarathene commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

georglauterbach commented Aug 30, 2022 •

edited

Loading

polarathene left a comment •

edited

Loading

georglauterbach commented Aug 31, 2022 •

edited

Loading

casperklein left a comment •

edited

Loading

georglauterbach commented Sep 9, 2022 •

edited

Loading

georglauterbach commented Sep 17, 2022 •

edited

Loading

georglauterbach commented Sep 18, 2022 •

edited

Loading