Fix Link-Local IPv6 Flags in the Resolver by lee-dot-n · Pull Request #9032 · aio-libs/aiohttp

lee-dot-n · 2024-09-05T21:27:22Z

Based on commit c48f2d1, the original use of the NI_NUMERICSERV and NI_NUMERICHOST flags was correct for the getnameinfo() call. As of 38dd9b8 the incorrect flags are now being used. Changed AI_* flags back to NI_* flags. Changed corresponding test.

Related issue number

Fixes #9028

Resolver class. Based on commit aio-libs@c48f2d1, the original use of the NI_NUMERICSERV and NI_NUMERICHOST flags was correct for the getnameinfo() call. As of aio-libs@38dd9b8 the incorrect flags are now being used. Changed AI_* flags back to NI_* flags. Changed corresponding test. Change-Id: I8b29ce2f39ea1cc097f4165b8ec55230f3b8eb6f

for more information, see https://pre-commit.ci

codecov · 2024-09-05T21:38:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.30%. Comparing base (e8d8e28) to head (e8a9150).
Report is 1065 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9032   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files         107      107           
  Lines       34355    34362    +7     
  Branches     4066     4066           
=======================================
+ Hits        33771    33778    +7     
  Misses        412      412           
  Partials      172      172

Flag	Coverage Δ
CI-GHA	`98.19% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`97.85% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.26% <88.88%> (+<0.01%)`	⬆️
OS-macOS	`97.53% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.63% <100.00%> (+<0.01%)`	⬆️
Py-3.10.14	`97.56% <100.00%> (+<0.01%)`	⬆️
Py-3.11.9	`97.79% <100.00%> (+<0.01%)`	⬆️
Py-3.12.5	`97.91% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`97.52% <100.00%> (+<0.01%)`	⬆️
Py-3.9.19	`97.46% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`97.07% <100.00%> (+<0.01%)`	⬆️
VM-macos	`97.53% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`97.85% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.26% <88.88%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dreamsorcerer

@bdraco is probably more familiar with this than me, but I think this looks right.

I'm also wondering if we can have a test that actually produces the exception in the original bug report?

bdraco

Looks like a good fix to me. 👍

Good catch

bdraco · 2024-09-05T23:38:17Z

Would be good to get a test for this case as well

lee-dot-n · 2024-09-06T15:32:06Z

Would be good to get a test for this case as well

~~The test that I changed in my commit seems to cover this case: ed9c04b#diff-4cbff640f1fc708fb7f2d3e79ac97f24cfe22063bad7ce6f67ab97b5b5171a0bL140~~
Sorry, just realized that test is for the AsyncResolver, not the ThreadedResolver. I will add to an existing test to check that the correct flags are called.

Would it be best to add a test that has a mock side effect that causes the exception?

bdraco · 2024-09-06T15:55:07Z

Would be good to get a test for this case as well

~~The test that I changed in my commit seems to cover this case: ed9c04b#diff-4cbff640f1fc708fb7f2d3e79ac97f24cfe22063bad7ce6f67ab97b5b5171a0bL140~~ Sorry, just realized that test is for the AsyncResolver, not the ThreadedResolver. I will add to an existing test to check that the correct flags are called.

Would it be best to add a test that has a mock side effect that causes the exception?

I would try to keep the mocking an minimal as possible but avoid doing any network I/O

case. Fixed various formatting errors in changelog and remove unused variables from test file. Change-Id: I47e9141ed0e819a6e9bac52abd48ebbb9adb8da1

Change-Id: I00525e397a11bd5c6e4985d8672b8f63188a6f90

lee-dot-n · 2024-09-06T19:06:42Z

Put up another commit with an alteration of an existing test and some various formatting errors fixed. I did not add a test for the behavior described in the bug report as that requires network I/O and/or testing the socket library, which I'm not sure makes sense here.

bdraco · 2024-09-06T19:30:31Z

Thanks @GitNMLee

patchback · 2024-09-06T19:48:13Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c693a81 on top of patchback/backports/3.10/c693a816ce84d46c445841c707f23e31f174cf28/pr-9032

Backporting merged PR #9032 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/c693a816ce84d46c445841c707f23e31f174cf28/pr-9032 upstream/3.10

Now, cherry-pick PR Fix Link-Local IPv6 Flags in the Resolver #9032 contents into that branch:
```
$ git cherry-pick -x c693a816ce84d46c445841c707f23e31f174cf28
```
If it'll yell at you with something like fatal: Commit c693a816ce84d46c445841c707f23e31f174cf28 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x c693a816ce84d46c445841c707f23e31f174cf28
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix Link-Local IPv6 Flags in the Resolver #9032 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/c693a816ce84d46c445841c707f23e31f174cf28/pr-9032

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2024-09-06T19:48:19Z

Backport to 3.11: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.11/c693a816ce84d46c445841c707f23e31f174cf28/pr-9032

Backported as #9047

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Co-authored-by: Nathan Lee <nathan.lee@garmin.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <git@sambull.org> Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit c693a81)

Co-authored-by: Nathan Lee <nathan.lee@garmin.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <git@sambull.org> Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit c693a81) Change-Id: I19977c5d84a9236bdddcaf025040ca68e18e77e4

…esolver (#9047) Co-authored-by: GitNMLee <89409038+GitNMLee@users.noreply.github.com> Fixes #9028 Fixes #123'). -->

…esolver (#9048) Co-authored-by: Nathan Lee <nathan.lee@garmin.com> Co-authored-by: pre-commit-ci[bot] Co-authored-by: Sam Bull <git@sambull.org> Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: GitNMLee <89409038+GitNMLee@users.noreply.github.com>

lee-dot-n requested review from asvetlov and webknjaz as code owners September 5, 2024 21:27

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 5, 2024

pre-commit-ci bot and others added 2 commits September 5, 2024 21:27

[pre-commit.ci] auto fixes from pre-commit.com hooks

5c57a42

for more information, see https://pre-commit.ci

Rename 9031.bugfix.rst to 9032.bugfix.rst

2cc6267

Dreamsorcerer reviewed Sep 5, 2024

View reviewed changes

Dreamsorcerer added backport-3.10 labels Sep 5, 2024

Update 9032.bugfix.rst

94728cd

bdraco approved these changes Sep 5, 2024

View reviewed changes

Nathan Lee and others added 3 commits September 6, 2024 14:00

Edit existing test to cover corresponding ThreadedResolver use

5c1b41a

case. Fixed various formatting errors in changelog and remove unused variables from test file. Change-Id: I47e9141ed0e819a6e9bac52abd48ebbb9adb8da1

Merge branch 'master' of https://github.com/GitNMLee/aiohttp

8cc41a6

Change-Id: I00525e397a11bd5c6e4985d8672b8f63188a6f90

Update test_resolver.py

0e73b52

bdraco approved these changes Sep 6, 2024

View reviewed changes

Merge branch 'master' into master

e8a9150

bdraco enabled auto-merge (squash) September 6, 2024 19:30

bdraco merged commit c693a81 into aio-libs:master Sep 6, 2024

patchback bot mentioned this pull request Sep 6, 2024

[PR #9032/c693a816 backport][3.11] Fix Link-Local IPv6 Flags in the Resolver #9047

Merged

bdraco pushed a commit that referenced this pull request Sep 6, 2024

[PR #9032/c693a816 backport][3.11] Fix Link-Local IPv6 Flags in the R…

4e1070e

…esolver (#9047) Co-authored-by: GitNMLee <89409038+GitNMLee@users.noreply.github.com> Fixes #9028 Fixes #123'). -->

bdraco mentioned this pull request Jan 9, 2025

Requests with IPv6 Link-Local address and Zone Identifier fail #10314

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Link-Local IPv6 Flags in the Resolver#9032

Fix Link-Local IPv6 Flags in the Resolver#9032
bdraco merged 8 commits intoaio-libs:masterfrom
lee-dot-n:master

lee-dot-n commented Sep 5, 2024 •

edited

Loading

Uh oh!

codecov bot commented Sep 5, 2024 •

edited

Loading

Uh oh!

Dreamsorcerer left a comment

Uh oh!

bdraco left a comment

Uh oh!

bdraco commented Sep 5, 2024

Uh oh!

lee-dot-n commented Sep 6, 2024 •

edited

Loading

Uh oh!

bdraco commented Sep 6, 2024

Uh oh!

lee-dot-n commented Sep 6, 2024

Uh oh!

bdraco commented Sep 6, 2024

Uh oh!

patchback bot commented Sep 6, 2024 •

edited

Loading

Uh oh!

patchback bot commented Sep 6, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lee-dot-n commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue number

Uh oh!

codecov bot commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Dreamsorcerer left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented Sep 5, 2024

Uh oh!

lee-dot-n commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Sep 6, 2024

Uh oh!

lee-dot-n commented Sep 6, 2024

Uh oh!

bdraco commented Sep 6, 2024

Uh oh!

patchback bot commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.10: 💔 cherry-picking failed — conflicts found

Backporting merged PR #9032 into master

Uh oh!

patchback bot commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.11: 💚 backport PR created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lee-dot-n commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

lee-dot-n commented Sep 6, 2024 •

edited

Loading

patchback bot commented Sep 6, 2024 •

edited

Loading

patchback bot commented Sep 6, 2024 •

edited

Loading