From 672e9cf19a3bb1da309e8cea6ee728e58f905366 Mon Sep 17 00:00:00 2001
From: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Date: Tue, 23 Aug 2022 11:24:23 +1200
Subject: [PATCH] tests: Ensure excessive FD limits are avoided (#2730)

* tests: Ensure excessive FD limits are avoided

Processes that run as daemons (`postsrsd` and `fail2ban-server`) initialize by closing all FDs (File Descriptors).

This behaviour queries that maximum limit and iterates through the entire range even if only a few FDs are open. In some environments (Docker, limit configured by distro) this can be a range exceeding 1 billion (from kernel default of 1024 soft, 4096 hard), causing an 8 minute delay with heavy CPU activity.

`postsrsd` has since been updated to use `close_range()` syscall, and `fail2ban` will now iterate through `/proc/self/fd` (open FDs) which should resolve the performance hit. Until those updates reach our Docker image, we need to workaround it with `--ulimit` option.

NOTE: If `docker.service` on a distro sets `LimitNOFILE=` to approx 1 million or lower, it should not be an issue. On distros such as Fedora 36, it is `LimitNOFILE=infinity` (approx 1 billion) that causes excessive delays.

* chore: Use Docker host limits instead

Typically on modern distros with systemd, this should equate to 1024 (soft) and 512K (hard) limits. A distro may override the built-in global defaults systemd sets via setting `DefaultLimitNOFILE=` in `/etc/systemd/user.conf` and `/etc/systemd/system.conf`.

* tests(fix): Better prevent non-deterministic failures

- `no_containers.bats` tests the external script `setup.sh` (without `-c`). It's expected that no existing DMS container is running  - otherwise it may attempt to use that container and fail. Detect this and fail early via `setup_file()` step.

- `mail_hostname.bats` had a odd timing failure with teardown due to the last tests bringing the containers down earlier (`docker stop` paired with the `docker run --rm`). Adding a moment of delay via `sleep` helps avoid that false positive scenario.
---
 test/mail_fail2ban.bats           |  5 ++++-
 test/mail_hostname.bats           | 22 +++++++++++++++++-----
 test/mail_undef_spam_subject.bats | 15 +++++++++++----
 test/no_container.bats            | 12 +++++++++---
 test/tests.bats                   |  5 ++---
 5 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/test/mail_fail2ban.bats b/test/mail_fail2ban.bats
index 71735c5d..22534bf7 100644
--- a/test/mail_fail2ban.bats
+++ b/test/mail_fail2ban.bats
@@ -9,7 +9,10 @@ function setup_file() {
     -e ENABLE_FAIL2BAN=1 \
     -e POSTSCREEN_ACTION=ignore \
     --cap-add=NET_ADMIN \
-    -h mail.my-domain.com -t "${NAME}"
+    --hostname mail.my-domain.com \
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   # Create a container which will send wrong authentications and should get banned
   docker run --name fail-auth-mailer \
diff --git a/test/mail_hostname.bats b/test/mail_hostname.bats
index c32c5260..31df350c 100644
--- a/test/mail_hostname.bats
+++ b/test/mail_hostname.bats
@@ -11,8 +11,10 @@ function setup_file() {
     -e PERMIT_DOCKER=network \
     -e ENABLE_SRS=1 \
     -e OVERRIDE_HOSTNAME=mail.my-domain.com \
-    -h unknown.domain.tld \
-    -t "${NAME}"
+    --hostname unknown.domain.tld \
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   PRIVATE_CONFIG_TWO=$(duplicate_config_for_container . mail_non_subdomain_hostname)
   docker run --rm -d --name mail_non_subdomain_hostname \
@@ -21,7 +23,9 @@ function setup_file() {
     -e PERMIT_DOCKER=network \
     -e ENABLE_SRS=1 \
     --hostname domain.com \
-    -t "${NAME}"
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   PRIVATE_CONFIG_THREE=$(duplicate_config_for_container . mail_srs_domainname)
   docker run --rm -d --name mail_srs_domainname \
@@ -32,7 +36,9 @@ function setup_file() {
     -e SRS_DOMAINNAME='srs.my-domain.com' \
     --domainname 'my-domain.com' \
     --hostname 'mail' \
-    -t "${NAME}"
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   PRIVATE_CONFIG_FOUR=$(duplicate_config_for_container . mail_domainname)
   docker run --rm -d --name mail_domainname \
@@ -42,7 +48,9 @@ function setup_file() {
     -e ENABLE_SRS=1 \
     --domainname 'my-domain.com' \
     --hostname 'mail' \
-    -t "${NAME}"
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   wait_for_smtp_port_in_container mail_override_hostname
   wait_for_smtp_port_in_container mail_non_subdomain_hostname
@@ -55,6 +63,10 @@ function setup_file() {
 }
 
 function teardown_file() {
+  # Running `docker rm -f` too soon after `docker stop` can result in failure during teardown with:
+  # "Error response from daemon: removal of container mail_domainname is already in progress"
+  sleep 1
+
   docker rm -f mail_override_hostname mail_non_subdomain_hostname mail_srs_domainname mail_domainname
 }
 
diff --git a/test/mail_undef_spam_subject.bats b/test/mail_undef_spam_subject.bats
index 8429f877..b2831e27 100644
--- a/test/mail_undef_spam_subject.bats
+++ b/test/mail_undef_spam_subject.bats
@@ -9,10 +9,13 @@ function setup() {
     -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
     -e ENABLE_SPAMASSASSIN=1 \
     -e SA_SPAM_SUBJECT="undef" \
-    -h mail.my-domain.com -t "${NAME}"
+    --hostname mail.my-domain.com \
+    --tty \
+    "${NAME}"
 
-  PRIVATE_CONFIG=$(duplicate_config_for_container . mail_undef_spam_subject_2)
-  CONTAINER=$(docker run -d \
+  CONTAINER='mail_undef_spam_subject_2'
+  PRIVATE_CONFIG=$(duplicate_config_for_container . "${CONTAINER}")
+  docker run -d \
     -v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \
     -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
     -v "$(pwd)/test/onedir":/var/mail-state \
@@ -30,7 +33,11 @@ function setup() {
     -e SASL_PASSWD="external-domain.com username:password" \
     -e ENABLE_MANAGESIEVE=1 \
     -e PERMIT_DOCKER=host \
-    -h mail.my-domain.com -t "${NAME}")
+    --name "${CONTAINER}" \
+    --hostname mail.my-domain.com \
+    --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
+    "${NAME}"
 
   wait_for_finished_setup_in_container mail_undef_spam_subject
   wait_for_finished_setup_in_container "${CONTAINER}"
diff --git a/test/no_container.bats b/test/no_container.bats
index 9b310804..723a1146 100644
--- a/test/no_container.bats
+++ b/test/no_container.bats
@@ -1,7 +1,13 @@
-load 'test_helper/bats-support/load'
-load 'test_helper/bats-assert/load'
 load 'test_helper/common'
 
+function setup_file() {
+  # Fail early if the test image is already running:
+  assert_not_equal "$(docker ps | grep -o "${NAME}")" "${NAME}"
+  # Test may fail if an existing DMS container is running,
+  # Which can occur from a prior test failing before reaching `no_container.bats`
+  # and that failure not properly handling teardown.
+}
+
 @test "[No Existing Container] checking setup.sh: setup.sh alias list" {
   mkdir -p ./test/alias/config && echo "test@example.org test@forward.com" > ./test/alias/config/postfix-virtual.cf
   run ./setup.sh -p ./test/alias/config alias list
@@ -135,4 +141,4 @@ load 'test_helper/common'
 
   run /bin/sh -c 'cat ./test/relay/config/postfix-relaymap.cf | grep -e "^@example.org\s*$" | wc -l | grep 1'
   assert_success
-}
\ No newline at end of file
+}
diff --git a/test/tests.bats b/test/tests.bats
index 1fbd89a5..7d910115 100644
--- a/test/tests.bats
+++ b/test/tests.bats
@@ -1,5 +1,3 @@
-load 'test_helper/bats-support/load'
-load 'test_helper/bats-assert/load'
 load 'test_helper/common'
 
 export IMAGE_NAME
@@ -37,8 +35,9 @@ setup_file() {
     -e SPOOF_PROTECTION=1 \
     -e SSL_TYPE='snakeoil' \
     -e VIRUSMAILS_DELETE_DELAY=7 \
-    -h mail.my-domain.com \
+    --hostname mail.my-domain.com \
     --tty \
+    --ulimit "nofile=$(ulimit -Sn):$(ulimit -Hn)" \
     --health-cmd "ss --listening --tcp | grep -P 'LISTEN.+:smtp' || exit 1" \
     "${NAME}"