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

allow origin ip/port in tcp/ssl connect options #98

Merged
merged 1 commit into from Dec 14, 2016

Conversation

lazedo
Copy link
Contributor

@lazedo lazedo commented Apr 7, 2016

No description provided.

@seriyps
Copy link
Collaborator

seriyps commented Apr 7, 2016

any notes / explanation?

@lazedo
Copy link
Contributor Author

lazedo commented Apr 7, 2016

@seriyps sure, sorry..., the use case is a server with lot of IP addresses that needs to connect to another relay server that unblocks the firewall only for one of the ip addresses, in this case not the default one.

@lazedo
Copy link
Contributor Author

lazedo commented Apr 11, 2016

@seriyps let me know if you need further description.
thanks

@seriyps
Copy link
Collaborator

seriyps commented Apr 11, 2016

But looks like you may just do

gen_smtp_client:send(..., [ {sockopts, [ {ip, {1, 2, 3, 4}}, {port, 1234} ]} ])

without this patch...

It's not documented, but it's there https://github.com/lazedo/gen_smtp/blob/patch-3/src/gen_smtp_client.erl#L505

And I can't understand how your patch will help with described problem.

@lazedo
Copy link
Contributor Author

lazedo commented Apr 11, 2016

socker:connect calls tcp_options and ssl_options which in turn call proplist_merge that will remove {ip, {x,x,x,x}}

@lazedo
Copy link
Contributor Author

lazedo commented Apr 11, 2016

to be clear, i use gen_smtp_client:send(..., [ {sockopts, [ {ip, {1, 2, 3, 4}}, {port, 1234} ]} ]) but it doesn't work without the patch because ip and port are removed by tcp_options or ssl_options due to the call to proplist_merge

@seriyps
Copy link
Collaborator

seriyps commented Apr 11, 2016

Ah, didn't noticed that proplist_merge drops keys, which are not defined in DefaultList.

I'm ok with this change as the simplest one, but maybe we should change the way how proplist_merge works? Eg, provide separate lists of allowed keys and default values? @Vagabond, @mworrell ?

@lazedo
Copy link
Contributor Author

lazedo commented May 8, 2016

@seriyps @Vagabond @mworrell any update here ?

1 similar comment
@lazedo
Copy link
Contributor Author

lazedo commented Dec 13, 2016

@seriyps @Vagabond @mworrell any update here ?

@mworrell
Copy link
Collaborator

I am ok with having explicit defaults.

(And yes, we should check that merge code... dropping elements doesn't sound intuitive.)

@mworrell mworrell merged commit 92785fd into gen-smtp:master Dec 14, 2016
@lazedo lazedo deleted the patch-3 branch March 1, 2017 23:47
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.

None yet

3 participants