Convert the rotate tests to the injected docker executable (#808)

[sehkone]

Summary

Every PATH-faking test under src/commands/rotate/ now hands production its fake docker through the injection seam the tree already carries, so none of them mutates the process environment. In Rust 2024 env::set_var is unsafe, and the shared Mutex those tests held serialised only the code that took it — a tokio runtime, an HTTP client and an env-filtered tracing subscriber read the environment from threads that hold nothing.

How each file reaches the seam:

• approle.rs — make_ctx takes the executable and sets it on the RotateContext it returns; the eleven converted tests point the whole tree at a fake by that one field.
• helpers.rs and stepca_password.rs — the Path::new("docker") literal they already passed becomes the fake's path.
• infra_cert.rs — the one directly-convertible test calls execute_reload_strategy outright. The three end-to-end tests drive a new run_rotate_with_exec(args, docker, messages) in src/commands/rotate.rs, mirroring the _with_exec pairs in infra.rs. run_rotate is that function with DOCKER_BIN supplied — signature and behaviour unchanged, and RotateArgs gains no field. The three keep their end-to-end coverage rather than being rewritten against a hand-built context.

The promoted fake

infra_cert.rs's private write_self_contained_fake_docker is promoted into rotate::test_support (visibility pub(super), so the init tree is neither reached nor disturbed), the local copy is gone, and both of its defects are fixed:

• Framed, not delimited. Each invocation appends its argument count and then exactly that many arguments, every field NUL-terminated: docker restart c appends 2\0restart\0c\0, docker a '' b appends 3\0a\0\0b\0, and a bare docker appends 0\0. The old printf '%s ' "$@" could not tell ["a b"] from ["a", "b"], and the older shared script truncated on every call so only the last invocation survived. Counting rather than scanning for a boundary means an empty argument needs no escaping and two invocations can never merge. decode_fake_docker_log sits beside the helper and returns one Vec<String> per invocation in call order; every converted assertion compares against it rather than re-parsing the log.
• Legal paths are handled, not rejected. The old copy asserted the log path contained no '. The promoted one POSIX-escapes it (' → '\''), so any path tempfile::tempdir() produces yields a working fake. The escaping and the script assembly work on the path's raw bytes rather than on a Display rendering: a Unix path is an arbitrary NUL-free byte sequence, and rendering one that is not UTF-8 would replace those bytes and aim the fake at a path nothing creates.

write_self_contained_fake_docker_exiting bakes in a non-zero exit code, which is what retires TEST_DOCKER_EXIT_ENV — the one test_support item this leaves without a caller, and therefore the one it deletes. ENV_LOCK, env_lock(), ScopedEnvVar, path_with_prepend, write_fake_docker_script and TEST_DOCKER_ARGS_ENV all stay, with live callers in the init tree.

Assertions

Every test that asserted on the argv still does, as exact equality against a decoded argument list — helpers.rs's two-element restart vector and stepca_password.rs's sixteen-element vector included. The six approle.rs tests that asserted nothing about docker still assert nothing; their fake exists only to keep the real docker off a developer's machine. The three that assert !args_log.exists() each still hand production a fake that would have written that log.

run_rotate_infra_cert_signals_and_verifies_openbao's comment reasoning from truncation is rewritten. With every invocation retained, its restart check now covers the whole run instead of the last call, and its reload check is tightened from contains("kill") && contains("SIGHUP") to exact equality against the whole ["kill", "-s", "SIGHUP", "bootroot-openbao"] record, so a longer log cannot satisfy it on an unrelated line.

Removals

• container_signal_addresses_the_named_container is removed rather than converted. Converted, it would assert the same argv against the same function as container_signal_runs_the_supplied_executable, differing only in the fake this issue removes; the surviving test absorbs its doc comment about addressing the instance's own container.
• The eleven #[allow(clippy::await_holding_lock)] attributes in approle.rs, with their comments, go with the lock whose existence justified them.

No production behaviour changes, no new unsafe, and nothing under src/commands/init/ or init::steps::test_support is touched.

Closes #808

Test plan

• cargo test --bin bootroot passes — 995 tests, 0 failures
• grep -rn "set_var\|remove_var\|env_lock\|ScopedEnvVar\|path_with_prepend" src/commands/rotate returns nothing (was 62 lines)
• grep -n "set_var\|remove_var" src/commands/rotate.rs returns exactly three lines — the set_var in ScopedEnvVar::set and the set_var/remove_var pair in its Drop, retained for the init tree
• grep -rn '"PATH"' src/commands/rotate returns nothing (was nineteen ScopedEnvVar::set("PATH", ...) sites)
• grep -rn "await_holding_lock" src/commands/rotate returns nothing (was eleven)
• grep -rln "env_lock\|ScopedEnvVar\|path_with_prepend" src/commands/init still lists exactly steps.rs, steps/openbao_tls.rs, steps/openbao_transition.rs, steps/stepca_setup.rs and steps/http01_admin_tls.rs
• Unit tests of the promoted helper and decoder, one assertion per property: argument boundaries (["--user", "1000:1000"] vs ["--user 1000:1000"]), three consecutive invocations in call order, empty arguments (["a", "", "b"] vs ["a"] then ["b"]), the degenerate argument-less record, a log path containing both ' and a space, and the baked-in exit code
• A non-UTF-8 log path keeps its bytes: one test asserts the generated script contains the path's own bytes (it runs everywhere, including on filesystems that refuse to create such a name), and one runs the fake end-to-end under a non-UTF-8 directory where the filesystem allows it. Restoring the Display rendering fails the first
• For at least one test per converted file, the expected argv was temporarily corrupted and the assertion failed
• Joining "--user", &user_arg into a single format!("--user {user_arg}") at the production call site fails change_stepca_passphrase_invokes_docker_with_force_and_expected_paths — the encoding did not go lossy
• Making the flow invoke docker fails one of the three approle.rs !args_log.exists() assertions
• run_rotate_infra_cert_signals_and_verifies_openbao's reload-signal assertion fails when the signal is wrong
• cargo clippy --all-targets -- -D warnings is clean
• cargo fmt -- --check --config group_imports=StdExternalCrate is clean
• scripts/preflight/ci/check.sh passes (only the pre-existing allowed rustls-pemfile unmaintained advisory)
• Full CI green, including the Docker E2E matrix — every check on the head commit passes, rotation and the rest of the matrix included. scripts/preflight/ci/e2e-matrix.sh and e2e-extended.sh could not run on this machine — port 8200 is held by unrelated local processes — so CI is where the rotation flows got exercised against the real docker. run-extended has no PR trigger, so it was dispatched by hand and passed; it ran on the commit before the head, which only changed how the fake's script embeds its log path.

[sehkone]

[Reviewer Round 1]

I found one issue that needs a follow-up before this is ready.

• src/commands/rotate.rs:394 converts args_log with args_log.display().to_string() before embedding it in the shell script. On Unix, a Path may contain arbitrary non-NUL bytes; Display replaces invalid UTF-8, so when tempfile::tempdir() is rooted below such a path (for example through TMPDIR), the fake redirects to a different, usually nonexistent, log path and exits before recording argv. This is the same class of legal temp path the issue requires the helper to handle, beyond the apostrophe-and-space case currently tested. Build the script from the raw Unix path bytes while applying single-quote escaping, or avoid embedding the supplied path in shell text; add a non-UTF-8 path test without mutating the process environment.

[sehkone]

[Review Verdict Round 1: NOT_APPROVED]

[sehkone]

[Author Round 1]

One review item, accepted.

args_log.display().to_string() loses non-UTF-8 path bytes — Fixed

Correct, and it is the same defect class as the apostrophe case the helper was already written to handle: a Unix path is an arbitrary NUL-free byte sequence, Display replaces every byte that is not valid UTF-8 with U+FFFD, and a tempdir rooted below such a path — reachable through TMPDIR — left the fake redirecting to a path nothing creates. The shell would fail to open it and the fake would exit before recording argv, so an assertion reading that log would be reading a file nothing ever wrote.

Fixed by building the script from the raw path bytes, which is the first of the two remedies you named:

• write_self_contained_fake_docker_exiting now assembles the script as a Vec<u8> and appends args_log.as_os_str().as_bytes() through the quoter, instead of formatting a String around Display.
• shell_single_quote takes &[u8] and returns Vec<u8>, copying every byte through unchanged and expanding only ' to '\''. The escaping is unchanged in behaviour; it just no longer has a lossy conversion in front of it.

fs::write already takes impl AsRef<[u8]>, so nothing else moved. No production code is touched, and this stays inside the helper the issue promotes.

I did not take the second option — deriving the log path from the script's own $0 — because the script and the log are deliberately separate paths here (several tests put the fake and its log in different directories, and the_fake_docker_handles_a_quoted_log_path puts them on either side of an awkward directory name). Tying one to the other would constrain the callers to make a point about quoting.

Tests

Two, neither mutating the process environment:

• the_fake_docker_script_embeds_the_log_path_verbatim writes a fake for a log path containing 0xFF, reads the generated script back, and asserts it contains the path's own bytes. It asserts on the script text rather than on running it, so it holds everywhere.
• the_fake_docker_handles_a_non_utf8_log_path creates a non-UTF-8 directory, runs the fake end-to-end and decodes the log.

The second needs a filesystem that accepts the name. Linux does; APFS and other UTF-8-enforcing filesystems reject it with EILSEQ, so it returns early there rather than failing on a property that is unobservable on that filesystem. That is why the first exists: it pins the same defect with no filesystem dependency, and it is the one that carries the check on macOS. Restoring args_log.display().to_string() fails it — verified, not assumed.

Verification

• cargo test --bin bootroot — 995 passed, 0 failed
• cargo clippy --all-targets -- -D warnings — clean
• cargo fmt -- --check --config group_imports=StdExternalCrate — clean
• The acceptance greps still hold: nothing in the src/commands/rotate directory names set_var, remove_var, env_lock, ScopedEnvVar, path_with_prepend, "PATH" or await_holding_lock; src/commands/rotate.rs still holds exactly the three retained mutation sites; the same five init files still consume rotate::test_support.

No CHANGELOG.md entry: this is test-harness-only and invisible to anyone running the last release.

The Docker E2E matrix and extended E2E still have to run in CI — port 8200 is held by unrelated processes on this machine — and run-extended needs a manual dispatch, as noted in the description.

[sehkone]

[Reviewer Round 2]

The Round 1 finding is resolved. The self-contained fake now embeds the log path from raw Unix path bytes, preserves the apostrophe escaping, and adds both a byte-preservation regression test and an end-to-end non-UTF-8-path test where the filesystem supports that name.

I found no remaining issues in the updated diff. The tests now inject the executable through the existing seams (including the new private run_rotate_with_exec wrapper for the end-to-end cases), retain the relevant argv and no-invocation assertions, and use a count-framed log format that preserves argument boundaries, empty arguments, and multiple calls. The obsolete environment-lock allowances and rotate-directory environment mutation sites are gone.

Approved.

[sehkone]

[Review Verdict Round 2: APPROVED]

[sehkone]

Suggested squash commit

Title

Inject the docker executable into rotate tests

Body

The tests under `src/commands/rotate/` faked `docker` by writing a
script into a tempdir and prepending it to the process `PATH`. In
Rust 2024 `env::set_var` is `unsafe`, and the shared `Mutex` those
tests held did not discharge that contract: it serialised only the
code that took it, while a tokio runtime, an HTTP client and an
env-filtered `tracing` subscriber read the environment from threads
holding nothing. The tree already carries a seam for this — the
executable is a caller-supplied value from `RotateContext::docker`
down to every spawn — so each test now hands production a fake by
path and mutates nothing process-global.

`run_rotate` built its context itself, leaving the three end-to-end
`infra_cert` tests with no way to name the executable. Rather than
drop that coverage by calling `rotate_infra_certs` against a
hand-built context, `run_rotate_with_exec` takes the executable and
`run_rotate` is that function with `DOCKER_BIN` supplied, mirroring
the `_with_exec` pairs in `infra.rs`. Signature, behaviour and
`RotateArgs` are unchanged.

The self-contained fake `infra_cert.rs` kept privately is promoted
into `rotate::test_support`, which is where its two defects get
fixed. Its space-joined log could not tell `["a b"]` from
`["a", "b"]`, so every invocation now appends its argument count
followed by that many NUL-terminated arguments: framing by count
rather than delimiting means an empty argument needs no escaping and
two invocations can never merge, and a decoder beside the helper
returns them in call order. It also asserted the log path held no
`'`, failing on a path `tempfile::tempdir()` may legitimately
produce; the path is POSIX-quoted instead, byte for byte, since
rendering a non-UTF-8 path through `Display` would aim the fake
somewhere nothing creates.

Baking the exit code into the script retires
`TEST_DOCKER_EXIT_ENV`, the one `test_support` item left without a
caller. `ENV_LOCK`, `ScopedEnvVar`, `path_with_prepend`,
`write_fake_docker_script` and `TEST_DOCKER_ARGS_ENV` stay for the
`init` tree, which separate work converts.

`container_signal_addresses_the_named_container` is removed rather
than converted: it would assert the same argv against the same
function as `container_signal_runs_the_supplied_executable`,
differing only in the fake this change deletes. The eleven
`#[allow(clippy::await_holding_lock)]` attributes in `approle.rs`
go with the lock that justified them.

No production behaviour changes and no new `unsafe`.

Closes #808