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

Bind outbound connection to address from configuration #124

Merged
merged 1 commit into from
Mar 21, 2015
Merged

Bind outbound connection to address from configuration #124

merged 1 commit into from
Mar 21, 2015

Conversation

ybubnov
Copy link

@ybubnov ybubnov commented Mar 19, 2015

For security reasons it would be great to support such feature as
binding outbound address, so nsqd could be firewalled on accepting
certain addresses, that reasonable in case when nsq-consumers or
producers are placed on machines with multiple interfaces.

@mreiferson
Copy link
Member

I'm not necessarily opposed to this change but can't this be addressed outside of go-nsq by configuring routing tables?

@twmb
Copy link
Contributor

twmb commented Mar 19, 2015

This will try to bind all outgoing connections to the same address. Unless I'm mistaken, if you set LocalAddr, this will fail when you have more than one server you're trying to connect to, as this will try to open two connections with the same source addr.

@ybubnov
Copy link
Author

ybubnov commented Mar 20, 2015

mreiferson, imagine machine with two NICs, when you start nsq listener or consumer, ip address of which NIC they will use? The answer is unpredictable, probably, it's possible to solve issue with iptables and masquerading of all outgoing ip addresses, but it's tricky, isn't it?

twmb, if you look into consumer.go you will find out, that it uses map of connections, where key is address of nsq daemon, so you can easily connect to multiple servers. Either, if you will use LocalAddr with port equal to 0, like config.Set("local_addr", "127.0.0.1:0") os find a free one and bind your client to that address, so there is no problems with multiple connections (they all will use different endpoints).

@twmb
Copy link
Contributor

twmb commented Mar 20, 2015

Cool, I hadn't read that yet. This looks fine to me other than binded should be bound in error messages. I'd change ConnTimeout to DialTimeout as the timeout is only used in dialing.

@ybubnov
Copy link
Author

ybubnov commented Mar 20, 2015

Thanks, fixed.

@mreiferson
Copy link
Member

mreiferson, imagine machine with two NICs, when you start nsq listener or consumer, ip address of which NIC they will use? The answer is unpredictable, probably, it's possible to solve issue with iptables and masquerading of all outgoing ip addresses, but it's tricky, isn't it?

Still feels wrong to me. I don't disagree that this patch addresses the issue, my concern is that this approach implies that all of the client libraries need a similar patch whereas, if you handled this at the networking layer, it would apply to anything connecting to nsqd.

Curious to hear @jehiah's thoughts...

@ybubnov
Copy link
Author

ybubnov commented Mar 20, 2015

If you leave LocalAddr parameter equals to nil clients would behave the same as before. It's an option, not a requirement.

http://golang.org/pkg/net/#Dialer

@jehiah
Copy link
Member

jehiah commented Mar 20, 2015

certainly won't be needed in most cases but i'm fine exposing this. 👍

@mreiferson
Copy link
Member

alright alright

}
return tval
}

func coerceString(v interface{}) (string, error) {
switch v.(type) {
switch v := v.(type) {
Copy link
Member

Choose a reason for hiding this comment

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

let's not shadow v here (and all the other instances below)

Copy link
Author

Choose a reason for hiding this comment

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

it's golang idiom - reuse name of variables in a type switch statements
https://golang.org/doc/effective_go.html#type_switch

Copy link
Member

Choose a reason for hiding this comment

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

TIL - thanks!

@mreiferson
Copy link
Member

@yashkin thanks, would you mind squashing down to one commit and we'll merge?

For security reasons it would be great to support such feature as
binding outbound address, so nsqd could be firewalled on accepting
certain addresses, that reasonable in case when nsq-consumers or
producers are placed on machines with multiple interfaces.
mreiferson added a commit that referenced this pull request Mar 21, 2015
Bind outbound connection to address from configuration
@mreiferson mreiferson merged commit f9995d9 into nsqio:master Mar 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants