Opened 7 years ago

Last modified 3 years ago

#3014 enhancement assigned

Support IPv6 at least as well as IPv4 is supported

Reported by: dripton Owned by: dripton
Priority: high Milestone:
Component: core Keywords: ipv6
Cc: therve, cassidy, exarkun, jknight, thijs, nicolas.pouillard@…, martin@…, andyhhp@…, thomas@…, jhonglei@… Branch: branches/tcp-ipv6-3014
(diff, github, buildbot, log)
Author: dripton, victorvieux Launchpad Bug:

Description (last modified by exarkun)

Twisted should provide functionality for IPv6 addresses equivalent to that provided for IPv4 addresses.

See IPv6 and tickets with keyword ipv6 for progress on this issue. This ticket will be resolved when a fair amount of progress on those other more specific tickets has been made.

(Disregard the attachments, comments, and branch for this ticket; they served to arrive at a plan for supporting IPv6 but many of them are now out of date.)

Attachments (7)

ipv6.2.patch (56.1 KB) - added by dripton 7 years ago.
Version 3 of the ipv6 patch
ipv6.patch (56.1 KB) - added by dripton 7 years ago.
Version 3 of the ipv6 patch
ipv6.log (16.3 KB) - added by exarkun 6 years ago.
the irc conversation
test_test_application.py.patch (1.0 KB) - added by victorvieux 5 years ago.
Fix TCP6Server and TCP6Client unittest
twisted_application_internet.py.patch (1.7 KB) - added by victorvieux 5 years ago.
TCP6Server & TCP6Client
twisted-ipv6.patch (15.7 KB) - added by philmayers 4 years ago.
Handles scope ID on connectTCP addresses; may need some more work server side
Twisted-11.0.0-ipv6.chjurk.diff (17.3 KB) - added by chjurk 3 years ago.
IPv6 patch for Twisted 11.0.0

Download all attachments as: .zip

Change History (96)

comment:1 Changed 7 years ago by glyph

  • Owner glyph deleted

comment:2 Changed 7 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to dripton

As you said, there is a massive code duplication, and this is not acceptable. At first sight, the differences looks pretty minimal. To give an example of one way to do it:
in BaseClient we currently use abstract.isIPAddress, you use abstract.isIPv6Address. Just create a class variable testIPAddress = abstract.isIPAddress in BaseClient , and override it in BaseClientTCP6. It's probably not that simple everywhere, but I think it's a good opportunity to clean up tcp a bit, which is a bit rusty.

The same apply for the tests, where we should parametrize IPV4 tests to be able to specify the connection and listen methods in subclasses.

Overall, the problem is that twisted.internet.tcp is a very bad module to get inspiration from. If you're willing to clean it up a bit so that tcp6 becomes straightforward, that would be awesome.

comment:3 Changed 7 years ago by dripton

  • Keywords review added
  • Priority changed from normal to highest

Here's a better patch. twisted.internet._tcp6.py was merged into tcp.py. test_tcp6.py was merged into test_tcp.py. Less code duplication, same functionality.

comment:4 Changed 7 years ago by therve

  • Owner changed from dripton to therve

That's much much better :). I'll try to have a look as soon as possible.

comment:5 Changed 7 years ago by therve

  • Keywords review removed
  • Owner changed from therve to dripton

General remarks:

  • the format of docstrings are the following:
    """
    Docstring.
    """
    

It would be great if you could have a look at the coding standard:
http://twistedmatrix.com/trac/browser/trunk/doc/development/policy/coding-standard.xhtml?format=raw
It probably lack some stuff, don't hesitate to come back to us.

  • Every new method and class should get a docstring, and every new attribute should be documented in the class docstring. That's also true for test methods. You're almost there, but not quite.

Now for more specific comments.

In abstract.py:

  • You don't need to import inet_pton from compat: it monkeypatch socket itself at load
  • don't use assert in IPv6Address.__init__: either raise an meaningful exception or do nothing

In base.py:

  • _fake_gethostbyname is not really a great name. It doesn't have to private too: what about getHostByName ?

In tcp.py:

  • The different classes should be named X6 instead of XTCP6: no need to repeat TCP, we're already in the tcp module.
  • ClientTCP6.__init__ still has too much duplicated code with Client: on 18 lines only one is different
  • same thing for ConnectorTCP6.__init__

In test_tcp.py:

  • the tests need to be skipped when the underlying platform doesn't support ipv6. It can maybe be done by create a simple socket.socket(socket.AF_INET6 and see if it doesn't fail.
  • listenfn and connectfn aren't great names. Maybe listenMethod and connectMethod?
  • same thing for ccConnectfn
  • You've splitted getsockname call to do that:
                tup = serverSocket.getsockname()
                serverHost = tup[0]
                serverPort = tup[1]
    

I think you'd better do that instead:

            serverHost, serverPort = serverSocket.getsockname()[:2]

Once the skip when not supported is implemented, I'll create a branch and check with the buildbot how the various platforms react.

That's all for now: lots of details and policy problems, but overall I don't think we're very far from the end. Thanks a lot!

comment:6 Changed 7 years ago by dripton

  • Keywords review added

Here's a third version of the patch. All your suggestions should be incorporated. Thanks.

Changed 7 years ago by dripton

Version 3 of the ipv6 patch

Changed 7 years ago by dripton

Version 3 of the ipv6 patch

comment:7 Changed 7 years ago by therve

  • Owner changed from dripton to therve

comment:8 Changed 7 years ago by therve

  • author changed from dripton to therve
  • Branch set to branches/tcp-ipv6-3014

(In [22434]) Branching to 'tcp-ipv6-3014'

comment:9 Changed 7 years ago by therve

(In [22435]) Apply dripton's patch, with some small corrections.

Refs #3014

comment:10 Changed 7 years ago by exarkun

  • Keywords review removed

I'm not positive, but I don't think this is up for review.

comment:11 follow-up: Changed 7 years ago by therve

For the record, we have several problems to integrate this cleanly:

  • we can't replace gethostbyname by getaddrinfo, because it starts to return ipv6 addresses when ipv4 ones are expected
  • in general, the new API connectTCP6/listenTCP6 doesn't seem great.

For now, I'm tempted to keep only connectTCP, and detect if we pass ipv6 addresses to it. We could have a API to resolve a host name to an IPv6 address. Regarding listenTCP, I'm tempted to listen on both IPv4 and IPv6, except if prevented.

So we're not really discussing the validity of the current patch, it seems to work properly. But there are some questions on how to integrate that in twisted correctly.

I have just a little technical problem, the skip doesn't work under windows. I'll try to investigate later.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by dripton

  • Priority changed from highest to normal

Replying to therve:

For the record, we have several problems to integrate this cleanly:

  • we can't replace gethostbyname by getaddrinfo, because it starts to return ipv6 addresses when ipv4 ones are expected

I think you have to use getaddrinfo instead of gethostbyname if you want IPv6 support. However, you can pass AF_INET as the family arg if you only want IPv4 addresses.

  • in general, the new API connectTCP6/listenTCP6 doesn't seem great.

Agreed.

For now, I'm tempted to keep only connectTCP, and detect if we pass ipv6 addresses to it. We could have a API to resolve a host name to an IPv6 address. Regarding listenTCP, I'm tempted to listen on both IPv4 and IPv6, except if prevented.

Currently, connectTCP is documented to take host and port as separate arguments. host is documented to be a hostname, and port is documented to be a number. Passing an IPv4 address to host works (but possibly causes an unnecessary lookup), and passing a valid service name to port also works.

