search

lmco / laikaboss

Laika BOSS: Object Scanning System
Apache License 2.0
732 stars 156 forks source link

IPv6 support #26

Open mtmcgrew opened 8 years ago

mtmcgrew commented 8 years ago

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

mtmcgrew commented 8 years ago

ipv6 yo

mtmcgrew commented 8 years ago

cc @marpaia

marnao commented 8 years ago

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: https://github.com/saltstack/salt/issues/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 commented 8 years ago

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 commented 8 years ago

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

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

mtmcgrew commented 8 years ago

Awesome, done.

marnao commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.