Skip to content

PERMIT_DOCKER=none as new default value#2424

Merged
georglauterbach merged 10 commits intodocker-mailserver:masterfrom
casperklein:new-permit-docker-default
Mar 2, 2022
Merged

PERMIT_DOCKER=none as new default value#2424
georglauterbach merged 10 commits intodocker-mailserver:masterfrom
casperklein:new-permit-docker-default

Conversation

@casperklein
Copy link
Copy Markdown
Member

@casperklein casperklein commented Feb 20, 2022

Description

Using DMS with podman can lead to security issues (e.g. an open relay), if not done properly. This PR aims a more secure out-of-the-box setup for podman users.

The following changes have been done:

  1. New default value for PERMIT_DOCKER is now none.
  2. An empty PERMIT_DOCKER value is now considered as none.
  3. To keep the old behaviour (adding the container IP address to Postfix's 'mynetworks'), PERMIT_DOCKER=container must be used.

Related to: #2393

Type of change

  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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/)
  • New and existing unit tests pass locally with my changes

@casperklein casperklein self-assigned this Feb 20, 2022
@casperklein casperklein added this to the v10.5.0 milestone Feb 20, 2022
@casperklein casperklein added area/documentation area/scripts kind/improvement Improve an existing feature, configuration file or the documentation service/postfix labels Feb 20, 2022
@casperklein casperklein changed the title New PERMIT_DOCKER default value PERMIT_DOCKER=none as new default value Feb 20, 2022
@casperklein casperklein marked this pull request as ready for review February 20, 2022 19:14
@casperklein casperklein requested a review from a team February 20, 2022 19:14
@wernerfred
Copy link
Copy Markdown
Member

3. To keep the old behaviour

Wouldn't that qualify for a breaking change?

@casperklein
Copy link
Copy Markdown
Member Author

My opinion: definitely. But see our maintainer discussion from 19. Dec and the provided example, which also dropped functionality.

tl;dr. if the container comes up without interaction, it's not a breaking change and people should read the changelog + adjust their config.

@polarathene
Copy link
Copy Markdown
Member

Breaking change should probably wait until next major?

What's the impact of existing users that upgrade and don't adjust the ENV accordingly? How soon would they realize something like this happened? (assuming they don't read changelog and expect things not to break from a minor release)

@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Feb 20, 2022

What's the impact of existing users that upgrade and don't adjust the ENV accordingly?

Two things I can think of:

  1. Some customization, that runs a script inside the container, which sends mails unauthenticated.
  2. Running an open-relay (podman) will probably be broken after this 😆

@polarathene
Copy link
Copy Markdown
Member

  1. Some customization, that runs a script inside the container, which sends mails unauthenticated.

Sounds like it warrants waiting on major version then?

I know we're a little less strict on that if the change isn't likely to affect many users in practice and our release notes clearly cover the change. I'm fine with this being a minor version change 👍

Comment thread target/scripts/startup/setup-stack.sh
polarathene
polarathene previously approved these changes Feb 21, 2022
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

LGTM, may want to adjust some log messages though?

Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
@georglauterbach
Copy link
Copy Markdown
Member

I have an upcoming PR as well that changes some default values (unifies ENV setup). These changes would technically be identical in impact to the changes in this PR. IMO we should wait for #2419 ad #2420 to be merged, release 10.5.0 first and then see whether 10.6.0 or 11.0.0 should be the next successor after this PR and my next PR are merged.

@casperklein
Copy link
Copy Markdown
Member Author

If this PR can go into 10.6.0, why not straight into 10.5.0? Whats the benefit in waiting?

@georglauterbach
Copy link
Copy Markdown
Member

If this PR can go into 10.6.0, why not straight into 10.5.0? Whats the benefit in waiting?

Fair point, and very much fine with me :)

Comment thread target/scripts/startup/setup-stack.sh
@wernerfred
Copy link
Copy Markdown
Member

