Opened 7 years ago

Last modified 4 days ago

#4362 enhancement new

Introduce a new resolver interface (for use by the reactor) which supports multiple addresses and multiple address families

Reported by: exarkun Owned by: glyph
Priority: normal Milestone:
Component: core Keywords: ipv6
Cc: jknight, twm, thijs Branch: getaddrinfo-4362-4
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, glyph, rwall

Description

The current resolver interface, IResolverSimple.getHostByName, is modeled on gethostbyname(3) and inherits the limitations of that API:

  • Only one address can be returned
  • The desired address family cannot be specified

IResolverSimple tries to account for the latter by indicating that IPv6 addresses will be returned in preference to IPv4 addresses, but this leads to bugs such as #4212, since the reactor can't actually do anything with an IPv6 address.

getaddrinfo(3) was introduced to address the shortcomings of gethostbyname(3). It allows multiple addresses to be returned, and includes information about the address family of each (although this may be redundant with the address itself, since an ipv4 literal and an ipv6 literal can be distinguished from each other anyway).

It also mixes in getservbyname(3) functionality, accepting a service name and including a port number as an attribute of each element in the result.

We should introduce an interface like getaddrinfo(3) and switch the reactor to prefer it. We can register an adapter from the old interface to continue to support users of it. The new interface will eventually provide better IPv6 functionality, though, and if used will avoid issues like #4212.

Here's a proposal for the actual interface:

class INameResolver(Interface):
    def getAddressInformation(name, service, family=None, socktype=None, proto=None, flags=None):
        """
        Look up the address information associated with the given name.

        @param name: A hostname to resolve.
        @type name: C{str}

        @param service: A port number or the name of the service for which to find address
            information.  For example, C{22} or C{"ssh"}.
        @type service: C{int} or C{str}

        @param family: If specified, limit results to addresses from this family.  Must be one
            of the address family constants from the socket module.  For example,
            L{socket.AF_INET}.

        @param socktype: If specified, limit results to addresses for this socket type.  Must be
            one of the socket type constants from the socket module.  For example,
            L{socket.SOCK_STREAM}.

        @param proto: If specified, limit results to addresses for this socket protocol.  Must
            be one of the protocol constants from the socket module.  For example,
            L{socket.IPPROTO_TCP}.

        @param flags: A bitvector specifying zero or more of the following::

            - Yea right.  Go read the `getaddrinfo(3)` man page.

        @raise ValueError: If one of the specified flags is not supported by the
            implementation.  All flags are optional.

        @return: A L{Deferred} which will fire when the resolution completes.  If resolution is
            successful, the result will be a list of objects with the following attributes:

            - C{family}: the family of this address
            - C{type}: the type of this address
            - C{protocol}: the protocol of this address
            - C{canonicalName}: the canonical name associated with this address, or an empty
                                string.
            - C{address}: The actual address itself, including a port number.  This is suitable
                          to be passed directly to L{socket.socket.connect}.
        """

Attachments (1)

getaddrinfo-4362-3-1.patch (14.4 KB) - added by rwall 4 years ago.
Merged forward and resolved conflicts with trunk - contains broken tests

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/getaddrinfo-4362

(In [28686]) Branching to 'getaddrinfo-4362'

comment:2 Changed 7 years ago by exarkun

Got a reasonable start on this. Some things I know are left to do:

  1. Finish testing all the cases of _ResolverComplexifier
  2. Add something like ThreadedResolver for INameResolver
  3. Make reactor.resolver be an IResolverSimple again
  4. Add an adapter from INameResolver to IResolverSimple to preserve backwards compatibility of reactor.resolver
  5. Think about a new reactor name resolution interface (and think about not providing one).
  6. Deprecate IReactorCore.resolve
  7. Deprecate reactor.resolver

comment:3 Changed 6 years ago by <automation>

  • Owner exarkun deleted

comment:4 Changed 6 years ago by exarkun

#4912 was a duplicate of this.

comment:5 Changed 5 years ago by jknight

  • Keywords ipv6 added

comment:6 Changed 4 years ago by glyph

I think a Deferred is the wrong type for this new API to return.

