feat: Allow changing the Dovecot vmail UID/GID via ENV#3550
Merged
polarathene merged 13 commits intodocker-mailserver:masterfrom Sep 30, 2023
Merged
feat: Allow changing the Dovecot vmail UID/GID via ENV#3550polarathene merged 13 commits intodocker-mailserver:masterfrom
polarathene merged 13 commits intodocker-mailserver:masterfrom
Conversation
Member
|
I ran the test suite locally with a clone of your branch, but with a slight modification to the This gave a variety of errors though. LDAP (non-concern)This one is just due to the rspamd_full.bats (big failure logs)This one is just due to how the test command was written a bit differently (I've raised a PR to address that). Instead of The 2nd failure is like |
Member
|
Failure keywords observed by log type (from prior comments test failures):
The bulk of test failures are due to either:
Notably, several tests log failures with explicit UID/GID errors were caught. In the
All logs with the `5000` or `9042` valuesLogs filtered by failure keywords |
polarathene
requested changes
Sep 28, 2023
Member
There was a problem hiding this comment.
There is quite a few mistakes to be corrected. Thanks for tackling this though!
- It's great to see you were comfortable enough to try setup a test (which we're definitely lacking proper guidance / docs for).
- You can go into the "Files changed" tab of this PR view and "Add suggestion to batch" everything I've suggested if you agree with the changes, and commit that via the web UI.
git pull your branch locally afterwards and continue resolving the failure ūser.bats will cause (you may want to comment out the wait for empty queue helper for now, substitute with a little bit of sleep for faster failures).
I've rewritten your PR description. Once all the review feedback is addressed, we'll deal with moving/renaming the test file and ENV. Docs will need to be improved a little bit to better clarify the feature (I'll assist with that).
Comment on lines
+36
to
+41
| @test '/var/log/mail/mail.log is error-free' { | ||
| _run_in_container grep 'non-null host address bits in' /var/log/mail/mail.log | ||
| assert_failure | ||
| _run_in_container grep ': error:' /var/log/mail/mail.log | ||
| assert_failure | ||
| } |
Member
There was a problem hiding this comment.
You didn't mention any reasoning for this in your PR description or commit messages.
I know this is taken from tests.bats, but I don't know why you've skimped on it with only a subset of error checks?
docker-mailserver/test/tests/serial/tests.bats
Lines 184 to 201 in 8c0cfa0
Some can be ignored (but they're harmless so adding all for now is fine), here's a proper snippet instead to use:
Suggested change
| @test '/var/log/mail/mail.log is error-free' { | |
| _run_in_container grep 'non-null host address bits in' /var/log/mail/mail.log | |
| assert_failure | |
| _run_in_container grep ': error:' /var/log/mail/mail.log | |
| assert_failure | |
| } | |
| # TODO: Migrate to test/helper/common.bash | |
| # This test case is shared with tests.bats, but provides context on errors + some minor edits | |
| # TODO: Could improve in future with keywords from https://github.com/docker-mailserver/docker-mailserver/pull/3550#issuecomment-1738509088 | |
| # Potentially via a helper that allows an optional fixed number of errors to be present if they were intentional | |
| @test '/var/log/mail/mail.log is error free' { | |
| # Postfix: https://serverfault.com/questions/934703/postfix-451-4-3-0-temporary-lookup-failure | |
| _run_in_container grep 'non-null host address bits in' /var/log/mail/mail.log | |
| assert_failure | |
| # Postfix delivery failure: https://github.com/docker-mailserver/docker-mailserver/issues/230 | |
| _run_in_container grep 'mail system configuration error' /var/log/mail/mail.log | |
| assert_failure | |
| # Unknown error source: https://github.com/docker-mailserver/docker-mailserver/pull/85 | |
| _run_in_container grep -i ': error:' /var/log/mail/mail.log | |
| assert_failure | |
| # Unknown error source: https://github.com/docker-mailserver/docker-mailserver/pull/320 | |
| _run_in_container grep -i 'not writable' /var/log/mail/mail.log | |
| assert_failure | |
| _run_in_container grep -i 'permission denied' /var/log/mail/mail.log | |
| assert_failure | |
| # Amavis: https://forum.howtoforge.com/threads/postfix-smtp-error-caused-by-clamav-cant-connect-to-a-unix-socket-var-run-clamav-clamd-ctl.81002/ | |
| _run_in_container grep -i '(!)connect' /var/log/mail/mail.log | |
| assert_failure | |
| # Postfix: https://github.com/docker-mailserver/docker-mailserver/pull/2597 | |
| _run_in_container grep -i 'using backwards-compatible default setting' /var/log/mail/mail.log | |
| assert_failure | |
| # Postgrey: https://github.com/docker-mailserver/docker-mailserver/pull/612#discussion_r117635774 | |
| _run_in_container grep -i 'connect to 127.0.0.1:10023: Connection refused' /var/log/mail/mail.log | |
| assert_failure | |
| } |
- Dovecot logs with
: Error:(sogrep -i ': error:) - Dovecot failure
failed: Permission denied(already covered bygrep -i 'permission denied', butfailed:may still be applicable elsewhere 🤷♂️ )
Additional keywords have been identified from failures I've caught running this PR: #3550 (comment)
polarathene
added
kind/new feature
polarathene
reviewed
Sep 28, 2023
polarathene
requested changes
Sep 28, 2023
Member
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
I've decided to just apply the feedback myself. You're welcome to read the review feedback if you're interested, or raise any concerns if you disagree with those changes.
Just focus on making ūser.bats pass correctly now by having user.sh apply whatever other configuration changes are required 👍
polarathene
changed the title
Contributor
Author
|
@polarathene Test pass correctly now thanks for your modification and advice!!! 💥 I have just searched 30 min how access to container log before to understand, I need to comment teardown function 😂 note: maybe a part of solution for other LDAP issue will be to add an UID parameter in the account creation script to custom with the LDAP UID dovecot userDB and modify chown for only chown /var/mail and not all folder hierarchy |
polarathene
requested changes
Sep 30, 2023
polarathene
requested changes
Sep 30, 2023
Member
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
Please update the remaining:
UID_DOCKER=>DMS_VMAIL_UIDGID_DOCKER=>DMS_VMAIL_GIDuser.sh=>vmail-id.shuser.bats=>vmail-id.batsaccounts.shneeds to update one more line with5000:500=>${DMS_VMAIL_UID}:${DMS_VMAIL_GID}
Afterwards this looks all good to go 👍
If your test fails regarding sieve, this is an unrelated bug to this PR (the failure is just actually caught now).
This was referenced Sep 30, 2023
Closed
Closed
Contributor
Author
|
I think we are ready to merge? |
polarathene
requested changes
Sep 30, 2023
Member
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
These suggestions were missed.
polarathene
previously approved these changes
Sep 30, 2023
Member
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
LGTM 👍
Thanks for the contribution! I won't merge it immediately, the other maintainers may be interested in blocking it for a review of their own first.
It's a small enough and harmless change to review though, and a useful feature for some users. I think you'll get another approval :)
georglauterbach
previously approved these changes
Sep 30, 2023
Co-authored-by: Georg Lauterbach <[email protected]>
vincentDcmps
dismissed stale reviews from georglauterbach and polarathene
via
e18e9a6
polarathene
approved these changes
Sep 30, 2023
Contributor
|
Documentation preview for this PR is ready! 🎉 Built with commit: e18e9a6 |
11 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
There is a deployment scenario with remote storage (NFS / NAS) where the appliance restricts what UID / GID values are allowed on the file system.
This causes a problem when using a volume bind mount for persisting the DMS
/var/mailonto the host, where the UID/GID of5000:5000(internally mapped todocker:docker) that is used by Dovecot in DMS is not permitted.Unlike a similar issue with LDAP supporting per-user UID/GID (where
chown -Rduring startup and change detection adjusts ownership over/var/mail), this PR only needs to adjust the static vmail user configured for Dovecot. This can be supported easily with ENV.Resolves: #3548 #405 #1611
Resolves discussion: https://github.com/orgs/docker-mailserver/discussions/3407#discussioncomment-6299183
Type of change
Fixes a bug by introducing a new feature.
Checklist:
docs/)