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

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