Only query host address families over DNS that the local network stack supports by derlih · Pull Request #5176 · aio-libs/aiohttp

derlih · 2020-10-29T14:08:15Z

What do these changes do?

Workaround for incorrect socket.getaddrinfo return value in rare case

Are there changes in behavior for the user?

No

Related issue number

Fixes #5156

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

asvetlov

I like this PR
@danielnelson could you please check the pull request and make sure that the problem disappeared?

codecov-io · 2020-10-29T16:17:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.45%. Comparing base (a2dad92) to head (093977a).
⚠️ Report is 6087 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5176   +/-   ##
=======================================
  Coverage   97.45%   97.45%           
=======================================
  Files          43       43           
  Lines        8779     8779           
  Branches     1412     1412           
=======================================
  Hits         8556     8556           
  Misses        112      112           
  Partials      111      111

Flag	Coverage Δ
unit	`97.31% <ø> (ø)`

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.

derlih · 2020-10-29T16:45:17Z

@asvetlov the code cov check failed because #4556 was merged without coverage of the line

if family == socket.AF_INET6 and address[3]:  # type: ignore

I can add a unit test for this branch as well to bypass the coverage check, but I don't understand how the mock data should look like.

Also in https://docs.aiohttp.org/en/latest/contributing.html is mentioned about make cov but there is no cov target in Makefile.

$ make cov
make: *** No rule to make target 'cov'.  Stop.

danielnelson · 2020-10-29T18:01:32Z

$ cat bug.py
import asyncio
import aiohttp


async def main():
    async with aiohttp.ClientSession() as session:
        async with session.get('http://python.org') as response:
            print(response.status)


asyncio.run(main())

$ python bug.py 
Bad address format in (<AddressFamily.AF_INET6: 10>, <SocketKind.SOCK_STREAM: 1>, 6, '', (10, b'\x01\xbb\x00\x00\x00\x00*\x04NB\x00-\x00\x00'))
200
Future exception was never retrieved
future: <Future finished exception=SSLError(1, '[SSL: KRB5_S_INIT] application data after close notify (_ssl.c:2691)')>
Traceback (most recent call last):
  File "/usr/lib/python3.7/asyncio/sslproto.py", line 530, in data_received
    ssldata, appdata = self._sslpipe.feed_ssldata(data)
  File "/usr/lib/python3.7/asyncio/sslproto.py", line 207, in feed_ssldata
    self._sslobj.unwrap()
  File "/usr/lib/python3.7/ssl.py", line 778, in unwrap
    return self._sslobj.shutdown()
ssl.SSLError: [SSL: KRB5_S_INIT] application data after close notify (_ssl.c:2691)

Not sure what that SSLError is about, possibly I did something incorrect when compiling?

I suspect that this patch would be very noisy to run, as every remote with an IPv6 address would log.

danielnelson · 2020-10-29T18:17:36Z

If I enable the AsyncResolver it also raises:

Traceback (most recent call last):
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 940, in _wrap_create_connection
    return await self._loop.create_connection(*args, **kwargs)  # type: ignore  # noqa
  File "/usr/lib/python3.7/asyncio/base_events.py", line 962, in create_connection
    raise exceptions[0]
  File "/usr/lib/python3.7/asyncio/base_events.py", line 928, in create_connection
    sock = socket.socket(family=family, type=type, proto=proto)
  File "/usr/lib/python3.7/socket.py", line 151, in __init__
    _socket.socket.__init__(self, family, type, proto, fileno)
OSError: [Errno 97] Address family not supported by protocol

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "bug.py", line 13, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.7/asyncio/runners.py", line 43, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.7/asyncio/base_events.py", line 587, in run_until_complete
    return future.result()
  File "bug.py", line 9, in main
    async with session.get(sys.argv[1]) as response:
  File "/home/dbn/src/aiohttp/aiohttp/client.py", line 1070, in __aenter__
    self._resp = await self._coro
  File "/home/dbn/src/aiohttp/aiohttp/client.py", line 482, in _request
    req, traces=traces, timeout=real_timeout
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 508, in connect
    proto = await self._create_connection(req, traces, timeout)
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 863, in _create_connection
    _, proto = await self._create_direct_connection(req, traces, timeout)
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 1021, in _create_direct_connection
    raise last_exc
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 1003, in _create_direct_connection
    client_error=client_error,
  File "/home/dbn/src/aiohttp/aiohttp/connector.py", line 946, in _wrap_create_connection
    raise client_error(req.connection_key, exc) from exc
aiohttp.client_exceptions.ClientConnectorError: Cannot connect to host www.python.org:80 ssl:default [Address family not supported by protocol]

derlih · 2020-10-30T07:48:02Z

@danielnelson looks like the code passed the resolver and tries to use secured connection for http.

aiohttp/resolver.py

webknjaz

I'm pretty sure that the correct solution should be setting the AI_ADDRCONFIG flag: #5156 (comment).

This reverts commit a557e4c.

This reverts commit 9d81913.

This reverts commit 48de143.

derlih · 2020-10-31T09:30:32Z

@webknjaz
Do you think if it makes sense to add socket.AI_ADDRCONFIG to "flags" in

            hosts.append(
                {
                    "hostname": hostname,
                    "host": host,
                    "port": port,
                    "family": family,
                    "proto": proto,
                    "flags": socket.AI_NUMERICHOST | socket.AI_NUMERICSERV,
                }
            )

?

webknjaz · 2020-10-31T10:38:23Z

@derlih no, that thing is copied from socket.getnameinfo() which works on already filtered out entries.

CHANGES/5156.bugfix

webknjaz · 2020-10-31T10:44:40Z

@danielnelson please verify that this fix works for you.

danielnelson · 2020-10-31T20:38:37Z

Yes, this patch is working great!