Ignoring backward compatibility, I think the right interface is for connectTCP to take a sockaddr tuple instead of separate host and port arguments. Scales better to IPv6 -- the tuple just has 2 more (optional) members. And this is consistent with socket.connect() and with connectTCP's own bindAddress arg.

Also, it should be documented that if the user passes in a valid IP address, the resolver will not be called. So if a user wants to do his own lookups, he can.

Of course backward compatibility can't be ignored. So maybe there should be a connectTCPEx, which is the fully flexible version, and connectTCP should keep its existing signature and just call connectTCPEx.

comment:13 in reply to: ↑ 12 ; follow-ups: Changed 7 years ago by glyph

Replying to dripton:

Ignoring backward compatibility, I think the right interface is for connectTCP to take a sockaddr tuple instead of separate host and port arguments. Scales better to IPv6 -- the tuple just has 2 more (optional) members. And this is consistent with socket.connect() and with connectTCP's own bindAddress arg.

Tuples result in hard-to-read code. What do the elements mean? Who makes the rules? A sockaddr object, perhaps - then the user has somewhere to go to read its documentation.

Also, it should be documented that if the user passes in a valid IP address, the resolver will not be called. So if a user wants to do his own lookups, he can.

Yay.

Of course backward compatibility can't be ignored. So maybe there should be a connectTCPEx, which is the fully flexible version, and connectTCP should keep its existing signature and just call connectTCPEx.

If you pass an IPv6 address to connectTCP, presumably you want to connect via IPv6; nobody's ever done that before, no compatibility requirements. If you pass a hostname, for compatibility, you want IPv4. So there's no reason to add a whole new reactor method, is there?

In my fantasy world, users would get IPv6 connectivity "for free", even in old Twisted programs. It seems like you can't put an IPv6 host on the same domain name as an IPv4 host anyway, so perhaps a compatible behavior for hostnames would be to *prefer* IPv4 hostnames but attempt IPv6 resolution if no IPv4 records are found. For example, if the user types "ipv6.chat.freenode.net" into the "hostname" field, why shouldn't that work? Their intention is not ambiguous, there is no IPv4 address for that hostname.

We had a discussion on #twisted that implied that this sort of automatic failover was undesirable, but I re-read the logs and still don't really understand it. Perhaps someone could attach a description to this ticket of some of the nasty scenarios that could take place if this were to happen.

(Also, everyone interested in this should be aware of endpoints, if they aren't already. It may be time to bite the bullet on that ticket.)

dripton, thanks for all the hard work. I realize this is kind of a murky area :).

comment:14 in reply to: ↑ 13 Changed 7 years ago by dripton

Replying to glyph:

Replying to dripton:

Ignoring backward compatibility, I think the right interface is for connectTCP to take a sockaddr tuple instead of separate host and port arguments. Scales better to IPv6 -- the tuple just has 2 more (optional) members. And this is consistent with socket.connect() and with connectTCP's own bindAddress arg.

Tuples result in hard-to-read code. What do the elements mean? Who makes the rules? A sockaddr object, perhaps - then the user has somewhere to go to read its documentation.

I agree, but in this particular case the Python socket module uses a tuple, so it's conforming to the standard way to express a sockaddr struct in Python. For that matter, connectTCP's bindAddress argument also uses a sockaddr tuple.

Of course backward compatibility can't be ignored. So maybe there should be a connectTCPEx, which is the fully flexible version, and connectTCP should keep its existing signature and just call connectTCPEx.

If you pass an IPv6 address to connectTCP, presumably you want to connect via IPv6; nobody's ever done that before, no compatibility requirements. If you pass a hostname, for compatibility, you want IPv4. So there's no reason to add a whole new reactor method, is there?

Yes. You might need to pass a flowinfo or a scopeid. connectTCP doesn't currently give you a way to do that. So we either need to add more optional args to connectTCP, which will confuse people using IPv4. Or we need to pass in a sockaddr, which is just (host, port) for IPv4 but has two more members in IPv6.

In my fantasy world, users would get IPv6 connectivity "for free", even in old Twisted programs. It seems like you can't put an IPv6 host on the same domain name as an IPv4 host anyway, so perhaps a compatible behavior for hostnames would be to *prefer* IPv4 hostnames but attempt IPv6 resolution if no IPv4 records are found. For example, if the user types "ipv6.chat.freenode.net" into the "hostname" field, why shouldn't that work? Their intention is not ambiguous, there is no IPv4 address for that hostname.

I think your assumption that you can't put IPv4 and IPv6 hosts in the same domain is overstated. Maybe "real" DNS servers don't currently do that, but mdns does, or at least Avahi does. If I do a getaddrinfo call for my Linux box with ".local" appended to the hostname, I get back both IPv4 and IPv6 answers in the list.

I agree that if the user just provides a hostname, it should Just Stinking Work, regardless of whether getaddrinfo returns an IPv4 or IPv6 address. (And if it returns both, we should probably just pick the first one returned rather than having a preference. If the user has a preference, he can make his own getaddrinfo call with the appropriate filtering.)

comment:15 in reply to: ↑ 13 Changed 6 years ago by dripton

Replying to glyph:

In my fantasy world, users would get IPv6 connectivity "for free", even in old Twisted programs. It seems like you can't put an IPv6 host on the same domain name as an IPv4 host anyway, so perhaps a compatible behavior for hostnames would be to *prefer* IPv4 hostnames but attempt IPv6 resolution if no IPv4 records are found. For example, if the user types "ipv6.chat.freenode.net" into the "hostname" field, why shouldn't that work? Their intention is not ambiguous, there is no IPv4 address for that hostname.

We had a discussion on #twisted that implied that this sort of automatic failover was undesirable, but I re-read the logs and still don't really understand it. Perhaps someone could attach a description to this ticket of some of the nasty scenarios that could take place if this were to happen.

I haven't seen the IRC logs, so maybe I'm missing something, but I think automatic failover is okay. As long as the user can override it, by passing in an IP address instead of a hostname.

While my gut says to prefer the first address returned by the DNS server (because maybe the DNS server is returning that one first for a good reason), I guess preferring IPv4 addresses over IPv6 addresses for maximum backward compatibility would be okay too. Again, if the user really cares, he can pass in an IP address.

It seems that everyone thinks having a separate connectTCP6 is ugly. I'd be fine with merging it into connectTCP, but this would mean adding optional scopeid and flowinfo args to connectTCP. (If they were omitted then you couldn't connect to link-local IPv6 addresses.)

Merging listenTCP6 into listenTCP makes sense to me.

I'll wait for feedback, then submit another patch.

Changed 6 years ago by exarkun

the irc conversation

comment:16 Changed 6 years ago by cassidy

  • Cc cassidy added

Any progress on this? Missing IPv6 support in Twisted is *really* annoying.

comment:17 Changed 6 years ago by exarkun

All the progress is in the branch. If you'd like IPv6 support in Twisted, you could help finish the branch.

comment:18 Changed 5 years ago by victorvieux

Hello,
could you add TCP6Server and TCP6Client to application.internet

--- twisted/application/internet.py     (revision 26995)
+++ twisted/application/internet.py     (working copy)
@@ -15,6 +15,7 @@
 They are as follows::
   TCPServer, TCPClient,
+  TCP6Server, TCP6Client,
   UNIXServer, UNIXClient,
   SSLServer, SSLClient,
   UDPServer, UDPClient,
@@ -58,11 +59,11 @@
     @type volatile: C{list}
     @ivar method: the type of method to call on the reactor, one of B{TCP},
