Populate relayhost_map from virtual accounts (and other fixes)#1596
Merged
erik-wramner merged 3 commits intomasterfrom Sep 5, 2020
unknown repository
Merged
Populate relayhost_map from virtual accounts (and other fixes)#1596erik-wramner merged 3 commits intomasterfrom unknown repository
erik-wramner merged 3 commits intomasterfrom
unknown repository
Conversation
Contributor
|
I think I will need some time to review this (unless you beat me to it @fbartels). One question though. You start by writing to the checksum file without sleep, but that should already have been done in _setup_chksum_file in the startup script which runs before this script is allowed to start. Moving the write to before the sleep just overwrites the file produced by the startup script. That is unnecessary work, but it can also hide changes. Assume that there were changes after the startup script ran and before the change script created the file. We would not act on them as the checksum file says that nothing has changed. In short I'd like to remove that first step and use the file created by the startup script to eliminate the risk for race conditions. |
Author
|
On Wed, Aug 26, 2020 at 05:10:18AM -0700, Erik Wramner wrote:
I think I will need some time to review this (unless you beat me to it @fbartels). One question though. You start by writing to the checksum file without sleep, but that should already have been done in _setup_chksum_file in the startup script which runs before this script is allowed to start. Moving the write to before the sleep just overwrites the file produced by the startup script.
Oh, I had not noticed the '_setup_chksum_file' step in start-mailserver.sh.
That is unnecessary work, but it can also hide changes. Assume that there were changes after the startup script ran and before the change script created the file. We would not act on them as the checksum file says that nothing has changed.
In short I'd like to remove that first step and use the file created by the startup script to eliminate the risk for race conditions.
I agree. Do you mind if I factor out the checksum generation code into
helper_functions.sh while I'm at it?
…--
mwnx
GPG: AEC9 554B 07BD F60D 75A3 AF6A 44E8 E4D4 0312 C726
ipfs.io/ipfs/QmdLc7sFYA3SeAocaFC5AaVJKfruj5ecePoj1iW2rWzgPS
|
fbartels
approved these changes
Aug 27, 2020
Member
fbartels
left a comment
fbartels
left a comment
There was a problem hiding this comment.
helper_functions.sh while I'm at it?
yes, sounds like a good idea.
If first glance I could not see anything else, than what was already said about the checksum function. lgtm
added 3 commits
August 28, 2020 14:57
If a change to one of the tracked files happened soon after (<1 second?) a previously detected change, it could end up going undetected. In particular, this could cause integration tests to fail (see next commits). Fixed by computing the new checksum file _before_ checking for changes.
check-for-changes.sh did not have a special case to handle lines in postfix-relaymap.cf consisting of only a domain (indicating that said domain should never be relayed). This case is handled by start-mailserver.sh so when such a line existed, things would work well until a config file update was detected by check-for-changes.sh. After that, the generated relayhost_map file would be corrupted. Fixed by factoring a 'populate_relayhost_map' function out of start-mailserver.sh and into helper_functions.sh and reusing it in check-for-changes.sh. Note: There are certainly quite a few more pieces of code that could be refactored in a similar fashion. Note2: check-for-changes.sh would previously never update the relayhost_map file when $ENABLE_LDAP was set to 1. I don't think this was intended —there is after all no such condition in start-mailserver.sh— and so this condition no longer applies.
Previously, only postfix-relaymap.cf and postfix-accounts.cf would be used to populate the relayhost_map file. Now, also use postfix-virtual.cf when present. To me, there is nothing absurd about sending mail "From:" a virtual account (or more specifically its domain) so it makes sense that when a $RELAY_HOST is defined it should be used for virtual accounts as well.
Author
|
I updated the first commit with the changes we talked about, so it should be ready for merging. |
This was referenced Sep 3, 2020
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I started this branch to enable relaying through virtual account domains (before, only domains found in postfix-accounts.cf and postfix-relaymap.cf were taken into account). I bumped into some bugs along the way so I ended up having to do a bit of fixing and refactoring (see first two commits).