Make the unit tests hermetic: no real iptables, Docker, or shared /tmp

[FAZuH]

Part of the whole-repo test-suite audit (2026-09-30). These tests report green while asserting nothing, which makes every other result in the suite untrustworthy. Not production-code bugs.

What to build

Unit tests that run against fakes the repo already owns, and that cannot interfere with each other or with the host.

Findings

• The host firewall is wired into unit tests. test_app_state() at crates/natmap/src/api.rs:949 builds a real IptablesManager (api.rs:952). Two tests then early-return on error, so they pass without asserting anything: add_dnat_duplicate_is_idempotent (api.rs:1067, if result.is_err() { return; }) and add_mapping_with_target_ip_success (api.rs:1163). crates/natmap/src/client.rs:219 (14 tests) and crates/natmap/src/daemon.rs:1047 do the same. FakeIptables already exists 120 lines away at daemon.rs:828, and test_app_state_with at daemon.rs:1027 already accepts an injected one. make_daemon with three fakes exists at daemon.rs:958 and is used by 24 tests in the same file.
• Real Docker socket plus a discarded error. crates/auto-discover/src/daemon.rs:765,780 and crates/natmap/src/daemon.rs:1054,1067 build real clients (DockerClient, ConsulClient::from_env, NatmapClient::default_socket), run them with let _ = ....await discarding the error, then assert logs_contain("container.id=123456789012") — the literal the test itself passed in. They pass whether or not Docker exists. natmap/daemon.rs:1072 additionally .unwrap()s Docker::connect_with_local_defaults(), so it panics on any box without Docker.
• A test that asserts nothing at all. tests/auto_discover/recovery.rs:476 large_config_many_services builds a shell script into let _script and returns. No run(), no assertion. It also duplicates registration.rs:391 concurrent_starts_all_registered, which does run.
• A silently skipped Docker test. tests/natmap_docker.rs:189 does which ip6tables || (echo "SKIP: ..." && exit 0), so the test passes on any host without ip6tables and says nothing.
• An assertion helper that cannot fail. tests/auto_discover/mod.rs:101 assert_pass checks output.contains("PASS"), but run() at mod.rs:44 already panics on non-zero exit and every script reaches its echo "PASS" only after its exit 1 checks. It is a tautology in 46 of 47 uses. Keep the helper only for the handful of tests that can genuinely soft-skip, and make those report SKIP visibly in cargo test output.
• Non-deterministic port allocation. crates/lab-lib/src/port.rs:255,267,285 use "127.0.0.1:0", which asks the OS for any port, so each call allocates a different random one. Bind port 0, read the real port back, assert on that.
• Overlapping hardcoded port bands. port.rs:219 uses 21000 + t*40 + i and crates/natmap/src/daemon.rs:952 uses 21000 + (pid % 400) * 24. They collide.
• Shared mutable state across tests. crates/natmap/src/api.rs:955,959 and daemon.rs:1017,1036 hardcode /tmp/natmap-test-state.json and /tmp/natmap.sock for about 20 tests. client.rs:226 correctly overrides both with a tempfile::TempDir; api.rs:949 does not.

Acceptance criteria

• No test in a #[cfg(test)] mod tests constructs IptablesManager, connects to Docker, or reaches Consul over the network
• Every test that can fail has at least one assertion that can fail; no early return on the subject under test
• cargo test -p lab-ops_natmap --lib and cargo test -p lab-ops_auto-discover --lib pass on a host with no Docker daemon and no iptables write permission
• No test depends on a fixed path under /tmp
• No test binds port 0 without reading back the allocated port
• The soft-skipping ip6tables test reports SKIP in cargo test output rather than passing silently

Blocked by

• Collapse the Config/Request type pairs in natmap models #42

Per the test-writing guidelines: a flaky test is worse than no test, because it erodes trust in the whole suite until failures are ignored. Do not add #[ignore] as a way out; use the existing fakes.

[FAZuH]

Landed on v0.2 as 7758ebe, merged ad6dfe9. Closed by hand: a closing keyword only fires from commits reaching the default branch.