-        B{UDP}, B{SSL} or B{UNIX}.
+        B{TCP6}, B{UDP}, B{SSL} or B{UNIX}.
     @type method: C{str}
     @ivar reactor: the current running reactor.
-    @type reactor: a provider of C{IReactorTCP}, C{IReactorUDP},
+    @type reactor: a provider of C{IReactorTCP}, C{IReactorTCP6}, C{IReactorUDP},
         C{IReactorSSL} or C{IReactorUnix}.
     @ivar _port: instance of port set when the service is started.
@@ -125,11 +126,11 @@
     @type volatile: C{list}
     @ivar method: the type of method to call on the reactor, one of B{TCP},
-        B{UDP}, B{SSL} or B{UNIX}.
+        B {TCP6}, B{UDP}, B{SSL} or B{UNIX}.
     @type method: C{str}
     @ivar reactor: the current running reactor.
-    @type reactor: a provider of C{IReactorTCP}, C{IReactorUDP},
+    @type reactor: a provider of C{IReactorTCP}, C{IReactorTCP6}, C{IReactorUDP},
         C{IReactorSSL} or C{IReactorUnix}.
     @ivar _connection: instance of connection set when the service is started.
@@ -194,7 +195,7 @@
 }
 import new
-for tran in 'Generic TCP UNIX SSL UDP UNIXDatagram Multicast'.split():
+for tran in 'Generic TCP TCP6 UNIX SSL UDP UNIXDatagram Multicast'.split():
     for side in 'Server Client'.split():
         if tran == "Multicast" and side == "Client":
             continue

comment:19 Changed 5 years ago by exarkun

It would be very helpful if you could also provide unit tests for these additions. Also, it's easier if patches are attached to the ticket as files, not pasted inline, since inline formatting usually destroys them (ie, makes them unapplyable).

comment:20 Changed 5 years ago by victorvieux

Hello,

Here is the patches attached and an unit test (testTCP6, like testTCP in TestInternet2)
OK with 'trial twisted'
I hope the unit test is good, i,m not very familiar with trial.

Changed 5 years ago by victorvieux

Fix TCP6Server and TCP6Client unittest

Changed 5 years ago by victorvieux

TCP6Server & TCP6Client

comment:21 Changed 5 years ago by thijs

  • Keywords review added
  • Owner therve deleted

Putting the patches up for review.

comment:22 Changed 5 years ago by exarkun

(In [27251]) Apply the ipv6 service patches

refs #3014

comment:23 Changed 5 years ago by exarkun

  • Author changed from therve to dripton
  • Cc exarkun added
  • Keywords review removed
  • Owner set to dripton

I applied the service stuff to the branch. It's not perfect, but it's a good start. However, several questions raised previously on this ticket about the code already in the branch have not yet been addressed. So, taking this out of review until someone addresses those points.

comment:24 Changed 5 years ago by thijs

  • Author changed from dripton to dripton, victorvieux
  • Owner changed from dripton to victorvieux

comment:25 Changed 5 years ago by dripton

In a Twisted meeting at PyCon, Itamar pointed out that we could work around the backward compatibility problem of DNS starting to return IPv6 addresses to legacy programs that only expected IPv4 addresses by requiring some kind of explicit resolver configuration to get IPv6 addresses. I think that's a reasonable way to proceed, and nobody in the room objected.

I think that breaks the design logjam, so I will resume working on this ticket soon.

comment:26 Changed 5 years ago by jknight

  • Cc jknight added

I think some of the concern over scope id is misplaced. In python, addresses are represented not as a struct sockaddr_*, but as a name and some extra info. Scope id is actually part of the IP address string, so Twisted can follow suit. That is:

>>> socket.getaddrinfo("jknight.local", "www")
[(30, 2, 17, '', ('fe80::219:e3ff:fe07:12f5%en1', 80, 0, 6)),
 (30, 1, 6, '', ('fe80::219:e3ff:fe07:12f5%en1', 80, 0, 6)),
 (2, 2, 17, '', ('10.0.1.101', 80)),
 (2, 1, 6, '', ('10.0.1.101', 80))]

So if we only store (and take as an API) "fe80::219:e3ff:fe07:12f5%en1" and 80, we can still connect to the right thing in connectTCP by doing:

>>> socket.getaddrinfo("fe80::219:e3ff:fe07:12f5%en1", 80, 0, socket.SOCK_STREAM)
[(30, 1, 6, '', ('fe80::219:e3ff:fe07:12f5%en1', 80, 0, 6))]

And from that, calling:

socket.socket(30, 1, 6)
socket.connect(('fe80::219:e3ff:fe07:12f5%en1', 80, 0, 6))

That seems like the right thing to do to me. Add an optional address family argument to connectTCP (defaulting to PF_UNSPEC aka 0), allowing the user to specify only use IPv4 or only use IPv6, and done.

comment:27 Changed 5 years ago by thijs

  • Cc thijs added
  • Priority changed from normal to high

comment:28 Changed 4 years ago by np

  • Cc nicolas.pouillard@… added

What are the missing pieces to have this feature merged. The lack of IPv6 support becomes more and more important these days!

comment:29 Changed 4 years ago by serverhorror

  • Cc martin@… added

comment:30 Changed 4 years ago by philmayers

Reading through this ticket I'm a bit confused by the various strands of the discussion. However it seems that:

  • most people thought a single connectTCP made sense
  • there was broad agreement that getaddrinfo made sense
  • there was some desire to get IPv6 "for free"

I have a patch that implements reactor.getaddrinfo, and refactors tcp.py to call this for resolution, loop over the resulting address families & addresses, attempt a connection to each in turn, using the first which succeeds.

This is the approach programs like OpenSSH and Internet Explorer use. It has the advantages that:

  • getaddrinfo is the "right" call to use, so Twisted apps will behave like other system apps (which on most systems means preferring IPv6 if IPv6 is enabled)
  • if the first address(es) aren't reachable it tries subsequent ones (of the same or different address families)
  • it is very simple to provide a "reactor.disableIPv6()" call - simply change the "0" you pass into getaddrinfo as the address family to AF_INET, and you're done.

If there is interest I can supply the patch; it is ~400 lines, and retains API compatibility. It does not provide server-side listenTCP code yet - there are choices to be made about using separate sockets / v4-mapped addresses there which are platform-dependent.

comment:31 Changed 4 years ago by philmayers

  • Cc p.mayers@… added

comment:32 follow-up: Changed 4 years ago by exarkun

I have a patch that implements reactor.getaddrinfo, and refactors tcp.py to call this for resolution, loop over the resulting address families & addresses, attempt a connection to each in turn, using the first which succeeds.

This sounds like a partial duplication of the work done for #4362.

most people thought a single connectTCP made sense

I think people wanted a single connectTCP to make sense. I don't see anything in the suggestion which suggests it actually does. Can you refresh my memory about how this can actually work though? I don't see any discussion of that on the ticket yet.

It seems like there should be a reactor method that actually accepts a getaddrinfo-like structure.

getaddrinfo is the "right" call to use

I think everyone agrees on this, at least. I've been waiting for #4362 before trying to make any more progress on IPv6. Sorry for not updating the ticket with that information.

if the first address(es) aren't reachable it tries subsequent ones

The default 30 second connection timeout doesn't allow for many retries. I think multiple address logic needs to be thought out a bit more (or at least whatever existing thoughts people have needs to be written down). Remember that the lowest level API should be able to express any operation people want to perform. Higher level APIs can be constructed on top of those for particular retry policies.

it is very simple to provide a "reactor.disableIPv6()"

I really dislike the idea of this API. If this is how we work around broken IPv6 configurations, then Twisted applications won't work in broken IPv6 configurations because every application will have to provide a way to let users indicate that they should call this method.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by philmayers

Replying to exarkun:

This sounds like a partial duplication of the work done for #4362.

Gah. I didn't see that. I'll take a look.

most people thought a single connectTCP made sense

I think people wanted a single connectTCP to make sense. I don't see anything in the suggestion which suggests it actually does. Can you refresh my memory about how this can actually work though? I don't see any discussion of that on the ticket yet.

Well, I'm not sure there's a non-normative definition of "makes sense".

The current situation is that connectTCP will accept hostnames. One view is that this is "wrong", and that a higher-level API should be used for hostnames, but whether or not you agree with that (I don't) that's not the case.

*If* an API takes hostnames and *if* the underlying framework supports more than one address family, I think it's pretty clear the API should resolve the name to the "best" address family without developer intervention. This is (I feel) particularly important for IPv6, because onerous work to enable it reduces uptake.

For that reason, I believe connectTCP should "do" IPv6.

Happily, if you pass the an IP into connectTCP (and then getaddrinfo) it will do the "right" thing and return a 1-element list with the (af,socktype...) tuple matching that IP. So it continues to do the right thing for people who *aren't* using hostnames.

It seems like there should be a reactor method that actually accepts a getaddrinfo-like structure.

Why? As opposed to an API that accepts a name?

Let's not even get into SRV or NAPTR lookups...

The default 30 second connection timeout doesn't allow for many retries.

Yes. It's a hard problem.

You can make a case that Twisted might try just the one IP for each address family, which might help for names with many responses (e.g. www.google.com).

Alternatively just try the 1st name returned, which is what the current (IPv4) code does. But if you've got broken IPv6, then it blows up.

Then of course there's the option of caching of which IPs are unreachable and filtering from subsequent getaddrinfo replies. But that starts to sound awfully like a higher-level API.

Remember that the lowest level API should be able to express any operation people want to perform. Higher level APIs can be constructed on top of those for particular retry policies.

Having connectTCP do the simplest possible thing directly contradicts with Twisted apps getting IPv6 "for free" as far as I can see. It's also not what connectTCP currently does ;o)

So, maybe there are two separate issues here:

  1. Cleaning up tcp.py to remove any address-family specific bits. Once this is done, passing in IPv4 or IPv6 numerical addresses should just work. It seems like this should be pretty uncontroversial.
  1. Decide whether to create a new name-based connect API, or change connectTCP behaviour

Personally I want connectTCP to turn names into IPv6 addresses and do the work for me. I realise however that might not be everyones view.

Could add a new parameter to connectTCP which indicates the name resolution behaviour?

reactor.connectTCP('www.domain.com', 80, connectStyle='[old|getaddrinfo|getaddrinfo-multi]')

Yuck

it is very simple to provide a "reactor.disableIPv6()"

I really dislike the idea of this API. If this is how we work around broken IPv6

I very much dislike it, but someone mentioned a requirement "up thread" IIRC. Personally, I'm of the opinion that fixing the broken connectivity is the correct solution. Really this should be handled at the OS-level, with decent probing through reachable IPv6 routers, and feedback from this altering the name lookup algorithm. But it is not going to happen.

It's not clear to me that Twisted can solve this problem. If you've got broken IPv6 connectivity, things are going to start getting flakey over the next few years as IPv6 switches on.

comment:34 Changed 4 years ago by philmayers

Huh. So it turns out a very small set of changes will suffice to let you listenTCP on an ipv6 socket (which will accept ipv4 clients on supporting systems) and make connections by numeric addresses to ipv6 hosts

$ svn diff|diffstat 
 abstract.py  |   12 +++++++++
 posixbase.py |    4 +--
 tcp.py       |   74 ++++++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 65 insertions(+), 25 deletions(-)

Much of that is changes relating to getPeer/getHost assuming that addresses are 2-tuples, and some very basic test code. I can supply the patch if there is any interest in such a 2-stage (transport first, name resolution later) approach.

A similar approach should work for UDP

comment:35 Changed 4 years ago by philmayers

In case it's of interest, the attached patch implements basic IPv6 support. It makes no changes to the name-lookup behaviour - DNS names will only ever result in an IPv4 connection.

Usage is as follows:

# makes an outbound IPv6 connection
reactor.connectTCP('2001:db8::1', 80)

# listens on an IPv6 socket; this will accept IPv4 connections as well if
# your OS supports it, in which case transport.getPeer/getHost will return
# addresses of the form ::ffff:192.168.1.1
reactor.listenTCP(80, factory, ipv6=True)

# listens on an IPv6 socket. This will receive both UDP4 and UDP6 datagrams
# if your OS supports it. datagramReceived will get addresses of the form
# ::ffff:192.168.1.1 if this is the case
reactor.listenUDP(53, proto, ipv6=True)

There are a few warts to clean up. Obviously the getPeer/getHost methods should not return IPv4Address objects. Probably easiest to replace these with calls to a factory function of the form:

def getPeer(self):
  return address.objfactory(self.addressFamily, self.socket.getpeername, '[TCP|UDP]')

There is some doubt in my mind over whether IPv4-mapped addresses i.e. ::ffff:192.168.1.1 should be converted into straight IPv4 addresses and then back-converted e.g. in UDP transport.write cases.

There are limited tests, but they cover the basics.

It might be worth considering whether some platforms (now or in the future) should default to "ipv6=True" on the basis of sys.platform/socket.has_ipv6. On linux, I have tested and this all works even if the machine has *no* IPv6 addresses.

Obviously resolver behaviour can be changed at a later date.

comment:36 in reply to: ↑ 33 Changed 4 years ago by glyph

Replying to philmayers:

Replying to exarkun:

I think people wanted a single connectTCP to make sense. I don't see anything in the suggestion which suggests it actually does. Can you refresh my memory about how this can actually work though? I don't see any discussion of that on the ticket yet.

Well, I'm not sure there's a non-normative definition of "makes sense".

The current situation is that connectTCP will accept hostnames. One view is that this is "wrong", and that a higher-level API should be used for hostnames, but whether or not you agree with that (I don't) that's not the case.

*If* an API takes hostnames and *if* the underlying framework supports more than one address family, I think it's pretty clear the API should resolve the name to the "best" address family without developer intervention. This is (I feel) particularly important for IPv6, because onerous work to enable it reduces uptake.

For that reason, I believe connectTCP should "do" IPv6.

Happily, if you pass the an IP into connectTCP (and then getaddrinfo) it will do the "right" thing and return a 1-element list with the (af,socktype...) tuple matching that IP. So it continues to do the right thing for people who *aren't* using hostnames.

This all sounds pretty reasonable to me. I would like it to be the case, since there's plenty of existing software that calls connectTCP.

I will try to lay out all of the problems with name resolution in one comment, since the discussion has been a bit scattered.

Working Twisted software on a working ipv4 network will suddenly stop working if we start resolving names to ipv6 addresses by default. Worse yet, if we do the "obvious" thing and just connecting to the first address we find (whether it's ipv6 or ipv4) there's no way for a user of the software to pass a string that fixes the application again, since the hostname by itself can't encode the information about whether to try to resolve it to ipv4 or ipv6. (Passing an address could be broken for other reasons, after all we have the DNS system for a reason.)

This is especially noticeable with '.local' mDNS hostnames, since machines might advertise link-local IPv6 addresses without knowing that link-local IPv6 routing is somehow broken. At least, that's my personal experience.

The patch you've proposed addresses this compatibility issue, and it is slightly unfortunate. Hopefully we can start using #4473 everywhere soon, and if not outright deprecate hostnames passed to connectTCP, at least provide a more featureful option.

It seems like there should be a reactor method that actually accepts a getaddrinfo-like structure.

Why? As opposed to an API that accepts a name?

I think "as opposed to an API that accepts a weirdly formatted string that actually represents a getaddrinfo-like structure".

The default 30 second connection timeout doesn't allow for many retries.

Yes. It's a hard problem.

You can make a case that Twisted might try just the one IP for each address family, which might help for names with many responses (e.g. www.google.com).

That still might lead to some really annoyingly slow connections that look broken.

Alternatively just try the 1st name returned, which is what the current (IPv4) code does. But if you've got broken IPv6, then it blows up.

Right.

Then of course there's the option of caching of which IPs are unreachable and filtering from subsequent getaddrinfo replies. But that starts to sound awfully like a higher-level API.

... and this is what applications should really be exposing to users (endpoints with these sorts of options) rather than calling connectTCP directly. Maybe you should file some enhancement tickets for those :).

it is very simple to provide a "reactor.disableIPv6()"

I really dislike the idea of this API. If this is how we work around broken IPv6

I very much dislike it, but someone mentioned a requirement "up thread" IIRC. Personally, I'm of the opinion that fixing the broken connectivity is the correct solution. Really this should be handled at the OS-level, with decent probing through reachable IPv6 routers, and feedback from this altering the name lookup algorithm. But it is not going to happen.

It's not clear to me that Twisted can solve this problem. If you've got broken IPv6 connectivity, things are going to start getting flakey over the next few years as IPv6 switches on.

Even if we had such an API, how would a user tell an application to pass it? This is ugly, but it would be slightly less ugly to have some mangling syntax like '$ipv4$somehost.example.com' that says "disable IPv6 for this connect attempt". Again... this starts to sound like a higher-level API, but it would enable us to give us application support "for free".

At this point I think the thing to do would be to open a separate ticket for IPv6-connectTCP-with-hostname support, since your current strategy is backwards compatible, if not necessarily ideal.

comment:37 follow-up: Changed 4 years ago by exarkun

At this point I think the thing to do would be to open a separate ticket for IPv6-connectTCP-with-hostname support, since your current strategy is backwards compatible, if not necessarily ideal.

I don't know if it's backwards compatible. What about applications expecting an actual IPv4 address to come back from getHost and getPeer?

comment:38 in reply to: ↑ 37 Changed 4 years ago by philmayers

Replying to exarkun:

I don't know if it's backwards compatible. What about applications expecting an actual IPv4 address to come back from getHost and getPeer?

The patch only makes IPv6 connections if you tell it to, either by supplying an IPv6 numerical address to connectTCP, or supplying ipv6=True to listenTCP or listenUDP. Therefore you have to make changes to your app for this to happen, so it's not a problem (is one view :o)

Having said that: supplying an IPv6 address to connectTCP currently fails with:

socket.gaierror: [Errno -9] Address family for hostname not supported

...so in fact there is a change in behaviour. It goes from not making the connection at all to making it.

Or are you referring to the IPv4Address abuse in the patch (which is clearly wrong)?

comment:39 follow-up: Changed 4 years ago by jknight

This is especially noticeable with '.local' mDNS hostnames, since machines might advertise link-local IPv6 addresses without knowing that link-local IPv6 routing is somehow broken. At least, that's my personal experience.

1) I have never seen that. How can you even break link-local routing?
2) If it is broken, everything else on your machine will have delays when connecting too: telnet, web browsers, ssh, etc. Twisted being similarly broken doesn't really seem like a problem worth worrying about.

Basically every piece of software on your machine now tries IPv6 connections first, other than Twisted. Making Twisted work the same way (and break in the face of host misconfiguration in the same way) cannot possibly be the wrong thing to do!

comment:40 Changed 4 years ago by philmayers

One thing worth noting is that getaddrinfo is pretty smart about DNS lookups in the face of IPv6 configs at least on glibc, because AI_ADDRCONF is the default option.

comment:41 in reply to: ↑ 39 Changed 4 years ago by philmayers

Basically every piece of software on your machine now tries IPv6 connections first, other than Twisted. Making Twisted work the same way (and break in the face of host misconfiguration in the same way) cannot possibly be the wrong thing to do!

That is certainly the case for client connections.

Servers are rather different, so again we're back to the issue of connectTCP/hostname resolution being separate from listenTCP and socket address types.

Ironically the latter seems easier to resolve since you can make it an opt-in. The former is an argument about behaviour change - make Twisted more consistent with wider OS behaviour versus retain older name resolution behaviour.

Given that many Twisted servers are also clients...

comment:42 follow-up: Changed 4 years ago by jknight

Regarding the actual patch:

  1. You don't appear to deal with scope id properly. See my comment from a few months ago on how this can be done with getaddrinfo to parse it out of the string. Passing AI_NUMERICHOST to it would enforce that it's only used for addresses.
  2. The optional ipv6 argument doesn't seem very useful; if you do listenTCP(80, foo, interface="::"), shouldn't that be the same thing as listenTCP(80, foo, ipv6=True)? And with listenTCP(80, foo, interface="::1"), it's just redundant, isn't it?
  3. Reusing IPv4Address for IPv6 addresses doesn't actually seem clearly wrong to me, other than its name. Could rename it IPAddress in the future, though.

comment:43 in reply to: ↑ 42 Changed 4 years ago by philmayers

Replying to jknight:

Regarding the actual patch:

  1. You don't appear to deal with scope id properly. See my comment from a few months ago on how this can be done with getaddrinfo to parse it out of the string. Passing AI_NUMERICHOST to it would enforce that it's only used for addresses.

Well spotted - will fix

  1. The optional ipv6 argument doesn't seem very useful; if you do listenTCP(80, foo, interface="::"), shouldn't that be the same thing as listenTCP(80, foo, ipv6=True)? And with listenTCP(80, foo, interface="::1"), it's just redundant, isn't it?

Excellent point

Changed 4 years ago by philmayers

Handles scope ID on connectTCP addresses; may need some more work server side

comment:44 Changed 4 years ago by philmayers

I think we'll need to process both the outbound address in connectTCP and the bindAddress.

Question: what should happen if a user does the following:

reactor.connectTCP('fe80::1%eth0', 22, bindAddress=('fe80::2', 2345))
# or
reactor.connectTCP('fe80::1', 22, bindAddress=('fe80::2%eth0', 2345))

...that is, only puts a scope in one of the connect or bind addresses? Should Twisted guess and set the scope in the other address? Fail? Pass the results unchanged to the socket API?

