Skip to content

service(postfix): Better handling of the compatibility_level setting#2597

Merged
polarathene merged 7 commits intodocker-mailserver:masterfrom
polarathene:chore/remove-compatibility-level
Jun 5, 2022
Merged

service(postfix): Better handling of the compatibility_level setting#2597
polarathene merged 7 commits intodocker-mailserver:masterfrom
polarathene:chore/remove-compatibility-level

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented May 17, 2022 •
edited
Loading

Description

I wanted to fix a typo specifically. But also looked into why the postfix compatibility level was set to 2 (both from the same PR in Sep 2017, while the compatibility level test was introduced in May 2017):

The minimal compatibility level had to be set to 2 in order for pcre to work and not creating errors in the logging.

It seems leaving this at the default (0) is passing the mentioned PCRE test just fine.

It was not made clear what logging errors were a concern. There was one regarding the sender-cleanup service that the PR added with implicit chroot default. The default compatibility level 0 would set this to y, otherwise it'd be n. This PR makes the n explicit.

If choosing to leave Postfix compatibility_level at the default 0, when the config is changed (eg: via postconf), it will log the following each time (only seems to happen with postfix reload or supervisorctl restart postfix):

May 17 07:46:10 mail postfix[3482]: Postfix is running with backwards-compatible default settings
May 17 07:46:10 mail postfix[3482]: See http://www.postfix.org/COMPATIBILITY_README.html for details
May 17 07:46:10 mail postfix[3482]: To disable backwards compatibility use "postconf compatibility_level=2" and "postfix reload"

This is noise, so reverting the compatibility_level back to being set to 2 would be understandable. Would we want this in _setup_postfix_override_configuration still, or in target/postfix/main.cf? Leaving it at 0 would catch any compatibility default settings being applied during tests though; each level is listed here.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

@polarathene
chore: User configs should no longer need a compatibility level
d81a084 
This should no longer be required.

Removing it falls back to the default `0` level which does output a log with every change made to config, suggesting to set it to `2` to hide that log notice.

Could explicitly set compatibility in the config, or revert this commit if suitable.
@polarathene
chore: Fix typo
169e7cd
@polarathene
chore: Apply explicit chroot default for sender-cleanup
8545923 
The implicit default is set to `y` as a compatibility fallback, but otherwise it is [advised to set to `n` going forward](http://www.postfix.org/COMPATIBILITY_README.html#chroot).

Test was changed to catch any backwards-compatibility logs, not just those for `chroot=y`. `using` added as a prefix to avoid catching log message whenever a setting is changed that the default compatibility level is active.
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented May 18, 2022

Haha :D I noticed the compatibility_level thing too :D I'm going to go ahead and remove it from my PR (#2598) as well :D

georglauterbach
georglauterbach previously approved these changes May 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@georglauterbach georglauterbach changed the title chore: Remove Postfix compatibility level service: remove Postfix compatibility_level May 18, 2022
@georglauterbach georglauterbach added this to the v11.1.0 milestone May 18, 2022
@georglauterbach georglauterbach added service/postfix kind/improvement Improve an existing feature, configuration file or the documentation area/configuration (file) labels May 18, 2022
georglauterbach added a commit that referenced this pull request May 18, 2022
@georglauterbach
appease ShellCheck and remove commpatibility_level
b090e92 
see #2597
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented May 18, 2022 •
edited
Loading

This is noise, so reverting the compatibility_level back to being set to 2 would be understandable. Would we want this in _setup_postfix_override_configuration still, or in target/postfix/main.cf?

Sorry, did not read all in the beginning. I would actually add it to /etc/postfix/main.cf with a value of 2. This way, we

  1. let the user decide whether to override it with a custom value
  2. remove clutter in a function that actually does other things

@georglauterbach georglauterbach mentioned this pull request May 18, 2022
Merged
8 tasks
@polarathene
chore: Set compatibility_level in main.cf
1716648 
Applying review feedback.

We retain the level`2` value previously set via scripts. This avoids log noise that isn't helpful.

In tests we override this to be `0` to leverage detecting config that should be adjusted, or prevent introducing implicit defaults that compatibility levels treat differently (eg: `chroot`).
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented May 19, 2022

  • I've applied compatibility_level = 2 to main.cf as suggested.
  • In our tests postfix-main.cf override, I have set the level back to the default 0. This shouldn't affect tests negatively, but potentially trigger a test failure that checks logs when a backwards compatibility implicit default differs from the implicit default of newer releases. If such failures are caught in future, we would then know to provide an explicit default (older or newer default, depending on what makes most sense).

@georglauterbach georglauterbach changed the title service: remove Postfix compatibility_level service: adjust Postfix compatibility_level May 20, 2022
casperklein
casperklein reviewed May 28, 2022
View reviewed changes
Comment thread target/postfix/main.cf
casperklein
casperklein previously approved these changes May 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. You may add my comment suggestion if you want.

@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented May 28, 2022

You may add my comment suggestion if you want.

I will do that thanks!

I saw something related to the compatibility level change in postfix docs that I want to look into further before merging this. I want to make sure it does get caught if it's compatibility default would impact tests 😅

@polarathene
chore: Remove compatibility_level=0 for tests
26eb445 
The log detection doesn't work for each affected feature until it's actually used.

Applied review feedback to give maintainers some context with this setting and why we have it presently set to `2`.
@polarathene polarathene dismissed stale reviews from casperklein and georglauterbach via 26eb445 May 30, 2022 07:48
@polarathene
chore(housekeeping): Tab indents to spaces
fb3ea00 
We generally prefer spaces to avoid mixed indents creeping in.
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented May 30, 2022

Reverted the change to set compatibility_level=0 for tests. Several settings didn't output any backwards-compatible log but did fallback to a conditional default shown by postconf. I believe when those features are interacted with, then the logs are emitted. It only affected the chroot one due to the service being started.

Other compatibility rules don't seem to be a concern with our current config. Since we can't properly detect each with the log detection and it may result in tests behaving differently than what we ship, I think it's best to ignore that idea 😅

When the next Debian release happens we should have Postfix 3.6 available and can bump the compatibility_level to 3.6.

@polarathene polarathene requested a review from casperklein May 31, 2022 01:21
@polarathene polarathene changed the title service: adjust Postfix compatibility_level service(postfix): Better handling of the compatibility_level setting May 31, 2022
@polarathene polarathene requested review from a team and wernerfred June 1, 2022 01:09
@polarathene polarathene merged commit 3d6e7a7 into docker-mailserver:master Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

@wernerfred wernerfred Awaiting requested review from wernerfred

Assignees

@polarathene polarathene

Labels

area/configuration (file) kind/improvement Improve an existing feature, configuration file or the documentation service/postfix

Projects

None yet

Milestone

v11.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@polarathene @georglauterbach @casperklein