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: enable_outbound can be set per domain #2335

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
2 participants
@gramakri
Collaborator

gramakri commented Feb 6, 2018

Fixes #
enable_outbound can be set per domain

Changes proposed in this pull request:
When haraka is managing multiple domains, it is sometimes desirable to use the mail relay only for specific domains. By making the enable_outbound flag per domain, we can disable the relay and use Haraka's internal MX based delivery.

Checklist:
[X] docs updated
[X] tests updated
[X ] Changes updated

@codecov

This comment has been minimized.

codecov bot commented Feb 6, 2018

Codecov Report

Merging #2335 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2335      +/-   ##
==========================================
- Coverage   59.61%   59.56%   -0.05%     
==========================================
  Files          28       28              
  Lines        6072     6072              
  Branches     1510     1510              
==========================================
- Hits         3620     3617       -3     
- Misses       2452     2455       +3
Impacted Files Coverage Δ
host_pool.js 93.33% <0%> (-4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49fc496...293bc6e. Read the comment docs.

@msimerson

This comment has been minimized.

Member

msimerson commented Feb 6, 2018

I'm +1 for making enable_outbound settings on a per-domain basis. This implementation seems excessively complicated and I'm curious why you implemented it this way.

I think the implementation would be simpler, consistent with how the rest of the per-domain options work, easier to reason about the config options (why does setting the main option to false disable the per-domain option?), and be more flexible if instead you used this logic:

• when per-domain enable_outbound is defined, honor it
• else use main.enable_outbound setting.

When implemented this way, you could have enable_outbound disabled by default (in main) and enable only for specific domains. This implementation excludes that use case.

@gramakri gramakri force-pushed the cloudron-io:enable_outbound branch from 1d18162 to bebd488 Feb 6, 2018

@gramakri

This comment has been minimized.

Collaborator

gramakri commented Feb 6, 2018

@msimerson Indeed, I have over-complicated it. I have updated the PR per your suggestion. Thanks!

@gramakri gramakri force-pushed the cloudron-io:enable_outbound branch from bebd488 to 5aa0507 Feb 6, 2018

exports.is_outbound_enabled = function (cfg) {
const plugin = this;
if ('enable_outbound' in cfg) return cfg.enable_outbound === 'true'; // pick up per-domain flag if set

This comment has been minimized.

@msimerson

msimerson Feb 6, 2018

Member

Are you sure this will do what you want? The in operator will fire if enable_outbound is defined in the cfg object. So you're going to return the string "true", even if the value of enable_outbound is boolean false. Shouldn't we be consistently using JS booleans instead of strings?

This comment has been minimized.

@gramakri

gramakri Feb 6, 2018

Collaborator

@msimerson The config values in subdomains are not parsed out as booleans. Is there a way to specify this in the booleans section when reading the config file?

This comment has been minimized.

@gramakri

gramakri Feb 6, 2018

Collaborator

Oh, I see *.enable_tls. Maybe I can do this for enable_outbound. trying now.

},
'per-domain enable_outbound is true even if top level is false' : function (test) {
test.expect(1);
this.plugin.cfg.enable_outbound = false; // this will be ignored

This comment has been minimized.

@msimerson

msimerson Feb 6, 2018

Member

In the PR I sent, I had corrected this path to: this.plugin.cfg.main.enable_outbound

This comment has been minimized.

@gramakri

gramakri Feb 6, 2018

Collaborator

Ah, I didn't see the PR at all and blindly force pushed. good catch.

'per-domain enable_outbound is true even if top level is false' : function (test) {
test.expect(1);
this.plugin.cfg.enable_outbound = false; // this will be ignored
this.plugin.cfg['test.com'].enable_outbound = 'true'; // this will be ignored

This comment has been minimized.

@msimerson

msimerson Feb 6, 2018

Member

Again, you should be using booleans here instead of a string.

},
'per-domain enable_outbound can be set to false' : function (test) {
test.expect(1);
this.plugin.cfg['test.com'].enable_outbound = 'false';

This comment has been minimized.

@msimerson

msimerson Feb 6, 2018

Member

again, use boolean instead of string.

smtp_forward: enable_outbound can be set per domain
When haraka is handling multiple domains, this option lets
one to disable outbound relay for specific domains

@gramakri gramakri force-pushed the cloudron-io:enable_outbound branch from 5aa0507 to 0f4aaf9 Feb 6, 2018

@msimerson

msimerson approved these changes Feb 6, 2018 edited

LGTM. I'm going to give another pair of eyes a chance to review before merging.

@msimerson msimerson merged commit 7c4cf78 into haraka:master Feb 8, 2018

3 of 4 checks passed

codecov/project 59.56% (-0.05%) compared to 49fc496
Details
codecov/patch Coverage not affected when comparing 49fc496...293bc6e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment