fix: postfix-main.cf may depend upon postfix-master.cf#3880
fix: postfix-main.cf may depend upon postfix-master.cf#3880polarathene merged 6 commits intomasterfrom
postfix-main.cf may depend upon postfix-master.cf#3880Conversation
Custom parameters must be referenced to be retained when `postconf -n` is run. If those parameters are referenced by `postfix-master.cf` this needs to update `master.cf` before updating `main.cf`.
polarathene
added
service/postfix
area/scripts
area/configuration (file)
kind/bug/fix
|
Should we add a test case for this? Or is the inline comment about order dependency sufficient? |
|
I think having a test for this is a good idea :) |
github-actions
Bot
added
the
meta/stale
polarathene
added
stale-bot/ignore
|
|
||
| # do not directly output to 'main.cf' as this causes a read-write-conflict | ||
| # Do not directly output to 'main.cf' as this causes a read-write-conflict | ||
| postconf -n >/tmp/postfix-main-new.cf 2>/dev/null |
There was a problem hiding this comment.
Due to postconf -n outputting any errors to /dev/null, this sort of information is omitted from the logs
As long as annoying errors are not expected, 2>/dev/null should be removed:
| postconf -n >/tmp/postfix-main-new.cf 2>/dev/null | |
| postconf -n >/tmp/postfix-main-new.cf |
There was a problem hiding this comment.
As long as annoying errors are not expected
What do you consider annoying?
I gave an example log line (warning), you get those as well for any duplicate parameters configured IIRC (due to postfix-main.cf appending overrides).
I can commit your suggestion if you think the added noise is worthwhile? (here is why we originally did this)
There was a problem hiding this comment.
Probably I misunderstood something.
By moving the postfix master part at the top, the mentioned error should be fixed? Therefore postconf -n should run fine without errors. If that's the case, stderr should not be redirected to dev/null.
There was a problem hiding this comment.
By moving the postfix master part at the top, the mentioned error should be fixed?
The log line example shown in this PR would be avoided if the setting is in master.cf yes.
Therefore
postconf -nshould run fine without errors. If that's the case, stderr should not be redirected to dev/null.
See the link to the original reason we added 2>/dev/null to avoid log spam due to a different condition. I assume that behaviour can still be reproduced and don't see how it would be desirable to show it.
EDIT: Confirmed, this can still be reproduced when removing 2>/dev/null.
While that output is very spammy for me (as the linked comment reports).. during actual container startup with a config file, the postconf command on this line only displayed a single warning line emitted per duplicate parameter in the produced main.cf file:
postconf: warning: /etc/postfix/main.cf, line 131: overriding earlier entry: max_idle=300
Still that is expected for each parameter in postfix-main.cf that has one already defined in main.cf, thus that noise isn't helpful in logs 🤷♂️
I suppose we could filter it out, so that other warnings are visible like the one this PR mentions:
postconf: warning: /etc/postfix/main.cf: unused parameter: some_custom_parameter=value
Process substitution seems to handle that. Not sure if this is optimal, but it works 😅
postconf -n >/tmp/postfix-main-new.cf 2> >(grep -v 'overriding earlier entry')
There was a problem hiding this comment.
Sorry for the long delay on getting back to this!
Test case added. I've done some more revisions, but the new comments as part of this review is all you should need AFAIK. Feel free to look over the test case itself, it should be easy to grok👍
@georglauterbach ping for review, since I can't renew an existing request 😓
| # Do not directly output to 'main.cf' as this causes a read-write-conflict. | ||
| # `postconf` output is filtered to skip expected warnings regarding overrides: | ||
| # https://github.com/docker-mailserver/docker-mailserver/pull/3880#discussion_r1510414576 | ||
| postconf -n >/tmp/postfix-main-new.cf 2> >(grep -v 'overriding earlier entry') |
There was a problem hiding this comment.
This is the main change I added since the last round of reviews.
It should allow for retaining other output from postconf that may be of interest when something unexpected happens, as opposed to the more blunt 2>/dev/null approach we have 👍
You're both more comfortable with shell syntax than I am, so if you have any feedback here, happy to go with that instead 😎
There was a problem hiding this comment.
You're both more comfortable with shell syntax than I am, so if you have any feedback here, happy to go with that instead 😎
No, looks all good to me!
| # This parameter is referenced by an override in `postfix-master.cf`: | ||
| custom_parameter = cidr:{{!172.16.0.42 REJECT}}, permit_sasl_authenticated, reject |
There was a problem hiding this comment.
The test adds this postfix-main.cf custom parameter, which postfix-master.cf uses for smtpd_client_restrictions value. This is the same as what the referenced discussion was using, it just prepends that cidr inline table to reject any IP that is not 172.16.0.42.
With the postfix.sh change to swap the order we apply postfix-main.cf and postfix-master.cf, the test demonstrates that we can correctly resolve/expand the $custom_parameter value in the updated /etc/postfix/master.cf file 👍
| # The `postconf` command can query both `main.cf` and `master.cf` at `/etc/postfix/`. | ||
| # Reference: http://www.postfix.org/postconf.1.html |
There was a problem hiding this comment.
Maintainer docs reference for this test file, since there's a few different options used with the postconf command in the test cases below, probably helpful to have here 😅
|
Cheers for the reviews/approvals! 😁 |
3 participants
Description
Custom parameters must be referenced to be retained when
postconf -nis run.If
postfix-main.cfdefines custom parameters that are referenced bypostfix-master.cf, that needs to first updatemaster.cfbefore updatingmain.cf(wherepostconf -nis relied upon).Failure was a little tricky to troubleshoot as it only occurred during initial startup, reproducing the logic afterwards manually, or via container restart (with internal state persisted) worked fine as
master.cfwas already configured with the custom parameters.Due to
postconf -noutputting any errors to/dev/null, this sort of information is omitted from the logs:Discovered while answering: https://github.com/orgs/docker-mailserver/discussions/3847
Type of change
Checklist
CHANGELOG.md