-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix AsyncResolver to match ThreadedResolver behavior #8270
Conversation
3db83a9
to
b20c18a
Compare
AsyncResolver was disabled by default because it did not implement all of functionality of ThreadedResolver because aiodns did not support getaddrinfo until aio-libs/aiodns#118 see #559
This is a followup to aio-libs#118 to add `getnameinfo` as well to support aio-libs/aiohttp#8270
This is a followup to aio-libs#118 to add `getnameinfo` as well to support aio-libs/aiohttp#8270
I've tested this on production and everything works as expected. I'll do another turn and address review comments after the aiodns release |
This is a followup to #118 to add `getnameinfo` as well to support aio-libs/aiohttp#8270
Functional testing looks good, but all the tests for the resolver are patching the old code path so lots to update |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8270 +/- ##
==========================================
+ Coverage 97.56% 97.58% +0.01%
==========================================
Files 107 107
Lines 32987 33062 +75
Branches 3853 3865 +12
==========================================
+ Hits 32184 32262 +78
+ Misses 588 586 -2
+ Partials 215 214 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270
This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270
Sure, I just think it's preferable to minimise requirement changes in order to get security/bug fixes. I'll let you two decide, I don't mind too much. |
The interface is flexible enough that anyone who needs to fix can copy the code until it's released so I'm fine with leaving it in the current state for 3.9. At least that's the plan for Home Assistant, and we will delete the temporary backport after 3.10 It looks like anyone using it already has a workaround in place of setting AF_INET instead of unspecified so they have been living with this issue for years anyways. That's not great for IPv6 adoption but they also have ThreadedResolver even if its performance is worse so there are viable workarounds here now. |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com>
I've been running this production for a few days now and everything is working well. Unless there are any additional review comments, or reason we need to wait, I'll merge this later today. |
Got distracted yesterday with the HA release so I didn't have time to get back to this. I'm going to push this to another production system to be sure everything is OK shortly before merging |
So far so good on both production systems. Will check it again after dinner before merging. |
All good Also good with IPv6 disabled in the kernel. I can't think of any other way to beat this up. So I'm happy with it now |
Backport to 3.10: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 012f986 on top of patchback/backports/3.10/012f9862d3fcb06d0b6c1abe394841c12ffe2b52/pr-8270 Backporting merged PR #8270 into master
🤖 @patchback |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com> (cherry picked from commit 012f986)
* Bump aiodns to 3.2.0 changelog: aio-libs/aiodns@v3.1.1...v3.2.0 * Switch to using the AsyncResolver with aiohttp This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270 * Switch to using the AsyncResolver with aiohttp This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270 * Switch to using the AsyncResolver with aiohttp This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270 * Switch to using the AsyncResolver with aiohttp This avoids creating executor jobs to do DNS resolution AsyncResolver was not usable before aio-libs/aiohttp#8270 because it did not fallback to fallback to A records and only returned AAAA records in most cases when IPv6 was available This is a backport of aio-libs/aiohttp#8270 * fixes * fix mocking in next_dns * fix unmocked calls in blink * more mocking fixes * more fixes * more fixes * Fix missing mocking in nextdns tests extracted from #114539 * extract from context
What do these changes do?
AsyncResolver was disabled by default because it did not implement all of functionality of ThreadedResolver because
aiodns
did not supportgetaddrinfo
until aio-libs/aiodns#118 which meant only IPv6 addresses were returned in most cases on systems that supported IPv6see #559
Are there changes in behavior for the user?
AsyncResolver
now behaves the same as the current defaultThreadedResolver
and will return both IPv6 and IPv4 addresses on systems that support it.Is it a substantial burden for the maintainers to support this?
no
Related issue number
Checklist
CONTRIBUTORS.txt
CHANGES/
foldername it
<issue_or_pr_num>.<type>.rst
(e.g.588.bugfix.rst
)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix
: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature
: A new behavior, public APIs. That sort of stuff..deprecation
: A declaration of future API removals and breakingchanges in behavior.
.breaking
: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc
: Notable updates to the documentation structure or buildprocess.
.packaging
: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib
: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc
: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.
needs