From dff7e428c0ba11e762206db57363dbf4f0272e00 Mon Sep 17 00:00:00 2001 From: Nathan Pierce Date: Sat, 28 Aug 2021 19:16:34 -0400 Subject: [PATCH] Revert "check-for-changes: performance improvements + wait for settle (#2104)" This reverts commit 232d463b81d475cef4755b410e2e8861b3e1b6c5. --- Makefile | 2 + target/bin/addmailuser | 10 + target/bin/delmailuser | 1 + target/scripts/check-for-changes.sh | 373 +++++++++++++--------------- target/scripts/postfix-wrapper.sh | 6 +- test/bats | 2 +- test/mail_ssl_letsencrypt.bats | 4 +- test/mail_with_relays.bats | 4 +- test/open_dkim.bats | 2 +- 9 files changed, 188 insertions(+), 216 deletions(-) diff --git a/Makefile b/Makefile index 15f11a47..f717e39e 100644 --- a/Makefile +++ b/Makefile @@ -4,6 +4,8 @@ NAME ?= mailserver-testing:ci VCS_REF = $(shell git rev-parse --short HEAD) VCS_VER = $(shell git describe --tags --contains --always) +export CDIR = $(shell pwd) + # ----------------------------------------------- # --- Generic Build Targets --------------------- # ----------------------------------------------- diff --git a/target/bin/addmailuser b/target/bin/addmailuser index a2c72286..0a0a7f76 100755 --- a/target/bin/addmailuser +++ b/target/bin/addmailuser @@ -63,3 +63,13 @@ echo "${FULL_EMAIL}|${HASH}" >> "${DATABASE}" USER="${FULL_EMAIL%@*}" DOMAIN="${FULL_EMAIL#*@}" + +# Tests fail if the creation of /var/mail/${DOMAIN}/${USER} doesn't happen fast enough after addmailuser executes (check-for-changes.sh race-condition) +if [[ -e "/tmp/docker-mailserver-config-chksum" ]] # Prevent infinite loop in tests like "checking accounts: user3 should have been added to /tmp/docker-mailserver/postfix-accounts.cf even when that file does not exist" +then + while [[ ! -d "/var/mail/${DOMAIN}/${USER}" ]] + do + echo "Waiting for dovecot to create /var/mail/${DOMAIN}/${USER}..." + sleep 1 + done +fi diff --git a/target/bin/delmailuser b/target/bin/delmailuser index 892fe27d..8c2cdbb3 100755 --- a/target/bin/delmailuser +++ b/target/bin/delmailuser @@ -154,6 +154,7 @@ use 'sudo docker exec mailserver rm -R /var/mail/${DOMAIN}/${USER}'" rmdir "/var/mail/${DOMAIN}" 2>/dev/null || true else echo "Mailbox directory '/var/mail/${DOMAIN}/${USER}' did not exist." >&2 + ERROR=true fi ${ERROR} && errex 'See the messages above.' diff --git a/target/scripts/check-for-changes.sh b/target/scripts/check-for-changes.sh index f2aee609..f089a5cc 100755 --- a/target/scripts/check-for-changes.sh +++ b/target/scripts/check-for-changes.sh @@ -41,9 +41,6 @@ PM_ADDRESS="${POSTMASTER_ADDRESS:=postmaster@${DOMAINNAME}}" _notify 'inf' "${LOG_DATE} Using postmaster address ${PM_ADDRESS}" sleep 10 -CMP_RESULT= -LAST_CMP_RESULT= - while true do LOG_DATE=$(date +"%Y-%m-%d %H:%M:%S ") @@ -53,231 +50,195 @@ do # get chksum and check it, no need to lock config yet _monitored_files_checksums >"${CHKSUM_FILE}.new" - CMP_RESULT=$(cmp -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new") - if [[ -n "${CMP_RESULT}" ]] # If there is a difference between checksum files + cmp --silent -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new" + # cmp return codes + # 0 – files are identical + # 1 – files differ + # 2 – inaccessible or missing argument + if [ $? -eq 1 ] then - # If the current run's CMP_RESULT is equal to the LAST_CMP_RESULT, changes have settled and we can safely proceed - # This prevents multiple restarts of postfix from happening all at once and causing problems or delays - if [[ "${CMP_RESULT}" == "${LAST_CMP_RESULT}" ]] + _notify 'inf' "${LOG_DATE} Change detected" + CHANGED=$(grep -Fxvf "${CHKSUM_FILE}" "${CHKSUM_FILE}.new" | sed 's/^[^ ]\+ //') + + # Bug alert! This overwrites the alias set by start-mailserver.sh + # Take care that changes in one script are propagated to the other + + # ! NEEDS FIX ----------------------------------------- + # TODO FIX -------------------------------------------- + # ! NEEDS EXTENSIONS ---------------------------------- + # TODO Perform updates below conditionally too -------- + # Also note that changes are performed in place and are not atomic + # We should fix that and write to temporary files, stop, swap and start + + for FILE in ${CHANGED} + do + case "${FILE}" in + "/etc/letsencrypt/acme.json" ) + for CERTDOMAIN in ${SSL_DOMAIN} ${HOSTNAME} ${DOMAINNAME} + do + _extract_certs_from_acme "${CERTDOMAIN}" && break + done + ;; + + * ) + _notify 'warn' 'File not found for certificate in check_for_changes.sh' + ;; + + esac + done + + # regenerate postix aliases + echo "root: ${PM_ADDRESS}" >/etc/aliases + if [[ -f /tmp/docker-mailserver/postfix-aliases.cf ]] then - _notify 'inf' "${LOG_DATE} Changes to checksum files settled" - CHANGED=$(grep -Fxvf "${CHKSUM_FILE}" "${CHKSUM_FILE}.new" | sed 's/^[^ ]\+ //') - WAIT_FOR_PIDS=() - # Bug alert! This overwrites the alias set by start-mailserver.sh - # Take care that changes in one script are propagated to the other + cat /tmp/docker-mailserver/postfix-aliases.cf >>/etc/aliases + fi + postalias /etc/aliases - # ! NEEDS FIX ----------------------------------------- - # TODO FIX -------------------------------------------- - # ! NEEDS EXTENSIONS ---------------------------------- - # TODO Perform updates below conditionally too -------- - # Also note that changes are performed in place and are not atomic - # We should fix that and write to temporary files, stop, swap and start + # regenerate postfix accounts + : >/etc/postfix/vmailbox + : >/etc/dovecot/userdb - for FILE in ${CHANGED} - do - case "${FILE}" in - "/etc/letsencrypt/acme.json" ) - { - for CERTDOMAIN in ${SSL_DOMAIN} ${HOSTNAME} ${DOMAINNAME} - do - _extract_certs_from_acme "${CERTDOMAIN}" && break - done - } & - WAIT_FOR_PIDS+=($!) - ;; + if [[ -f /tmp/docker-mailserver/postfix-accounts.cf ]] && [[ ${ENABLE_LDAP} -ne 1 ]] + then + sed -i 's/\r//g' /tmp/docker-mailserver/postfix-accounts.cf + echo "# WARNING: this file is auto-generated. Modify config/postfix-accounts.cf to edit user list." >/etc/postfix/vmailbox - * ) - _notify 'warn' 'File not found for certificate in check_for_changes.sh' - ;; - - esac - done - - # regenerate postix aliases - { - echo "root: ${PM_ADDRESS}" >/etc/aliases - if [[ -f /tmp/docker-mailserver/postfix-aliases.cf ]] - then - cat /tmp/docker-mailserver/postfix-aliases.cf >>/etc/aliases - fi - postalias /etc/aliases - } & - WAIT_FOR_PIDS+=($!) - - # regenerate postfix accounts - { - : >/etc/postfix/vmailbox - : >/etc/dovecot/userdb - } & - WAIT_FOR_PIDS+=($!) - - if [[ -f /tmp/docker-mailserver/postfix-accounts.cf ]] && [[ ${ENABLE_LDAP} -ne 1 ]] - then - sed -i 's/\r//g' /tmp/docker-mailserver/postfix-accounts.cf - echo "# WARNING: this file is auto-generated. Modify config/postfix-accounts.cf to edit user list." >/etc/postfix/vmailbox - - # Checking that /tmp/docker-mailserver/postfix-accounts.cf ends with a newline - # shellcheck disable=SC1003 - sed -i -e '$a\' /tmp/docker-mailserver/postfix-accounts.cf - chown dovecot:dovecot /etc/dovecot/userdb - chmod 640 /etc/dovecot/userdb - sed -i -e '/\!include auth-ldap\.conf\.ext/s/^/#/' /etc/dovecot/conf.d/10-auth.conf - sed -i -e '/\!include auth-passwdfile\.inc/s/^#//' /etc/dovecot/conf.d/10-auth.conf - - # rebuild relay host - if [[ -n ${RELAY_HOST} ]] - then - # keep old config - : >/etc/postfix/sasl_passwd - if [[ -n ${SASL_PASSWD} ]] - then - echo "${SASL_PASSWD}" >>/etc/postfix/sasl_passwd - fi - - # add domain-specific auth from config file - if [[ -f /tmp/docker-mailserver/postfix-sasl-password.cf ]] - then - while read -r LINE - do - if ! grep -q -e "\s*#" <<< "${LINE}" - then - echo "${LINE}" >>/etc/postfix/sasl_passwd - fi - done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-sasl-password.cf || true) - fi - - # add default relay - if [[ -n "${RELAY_USER}" ]] && [[ -n "${RELAY_PASSWORD}" ]] - then - echo "[${RELAY_HOST}]:${RELAY_PORT} ${RELAY_USER}:${RELAY_PASSWORD}" >>/etc/postfix/sasl_passwd - fi - fi - - # creating users ; 'pass' is encrypted - # comments and empty lines are ignored - while IFS=$'|' read -r LOGIN PASS USER_ATTRIBUTES - do - USER=$(echo "${LOGIN}" | cut -d @ -f1) - DOMAIN=$(echo "${LOGIN}" | cut -d @ -f2) - - # test if user has a defined quota - if [[ -f /tmp/docker-mailserver/dovecot-quotas.cf ]] - then - declare -a USER_QUOTA - IFS=':' ; read -r -a USER_QUOTA < <(grep "${USER}@${DOMAIN}:" -i /tmp/docker-mailserver/dovecot-quotas.cf) - unset IFS - - [[ ${#USER_QUOTA[@]} -eq 2 ]] && USER_ATTRIBUTES="${USER_ATTRIBUTES} userdb_quota_rule=*:bytes=${USER_QUOTA[1]}" - fi - - echo "${LOGIN} ${DOMAIN}/${USER}/" >>/etc/postfix/vmailbox - - # user database for dovecot has the following format: - # user:password:uid:gid:(gecos):home:(shell):extra_fields - # example : - # ${LOGIN}:${PASS}:5000:5000::/var/mail/${DOMAIN}/${USER}::userdb_mail=maildir:/var/mail/${DOMAIN}/${USER} - echo "${LOGIN}:${PASS}:5000:5000::/var/mail/${DOMAIN}/${USER}::${USER_ATTRIBUTES}" >>/etc/dovecot/userdb - mkdir -p "/var/mail/${DOMAIN}/${USER}" - - if [[ -e /tmp/docker-mailserver/${LOGIN}.dovecot.sieve ]] - then - cp "/tmp/docker-mailserver/${LOGIN}.dovecot.sieve" "/var/mail/${DOMAIN}/${USER}/.dovecot.sieve" - fi - - echo "${DOMAIN}" >>/tmp/vhost.tmp - done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-accounts.cf) - fi + # Checking that /tmp/docker-mailserver/postfix-accounts.cf ends with a newline + # shellcheck disable=SC1003 + sed -i -e '$a\' /tmp/docker-mailserver/postfix-accounts.cf + chown dovecot:dovecot /etc/dovecot/userdb + chmod 640 /etc/dovecot/userdb + sed -i -e '/\!include auth-ldap\.conf\.ext/s/^/#/' /etc/dovecot/conf.d/10-auth.conf + sed -i -e '/\!include auth-passwdfile\.inc/s/^#//' /etc/dovecot/conf.d/10-auth.conf + # rebuild relay host if [[ -n ${RELAY_HOST} ]] then - _populate_relayhost_map & - WAIT_FOR_PIDS+=($!) - fi - - if [[ -f /etc/postfix/sasl_passwd ]] - then - { - chown root:root /etc/postfix/sasl_passwd - chmod 0600 /etc/postfix/sasl_passwd - } & - WAIT_FOR_PIDS+=($!) - fi - - if [[ -f postfix-virtual.cf ]] - then - # regenerate postfix aliases - : >/etc/postfix/virtual - : >/etc/postfix/regexp - - if [[ -f /tmp/docker-mailserver/postfix-virtual.cf ]] + # keep old config + : >/etc/postfix/sasl_passwd + if [[ -n ${SASL_PASSWD} ]] then - cp -f /tmp/docker-mailserver/postfix-virtual.cf /etc/postfix/virtual - - # the `to` seems to be important; don't delete it - # shellcheck disable=SC2034 - while read -r FROM TO - do - UNAME=$(echo "${FROM}" | cut -d @ -f1) - DOMAIN=$(echo "${FROM}" | cut -d @ -f2) - - # if they are equal it means the line looks like: "user1 other@domain.tld" - [ "${UNAME}" != "${DOMAIN}" ] && echo "${DOMAIN}" >>/tmp/vhost.tmp - done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-virtual.cf || true) + echo "${SASL_PASSWD}" >>/etc/postfix/sasl_passwd fi - if [[ -f /tmp/docker-mailserver/postfix-regexp.cf ]] + # add domain-specific auth from config file + if [[ -f /tmp/docker-mailserver/postfix-sasl-password.cf ]] then - cp -f /tmp/docker-mailserver/postfix-regexp.cf /etc/postfix/regexp - sed -i -e '/^virtual_alias_maps/{ + while read -r LINE + do + if ! grep -q -e "\s*#" <<< "${LINE}" + then + echo "${LINE}" >>/etc/postfix/sasl_passwd + fi + done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-sasl-password.cf || true) + fi + + # add default relay + if [[ -n "${RELAY_USER}" ]] && [[ -n "${RELAY_PASSWORD}" ]] + then + echo "[${RELAY_HOST}]:${RELAY_PORT} ${RELAY_USER}:${RELAY_PASSWORD}" >>/etc/postfix/sasl_passwd + fi + fi + + # creating users ; 'pass' is encrypted + # comments and empty lines are ignored + while IFS=$'|' read -r LOGIN PASS USER_ATTRIBUTES + do + USER=$(echo "${LOGIN}" | cut -d @ -f1) + DOMAIN=$(echo "${LOGIN}" | cut -d @ -f2) + + # test if user has a defined quota + if [[ -f /tmp/docker-mailserver/dovecot-quotas.cf ]] + then + declare -a USER_QUOTA + IFS=':' ; read -r -a USER_QUOTA < <(grep "${USER}@${DOMAIN}:" -i /tmp/docker-mailserver/dovecot-quotas.cf) + unset IFS + + [[ ${#USER_QUOTA[@]} -eq 2 ]] && USER_ATTRIBUTES="${USER_ATTRIBUTES} userdb_quota_rule=*:bytes=${USER_QUOTA[1]}" + fi + + echo "${LOGIN} ${DOMAIN}/${USER}/" >>/etc/postfix/vmailbox + + # user database for dovecot has the following format: + # user:password:uid:gid:(gecos):home:(shell):extra_fields + # example : + # ${LOGIN}:${PASS}:5000:5000::/var/mail/${DOMAIN}/${USER}::userdb_mail=maildir:/var/mail/${DOMAIN}/${USER} + echo "${LOGIN}:${PASS}:5000:5000::/var/mail/${DOMAIN}/${USER}::${USER_ATTRIBUTES}" >>/etc/dovecot/userdb + mkdir -p "/var/mail/${DOMAIN}/${USER}" + + if [[ -e /tmp/docker-mailserver/${LOGIN}.dovecot.sieve ]] + then + cp "/tmp/docker-mailserver/${LOGIN}.dovecot.sieve" "/var/mail/${DOMAIN}/${USER}/.dovecot.sieve" + fi + + echo "${DOMAIN}" >>/tmp/vhost.tmp + done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-accounts.cf) + fi + + [[ -n ${RELAY_HOST} ]] && _populate_relayhost_map + + + if [[ -f /etc/postfix/sasl_passwd ]] + then + chown root:root /etc/postfix/sasl_passwd + chmod 0600 /etc/postfix/sasl_passwd + fi + + if [[ -f postfix-virtual.cf ]] + then + # regenerate postfix aliases + : >/etc/postfix/virtual + : >/etc/postfix/regexp + + if [[ -f /tmp/docker-mailserver/postfix-virtual.cf ]] + then + cp -f /tmp/docker-mailserver/postfix-virtual.cf /etc/postfix/virtual + + # the `to` seems to be important; don't delete it + # shellcheck disable=SC2034 + while read -r FROM TO + do + UNAME=$(echo "${FROM}" | cut -d @ -f1) + DOMAIN=$(echo "${FROM}" | cut -d @ -f2) + + # if they are equal it means the line looks like: "user1 other@domain.tld" + [ "${UNAME}" != "${DOMAIN}" ] && echo "${DOMAIN}" >>/tmp/vhost.tmp + done < <(grep -v "^\s*$\|^\s*\#" /tmp/docker-mailserver/postfix-virtual.cf || true) + fi + + if [[ -f /tmp/docker-mailserver/postfix-regexp.cf ]] + then + cp -f /tmp/docker-mailserver/postfix-regexp.cf /etc/postfix/regexp + sed -i -e '/^virtual_alias_maps/{ s/ regexp:.*// s/$/ regexp:\/etc\/postfix\/regexp/ }' /etc/postfix/main.cf - fi fi - - # shellcheck disable=SC2044 - for USER_MAIL_LOCATION in $(find /var/mail -maxdepth 3 -a \( \! -user 5000 -o \! -group 5000 \)); do - chown -R 5000:5000 "${USER_MAIL_LOCATION}" & - WAIT_FOR_PIDS+=($!) - done - - # relies on if [[ -f postfix-virtual.cf ]] block - if [[ -f /tmp/vhost.tmp ]] - then - sort < /tmp/vhost.tmp | uniq >/etc/postfix/vhost - rm /tmp/vhost.tmp - fi - - # shellcheck disable=SC2086 - wait ${WAIT_FOR_PIDS[*]} - - supervisorctl restart postfix & - wait $! - # prevent restart of dovecot when smtp_only=1 - if [[ ${SMTP_ONLY} -ne 1 ]] - then - supervisorctl restart dovecot & - wait $! - fi - - LAST_CMP_RESULT= - CMP_RESULT= - - # mark changes as applied - mv "${CHKSUM_FILE}.new" "${CHKSUM_FILE}" - - else # The files differ... - _notify 'inf' "${LOG_DATE} Changes to checksum files detected... Ensuring changes have settled before proceeding..." - LAST_CMP_RESULT="${CMP_RESULT}" - sleep 5 # The longer the sleep here, the more time there is for users to make changes before a full restart fi - else # Checksum files don't differ - LAST_CMP_RESULT= + if [[ -f /tmp/vhost.tmp ]] + then + sort < /tmp/vhost.tmp | uniq >/etc/postfix/vhost + rm /tmp/vhost.tmp + fi + + if find /var/mail -maxdepth 3 -a \( \! -user 5000 -o \! -group 5000 \) | read -r + then + chown -R 5000:5000 /var/mail + fi + + supervisorctl restart postfix + + # prevent restart of dovecot when smtp_only=1 + [[ ${SMTP_ONLY} -ne 1 ]] && supervisorctl restart dovecot fi + # mark changes as applied + mv "${CHKSUM_FILE}.new" "${CHKSUM_FILE}" remove_lock "${SCRIPT_NAME}" + sleep 1 done exit 0 diff --git a/target/scripts/postfix-wrapper.sh b/target/scripts/postfix-wrapper.sh index 058dadf6..3a82c1a7 100755 --- a/target/scripts/postfix-wrapper.sh +++ b/target/scripts/postfix-wrapper.sh @@ -20,11 +20,11 @@ trap "service postfix stop" SIGINT trap "service postfix stop" SIGTERM trap "service postfix reload" SIGHUP -service postfix start & -wait +service postfix start +sleep 5 # wait until postfix is dead (triggered by trap) while kill -0 "$(< /var/spool/postfix/pid/master.pid)" do - sleep 1 + sleep 5 done diff --git a/test/bats b/test/bats index 9f27e3c4..54e965fa 160000 --- a/test/bats +++ b/test/bats @@ -1 +1 @@ -Subproject commit 9f27e3c47d70ad5aa99e4674593155652c195344 +Subproject commit 54e965fa9d269c2b3ff9036d81f32bac3df0edea diff --git a/test/mail_ssl_letsencrypt.bats b/test/mail_ssl_letsencrypt.bats index f4017501..36b3e8df 100644 --- a/test/mail_ssl_letsencrypt.bats +++ b/test/mail_ssl_letsencrypt.bats @@ -113,11 +113,9 @@ function teardown_file() { cp "$(private_config_path mail_lets_acme_json)/letsencrypt/acme-changed.json" "$(private_config_path mail_lets_acme_json)/acme.json" sleep 11 run docker exec mail_lets_acme_json /bin/bash -c "supervisorctl tail changedetector" - assert_output --partial "Changes to checksum files detected" - sleep 6 - run docker exec mail_lets_acme_json /bin/bash -c "supervisorctl tail changedetector" assert_output --partial "postfix: stopped" assert_output --partial "postfix: started" + assert_output --partial "Change detected" run docker exec mail_lets_acme_json /bin/bash -c "cat /etc/letsencrypt/live/mail.my-domain.com/key.pem" assert_output "$(cat "$(private_config_path mail_lets_acme_json)/letsencrypt/changed/key.pem")" diff --git a/test/mail_with_relays.bats b/test/mail_with_relays.bats index c02a80ca..1c7c3e70 100644 --- a/test/mail_with_relays.bats +++ b/test/mail_with_relays.bats @@ -51,7 +51,7 @@ function teardown_file() { run docker exec mail_with_relays grep -e domainzero.tld /etc/postfix/relayhost_map assert_output '' run ./setup.sh -c mail_with_relays email add user0@domainzero.tld password123 - run_until_success_or_timeout 15 docker exec mail_with_relays grep -e domainzero.tld /etc/postfix/relayhost_map + run_until_success_or_timeout 10 docker exec mail_with_relays grep -e domainzero.tld /etc/postfix/relayhost_map assert_output -e '^@domainzero.tld[[:space:]]+\[default.relay.com\]:2525$' } @@ -59,7 +59,7 @@ function teardown_file() { run docker exec mail_with_relays grep -e domain2.tld /etc/postfix/relayhost_map assert_output '' run ./setup.sh -c mail_with_relays alias add user2@domain2.tld user2@domaintwo.tld - run_until_success_or_timeout 15 docker exec mail_with_relays grep -e domain2.tld /etc/postfix/relayhost_map + run_until_success_or_timeout 10 docker exec mail_with_relays grep -e domain2.tld /etc/postfix/relayhost_map assert_output -e '^@domain2.tld[[:space:]]+\[default.relay.com\]:2525$' } diff --git a/test/open_dkim.bats b/test/open_dkim.bats index b8370063..c5e3a630 100644 --- a/test/open_dkim.bats +++ b/test/open_dkim.bats @@ -21,7 +21,7 @@ function setup_file --name "${CONTAINER_NAME}" \ --cap-add=SYS_PTRACE \ -v "${PRIVATE_CONFIG}":/tmp/docker-mailserver \ - -v "$(pwd)/test/test-files":/tmp/docker-mailserver-test:ro \ + -v "${CDIR}/test/test-files":/tmp/docker-mailserver-test:ro \ -e DEFAULT_RELAY_HOST=default.relay.host.invalid:25 \ -e PERMIT_DOCKER=host \ -e DMS_DEBUG=0 \