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

Posfix: add option to re-enable reject_unknown_client_hostname after #3248 #3255

Merged
merged 3 commits into from Apr 16, 2023

Conversation

georglauterbach
Copy link
Member

Description

See #3248 (comment).

Fixes concern raised in #3248

Type of change

  • Improvement (non-breaking change that does improve existing functionality)
  • 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

Previously, we had 1000000000000 functions for every tine setup step.
This commit merges these functions into 2 (or 3, if you will). Setup
routines have not been altered, log messages have been adjusted though
(like already done with Rspamd).

No change in functionality.
to enable or disable `reject_unknown_client_hostname` for Postfix's
`smtpd_sender_restrictions`.
@georglauterbach
Copy link
Member Author

@casperklein please review the first commit separately from the second one. I saw the setup file for Postfix and just could not help myself but end the trauma we have with these 1000 separate setup functions. Functionality-wise, nothing changes - if you will, this is just style.

Reviewing each commit on its own makes the diff less fuzzy.

@casperklein
Copy link
Member

LGTM 👍


Serious question: In the past, there was lot of code refactoring where code like:

# do one thing
foo bar

# do another thing
baz bla

was converted to

function one_thing() {
	foo bar
}

function another_thing() {
	baz bla
}

one_thing
another_thing

We had a discussion about this, wether it's worth to create functions that are only called once or not. We sticked with functions. The first commit in this PR now reverts this more or less.
Previously I was team "no-functions". But after introducing them, I started to like them 😆
As you wrote, it's only styling. I am fine with both variant. But maybe consider this: we should follow and stick to one style, instead of changing in more or less short intervals.


_setup_spoof_protection could also be part of _register_setup_function '_setup_postfix_late'?

@georglauterbach
Copy link
Member Author

Serious question: In the past, there was lot of code refactoring where code like:

# do one thing
foo bar

# do another thing
baz bla

was converted to

function one_thing() {
	foo bar
}

function another_thing() {
	baz bla
}

one_thing
another_thing

We had a discussion about this, wether it's worth to create functions that are only called once or not. We sticked with functions. The first commit in this PR now reverts this more or less. Previously I was team "no-functions". But after introducing them, I started to like them 😆 As you wrote, it's only styling. I am fine with both variant. But maybe consider this: we should follow and stick to one style, instead of changing in more or less short intervals.

I actually do not remember 😆 But for me, the important part is that there is at least one function commands are executed in, and that we do not just execute commands in non-function-contexts. So this

# do one thing
foo bar

# do another thing
baz bla

should be this:

function one_thing() {
  # do one thing
  foo bar

  # do another thing
  baz bla
}

one_thing

I am not a big fan of one function executing one command, although that may be fine in single places. The Postfix setup did it a bit too much, hence my refactoring.

_setup_spoof_protection could also be part of _register_setup_function '_setup_postfix_late'?

Probably 🤔 But I'd like to defer this, to move _setup_spoof_protection into the Postfix setup file.

@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: baebaaa

@georglauterbach georglauterbach merged commit c8dfb9a into master Apr 16, 2023
9 checks passed
@georglauterbach georglauterbach deleted the posfix/reject_unknown_client_hostname branch April 16, 2023 12:09
__postfix__log 'trace' 'Configuring user access'
if [[ -f /tmp/docker-mailserver/postfix-send-access.cf ]]
then
sed -i 's|(smtpd_sender_restrictions =)|\1 check_sender_access texthash:/tmp/docker-mailserver/postfix-send-access.cf,|' /etc/postfix/main.cf
Copy link
Contributor

Choose a reason for hiding this comment

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

missing -E (pull request #3272)

sed -i -E 's|(smtpd_sender_restrictions =)|\1 check_sender_access texthash:/tmp/docker-mailserver/postfix-send-access.cf,|' /etc/postfix/main.cf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants