Opened 3 years ago

Closed 3 years ago

#5084 enhancement closed fixed (fixed)

Accept IPv6 address literals (with embedded scope ids) in IReactorTCP.listenTCP

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: ipv6
Cc: p.mayers@… Branch: branches/listentcp-ipv6-5084-4
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

As an early step towards good IPv6 support, the reactor needs to be able to set up listening TCP ports on IPv6 addresses. Since previously it was an error to pass an IPv6 literal to any listenTCP implementation, we can begin supporting that without breaking any existing software.

Nothing about how IPv4 support is provided will change - IPv4 address literals and hostnames will continue to listen only on an IPv4 address. The default will still be to listen on INADDR_ANY, which does not include an IPv6 addresses.

The resulting IListeningPort will return an IPv6Address from its getHost implementations.

When a connection is established to the server, the factory's buildProtocol method will be invoked with an IPv6Address instance giving the peer's address.

The protocol will be connected to an ITCPTransport implementation which also returns IPv6Address instances from its getHost and getPeer implementations.

ITCPTransport.getPeer and ITCPTransport.getHost are documented as returning IPv4Address instances. This should probably change.

IPv6Address is very similar to IPv4Address. It has host and port attributes. It might even be possible to use the same class for each of these, but having two makes it easier to tell the address family apart so I think it's a good idea. If we ever support scopeid or flowid more explicitly, it may be useful to expose those as attributes on IPv6Address as well.

Once this is resolved, Twisted servers should be able to listen for TCP connections on IPv6 addresses, eg ::1 or fe80::1%lo0 or ::. Note that where the platform requires the scope id, listenTCP will require the scope id.

Change History (16)

comment:1 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/listentcp-ipv6-5084

(In [31770]) Branching to 'listentcp-ipv6-5084'

comment:2 Changed 3 years ago by philmayers

This sounds good.

Are you writing this code, or do you already have the code?

Or are you looking for a contribution to implement this?

comment:3 Changed 3 years ago by philmayers

  • Cc p.mayers@… added

comment:4 Changed 3 years ago by exarkun

(In [31771]) A simple unit test which will probably require much of a working listenTCP(ipv6) implementation before it passes

refs #5084

comment:5 Changed 3 years ago by exarkun

Are you writing this code, or do you already have the code?

As evident from above, I'm working on this a bit. :) I'm not promising I'll finish it, but it seemed like it would be somewhat easy to grab the relevant pieces of the patch on #3014 to implement just this part of things. One thing I want to ensure, though, is that the unit tests are as good as we can make them. A lot of the existing unit tests, particularly the ones in twisted.internet.test, are very complicated and test only the funkiest edge cases. We should make sure all the cases of IPv6 support are tested, not just the weird edge cases.

comment:6 Changed 3 years ago by exarkun

  • Branch changed from branches/listentcp-ipv6-5084 to branches/listentcp-ipv6-5084-2

(In [31813]) Branching to 'listentcp-ipv6-5084-2'

comment:7 Changed 3 years ago by exarkun

  • Branch changed from branches/listentcp-ipv6-5084-2 to branches/listentcp-ipv6-5084-3

(In [32029]) Branching to 'listentcp-ipv6-5084-3'

comment:8 Changed 3 years ago by exarkun

There's some working stuff in the branch. Scope ids aren't working yet though. For the IPv6 case we need to use getaddrinfo to turn the "%"-separated suffix into a numeric scope id and then pass the numeric scope id to bind.

comment:9 Changed 3 years ago by exarkun

To test the link local parts, I resorted to a new third-party dependency, netifaces. Only the tests require it.

comment:10 Changed 3 years ago by exarkun

netifaces doesn't support IPv6 on Windows.

comment:11 Changed 3 years ago by exarkun

  • Keywords review added

PenguinOfDoom implemented a netifaces equivalent that satisfies our requirements. I implemented one that works on Linux and OS X. So netifaces is not a dependency of this branch now.

The build results are here; additionally I forced an OpenBSD build just to see what would happen. The scope id tests didn't run because no link local address was found (whether that's because the machine has none or because of a problem with the detection code, I don't know), but the rest of the IPv6 tests did and passed.

I expect that once this code looks okay, it will be merged into an IPv6 roll-up branch, to be joined by the other planned IPv6 functionality.

comment:12 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  1. It would interesting to change tcp.Port __repr__, as well as the message in startListening, to denotate that the Port is listening on IPV6.
  1. _posixifaces.py and _win32ifaces.py are missing several blank lines between module members.
  1. Talking about those, they are really cool. It's a shame they're hidden in test and private: what about creating a ticket for providing that functionality (list the interfaces)?

Anyway looks good, buildbot seems happy, and it works locally, so +1 from me.

comment:13 Changed 3 years ago by exarkun

  • Branch changed from branches/listentcp-ipv6-5084-3 to branches/listentcp-ipv6-5084-4

(In [33307]) Branching to 'listentcp-ipv6-5084-4'

comment:14 Changed 3 years ago by exarkun

(In [33310]) whitespace

refs #5084

comment:15 Changed 3 years ago by exarkun

Thanks very much for the review. I'm giving up on the roll-up branch plan. The tools (bzr-svn) aren't quite up to the workflow I'd imagined. This feature is internally consistent, though it doesn't represent complete IPv6 support for Twisted. To avoid carrying this code in a branch indefinitely while waiting for better tools, I'm just going to merge this now. I addressed the whitespace review comment in r33310 and I filed #5426 for the Port.startListening enhancement.

comment:16 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [33351]) Merge listentcp-ipv6-5084-4

Author: exarkun, PenguinOfDoom
Reviewer: therve
Fixes: #5084

Add support for passing an IPv6 address literal as the interface to
IReactorTCP.listenTCP implementations.

Note: See TracTickets for help on using tickets.