One of the deficiencies of getaddrinfo is that it blocks. Obviously a Deferred resolves that. But another deficiency of getaddrinfo is that it doesn't complete (blocking or not) until all of its results are ready; certain types of requests (for example: mdns) may take significantly longer than others, and it would be great to get a successful result quickly if that is possible. Platform-specific interfaces such as getaddrinfo_a and CFHostStartInfoResolution can invoke callbacks for each name-resolution attempt, whether it succeeded or failed.

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

I disagree. I don't think it makes sense or is useful to return partial results for one name look up. Getaddrinfo_a doesn't. It takes a *list* of hostnames to resolve. This API doesn't and shouldn't do that, so a Deferred is sufficient.

comment:8 in reply to: ↑ 7 Changed 4 years ago by glyph

Replying to jknight:

I disagree. I don't think it makes sense or is useful to return partial results for one name look up. Getaddrinfo_a doesn't. It takes a *list* of hostnames to resolve. This API doesn't and shouldn't do that, so a Deferred is sufficient.

OK. Thanks for pointing out that detail; looking at the definition of struct gaicb now it is obvious, of course. I guess a Deferred that fires a list is the right structure after all.

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

Asynchronous stuff is hard in C. It's easier in Twisted, I hope. "glibc can't do incremental asynchronous resolution" sounds like a bad justification for "Twisted shouldn't do incremental asynchronous resolution".

Maybe there's another reason, though?

Changed 4 years ago by rwall

Merged forward and resolved conflicts with trunk - contains broken tests

comment:10 Changed 4 years ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

comment:11 Changed 4 years ago by rwall

  • Author changed from exarkun to exarkun, rwall
  • Branch changed from branches/getaddrinfo-4362 to branches/getaddrinfo-4362-2

(In [37217]) Branching to 'getaddrinfo-4362-2'

comment:12 Changed 4 years ago by rwall

(In [37218]) merge forward by applying getaddrinfo-4362-3-1.patch. refs #4362

comment:14 Changed 4 years ago by rwall

(In [37251]) Made test_failure pass. Instead of passing an explicit failure to FakeResolver, just request an unknown name.

Also some smaller changes...

  • Various small docstring improvements
  • Fix a twistedchecker comment init caps warning.
  • Add doc strings for FakeResolver
  • Fix long line
  • Add docstrings for MemoryNameResolver
  • More complete docstrings for _ResolverComplexifier
  • Add docstrings for test_base.py
  • Add docstrings for AddressInformation
  • Added a placeholder ThreadedNameResolver to make the tests happier.

refs #4362

comment:15 follow-ups: Changed 4 years ago by rwall

  • Owner rwall deleted
  • Status changed from assigned to new

Hi exarkun,

I've done a bit more work on this ticket but now I need some guidance.

First here's what I've done on top of you original branch (in log:branches/getaddrinfo-4362-2)

  1. Merged forward resolving some conflicts
  2. Added ThreadedNameResolver + tests
  3. Added a timeout parameter to INameResolver.getAddressInformation
  4. Registered an IResolverSimple to INameResolver adapter.

I think that covers points 1 and 2 in your comment:2 (above)

I have some questions about your other points.

Make reactor.resolver be an IResolverSimple again

Is this for backwards compatibility? So instead of adapting the resolver in reactor.installResolver... we would do the adaption inside reactor.resolve everytime it's called. or not at all.

Add an adapter from INameResolver to IResolverSimple to preserve

backwards compatibility of reactor.resolver

So maybe you meant

def installResolver(self, resolver):
    self._resolver = INameResolver(resolver)

reactor.resolver then becomes a descriptor which issues a deprecation warning and returns IResolverSimple(reactor._resolver)

Think about a new reactor name resolution interface (and think about

not providing one).

So reactor.resolve must continue to return an IP address (as per IReactorCore)

In which case why bother changing any of the reactor code in this branch.

Why can't this branch simply be about adding INameResolver, ThreadedNameReactor, and optionally the deprecations below...

Then decide on the reactor interface based on the usage of getaddrinfo in #4859.

  • Deprecate IReactorCore.resolve
  • Deprecate reactor.resolver