This stuff is also a nightmare to unit-test, since there are no scoped addresses valid on loopback :o(

comment:45 Changed 4 years ago by jknight

Pass the results unchanged to the socket API?

Yes, that.

comment:46 follow-up: Changed 4 years ago by dripton

  • Owner changed from victorvieux to dripton
  • Status changed from new to assigned

This feature just became high priority at my job again. I have been instructed to make it one of my top priorities over the next couple of weeks. I'm gonna go try to crash together my patch, victorvieux's patches, and philmayers' patch against the latest Twisted trunk, and see what happens.

comment:47 in reply to: ↑ 46 Changed 4 years ago by glyph

Replying to dripton:

This feature just became high priority at my job again. I have been instructed to make it one of my top priorities over the next couple of weeks. I'm gonna go try to crash together my patch, victorvieux's patches, and philmayers' patch against the latest Twisted trunk, and see what happens.

Glad to hear it.

Note that the interface for endpoints have now been defined, and "TCP4" endpoints have been implemented. If you could include an implementation of "TCP6" endpoints, that would be great. #4473 is getting close to done, too; adding some code to that branch, or getting that branch merged and then doing some work in a separate ticket, would be an easy way to turn a string into an unambiguous IPv6 connect attempt / listening port, with all the awful junk like scope IDs and whatnot.

comment:48 follow-up: Changed 4 years ago by exarkun

Glyph gets excited sometimes. ;) And it's true that having an IPv6 endpoint will be really nice. Please keep the resolution to this ticket limited to providing a reactor interface to IPv6 though. The IPv6 endpoint should be easily implemented afterwards for #4470.

comment:49 in reply to: ↑ 48 Changed 4 years ago by glyph

Replying to exarkun:

Glyph gets excited sometimes. ;) And it's true that having an IPv6 endpoint will be really nice.

I am excited ALL THE TIME.

Please keep the resolution to this ticket limited to providing a reactor interface to IPv6 though. The IPv6 endpoint should be easily implemented afterwards for #4470.

Ah, I was looking for that ticket. Thanks for the link.

comment:50 Changed 4 years ago by philmayers

Can someone clarify exactly what is being aimed for here? Specifically, are we aiming for separate connectTCP6/listenTCP6 calls, or common calls which can handle either (or both, via IPv4-mapped) address families? Both have been suggested and argued for/against in the ticket history I think.

I personally feel (quite strongly) the latter is the correct approach but I realise others don't. It has the advantage of giving the ability to handle UDP4/6 with one rather than two protocol instances. It has the complexity of how backwards compatibility would work, especially with name-based connect calls.

But either way, clarity over which we want would be handy!

comment:51 follow-up: Changed 4 years ago by dripton

I think everyone has agreed that we want IPv6-capable connectTCP and listenTCP, not separate connectTCP6 and listenTCP6. That was one of the things that kept my original patch from being merged. The other was the resolver issue, which is now #4362.

I think the right path is to finish #4362 first (always setting the option to only get IPv4 addresses), then come back and add IPv6 support to connectTCP and listenTCP (this ticket), and finally to add them for UDP (new ticket).

comment:52 in reply to: ↑ 51 Changed 4 years ago by glyph

Replying to dripton:

I think everyone has agreed that we want IPv6-capable connectTCP and listenTCP, not separate connectTCP6 and listenTCP6. That was one of the things that kept my original patch from being merged. The other was the resolver issue, which is now #4362.

I think the right path is to finish #4362 first (always setting the option to only get IPv4 addresses), then come back and add IPv6 support to connectTCP and listenTCP (this ticket), and finally to add them for UDP (new ticket).

This sounds like a good plan to me. Thanks for summing it up so we all know what's happening next.

comment:53 follow-ups: Changed 4 years ago by exarkun

then come back and add IPv6 support to connectTCP and listenTCP (this ticket)

How is this not going to break every Twisted application deployed in an IPv4-only environment?

comment:54 in reply to: ↑ 53 ; follow-up: Changed 4 years ago by jknight

Replying to exarkun:

How is this not going to break every Twisted application deployed in an IPv4-only environment?

Same way that "telnet" still works, hopefully.

comment:55 in reply to: ↑ 54 Changed 4 years ago by glyph

Replying to jknight:

Replying to exarkun:

How is this not going to break every Twisted application deployed in an IPv4-only environment?

Same way that "telnet" still works, hopefully.

That, and "... always setting the option to only get IPv4 addresses" was one of the parts of the plan that were suggested before supporting IPv6 in listenTCP/connectTCP. Also, why would an IPv4-only environment even have a DNS server that returns IPv6 addresses? I don't see how this could break any application in an IPv4-only environment. Can you state your concern more specifically?

comment:56 in reply to: ↑ 53 Changed 4 years ago by philmayers

Replying to exarkun:

How is this not going to break every Twisted application deployed in an IPv4-only environment?

Assuming the twisted API defaults to creating AF_INET sockets (and returning AF_INET family from any name->address lookup functions), nothing should change. The approach in the patch I sent does this for addresses; unless you supply a v6 address to connectTCP, or a v6 listenAddress to listenTCP, it'll do v4 only.

comment:57 follow-up: Changed 4 years ago by exarkun

Same way that "telnet" still works, hopefully.

Can you expand on this a little? I don't follow. netkit telnet has -4 and -6 command line options to tell it which kind of address to resolve. I'm not exactly sure what it does by default. Is this an example of the AI_ADDRCONFIG feature of getaddrinfo?

Also, why would an IPv4-only environment even have a DNS server that returns IPv6 addresses?

All environments have such DNS servers. I have no IPv6 connectivity, but I can resolve ipv6.chat.freenode.net just fine.

The approach in the patch I sent does this for addresses; unless you supply a v6 address to connectTCP, or a v6 listenAddress to listenTCP, it'll do v4 only.

So there's no proposal for an API that accepts hostnames and connects to IPv6 addresses?

comment:58 in reply to: ↑ 57 ; follow-up: Changed 4 years ago by jknight

Replying to exarkun:

Same way that "telnet" still works, hopefully.

Can you expand on this a little? I don't follow. netkit telnet has -4 and -6 command line options to tell it which kind of address to resolve. I'm not exactly sure what it does by default. Is this an example of the AI_ADDRCONFIG feature of getaddrinfo?

No, this is where you loop over all the results of getaddrinfo and try each one until one works. AI_ADDRCONFIG might sometimes spare you an attempt to connect via IPv6 when that can't possibly work, but should only ever be treated as an optimization. Another thing getaddrinfo will do is reorder things: In particular, on my boxes, ipv6 is *always* enabled, if only for link-local. But, since I don't have global IPv6 connectivity, it tries ipv6 addrs last instead of first.

So there's no proposal for an API that accepts hostnames and connects to IPv6 addresses?

This can come later, but as soon as twisted has an API which loops over every address in the results of a name lookup, like you're supposed to do, that API can also support IPv6 transparently. I'm not sure if that can be added to connectTCP or not.

If it cannot, then connectTCP should be eventually deprecated when used with a name, (vs an IP address)

comment:59 in reply to: ↑ 58 Changed 4 years ago by glyph

Replying to jknight:

Replying to exarkun:

Same way that "telnet" still works, hopefully.

Can you expand on this a little? I don't follow. netkit telnet has -4 and -6 command line options to tell it which kind of address to resolve. I'm not exactly sure what it does by default. Is this an example of the AI_ADDRCONFIG feature of getaddrinfo?

No, this is where you loop over all the results of getaddrinfo and try each one until one works. AI_ADDRCONFIG might sometimes spare you an attempt to connect via IPv6 when that can't possibly work, but should only ever be treated as an optimization. Another thing getaddrinfo will do is reorder things: In particular, on my boxes, ipv6 is *always* enabled, if only for link-local. But, since I don't have global IPv6 connectivity, it tries ipv6 addrs last instead of first.

What is 'it' that tries ipv6 addresses? Do you mean that getaddrinfo returns AF_INET addrinfos first?

So there's no proposal for an API that accepts hostnames and connects to IPv6 addresses?

This can come later

I think that we've all agreed that that is not part of this ticket.

, but as soon as twisted has an API which loops over every address in the results of a name lookup, like you're supposed to do, that API can also support IPv6 transparently. I'm not sure if that can be added to connectTCP or not.

