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

smtp_forward: tighten up queue.wants routing #3199

Merged
merged 4 commits into from
Jun 13, 2023
Merged

smtp_forward: tighten up queue.wants routing #3199

merged 4 commits into from
Jun 13, 2023

Conversation

msimerson
Copy link
Member

@msimerson msimerson commented Jun 13, 2023

Changes proposed in this pull request:

  • smtp_forward: tighten up queue.wants routing
  • doc(smtp_forward): improve markdown formatting
  • smtp_forward: add a couple tests

Checklist:

  • docs updated
  • tests updated
  • Changes updated

@msimerson msimerson changed the title Smtp forward smtp_forward: tighten up queue.wants routing Jun 13, 2023
@msimerson msimerson merged commit f3685a7 into master Jun 13, 2023
@msimerson msimerson deleted the smtp_forward branch June 13, 2023 06:58
@msimerson msimerson mentioned this pull request Jun 13, 2023
msimerson added a commit that referenced this pull request Jun 16, 2023
#### Fixed

- feat(q_forward): add LMTP routing handling #3199
- chore(q_forward): tighten up queue.wants handling #3199
- doc(q_forward): improve markdown formatting #3199
- helo.checks: several fixes, #3191
- q/smtp_forward: correct path to next_hop #3186
- don't leak addr parsing errors into SMTP conversation #3185
- connection: handle dns.reverse invalid throws on node v20 #3184
- rename redis command setex to setEx #3181

#### Changed

- test(helo.checks): add regression tests for #3191 #3195
- connection: handle dns.reverse invalid throws on node v20
- build(deps): bump ipaddr.js from 2.0.1 to 2.1.0 #3194
- chore: bump a few dependency versions #3184
- dns_list_base: avoid test failure when public DNS used #3184
- doc(outbound.ini) update link #3159
- doc(clamd.md) fixed spelling error #3155
@gramakri
Copy link
Collaborator

@msimerson this change causes a regression in our tests. the mx hook is not registered anymore if enable_outbound is false. Is this intentional ? The mx hook is how the per-domain relay configuration is loaded.

61126ab#diff-358fdbf915639f898db16fb2bab56c2284331a4e8bfe65e25fe3b0f1f13bdcfeR33

@gramakri
Copy link
Collaborator

gramakri commented Jun 19, 2023

To add some more info, the way we configure the forwad plugin is:

enable_outbound=false
domain_selector=mail_from
[cloudron.addon]
enable_outbound=true
host=172.18.0.1
port=2587
enable_tls=true
auth_type=plain
auth_user=relay_user
auth_pass=relay_pass

With the above config, emails fromxx@cloudron.addon are not forwarded to the relay.

@msimerson
Copy link
Member Author

if enable_outbound is false. Is this intentional ?

Well yes. Sorry. The get_mx hook doesn't forward (in the sense of the smtp_forward plugin) at all. Instead, messages are accepted into Haraka's queue and then delivered with outbound. The get_mx hook can then provide a more specific route to outbound. But it's really not forwarding, it's MX routing, right? And so get_mx should be controlled by outbound_enabled.

That's what I was thinking. But I missed something important: that disables the ability to have [outbound] forward MX routing disabled in the main config and enabled per-domain. And vise versa. #3204 does the needful, but it needs to have a test added so prevent future regressions. smtp_forward is used in quite a few ways (I also [ab]use it to deliver via LMTP using outbound).

@gramakri
Copy link
Collaborator

But I missed something important: that disables the ability to have [outbound] forward MX routing disabled in the main config and enabled per-domain

Yes, indeed, this is exactly how we were using it. I see you already reverted that part of the code, thanks! I will test and get back.

@gramakri
Copy link
Collaborator

Can confirm your revert fixed the issue (for us).

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.

2 participants