From 584577787af95887401f6922c703bb2edabf11b1 Mon Sep 17 00:00:00 2001 From: Brennan Kinney <5098581+polarathene@users.noreply.github.com> Date: Tue, 16 Nov 2021 10:50:45 +1300 Subject: [PATCH] refactor: Internal HOSTNAME and DOMAINNAME configuration (#2280) Better logical flow, handling and inline documentation. Despite the verbosity, it's better to make this visible here for maintenance and debugging purposes than trying to dig through issue/PR or commit history for it. * fix: Panic when HOSTNAME is misconfigured * chore: Add more comment docs for maintainers * tests(fix): Use `--domainname` not ENV `DOMAINNAME` Co-authored-by: Casper Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com> --- target/scripts/helper-functions.sh | 68 +++++++++++++++++++++++------- test/mail_hostname.bats | 14 +++--- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/target/scripts/helper-functions.sh b/target/scripts/helper-functions.sh index 22faf5e0..fc1f8031 100755 --- a/target/scripts/helper-functions.sh +++ b/target/scripts/helper-functions.sh @@ -288,27 +288,65 @@ export -f _monitored_files_checksums # ? --------------------------------------------- General +# Outputs the DNS label count (delimited by `.`) for the given input string. +# Useful for determining an FQDN like `mail.example.com` (3), vs `example.com` (2). +function _get_label_count +{ + awk -F '.' '{ print NF }' <<< "${1}" +} + +# Sets HOSTNAME and DOMAINNAME globals used throughout the scripts, +# and any subprocesses called that intereact with it. function _obtain_hostname_and_domainname { - if [[ -n "${OVERRIDE_HOSTNAME}" ]] + # Normally this value would match the output of `hostname` which mirrors `/proc/sys/kernel/hostname`, + # However for legacy reasons, the system ENV `HOSTNAME` was replaced here with `hostname -f` instead. + # + # TODO: Consider changing to `DMS_FQDN`; a more accurate name, and removing the `export`, assuming no + # subprocess like postconf would be called that would need access to the same value via `$HOSTNAME` ENV. + # + # TODO: `OVERRIDE_HOSTNAME` was introduced for non-Docker runtimes that could not configure an explicit hostname. + # k8s was the particular runtime in 2017. This does not update `/etc/hosts` or other locations, thus risking + # inconsistency with expected behaviour. Investigate if it's safe to remove support. (--net=host also uses this as a workaround) + export HOSTNAME="${OVERRIDE_HOSTNAME:-$(hostname -f)}" + + # If the container is misconfigured.. `hostname -f` (which derives it's return value from `/etc/hosts` or DNS query), + # will result in an error that returns an empty value. This warrants a panic. + if [[ -z ${HOSTNAME} ]] then - export HOSTNAME="${OVERRIDE_HOSTNAME}" - export DOMAINNAME="${DOMAINNAME:-${HOSTNAME#*.}}" - # Handle situations where the hostname is name.tld and hostname -d ends up just showing "tld" - if [[ ! "${DOMAINNAME}" =~ .*\..* ]] + dms_panic__misconfigured 'obtain_hostname' '/etc/hosts' + fi + + # If the `HOSTNAME` is more than 2 labels long (eg: mail.example.com), + # We take the FQDN from it, minus the 1st label (aka _short hostname_, `hostname -s`). + # + # TODO: For some reason we're explicitly separating out a domain name from our FQDN, + # `hostname -d` was probably not the correct command for this intention either. + # Needs further investigation for relevance, and if `/etc/hosts` is important for consumers + # of this variable or if a more deterministic approach with `cut` should be relied on. + if [[ $(_get_label_count "${HOSTNAME}") -gt 2 ]] + then + if [[ -n ${OVERRIDE_HOSTNAME} ]] then - DOMAINNAME="${HOSTNAME}" - fi - else - # These hostname commands will fail with "hostname: Name or service not known" - # if the hostname is not valid (important for tests) - HOSTNAME="$(hostname -f)" - DOMAINNAME="${DOMAINNAME:-$(hostname -d)}" - if [[ ! "${DOMAINNAME}" =~ .*\..* ]] - then - DOMAINNAME="${HOSTNAME}" + # Emulates the intended behaviour of `hostname -d`: + # Assign the HOSTNAME value minus everything up to and including the first `.` + DOMAINNAME=${HOSTNAME#*.} + else + # Operates on the FQDN returned from querying `/etc/hosts` or fallback DNS: + # + # Note if you want the actual NIS `domainname`, use the `domainname` command, + # or `cat /proc/sys/kernel/domainname`. + # Our usage of `domainname` is under consideration as legacy, and not advised + # going forward. In future our docs should drop any mention of it. + + #shellcheck disable=SC2034 + DOMAINNAME="$(hostname -d)" fi fi + + # Otherwise we assign the same value (eg: example.com): + # Not an else statement in the previous conditional in the event that `hostname -d` fails. + DOMAINNAME="${DOMAINNAME:-${HOSTNAME}}" } # Call this method when you want to panic (emit a 'FATAL' log level error, and exit uncleanly). diff --git a/test/mail_hostname.bats b/test/mail_hostname.bats index 3d13618e..ae64ec18 100644 --- a/test/mail_hostname.bats +++ b/test/mail_hostname.bats @@ -35,9 +35,9 @@ function setup_file() { -e PERMIT_DOCKER=network \ -e DMS_DEBUG=0 \ -e ENABLE_SRS=1 \ - -e SRS_DOMAINNAME=srs.my-domain.com \ - -e DOMAINNAME=my-domain.com \ - -h unknown.domain.tld \ + -e SRS_DOMAINNAME='srs.my-domain.com' \ + --domainname 'my-domain.com' \ + --hostname 'mail' \ -t "${NAME}" PRIVATE_CONFIG_FOUR="$(duplicate_config_for_container . mail_domainname)" @@ -47,14 +47,14 @@ function setup_file() { -e PERMIT_DOCKER=network \ -e DMS_DEBUG=0 \ -e ENABLE_SRS=1 \ - -e DOMAINNAME=my-domain.com \ - -h unknown.domain.tld \ + --domainname 'my-domain.com' \ + --hostname 'mail' \ -t "${NAME}" wait_for_smtp_port_in_container mail_override_hostname - + wait_for_smtp_port_in_container mail_non_subdomain_hostname - + wait_for_smtp_port_in_container mail_srs_domainname wait_for_smtp_port_in_container mail_domainname