If it cannot, then connectTCP should be eventually deprecated when used with a name, (vs an IP address)

connectTCP is on its way to deprecation as a whole anyway, because a consensus is emerging that clientConnectionFailed totally sucks. In its replacement, maybe we just shouldn't bother to support hostnames, which would be a nice encouragement to just use endpoints, which are The Future™. Further, if we did that, we could get rid of the reactor-level resolver and just have a separate "resolver" parameter to those endpoints which require it.

Based on my current level of understanding (which may still be incomplete), here's what I'd like to see happen, in roughly this order. First I'd like to deal with this ticket (i.e. connectTCP/listenTCP with an IPv6 IP address). Some of these others may already have been filed:

  • listenUDP/IUDPTransport.write (ugh, why isn't this IDatagramTransport) with IPv6 support
  • HostnameEndpoint which does multiple connection attempts over IPv4/IPv6, as James suggests is correct. An argument to this object's constructor should be a listener for failed connection attempts, and a listener for successful name resolution; we still want the return value for connect itself to be a Deferred that ultimately fires with a protocol (or fails with an exception that encapsulates all of the failed attempts), but it should be possible for applications to detect these attempts, as telnet does, and do something useful with the information, even if 'something useful' is just updating a progress bar.
  • SSLHostnameEndpoint, to do the same thing except with SSL negotiation. (And ideally, failed SSL negotiation should be treated the same as a failed connection attempt!)
  • #4473
  • HostnameEndpoint being the default (or something like it) for endpoints.clientFromString. I don't know how we can relay the extra fancy arguments for HostnameEndpoint to clientFromString; perhaps it can just emit structured log messages.
  • add newConnectTCP4 and newConnectTCP6 reactor APIs which do only one connect attempt, and don't do name resolution. This should return a Deferred. I don't think we need SSL here, because the new API should be flexible enough - maybe allowing you to provide a custom transport class? - to allow us to do the SSL-as-transport optimization without having to have a separate method for that. But that's open for discussion on this particular issue. These names are just placeholders, please don't ever put 'new' into an actual method name.
  • change the behavior of connectTCP to have the same behavior as HostnameEndpoint, so that older applications start inheriting this good behavior. I think that whether we want to do this or not depends on the relative rates of IPv6 adoption and endpoints API adoption :).
  • deprecate connectTCP and connectSSL
  • deprecate reactor-level resolver APIs

Does this make sense to everybody as a long-term IPv6 strategy?

comment:60 Changed 4 years ago by jknight

I like the suggestion to do this ticket [support IPv6 /literal addresses/ in connectTCP/listenTCP] first. It seems to be the most well-baked of the bunch, and doesn't really need to block on getting name resolution stuff done. Just Do It. :)

And I agree broadly with that set of steps. A couple of specific comments:

SSLHostnameEndpoint, to do the same thing except with SSL negotiation. (And ideally, failed SSL negotiation should be treated the same as a failed connection attempt!)

Not sure about trying the next host on failed ssl negotiation; I'd want to see some precedent for that before going there.

add newConnectTCP4 and newConnectTCP6 reactor APIs

Even with the disclaimer not to use the name, I must protest the use of "4" and "6" there: there's no need for separate methods. Just one "newConnectTCP" is good enough.

Probably there should be an overarching "Fully support IPv6" ticket which links to all the sub-tickets for implementing specific bits.

comment:61 follow-up: Changed 4 years ago by dripton

If newConnectTCPNotItsFinalName only allows IPv6 addresses, not IPv6 hostnames, then there's no way to smuggle in a network interface name along with the hostname like "foo.bar.com%eth0" So I think there has to be an explicit scopeid optional argument, to let this API can work with link-local addresses, which I consider critical. Is everyone okay with that?

comment:62 in reply to: ↑ 61 ; follow-up: Changed 4 years ago by philmayers

Replying to dripton:

So I think there has to be an explicit scopeid optional argument, to let this API can work with link-local addresses, which I consider critical. Is everyone okay with that?

That means Twisted apps will have to parse the scope from user inputted addresses. I'm not terribly keen on that.

Ideally we would run IPv6 addresses through a getaddrinfo() call passing AI_NUMERICHOST to parse the scopeID, and suppressing DNS lookups.

However, Firefox has had problems on some platforms (MacOS X versions, I think) where AI_NUMERICHOST "isn't".

comment:63 Changed 4 years ago by philmayers

See for example issues with getaddrinfo() here:

http://code.google.com/p/chromium/issues/detail?id=28943
https://bugzilla.mozilla.org/show_bug.cgi?id=404399