At which point I think I understand why you paused work on this branch in the first place :)

Anyway. It's been interesting to learn a little more about the gai etc.

If you have any time to comment I'd be interested hear what you think.

-RichardW.

comment:16 Changed 4 years ago by twm

  • Cc twm added

comment:17 in reply to: ↑ 15 Changed 3 years ago by thijs

  • Cc thijs added
  • Keywords review added
  • Owner set to exarkun

Replying to rwall:

Hi exarkun,

I've done a bit more work on this ticket but now I need some guidance.

I have some questions about your other points. If you have any time to comment I'd be interested hear what you think.

-RichardW.

Assigning it to exarkun for rwall's questions.

comment:18 in reply to: ↑ 15 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner changed from exarkun to rwall

Replying to rwall:

Hi exarkun,

I'll be filling in for exarkun today.

I've done a bit more work on this ticket but now I need some guidance.

Thanks!

First here's what I've done on top of you original branch (in log:branches/getaddrinfo-4362-2)

  1. Merged forward resolving some conflicts
  2. Added ThreadedNameResolver + tests
  3. Added a timeout parameter to INameResolver.getAddressInformation
  4. Registered an IResolverSimple to INameResolver adapter.

Great.

I think that covers points 1 and 2 in your comment:2 (above)

I have some questions about your other points.

Make reactor.resolver be an IResolverSimple again

Is this for backwards compatibility?

Yes. The backwards compatibility contract here is that the resolver attribute is an IResolverSimple.

So instead of adapting the resolver in reactor.installResolver... we would do the adaption inside reactor.resolve everytime it's called. or not at all.

Well... you don't need to do that. Just because resolver is a public attribute does not mean you need to consult it each time you do a resolution. You can do the adaptation in installResolver as long as the new attribute you're storing is under a different name. You may even want to make the new attribute public, if there are reasons that callers would want to access it directly.

Add an adapter from INameResolver to IResolverSimple to preserve

backwards compatibility of reactor.resolver

So maybe you meant ... reactor.resolver then becomes a descriptor which issues a deprecation warning and returns IResolverSimple(reactor._resolver)

Right, exactly.

Think about a new reactor name resolution interface (and think about

not providing one).

So reactor.resolve must continue to return an IP address (as per IReactorCore)

Yup, them's the breaks. It has a contract, it can't change the contract.

In which case why bother changing any of the reactor code in this branch.

Why can't this branch simply be about adding INameResolver, ThreadedNameReactor, and optionally the deprecations below...

I'm definitely in favor of smaller branches.

Then decide on the reactor interface based on the usage of getaddrinfo in #4859.

  • Deprecate IReactorCore.resolve
  • Deprecate reactor.resolver

At which point I think I understand why you paused work on this branch in the first place :)

Anyway. It's been interesting to learn a little more about the gai etc.

If you have any time to comment I'd be interested hear what you think.

Did that answer most of your questions about this?

comment:19 Changed 11 months ago by glyph

Today I discovered an interesting misfeature: since we don't have this reactor interface and HostnameEndpoint directly spins off getaddrinfo into a thread using deferToThread, HostnameEndpoint becomes somewhat test-hostile. Which, because pyOpenSSL is also test-hostile, #5642 is hard to implement nice expressive tests for. So I am going to bump the priority and do some work on this.

comment:20 Changed 7 months ago by glyph

We managed to test #5642, but the absence of this interface makes every test of HostnameEndpoint overly fragile and potentially buggy; there's a private makeHostnameEndpointSynchronous helper which is not available to applications.

comment:21 Changed 7 months ago by glyph

  • Owner changed from rwall to glyph

This is now effectively blocking #6712 so I will try to pick it up this week.

comment:22 Changed 7 months ago by glyph

  • Author changed from exarkun, rwall to exarkun, glyph, rwall
  • Branch changed from branches/getaddrinfo-4362-2 to branches/getaddrinfo-4362-3

(In [46789]) Branching to getaddrinfo-4362-3.

