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

Unconditional Url parsing breaks NamedPipeConnector on Windows #708

Closed
blaenk opened this issue Jul 28, 2024 · 1 comment · Fixed by #709
Closed

Unconditional Url parsing breaks NamedPipeConnector on Windows #708

blaenk opened this issue Jul 28, 2024 · 1 comment · Fixed by #709
Milestone

Comments

@blaenk
Copy link
Contributor

blaenk commented Jul 28, 2024

On windows, the DEFAULT_DOCKER_HOST is correctly a named pipe path:

/// The default `DOCKER_HOST` address that a windows client will try to connect to.
#[cfg(windows)]
pub const DEFAULT_DOCKER_HOST: &str = "npipe:////./pipe/docker_engine";

However, as part of computing the docker_host() based on the configuration hierarchy, it is unconditionally parsed into a Url:

Url::from_str(DEFAULT_DOCKER_HOST).expect("default host is valid URL")

This has the effect of normalizing the Url into the following value (note the removed period from the beginning of .pipe):

npipe:////pipe/docker_engine

When bollard attempts to connect through that named pipe, it removes the npipe:// prefix, expecting to be left with just //.pipe/docker_engine, but due to this artifact, it is instead left with //pipe/docker_engine.

https://github.com/fussybeaver/bollard/blob/865805f87e4066dbf1a283139e1c6148b62ccd80/src/docker.rs#L838-L843

This results in:

called `Result::unwrap()` on an `Err` value: Client(CreateContainer(HyperLegacyError { err: hyper_util::client::legacy::Error(Connect, Os { code: 53, kind: NotFound, message: "The network path was not found." }) }))

I confirmed this is a problem by forking this repo and hardcoding the value passed into the bollard client with the named path "npipe:////./pipe/docker_engine"

#[cfg(windows)]
"npipe" => Docker::connect_with_named_pipe(
host.as_str(),
DEFAULT_TIMEOUT.as_secs(),
API_DEFAULT_VERSION,
),

I think we can still parse into a Url to check the scheme for example, but we probably shouldn't use the parsed Url itself to pass on to clients. Consider that the bollard and hyper clients just take an &str anyway. If there's an error with the url formatting, it'll come out eventually.

Separately, It might be worthwhile adding a matrix to also run tests against Windows.

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 28, 2024

Thank you for the detailed description!
I’ll try to add windows runners into CI matrix

I think the main issue is normalization, and there is no way (currently?) to disable this behavior for Url.

I would like to have stricter and more explicit type used, but it seems related to servo/rust-url#602

So I’m good with proposed solution

@DDtKey DDtKey closed this as completed in 2f05648 Jul 30, 2024
@DDtKey DDtKey added this to the 0.21.0 milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants