refactor: `relay.sh` by polarathene · Pull Request #3845 · docker-mailserver/docker-mailserver

polarathene · 2024-01-28T07:32:45Z

Description

While revising the docs on this feature, I was reminded how frustratingly awkward the current implementation is 😮‍💨

So I've detoured to make some changes there:

relay.sh: Much better maintainer comments + simplified logic + fixes.
Partially contributes towards [TODO]: Refactor Relay host support #3607
Minor revisions to check-for-changes.sh + postfix.sh, primarily VHOST_UPDATED addition + comment revisions.
Additionally allows DEFAULT_RELAY_HOST to support credentials without redundantly needing RELAY_HOST, which should better help cater to this recent use-case bug report (the opposite, RELAY_HOST when set enforced credentials).

Changes have been staged out into scoped commits with individual commit messages for added context. If you want an easier set of diffs to review through, that'd be a nicer experience to work through 👍

Two other docs pages for the relay host feature will be revised as a separate follow-up PR.

Context

For reference my prior refactor on this functionality was in May 2022 where full history of prior contributions was documented.
In a past discussion (Oct 2023):
- I advised avoiding the RELAY_PASSWORD ENV for credentials and just handling that directly in the user configuration file instead.
- My response there also provides a breakdown of the current relay.sh implementation prior to this PR.
Future work to support relays via implicit TLS (port 465) has config changes required covered here (and also now better described in relay.sh comments) and this follow-up comment
A compose.yaml local test environment with several DMS instances configured for relaying is available:
- Recent basic example, single compose.yaml file: Setting RELAY_HOST forces expectation to provide relay credentials #3842 (comment)
- 2022 compose.yaml + multiple files for CoreDNS config: relaying: mails are not relayed with implicit tls (465) / improve scripts & configuration #2601 (comment)
- Revised version of the 2022 example (adapted to the reported issue): [FR] Send-only domains #2813 (comment)

Type of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
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

The functionality is effectively the same for the two configs for the most part when it comes to parsing out a domain from the target value. Virtual aliases is more flexible in value, which may not have a domain-part present (manual user edit).

- Moves the "handle changes" logic into it's own scoped function, out of the main change detection loop logic. - This will be benefit a future commit change that will rely on `VHOST_UPDATED=1`.

- Better phrasing of the current logic comments. - Regex patterns assigned to variables (easier to grok intention) - Bulk of the logic for generating `/etc/postfix/relayhost_map` wrapped into a separate function with Postfix config setting handled separately.

- Split the two distinct features that configure `/etc/postfix/relayhost_map` into separate functions (_`MATCH_VALID` var no longer needed for legacy support_). - Instead of extracting domains from `postfix-accounts.cf` + `postfix-virtual.cf`, this has already been handled at `/etc/postfix/vhost`, sourcing from there is far less complicated. - Rename loop var `DOMAIN_PART`to `SENDER_DOMAIN` for better context of what it represents when appended to the config file. - Revised maintenance notes + guidance towards a future refactor of this relayhost feature support.

- Remove comment regarding `smtp_sasl_password_maps = static:${RELAY_USER}:${RELAY_PASSWORD}`, it could be used but `main.cf` presently has `644` permissions vs the `sasl_passwd` file permissions of `600`, less secure at preventing leaking of secrets (ignoring the ENV exposure itself). - Move the `main.cf` settings specific to relayhost credentials support / security into to the relevant function scope instead. This also allows for the configuration to be applied by a change detection event without container restart requirement. - Outer functions for setup and change detection to call have a clearer config dependency guard, as does the `_legacy_support()`. - These changes now support `DEFAULT_RELAY_HOST` to leverage the relay credentials ENV as well. - `DATABASE_RELAYHOSTS` is available in scope to the functions called here that reference it.

polarathene · 2024-01-28T23:25:28Z

If necessary I have added tests that prove my fix is effective or that my feature works

Tests are a bit lacking for this feature presently, although I'm fairly confident the PR isn't doing anything not covered in the changelog.

Better quality guidance on configuring relay hosts.

polarathene · 2024-01-29T02:08:39Z

ENV docs visual comparison

Before

After