I still See it as a breaking Change and therefore a Major Version bump. If we wajt to avoid a Lot of Major Version bumps we could:

  1. Wait with merge of breaking Changes until we have a bunch and Release them all together or
  2. Bring in More and deeper thinking to the reviewal/Implementation process and Pick the "correct" Default straight away instead of changing it Afterwards many Times.

Ideally we should also have a grace period to warn Users already 1-2 Version ahead about upcoming breaking Changes behaviors.

@georglauterbach
Copy link
Copy Markdown
Member

I still See it as a breaking Change and therefore a Major Version bump. If we wajt to avoid a Lot of Major Version bumps we could:

1. Wait with merge of breaking Changes until we have a bunch and Release them all together or

2. Bring in More and deeper thinking to the reviewal/Implementation process and Pick the "correct" Default straight away instead of changing it Afterwards many Times.

Ideally we should also have a grace period to warn Users already 1-2 Version ahead about upcoming breaking Changes behaviors.

While this would be optimal, we're maintaining this in our free time and we do not have the resources to plan one or two (even minor) versions ahead. I've got no problem with releasing 10.5.0 "now" (after feature freeze), and merging this into master to be on :edge first. The other PR of mine could similarly wait.

@polarathene
Copy link
Copy Markdown
Member

We could also have a separate PR that is long-lived open which is a next/major branch that PRs like this could be merged into, then when ready that could merge to master before making a new major release.

We'd just have the long-lived PR frequently syncing up with master branch to ensure tests pass and all commits cleanly carry over to master without any conflicts?

@georglauterbach
Copy link
Copy Markdown
Member

We could also have a separate PR that is long-lived open which is a next/major branch that PRs like this could be merged into, then when ready that could merge to master before making a new major release.

We'd just have the long-lived PR frequently syncing up with master branch to ensure tests pass and all commits cleanly carry over to master without any conflicts?

This sounds like a possible solution, but we'd need to make sure no :edge images are built from this branch (haven't checked whether this would happen at the moment (we could introduce a :bleeding tag or something 😆). We'd manually need to sync with master, but this should not be a big issue as not too many PR's well container breaking changes.

@polarathene
Copy link
Copy Markdown
Member

It would also need some extra care with merging as we'd want to rebase the commits instead of squash merge for such a branch AFAIK when merging to master for a release. Might also be important to make that branch protected as well.

@wernerfred
Copy link
Copy Markdown
Member

Or Just Release New Major Versions If a breaking Change occurs 😅
No big Deal imho, i have No Problem with high Version Numbers. Thats semver ;)

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Feb 21, 2022

i have No Problem with high Version Numbers.

Does make our docs version selector less pleasant to use though 😅

The separate branch approach is only useful for queuing up changes for a major release, which isn't that common of a need.

Maybe you've got a good point though, and major version releases don't need to be few and far between, but treated just like minor/patch releases..

@georglauterbach
Copy link
Copy Markdown
Member

So, according to the maintainers' discussion, this should be tagged v11.0.0, right? I will tag it as such, if someone objects, let me know :)

@georglauterbach georglauterbach modified the milestones: v10.5.0, v11.0.0 Feb 22, 2022
@georglauterbach georglauterbach added the meta/feature freeze On hold due to upcoming release process label Feb 22, 2022
@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Mar 2, 2022
@georglauterbach
Copy link
Copy Markdown
Member

@casperklein we can now go ahead and start merging the open PRs. I'd start with the numerically lowest, i.e. this one and work our way forward.

@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Mar 2, 2022

I'd start with the numerically lowest,

I good plan. However, I just updated the branch in #2434 to merge next 😆

Let's merge this and then from low to high.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 2, 2022

Documentation preview for this PR is ready! 🎉

Built with commit: 5053132

@georglauterbach georglauterbach merged commit 57c52d7 into docker-mailserver:master Mar 2, 2022
@casperklein casperklein deleted the new-permit-docker-default branch March 2, 2022 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants