[Twisted-Python] RFC: IPv6 multicast join/ticket 6597
Hello, I'm looking at making the changes to support IPv6 multicast groups as described in ticket 6597 but wanted to get some feedback (and get a feel whether this is even desirable) before formally submitting any patches. Specifically: 1. I've read [1] and it alludes to udp hopefully disappearing, is that something in the works? Is there a new approach to solving this problem I should look at? A branch in the works where this (conceptual) change belongs? Note: The addressFamily attribute referenced already exists and is set properly for IPv6. 2. The attached change has the side effect that calls to ReactorBase.resolve() with IPv6 literals will now likely succeed where they may have failed in the past. That means clients counting on resolve raising an exception for an IPv6 literal will break. Not sure whether this is considered a compatibility issue, but I wanted to raise it. 3. One alternative to the above would be a complete API separation, via something like joinIPv6Group(), and a new resolve. Is that more appealing in this case? Caveats: 1. I have not finished all of the documentation related to developers, I will do so prior to formal submission. 2. I know I need tests and docs and will submit them with the final changes. On to the patches. With these changes, I can use the joinGroup API to add myself to an IPv6 multicast group on Linux (verified via /proc/net/igmp6). Additionally, trial reports the same two failures before and after these changes (twisted.python.test.test_release.APIBuilderTests.test_build and test_buildWithPolicy). These changes struck me as the obvious approach, but given the changes to resolve, not necessarily the best. diff --git a/twisted/internet/base.py b/twisted/internet/base.py index 4f2c862..e813741 100644 --- a/twisted/internet/base.py +++ b/twisted/internet/base.py @@ -567,6 +567,8 @@ class ReactorBase(object): return defer.succeed('0.0.0.0') if abstract.isIPAddress(name): return defer.succeed(name) + elif abstract.isIPv6Address(name): + return defer.succeed(name) return self.resolver.getHostByName(name, timeout) # Installation. diff --git a/twisted/internet/udp.py b/twisted/internet/udp.py index b5a5322..210b079 100644 --- a/twisted/internet/udp.py +++ b/twisted/internet/udp.py @@ -485,7 +485,10 @@ class MulticastMixin: def _joinAddr1(self, addr, interface, join): - return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join) + if self.addressFamily == socket.AF_INET: + return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join) + else: + return self.reactor.resolve(interface).addCallback(self._joinAddrIPv6, addr, join) def _joinAddr2(self, interface, addr, join): @@ -500,6 +503,18 @@ class MulticastMixin: except socket.error as e: return failure.Failure(error.MulticastJoinError(addr, interface, *e.args)) + def _joinAddrIPv6(self, interface, addr, join): + addr = socket.inet_pton(socket.AF_INET6, addr) + interface = socket.inet_pton(socket.AF_INET6, interface) + if join: + cmd = socket.IPV6_JOIN_GROUP + else: + cmd = socket.IPV6_LEAVE_GROUP + try: + self.socket.setsockopt(socket.IPPROTO_IPV6, cmd, addr + interface) + except socket.error as e: + return failure.Failure(error.MulticastJoinError(addr, interface, *e.args)) + def leaveGroup(self, addr, interface=""): """Leave multicast group, return Deferred of success.""" This does require the client specify the interface argument when calling joinGroup, e.g. self.transport.joinGroup("ff02::1", interface="::"). Thanks in advance for any feedback! -Jason Litzinger [1] http://twistedmatrix.com/pipermail/twisted-python/2016-March/030188.html
On Jul 25, 2016, at 9:43 PM, Jason Litzinger <jlitzingerdev@gmail.com> wrote:
Hello,
I'm looking at making the changes to support IPv6 multicast groups as described in ticket 6597 but wanted to get some feedback (and get a feel whether this is even desirable) before formally submitting any patches.
Thanks for taking this up!
Specifically:
1. I've read [1] and it alludes to udp hopefully disappearing, is that something in the works? Is there a new approach to solving this problem I should look at? A branch in the works where this (conceptual) change belongs?
Note: The addressFamily attribute referenced already exists and is set properly for IPv6.
'twisted.internet.udp', as an importable module; not 'udp' as a feature of Twisted (or of the Internet, for that matter).
2. The attached change has the side effect that calls to ReactorBase.resolve() with IPv6 literals will now likely succeed where they may have failed in the past. That means clients counting on resolve raising an exception for an IPv6 literal will break. Not sure whether this is considered a compatibility issue, but I wanted to raise it.
We have explicitly avoided adding IPv6 name resolution to the reactor because the reactor's API for name resolution is fundamentally the wrong shape for IPv6. If you want to add the ability to resolve IPv6 names to the reactor itself, please see this ticket: https://twistedmatrix.com/trac/ticket/4362 <https://twistedmatrix.com/trac/ticket/4362> For the purposes of this ticket alone, you should probably just skip resolution in _joinAddr1 if resolution is
3. One alternative to the above would be a complete API separation, via something like joinIPv6Group(), and a new resolve. Is that more appealing in this case?
Given what we've done with connectTCP et. al., it makes sense to leave 'joinGroup' as the API for doing this. But we probably want to leave '.resolve' alone.
Caveats:
1. I have not finished all of the documentation related to developers, I will do so prior to formal submission.
I think we can do the narrative docs in a separate PR, as the interface looks like the straightforward expansion of the IPv4 interface. You should clean up the reference documentation (i.e. docstrings) to ensure they're accurate of course.
2. I know I need tests and docs and will submit them with the final changes.
Testing multicast is ... challenging. I barely have any idea how to set up a test environment for IPv4, and no idea what to do for IPv6. If you can speak to this in your tests (and hopefully docs as well) that would be super helpful.
On to the patches. With these changes, I can use the joinGroup API to add myself to an IPv6 multicast group on Linux (verified via /proc/net/igmp6). Additionally, trial reports the same two failures before and after these changes (twisted.python.test.test_release.APIBuilderTests.test_build and test_buildWithPolicy). These changes struck me as the obvious approach, but given the changes to resolve, not necessarily the best.
diff --git a/twisted/internet/base.py b/twisted/internet/base.py index 4f2c862..e813741 100644 --- a/twisted/internet/base.py +++ b/twisted/internet/base.py @@ -567,6 +567,8 @@ class ReactorBase(object): return defer.succeed('0.0.0.0') if abstract.isIPAddress(name): return defer.succeed(name) + elif abstract.isIPv6Address(name): + return defer.succeed(name) return self.resolver.getHostByName(name, timeout)
# Installation. diff --git a/twisted/internet/udp.py b/twisted/internet/udp.py index b5a5322..210b079 100644 --- a/twisted/internet/udp.py +++ b/twisted/internet/udp.py @@ -485,7 +485,10 @@ class MulticastMixin:
def _joinAddr1(self, addr, interface, join): - return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join) + if self.addressFamily == socket.AF_INET: + return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join) + else: + return self.reactor.resolve(interface).addCallback(self._joinAddrIPv6, addr, join)
def _joinAddr2(self, interface, addr, join): @@ -500,6 +503,18 @@ class MulticastMixin: except socket.error as e: return failure.Failure(error.MulticastJoinError(addr, interface, *e.args))
+ def _joinAddrIPv6(self, interface, addr, join): + addr = socket.inet_pton(socket.AF_INET6, addr) + interface = socket.inet_pton(socket.AF_INET6, interface) + if join: + cmd = socket.IPV6_JOIN_GROUP + else: + cmd = socket.IPV6_LEAVE_GROUP + try: + self.socket.setsockopt(socket.IPPROTO_IPV6, cmd, addr + interface) + except socket.error as e: + return failure.Failure(error.MulticastJoinError(addr, interface, *e.args)) +
def leaveGroup(self, addr, interface=""): """Leave multicast group, return Deferred of success."""
This does require the client specify the interface argument when calling joinGroup, e.g. self.transport.joinGroup("ff02::1", interface="::").
Thanks in advance for any feedback!
-Jason Litzinger
[1] http://twistedmatrix.com/pipermail/twisted-python/2016-March/030188.html
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Thanks for taking this up!
No problem, do I need to reflect anything in the Ticket to indicate I'm looking at it?
'twisted.internet.udp', as an importable module; not 'udp' as a feature of Twisted (or of the Internet, for that matter).
My question was poorly phrased, I assumed that it was the module that was problematic, and wondered if anyone was working on an alternative where this is better suited.
We have explicitly avoided adding IPv6 name resolution to the reactor because the reactor's API for name resolution is fundamentally the wrong shape for IPv6. If you want to add the ability to resolve IPv6 names to the reactor itself, please see this ticket: https://twistedmatrix.com/trac/ticket/4362 <https://twistedmatrix.com/trac/ticket/4362>
For the purposes of this ticket alone, you should probably just skip resolution in _joinAddr1 if resolution is
I assume you mean skip resolution in joinGroup as well? That's the only way to avoid resolve completely. Additionally, any objections to me updating setTTL in this patch? It's pretty common to set the hop limit when doing a multicast. Not required, but common.
Testing multicast is ... challenging. I barely have any idea how to set up a test environment for IPv4, and no idea what to do for IPv6. If you can speak to this in your tests (and hopefully docs as well) that would be super helpful.
Indeed. I have an interest beyond the scope of this change so I'll see what I can do/find. Thanks! -Jason Litzinger
On Jul 26, 2016, at 11:00 PM, Jason Litzinger <jlitzingerdev@gmail.com> wrote:
Thanks for taking this up!
No problem, do I need to reflect anything in the Ticket to indicate I'm looking at it?
Nothing specific, although any conclusions drawn on the mailing list, or any specific thoughts you have about how you're going to proceed, are always helpful to record on the ticket for future reference. Even if you think you're going to get it done in the next couple of days, chances are you'll take an 18 month hiatus in the middle and it's always helpful to have those notes to come back to :).
'twisted.internet.udp', as an importable module; not 'udp' as a feature of Twisted (or of the Internet, for that matter). My question was poorly phrased, I assumed that it was the module that was problematic, and wondered if anyone was working on an alternative where this is better suited.
The module's implementation is actually fine; the only problem is that it's exposed to third-party applications, and that there are some things that those applications can't achieve without it being so exposed. We need to make it private, but first, we need to address all the issues like this :).
We have explicitly avoided adding IPv6 name resolution to the reactor because the reactor's API for name resolution is fundamentally the wrong shape for IPv6. If you want to add the ability to resolve IPv6 names to the reactor itself, please see this ticket: https://twistedmatrix.com/trac/ticket/4362 <https://twistedmatrix.com/trac/ticket/4362>
For the purposes of this ticket alone, you should probably just skip resolution in _joinAddr1 if resolution is
I assume you mean skip resolution in joinGroup as well? That's the only way to avoid resolve completely.
Right, I meant to check isIPv6Address in joinGroup and everything it calls.
Additionally, any objections to me updating setTTL in this patch? It's pretty common to set the hop limit when doing a multicast. Not required, but common.
Smaller patches are better. What do you want to 'update' about it? If it's an independent change, just submit a different ticket and it will probably land quickly if it's an obvious fix.
Testing multicast is ... challenging. I barely have any idea how to set up a test environment for IPv4, and no idea what to do for IPv6. If you can speak to this in your tests (and hopefully docs as well) that would be super helpful.
Indeed. I have an interest beyond the scope of this change so I'll see what I can do/find.
If you have an interest in UDP generally, a fix for this horribly embarrassing and probably pretty important bug <https://twistedmatrix.com/trac/ticket/2790 <https://twistedmatrix.com/trac/ticket/2790>> would be (A) super appreciated, and (B) really straightforward (the ambivalence on the ticket is all about how to test it "realistically", but a straightforward unit test with a fake socket would probably be fine). Thanks again, -glyph
Nothing specific, although any conclusions drawn on the mailing list, or any specific thoughts you have about how you're going to proceed, are always helpful to record on the ticket for future reference. Even if you think you're going to get it done in the next couple of days, chances are you'll take an 18 month hiatus in the middle and it's always helpful to have those notes to come back to :).
Agree completely, done.
Smaller patches are better. What do you want to 'update' about it? If it's an independent change, just submit a different ticket and it will probably land quickly if it's an obvious fix. It isn't so much update as I think it is equally broken for IPv6. If I'm not misunderstanding my reference (UNPv3), socket.IP_MULTICAST_TTL is specific to IPv4, while IPV6_MULTICAST_HOPS is required for v6. I attempted to use the former in some IPv6 test code and, surprise surprise, inspecting the IP header revealed it didn't work. Using the latter worked.
Regardless, I agree, smaller patches keep things sane, I'll put this on the queue after joinGroup. Thanks, -Jason
+ def _joinAddrIPv6(self, interface, addr, join): + addr = socket.inet_pton(socket.AF_INET6, addr) + interface = socket.inet_pton(socket.AF_INET6, interface) + if join: + cmd = socket.IPV6_JOIN_GROUP + else: + cmd = socket.IPV6_LEAVE_GROUP + try: + self.socket.setsockopt(socket.IPPROTO_IPV6, cmd, addr + interface) + except socket.error as e: + return failure.Failure(error.MulticastJoinError(addr, interface, *e.args)) To make sure its out there, the above is completely wrong. It happens to work, but is wrong. The argument to setsockopt for v6 is very different from v4. The above happens to work with "::" as the interface, but is very very wrong.
-Jason
Testing multicast is ... challenging. I barely have any idea how to set up a test environment for IPv4, and no idea what to do for IPv6. If you can speak to this in your tests (and hopefully docs as well) that would be super helpful. On that note, one effect of actually testing that a registered socket receives data is that, if no interfaces are up, the tests are going to start failing (passing tests).
The existing Multicast test class exibits the same behavior on my machine if I take my interfaces down, so can I assume that this is an acceptable constraint? The constraint being that, if no network interfaces are up, these tests are going to start failing, at least on Linux. I'm looking for a better option, but haven't found one as of yet. Thanks! -Jason Litzinger
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On 4 Aug 2016, at 04:59, Jason Litzinger <jlitzingerdev@gmail.com> wrote:
Testing multicast is ... challenging. I barely have any idea how to set up a test environment for IPv4, and no idea what to do for IPv6. If you can speak to this in your tests (and hopefully docs as well) that would be super helpful. On that note, one effect of actually testing that a registered socket receives data is that, if no interfaces are up, the tests are going to start failing (passing tests).
The existing Multicast test class exibits the same behavior on my machine if I take my interfaces down, so can I assume that this is an acceptable constraint? The constraint being that, if no network interfaces are up, these tests are going to start failing, at least on Linux.
When you say *no* interfaces, do you mean actually no interfaces? As in, no loopback as well? Cory
When you say *no* interfaces, do you mean actually no interfaces? As in, no loopback as well?
I do not, sorry, that was poorly worded. The loopback interface was up for all test runs, and all other interfaces on my machine (ethernet and wifi) were down. Down in this context has two meanings (I tested both): 1. I manually took the interface down with "ifconfig <iface> down" 2. I disconnected from my wireless network, so I had no address assigned to any interface save the loopback. In both cases, tests that pass with connectivity will fail due to the index parameter. I did try specifying the loopback index, however, I received a "Network Unreachable" error. I didn't have time to dig into whether it was possible to do what I wanted with the loopback interface last night. Thanks, -Jason *Note: I'm using my gmail client for the reply here instead of mutt...hopefully it isn't garbled.
On 4 Aug 2016, at 17:03, Jason Litzinger <jlitzingerdev@gmail.com> wrote:
When you say *no* interfaces, do you mean actually no interfaces? As in, no loopback as well?
I do not, sorry, that was poorly worded. The loopback interface was up for all test runs, and all other interfaces on my machine (ethernet and wifi) were down. Down in this context has two meanings (I tested both): 1. I manually took the interface down with "ifconfig <iface> down" 2. I disconnected from my wireless network, so I had no address assigned to any interface save the loopback.
In both cases, tests that pass with connectivity will fail due to the index parameter.
I did try specifying the loopback index, however, I received a "Network Unreachable" error. I didn't have time to dig into whether it was possible to do what I wanted with the loopback interface last night.
So with the caveat that I am not the author of most of these tests, and I don’t understand their special circumstances… My intuition is that no interface other than the loopback should be required to run the Twisted tests. The loopback should be sufficient to test pretty much anything that Twisted has to test. As a result, if things fail in that scenario, I’d consider it a bug. Cory
On Thu, 4 Aug 2016 at 18:04 Jason Litzinger <jlitzingerdev@gmail.com> wrote:
I did try specifying the loopback index, however, I received a "Network Unreachable" error. I didn't have time to dig into whether it was possible to do what I wanted with the loopback interface last night.
By default, the "multicast" flag is not enabled for the loopback interface on Linux. Does running "ip link set dev lo multicast on" help here? (Although requiring this would then be rather problematic for testing, as doing this in containerized environments and restricted environments like Travis is presumably impossible)
By default, the "multicast" flag is not enabled for the loopback interface on Linux. Does running "ip link set dev lo multicast on" help here? (Although requiring this would then be rather problematic for testing, as doing this in containerized environments and restricted environments like Travis is presumably impossible) It didn't seem to. Despite the subject, I'll focus on the existing, IPv4 tests, rather than the new tests I'm writing (to eliminate any issues in my code, though the change didn't help there either).
When I run with at least one interface up, my results for `./bin/trial twisted` at SHA baba4c661a6606a56d8dab053cfe7b336a3c593e are: PASSED (skips=2140, successes=9472) When I take disconnect my wireless, and manually take down my lxr interface, `ip link` shows: 1: lo: <LOOPBACK,MULTICAST,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1 link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 2: enp1s0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000 link/ether <removed> brd ff:ff:ff:ff:ff:ff 3: wlp2s0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state DOWN mode DORMANT group default qlen 1000 link/ether <removed> brd ff:ff:ff:ff:ff:ff 4: lxcbr0: <BROADCAST,MULTICAST> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000 link/ether <removed> brd ff:ff:ff:ff:ff:ff And the results of `./bin/trial` are: FAILED (skips=2212, failures=3, errors=8, successes=9393) Specific to multicast, one of the errors is: Traceback (most recent call last): Failure: twisted.internet.error.MulticastJoinError: ('\xe1\x00\x00\xfa', '\x00\x00\x00\x00', 19, 'No such device') twisted.test.test_udp.MulticastTests.test_multicast However, I can eliminate the error for test_multicast with the following patch, and this works even when multicast is not explicitly enabled on the loopback. diff --git a/twisted/test/test_udp.py b/twisted/test/test_udp.py index 6cf4583..79fd9d0 100644 --- a/twisted/test/test_udp.py +++ b/twisted/test/test_udp.py @@ -623,10 +623,10 @@ class MulticastTests(unittest.TestCase): received from it. """ c = Server() - p = reactor.listenMulticast(0, c) + p = reactor.listenMulticast(0, c, interface="127.0.0.1") addr = self.server.transport.getHost() - joined = self.server.transport.joinGroup("225.0.0.250") + joined = self.server.transport.joinGroup("225.0.0.250", interface="127.0.0.1") def cbJoined(ignored): d = self.server.packetReceived = Deferred() Unfortunately, a similar change on my IPv6 code didn't yield the results I was hoping for, but, I assume that's my problem. Is there interest in a separate patch for these tests? I assume a new ticket? Thanks, -Jason Litzinger
An update for those interested. I've been hoping for a solution to test IPv6 multicast using only the loopback interface. Unfortunately, the only working (on Linux) solution I've found is to manually add a route for the multicast prefix used in the tests. E.g. ip -6 route add ff01::/16 dev lo Without this, and assuming I specify the loopback interface index, a "Network Unreachable" error results. I'm fairly certain the culprit is a kernel route: unreachable default dev lo table unspec proto kernel metric 4294967295 error -101 pref medium local ::1 dev lo table local proto none metric 0 pref medium unreachable default dev lo table unspec proto kernel metric 4294967295 error -101 pref medium That's the default output of `ip -6 route show table all` on my machine (Ubuntu 16.04). I haven't found much information on why that route exists (let alone twice), so I'm going to have to turn to the kernel source. Regardless, route manipulation for tests is a non-starter, so unless there's a way to flag the tests as requiring a network interface, it'll be a while before I submit patches on the original feature. Thanks for all the suggestions/feedback and I welcome any more. -Jason
participants (4)
-
Cory Benfield -
Glyph Lefkowitz -
Jason Litzinger -
Tristan Seligmann