Lock file create and remove improvements (#2183)

* changed the locking function to better support multiple servers running at once and sharing the same config

* helper function testing now runs inside of container

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

Dockerfile

@@ -61,7 +61,7 @@ RUN \
   pax pflogsumm postgrey p7zip-full postfix-ldap postfix-pcre \
   postfix-policyd-spf-python postsrsd pyzor \
   razor rpm2cpio rsyslog sasl2-bin spamassassin supervisor \
-  unrar-free unzip whois xz-utils && \
+  unrar-free unzip uuid whois xz-utils && \
   # Fail2Ban
   gpg --keyserver ${FAIL2BAN_GPG_PUBLIC_KEY_SERVER} \
     --recv-keys ${FAIL2BAN_GPG_PUBLIC_KEY_ID} 2>&1 && \

Makefile

@@ -25,7 +25,7 @@ clean:
 # remove running and stopped test containers
 	-@ [[ -d config.bak ]] && { rm -rf config ; mv config.bak config ; } || :
 	-@ [[ -d testconfig.bak ]] && { sudo rm -rf test/config ; mv testconfig.bak test/config ; } || :
-	-@ for container in $$(docker ps -a --filter name='^/mail$$|^ldap_for_mail$$|^mail_override_hostname$$|^mail_non_subdomain_hostname$$|^open-dkim$$|^hadolint$$|^eclint$$|^shellcheck$$' | sed 1d | cut -f 1-1 -d ' '); do docker rm -f $$container; done
+	-@ for container in $$(docker ps -a --filter name='^/mail$$|^ldap_for_mail$$|^mail_override_hostname$$|^mail_non_subdomain_hostname$$|^open-dkim$$|^hadolint$$|^eclint$$|^shellcheck$$|mail_changedetector.*' | sed 1d | cut -f 1-1 -d ' '); do docker rm -f $$container; done
 	-@ sudo rm -rf test/onedir test/alias test/quota test/relay test/config/dovecot-lmtp/userdb test/config/key* test/config/opendkim/keys/domain.tld/ test/config/opendkim/keys/example.com/ test/config/opendkim/keys/localdomain2.com/ test/config/postfix-aliases.cf test/config/postfix-receive-access.cf test/config/postfix-receive-access.cfe test/config/dovecot-quotas.cf test/config/postfix-send-access.cf test/config/postfix-send-access.cfe test/config/relay-hosts/chksum test/config/relay-hosts/postfix-aliases.cf test/config/dhparams.pem test/config/dovecot-lmtp/dh.pem test/config/relay-hosts/dovecot-quotas.cf test/config/user-patches.sh test/alias/config/postfix-virtual.cf test/quota/config/dovecot-quotas.cf test/quota/config/postfix-accounts.cf test/relay/config/postfix-relaymap.cf test/relay/config/postfix-sasl-password.cf test/duplicate_configs/
 
 # -----------------------------------------------

target/bin/addmailuser

@@ -42,9 +42,8 @@ PASSWD="${*}"
 [[ -z ${FULL_EMAIL} ]] && { __usage ; errex 'No username specified' ; }
 [[ "${FULL_EMAIL}" =~ .*\@.* ]] || { __usage ; errex 'Username must include the domain' ; }
 
-# Protect config file with lock to avoid race conditions
 touch "${DATABASE}"
-create_lock "$(basename "$0")"
+create_lock # Protect config file with lock to avoid race conditions
 if grep -qi "^$(escape "${FULL_EMAIL}")|" "${DATABASE}"
 then
   echo "User '${FULL_EMAIL}' already exists."

target/bin/delmailuser

@@ -86,7 +86,8 @@ then
   fi
 fi
 
-create_lock "$(basename "$0")"
+create_lock # Protect config file with lock to avoid race conditions
+
 for EMAIL in "${@}"
 do
   ERROR=false

target/bin/setquota

@@ -32,8 +32,7 @@ then
   errex "invalid quota format. e.g. 302M (B (byte), k (kilobyte), M (megabyte), G (gigabyte) or T (terabyte))"
 fi
 
-# Protect config file with lock to avoid race conditions
-create_lock "$(basename "$0")"
+create_lock # Protect config file with lock to avoid race conditions
 
 touch "${DATABASE}"
 if [ -z "${QUOTA}" ]; then

target/bin/updatemailuser

@@ -27,8 +27,7 @@ fi
 
 HASH="$(doveadm pw -s SHA512-CRYPT -u "${USER}" -p "${PASSWD}")"
 
-# Protect config file with lock to avoid race conditions
 touch "${DATABASE}"
-create_lock "$(basename "$0")"
+create_lock # Protect config file with lock to avoid race conditions
 grep -qi "^$(escape "${USER}")|" "${DATABASE}" 2>/dev/null || errex "User \"${USER}\" does not exist"
 sed -i "s ^""${USER}""|.* ""${USER}""|""${HASH}"" " "${DATABASE}"

target/scripts/check-for-changes.sh

@@ -6,8 +6,6 @@
 LOG_DATE=$(date +"%Y-%m-%d %H:%M:%S ")
 _notify 'task' "${LOG_DATE} Start check-for-changes script."
 
-SCRIPT_NAME="$(basename "$0")"
-
 # ? ––––––––––––––––––––––––––––––––––––––––––––– Checks
 
 cd /tmp/docker-mailserver || exit 1
@@ -40,9 +38,6 @@ while true
 do
   LOG_DATE=$(date +"%Y-%m-%d %H:%M:%S ")
 
-  # Lock configuration while working
-  create_lock "${SCRIPT_NAME}"
-
   # get chksum and check it, no need to lock config yet
   _monitored_files_checksums >"${CHKSUM_FILE}.new"
   cmp --silent -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new"
@@ -53,6 +48,7 @@ do
   if [ $? -eq 1 ]
   then
     _notify 'inf' "${LOG_DATE} Change detected"
+    create_lock # Shared config safety lock
     CHANGED=$(grep -Fxvf "${CHKSUM_FILE}" "${CHKSUM_FILE}.new" | sed 's/^[^ ]\+  //')
 
     # Bug alert! This overwrites the alias set by start-mailserver.sh
@@ -227,11 +223,12 @@ s/$/ regexp:\/etc\/postfix\/regexp/
 
     # prevent restart of dovecot when smtp_only=1
     [[ ${SMTP_ONLY} -ne 1 ]] && supervisorctl restart dovecot
+
+    remove_lock
   fi
 
   # mark changes as applied
   mv "${CHKSUM_FILE}.new" "${CHKSUM_FILE}"
-  remove_lock "${SCRIPT_NAME}"
 
   sleep 1
 done

target/scripts/helper-functions.sh

@@ -1,6 +1,8 @@
 #! /bin/bash
 
 DMS_DEBUG="${DMS_DEBUG:=0}"
+SCRIPT_NAME="$(basename "$0")" # This becomes the sourcing script name (Example: check-for-changes.sh)
+LOCK_ID="$(uuid)" # Used inside of lock files to identify them and prevent removal by other instances of docker-mailserver
 
 # ? --------------------------------------------- BIN HELPER
 
@@ -17,17 +19,33 @@ function escape
 
 function create_lock
 {
-    SCRIPT_NAME="$1"
-    LOCK_FILE="/tmp/docker-mailserver/${SCRIPT_NAME}.lock"
-    [[ -e "${LOCK_FILE}" ]] && errex "Lock file ${LOCK_FILE} exists. Another $1 execution is happening. Try again later."
-    trap remove_lock EXIT # This won't work if the script is, for example, check-for-changes.sh which uses a while loop to stay running; you'll need to include a remove_lock call at the end of your logic
-    touch "${LOCK_FILE}"
+  LOCK_FILE="/tmp/docker-mailserver/${SCRIPT_NAME}.lock"
+  while [[ -e "${LOCK_FILE}" ]]
+  do
+    _notify 'warn' "Lock file ${LOCK_FILE} exists. Another ${SCRIPT_NAME} execution is happening. Trying again shortly..."
+    # Handle stale lock files left behind on crashes
+    # or premature/non-graceful exits of containers while they're making changes
+    if [[ -n "$(find "${LOCK_FILE}" -mmin +1 2>/dev/null)" ]]
+    then
+      _notify 'warn' "Lock file older than 1 minute. Removing stale lock file."
+      rm -f "${LOCK_FILE}"
+      _notify 'inf' "Removed stale lock ${LOCK_FILE}."
+    fi
+    sleep 5
+  done
+  trap remove_lock EXIT
+  echo "${LOCK_ID}" > "${LOCK_FILE}"
 }
 
 function remove_lock
 {
-  SCRIPT_NAME=${SCRIPT_NAME:-$1}
-  rm -f "/tmp/docker-mailserver/${SCRIPT_NAME}.lock"
+  LOCK_FILE="${LOCK_FILE:-"/tmp/docker-mailserver/${SCRIPT_NAME}.lock"}"
+  [[ -z "${LOCK_ID}" ]] && errex "Cannot remove ${LOCK_FILE} as there is no LOCK_ID set"
+  if [[ -e "${LOCK_FILE}" && $(grep -c "${LOCK_ID}" "${LOCK_FILE}") -gt 0 ]] # Ensure we don't delete a lock that's not ours
+  then
+    rm -f "${LOCK_FILE}"
+    _notify 'inf' "Removed lock ${LOCK_FILE}."
+  fi
 }
 
 # ? ––––––––––––––––––––––––––––––––––––––––––––– IP & CIDR

test/helper-functions.bats

@@ -0,0 +1,43 @@
+load 'test_helper/common'
+
+function setup() {
+    run_setup_file_if_necessary
+}
+
+function teardown() {
+    run_teardown_file_if_necessary
+}
+
+function setup_file() {
+    local PRIVATE_CONFIG
+    PRIVATE_CONFIG="$(duplicate_config_for_container .)"
+    docker run -d --name mail_helper_functions \
+        --cap-add=NET_ADMIN \
+        -v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \
+        -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
+        -e ENABLE_FETCHMAIL=1 \
+        -e DMS_DEBUG=0 \
+        -h mail.my-domain.com -t "${NAME}"
+    wait_for_finished_setup_in_container mail_helper_functions
+}
+
+function teardown_file() {
+    docker rm -f mail_helper_functions
+}
+
+@test "first" {
+    skip 'this test must come first to reliably identify when to run setup_file'
+}
+
+@test "check helper-functions.sh: _sanitize_ipv4_to_subnet_cidr" {
+    run docker exec mail_helper_functions bash -c ". /usr/local/bin/helper-functions.sh; _sanitize_ipv4_to_subnet_cidr 255.255.255.255/0"
+    assert_output "0.0.0.0/0"
+    run docker exec mail_helper_functions bash -c ". /usr/local/bin/helper-functions.sh; _sanitize_ipv4_to_subnet_cidr 192.168.255.14/20"
+    assert_output "192.168.240.0/20"
+    run docker exec mail_helper_functions bash -c ". /usr/local/bin/helper-functions.sh; _sanitize_ipv4_to_subnet_cidr 192.168.255.14/32"
+    assert_output "192.168.255.14/32"
+}
+
+@test "last" {
+    skip 'this test is only there to reliably mark the end for the teardown_file'
+}

test/helper_functions.bats

@@ -1,15 +0,0 @@
-load 'test_helper/bats-support/load'
-load 'test_helper/bats-assert/load'
-
-# load the helper function into current context
-# shellcheck source=../target/scripts/helper-functions.sh
-. ./target/scripts/helper-functions.sh
-
-@test "check helper function: _sanitize_ipv4_to_subnet_cidr" {
-    output=$(_sanitize_ipv4_to_subnet_cidr 255.255.255.255/0)
-    assert_output "0.0.0.0/0"
-    output=$(_sanitize_ipv4_to_subnet_cidr 192.168.255.14/20)
-    assert_output "192.168.240.0/20"
-    output=$(_sanitize_ipv4_to_subnet_cidr 192.168.255.14/32)
-    assert_output "192.168.255.14/32"
-}

test/mail_changedetector.bats

@@ -0,0 +1,94 @@
+load 'test_helper/common'
+
+function setup() {
+  run_setup_file_if_necessary
+}
+
+function teardown() {
+  run_teardown_file_if_necessary
+}
+
+function setup_file() {
+  local PRIVATE_CONFIG
+  PRIVATE_CONFIG="$(duplicate_config_for_container . mail_changedetector_one)"
+
+  docker run -d --name mail_changedetector_one \
+  -v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \
+  -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
+  -e DMS_DEBUG=1 \
+  -h mail.my-domain.com -t "${NAME}"
+  wait_for_finished_setup_in_container mail_changedetector_one
+
+  docker run -d --name mail_changedetector_two \
+  -v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \
+  -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \
+  -e DMS_DEBUG=1 \
+  -h mail.my-domain.com -t "${NAME}"
+  wait_for_finished_setup_in_container mail_changedetector_two
+}
+
+function teardown_file() {
+  docker rm -f mail_changedetector_one
+  docker rm -f mail_changedetector_two
+}
+
+# this test must come first to reliably identify when to run setup_file
+@test "first" {
+  skip 'Starting testing of changedetector'
+}
+
+@test "checking changedetector: servers are ready" {
+  wait_for_service mail_changedetector_one changedetector
+  wait_for_service mail_changedetector_two changedetector
+}
+
+@test "checking changedetector: can detect changes & between two containers using same config" {
+  echo "" >> "$(private_config_path mail_changedetector_one)/postfix-accounts.cf"
+  sleep 15
+  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "postfix: stopped"
+  assert_output --partial "postfix: started"
+  assert_output --partial "Change detected"
+  assert_output --partial "Removed lock"
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "postfix: stopped"
+  assert_output --partial "postfix: started"
+  assert_output --partial "Change detected"
+  assert_output --partial "Removed lock"
+}
+
+@test "checking changedetector: lock file found, blocks, and doesn't get prematurely removed" {
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl stop changedetector"
+  docker exec mail_changedetector_one /bin/bash -c "touch /tmp/docker-mailserver/check-for-changes.sh.lock"
+  echo "" >> "$(private_config_path mail_changedetector_one)/postfix-accounts.cf"
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl start changedetector"
+  sleep 15
+  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "check-for-changes.sh.lock exists"
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "check-for-changes.sh.lock exists"
+  # Ensure starting a new check-for-changes.sh instance (restarting here) doesn't delete the lock
+  docker exec mail_changedetector_two /bin/bash -c "rm -f /var/log/supervisor/changedetector.log"
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl restart changedetector"
+  sleep 5
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl tail changedetector"
+  refute_output --partial "check-for-changes.sh.lock exists"
+  refute_output --partial "Removed lock"
+}
+
+@test "checking changedetector: lock stale and cleaned up" {
+  docker rm -f mail_changedetector_two
+  docker exec mail_changedetector_one /bin/bash -c "touch /tmp/docker-mailserver/check-for-changes.sh.lock"
+  echo "" >> "$(private_config_path mail_changedetector_one)/postfix-accounts.cf"
+  sleep 15
+  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "check-for-changes.sh.lock exists"
+  sleep 65
+  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail changedetector"
+  assert_output --partial "Removed stale lock"
+}
+
+# this test is only there to reliably mark the end for the teardown_file
+@test "last" {
+  skip 'Finished testing of changedetector'
+}