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

Code review: real-postfix #3

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Conversation

dehnert
Copy link
Contributor

@dehnert dehnert commented Dec 11, 2020

This is a placeholder PR for tracking the code review of the real-postfix role. It consists of the current master (commit 40aaee3), plus commits to ansible-realserver (commit b908d64) that modify ansible/roles/real-postfix (git filter-repo --path ansible/roles/real-postfix/ plus some cherry-picking onto master). It shouldn't actually get merged (and may or may not be a reasonable way to code review this...).

quentinmit and others added 29 commits December 11, 2020 18:23
…rewrite -> @scripts.mit.edu happens before transport_maps.
…port_maps, so we need to avoid returning virtual aliases
@dehnert
Copy link
Contributor Author

dehnert commented Dec 12, 2020

/etc/postfix/virtual isn't new, but it doesn't seem to be referenced anywhere. Do you know if there's a reason it sticks around? It looks like it's an upstream default for virtual_alias_maps, but that's an option we're... rather emphatically not using the default value of.

Copy link
Contributor Author

@dehnert dehnert left a comment

Choose a reason for hiding this comment

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

I've reviewed, I think, all of this except for the virtual_alias_{maps,domains} and smtp_generic_maps part -- which is to say, about half the files, and I think much less than that of the complexity. Files remaining to review:

  • generic_strip_pool
  • pass-scripts.mit.edu{,-suffix}
  • main.cf.j2
  • virtual-alias-domains-ldap.cf.j2
  • virtual-alias-maps-ldap{,-reserved}.cf.j2
  • virtual-alias-maps-relay{,-user,-user-suffix}-ldap.cf.j2

I've got some comments, but they're mostly ~style things -- I wouldn't have a problem with merging the portion I've reviewed as-is if you wanted.

ansible/roles/real-postfix/files/aliases Show resolved Hide resolved
ansible/roles/real-postfix/files/aliases Show resolved Hide resolved
ansible/roles/real-postfix/files/aliases Show resolved Hide resolved
ansible/roles/real-postfix/files/prune-mailq Show resolved Hide resolved
@@ -0,0 +1,7 @@
# N.B. If this /does/ match, the user is /blocked/.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels unnecessarily confusing. While I appreciate a desire to match the authorized_submit_users config option name, I think it'd be better to name this file blocked-submit-users-ldap.cf or similar, which then wouldn't need a comment to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked about this some today, and I think the conclusion was that while renaming would make sense, there's a lack of enthusiasm to mess with what's not broken, which seems reasonable.

Copy link
Contributor Author

@dehnert dehnert left a comment

Choose a reason for hiding this comment

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

I have more concerns with this batch, especially as it relates to domains on the other pool. If this was pre-commit code review, I'd want to see them addressed before merging.

@dehnert
Copy link
Contributor Author

dehnert commented Dec 12, 2020

I also wrote some comments on what virtual_alias_domains is doing that may be useful -- it's at dehnert@05ea851.

@dehnert
Copy link
Contributor Author

dehnert commented Jul 15, 2021

My little test script:

$ cat test_mail.py 
#!/usr/bin/python3

# Test Scripts mail forwarding setup

# Styles of email addresses
# [email protected] -> $locker
# *@locker.scripts.mit.edu
# *@$domain -> $domain's locker

# Mail enters scripts system at:
# F22 host
# F30 host

# Locker is configured with:
# F22
# F30

# Domain is configured with
# F22
# F30

# Same as above, but using force_pool

import smtplib

SCRIPTS_NAMES = ['scripts-f20.mit.edu', 'scripts-f30.mit.edu']
SCRIPTS_NAMES = ['w-e.mit.edu']
RECIPIENTS = [
    '[email protected]',
    '[email protected]',
    '[email protected]',
    '[email protected]',
]
SENDER = '[email protected]'

MSG = """To: %(recipient)s
Subject: test to %(recipient)s via %(host)s

test to %(recipient)s via %(host)s
"""

for host in SCRIPTS_NAMES:
    with smtplib.SMTP(host) as smtp:
        for recipient in RECIPIENTS:
            msg = MSG % dict(recipient=recipient, host=host)
            smtp.sendmail(SENDER, [recipient], msg)

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

Successfully merging this pull request may close these issues.

3 participants