georglauterbach · 2024-01-29T09:26:40Z

I will review in an hour or so 🚀

georglauterbach

Overall LGTM 👍🏼 Much cleaner to read now! I have some nitpicks in the scripts for you to address still :)

Co-authored-by: Georg Lauterbach <[email protected]>

github-actions · 2024-01-30T20:50:32Z

Documentation preview for this PR is ready! 🎉

Built with commit: 59f5638

polarathene added service/postfix area/scripts area/features kind/improvement Improve an existing feature, configuration file or the documentation kind/bug/fix A fix (PR) for a confirmed bug labels Jan 28, 2024

polarathene added this to the v14.0.0 milestone Jan 28, 2024

polarathene self-assigned this Jan 28, 2024

polarathene added 2 commits January 29, 2024 11:28

chore: relay.sh helper - Reference user config paths via variables

569dd60

polarathene force-pushed the docs/refactor-relayhosts-feature branch from 012815c to 366da36 Compare January 28, 2024 22:30

polarathene added 5 commits January 29, 2024 11:45

chore: check-for-change.sh - Support VHOST change visibility

9eabef9

- Moves the "handle changes" logic into it's own scoped function, out of the main change detection loop logic. - This will be benefit a future commit change that will rely on `VHOST_UPDATED=1`.

docs: relay.sh - Additional comment revisions

870635b

polarathene force-pushed the docs/refactor-relayhosts-feature branch from 366da36 to 02ec9d7 Compare January 28, 2024 23:18

polarathene requested review from casperklein and georglauterbach January 28, 2024 23:23

polarathene commented Jan 28, 2024

View reviewed changes

Comment thread target/scripts/helpers/relay.sh Outdated

polarathene commented Jan 29, 2024

View reviewed changes

Comment thread docs/content/config/environment.md

polarathene added 2 commits January 29, 2024 13:27

docs: Revised ENV docs on relay host config

a0ff86d

Better quality guidance on configuring relay hosts.

chore: Add entry to CHANGELOG.md

a5feec5

polarathene force-pushed the docs/refactor-relayhosts-feature branch from 44c5da0 to a5feec5 Compare January 29, 2024 00:27

polarathene mentioned this pull request Jan 29, 2024

[TODO]: Refactor Relay host support #3607

Open

polarathene commented Jan 29, 2024

View reviewed changes

Comment thread target/scripts/helpers/postfix.sh

georglauterbach requested changes Jan 29, 2024

View reviewed changes

Apply suggestions from code review

7aaa515

Co-authored-by: Georg Lauterbach <[email protected]>

This comment was marked as resolved.

Sign in to view

polarathene commented Jan 29, 2024

View reviewed changes

Comment thread target/scripts/helpers/relay.sh Outdated

polarathene added 3 commits January 30, 2024 11:31

fix: relay.sh - grep regex compatibility with + requires -E

7b073c4

Merge branch 'master' into docs/refactor-relayhosts-feature

78f1521

chore: postfix.sh - FIRST_FIELD => More descriptive field name

22bac61

georglauterbach approved these changes Jan 30, 2024

View reviewed changes

Merge branch 'master' into docs/refactor-relayhosts-feature

59f5638

polarathene merged commit 5b54d1d into docker-mailserver:master Jan 30, 2024

This was referenced Jan 30, 2024

Setting RELAY_HOST forces expectation to provide relay credentials #3842

Closed

docs: Complete rewrite of Relay Host pages #3861

Merged

AlMaVizca mentioned this pull request Jun 20, 2024

Update chart to use docker-mailserver 14.0.0 docker-mailserver/docker-mailserver-helm#120

Merged

4 tasks

Uh oh!

Conversation

polarathene commented Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Context

Type of change

Checklist:

Uh oh!

polarathene commented Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene commented Jan 29, 2024

ENV docs visual comparison

Before

After

Uh oh!

georglauterbach commented Jan 29, 2024

Uh oh!

georglauterbach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

github-actions Bot commented Jan 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

polarathene commented Jan 28, 2024 •

edited

Loading

polarathene commented Jan 28, 2024 •

edited

Loading

georglauterbach left a comment •

edited

Loading