The criterion is met, and it is demonstrated rather than asserted

A shim directory holding logging stubs for iptables, ip6tables, ip, docker and consul was placed first on PATH and the three lib suites run under it:

suite	result
lab-ops_natmap --lib	ok. 141 passed; 0 failed
lab-ops_auto-discover --lib	ok. 80 passed; 0 failed
lab-ops_lab-lib --lib	ok. 14 passed; 0 failed

Host binaries reached: 9 ip, and nothing else. Zero iptables, zero ip6tables, zero docker, zero consul.

For contrast, the full suite reaches docker 160 times, consul 44, ip 117 and iptables 3 — all from the integration suites that are supposed to.

Also run with DOCKER_HOST=unix:///nonexistent.sock: all three pass. Before this change handle_docker_event_span_has_container_id panicked there, because it unwrapped Docker::connect_with_local_defaults().

Two early-returning tests, and the reason the return existed

add_dnat_duplicate_is_idempotent bailed on any error. Removing the bail exposed why: add_dnat really binds the ext port, and 1.2.3.4:80 needs root, so the test was green on this host by giving up. It now uses 127.0.0.1 with test_port() and asserts both responses equal, one state entry, and that the fake recorded exactly one install_dnat — which is what idempotent means.

Four tests that were discarding a real error

handle_container_start_span_fields and handle_container_die_logs_deregister built a real DockerClient and ConsulClient, ran with let _ = ....await, and asserted on a log line containing the literal the test had just passed in. They pass whether or not Docker exists. Both now use the file's own make_daemon with fakes and assert result.is_ok(), which had been swallowing a real error on every run.

The tautology

assert_pass checked output.contains("PASS"), but run() already panics on non-zero exit and every script reaches its echo "PASS" only after its exit 1 checks. A tautology in all 44 of its 44 uses. The marker check moved into run(), the helper and its 44 call sites are gone, and each test is one run(&script); line still carrying two can-fail assertions. Fewer lines, strictly stronger, nothing left vacuous.

The ip6tables test's which ip6tables || (echo SKIP && exit 0) now fails loudly if it skips. libtest has no dynamic skip and the repo runs no --nocapture, so a printed SKIP would still be invisible. This is the one assertion strengthened past "green on a capable host"; argued with it if you disagree.

Ports and shared state

The two hardcoded bands no longer collide: lab-lib is 12000 + (pid%400)*8 reaching at most 17751, natmap is 21000 + (pid%400)*24 reaching at most 30600 — disjoint, both below EPHEMERAL_PORT_START. Residual: natmap's band is 24 wide while the binary asks for about 30, so it can spill into the next pid's band. Pre-existing, bites only two concurrent runs.

/tmp/natmap-test-state.json and /tmp/natmap.sock, shared by about 20 tests, are gone; test_app_state_with takes a &TempDir. The /tmp paths inside tests/auto_discover/mod.rs are inside the container and are correct.

Verification

./dev.sh test: 427 passed, 0 failed, 10 targets, exit 0, loadavg 0.94/0.91 before and 3.48/1.63 after. Unchanged from the pre-ticket 427, because no test was removed: large_config_many_services was already fixed by an earlier ticket (the cited recovery.rs:476 no longer exists; the file is 308 lines and the test does run), and assert_pass was a helper rather than a test.

No scenario was left uncovered and no test was deleted or weakened.

Three design findings, deliberately not fixed here

1. PolicyRouteManager is still real in AppState — the source of all 9 ip calls. The test passes only because non-root fails and the test swallows it with Err(_) => {}. Same class as the iptables finding; the fix is a PolicyRoute trait plus a fake, which is production code. Belongs with One ensure primitive per rule kind in the natmap daemon #28.
2. handle_docker_event(&self, event, docker: &Docker) takes a concrete bollard type. auto-discover has a DockerOps trait; natmap has no equivalent, which is why a plain file standing in for a socket was needed.
3. PortAllocator::allocate returns Result<()>, so a caller given port 0 can never learn what the OS assigned. That is the root reason the port.rs tests cannot literally "bind 0, read back" as the ticket asked.