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

Question: Minion select interface or source IP address to use when connecting to the master #38744

Closed
mirceaulinic opened this issue Jan 15, 2017 · 17 comments

Comments

@mirceaulinic
Copy link
Member

commented Jan 15, 2017

Description of Issue/Question

I have been looking in the minion configuration options, but I didn't see anything related. Did I miss anything?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

I do not believe this is currently possible.

I see an interface option, but this appears to be only for setting which interface the master and syndic bind to, and not for which interface for the minion to use.

@gtmanfred gtmanfred added the Question label Jan 17, 2017
@gtmanfred gtmanfred added this to the Blocked milestone Jan 17, 2017
@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

I dug a bit into this and it looks like the Tornado TCPClient does not support specifying a certain source IP: tornadoweb/tornado#1931

If you don't mind, I'd prefer to keep this open to keep tracking of it.
Firstly I will try to introduce the required feature in Tornado (although I'm not aware of all details required) and then I'll come with a PR in Salt. So... it might take a while :)

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2017

@gtmanfred this feature has been introduced in Tornado's TCPClient: tornadoweb/tornado#1935 and will be included in the next release (probably soon).
Do you think we could introduce this feature in Nitrogen? If so, what plan would you suggest, considering that it would require a certain minimum Tornado version?

Thanks,
Mircea

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2017

I do not know.

@cachedout @thatch45 opinions?

Thanks,
Daniel

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

This is not currently an option, but yes it would be good to add. we can easily make something like this work with older versions of tornado too since it would just be passing through an option, before the option is set we would do a tornado version check and log an error that the tornado version is too old

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

Thanks @thatch45! This sounds like a good plan to me.
I am going to put this on hold till Tornado will have the source ip/port options and then I'll come with a PR.

@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

Awesome, thanks @mirceaulinic !

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2017

Tornado 4.5 and 4.5.1 have been released, supporting two arguments: source_ip and source_port. Would be acceptable to submit the necessary changes into Nitrogen, or develop?

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Looking a bit closer into it, seems that the configuration option I need is already there and it's called interface rather than source_ip (what I would expect). When referring to interface I would expect there to configure the name of the interface, rather than an IP address (or allow both). Anyway, that's no biggie, just food for thoughts and looking for more opinions.

Fortunately this looks like it's exactly what I need, but it's not documented under https://docs.saltstack.com/en/latest/ref/configuration/minion.html, but there's a brief note in the code: https://github.com/saltstack/salt/blob/develop/salt/config/__init__.py#L532-L533. Once someone confirms it's not just me biased and hoping for this be already exists, I will submit a documentation improvement adding this option. Meanwhile, I will give it a try. Thanks!

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

^ Please ignore my previous comment, I was celebrating too early.

Just realised that @gtmanfred already clarified that above...

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

I have been looking at implementing this, but I'm afraid I don't understand very well several details from the transport classes (i.e., https://github.com/saltstack/salt/tree/develop/salt/transport).

In particular, I'd like to know for the beginning who's who, what class is used by the master and what class is used by the minion.
The new source_ip and source_port options would probably be used only by the minion so I suppose they should only be added on the minion side, as it connects to the master.

Any pointer would be greatly appreciated.
Thanks!

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2017

Hi Mircea,

From the minion perspective, you really only need to look at salt.transport.client. You'll see use of AsyncPubChannel used to connect to the master publisher (TCP/4505 by default) and to the ReqServer which is TCP/4506. It sounds like you're just looking for minion options but you'll see the same Pub and Req nomenclature used on the server (master) side as well. Let me know if you'd like more detail there.

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

Thanks for looking into this @cachedout and info.

I have looked at the transport.client, but I don't quite grasp where would be the right place to use these new options.
I would have expected that we need to adjust the classes from each transport. For example, to use the source_ip and source_port for the ZeroMQ transport, we'd need to replace this line: https://github.com/saltstack/salt/blob/develop/salt/transport/zeromq.py#L455 (self.uri = 'tcp://{interface}:{ret_port}'.format(**self.opts)) with self.uri = 'tcp://{source_ip}:{source_port};{interface}:{ret_port}'.format(**self.opts) etc. Okay, maybe that isn't one of the right places to adjust, but I think you see what I mean?

Please let me know if I'm wrong and I should look elsewhere.

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2017

For the record, I have revisited this again, looks like ZeroMQ should be able to connect using a specific source IP/port combination:

http://api.zeromq.org/4-1:zmq-tcp

// Connecting using a IP address and bind to an IP address
rc = zmq_connect(socket, "tcp://192.168.1.17:5555;192.168.1.1:5555"); assert (rc == 0);

However, it seems that it doesn't (at least for REQ-REP sockets which seem to be used for the communication between the master and the minion): zeromq/pyzmq#1088 (comment) -- pending clarification if this option is really supported, etc. Pinging @saltstack/team-transport: am I right that the socket I need to adjust is the REQ-REP one?

I added the options though for the TCP transport: develop...cloudflare:source-ip-port-opts for a better overview of what I've done so far. Any feedback and suggestion much appreciated, thanks!

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2017

It actually does work with a newer version of ZMQ - still trying to find out when this has been introduced: somewhere between 14.4.0 and 16.0.3, but the release notes are not extremely detailed: https://github.com/zeromq/pyzmq/releases

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2017

PR: #44642

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

Closed via 8c34fb1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.