comment:23 Changed 6 weeks ago by glyph

  • Branch changed from branches/getaddrinfo-4362-3 to getaddrinfo-4362-4

We need a github watcher that pushes this 'branch' field update :-\

comment:24 in reply to: ↑ 9 Changed 6 days ago by glyph

Replying to exarkun:

Asynchronous stuff is hard in C. It's easier in Twisted, I hope. "glibc can't do incremental asynchronous resolution" sounds like a bad justification for "Twisted shouldn't do incremental asynchronous resolution".

Maybe there's another reason, though?

Looks like this question never got answered, and since I'm in the process of retooling the interface, it should probably be addressed. And in the intervening 4 years, maybe my opinion has changed a little.

Happy Eyeballs repeatedly references the "host's address preference policy", which can apparently be configured via DHCPv6, as per this citation: https://tools.ietf.org/html/rfc6555#ref-ADDR-SELECT. So with getaddrinfo and the like (all the wacky platform-specific variants seem the same, as James noted with getaddrinfo_a and I've been able to discover on Windows, e.g. https://msdn.microsoft.com/en-us/library/windows/desktop/ms738518(v=vs.85).aspx), all the hostname resolutions are going to come back in one big bundle, because they need to be sorted after the last one arrives.

However.

While the spec says that getaddrinfo should behave that way, that does not mean that the baseline interface for doing hostname resolution must behave this way. In a high-scale web spider, for example, the "host address preference policy" could be considered to be encapsulated in the application's twisted.names configuration, and that policy could be "resolve everything as fast as possible".

This might even be useful when calling getaddrinfo itself. It's definitely not unheard of for some networks to break horribly when you try to do IPv6 resolution but behave sensibly for IPv4. We could cheat the specs a little bit and kick off a parallel v4-only resolution if the v6 resolution is taking an appreciable amount of time. However, it would be impossible to relay all the addresses back to the application promptly if we were restrited by a getaddrinfo style all-at-once return API.

So, after much consideration I am thinking we do want to have an interface which gives back name resolutions incrementally, to make that possible in the cases where it is possible and we care.

comment:25 Changed 6 days ago by glyph

I updated the branch here - https://github.com/twisted/twisted/pull/547 - and I was planning to try to put it back into review today, but after spending hours re-understanding the problem domain here I think i'm going to start this branch over from scratch with a streaming interface, one which presents something in terms of an IProtocol-style object that receives resolutionStarted, addressReceived, and resolutionComplete events; the addresses in question should be IPv4Address and IPv6Address objects. HostnameEndpoint will have to change to manage a little state machine that cancels an outstanding name resolution if one is in progress when it successfully connects.

comment:26 Changed 4 days ago by glyph

New PR up, definitely not ready for review, here: https://github.com/twisted/twisted/pull/548

comment:27 Changed 4 days ago by glyph

A couple of notes on the new proposed interface in that branch:

  1. it doesn't expose port-number resolution. this is a dumb thing. nobody types service names. if they did, the wording of various specs constrain the platform's behavior to never doing anything but looking in /etc/services. Apparently honoring getaddrinfo can even get you in trouble here, since some OSes (iOS 9, maybe others) sometimes have bugs where they forget to fill out the 'port' field of the address. the only utility of the port argument to getaddrinfo is to pre-populate a data structure, not to do any meaningful resolution. so: pass an integer, just have it populate, skip the dependence on whatever random data happens to be in one machine's /etc/services file.
  2. it doesn't expose the canonical name anywhere. this seems like losing potentially useful information, except, that information can only really be used for one purpose: to screw up which TLS name to actually validate against by trusting local DNS. Don't do it!
  3. it carefully avoids talking about AF_INET, SOCK_STREAM, PF_WHATEVER and IPPROTO_NONSENSE. The type language we've invented in Twisted for address families isn't the absolute best, and hopefully eventually we can overhaul it, but it's already there, and it's definitely worse to make users have to understand two crappy mini-langauges about transport protocols.

I did add scope ID and flow info to IPv6Address, though, because those are kinda important for some edge-cases of IPv6 connectivity (particularly scope ID).

Note: See TracTickets for help on using tickets.