Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: /var/mail-state should not symlink non-existing directories #4018

Merged
merged 9 commits into from
May 19, 2024

Conversation

polarathene
Copy link
Member

@polarathene polarathene commented May 16, 2024

Description

  • Log an error when the expected service state directory doesn't exist.
  • The location /var/lib/getmail/ doesn't seem like it should have been introduced. Drop it in favor of /tmp/docker-mailserver/getmail. It appears to be for storing remote mail that was retrieved if not configured to send to Dovecot like our docs advise. This location was never valid anyway (as referenced issue covers).
  • Additional getmail insights are covered in a later comment of mine below.
  • Fetchmail debug command force enabled the feature to avoid requiring the ENV. Probably before a release when we moved the setup CLI into the container itself. Since the feature setup script called already gates on the feature ENV toggle and outputs a debug log when disabled... don't implicitly enable the feature. (EDIT: Sorry lacking time to support this unrelated change due to a test case reliance on it, reverted)
Original message
  • The Getmail service doesn't have a directory in /var/lib/ by default, yet mail-state is configured to symlink it, and the service in DMS is configured to expect /var/lib/getmail exists.
  • Handled in mail-state logic to ensure it doesn't silently fail. (EDIT: Just realized this won't solve it due to the condition it's in 😂 )

It should probably skip the symlink in this case and omit an error to the logs? Quick fix would be to have the Getmail service startup script itself ensure the directory is configured.

Fixes #4015

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • If necessary, I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added information about changes made in this PR to CHANGELOG.md

…king

The Getmail service doesn't have a directory in `/var/lib/` by default, yet `mail-state` is configured to symlink it, and the service in DMS is configured to expect `/var/lib/getmail` exists.

Handled in `mail-state` logic to ensure it doesn't silently fail.
@polarathene polarathene added area/scripts kind/bug/fix A fix (PR) for a confirmed bug labels May 16, 2024
@polarathene polarathene added this to the v14.0.0 milestone May 16, 2024
@polarathene polarathene self-assigned this May 16, 2024
@polarathene polarathene marked this pull request as ready for review May 16, 2024 01:01
@georglauterbach
Copy link
Member

Existing tests are failing:

not ok 30 [Getmail] debug-getmail works as expected in 565ms
# (from function `assert_line' in file test/test_helper/bats-assert/src/assert_line.bash, line 219,
#  in test file test/tests/parallel/set1/getmail.bats, line 58)
#   `assert_line --regexp 'retriever:.*SimpleIMAPSSLRetriever\(ca_certs="None", certfile="None", getmaildir="\/tmp\/docker-mailserver\/getmail", imap_on_delete="None", imap_search="None", keyfile="None", mailboxes="\(.*INBOX.*\)", move_on_delete="None", ***, password_command="\(\)", port="993", record_mailbox="True", server="imap.remote-service.test", ssl_cert_hostname="None", ssl_ciphers="None", ssl_fingerprints="\(\)", ssl_version="None", timeout="180", use_cram_md5="False", use_kerberos="False", use_peek="True", use_xoauth2="False", username="user3"\)'' failed
#
# -- no output line matches regular expression --
# regexp : retriever:.*SimpleIMAPSSLRetriever\(ca_certs="None", certfile="None", getmaildir="\/tmp\/docker-mailserver\/getmail", imap_on_delete="None", imap_search="None", keyfile="None", mailboxes="\(.*INBOX.*\)", move_on_delete="None", ***, password_command="\(\)", port="993", record_mailbox="True", server="imap.remote-service.test", ssl_cert_hostname="None", ssl_ciphers="None", ssl_fingerprints="\(\)", ssl_version="None", timeout="180", use_cram_md5="False", use_kerberos="False", use_peek="True", use_xoauth2="False", username="user3"\)
# output (22 lines):
#     retriever:  SimpleIMAPSSLRetriever(ca_certs="None", certfile="None", getmaildir="/var/lib/getmail", imap_on_delete="None", imap_search="None", keyfile="None", mailboxes="('INBOX',)", move_on_delete="None", ***, password_command="()", port="993", record_mailbox="True", server="imap.remote-service.test", ssl_cert_hostname="None", ssl_ciphers="None", ssl_fingerprints="()", ssl_version="None", timeout="180", use_cram_md5="False", use_kerberos="False", use_peek="True", use_xoauth2="False", username="user3")
#     destination:  MDA_external(allow_root_commands="True", arguments="('-d', 'user3@example.test')", command="deliver", group="None", ignore_stderr="False", path="/usr/lib/dovecot/deliver", pipe_stdout="True", unixfrom="False", user="None")
#     options:
#       delete : False
#       delete_after : 0
#       delete_bigger_than : 0
#       delivered_to : False
#       fingerprint : False
#       logfile : logfile(filename="/var/log/mail/getmail-user3.log")
#       max_bytes_per_session : 0
#       max_message_size : 0
#       max_messages_per_session : 500
#       message_log : /var/log/mail/getmail-user3.log
#       message_log_syslog : False
#       message_log_verbose : False
#       netrc_file : None
#       read_all : False
#       received : False
#       skip_imap_fetch_size : False
#       to_oldmail_on_each_mail : False
#       use_netrc : False
#       verbose : 0
# --
#

