search

testcontainers / testcontainers-rs

A library for integration-testing against docker containers from within Rust.
https://rust.testcontainers.org
Apache License 2.0
724 stars 143 forks source link

Unconditional `Url` parsing breaks `NamedPipeConnector` on Windows #708

Closed blaenk closed 2 months ago

blaenk commented 2 months ago

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

https://github.com/testcontainers/testcontainers-rs/blob/e354b7532b5d448ab37c9f95dab6c68c592cb62b/testcontainers/src/core/env/config.rs#L32-L34

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

https://github.com/testcontainers/testcontainers-rs/blob/e354b7532b5d448ab37c9f95dab6c68c592cb62b/testcontainers/src/core/env/config.rs#L141

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"

https://github.com/testcontainers/testcontainers-rs/blob/e354b7532b5d448ab37c9f95dab6c68c592cb62b/testcontainers/src/core/client/bollard_client.rs#L31-L36

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 commented 2 months ago

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 https://github.com/servo/rust-url/issues/602

So I’m good with proposed solution