`postfix start-fg` was not properly responding to signals received to stop. This caused `supervisorctl restart postfix` and `supervisor stop postfix` to not work as expected (_stopping the Postfix master process, before attempting to start the service again_).
Supervisor does not support custom commands for restarting or stopping a service, relying only on managing the process via a signal. In the past we used a wrapper script to TRAP the signals and trigger commands that way.
However there is a feature which allows us to proxy signals to a different process by referencing a PID file. As Postfix master process creates a pid file when started, we can avoid a wrapper script and the `supervisorctl` functionality works as intended 👍
* postfix: remove smtpd_sasl_auth_enable global setting
* tests: disable auth on 25 port
* tests: revert ldap-smtp-auth-spoofed-sender-with-filter-exception.txt
* Skip failing test
The test seems to have been broken from the beginning.
Sadly, no LDAP maintainers can verify. Added a TODO item if ever a LDAP maintainer comes around.
* Apply PR feedback
---------
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* chore: Only replace `CHKSUM_FILE` when a change has been processed
* chore: Change Detection service should be the last daemon started
* chore: Remove 10 second startup delay for change detector
There should be no concern with conflicts as any writes should have already been done by the time this daemon service is started.
* tests(fix): `smtp_delivery.bats` must wait for Amavis
The change event for adding a user can be processed much sooner now, which means Amavis may not yet be ready.
Added extra condition to wait on at least the Amavis port being reachable, and some failure asserts with the mail queue to better catch / debug when this problem occurs.
* chore: Add some minor delay to avoid Amavis failing to connect
* tests(refactor): Make test cases for opendkim keysizes DRY
- These all do roughly the same logic that can be split into two separate methods.
- `_should_generate_dkim_key()` covers a bit more logic as it can be leveraged to handle other test cases that also perform the same logic.
- The `config/opendkim/` doesn't seem necessary for tests. Only the first few test cases here are testing against it, so we can conditionally make that available. `process_check_restart.bats` also depended on it to run OpenDKIM successfully, but this was due to the `setup-stack.sh` config defaults failing to find an "empty" file forcing `supervisord` to constantly restart the process..
- With this, there we inverse the default opendkim config, so we don't have to mount unique / empty subfolders for each test case, followed by copying over the two extra configs.
* tests(refactor): DRY up more test cases
All the remaining test cases but the last one were refactored here for a clean commit diff. The last test case will be refactored in the following commit.
Plenty of repeated logic spread across these test cases, now condensed into shared methods.
* tests(refactor): Make final test case DRY
* chore: Migrate to new testing helpers
* chore: Revise test case descriptions
* tests(refactor): Improve and simplify assertions
* tests(refactor): Use common container setup instead of `docker run`
- As the majority of test cases are only running `open-dkim` helper, we don't actually have to wait for a full container setup. So an alternative container start is called.
- Also improves assertions a bit more instead of just counting lines.
- Some test cases don't bind mount all of `/tmp/docker-mailserver` contents, thus don't raise permission errors on subsequent test runs.
- Instead of `rm -f` on some config files, have opted to mount them read-only instead, or alternatively mount an anonymous empty volume instead.
- Collapsed the first three test cases into one, thus no `setup_file()` necessary.
- Shift the `_wait_for_finished_setup_in_container()` method into `_common_container_setup()` instead since nothing else is using `_common_container_start()` yet, this allows for avoiding the wait.
* tests(refactor): Collapse dkim key size test cases into single test case
This makes these tests a bit more DRY, and enhances the raised quality issue with these tests. Now not only is the domain checked in the generated DNS dkim record, but we also verify the key size is corrected in the public and private keys via openssl.
* chore: Revise container names
* chore: Swap order of test case 1 and 2
* tests(refactor): Assert generated log output
- `__should_have_tables_trustedhosts_for_domain` shifted in each test case to just after generating the domains keys.
- Asserts `open-dkim` logs instead of just counting them.
- Added checks for domains that should not be present in a test case.
- Additional coverage and notes about the alias from vhost `@localdomain.com`
- Single assert statement with switch statement as all are using common args.
* chore: Minor changes
* tests(refactor): Share `find` logic in helpers and tests
* tests(fix): Listing file content does not need to match line order
The order printed from local system vs CI differed causing the CI to fail. The order of lines is irrelevant so `--index` is not required.
Additionally correct the prefix of the called method to be only one `_` now that it's a `common.bash` helper method.
* chore: Collapse custom DKIM selector test into custom DKIM domain test
These cover the same test logic for the most part, the first domain could also be testing the custom selector.
`special_use_folders.bats` + `mailbox_format_dbox` can assert lines instead, removing the need for `--partial`.
* Apply suggestions from code review
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* chore: Split switch statement method into wrapper methods
---------
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* ci(fix): Temporarily avoid specifying `provenance`
As the test workflow does not use the `docker-container` buildx driver, it uses the Docker Engine bundled BuildKit version which until v23 release does not support attestations.
Likewise the current buildx version in CI is `0.10.0` which does not respect `--provenance false`, the presence of the option appears to trigger a BuildKit version compatibility check and fail early before it considers the value of the option.
* chore: Use buildx `docker-container` driver instead
An alternative solution to omitting `provenance: false` (_not supported by buildx 0.10.0 with default `docker` driver when Docker Engine bundles BuildKit less than 0.10.0, which is the case prior to the Docker Engine v23 release_).
This approach provides more consistency with the build and publish workflows by using the same buildx `docker-container` driver (_and thus newer BuildKit, enabling support for `provenance: false`_).
* chore: Revise test workflow inline docs
Buildx `docker-container` driver is not needed here, but it does seem like it improves cache-hit ratio when building from the retrieved build cache (from the earlier build workflow). Possibly due to building with the same BuildKit version.
* refactor `mail_pop3.bats`
* refactor `mail_with_imap.bats`
* refactor `mail_with_relays.bats`
* moved test that that did not belong into POP3 test
* slightly clean up `no_container.bats`
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* chore: Extract out accounts test cases from `tests.bats`
Standard test file format, the test cases have been copied over unmodified.
* chore: Revise test case descriptions
* tests(refactor): `accounts.bats`
Revised test cases:
- Some common test case logic extracted to test methods.
- Update direct user management commands to use the `setup email ...` variants.
- Improved assertions.
- Removed `sleep 2` lines as the need for that is ambiguous (may no longer be relevant?)
- Additional commentary for maintaining
- Two test cases for missing `postfix-accounts.cf` opted to just run the image without any volumes instead, as the `without-accounts/` folder was empty anyway. 2nd test case can instead use a single `docker run` to check the newly created`postfix-accounts.cf` content.
- `test/config/without-accounts/` remains as `open_dkim.bats` presently uses it.
* chore: Remove unnecessary account removal assert
Traced this back to the original PR where it appears to have been a typo and was probably intended as a cleanup on the `user4` account. Not necessary, removing.
* chore: Rename `accounts.bat` -> `account_management.bats`
---------
* feedback: Avoid `ls` for detecting directories
Replace `ls -d` approach from original test cases
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* feedback: Remove asserting empty output on failure
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* only add Amavis configuration to Postfix when enabled
Since I am running Rspamd nowadays, I noticed there still are ports open
that belong to Amavis. This is because the Amavis configuration is a
fixed part of Postfix's `master.cf`. I changed that. Now, the Amavis
section is added when Amavis really is enabled.
I took the chance and added proper indentation to `master.cf`; hence the
diff is a bit fuzzy. **But**, only the Amavis part was adjusted, the
rest is just styling.
* tests: Migrate and combine ENV tests for `*_INET_PROTOCOLS`
These two features + tests were introduced years apart but serve the same purpose for both Postfix and Dovecot.
Using `--openssl` uses the native `openssl` package within the image instead of the older `1.0.2` bundled from `testssl.sh`.
The test is only testing cipher suite compatibility is what we expect it to be, thus we do not need to run `testssl.sh` with a broader range of ciphers.
* add functionality for filtering mail log by ID
This was not planned, but as @polarthene mentioned in
https://github.com/docker-mailserver/docker-mailserver/pull/3033#issuecomment-1407169569
, filtering the mail log by email ID would be (the only) correct
approach for the Rspamd test (to eliminate race conditions).
I asserted the currect state, and came to the conclusion that this might
(or actually is) something we want in more than one place. So I went
ahead and implemented a solution.
The solution for acquiring the ID is a bit slower because it ensures the
mail queue is empty _before_ and _after_ the mail is sent. This is the
tradeoff one has to make if they want to send multiple emails in one
test file and get their IDs.
I hope you like this approach. I will provide another PR that adjusts
our current tests to use these new functions.
* added note about our helper functions in the docs
I think our work for our custom test framework should be noted in the
docs for newcomers to better understand what they should do.
* adjust Rspamd test to use new helpers for sending
* improve filter helpers further
* add sanity check when acquiring mail ID
* re-add `refute_output` to test which should now work well
This doesn't make any difference to the tests performed here (_partly due to `--preference`_).
It would make a difference if performing a test for receiving a grade, which would otherwise fail due to chain of trust not being verifiable for a self-signed certificate (_or a signed certificate without a CA public key to verify against_)
This appears to have been added to replace the `fam` package in an early version of DMS with Courier for IMAP instead of Dovecot on an Ubuntu 14.04 base image.
It does not appear to serve a purpose anymore.
* chore: Remove the wrapper script for `fail2ban`
- This does not appear necessary. The server can be run with foreground mode.
- `daemons-stack.sh` removal of the socket can be handled by the fail2ban server when using the `-x` option.
* chore: Remove `touch /var/log/auth.log`
These were both added as supposed fixes in 2016 for the then Ubuntu 2014 base image.
Removing them causes no failures in tests.
* 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 but `fail2ban` would complain in the log that it was not able to initialize the module for it. There only appears to be a `python-gamin` dependent on EOL python 2, no longer available from Debian Bullseye.
* chore: Use a common method to check domain and fqdn config
* chore: Shift other test cases into shared test methods
* chore: Add another shared method for checking mail headers
* chore: Add another shared method for checking hostname
* refactor: Improve quality of shared test methods
Based on changes from an earlier closed hostname PR from Oct 2021 with additional revision to use `assert_output` and more thorough checking of values expected in output.
* chore: Move clean shutdown test to `process-check-restart.bats`
This was originally a single test case in `tests.bats` intended for `supervisord` testing.
It seems at some point it got reassigned to a hostname override test container, and then migrated to separate test file for hostname override test by accident.
It now belongs in the correct place again, as hostname config should have nothing to do with a graceful shutdown?
* chore: Prepare for migrating to use `test/helper/setup.bash`
* chore: Rename containers and configured FQDN settings
* chore: Convert to using common container setup helpers
Wait for SMTP port is left at the end to avoid additional start-up delays.
* chore: Use `_run_in_container_bash` helper
* chore: Be more specific on matching mail headers
- I could do multiple container grep calls instead, but opted to match by lines in file. This better ensures values are being matched to the correct lines.
- Renamed the test case descriptions.
- Expanded test coverage of the 4th container as it represents another DNS config, while the 3rd is just the 4th container with the `SRS_DOMAINNAME` env added, no value in more coverage there.
* chore: Remove redundant test coverage in `tests.bats`
These checks are performed in `mail_hostname.bats` with better coverage.
* chore: Move each containers setup into it's own test-case instead
* chore: Re-arrange container name IDs
The original `fqdn-with-subdomain` is now `with-nis-domain` which is more accurate. A new test case will properly cover the default `--hostname` only config that is not a bare domain.
* chore: Re-arrange test cases to align with new ID ordering
This commit just shifts the test cases, no new changes to any content beyond that.
* chore: Add new test case for default config
* chore: Review feedback `_run_in_container_bash` to `_run_in_container`
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* chore: Additional review feedback
- Fix a suggested change bug with quote wrapping an interpolated variable.
- Convert two other `_bash` methods that were missed from review.
- Apply the last two suggested changes from review.
* chore: `_exec_in_container_bash` to `_exec_in_container`
The `| head -n 1` can be dropped if we know for sure it's only one line, which is what we expect. Quotes can then be dropped too.
---------
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
* tests(fix): `spam_junk_folder.bats` Wait on Amavis port to be ready
Postfix can potentially be ready before Amavis is. This caused test failures as mail was sent before Amavis was ready to process it.
Both test cases shared the same test logic, except for the expected location to deliver the spam to. Extracted into a shared test method, and moved the port conditions into there.
* tests(chore): `spam_junk_folder.bats` minor revisions
Test case descriptions, container names and test prefix are now more descriptive of what is under test here (an ENV for Amavis).
* tests(chore): Move Amavis bounce test into `spam_junk_folder.bats`
These two tests seem to be related to the same feature. Grouping them into a single test file instead.
* tests(refactor): Split shared method into smaller methods
Now it can be better shared with the bounce test case.
* tests(chore): Shift test cases to match their CONTAINER_NAME order
No changes to code, just cut + paste of the `CONTAINER3_NAME` test case to shift it to the last test case position.
* added options to toggle OpenDKIM & OpenDMARC
rspamd can provide DKIM signing and DMARC checking itself, so users
should be able to disable OpenDKIM & OpenDMARC. The default is left at
1, so users have to to opt-in when the want to disable the features.
* misc small enhancements
* adjusted start of rspamd
The order of starting redis + rspamd was reversed (now correct) and
rspamd now starts with the correct user.
* adjusted rspamd core configuration
The main configuration was revised. This includes AV configuration as
well as worker/proxy/controller configuration used to control the main
rspamd processes.
The configuration is not tested extensively, but well enough that I am
confident to go forward with it until we declare rspamd support as
stable.
* update & improve the documentation
* add tests
These are some initial tests which test the most basic functionality.
* tests(refactor): Improve consistency and documentation for test helpers (#3012)
* added `ALWAYS_RUN` target `Makefile` recipies (#3013)
This ensures the recipies are always run.
Co-authored-by: georglauterbach <44545919+georglauterbach@users.noreply.github.com>
* adjusted rspamd test to refactored test helper functions
* improve documentation
* apply suggestions from code review (no. 1 by @polarthene)
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* streamline heredoc (EOM -> EOF)
* adjust rspamd test (remove unnecessary run arguments)
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
* fix: RSPAM ENV should only add to array if ENV enabled
* fix: Correctly match ownership for Postfix content
- `/var/lib/postfix` dir and content is `postfix:postfix`, not `postfix:root`.
- `/var/spool/postfix` is `root:root` not `postfix:root` like it's content.
- Add additional comments, including ownership changes by Postfix to `/var/spool/postfix` when process starts / restarts.
* fix: Ensure correct `chown -R` user and groups applied
These were all fine except for clamav not using the correct clamav group. `fetchmail` group is `nogroup` as per the group set by the debian package.
Additionally formatted the `-eq 1 ]]` content to align on the same columns, and added additional comment about the purpose of this `chown -R` usage so that it's clear what bug / breakage it's attempting to prevent / fix.
* refactor: `misc-stack.sh` conditional handling
The last condition doesn't get triggered at all AFAIK. Nor does it make sense to make a folder path with `mkdir -p` to symlink to when the container does not have anything to copy over?
- If that was for files, the `mkdir -p` approach seems invalid?
- If it was for a directory that could come up later, it should instead be created in advance? None of the current values for `FILES` seem to hit this path.
Removing as it doesn't seem relevant to current support.
Symlinking was done for each case, I've opted to just perform that after the conditional instead.
Additional inline docs added for additional context.
* chore: Move amavis `chown -R` fix into `misc-stack.sh`
This was handled separately for some reason. It belongs with the other services handling this fix in `misc-stack.sh`.
The `-h` option isn't relevant, when paired with `-R` it has no effect.
* fix: Dockerfile should preserve `clamav` ownership with `COPY --link`
The UID and GID were copied over but would not match `clamav` user and group due to numeric ID mismatch between containers. `--chown=clamav` fixes that.
* chore: Workaround `buildx` bug with separate `chown -R`
Avoids increasing the image weight from this change by leveraging `COPY` in the final stage.
* chore: `COPY --link` from a separate stage instead of relying on scratch
The `scratch` approach wasn't great. A single layer invalidation in the previous stage would result in a new 600MB layer to store.
`make build` with this change seems to barely be affected by such if a change came before copying over the linked stage, although with `buildx` and the `docker-container` driver with `--load` it would take much longer to import and seemed to keep adding storage. Possibly because I was testing with a minimal `buildx` command, that wasn't leveraging proper cache options?
* lint: Appease the linting gods
* chore: Align `misc-stack.sh` paths for `chown -R` operations
Review feedback
Co-authored-by: Casper <casperklein@users.noreply.github.com>
* fix: Reduce one extra cache layer copy
No apparent advantage of a `COPY --link` initially in separate stage.
Just `COPY --chown` in the separate stage and `COPY --link` the stage content. 230MB less in build cache used.
* fix: Remove separate ClamAV stage by adding `clamav` user explicitly
Creating the user before the package is installed allows to ensure a fixed numeric ID that we can provide to `--chown` that is compatible with `--link`.
This keeps the build cache minimal for CI, without being anymore complex as a workaround than the separate stage was for the most part.
* chore: Add reference link regarding users to `misc-stack.sh`
