Change default value of ONE_DIR

[casperklein]

Description

This changes the default value of ONE_DIR from 0 to 1.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change that does improve existing functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

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

[casperklein]

Does anyone knows, whats the purpose of having this ENV here?

[georglauterbach]

I think this variable is read in some scripts / needed somewhere even before it is read in start-mailserver.sh.

[casperklein]

somewhere even before it is read in start-mailserver.sh

Startup: dumb-init --> supervisor --> start-mailserver.sh

I couldn't find / think of a place that comes before start-mailserver.sh. The same goes as well for the other ENVs.
Maybe in a separate PR we can give it a try (removing the ENVs) and see if tests pass 😃

[casperklein]

#10 0.767 Mon Aug 23 19:08:21 2021 -> !Database update process failed: Forbidden; Blocked by CDN

The build-and-test check fails due a temporary clamav issue. This is not related to changes in PR.

[polarathene]

Just copying this over from maintainer discussion to provide additional context behind the decision for the change, in case anyone stumbles upon this PR and wonders why it was done:

The only downside I can think of is the additional disk space used.

But when using fail2ban for example, this preserves the bans during container restarts. See also this comment

If I remember correctly, ClamAV will take some good amount of space. My mail-state is currently arround ~24MB (without ClamAV)

Most notably the linked comment mentions ONE_DIR=1 places all state into /var/mail-state so that state like Fail2Ban can be preserved across restarts. That's usually desirable for this service, so makes a good default.

[polarathene]

fails due a temporary clamav issue.

Are you sure it's temporary? The error message seems to make it rather clear that it'll continue to be an issue. I don't think it matters how much we run the CI test, but that it'd be from Github IPs that would be getting blocked?:

#90 0.411 Mon Aug 23 19:43:50 2021 ->     Ensure you are the most updated version by visiting https://www.clamav.net/downloads
#90 0.411 Mon Aug 23 19:43:50 2021 ->  2. Your network is explicitly denied by the FreshClam CDN.
#90 0.411 Mon Aug 23 19:43:50 2021 ->     In order to rectify this please check that you are:
#90 0.411 Mon Aug 23 19:43:50 2021 ->    a. Running an up-to-date version of FreshClam
#90 0.411 Mon Aug 23 19:43:50 2021 ->    b. Running FreshClam no more than once an hour
#90 0.411 Mon Aug 23 19:43:50 2021 ->    c. If you have checked (a) and (b), please open a ticket at
#90 0.411 Mon Aug 23 19:43:50 2021 ->       https://bugzilla.clamav.net under the 'Mirrors' component
#90 0.411 Mon Aug 23 19:43:50 2021 ->       and we will investigate why your network is blocked.
#90 0.411 Mon Aug 23 19:43:50 2021 -> !Database update process failed: Forbidden; Blocked by CDN
#90 0.411 Mon Aug 23 19:43:50 2021 -> !Update failed.

Related upstream issue:

..frequently download the whole database set, or are deploying containers or VMs that don't contain a baseline set of databases and thus when started download the whole database sets.

This has become increasingly costly for the ClamAV project. So the ClamAV project is making a best effort to require users to use FreshClam to update existing databases and to not download the entire database set unless absolutely necessary.

To that end, programs like wget have been blocked entirely (error code 403) and the whole database files like daily.cvd, main.cvd, and bytecode.cvd are being rate limited (error code 429).

The update patch files (the *.cdiff files) are not rate limited, so good netizens can easily update their existing ClamAV installs by running FreshClam.

If it continues to be a problem we might need a workaround (perhaps a separate base image as a cache). I'll restart the test suite.

[casperklein]

Are you sure it's temporary?
but that it'd be from Github IPs that would be getting blocked?

I also encounter this, when building DMS locally. So the error is not (only?) related to Github IPs.

[polarathene]

I'll restart the test suite.

Seems we have a test failure, could it be related to this change? The test doesn't seem to have used ONE_DIR=1 with the container:

not ok 299 checking quota: warn message received when quota exceeded
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 902)
#   `assert_output "2"' failed
# 
# -- output differs --
# expected : 2
# actual   : 3
# --

docker-mailserver/test/tests.bats

Lines 899 to 902 in 13e8c79

	# ensure only the first big message and the warn message are present (other messages are rejected: mailbox is full)
	run docker exec mail sh -c 'ls /var/mail/otherdomain.tld/quotauser/new/ | wc -l'
	assert_success
	assert_output "2"

