fix DinD host detection when using local socket - #283
Conversation
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
Rebased to the master. @tillahoffmann, any chance for a review? |
Sorry, something went wrong.
|
rebased to master |
Sorry, something went wrong.
|
Thank you for these improvements! Are there tests that we could add that fail before the change is introduced to avoid future regressions? |
Sorry, something went wrong.
tillahoffmann
left a comment
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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I think something like this:
try:
from docker.transport.npipeconn import NpipeHTTPAdapter
except ImportError:
class NpipeHTTPAdapter: ...Would be a more proper solution.
It is much clearer what happens, does not depend the design of the Adapter and respects subclasses.
Sorry, something went wrong.
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.
|
@tillahoffmann Hi, it's been more than half a year since I've opened this PR. Can we maybe merge this? |
Sorry, something went wrong.
|
This should be obsolete now that #714 is merged. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
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.