Skip to content

Commit

Permalink
fix: don't assume docker host is a url (#709)
Browse files Browse the repository at this point in the history
This is one way to fix
#708

Although we still parse into a `Url`, we don't pass that value on to
clients, since `Url` may mutate the input, which in the case of #708,
breaks Windows `NamedPipeConnector`, which is the default way of
connecting to the Docker daemon.

I also disabled the SCTP tests on Windows since it doesn't seem there's
support for that on Windows.
  • Loading branch information
blaenk committed Jul 30, 2024
1 parent fe9de51 commit 2f05648
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 49 deletions.
22 changes: 12 additions & 10 deletions testcontainers/src/core/client.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::io;
use std::{io, str::FromStr};

use bollard::{
auth::DockerCredentials,
Expand All @@ -12,6 +12,7 @@ use bollard::{
use bollard_stubs::models::{ContainerInspectResponse, ExecInspectResponse, Network};
use futures::{StreamExt, TryStreamExt};
use tokio::sync::OnceCell;
use url::Url;

use crate::core::{
client::exec::ExecResult,
Expand Down Expand Up @@ -312,15 +313,16 @@ impl Client {

pub(crate) async fn docker_hostname(&self) -> Result<url::Host, ClientError> {
let docker_host = self.config.docker_host();
match docker_host.scheme() {
"tcp" | "http" | "https" => {
docker_host
.host()
.map(|host| host.to_owned())
.ok_or_else(|| {
ConfigurationError::InvalidDockerHost(docker_host.to_string()).into()
})
}
let docker_host_url = Url::from_str(docker_host)
.map_err(|e| ConfigurationError::InvalidDockerHost(e.to_string()))?;

match docker_host_url.scheme() {
"tcp" | "http" | "https" => docker_host_url
.host()
.map(|host| host.to_owned())
.ok_or_else(|| {
ConfigurationError::InvalidDockerHost(docker_host.to_string()).into()
}),
"unix" | "npipe" => {
if is_in_container().await {
let host = self
Expand Down
28 changes: 10 additions & 18 deletions testcontainers/src/core/client/bollard_client.rs
Original file line number Diff line number Diff line change
@@ -1,39 +1,31 @@
use std::time::Duration;
use std::{str::FromStr, time::Duration};

use bollard::{Docker, API_DEFAULT_VERSION};
use url::Url;

use crate::core::env;

const DEFAULT_TIMEOUT: Duration = Duration::from_secs(2 * 60);

pub(super) fn init(config: &env::Config) -> Result<Docker, bollard::errors::Error> {
let host = config.docker_host();
let host_url = Url::from_str(host)?;

match host.scheme() {
match host_url.scheme() {
"https" => connect_with_ssl(config),
"http" | "tcp" => {
if config.tls_verify() {
connect_with_ssl(config)
} else {
Docker::connect_with_http(
host.as_str(),
DEFAULT_TIMEOUT.as_secs(),
API_DEFAULT_VERSION,
)
Docker::connect_with_http(host, DEFAULT_TIMEOUT.as_secs(), API_DEFAULT_VERSION)
}
}
#[cfg(unix)]
"unix" => Docker::connect_with_unix(
host.as_str(),
DEFAULT_TIMEOUT.as_secs(),
API_DEFAULT_VERSION,
),
"unix" => Docker::connect_with_unix(host, DEFAULT_TIMEOUT.as_secs(), API_DEFAULT_VERSION),
#[cfg(windows)]
"npipe" => Docker::connect_with_named_pipe(
host.as_str(),
DEFAULT_TIMEOUT.as_secs(),
API_DEFAULT_VERSION,
),
"npipe" => {
Docker::connect_with_named_pipe(host, DEFAULT_TIMEOUT.as_secs(), API_DEFAULT_VERSION)
}
_ => Err(bollard::errors::Error::UnsupportedURISchemeError {
uri: host.to_string(),
}),
Expand All @@ -44,7 +36,7 @@ fn connect_with_ssl(config: &env::Config) -> Result<Docker, bollard::errors::Err
let cert_path = config.cert_path().expect("cert path not found");

Docker::connect_with_ssl(
config.docker_host().as_str(),
config.docker_host(),
&cert_path.join("key.pem"),
&cert_path.join("cert.pem"),
&cert_path.join("ca.pem"),
Expand Down
31 changes: 11 additions & 20 deletions testcontainers/src/core/env/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ use std::{
str::FromStr,
};

use url::Url;

use crate::core::env::GetEnvValue;

#[derive(Debug, thiserror::Error)]
Expand Down Expand Up @@ -35,8 +33,8 @@ pub const DEFAULT_DOCKER_HOST: &str = "npipe:////./pipe/docker_engine";

#[derive(Debug, Default)]
pub(crate) struct Config {
tc_host: Option<Url>,
host: Option<Url>,
tc_host: Option<String>,
host: Option<String>,
tls_verify: Option<bool>,
cert_path: Option<PathBuf>,
command: Option<Command>,
Expand All @@ -48,9 +46,9 @@ pub(crate) struct Config {
#[derive(Debug, Default, serde::Deserialize)]
struct TestcontainersProperties {
#[serde(rename = "tc.host")]
tc_host: Option<Url>,
tc_host: Option<String>,
#[serde(rename = "docker.host")]
host: Option<Url>,
host: Option<String>,
#[serde_as(as = "Option<serde_with::BoolFromInt>")]
#[serde(rename = "docker.tls.verify")]
tls_verify: Option<bool>,
Expand Down Expand Up @@ -103,11 +101,7 @@ impl Config {
where
E: GetEnvValue,
{
let host = E::get_env_value("DOCKER_HOST")
.as_deref()
.map(FromStr::from_str)
.transpose()
.map_err(|e: url::ParseError| ConfigurationError::InvalidDockerHost(e.to_string()))?;
let host = E::get_env_value("DOCKER_HOST");
let tls_verify = E::get_env_value("DOCKER_TLS_VERIFY").map(|v| v == "1");
let cert_path = E::get_env_value("DOCKER_CERT_PATH").map(PathBuf::from);
let command = E::get_env_value("TESTCONTAINERS_COMMAND")
Expand All @@ -132,14 +126,11 @@ impl Config {
/// 2. `DOCKER_HOST` environment variable.
/// 3. Docker host from the `docker.host` property in the `~/.testcontainers.properties` file.
/// 4. Else, the default Docker socket will be returned.
pub(crate) fn docker_host(&self) -> Url {
pub(crate) fn docker_host(&self) -> &str {
self.tc_host
.as_ref()
.or(self.host.as_ref())
.cloned()
.unwrap_or_else(|| {
Url::from_str(DEFAULT_DOCKER_HOST).expect("default host is valid URL")
})
.as_deref()
.or(self.host.as_deref())
.unwrap_or(DEFAULT_DOCKER_HOST)
}

pub(crate) fn tls_verify(&self) -> bool {
Expand Down Expand Up @@ -211,8 +202,8 @@ mod tests {

#[test]
fn deserialize_java_properties() {
let tc_host = Url::parse("http://tc-host").unwrap();
let docker_host = Url::parse("http://docker-host").unwrap();
let tc_host = "http://tc-host".into();
let docker_host = "http://docker-host".into();
let tls_verify = 1;
let cert_path = "/path/to/cert";

Expand Down
2 changes: 2 additions & 0 deletions testcontainers/src/runners/async_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ mod tests {
Ok(())
}

#[cfg(unix)]
#[tokio::test]
async fn async_run_command_should_map_exposed_port_udp_sctp() -> anyhow::Result<()> {
let client = Client::lazy_client().await?;
Expand Down Expand Up @@ -370,6 +371,7 @@ mod tests {
Ok(())
}

#[cfg(unix)]
#[tokio::test]
async fn async_run_command_should_map_ports_udp_sctp() -> anyhow::Result<()> {
let client = Client::lazy_client().await?;
Expand Down
3 changes: 2 additions & 1 deletion testcontainers/tests/async_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ async fn bollard_can_run_hello_world_with_multi_thread() -> anyhow::Result<()> {
}

async fn cleanup_hello_world_image() -> anyhow::Result<()> {
let docker = Docker::connect_with_unix_defaults()?;
let docker = Docker::connect_with_local_defaults()?;

futures::future::join_all(
docker
.list_images::<String>(None)
Expand Down

0 comments on commit 2f05648

Please sign in to comment.