docker-mailserver/test/tests.bats

Lines 16 to 37 in 13e8c79

	docker run --rm -d --name mail \
	-v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \
	-v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
	-v "$(pwd)/test/onedir":/var/mail-state \
	-e ENABLE_CLAMAV=1 \
	-e SPOOF_PROTECTION=1 \
	-e ENABLE_SPAMASSASSIN=1 \
	-e REPORT_RECIPIENT=user1@localhost.localdomain \
	-e REPORT_SENDER=report1@mail.my-domain.com \
	-e SA_TAG=-5.0 \
	-e SA_TAG2=2.0 \
	-e SA_KILL=3.0 \
	-e AMAVIS_LOGLEVEL=2 \
	-e SA_SPAM_SUBJECT="SPAM: " \
	-e VIRUSMAILS_DELETE_DELAY=7 \
	-e ENABLE_SRS=1 \
	-e SASL_PASSWD="external-domain.com username:password" \
	-e ENABLE_MANAGESIEVE=1 \
	--cap-add=SYS_PTRACE \
	-e PERMIT_DOCKER=host \
	-e DMS_DEBUG=0 \
	-h mail.my-domain.com -t "${NAME}"

[georglauterbach]

I re-ran CI yesterday and the error is still present. So yeah, the error does not seem to be temporary.

[casperklein]

The test doesn't seem to have used ONE_DIR=1 with the container

If ONE_DIR is not defined, it now defaults to 1. Before it was 0. I'll add ONE_DIR=0 and see if that makes a change.

[polarathene]

Tests have passed, but this is also an indication that the test is not covering ONE_DIR=1 which we want to make the default, it may be worth looking into fixing that?

[casperklein]

I am not that familiar with our tests. But in theory, there should be no difference in how DMS services works, if ONE_DIR is set or not (at least, I can't imagine a reason).

[casperklein]

I've no idea right now why our tests fail with ONE_DIR=1.

Is anyone else going to take a look?

Merge anyway or discard this PR? What do you think @georglauterbach @polarathene

[NorseGaud]

Hi, can you re-enable ONE_DIR=1 throughout the test suite (I want to see how it fails). Let's keep this PR open and not merge just yet. I can take a look once I finish a few other things, unless you're in a rush.

[georglauterbach]

I agree with @NorseGaud, we can leave this open, no rush ATM. Ideally, we want to have ONE_DIR=1 throughout the tests.

[casperklein]

You can see the failed test here:

not ok 299 checking quota: warn message received when quota exceeded
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 902)
#   `assert_output "2"' failed
# 
# -- output differs --
# expected : 2
# actual   : 3
# --
# 

[georglauterbach]

You can see the failed test here:

not ok 299 checking quota: warn message received when quota exceeded

# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,

...

The quota test has always been an issue, and we can "safely" restart the test suite if this test fails. The tests seem to pass ATM.

[casperklein]

The tests seem to pass ATM.

Because of 8cc892c

I'll revert that to see if the test still fails.

[georglauterbach]

The tests seem to pass ATM.

Because of 8cc892c

I'll revert that to see if the test still fails.

Ah, I see. But test still pass :)

[casperklein]

As our tests pass, with ONE_DIR' set to 0 or 1, I think this can be merged?

[georglauterbach]

As our tests pass, with ONE_DIR' set to 0 or 1, I think this can be merged?

Agreed.

[NorseGaud]

@georglauterbach @casperklein this is a pretty large change for existing users right? Maybe we should wait until a minor version bump?

[georglauterbach]

This is built for :edge anyway. Users using :edge should be aware that there can be changes at any time with this tag. But I wouldn't argue about releasing v10.2.0 if you'd like to do this afterwards. You could draft a release. @wernerfred has done the last few releases. Maybe you can have a look at his releases for an orientation of what to do :) We can wait for the other PRs marked for 10.2.0 and then release it.

[casperklein]

As @georglauterbach wrote, merging only affects the :edge build. Users with versioned or latest tags are not affected. Only, once 10.2.0 is released.

[georglauterbach]

@casperklein I think you can merge this. It would be interesting to see what happens when the ENVs are removed from the Dockerfile. A separate PR could be opened for that.