Fix domain names resolution #1488
Conversation
There was a problem hiding this comment.
Thanks. This looks like a good start. I've left some comments inline. Additionally, it would be good to have a test for the A and AAAA codepaths here.
I see that getHostByName does have test coverage now but whatever existing tests there are were passing before so they're not proving the use of A/AAAA.
I wonder if twisted.names.test.test_client.FakeResolver could just go away, removing the now-uncommon behavior of guaranteeing an A response to an ANY query.
Thanks again for your efforts so far.
| @@ -0,0 +1 @@ | |||
| Replace dns client ANY request with A and AAAA requests | |||
There was a problem hiding this comment.
This doesn't follow the style-guide for newsfragments: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles
Perhaps:
twisted.names.common.ResolverBase.getHostByName now uses A and AAAA queries instead of ANY queries.
There was a problem hiding this comment.
| Replace dns client ANY request with A and AAAA requests | |
| twisted.names.common.ResolverBase.getHostByName now makes specific queries for A and AAAA records instead of an ANY query. |
| return d | ||
|
|
||
| def _cbRecords(self, records, name, effort): | ||
| def _cbRecordsA(self, records, name, effort, timeout): |
There was a problem hiding this comment.
A docstring for this method would be nice.
| return d | ||
| return result | ||
|
|
||
| def _cbRecordsAAAA(self, records, name, effort): |
There was a problem hiding this comment.
A docstring for this method would be nice.
| (ans, auth, add) = records | ||
| result = extractRecord(self, dns.Name(name), ans + auth + add, effort) | ||
| if not result: | ||
| # if A lookup doesn't provide IP then lookup AAAA |
There was a problem hiding this comment.
I think it should be wise to lookup AAAA records regardless of A results, it makes IPv6 stand on even field as IPv4, as perceived DNS requests (A vs AAAA) would even out, and it could help further adoption of IPv6
There was a problem hiding this comment.
This seems like it might make a good follow-up.
There was a problem hiding this comment.
What do you mean by that? Make another PR which applies this suggestion?
There was a problem hiding this comment.
File a ticket that describes the problem or the improved desired behavior and then make a PR that resolves it, yes.
There was a problem hiding this comment.
I agree with @ShadowJonathan that it should make queries for both A and AAAA by default, probably in parallel. It might also be worth making this behavior configurable for hosts that are ipv4-only, That seems like a good thing to put into a separate ticket+PR unless @taroved feels the need to do it in this one.
There was a problem hiding this comment.
@wiml I don't know how to do it in parallel. I think that it good thing to put into a separate ticket+PR
I'm going to implement dns names resolution. The current fix replace dns ANY request with A and AAAA requests.
Current ANY request doesn't provide IP addresses for most domain names
tox -e lintto format my patch to meet the Twisted Coding Standardreviewto the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.