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

IPv6 support #26

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

IPv6 support #26

wants to merge 4 commits into from

Conversation

mtmcgrew
Copy link
Contributor

This diff allows laikad to bind to IPv6 interfaced when available.

@mtmcgrew
Copy link
Contributor Author

ipv6 yo

@mtmcgrew
Copy link
Contributor Author

mtmcgrew commented Jan 4, 2016

cc @marpaia

@marnao
Copy link
Contributor

marnao commented Jan 5, 2016

Michael,

Thanks very much for the contribution! We appreciate it. I have some feedback on this pull request below:

I have some concerns about enabling IPv6 support by default given some of the research I've done.

It appears that in ZeroMQ 3.1, IPv6 support was added as "experimental" and I don't see anything in the release notes following that declaration. I read reports of IPv4 connections not working properly (on 3.x) with IPv6 support enabled. (see: saltstack/salt#2851 ) Given that 3.x is still considered stable and used by many distributions, we should keep IPv6 off by default. This may well be fixed in 4.x but I can't find any indication either way.

Would you please add a config option to laikad to enable IPv6 (off by default)? Also note that in ZeroMQ 4.x the IPV4ONLY option was deprecated and renamed to IPV6. Perhaps you could account for both cases?

Thanks,
Matt

@mtmcgrew
Copy link
Contributor Author

mtmcgrew commented Jan 5, 2016

Good feedback, done.

Accounting for different version of ZeroMQ is tricky. We can only get the version of pyzmq by doing

import zmq 
print zmq.__version__

This will give us the version of pyzmq (eg. 14.0.1) but not the version of ZeroMQ it was compiled against. The best way I can think of to solve this is to try to use zmq.IPV6 and if fails with an AttributeError due to it not being in zmq.h it will then use zmq.IPV4ONLY

@marnao
Copy link
Contributor

marnao commented Jan 5, 2016

You could use hasattr on zmq to determine if IPV6 is available.

import zmq
hasattr(zmq, 'IPV6')
False
hasattr(zmq, 'IPV4ONLY')
True

@mtmcgrew
Copy link
Contributor Author

mtmcgrew commented Jan 5, 2016

Awesome, done.

@marnao
Copy link
Contributor

marnao commented Jan 5, 2016

One more thing-- there's an additional context created by the worker here: https://github.com/mtmcgrew/laikaboss/blob/832fd9a37c3bf1a5f2d1e3f2f036300d3ac46430/laikad.py#L648

If the worker is connecting to the broker on an IPv6 address you'll also want to enable it there.

@marnao
Copy link
Contributor

marnao commented Jan 5, 2016

Awesome, thanks for the quick updates and formatting cleanup along the way!

We will do a little bit of testing and get this merged in ASAP.

Again, appreciate the contributions.

@marnao marnao self-assigned this Jan 5, 2016
@marnao
Copy link
Contributor

marnao commented Jan 18, 2016

As I was testing this I realized there was an associated change needed to the client library to enable IPv6 on the client's socket. Would you like to contribute that as well? If not I'll add it separately.

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