From bafe816fc7446536c3f0bf0f134d0adeb9044795 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Thu, 23 Apr 2026 12:50:39 -0300 Subject: [PATCH] =?UTF-8?q?smoke:=20cover=20the=20gaps=20=E2=80=94=20NAT,?= =?UTF-8?q?=20vm=20ports/restart/kill/prune,=20workspace=20variants,=20ssh?= =?UTF-8?q?-config?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of banger's advertised CLI surface vs. what smoke was exercising turned up several gaps where a regression would have shipped silently. New scenarios: - NAT: asserts the per-VM POSTROUTING MASQUERADE rule is installed with --nat (scoped to the guest /32), idempotent across stop/start, and torn down on delete. End-to-end curl tests don't work here because the bridge IP and uplink IP both belong to the host — a guest reaching the uplink lands on host-local loopback whether MASQUERADE is set up or not — so the test pins the iptables rule itself. Skipped if passwordless `sudo iptables` isn't available. - vm ports: sshd :22 must be visible with the .vm endpoint (not localhost, not the raw guest IP — the daemon prefers the DNS record when one exists). - vm restart: dedicated verb, not a stop+start alias. Asserts a fresh boot_id to prove the kernel actually recycled. - vm kill --signal KILL: forceful termination path (distinct from `vm stop`'s graceful Ctrl-Alt-Del). Post-kill state must be 'stopped' (not 'error') and the dm-snapshot must be cleaned up. - vm prune -f: batch delete of non-running VMs while preserving any that are still running. Regression guard for the case where prune could wipe a live session. - workspace prepare --readonly: mode bits on /root/repo/ must drop all write bits. Enforcement is advisory against a root guest, so the test asserts the bits, not EACCES. - workspace prepare --mode full_copy: alternate transfer path (tarred into rootfs, no overlay) still lands the repo contents at /root/repo. - workspace export --base-commit: guest-side commits captured in the patch when the pre-commit SHA is pinned. The feature's whole reason for existing; it had zero coverage. Includes a control assertion that the plain (no --base-commit) export does NOT see the committed file. - ssh-config --install / --uninstall: HOME-isolated to a smoke tempdir so we don't touch the invoking user's ~/.ssh/config. Seeds a pre-existing config to catch any regression where install clobbers instead of appending. Asserts idempotency (second install doesn't duplicate the Include line) and clean round-trip (uninstall leaves the user's own content intact). Coverage deltas from smoke (vs the last run): internal/hostnat 14.1% → 64.1% (+50pp — NAT rule dance) internal/daemon/opstate 56.2% → 87.5% (+31pp) internal/daemon 43.4% → 49.4% (+6pp) internal/cli 36.1% → 40.4% (+4pp) internal/daemon/workspace 64.1% → 67.5% (+3pp) Scenario count: 12 → 21. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/smoke.sh | 290 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) diff --git a/scripts/smoke.sh b/scripts/smoke.sh index 15ca8d0..7bc824b 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -289,6 +289,296 @@ set -e # shellcheck disable=SC2064 trap "rm -rf '$runtime_dir'" EXIT +# --- vm restart (dedicated verb) -------------------------------------- +# `vm restart` is its own verb, not a stop+start composite at the API +# level — it must end up with a freshly booted guest. The assertion is +# a fresh boot ID: /proc/sys/kernel/random/boot_id changes on every +# kernel boot, so post-restart != pre-restart proves the kernel was +# actually recycled rather than the verb no-op'ing. +log 'vm restart: boot_id must change across the verb' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-restart >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-restart >/dev/null || die 'vm restart: create failed' +wait_for_ssh smoke-restart || die 'vm restart: initial ssh never came up' +boot_before="$("$BANGER" vm ssh smoke-restart -- cat /proc/sys/kernel/random/boot_id | tr -d '[:space:]')" +[[ -n "$boot_before" ]] || die 'vm restart: could not read initial boot_id' + +"$BANGER" vm restart smoke-restart >/dev/null || die 'vm restart: verb failed' +wait_for_ssh smoke-restart || die 'vm restart: ssh did not come up after restart' +boot_after="$("$BANGER" vm ssh smoke-restart -- cat /proc/sys/kernel/random/boot_id | tr -d '[:space:]')" +[[ -n "$boot_after" ]] || die 'vm restart: could not read post-restart boot_id' +[[ "$boot_before" != "$boot_after" ]] \ + || die "vm restart: boot_id unchanged ($boot_before); verb didn't actually reboot the guest" + +"$BANGER" vm delete smoke-restart >/dev/null || die 'vm restart: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- vm kill (--signal KILL, forceful path) --------------------------- +# `vm stop` takes the graceful Ctrl-Alt-Del route. `vm kill --signal +# KILL` is the explicit "the guest is wedged, drop it" path. It must +# (a) terminate firecracker, (b) leave the VM record in a stopped +# state (not 'error'), (c) tear down the dm-snapshot + loops so the +# next create/start doesn't trip over leftovers. +log 'vm kill --signal KILL: forceful terminate, state=stopped, no leaked dm device' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-kill >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-kill >/dev/null || die 'vm kill: create failed' +dm_name="$("$BANGER" vm show smoke-kill 2>/dev/null | awk -F'"' '/"dm_dev"|fc-rootfs-/ {for(i=1;i<=NF;i++) if($i~/^fc-rootfs-/) print $i}' | head -1 || true)" +"$BANGER" vm kill --signal KILL smoke-kill >/dev/null || die 'vm kill: verb failed' +show_out="$("$BANGER" vm show smoke-kill)" || die 'vm kill: show after kill failed' +grep -q '"state": "stopped"' <<<"$show_out" || die "vm kill: post-kill state not stopped: $show_out" +if [[ -n "$dm_name" ]]; then + if sudo -n dmsetup ls 2>/dev/null | awk '{print $1}' | grep -qx "$dm_name"; then + die "vm kill: dm device $dm_name still mapped (cleanup didn't run)" + fi +fi +"$BANGER" vm delete smoke-kill >/dev/null || die 'vm kill: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- vm prune (-f) ---------------------------------------------------- +# Create two VMs: one running, one stopped. `vm prune -f` must delete +# the stopped one and leave the running one alone. Skip interactive +# confirmation with -f (smoke has no tty). Regression guard: a bug +# that deleted the running VM would wreck any session the user had. +log 'vm prune -f: removes stopped VMs, preserves running ones' +cleanup_prune() { + "$BANGER" vm delete smoke-prune-running >/dev/null 2>&1 || true + "$BANGER" vm delete smoke-prune-stopped >/dev/null 2>&1 || true +} +# shellcheck disable=SC2064 +trap "cleanup_prune; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-prune-running >/dev/null || die 'vm prune: create running failed' +"$BANGER" vm create --name smoke-prune-stopped >/dev/null || die 'vm prune: create stopped failed' +"$BANGER" vm stop smoke-prune-stopped >/dev/null || die 'vm prune: stop the stopped one failed' + +"$BANGER" vm prune -f >/dev/null || die 'vm prune: verb failed' + +"$BANGER" vm show smoke-prune-running >/dev/null 2>&1 || die 'vm prune: running VM was deleted (regression!)' +if "$BANGER" vm show smoke-prune-stopped >/dev/null 2>&1; then + die 'vm prune: stopped VM survived prune' +fi + +"$BANGER" vm delete smoke-prune-running >/dev/null || die 'vm prune: cleanup delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- vm ports --------------------------------------------------------- +# sshd binds :22 in every guest — it's the minimum promise of a VM. +# If `vm ports` can't see that, the host→guest port visibility pipe +# (vsock-agent on-demand query, daemon aggregation, CLI rendering) is +# broken. Endpoint shape is also asserted: daemon prefers the +# .vm DNS record over the raw guest IP, so we grep for the +# name form. +log 'vm ports: sshd :22 visible from host, endpoint uses the VM DNS name' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-ports >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-ports >/dev/null || die 'vm ports: create failed' +wait_for_ssh smoke-ports || die 'vm ports: ssh did not come up' + +ports_out="$("$BANGER" vm ports smoke-ports 2>&1)" \ + || die "vm ports: verb failed: $ports_out" +grep -q 'smoke-ports.vm:22' <<<"$ports_out" \ + || die "vm ports: expected 'smoke-ports.vm:22' in output; got: $ports_out" +grep -q 'sshd' <<<"$ports_out" \ + || die "vm ports: expected process 'sshd' in output; got: $ports_out" + +"$BANGER" vm delete smoke-ports >/dev/null || die 'vm ports: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- workspace prepare --readonly ------------------------------------- +# --readonly runs `chmod -R a-w` over the workspace. Root in the +# guest bypasses DAC anyway, so this is advisory rather than +# enforced — the point of the flag is tooling contract: "the +# mode bits SAY readonly". Assert that contract: the write bit +# must be cleared on the guest file after --readonly prepare, and +# set without it. A regression where the chmod silently no-op'd +# would leave the bits unchanged. +log 'workspace prepare --readonly: mode bits reflect the request' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-ro >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-ro >/dev/null || die 'workspace ro: create failed' +"$BANGER" vm workspace prepare smoke-ro "$repodir" --readonly >/dev/null \ + || die 'workspace ro: prepare --readonly failed' + +# stat octal mode. a-w clears the 0222 write bits across u/g/o, so +# none of the write bits should be set on the file. +ro_mode="$("$BANGER" vm ssh smoke-ro -- stat -c '%a' /root/repo/smoke-file.txt | tr -d '[:space:]')" +[[ -n "$ro_mode" ]] || die 'workspace ro: could not read mode bits' +case "$ro_mode" in + *[2367]) + die "workspace ro: file still has write bit set after --readonly (mode=$ro_mode)" + ;; +esac + +# Read must still succeed (--readonly means a-w, not a-r). +"$BANGER" vm ssh smoke-ro -- cat /root/repo/smoke-file.txt >/dev/null \ + || die 'workspace ro: read denied — --readonly dropped read perm too' + +"$BANGER" vm delete smoke-ro >/dev/null || die 'workspace ro: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- workspace prepare --mode full_copy ------------------------------- +# Default mode is shallow_overlay. full_copy copies the repo via a +# different transfer path (tar stream into the guest's rootfs with +# no overlay). Smoke asserts it still lands the content at the same +# guest path. +log 'workspace prepare --mode full_copy: alternate transfer path still delivers' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-fc >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-fc >/dev/null || die 'workspace fc: create failed' +"$BANGER" vm workspace prepare smoke-fc "$repodir" --mode full_copy >/dev/null \ + || die 'workspace fc: prepare --mode full_copy failed' +fc_out="$("$BANGER" vm ssh smoke-fc -- cat /root/repo/smoke-file.txt)" \ + || die 'workspace fc: guest read failed' +grep -q 'smoke-workspace-marker' <<<"$fc_out" \ + || die "workspace fc: marker missing in full_copy workspace: $fc_out" + +"$BANGER" vm delete smoke-fc >/dev/null || die 'workspace fc: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- workspace export --base-commit (committed guest delta) ----------- +# Without --base-commit, export diffs the worktree against HEAD — it +# misses commits the worker made inside the guest (because the guest +# HEAD advanced). With --base-commit pinned at the prepare-time SHA, +# those commits land in the patch. This is the happy path the feature +# was added for; zero coverage until now. +log 'workspace export --base-commit: guest-side commits captured in patch' +# shellcheck disable=SC2064 +trap "\"$BANGER\" vm delete smoke-basecommit >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + +"$BANGER" vm create --name smoke-basecommit >/dev/null || die 'export base: create failed' +"$BANGER" vm workspace prepare smoke-basecommit "$repodir" >/dev/null \ + || die 'export base: prepare failed' + +# Capture the prepare-time HEAD from the guest directly (same SHA the +# daemon returns as HeadCommit in the RPC result). +base_sha="$("$BANGER" vm ssh smoke-basecommit -- sh -c 'cd /root/repo && git rev-parse HEAD' | tr -d '[:space:]')" +[[ "${#base_sha}" -eq 40 ]] || die "export base: bad base sha: $base_sha" + +# Make a guest-side commit: new file + git add + git commit. Without +# --base-commit, this commit would be invisible to a HEAD-relative diff. +"$BANGER" vm ssh smoke-basecommit -- sh -c "cd /root/repo && git -c user.email=smoke@smoke -c user.name=smoke checkout -b smoke-branch >/dev/null 2>&1 && echo committed-marker > smoke-committed.txt && git add smoke-committed.txt && git -c user.email=smoke@smoke -c user.name=smoke commit -q -m 'guest side'" \ + || die 'export base: guest-side commit failed' + +# Control: plain export (no --base-commit) must NOT see the committed file. +plain_patch="$runtime_dir/smoke-plain.diff" +"$BANGER" vm workspace export smoke-basecommit --output "$plain_patch" \ + || die 'export base: plain export failed' +if grep -q 'smoke-committed.txt' "$plain_patch"; then + die 'export base: plain export unexpectedly captured the guest-side commit' +fi + +# With --base-commit pinned to the pre-commit SHA, the delta appears. +base_patch="$runtime_dir/smoke-base.diff" +"$BANGER" vm workspace export smoke-basecommit --base-commit "$base_sha" --output "$base_patch" \ + || die 'export base: --base-commit export failed' +[[ -s "$base_patch" ]] || die 'export base: patch file empty' +grep -q 'smoke-committed.txt' "$base_patch" \ + || die "export base: --base-commit patch missing committed marker (head: $(head -c 400 "$base_patch"))" + +"$BANGER" vm delete smoke-basecommit >/dev/null || die 'export base: delete failed' +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + +# --- ssh-config install / uninstall (HOME-isolated) ------------------- +# `banger ssh-config --install` edits ~/.ssh/config. Smoke runs under +# the invoking user, so we isolate by pointing HOME at the smoke XDG +# dir before the commands run (os.UserHomeDir respects $HOME on +# Linux). No daemon / VM involved — pure CLI + filesystem surface, +# exercising the install/status/uninstall code paths end-to-end. +log 'ssh-config --install / --uninstall: idempotent, survives round-trip' +fake_home="$BANGER_SMOKE_XDG_DIR/fake-home" +mkdir -p "$fake_home/.ssh" +# Seed a pre-existing ~/.ssh/config so install must APPEND, not +# replace. A bug that clobbered pre-existing content would nuke the +# user's real config on first run. +printf 'Host myserver\n HostName example.invalid\n' > "$fake_home/.ssh/config" + +( + export HOME="$fake_home" + "$BANGER" ssh-config --install >/dev/null || die 'ssh-config: install failed' + grep -q '^Include ' "$fake_home/.ssh/config" \ + || die "ssh-config: install didn't add Include line to ~/.ssh/config" + grep -q '^Host myserver' "$fake_home/.ssh/config" \ + || die 'ssh-config: install clobbered pre-existing content (!!)' + + # Second install must be idempotent (no duplicate Include lines). + "$BANGER" ssh-config --install >/dev/null || die 'ssh-config: second install failed' + include_count="$(grep -c '^Include .*banger' "$fake_home/.ssh/config")" + [[ "$include_count" == "1" ]] \ + || die "ssh-config: install not idempotent (Include appeared $include_count times)" + + "$BANGER" ssh-config --uninstall >/dev/null || die 'ssh-config: uninstall failed' + if grep -q '^Include .*banger' "$fake_home/.ssh/config"; then + die 'ssh-config: uninstall left the Include line behind' + fi + grep -q '^Host myserver' "$fake_home/.ssh/config" \ + || die 'ssh-config: uninstall nuked user content (!!)' +) + +# --- NAT rule installation (per-VM MASQUERADE) ------------------------ +# `--nat` installs a per-VM iptables POSTROUTING MASQUERADE rule +# scoped to the guest's /32 (see natCapability). End-to-end curl +# tests don't work here because the bridge IP and the host's uplink +# IP both belong to the host — a guest reaching the uplink address +# lands on the host's local loopback whether MASQUERADE is set up +# or not. So assert the rule itself: NAT VM gets a POSTROUTING +# MASQUERADE, non-NAT VM does not. This catches the two most +# plausible regressions (rule never installed; rule not scoped to +# the right VM) without depending on an external reachable host. +log 'NAT: --nat installs a per-VM MASQUERADE rule; no --nat means no rule' +if ! sudo -n iptables -t nat -S POSTROUTING >/dev/null 2>&1; then + log 'NAT: skipping — passwordless sudo iptables unavailable' +else + # shellcheck disable=SC2064 + trap "\"$BANGER\" vm delete smoke-nat >/dev/null 2>&1 || true; \"$BANGER\" vm delete smoke-nocnat >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT + + "$BANGER" vm create --name smoke-nat --nat >/dev/null || die 'NAT: create --nat failed' + "$BANGER" vm create --name smoke-nocnat >/dev/null || die 'NAT: control create failed' + + nat_ip="$("$BANGER" vm show smoke-nat 2>/dev/null | awk -F'"' '/"guest_ip"/ {print $4}')" + ctl_ip="$("$BANGER" vm show smoke-nocnat 2>/dev/null | awk -F'"' '/"guest_ip"/ {print $4}')" + [[ -n "$nat_ip" && -n "$ctl_ip" ]] || die "NAT: couldn't read guest IPs (nat='$nat_ip', ctl='$ctl_ip')" + + postrouting="$(sudo -n iptables -t nat -S POSTROUTING 2>/dev/null || true)" + grep -q -- "-s $nat_ip/32.*-j MASQUERADE" <<<"$postrouting" \ + || die "NAT: --nat VM has no POSTROUTING MASQUERADE rule for $nat_ip; got:"$'\n'"$postrouting" + if grep -q -- "-s $ctl_ip/32.*-j MASQUERADE" <<<"$postrouting"; then + die "NAT: control VM unexpectedly has a MASQUERADE rule for $ctl_ip" + fi + + # Stop + start the --nat VM to exercise the install-is-idempotent + # path (capability runs again on each start; a buggy add-without- + # check would leave two identical rules behind). + "$BANGER" vm stop smoke-nat >/dev/null || die 'NAT: stop --nat VM failed' + "$BANGER" vm start smoke-nat >/dev/null || die 'NAT: restart --nat VM failed' + postrouting="$(sudo -n iptables -t nat -S POSTROUTING 2>/dev/null || true)" + rule_count="$(grep -c -- "-s $nat_ip/32.*-j MASQUERADE" <<<"$postrouting" || true)" + [[ "$rule_count" == "1" ]] \ + || die "NAT: MASQUERADE rule count for $nat_ip = $rule_count after restart, want 1" + + # Delete must tear the rule down — regression guard against leaks. + "$BANGER" vm delete smoke-nat >/dev/null || die 'NAT: delete --nat VM failed' + "$BANGER" vm delete smoke-nocnat >/dev/null || die 'NAT: delete control VM failed' + postrouting="$(sudo -n iptables -t nat -S POSTROUTING 2>/dev/null || true)" + if grep -q -- "-s $nat_ip/32.*-j MASQUERADE" <<<"$postrouting"; then + die "NAT: delete left a MASQUERADE rule behind for $nat_ip" + fi +fi +# shellcheck disable=SC2064 +trap "rm -rf '$runtime_dir'" EXIT + # --- invalid spec rejection + no artifact leak ------------------------ # Tests the negative-path create flow: a blatantly invalid VM spec # must fail before any VM row is persisted. The review cycle flagged