refactor(backup)!: run argv lists, never a shell

The backup built command strings and handed them to shell=True, so four interpolated values per dump - user, password, container, database - were each a way out of the command. validate_database covered one of them since the previous commit; now there is nothing to cover: every command is an argv list, and a value can only ever be an argument.

execute_to_file absorbs the atomic dump write. The shell redirect into <file>.tmp and the separate mv process become a Python file handle and os.replace, and a failing dump deletes its partial file instead of leaving it. PGPASSWORD moves out of the command string into the child's environment, where a process listing does not show it.

docker exec is built in one place, docker_exec_argv; db.py's three hand-built copies and the probe use it. The dead docker_volume_exists goes - never called, and the restore side owns the living twin. The rsync quoting in --link-dest falls away: inside an argv it would have become part of the path.

The snapshot module's injected runner changes type with it, which the three e2e drivers implement - the first conversion missed them, btrfs ran with no arguments, and the e2e caught it. Marked breaking for that contract: any external runner injected into volume_snapshot must now accept a list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-17 16:30:25 +02:00
parent 03013b6c76
commit 1d86277a94
13 changed files with 244 additions and 154 deletions

View File

@@ -7,7 +7,10 @@ import re
import pandas
from .shell import BackupException, execute_shell_command
from baudolo.databases import CLUSTER_ROW, validate_database
from .docker import docker_exec_argv
from .shell import BackupException, execute_to_file
log = logging.getLogger(__name__)
@@ -21,47 +24,21 @@ def get_instance(container: str, database_containers: list[str]) -> str:
return re.split(r"(_|-)(database|db|postgres)", container)[0]
def _validate_database_value(value: str | None, *, instance: str) -> str:
"""
Enforce explicit database semantics:
- "*" => dump ALL databases (cluster dump for Postgres)
- "<name>" => dump exactly this database
- "" => invalid configuration (would previously result in NaN / nan.backup.sql)
"""
v = (value or "").strip()
if v == "":
raise ValueError(
f"Invalid databases.csv entry for instance '{instance}': "
"column 'database' must be '*' or a concrete database name (not empty)."
)
return v
def _atomic_write_cmd(cmd: str, out_file: str) -> None:
"""
Write dump output atomically:
- write to <file>.tmp
- rename to <file> only on success
This prevents empty or partial dump files from being treated as valid backups.
"""
tmp = f"{out_file}.tmp"
execute_shell_command(f"{cmd} > {tmp}")
execute_shell_command(f"mv {tmp} {out_file}")
def fallback_pg_dumpall(
container: str, username: str, password: str, out_file: str
) -> None:
"""
Perform a full Postgres cluster dump using pg_dumpall.
"""
cmd = (
f"PGPASSWORD={password} docker exec -i {container} "
f"pg_dumpall -U {username} -h localhost"
execute_to_file(
docker_exec_argv(
container,
["pg_dumpall", "-U", username, "-h", "localhost"],
interactive=True,
),
out_file,
env={"PGPASSWORD": password},
)
_atomic_write_cmd(cmd, out_file)
def backup_database(
@@ -99,13 +76,13 @@ def backup_database(
user = (getattr(row, "username", "") or "").strip()
password = (getattr(row, "password", "") or "").strip()
db_value = _validate_database_value(raw_db, instance=instance_name)
db_value = validate_database(raw_db, instance=instance_name)
if db_value == "*":
if db_value == CLUSTER_ROW:
if db_type != "postgres":
raise ValueError(
f"databases.csv entry for instance '{instance_name}': "
"'*' is currently only supported for Postgres."
f"'{CLUSTER_ROW}' is currently only supported for Postgres."
)
cluster_file = os.path.join(out_dir, f"{instance_name}.cluster.backup.sql")
@@ -118,23 +95,46 @@ def backup_database(
if db_type == "mariadb":
# Force TCP so auth matches '<user>'@'%' instead of socket -> 'localhost'.
cmd = (
f"docker exec {container} {dump_tool} "
f"-h 127.0.0.1 --protocol=tcp "
f"-u {user} -p{password} {db_name}"
execute_to_file(
docker_exec_argv(
container,
[
dump_tool,
"-h",
"127.0.0.1",
"--protocol=tcp",
"-u",
user,
f"-p{password}",
db_name,
],
),
dump_file,
)
_atomic_write_cmd(cmd, dump_file)
produced = True
continue
if db_type == "postgres":
try:
cmd = (
f"PGPASSWORD={password} docker exec -i {container} "
f"pg_dump -U {user} -d {db_name} -h localhost "
f"--no-owner --no-privileges"
execute_to_file(
docker_exec_argv(
container,
[
"pg_dump",
"-U",
user,
"-d",
db_name,
"-h",
"localhost",
"--no-owner",
"--no-privileges",
],
interactive=True,
),
dump_file,
env={"PGPASSWORD": password},
)
_atomic_write_cmd(cmd, dump_file)
produced = True
except BackupException as e:
raise BackupException(

View File

@@ -1,18 +1,27 @@
from __future__ import annotations
from collections.abc import Sequence
from .shell import BackupException, execute_shell_command
def docker_exec_argv(
container: str, argv: Sequence[str], *, interactive: bool = False
) -> list[str]:
"""The argv that runs *argv* inside *container*."""
return ["docker", "exec", *(["-i"] if interactive else []), container, *argv]
def get_image_info(container: str) -> str:
return execute_shell_command(
f"docker inspect --format '{{{{.Config.Image}}}}' {container}"
["docker", "inspect", "--format", "{{.Config.Image}}", container]
)[0]
def image_id(container: str) -> str:
"""The container's image ID, identical for every replica of one image."""
return execute_shell_command(
f"docker inspect --format '{{{{.Image}}}}' {container}"
["docker", "inspect", "--format", "{{.Image}}", container]
)[0].strip()
@@ -24,19 +33,26 @@ def has_tool(container: str, tool: str) -> bool:
tool it ships.
"""
try:
execute_shell_command(f"docker exec {container} {tool} --version")
execute_shell_command(docker_exec_argv(container, [tool, "--version"]))
except BackupException:
return False
return True
def docker_volume_names() -> list[str]:
return execute_shell_command("docker volume ls --format '{{.Name}}'")
return execute_shell_command(["docker", "volume", "ls", "--format", "{{.Name}}"])
def containers_using_volume(volume_name: str) -> list[str]:
return execute_shell_command(
f"docker ps --filter volume=\"{volume_name}\" --format '{{{{.Names}}}}'"
[
"docker",
"ps",
"--filter",
f"volume={volume_name}",
"--format",
"{{.Names}}",
]
)
@@ -50,12 +66,25 @@ def is_swarm_task(container: str) -> bool:
keeps failing the run loudly instead of silently skipping the stop."""
try:
out = execute_shell_command(
"docker inspect --format "
f"'{{{{index .Config.Labels \"com.docker.swarm.task.id\"}}}}' {container}"
[
"docker",
"inspect",
"--format",
'{{index .Config.Labels "com.docker.swarm.task.id"}}',
container,
]
)
except BackupException:
still_listed = execute_shell_command(
f"docker ps -a --filter name=^{container}$ --format '{{{{.Names}}}}'"
[
"docker",
"ps",
"-a",
"--filter",
f"name=^{container}$",
"--format",
"{{.Names}}",
]
)
if still_listed and still_listed[0].strip():
raise
@@ -82,17 +111,5 @@ def change_containers_status(containers: list[str], status: str) -> None:
if not containers:
print(f"No containers to {status}.", flush=True)
return
names = " ".join(containers)
print(f"{status.capitalize()} containers: {names}...", flush=True)
execute_shell_command(f"docker {status} {names}")
def docker_volume_exists(volume: str) -> bool:
# Avoid throwing exceptions for exists checks.
try:
execute_shell_command(
f"docker volume inspect {volume} >/dev/null 2>&1 && echo OK"
)
return True
except BackupException:
return False
print(f"{status.capitalize()} containers: {' '.join(containers)}...", flush=True)
execute_shell_command(["docker", status, *containers])

View File

@@ -11,7 +11,7 @@ from .shell import BackupException, execute_shell_command
def get_machine_id() -> str:
return execute_shell_command("sha256sum /etc/machine-id")[0][0:64]
return execute_shell_command(["sha256sum", "/etc/machine-id"])[0][0:64]
def stamp_directory(version_dir: str) -> None:

View File

@@ -1,26 +1,71 @@
"""Running external commands without a shell.
Every command is an argv list. A database name, a password or a container name
therefore cannot close a quote and start a second command, which a formatted
string handed to ``shell=True`` allowed.
"""
from __future__ import annotations
import os
import subprocess
from collections.abc import Mapping, Sequence
class BackupException(Exception):
"""Generic exception for backup errors."""
def execute_shell_command(command: str) -> list[str]:
"""Execute a shell command and return its output lines."""
print(command, flush=True)
def _child_env(env: Mapping[str, str] | None) -> dict[str, str] | None:
return None if env is None else {**os.environ, **env}
def _fail(command: Sequence[str], returncode: int, out: bytes, err: bytes) -> None:
raise BackupException(
f"Error in command: {' '.join(command)}\n"
f"Output: {out}\nError: {err}\n"
f"Exit code: {returncode}"
)
def execute_shell_command(
command: Sequence[str], *, env: Mapping[str, str] | None = None
) -> list[str]:
"""Run *command* and return its stdout lines.
Args:
command: argv, the program first.
env: variables added to the child's environment, for values that must
not appear in the argv of a process listing.
"""
command = list(command)
print(" ".join(command), flush=True)
process = subprocess.Popen(
[command],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
shell=True,
command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_child_env(env)
)
out, err = process.communicate()
if process.returncode != 0:
raise BackupException(
f"Error in command: {command}\n"
f"Output: {out}\nError: {err}\n"
f"Exit code: {process.returncode}"
)
_fail(command, process.returncode, out, err)
return [line.decode("utf-8") for line in out.splitlines()]
def execute_to_file(
command: Sequence[str], out_file: str, *, env: Mapping[str, str] | None = None
) -> None:
"""Run *command*, writing its stdout to *out_file* only once it succeeded.
The output goes to a sibling temporary file first, so a partial or empty
stream from a failing dump never takes the place of a valid backup.
"""
command = list(command)
print(" ".join(command), flush=True)
tmp = f"{out_file}.tmp"
with open(tmp, "wb") as handle:
process = subprocess.Popen(
command, stdout=handle, stderr=subprocess.PIPE, env=_child_env(env)
)
_, err = process.communicate()
if process.returncode != 0:
os.unlink(tmp)
_fail(command, process.returncode, b"", err)
os.replace(tmp, out_file)

View File

@@ -48,23 +48,27 @@ def _resolver(subject: str, root: str) -> Callable[[str], str]:
return resolve
def _btrfs(subject: str, name: str, run: Callable[[str], list[str]]) -> tuple[str, str]:
def _btrfs(
subject: str, name: str, run: Callable[[list[str]], list[str]]
) -> tuple[str, list[str]]:
# The snapshot goes inside the subject, never beside it: the kernel rejects
# a snapshot whose destination is on another filesystem, which is exactly
# what the parent directory is when the subject is a mountpoint of its own.
target = os.path.join(os.path.abspath(subject), f".{name}")
run(f"btrfs subvolume snapshot -r {subject} {target}")
return target, f"btrfs subvolume delete {target}"
run(["btrfs", "subvolume", "snapshot", "-r", subject, target])
return target, ["btrfs", "subvolume", "delete", target]
def _zfs(subject: str, name: str, run: Callable[[str], list[str]]) -> tuple[str, str]:
output = run(f"zfs list -H -o name {subject}")
def _zfs(
subject: str, name: str, run: Callable[[list[str]], list[str]]
) -> tuple[str, list[str]]:
output = run(["zfs", "list", "-H", "-o", "name", subject])
dataset = (output[0] if output else "").strip()
if not dataset:
raise SnapshotError(f"no zfs dataset is mounted at {subject}")
run(f"zfs snapshot {dataset}@{name}")
run(["zfs", "snapshot", f"{dataset}@{name}"])
root = os.path.join(subject, ".zfs", "snapshot", name)
return root, f"zfs destroy {dataset}@{name}"
return root, ["zfs", "destroy", f"{dataset}@{name}"]
_CREATE = {"btrfs": _btrfs, "zfs": _zfs}
@@ -129,7 +133,7 @@ def volume_snapshot(
kind: str,
subject: str,
tag: str,
run: Callable[[str], list[str]] = execute_shell_command,
run: Callable[[list[str]], list[str]] = execute_shell_command,
) -> Iterator[Callable[[str], str]]:
"""Yield a resolver mapping a path under ``subject`` into a snapshot of it.

View File

@@ -30,7 +30,7 @@ class Backing:
def inspect_backing(volume_name: str) -> Backing:
reported = execute_shell_command(
f"docker volume inspect --format '{{{{json .}}}}' {volume_name}"
["docker", "volume", "inspect", "--format", "{{json .}}", volume_name]
)[0]
data = json.loads(reported)
return Backing(
@@ -73,13 +73,12 @@ def backup_volume(
pathlib.Path(dest).mkdir(parents=True, exist_ok=True)
last = get_last_backup_dir(versions_dir, volume_name, dest)
link_dest = f"--link-dest='{last}'" if last else ""
verify = "--checksum " if authoritative else ""
cmd = (
f"rsync -aP --no-D --delete --delete-excluded "
f"{verify}{link_dest} {source} {dest}"
)
cmd = ["rsync", "-aP", "--no-D", "--delete", "--delete-excluded"]
if authoritative:
cmd.append("--checksum")
if last:
cmd.append(f"--link-dest={last}")
cmd += [source, dest]
try:
execute_shell_command(cmd)

View File

@@ -23,13 +23,11 @@ from baudolo.backup.volume import Backing
SUBJECT = sys.argv[1]
def shell(command: str) -> list[str]:
proc = subprocess.run(
command, shell=True, capture_output=True, text=True, check=False
)
def shell(command: list[str]) -> list[str]:
proc = subprocess.run(command, capture_output=True, text=True, check=False)
if proc.returncode != 0:
raise SnapshotError(
f"{command} exited {proc.returncode}: {proc.stderr.strip()}"
f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}"
)
return proc.stdout.splitlines()
@@ -50,7 +48,7 @@ def volume(name: str, payload: str) -> Path:
plain = volume("plain", "plain-payload")
own = Path(SUBJECT) / "volumes" / "own" / "_data"
own.mkdir(parents=True, exist_ok=True)
shell(f"mount -t tmpfs tmpfs {own}")
shell(["mount", "-t", "tmpfs", "tmpfs", own])
(own / "state").write_text("own-payload")
check("a plain volume is captured", unsnapshotted(Backing(str(plain)), SUBJECT) is None)

View File

@@ -22,13 +22,11 @@ VERSIONS = "/backups"
GENERATION = f"{VERSIONS}/20260731"
def shell(command: str) -> list[str]:
proc = subprocess.run(
command, shell=True, capture_output=True, text=True, check=False
)
def shell(command: list[str]) -> list[str]:
proc = subprocess.run(command, capture_output=True, text=True, check=False)
if proc.returncode != 0:
raise SnapshotError(
f"{command} exited {proc.returncode}: {proc.stderr.strip()}"
f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}"
)
return proc.stdout.splitlines()

View File

@@ -19,13 +19,11 @@ SUBJECT = sys.argv[2]
EXPECT = sys.argv[3]
def shell(command: str) -> list[str]:
proc = subprocess.run(
command, shell=True, capture_output=True, text=True, check=False
)
def shell(command: list[str]) -> list[str]:
proc = subprocess.run(command, capture_output=True, text=True, check=False)
if proc.returncode != 0:
raise SnapshotError(
f"{command} exited {proc.returncode}: {proc.stderr.strip()}"
f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}"
)
return proc.stdout.splitlines()

View File

@@ -13,16 +13,16 @@ def _df(rows):
)
def _capture_commands(*, db_type, rows, container, dump_tool="mariadb-dump"):
def _capture_dumps(*, db_type, rows, container, dump_tool="mariadb-dump"):
"""Every (argv, env) the dump path would have run."""
captured = []
def _capture(cmd):
captured.append(cmd)
return []
def _capture(command, out_file, *, env=None):
captured.append((list(command), env))
with (
tempfile.TemporaryDirectory() as td,
patch.object(db_mod, "execute_shell_command", side_effect=_capture),
patch.object(db_mod, "execute_to_file", side_effect=_capture),
):
db_mod.backup_database(
container=container,
@@ -41,45 +41,64 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase):
# the connection is auth-matched against '%' instead of socket->localhost.
def test_mariadb_dump_forces_tcp_loopback(self):
captured = _capture_commands(
captured = _capture_dumps(
db_type="mariadb",
rows=[("mariadb", "appdb", "appuser", "s3cret")],
container="mariadb",
)
dump_cmds = [c for c in captured if "mariadb-dump" in c]
self.assertEqual(
len(dump_cmds), 1, f"expected one dump command, got: {captured}"
)
self.assertEqual(len(captured), 1, f"expected one dump, got: {captured}")
cmd = dump_cmds[0]
self.assertIn("-h 127.0.0.1", cmd)
self.assertIn("--protocol=tcp", cmd)
self.assertIn("-u appuser", cmd)
self.assertIn("-ps3cret", cmd)
self.assertIn(" appdb", cmd)
argv, env = captured[0]
self.assertEqual(argv[:3], ["docker", "exec", "mariadb"])
self.assertIn("--protocol=tcp", argv)
self.assertEqual(argv[argv.index("-h") + 1], "127.0.0.1")
self.assertEqual(argv[argv.index("-u") + 1], "appuser")
self.assertIn("-ps3cret", argv)
self.assertEqual(argv[-1], "appdb")
self.assertIsNone(env)
def test_the_probed_client_is_the_one_invoked(self):
captured = _capture_commands(
captured = _capture_dumps(
db_type="mariadb",
rows=[("mariadb", "appdb", "appuser", "s3cret")],
container="mariadb",
dump_tool="mysqldump",
)
dump_cmds = [c for c in captured if "mysqldump" in c]
self.assertEqual(
len(dump_cmds), 1, f"expected one dump command, got: {captured}"
)
self.assertNotIn("mariadb-dump", dump_cmds[0])
argv, _env = captured[0]
self.assertIn("mysqldump", argv)
self.assertNotIn("mariadb-dump", argv)
def test_postgres_dump_unaffected(self):
captured = _capture_commands(
captured = _capture_dumps(
db_type="postgres",
rows=[("pg", "appdb", "appuser", "s3cret")],
container="pg",
)
dump_cmds = [c for c in captured if "pg_dump" in c and "pg_dumpall" not in c]
self.assertEqual(len(dump_cmds), 1)
self.assertNotIn("--protocol=tcp", dump_cmds[0])
argv, _env = captured[0]
self.assertIn("pg_dump", argv)
self.assertNotIn("--protocol=tcp", argv)
def test_the_password_travels_in_the_environment_not_the_argv(self):
"""A process listing shows argv; PGPASSWORD must not be in it."""
captured = _capture_dumps(
db_type="postgres",
rows=[("pg", "appdb", "appuser", "s3cret")],
container="pg",
)
argv, env = captured[0]
self.assertEqual(env, {"PGPASSWORD": "s3cret"})
self.assertNotIn("s3cret", argv)
class TestNoShellReachesTheDump(unittest.TestCase):
def test_a_hostile_database_name_never_reaches_a_command(self):
"""validate_database refuses it, so no argv is built at all."""
with self.assertRaises(ValueError):
_capture_dumps(
db_type="postgres",
rows=[("pg", "app;rm -rf /", "appuser", "s3cret")],
container="pg",
)
if __name__ == "__main__":

View File

@@ -35,7 +35,9 @@ class TestHasTool(unittest.TestCase):
with patch.object(docker_mod, "execute_shell_command", side_effect=_capture):
docker_mod.has_tool("c1", "pg_dumpall")
self.assertEqual(captured, ["docker exec c1 pg_dumpall --version"])
self.assertEqual(
captured, [["docker", "exec", "c1", "pg_dumpall", "--version"]]
)
if __name__ == "__main__":

View File

@@ -10,13 +10,13 @@ from baudolo.backup.snapshot import SnapshotError, volume_snapshot
class Runner:
def __init__(self, replies: dict[str, list[str]] | None = None) -> None:
self.calls: list[str] = []
self.calls: list[list[str]] = []
self.replies = replies or {}
def __call__(self, command: str) -> list[str]:
self.calls.append(command)
def __call__(self, command: list[str]) -> list[str]:
self.calls.append(list(command))
for prefix, reply in self.replies.items():
if command.startswith(prefix):
if " ".join(command).startswith(prefix):
return reply
return []
@@ -28,7 +28,14 @@ class TestBtrfs(unittest.TestCase):
pass
self.assertEqual(
run.calls[0],
"btrfs subvolume snapshot -r /var/lib/docker /var/lib/docker/.baudolo-20260731",
[
"btrfs",
"subvolume",
"snapshot",
"-r",
"/var/lib/docker",
"/var/lib/docker/.baudolo-20260731",
],
)
def test_it_removes_the_snapshot_afterwards(self) -> None:
@@ -36,7 +43,8 @@ class TestBtrfs(unittest.TestCase):
with volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=run):
pass
self.assertEqual(
run.calls[-1], "btrfs subvolume delete /var/lib/docker/.baudolo-20260731"
run.calls[-1],
["btrfs", "subvolume", "delete", "/var/lib/docker/.baudolo-20260731"],
)
def test_it_maps_a_volume_path_into_the_snapshot(self) -> None:
@@ -66,7 +74,7 @@ class TestBtrfs(unittest.TestCase):
volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=run),
):
raise ZeroDivisionError
self.assertTrue(run.calls[-1].startswith("btrfs subvolume delete"))
self.assertEqual(run.calls[-1][:3], ["btrfs", "subvolume", "delete"])
class TestZfs(unittest.TestCase):
@@ -77,13 +85,15 @@ class TestZfs(unittest.TestCase):
run = self._run()
with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run):
pass
self.assertIn("zfs snapshot tank/docker@baudolo-20260731", run.calls)
self.assertIn(["zfs", "snapshot", "tank/docker@baudolo-20260731"], run.calls)
def test_it_destroys_the_snapshot_afterwards(self) -> None:
run = self._run()
with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run):
pass
self.assertEqual(run.calls[-1], "zfs destroy tank/docker@baudolo-20260731")
self.assertEqual(
run.calls[-1], ["zfs", "destroy", "tank/docker@baudolo-20260731"]
)
def test_it_maps_a_volume_path_through_the_dot_zfs_directory(self) -> None:
run = self._run()
@@ -131,8 +141,8 @@ class TestRejections(unittest.TestCase):
class Busy(Runner):
def __call__(self, command: str) -> list[str]:
if command.startswith("btrfs subvolume delete"):
def __call__(self, command: list[str]) -> list[str]:
if command[:3] == ["btrfs", "subvolume", "delete"]:
raise BackupException("target is busy")
return super().__call__(command)

View File

@@ -48,7 +48,7 @@ class TestBackupVolume(unittest.TestCase):
def test_it_keeps_no_twin_of_what_the_second_pass_replaces(self) -> None:
command = self.copy(authoritative=True)
self.assertIn("rsync -aP ", command)
self.assertEqual(command[:2], ["rsync", "-aP"])
self.assertNotIn("--backup", command)
def test_it_creates_the_destination(self) -> None: