fix DinD host detection when using local socket #283
Conversation
Codecov Report
@@ Coverage Diff @@
## main #283 +/- ##
=======================================
Coverage ? 87.30%
=======================================
Files ? 30
Lines ? 843
Branches ? 57
=======================================
Hits ? 736
Misses ? 76
Partials ? 31 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ikravets
force-pushed
the
fix-dind-host-detection
branch
from
dfd26e6
to
afa057e
Compare
|
Rebased to the master. @tillahoffmann, any chance for a review? |
ikravets
force-pushed
the
fix-dind-host-detection
branch
from
afa057e
to
01f9e77
Compare
|
rebased to master |
|
Thank you for these improvements! Are there tests that we could add that fail before the change is introduced to avoid future regressions? |
tillahoffmann
added
👀 requires attention
📦 package: core
and removed
👀 requires attention
labels
There was a problem hiding this comment.
This is great, thank you for having such a detailed look into improving the host detection. Host detection is the challenge that seems to cause most trouble for our users so far.
Do you know whether this change would also resolve the issue of localnpipe appearing in the connection string, as discussed in #108? You're much more clued up on this than I am.
| adapter = self.client.api.get_adapter(self.client.api.base_url) | ||
| is_ipc = isinstance(adapter, UnixHTTPAdapter) | ||
| is_ipc |= hasattr(adapter, "socket_path") or hasattr(adapter, "npipe_path") | ||
| is_ipc |= 'unix' in url.scheme or 'npipe' in url.scheme |
There was a problem hiding this comment.
Even if the NpipeHTTPAdapter is only used on windows, would it still be available for an isinstance check independent of the platform? Or does the docker package not even expose NpipeHTTPAdapter if it's not on windows?
Alternatively, would checking adapter.__class__.name == 'NpipeHTTPAdapter' be easier to understand than checking the attributes of the adapter?
There was a problem hiding this comment.
I don't think that NpipeHTTPAdapter symbol is available outside of windows. Checking the class name would break for NpipeHTTPAdapter subclasses. So I went on with a classic duck-typing.
When using IPC socket (unix/npipe) to communicate with local docker server docker-py implementation rewrites the base url to HTTP, see https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/api/client.py#L143-L168 We cannot rely on URL scheme in order to detect such connections. Instead, we detect connection adapters added by docker-py api client: - UnixHTTPAdapter https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/transport/unixconn.py - NpipeHTTPAdapter https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/transport/npipeconn.py As NpipeHTTPAdapter type may not be available outside of Windows we rely on adapter-specific attributes.
ikravets
force-pushed
the
fix-dind-host-detection
branch
from
f6eea5f
to
f9d4508
Compare
|
@tillahoffmann Hi, it's been more than half a year since I've opened this PR. Can we maybe merge this? |
|
@tillahoffmann a kind ping |
When using IPC socket (unix/npipe) to communicate with local docker server docker-py implementation rewrites the base url to HTTP, see https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/api/client.py#L143-L168
We cannot rely on URL scheme in order to detect such connections. Instead, we detect connection adapters added by docker-py api client:
UnixHTTPAdapter https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/transport/unixconn.py
NpipeHTTPAdapter https://github.com/docker/docker-py/blob/e901eac7a8c5f29c7720eafb9f58c8356cca2324/docker/transport/npipeconn.py
As NpipeHTTPAdapter type may not be available outside of Windows we rely on adapter-specific attributes.