diff --git a/src/commands/init/steps.rs b/src/commands/init/steps.rs index 1a5330ff..d6cd6c41 100644 --- a/src/commands/init/steps.rs +++ b/src/commands/init/steps.rs @@ -886,27 +886,23 @@ mod rollback_tests { use std::fs; use std::path::PathBuf; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, - write_fake_docker_script, - }; + use super::test_support::write_self_contained_fake_docker; let dir = tempfile::tempdir().unwrap(); let messages = crate::i18n::test_messages(); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).unwrap(); - write_fake_docker_script(&bin_dir.join("docker")); + // A fake that *would* log is what makes the absent log evidence: + // it is named through the seam, so anything the rollback ran + // would have left a record. + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let hcl_path = dir.path().join("openbao.hcl"); fs::write(&hcl_path, "tls_cert_file = ...\n").unwrap(); let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let rollback = InitRollback { + docker: Some(fake), hcl_backup: Some(RollbackFile { path: hcl_path.clone(), original: Some("tls_disable = 1\n".to_string()), @@ -987,6 +983,21 @@ pub(super) mod test_support { pub(in crate::commands::init::steps) fn write_self_contained_fake_docker( path: &Path, args_log: &Path, + ) { + write_self_contained_fake_docker_exiting(path, args_log, 0); + } + + /// [`write_self_contained_fake_docker`] whose every invocation exits + /// `exit_code` after logging, so a test can steer the failure path + /// of a docker call production spawns on its behalf. + /// + /// The log it writes is the same one the zero-exit writer produces — + /// one appended, space-joined line per invocation — so a test that + /// swaps one writer for the other keeps its assertions. + pub(in crate::commands::init::steps) fn write_self_contained_fake_docker_exiting( + path: &Path, + args_log: &Path, + exit_code: u8, ) { let log = args_log.display().to_string(); assert!( @@ -994,7 +1005,7 @@ pub(super) mod test_support { "the log path is interpolated into a single-quoted shell word" ); let script = format!( - "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit 0\n" + "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit {exit_code}\n" ); fs::write(path, script).expect("fake docker script should be written"); fs::set_permissions(path, fs::Permissions::from_mode(0o700)) diff --git a/src/commands/init/steps/http01_admin_tls.rs b/src/commands/init/steps/http01_admin_tls.rs index 09464529..c683a14f 100644 --- a/src/commands/init/steps/http01_admin_tls.rs +++ b/src/commands/init/steps/http01_admin_tls.rs @@ -267,6 +267,7 @@ pub(crate) fn strip_responder_tls_config(secrets_dir: &Path, messages: &Messages mod tests { use std::collections::BTreeMap; + use super::super::test_support::write_self_contained_fake_docker; use super::*; /// The responder container name a default install renders. const DEFAULT_RESPONDER_CONTAINER: &str = "bootroot-http01"; @@ -301,9 +302,9 @@ mod tests { #[test] fn reissue_falls_back_to_instance_scoped_sans() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - std::fs::create_dir(&bin_dir).unwrap(); - crate::commands::rotate::test_support::write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let secrets_dir = dir.path().join("secrets"); let tls_dir = secrets_dir.join("bootroot-http01").join("tls"); @@ -324,26 +325,10 @@ mod tests { expires_at: None, }; - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = crate::commands::rotate::test_support::env_lock(); - let _path = crate::commands::rotate::test_support::ScopedEnvVar::set( - "PATH", - crate::commands::rotate::test_support::path_with_prepend(&bin_dir), - ); - let _log = crate::commands::rotate::test_support::ScopedEnvVar::set( - crate::commands::rotate::test_support::TEST_DOCKER_ARGS_ENV, - &args_log, - ); - reissue_http01_admin_tls_cert( - &secrets_dir, - &entry, - "insight-http01", - Path::new("docker"), - &messages, - ) - .expect("re-issuance must succeed against the fake docker"); + reissue_http01_admin_tls_cert(&secrets_dir, &entry, "insight-http01", &fake, &messages) + .expect("re-issuance must succeed against the fake docker"); let log = std::fs::read_to_string(&args_log).unwrap_or_default(); assert!( diff --git a/src/commands/init/steps/openbao_tls.rs b/src/commands/init/steps/openbao_tls.rs index 0d11ee30..a46bb2d0 100644 --- a/src/commands/init/steps/openbao_tls.rs +++ b/src/commands/init/steps/openbao_tls.rs @@ -475,34 +475,14 @@ mod tests { use std::collections::BTreeMap; use std::fs; - use super::super::test_support::write_self_contained_fake_docker; - use super::*; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, + use super::super::test_support::{ + write_self_contained_fake_docker, write_self_contained_fake_docker_exiting, }; + use super::*; /// The `OpenBao` container name a default install renders. const DEFAULT_OPENBAO_CONTAINER: &str = "bootroot-openbao"; - /// Fake `docker` that *appends* one line per invocation, unlike the - /// shared helper which truncates: the ordering of the two containers - /// this file runs is exactly what the wiring test below pins. - /// - /// `exit_code` is returned by every invocation, so a non-zero value - /// makes the first container this file runs — the chown — fail. - fn write_appending_fake_docker(path: &Path, exit_code: u8) { - let script = format!( - r#"#!/bin/sh -set -eu -{{ printf '%s ' "$@"; printf '\n'; }} >> "${{BOOTROOT_TEST_DOCKER_ARGS:?missing log path}}" -exit {exit_code} -"# - ); - fs::write(path, script).expect("fake docker script should be written"); - fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); - } - #[test] fn build_sans_includes_specific_ip() { let sans = build_openbao_tls_sans("192.168.1.10:8200", None, DEFAULT_OPENBAO_CONTAINER); @@ -532,9 +512,9 @@ exit {exit_code} #[test] fn reissue_falls_back_to_instance_scoped_sans() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -557,18 +537,14 @@ exit {exit_code} expires_at: None, }; - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); reissue_openbao_tls_cert( &compose_dir, &secrets_dir, &entry, "insight-openbao", - Path::new("docker"), + &fake, &messages, ) .expect("re-issuance must succeed against the fake docker"); @@ -860,9 +836,9 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_chowns_output_dir_before_creating_the_cert() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -874,18 +850,13 @@ exit {exit_code} fs::write(tls_dir.join("server.crt"), "cert").unwrap(); fs::write(tls_dir.join("server.key"), "key").unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect("issuing the certificate must succeed against the fake docker"); @@ -936,26 +907,21 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_aborts_when_the_chown_fails() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 1); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker_exiting(&fake, &args_log, 1); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); fs::create_dir_all(&secrets_dir).unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let error = issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect_err("a failing chown must fail the issuance"); @@ -988,9 +954,12 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_refuses_a_symlinked_output_dir() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + // The fake would log any invocation it received, so the empty + // log below is evidence that nothing ran rather than evidence + // that nothing could. + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -1003,18 +972,13 @@ exit {exit_code} fs::create_dir_all(tls_dir.parent().expect("openbao dir")).unwrap(); std::os::unix::fs::symlink(&elsewhere, &tls_dir).unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let error = issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect_err("a symlinked output directory must fail the issuance"); diff --git a/src/commands/init/steps/openbao_transition.rs b/src/commands/init/steps/openbao_transition.rs index 6c756a94..4bf765d8 100644 --- a/src/commands/init/steps/openbao_transition.rs +++ b/src/commands/init/steps/openbao_transition.rs @@ -350,10 +350,8 @@ mod tests { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; + use super::super::test_support::write_self_contained_fake_docker; use super::*; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, write_fake_docker_script, - }; use crate::i18n::test_messages; const PROBE_ATTEMPTS: u32 = 1; @@ -577,37 +575,32 @@ mod tests { /// The availability pre-check runs before Docker is touched, so a /// deployment is never knocked into a sealed state `init` cannot /// recover from. - #[test] - fn no_available_source_fails_before_any_docker_call() { + #[tokio::test] + async fn no_available_source_fails_before_any_docker_call() { let dir = tempdir().expect("temp dir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + // The transition is handed a fake that logs every invocation, so + // the absent log below proves no docker command was emitted. + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose = dir.path().join("docker-compose.yml"); let override_path = dir.path().join("docker-compose.openbao-exposed.yml"); let default_file = dir.path().join("unseal-keys.txt"); - let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); let transition = OpenBaoTlsTransition { + docker: &fake, probe_attempts: PROBE_ATTEMPTS, probe_delay: PROBE_DELAY, ..OpenBaoTlsTransition::new(&compose, &override_path, UNREACHABLE_HTTPS_URL, dir.path()) }; let mut recreated = false; - // `block_on` rather than `#[tokio::test]`: the environment lock - // has to stay held across the whole run so a parallel test - // cannot swap PATH out from under the fake `docker`. - let err = runtime - .block_on(transition.run( + let err = transition + .run( &inputs(&[], None, default_file, false), &mut recreated, &test_messages(), - )) + ) + .await .expect_err("no unseal key source is available"); assert!( @@ -785,39 +778,33 @@ mod tests { /// same failure a listener still answering plaintext produces. /// `state.openbao_url` is advanced by the caller only after this /// returns `Ok`, so the pre-TLS plaintext URL survives. - #[test] - fn tls_probe_failure_stops_before_the_url_is_recorded() { + #[tokio::test] + async fn tls_probe_failure_stops_before_the_url_is_recorded() { let dir = tempdir().expect("temp dir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose = dir.path().join("docker-compose.yml"); let override_path = dir.path().join("docker-compose.openbao-exposed.yml"); let state_path = dir.path().join("state.json"); let plaintext_state = "{\"openbao_url\":\"http://127.0.0.1:8200\"}\n"; fs::write(&state_path, plaintext_state).expect("write state"); let in_memory = vec!["memory-key".to_string()]; - let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); let transition = OpenBaoTlsTransition { + docker: &fake, probe_attempts: PROBE_ATTEMPTS, probe_delay: PROBE_DELAY, ..OpenBaoTlsTransition::new(&compose, &override_path, UNREACHABLE_HTTPS_URL, dir.path()) }; let mut recreated = false; - // See `no_available_source_fails_before_any_docker_call` for why - // this drives the future with `block_on`. - let err = runtime - .block_on(transition.run( + let err = transition + .run( &inputs(&in_memory, None, dir.path().join("unseal-keys.txt"), false), &mut recreated, &test_messages(), - )) + ) + .await .expect_err("nothing is listening on the probed URL"); assert!( diff --git a/src/commands/init/steps/stepca_setup.rs b/src/commands/init/steps/stepca_setup.rs index 3a928c57..81885fb6 100644 --- a/src/commands/init/steps/stepca_setup.rs +++ b/src/commands/init/steps/stepca_setup.rs @@ -572,41 +572,13 @@ mod tests { assert!(!names.contains(&DEFAULT_CA_CONTAINER.to_string())); } + /// The executable seam: the caller names the program, and the child + /// that runs is the one it named. + /// /// The restart bypasses Compose, so nothing but the name it is /// handed decides which install's sidecar goes down. It used to /// inline a default-instance literal, which on a non-default /// instance restarted a co-located default install's sidecar. - #[test] - fn restarting_the_stepca_sidecar_names_the_container_it_is_given() { - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, - write_fake_docker_script, - }; - - let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - - assert!(restart_stepca_openbao_agent( - "insight-openbao-agent-stepca", - Path::new("docker") - )); - - let logged = fs::read_to_string(&args_log).expect("read docker args"); - assert_eq!( - logged.lines().collect::>(), - vec!["restart", "insight-openbao-agent-stepca"] - ); - } - - /// The executable seam: the caller names the program, and the child - /// that runs is the one it named. /// /// The fake carries its own argv-log path in its script text, so /// nothing here sets a variable on this process or edits `PATH` — diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index eefc7d99..8f6c4057 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -278,89 +278,18 @@ async fn run_rotate_with_exec( Ok(RotateOutcome::Completed) } -/// Shared test harness for commands that shell out to `docker`. +/// Test harness for the `rotate` commands that shell out to `docker`. /// -/// The fake-`docker` script and the environment scoping around it are -/// `pub(in crate::commands)` rather than `pub(super)` because the -/// `OpenBao` TLS transition in `init` asserts on the docker argv it -/// emits with the same harness. +/// It is private to this module and its descendants: a test names the +/// fake executable through the `docker` seam production already +/// carries, so nothing outside `rotate` reaches in here any more. #[cfg(test)] -pub(super) mod test_support { - use std::env; - use std::ffi::{OsStr, OsString}; +mod test_support { use std::fs; use std::path::Path; - use std::sync::{LazyLock, Mutex, MutexGuard}; pub(super) use crate::i18n::test_messages; - static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - pub(in crate::commands) const TEST_DOCKER_ARGS_ENV: &str = "BOOTROOT_TEST_DOCKER_ARGS"; - - pub(in crate::commands) struct ScopedEnvVar { - key: &'static str, - previous: Option, - } - - impl ScopedEnvVar { - pub(in crate::commands) fn set(key: &'static str, value: impl AsRef) -> Self { - let previous = env::var_os(key); - // SAFETY: Tests hold ENV_LOCK while mutating process environment. - unsafe { - env::set_var(key, value); - } - Self { key, previous } - } - } - - impl Drop for ScopedEnvVar { - fn drop(&mut self) { - // SAFETY: Tests hold ENV_LOCK while mutating process environment. - unsafe { - if let Some(previous) = &self.previous { - env::set_var(self.key, previous); - } else { - env::remove_var(self.key); - } - } - } - } - - pub(in crate::commands) fn env_lock() -> MutexGuard<'static, ()> { - ENV_LOCK - .lock() - .expect("environment lock must not be poisoned") - } - - pub(in crate::commands) fn write_fake_docker_script(path: &Path) { - let script = r#"#!/bin/sh -set -eu -printf '%s\n' "$@" > "${BOOTROOT_TEST_DOCKER_ARGS:?missing log path}" -if [ -n "${BOOTROOT_TEST_DOCKER_STDERR:-}" ]; then - printf '%s' "${BOOTROOT_TEST_DOCKER_STDERR}" 1>&2 -fi -if [ -n "${BOOTROOT_TEST_DOCKER_EXIT:-}" ]; then - exit "${BOOTROOT_TEST_DOCKER_EXIT}" -fi -exit 0 -"#; - fs::write(path, script).expect("fake docker script should be written"); - #[cfg(unix)] - fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); - } - - #[cfg(unix)] - use std::os::unix::fs::PermissionsExt; - - pub(in crate::commands) fn path_with_prepend(bin_dir: &Path) -> OsString { - let mut paths = vec![bin_dir.to_path_buf()]; - if let Some(existing) = env::var_os("PATH") { - paths.extend(env::split_paths(&existing)); - } - env::join_paths(paths).expect("PATH components should be valid") - } - /// Writes a fake `docker` at `path` that appends one record per /// invocation to `args_log` and reads nothing from its environment. /// diff --git a/src/commands/verify.rs b/src/commands/verify.rs index 844951c8..871f9a27 100644 --- a/src/commands/verify.rs +++ b/src/commands/verify.rs @@ -1,5 +1,5 @@ use std::collections::HashSet; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -35,7 +35,13 @@ pub(crate) fn run_verify(args: &VerifyArgs, messages: &Messages) -> Result<()> { print_verify_plan(&entry.service_name, agent_config, messages); - let agent_binary = resolve_agent_binary(args.agent_binary.as_deref(), messages)?; + let search_path = std::env::var_os("PATH"); + let agent_binary = resolve_agent_binary( + args.agent_binary.as_deref(), + search_path.as_deref(), + Path::new("."), + messages, + )?; let status = Command::new(&agent_binary) .args(oneshot_agent_args(entry, agent_config)) .status() @@ -155,7 +161,20 @@ fn oneshot_agent_args(entry: &ServiceEntry, agent_config: &Path) -> Vec, messages: &Messages) -> Result { +/// Resolves the agent executable from the explicit override, then a +/// sibling of the running binary, then `search_path`. +/// +/// `search_path` is the `PATH` value to search — `None` for a variable +/// that is unset, which the error message distinguishes — and +/// `empty_entry_base` is the directory an empty entry stands for. Both +/// are supplied by the caller rather than read here, so the search is +/// exercised without moving anything process-global. +fn resolve_agent_binary( + override_path: Option<&Path>, + search_path: Option<&OsStr>, + empty_entry_base: &Path, + messages: &Messages, +) -> Result { let mut candidates = Vec::new(); if let Some(path) = override_path { @@ -176,7 +195,7 @@ fn resolve_agent_binary(override_path: Option<&Path>, messages: &Messages) -> Re } } - let (found, path_candidates) = find_on_path(AGENT_BINARY_NAME); + let (found, path_candidates) = find_on_path(AGENT_BINARY_NAME, search_path, empty_entry_base); for candidate in &path_candidates { candidates.push(candidate.display().to_string()); } @@ -190,17 +209,27 @@ fn resolve_agent_binary(override_path: Option<&Path>, messages: &Messages) -> Re anyhow::bail!(messages.error_bootroot_agent_not_found(&candidates.join(", "))); } -fn find_on_path(name: &str) -> (Option, Vec) { +/// Searches `search_path` for `name`, returning the first match and +/// every candidate it looked at. +/// +/// `search_path` is the `PATH` value itself rather than a variable name, +/// and `empty_entry_base` is the directory an empty entry stands for — +/// `.` for the production caller, which is what POSIX prescribes. +fn find_on_path( + name: &str, + search_path: Option<&OsStr>, + empty_entry_base: &Path, +) -> (Option, Vec) { let mut checked = Vec::new(); - let Some(path_var) = std::env::var_os("PATH") else { + let Some(path_var) = search_path else { return (None, checked); }; let mut found = None; - for dir in std::env::split_paths(&path_var) { + for dir in std::env::split_paths(path_var) { // POSIX treats an empty PATH entry (leading/trailing/doubled `:`) as // the current working directory, so normalise before searching. let search_dir = if dir.as_os_str().is_empty() { - PathBuf::from(".") + empty_entry_base.to_path_buf() } else { dir }; @@ -386,14 +415,10 @@ fn expected_dns_name(entry: &ServiceEntry, messages: &Messages) -> Result = Mutex::new(()); - fn test_service_entry(delivery_mode: DeliveryMode) -> ServiceEntry { ServiceEntry { service_name: "edge-proxy".to_string(), @@ -635,46 +660,38 @@ mod tests { #[test] fn resolve_agent_binary_prefers_override() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let explicit = dir.path().join("bootroot-agent"); write_executable(&explicit); - let resolved = resolve_agent_binary(Some(&explicit), &messages).unwrap(); + // An empty search path is the honest input for a case whose + // whole point is that the override short-circuits the search. + let resolved = + resolve_agent_binary(Some(&explicit), None, Path::new("."), &messages).unwrap(); assert_eq!(resolved, explicit); } #[test] fn resolve_agent_binary_falls_back_to_path() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let path_entry = dir.path().to_path_buf(); let binary = path_entry.join(AGENT_BINARY_NAME); write_executable(&binary); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", path_entry.as_os_str()); - } - let resolved = resolve_agent_binary(None, &messages); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } - - let resolved = resolved.unwrap(); + let resolved = resolve_agent_binary( + None, + Some(path_entry.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap(); assert_eq!(resolved, binary); } #[test] fn resolve_agent_binary_error_names_candidates() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); @@ -684,19 +701,13 @@ mod tests { std::fs::create_dir_all(&path_dir_b).unwrap(); let path_value = std::env::join_paths([&path_dir_a, &path_dir_b]).unwrap(); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", &path_value); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } + let err = resolve_agent_binary( + Some(&missing), + Some(path_value.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap_err(); let rendered = err.to_string(); assert!( @@ -717,7 +728,6 @@ mod tests { #[test] fn resolve_agent_binary_error_lists_empty_path_segment_as_cwd() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); @@ -726,19 +736,15 @@ mod tests { // Leading separator yields an empty segment, which POSIX treats as `.`. let path_value = std::env::join_paths([PathBuf::new(), path_dir.clone()]).unwrap(); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", &path_value); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } + // The production `.` base, because what is pinned here is how an + // empty entry renders in the error. + let err = resolve_agent_binary( + Some(&missing), + Some(path_value.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap_err(); let rendered = err.to_string(); let cwd_candidate = PathBuf::from(".").join(AGENT_BINARY_NAME); @@ -756,7 +762,6 @@ mod tests { #[cfg(unix)] #[test] fn resolve_agent_binary_uses_cwd_for_empty_path_segment() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let agent = dir.path().join(AGENT_BINARY_NAME); @@ -766,48 +771,24 @@ mod tests { // Trailing separator yields an empty segment after the non-empty entry. let path_value = std::env::join_paths([path_dir, PathBuf::new()]).unwrap(); - let original_path = std::env::var_os("PATH"); - let original_cwd = std::env::current_dir().unwrap(); - // SAFETY: Serialized via ENV_LOCK; cwd and PATH restored below. - unsafe { - std::env::set_var("PATH", &path_value); - } - std::env::set_current_dir(dir.path()).unwrap(); - let resolved = resolve_agent_binary(None, &messages); - std::env::set_current_dir(&original_cwd).unwrap(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } - - let resolved = resolved.expect("empty PATH segment should resolve via cwd"); - // Relative candidate; resolution used the cwd we set above. - assert_eq!(resolved, PathBuf::from(".").join(AGENT_BINARY_NAME)); + // The tempdir stands in for the working directory an empty entry + // means, so the resolution the process-wide `chdir` used to set + // up is exercised without moving this process anywhere. + let resolved = + resolve_agent_binary(None, Some(path_value.as_os_str()), dir.path(), &messages) + .expect("empty PATH segment should resolve via the empty-entry base"); + assert_eq!(resolved, agent); assert!(agent.is_file()); } #[test] fn resolve_agent_binary_error_handles_unset_path() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::remove_var("PATH"); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - if let Some(value) = original_path { - std::env::set_var("PATH", value); - } - } + let err = + resolve_agent_binary(Some(&missing), None, Path::new("."), &messages).unwrap_err(); let rendered = err.to_string(); assert!(