These make me sad :o(

Clearly getaddrinfo is the right call but if it's slow/unreliable...

comment:64 in reply to: ↑ 62 Changed 4 years ago by dripton

Replying to philmayers:

That means Twisted apps will have to parse the scope from user inputted addresses. I'm not terribly keen on that.

Ideally we would run IPv6 addresses through a getaddrinfo() call passing AI_NUMERICHOST to parse the scopeID, and suppressing DNS lookups.

This is why I said we should finish the resolver ticket #4362 first.

If getaddrinfo is slow or unreliable on some operating systems, IPv6 just won't work very well on them until they fix it. I don't see anything Twisted can do about that.

comment:65 follow-up: Changed 4 years ago by jknight

This is why I said we should finish the resolver ticket #4362 first.

I don't think that follows. Calling getaddrinfo to parse an IPv6 address literal with a scope id pretty much has nothing to do with a fully-featured resolver interface. Directly calling getaddrinfo as in the last patch is perfectly fine.

I would agree with not worrying about slowness in OSX's getaddrinfo -- that's something apple will probably want to fix, eventually. We should still use it.

comment:66 follow-up: Changed 4 years ago by exarkun

If newConnectTCPNotItsFinalName only allows IPv6 addresses, not IPv6 hostnames, then there's no way to smuggle in a network interface name along with the hostname like "foo.bar.com%eth0"

I think a new reactor method that supports either IPv4 or IPv6 can accept hostnames for either, so this shouldn't be an issue.

comment:67 in reply to: ↑ 66 ; follow-up: Changed 4 years ago by glyph

Replying to exarkun:

If newConnectTCPNotItsFinalName only allows IPv6 addresses, not IPv6 hostnames, then there's no way to smuggle in a network interface name along with the hostname like "foo.bar.com%eth0"

I think a new reactor method that supports either IPv4 or IPv6 can accept hostnames for either, so this shouldn't be an issue.

As I suggested in the plan above, I would like the new reactor method to not support hostnames, though, because a hostname should represent multiple connect attempts. I would like the reactor method to be low-level; the endpoint should know about retry policies and name resolution and all that. For example, it's usually a good idea to try each name resolution result in turn, but in some specialized latency-sensitive applications you would actually prefer to make all connection attempts simultaneously, wait for the first one to succeed, use that one, and then cancel all the rest. Having that kind of logic in the reactor makes it difficult to customize. Composition over inheritance, and all that.

What's wrong with allowing '2001:6b0:e:2018::172%eth0', though, even if we don't allow hostnames? Will running such an address through getaddrinfo(... AI_NUMERICHOST ...) ever result in more than one address, or something else surprising?

comment:68 in reply to: ↑ 65 Changed 4 years ago by glyph

Replying to jknight:

This is why I said we should finish the resolver ticket #4362 first.

I don't think that follows. Calling getaddrinfo to parse an IPv6 address literal with a scope id pretty much has nothing to do with a fully-featured resolver interface. Directly calling getaddrinfo as in the last patch is perfectly fine.

I would agree with not worrying about slowness in OSX's getaddrinfo -- that's something apple will probably want to fix, eventually. We should still use it.

This was fixed in Leopard. According to Twisted's buildbots, we only support Snow Leopard at this point anyway. The Mozilla bug you're citing is very old, and they reported the issue at the time (as they specifically mention on the report).

comment:69 in reply to: ↑ 67 Changed 4 years ago by philmayers

Replying to glyph:

What's wrong with allowing '2001:6b0:e:2018::172%eth0', though, even if we don't allow hostnames? Will running such an address through getaddrinfo(... AI_NUMERICHOST ...) ever result in more than one address, or something else surprising?

Only on buggy systems, and consensus seems to be they've either all gone away, or we don't care because they need fixing / won't be using IPv6 because they're old installs.

It sounds like there's a pretty good consensus on what the aim is then; bonus.

comment:70 Changed 4 years ago by exarkun

I would like the reactor method to be low-level

Why are we going to mash IPv4 and IPv6 together then? Or why are we doing it in a totally ad hoc way, instead of adding connectAddrInfo(struct addrinfo*)?

comment:71 Changed 4 years ago by jknight

It's getting hard to follow what comments are targeted at this ticket vs other future tickets. It'd be nice to retarget such comments against a über-ticket or an individual component ticket, and leave this one available for work on and comments on the actual work to be done for this ticket and patch attached here.

comment:72 Changed 4 years ago by exarkun

Yea, that's true. So, specifically, it would be great if someone would summarize the current consensus and open questions on, say, a wiki page. Then we can address the open questions (if there even are any) and move forward on this ticket (or file new finer-grained tickets as necessary).

comment:73 Changed 4 years ago by andyhhp

  • Cc andyhhp@… added

comment:74 Changed 4 years ago by thomasvs

  • Cc thomas@… added

comment:75 Changed 4 years ago by glyph

In an attempt to simplify a little bit, I have just filed #4859 for the do-the-right-thing-and-just-connect-to-this-host client stream endpoint, which I think is practically speaking what we all want users to interact with eventually. I will have the requisite fist-fight with exarkun and itamar this weekend - and maybe even jknight if he's around - and edit the description of this ticket assuming everybody does in fact agree.

Here's where I feel like the emerging consensus is headed. Roughly in order, the following things should happen.

  • There should be new reactor methods, listenTCP6, listenUDP6, connectTCP6, which takes IPv6 addresses and connects to them. This is an interface which very few applications should use. Not quite no applications, because there are lots of apps that use ReconnectingClientFactory and we should probably present an interface that is compatible with those. I think that these new interfaces are really the purpose of this ticket. Some good points have been made in favor of connectAddrInfo, but I'm not sure that will translate well to other address types. This is not necessarily the most convenient API, but that's okay, because the real goal here is that...
  • Immediately following this there should be an implementation of TCP6 client and server endpoints, and then...
  • there should be an implementation of "smart" endpoints, which will listen and/or connect on all addresses which are suitable given an interface or hostname specification (or lack thereof). The client one is #4859 but I haven't filed one for the server yet.

In the distant future, I think we should consider deprecating and removing the ability to pass hostnames to (connect/listen)(TCP/UDP)[6] and just present the smart endpoints as the only "right" way to do it.

If someone's willing to shoulder the work, we could also possibly add a connect/listenTCP4 and then have connect/listenTCP do the "smart" thing, but at this point I don't think it's a particularly good idea.

A lot of discussion on this ticket has centered around older applications getting IPv6 support "for free". However, that's the only possible thing one could shoehorn "for free" into connectTCP; and it's pretty tricky to begin with. There are lots of other benefits to having applications use endpoint string descriptions; many other networking, security, and topology issues that can be addressed beyond just IPv6 with that API.

comment:76 Changed 4 years ago by exarkun

See IPv6 for the latest state of the plan (further comments welcome, of course).

comment:77 follow-up: Changed 3 years ago by philmayers

Ok, I've just read this:

http://perlmonkey.blogspot.com/2011/03/monkeying-around-with-ipv6.html

...which inspired me to go and read the "new" new plan in the wiki page.

That has me confused - glyph's comment seems to explicitly reject IPv6 support in existing connectTCP and listenTCP APIs (a mistake, IMHO) but the wiki page documents just such support.

What's the bottom line here?

Obviously the patch I wrote adds v6 to the existing connect/listenTCP calls. If it's been decided that's the acceptable route, can we move to reviewing code? Someones, anyones code? ;o)

comment:78 in reply to: ↑ 77 Changed 3 years ago by glyph

Replying to philmayers:

What's the bottom line here?

Pay attention to the IPv6 wiki page. It's the most recently updated consensus. I was trying to summarize here with my comment, but then there was some offline discussion about what should be on the page, so I think it's right.

If it's not right, we'll fix it, but I'm not going to fix my comment :)

comment:79 Changed 3 years ago by exarkun

  • Description modified (diff)
  • Keywords ipv6 added
  • Summary changed from TCP over IPv6 (patch) to Support IPv6 at least as well as IPv4 is supported

Adjusting the ticket description to reflect recent consensus.

comment:80 Changed 3 years ago by jknight

  • Description modified (diff)

Replace list of ticket numbers with keyword search page link.

comment:81 Changed 3 years ago by exarkun

  • Description modified (diff)

comment:82 Changed 3 years ago by honglei

  • Cc jhonglei@… added

comment:83 Changed 3 years ago by honglei

I suggest add this Ticket in the Milestone, Twisted-12.0 or others. http://www.computerweekly.com/blogs/Bloor-on-IT-security/2011/08/what-did-ipv6-day-prove.html

comment:84 Changed 3 years ago by exarkun

Thanks for your interest here. However, adding the ticket to a milestone won't get the feature implemented any sooner. Only working on implementing the feature will do that. See the related tickets for places to help out, or consider sponsoring some development. Thanks.

comment:85 Changed 3 years ago by philmayers

  • Cc p.mayers@… removed

comment:86 Changed 3 years ago by chjurk

As far as I can see this isn't going further. At some point it is important to keep older programs working with a current Twisted version, but in my opinion it is highly recommended to get IPv6 support into Twisted as soon possible. Are there any updates for this? I'd like to collaborate to get this done.

comment:87 Changed 3 years ago by exarkun

As far as I can see this isn't going further.

The plan outlined on IPv6 has been partially executed. See comments on #5084 for the present state of the work.

At some point it is important to keep older programs working with a current Twisted version

I'm not sure how this statement relates to this ticket.

I'd like to collaborate to get this done.

#5085 is probably a logical next step. Thanks!

Changed 3 years ago by chjurk

IPv6 patch for Twisted 11.0.0

comment:88 Changed 3 years ago by chjurk

The patch above allows the use of IPv6 addresses for both listenTCP, connectTCP etc.
For legacy reasons, IPv4 addresses are preferred when resolving hostnames.

comment:89 Changed 3 years ago by exarkun

Thanks for your interest, but this patch doesn't follow the plan outlined on IPv6. At the very least, can you explain why we should do it this way instead of the way explained on that wiki page? Also, it took a long time to come up with the plan on that wiki page, because there are a lot of subtle considerations involved. If your patch doesn't address those considerations it's not as likely to be appled.

Also, the patch duplicates lots of the work done for #5084 and also conflicts with it as well. It also has no unit tests, which are absolutely required for all Twisted changes.

Thanks again. I hope you'll continue your efforts on this, it's much appreciated.

Note: See TracTickets for help on using tickets.