If anyone needs to reproduce, I needed to update my test script to read the response in order to avoid the ssl.SSLError: [SSL: KRB5_S_INIT] application data after close notify (_ssl.c:2691) error. I think this was related to #3535.

import asyncio
import aiohttp
import sys


async def main():
    async with aiohttp.ClientSession() as session:
        async with session.get(sys.argv[1]) as response:
            print(response.status)
            await response.read()


asyncio.run(main())

$ python bug.py http://python.org
200

Of course, this change does not resolve the issue if using the AsyncResolver. Would it make sense for me to open a new issue for this?

asvetlov · 2020-11-01T09:30:34Z

Ok, let's apply the patch.
KRB5_S_INIT warning will be solved in aiohttp 4.0 I believe, #5102 is a blocker for this feature currently.
Agree, AsyncResolver is a different beast and should be addressed separately.

Thank you all, and especially @derlih for the patch (and patience in the patch implementation changing).

@webknjaz do you want to approve it?

…k supports (#5176) * Fix for #5156 * test for #5156 * add changes file * rearrange if/else * Revert "rearrange if/else" This reverts commit a557e4c. * Revert "test for #5156" This reverts commit 9d81913. * Revert "Fix for #5156" This reverts commit 48de143. * Add AI_ADDRCONFIG flag to loop.getaddrinfo * update changes file

github-actions · 2020-11-01T09:41:25Z

💚 Backport successful

The PR was backported to the following branches:

✅ 3.7
✅ 3.8

…k supports (#5176) (#5190) * Fix for #5156 * test for #5156 * add changes file * rearrange if/else * Revert "rearrange if/else" This reverts commit a557e4c. * Revert "test for #5156" This reverts commit 9d81913. * Revert "Fix for #5156" This reverts commit 48de143. * Add AI_ADDRCONFIG flag to loop.getaddrinfo * update changes file Co-authored-by: Dmitry Erlikh <[email protected]>

…k supports (#5176) (#5189) * Fix for #5156 * test for #5156 * add changes file * rearrange if/else * Revert "rearrange if/else" This reverts commit a557e4c. * Revert "test for #5156" This reverts commit 9d81913. * Revert "Fix for #5156" This reverts commit 48de143. * Add AI_ADDRCONFIG flag to loop.getaddrinfo * update changes file Co-authored-by: Dmitry Erlikh <[email protected]>

…k supports (aio-libs#5176) * Fix for aio-libs#5156 * test for aio-libs#5156 * add changes file * rearrange if/else * Revert "rearrange if/else" This reverts commit a557e4c. * Revert "test for aio-libs#5156" This reverts commit 9d81913. * Revert "Fix for aio-libs#5156" This reverts commit 48de143. * Add AI_ADDRCONFIG flag to loop.getaddrinfo * update changes file

derlih added 3 commits October 29, 2020 15:05

Fix for aio-libs#5156

48de143

test for aio-libs#5156

9d81913

add changes file

2339db5

derlih requested a review from asvetlov as a code owner October 29, 2020 14:08

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 29, 2020

asvetlov reviewed Oct 29, 2020

View reviewed changes

rearrange if/else

a557e4c

webknjaz reviewed Oct 30, 2020

View reviewed changes

aiohttp/resolver.py Outdated Show resolved Hide resolved

webknjaz requested changes Oct 30, 2020

View reviewed changes

derlih added 4 commits October 31, 2020 10:22

Revert "rearrange if/else"

3231774

This reverts commit a557e4c.

Revert "test for aio-libs#5156"

10057e8

This reverts commit 9d81913.

Revert "Fix for aio-libs#5156"

eb15898

This reverts commit 48de143.

Add AI_ADDRCONFIG flag to loop.getaddrinfo

62df825

webknjaz reviewed Oct 31, 2020

View reviewed changes

CHANGES/5156.bugfix Outdated Show resolved Hide resolved

webknjaz changed the title ~~Issue 5156~~ Only query host address families over DNS that the local network stack supports Oct 31, 2020

webknjaz added backport-3.7 labels Oct 31, 2020

update changes file

093977a

derlih deleted the issue_5156 branch November 1, 2020 09:10

webknjaz approved these changes Nov 1, 2020

View reviewed changes

webknjaz merged commit 9bce9c2 into aio-libs:master Nov 1, 2020

github-actions bot mentioned this pull request Nov 1, 2020

[3.7] Only query host address families over DNS that the local network stack supports (#5176) #5189

Merged

github-actions bot mentioned this pull request Nov 1, 2020

[3.8] Only query host address families over DNS that the local network stack supports (#5176) #5190

Merged

webknjaz mentioned this pull request Jul 19, 2021

resolver.py/ThreadResolver: address[3] does not exist #5901

Closed

1 task

Uh oh!

Conversation

derlih commented Oct 29, 2020 • edited by webknjaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 29, 2020 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

derlih commented Oct 29, 2020

Uh oh!

danielnelson commented Oct 29, 2020 • edited by webknjaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielnelson commented Oct 29, 2020 • edited by webknjaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derlih commented Oct 30, 2020

Uh oh!

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

derlih commented Oct 31, 2020

Uh oh!

webknjaz commented Oct 31, 2020

Uh oh!

Uh oh!

webknjaz commented Oct 31, 2020

Uh oh!

danielnelson commented Oct 31, 2020

Uh oh!

asvetlov commented Nov 1, 2020

Uh oh!

github-actions bot commented Nov 1, 2020

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

derlih commented Oct 29, 2020 •

edited by webknjaz

Loading

codecov-io commented Oct 29, 2020 •

edited by codecov bot

Loading

danielnelson commented Oct 29, 2020 •

edited by webknjaz

Loading

danielnelson commented Oct 29, 2020 •

edited by webknjaz

Loading