Skip to content
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

Error while trying to use unix socket, when aiohttp>=3.10 #8600

Closed
aljazmedic opened this issue Aug 4, 2024 · 11 comments · Fixed by #8632
Closed

Error while trying to use unix socket, when aiohttp>=3.10 #8600

aljazmedic opened this issue Aug 4, 2024 · 11 comments · Fixed by #8632
Labels
bug regression Something that used to work stopped working "as before" after upgrade
Milestone

Comments

@aljazmedic
Copy link

Long story short

  • Expected behaviour: Communicating via unix socket should work.
  • Actual behaviour: Error. Its traceback:
Traceback (most recent call last):
  File "/tmp/aiodocker_fail/example.py", line 12, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 654, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/example.py", line 8, in main
    await docker.version()
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 190, in version
    data = await self._query_json("version")
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 319, in _query_json
    async with self._query(
  File "/usr/lib/python3.11/contextlib.py", line 210, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 231, in _query
    yield await self._do_query(
          ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 257, in _do_query
    await self._check_version()
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 209, in _check_version
    ver = await self._query_json("version", versioned_api=False)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 319, in _query_json
    async with self._query(
  File "/usr/lib/python3.11/contextlib.py", line 210, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 231, in _query
    yield await self._do_query(
          ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 267, in _do_query
    response = await self.session.request(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiohttp/client.py", line 510, in _request
    raise NonHttpUrlClientError(url)
aiohttp.client_exceptions.NonHttpUrlClientError: unix://localhost/version
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7f5ede16bf10>

How to reproduce

In fresh virtual environment run pip install aiodocker, the following script fails:

#!/usr/bin/env python3
from aiodocker import Docker
import asyncio

async def main():
    docker = Docker()
    await docker.version()

if __name__ == "__main__":
    asyncio.run(main())

Your environment

Just the latest version of aiodocker with combination of 3.10 aiohttp.

My complete requirements.txt of the environment:

aiodocker==0.22.2
aiohappyeyeballs==2.3.4
aiohttp==3.10.0
aiosignal==1.3.1
attrs==24.1.0
frozenlist==1.4.1
idna==3.7
multidict==6.0.5
yarl==1.9.4

Current fix for me is the usage of aiohttp==3.9.0

@bdraco bdraco transferred this issue from aio-libs/aiodocker Aug 4, 2024
@bdraco bdraco added this to the 3.10.2 milestone Aug 4, 2024
@bdraco bdraco added the bug label Aug 4, 2024
@bdraco
Copy link
Member

bdraco commented Aug 4, 2024

Likely similar to #8482

@Artucuno
Copy link

Artucuno commented Aug 4, 2024

+1

@Dreamsorcerer
Copy link
Member

I actually didn't realise the client can make unix socket requests.

Looking at the connectors, there is UnixConnector and also NamedPipeConnector. It is also possible to create custom connectors...

Maybe we need the connector to define what a valid URL looks like and then the client should use that information?

@bdraco
Copy link
Member

bdraco commented Aug 4, 2024

That sounds like a better design to me. The downside is it might technically be a breaking change for anyone subclassing the BaseConnector as they would have to add this now.

@Dreamsorcerer
Copy link
Member

Maybe allow anything in the BaseConnector for 3.10, and have it abstract in v4.

@bdraco bdraco added the regression Something that used to work stopped working "as before" after upgrade label Aug 4, 2024
@ualex73
Copy link

ualex73 commented Aug 5, 2024

Same problem here with monitor_docker component using aiodocker. The aiohttp 3.10.x broke the unix socket and the tcp socket.

Following worked in aiohttp 3.9.5:
tcp://IP:2375
unix://localhost

Both are broken now, for the same reason. The tcp can be fixed by replacing it with http.

@ualex73
Copy link

ualex73 commented Aug 5, 2024

@Dreamsorcerer

UnixConnector

It looks like the aiodocker is already doing that, I see in the code:

connector = aiohttp.UnixConnector(docker_host[UNIX_PRE_LEN:])
and also a pipe one:
connector = aiohttp.NamedPipeConnector(docker_host[WIN_PRE_LEN:].replace("/", "\"))

@Dreamsorcerer
Copy link
Member

It looks like the aiodocker is already doing that, I see in the code:

We're aware of that. The point is that currently the client just tries a whitelist of protocols, but really the whitelist should depend which connector is being used (http:// won't work if UnixConnector is used, for example).

I guess tcp:// should be added to the list? I'm not aware of this being a recognised URL protocol though, so not sure why that is in use..

@bdraco
Copy link
Member

bdraco commented Aug 7, 2024

I don't think tcp:// is needed since that is always expected to have a higher level http,https,ws,or wss.

@Dreamsorcerer
Copy link
Member

I don't think tcp:// is needed since that is always expected to have a higher level http,https,ws,or wss.

The linked issue suggests there are multiple people doing this in homeassistant though? ualex73/monitor_docker#157 (comment)

@bdraco
Copy link
Member

bdraco commented Aug 7, 2024

I guess we do need it. I couldn't find anything in the aiodocker code that uses it though but probably because its passed in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug regression Something that used to work stopped working "as before" after upgrade
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants