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

check for local_mx only when default route is used #3307

Merged
merged 4 commits into from
Apr 11, 2024
Merged

check for local_mx only when default route is used #3307

merged 4 commits into from
Apr 11, 2024

Conversation

analogic
Copy link
Collaborator

@analogic analogic commented Apr 5, 2024

github somehow broke the #3300 PR - still showing me "processing updates" and refusing to accept changes, so this is a second attempt to update cfg.local_mx_ok functionality

It only checks local mx if it falls through to the default route - so any plugin's mx is excluded.

@msimerson
Copy link
Member

👍 I like this approach better than #3300. I'd like to consider it for another day or two, and give others a chance to weigh in.

@msimerson
Copy link
Member

The bit of code you had to change in found_mx I find to be clumsy (manually copying over every property). Instead, how about improving that code with Object.assign:

diff --git a/outbound/hmail.js b/outbound/hmail.js
index 68f57f72..1a817fd7 100644
--- a/outbound/hmail.js
+++ b/outbound/hmail.js
@@ -272,15 +272,11 @@ class HMailItem extends events.EventEmitter {
                 if (mxlist[mx].path) {
                     this.mxlist.push(mxlist[mx]);
                 }
-                else if (obc.cfg.ipv6_enabled) {
-                    this.mxlist.push(
-                        { exchange: mxlist[mx].exchange, priority: mxlist[mx].priority, port: mxlist[mx].port, using_lmtp: mxlist[mx].using_lmtp, family: 'AAAA' },
-                        { exchange: mxlist[mx].exchange, priority: mxlist[mx].priority, port: mxlist[mx].port, using_lmtp: mxlist[mx].using_lmtp, family: 'A' }
-                    );
-                }
                 else {
-                    mxlist[mx].family = 'A';
-                    this.mxlist.push(mxlist[mx]);
+                    this.mxlist.push(Object.assign({}, mxlist[mx], { family: 'A' });
+                    if (obc.cfg.ipv6_enabled) {
+                        this.mxlist.push(Object.assign({}, mxlist[mx], { family: 'AAAA' });
+                    }
                 }
             }
             this.try_deliver();

or even better still, use the es6 spread operator:

this.mxlist.push({ ...mxlist[mx], ...{ family: 'A' });
if (obc.cfg.ipv6_enabled) {
  this.mxlist.push({ ...mxlist[mx], ...{ family: 'AAAA' });
}

@@ -300,7 +302,7 @@ class HMailItem extends events.EventEmitter {
const mx = this.mxlist.shift();
const host = mx.exchange;

if (!obc.cfg.local_mx_ok) {
if (mx.default && !obc.cfg.local_mx_ok) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying hard to like the name default here. It's not very self documenting and requires one to go figure out how default got set. In this case, default means "looked up via DNS". But that's a bit long for a name. Perhaps something like from_dns or via_dns, so the if reads something like "if the mx was looked up in DNS and ! local_mx_ok)". That'll also avoid namespace collisions when someone goes searching to find how that property got set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like default either, naming is hard...

@msimerson msimerson merged commit 8b3a76e into haraka:master Apr 11, 2024
12 checks passed
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