[Twisted-Python] RFC: IPv6 multicast join/ticket 6597

[Jason Litzinger]

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

[Glyph Lefkowitz]

Post by Jason Litzinger
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!

Post by Jason Litzinger
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).

Post by Jason Litzinger
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

Post by Jason Litzinger
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.

Post by Jason Litzinger
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.

Post by Jason Litzinger
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.

Post by Jason Litzinger
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
return defer.succeed('0.0.0.0')
return defer.succeed(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
- return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join)
+ return self.reactor.resolve(interface).addCallback(self._joinAddr2, addr, join)
+ return self.reactor.resolve(interface).addCallback(self._joinAddrIPv6, addr, join)
return failure.Failure(error.MulticastJoinError(addr, interface, *e.args))
+ addr = socket.inet_pton(socket.AF_INET6, addr)
+ interface = socket.inet_pton(socket.AF_INET6, interface)
+ cmd = socket.IPV6_JOIN_GROUP
+ cmd = socket.IPV6_LEAVE_GROUP
+ self.socket.setsockopt(socket.IPPROTO_IPV6, cmd, addr + interface)
+ return failure.Failure(error.MulticastJoinError(addr, interface, *e.args))
+
"""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
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Jason Litzinger]

Post by Glyph Lefkowitz
Thanks for taking this up!

No problem, do I need to reflect anything in the Ticket to indicate I'm
looking at it?

Post by Glyph Lefkowitz
'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.

Post by Glyph Lefkowitz
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.

Post by Glyph Lefkowitz
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

[Glyph Lefkowitz]

Post by Jason Litzinger

Post by Glyph Lefkowitz
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 :).

Post by Jason Litzinger

Post by Glyph Lefkowitz
'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 :).

Post by Jason Litzinger

Post by Glyph Lefkowitz
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.

Post by Jason Litzinger
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.

Post by Jason Litzinger

Post by Glyph Lefkowitz
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

[Jason Litzinger]

Post by Glyph Lefkowitz
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.

Post by Glyph Lefkowitz
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

[Jason Litzinger]

Post by Jason Litzinger

Post by Glyph Lefkowitz
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

Post by Jason Litzinger
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Cory Benfield]

Post by Jason Litzinger

Post by Glyph Lefkowitz
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

[Jason Litzinger]

Post by Cory Benfield
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.

[Cory Benfield]

Post by Jason Litzinger

Post by Cory Benfield
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
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

[Tristan Seligmann]

Post by Jason Litzinger
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)

[Jason Litzinger]

Post by Tristan Seligmann
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

[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

[Jason Litzinger]

Post by Jason Litzinger
+ addr = socket.inet_pton(socket.AF_INET6, addr)
+ interface = socket.inet_pton(socket.AF_INET6, interface)
+ cmd = socket.IPV6_JOIN_GROUP
+ cmd = socket.IPV6_LEAVE_GROUP
+ self.socket.setsockopt(socket.IPPROTO_IPV6, cmd, addr + interface)
+ 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