I have no idea why the EC lint is failing, though..

@polarathene
Copy link
Member Author

TL;DR: Seems that getmail should not be configured for /var/mail-state and this got missed during review as it was likened to fetchmail which is setup differently as a service (although it may technically not need a state dir either for all I know 🤷‍♂️ )


Existing tests are failing

This is due to the logic here:

if [[ -d /var/lib/getmail ]]; then
GETMAILDIR=/var/lib/getmail
else
mkdir -p /tmp/docker-mailserver/getmail
GETMAILDIR=/tmp/docker-mailserver/getmail
fi

Previously since the internal directory never existed in the first place (except I guess as a symlink to nothing) it would fallback to the config volume and configure for that instead...

I guess we missed that during review 🤷‍♂️ It shouldn't store data at locations for different concerns? (runtime state that's usually discardable vs config volume that is generally used to persist anything else)


The cronjob (for runtime usage) has the /var/lib/getmail location hard coded (whereas the debug setup command above offers the alternative /tmp/docker-mailserver path):

for FILE in /etc/getmailrc.d/getmailrc*; do
if ! pgrep -f "${FILE}$" &>/dev/null; then
getmail --getmaildir /var/lib/getmail --rcfile "${FILE}"
fi
done


More context

https://getmail6.org/configuration.html#running-commandline-options

To use getmail, simply run the script getmail, which is typically installed in /usr/local/bin/ by default.
getmail will read the default getmail rc file (getmailrc) from the default configuration/data directory (~/.getmail/) and begin operating.

--getmaildir=DIR or -gDIR — use DIR for configuration and data files

--rcfile=FILE or -rFILE — read getmail rc file FILE instead of the default.

  • The file path is assumed to be relative to the getmaildir directory unless this value starts with a slash (/).
  • This option can be given multiple times to have getmail retrieve mail from multiple accounts.

Unlike fetchmail, there is no daemon service or polling, thus the cronjob:

RC files are provided via the config volume and copied over to internal location in /etc:

# Generate getmailrc configs, starting with the `/etc/getmailrc_general` base config,
# Add a unique `message_log` config, then append users own config to the end.
for FILE in /tmp/docker-mailserver/getmail-*.cf; do

These all have a base config (which our docs advise mounting directly over instead of supporting via the config volume) but to be practical with DMS needs to configure to deliver over to Dovecot:

[retriever]
type = SimpleIMAPSSLRetriever
server = imap.remote-service.test
username = user3
password=secret
[destination]
type = MDA_external
path = /usr/lib/dovecot/deliver
allow_root_commands = true
arguments =("-d","user3@example.test")

I'm not sure why this even needs to be in DMS beyond convenience, there isn't anything specific about it to integrate from the looks of it. Although that's probably the same case with fetchmail. Although there's this interesting FAQ entry from the original getmail author criticizing fetchmail.


getmaildir in this case is a location to dump the retrieved remote mail if you haven't configured it to deliver to Dovecot (opt-in). We don't have anything that seems to suggest using /tmp/docker-mailserver/getmail other than in the debug script itself (and test case matching the debug output).

So any existing users of the feature either have had mail delivered to Dovecot as per guidance in the docs, or it's been stored in /var/mail-state/lib-getmail (but we don't document this expectation, and the associated volume isn't really intended for it as it's not runtime state related). The getmail command is run as root UID/GID AFAIK? There's really no value for it being handled in /var/mail-state then.

@polarathene polarathene merged commit ed669bd into master May 19, 2024
7 checks passed
@polarathene polarathene deleted the fix/ensure-mkdir-before-symlinking-mail-state branch May 19, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scripts kind/bug/fix A fix (PR) for a confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug report: getmail not set up
2 participants