diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index 96971fa..eb0829e 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -17,13 +17,27 @@ if TYPE_CHECKING: log = logging.getLogger(__name__) -def get_instance(container: str, database_containers: list[str]) -> str: - """ - Derive a stable instance name from the container name. +def get_instance(container: str, database_containers: list[str]) -> str | None: + """The databases.csv instance a container serves, or None for no database. + + A declared container is its own instance. Every other name is normalised by + stripping a database suffix token, which maps both `-database` from + compose and `_database.1.` from swarm onto the same instance. + + Args: + container: the running container's name. + database_containers: names passed via --database-containers, taken as + declared engines whatever they are called. + + Returns: + The instance name, or None when the name carries no database token: an + application container is not an engine, even when it ships the client + tools that would let a dump command start. """ if container in database_containers: return container - return re.split(r"(_|-)(database|db|postgres)", container)[0] + parts = re.split(r"(_|-)(database|db|postgres)", container) + return parts[0] if len(parts) > 1 else None def fallback_pg_dumpall( @@ -37,6 +51,7 @@ def fallback_pg_dumpall( container, ["pg_dumpall", "-U", username, "-h", "localhost"], interactive=True, + forward_env=["PGPASSWORD"], ), out_file, env={"PGPASSWORD": password}, @@ -62,6 +77,9 @@ def backup_database( Returns True if at least one dump was produced. """ instance_name = get_instance(container, database_containers) + if instance_name is None: + log.debug("Container '%s' carries no database token", container) + return False entries = databases_df[databases_df["instance"] == instance_name] if entries.empty: @@ -133,6 +151,7 @@ def backup_database( "--no-privileges", ], interactive=True, + forward_env=["PGPASSWORD"], ), dump_file, env={"PGPASSWORD": password}, diff --git a/src/baudolo/backup/docker.py b/src/baudolo/backup/docker.py index 5ff62f8..d1c4ccb 100644 --- a/src/baudolo/backup/docker.py +++ b/src/baudolo/backup/docker.py @@ -9,10 +9,35 @@ if TYPE_CHECKING: def docker_exec_argv( - container: str, argv: Sequence[str], *, interactive: bool = False + container: str, + argv: Sequence[str], + *, + interactive: bool = False, + forward_env: Sequence[str] = (), ) -> list[str]: - """The argv that runs *argv* inside *container*.""" - return ["docker", "exec", *(["-i"] if interactive else []), container, *argv] + """The argv that runs *argv* inside *container*. + + Args: + container: the container to run in. + argv: the command, already split. + interactive: keep stdin open, for a command that is fed a dump. + forward_env: names of environment variables to hand to the container. + Passed as bare ``-e NAME``, so docker copies the value out of this + process's own environment; spelling ``-e NAME=value`` instead would + publish a secret in the host's process list. + + Returns: + The argv list. + """ + forwarded = [arg for name in forward_env for arg in ("-e", name)] + return [ + "docker", + "exec", + *(["-i"] if interactive else []), + *forwarded, + container, + *argv, + ] def get_image_info(container: str) -> str: diff --git a/tests/e2e/test_e2e_app_container_ships_client_tools.py b/tests/e2e/test_e2e_app_container_ships_client_tools.py new file mode 100644 index 0000000..2a39799 --- /dev/null +++ b/tests/e2e/test_e2e_app_container_ships_client_tools.py @@ -0,0 +1,163 @@ +"""An application container that ships the engine's client tools. + +This is the shape a dedicated database deploys in: the engine runs as +`-database` while the application itself runs as ``, and neither is +declared through --database-containers, so both names go through the instance +regex. `-database` loses its suffix and lands on the instance `` - +and `` carries no database token at all, so a fallback that returns the +name unchanged lands on that same instance and offers the application container +as a second engine for the same row. + +Discourse is the live example: its application container is named `discourse` +by its own launcher and ships pg_dumpall, so a dump command starts there and +writes a file that looks like a backup and holds none of the data. +""" + +import unittest +from pathlib import Path + +from baudolo.generation import DUMP_SUFFIX, FILES_DIR, SQL_DIR + +from .helpers import ( + POSTGRES_DATA_DIR, + POSTGRES_IMAGE, + backup_path, + backup_run, + cleanup_docker, + create_minimal_compose_dir, + ensure_empty_dir, + latest_version_dir, + require_docker, + run, + unique, + wait_for_postgres, + write_databases_csv, +) + +MARKER = "the-application-volume-holds-files" +PAYLOAD = "shop-payload" + + +class TestE2EAppContainerShipsClientTools(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + require_docker() + # uuid4 hex may begin with "db", which the instance regex would split + # on and turn the application container into a different instance, + # hiding exactly the collision this module is about. + cls.prefix = unique("baudolo-e2e-app-tools").replace("-db", "-xb") + cls.backups_dir = f"/tmp/{cls.prefix}/Backups" + ensure_empty_dir(cls.backups_dir) + cls.compose_dir = create_minimal_compose_dir(f"/tmp/{cls.prefix}") + cls.repo_name = cls.prefix + + cls.engine = f"{cls.prefix}-shop-database" + cls.app = f"{cls.prefix}-shop" + cls.engine_volume = f"{cls.prefix}-shop-database-vol" + cls.app_volume = f"{cls.prefix}-shop-app-vol" + cls.containers = [cls.engine, cls.app] + cls.volumes = [cls.engine_volume, cls.app_volume] + + run(["docker", "volume", "create", cls.engine_volume]) + run(["docker", "volume", "create", cls.app_volume]) + + run( + [ + "docker", + "run", + "-d", + "--name", + cls.engine, + "-e", + "POSTGRES_PASSWORD=shoppw", + "-e", + "POSTGRES_DB=shopdb", + "-e", + "POSTGRES_USER=postgres", + "-v", + f"{cls.engine_volume}:{POSTGRES_DATA_DIR}", + POSTGRES_IMAGE, + ] + ) + + run( + [ + "docker", + "run", + "-d", + "--name", + cls.app, + "--entrypoint", + "sh", + "-v", + f"{cls.app_volume}:/data", + POSTGRES_IMAGE, + "-c", + f"echo '{MARKER}' > /data/marker.txt && sleep 3600", + ] + ) + + wait_for_postgres(cls.engine, user="postgres", timeout_s=90) + run( + [ + "docker", + "exec", + cls.engine, + "sh", + "-lc", + ( + 'psql -U postgres -d shopdb -c "CREATE TABLE orders (id int, ' + f"note text); INSERT INTO orders VALUES (1,'{PAYLOAD}');\"" + ), + ], + check=True, + ) + + cls.databases_csv = f"/tmp/{cls.prefix}/databases.csv" + write_databases_csv( + cls.databases_csv, + [(cls.app, "shopdb", "postgres", "shoppw")], + ) + + backup_run( + backups_dir=cls.backups_dir, + repo_name=cls.repo_name, + compose_dir=cls.compose_dir, + databases_csv=cls.databases_csv, + database_containers=["dummy-db"], + images_no_stop_required=[POSTGRES_IMAGE], + ) + cls.hash, cls.version = latest_version_dir(cls.backups_dir, cls.repo_name) + + @classmethod + def tearDownClass(cls) -> None: + cleanup_docker(containers=cls.containers, volumes=cls.volumes) + + def volume_dir(self, volume: str) -> Path: + return backup_path(self.backups_dir, self.repo_name, self.version, volume) + + def test_the_engine_volume_was_dumped(self) -> None: + dump = self.volume_dir(self.engine_volume) / SQL_DIR / f"shopdb{DUMP_SUFFIX}" + self.assertTrue(dump.is_file(), f"expected a dump at {dump}") + self.assertIn(PAYLOAD, dump.read_text(encoding="utf-8")) + + def test_the_application_volume_produced_no_dump(self) -> None: + """The collision this module exists for: the application container + answers the same instance as the engine and starts a dump of its own.""" + sql_dir = self.volume_dir(self.app_volume) / SQL_DIR + self.assertFalse( + sql_dir.exists(), + f"the application container was dumped: {sorted(sql_dir.iterdir())}" + if sql_dir.exists() + else "", + ) + + def test_the_application_volume_was_backed_up_as_files(self) -> None: + """Refusing the dump must not cost the volume its backup.""" + marker = self.volume_dir(self.app_volume) / FILES_DIR / "marker.txt" + self.assertTrue(marker.is_file(), f"expected a file backup at {marker}") + self.assertIn(MARKER, marker.read_text(encoding="utf-8")) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/e2e/test_e2e_postgres_password_required.py b/tests/e2e/test_e2e_postgres_password_required.py new file mode 100644 index 0000000..adab8bf --- /dev/null +++ b/tests/e2e/test_e2e_postgres_password_required.py @@ -0,0 +1,159 @@ +"""An engine whose loopback auth really demands a password. + +Every other Postgres scenario runs stock postgres:alpine, whose generated +pg_hba grants trust on 127.0.0.1 and ::1 - so `pg_dump -h localhost` never +needs the password and a dump succeeds whether or not baudolo hands one to the +container. This module makes the password mandatory, which is what a dedicated +engine on a real host does. +""" + +import unittest +from pathlib import Path + +from baudolo.generation import CLUSTER_SUFFIX, DUMP_SUFFIX, FILES_DIR, SQL_DIR + +from .helpers import ( + POSTGRES_DATA_DIR, + POSTGRES_IMAGE, + backup_path, + backup_run, + cleanup_docker, + create_minimal_compose_dir, + ensure_empty_dir, + latest_version_dir, + require_docker, + run, + unique, + wait_for_postgres, + write_databases_csv, +) + + +class TestE2EPostgresPasswordRequired(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + require_docker() + cls.prefix = unique("baudolo-e2e-pg-password-required") + cls.backups_dir = f"/tmp/{cls.prefix}/Backups" + ensure_empty_dir(cls.backups_dir) + cls.compose_dir = create_minimal_compose_dir(f"/tmp/{cls.prefix}") + cls.repo_name = cls.prefix + + cls.pg_container = f"{cls.prefix}-pg" + cls.pg_volume = f"{cls.prefix}-pg-vol" + cls.containers = [cls.pg_container] + cls.volumes = [cls.pg_volume] + + run(["docker", "volume", "create", cls.pg_volume]) + run( + [ + "docker", + "run", + "-d", + "--name", + cls.pg_container, + "-e", + "POSTGRES_PASSWORD=pgpw", + "-e", + "POSTGRES_DB=appdb", + "-e", + "POSTGRES_USER=postgres", + # The entrypoint evals this into its initdb call, so the host + # lines of pg_hba demand scram while the local socket stays + # trust - the entrypoint's own init and the seeding below keep + # working, and only a TCP connection needs the password. + "-e", + "POSTGRES_INITDB_ARGS=--auth-host=scram-sha-256", + "-v", + f"{cls.pg_volume}:{POSTGRES_DATA_DIR}", + POSTGRES_IMAGE, + ] + ) + wait_for_postgres(cls.pg_container, user="postgres", timeout_s=90) + + run( + [ + "docker", + "exec", + cls.pg_container, + "sh", + "-lc", + ( + 'psql -U postgres -d appdb -c "CREATE TABLE t (id int primary ' + "key, v text); INSERT INTO t VALUES (1,'ok');\"" + ), + ], + check=True, + ) + + cls.unauthenticated = run( + [ + "docker", + "exec", + cls.pg_container, + "sh", + "-lc", + "pg_dump -U postgres -d appdb -h localhost", + ], + capture=True, + check=False, + ) + + cls.databases_csv = f"/tmp/{cls.prefix}/databases.csv" + write_databases_csv( + cls.databases_csv, + [ + (cls.pg_container, "appdb", "postgres", "pgpw"), + (cls.pg_container, "*", "postgres", "pgpw"), + ], + ) + + backup_run( + backups_dir=cls.backups_dir, + repo_name=cls.repo_name, + compose_dir=cls.compose_dir, + databases_csv=cls.databases_csv, + database_containers=[cls.pg_container], + images_no_stop_required=[POSTGRES_IMAGE], + only_sql=True, + ) + cls.hash, cls.version = latest_version_dir(cls.backups_dir, cls.repo_name) + + @classmethod + def tearDownClass(cls) -> None: + cleanup_docker(containers=cls.containers, volumes=cls.volumes) + + def volume_dir(self) -> Path: + return backup_path( + self.backups_dir, self.repo_name, self.version, self.pg_volume + ) + + def test_a_dump_without_the_password_is_refused_by_the_server(self) -> None: + """Without this the module is vacuous: a pg_hba still saying trust would + let a baudolo that forwards nothing pass just as well.""" + self.assertNotEqual(self.unauthenticated.returncode, 0) + self.assertIn("no password supplied", self.unauthenticated.stderr or "") + + def test_the_configured_database_was_dumped(self) -> None: + dump = self.volume_dir() / SQL_DIR / f"appdb{DUMP_SUFFIX}" + self.assertTrue(dump.is_file(), f"expected a dump at {dump}") + self.assertIn("Dumped by pg_dump", dump.read_text(encoding="utf-8")) + + def test_the_dump_carries_the_payload(self) -> None: + """pg_dump emits table data as COPY ... FROM stdin, so the row reads as + tab-separated values rather than as an INSERT literal.""" + dump = self.volume_dir() / SQL_DIR / f"appdb{DUMP_SUFFIX}" + self.assertIn("COPY public.t (id, v) FROM stdin;", dump.read_text("utf-8")) + self.assertIn("1\tok", dump.read_text(encoding="utf-8")) + + def test_the_cluster_row_was_dumped_too(self) -> None: + cluster = self.volume_dir() / SQL_DIR / f"{self.pg_container}{CLUSTER_SUFFIX}" + self.assertTrue(cluster.is_file(), f"expected a cluster dump at {cluster}") + self.assertIn("CREATE DATABASE", cluster.read_text(encoding="utf-8")) + + def test_only_sql_left_no_file_copy_behind(self) -> None: + self.assertFalse((self.volume_dir() / FILES_DIR).exists()) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/backup/test_docker_exec_env.py b/tests/unit/backup/test_docker_exec_env.py new file mode 100644 index 0000000..15d7077 --- /dev/null +++ b/tests/unit/backup/test_docker_exec_env.py @@ -0,0 +1,61 @@ +"""How a secret reaches the command running inside the container.""" + +from __future__ import annotations + +import unittest + +from baudolo.backup.db import fallback_pg_dumpall +from baudolo.backup.docker import docker_exec_argv + + +class TestForwardEnv(unittest.TestCase): + def test_nothing_is_added_when_no_variable_is_named(self) -> None: + self.assertEqual( + docker_exec_argv("c1", ["true"]), + ["docker", "exec", "c1", "true"], + ) + + def test_a_named_variable_is_forwarded_without_its_value(self) -> None: + """-e NAME=value would publish the secret in the host's process list.""" + argv = docker_exec_argv("c1", ["true"], forward_env=["PGPASSWORD"]) + self.assertEqual(argv, ["docker", "exec", "-e", "PGPASSWORD", "c1", "true"]) + + def test_the_flag_precedes_the_container(self) -> None: + """docker reads options before the container name, arguments after it.""" + argv = docker_exec_argv( + "c1", ["pg_dump", "-U", "u"], interactive=True, forward_env=["PGPASSWORD"] + ) + self.assertLess(argv.index("-e"), argv.index("c1")) + self.assertLess(argv.index("-i"), argv.index("c1")) + self.assertGreater(argv.index("pg_dump"), argv.index("c1")) + + def test_several_variables_each_get_their_own_flag(self) -> None: + argv = docker_exec_argv("c1", ["true"], forward_env=["A", "B"]) + self.assertEqual(argv[:6], ["docker", "exec", "-e", "A", "-e", "B"]) + + +class TestPostgresDumpCarriesThePassword(unittest.TestCase): + def test_the_cluster_dump_forwards_pgpassword(self) -> None: + seen: dict = {} + + def fake(command, out_file, *, env=None): + seen["command"] = command + seen["env"] = env + + import baudolo.backup.db as db + + original = db.execute_to_file + db.execute_to_file = fake + try: + fallback_pg_dumpall("pg", "user", "secret", "/tmp/out.sql") + finally: + db.execute_to_file = original + + self.assertIn("-e", seen["command"]) + self.assertEqual(seen["command"][seen["command"].index("-e") + 1], "PGPASSWORD") + self.assertEqual(seen["env"], {"PGPASSWORD": "secret"}) + self.assertNotIn("secret", seen["command"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/backup/test_get_instance.py b/tests/unit/backup/test_get_instance.py new file mode 100644 index 0000000..8a99371 --- /dev/null +++ b/tests/unit/backup/test_get_instance.py @@ -0,0 +1,67 @@ +"""Which databases.csv instance a container name resolves to. + +The cases are the container names real deployments produce, in both compose +and swarm, so a change to the regex has to state which shape it gives up. +""" + +from __future__ import annotations + +import unittest + +from baudolo.backup.db import get_instance + + +class TestDeclaredContainers(unittest.TestCase): + def test_a_declared_container_is_its_own_instance(self) -> None: + self.assertEqual( + get_instance("postgres-central", ["postgres-central"]), "postgres-central" + ) + + def test_a_declaration_beats_the_regex(self) -> None: + """A declared name is taken whole even when it carries a token the + fallback would otherwise strip.""" + self.assertEqual( + get_instance("shop-database", ["shop-database"]), "shop-database" + ) + + def test_an_undeclared_central_engine_resolves_to_nothing(self) -> None: + """`postgres-central` has no separator before its token, so nothing is + stripped - a central engine has to be declared to be found.""" + self.assertIsNone(get_instance("postgres-central", [])) + + +class TestDedicatedEngines(unittest.TestCase): + def test_compose_names_the_container_with_a_hyphen(self) -> None: + self.assertEqual(get_instance("discourse-database", []), "discourse") + + def test_swarm_names_the_task_with_an_underscore_and_a_slot(self) -> None: + """Swarm suppresses container_name and names the task + _.., which must land on the same instance as + the compose name so one databases.csv serves both modes.""" + self.assertEqual(get_instance("discourse_database.1.k3f9x2", []), "discourse") + + def test_an_explicitly_named_engine_keeps_its_entity(self) -> None: + self.assertEqual(get_instance("bigbluebutton-postgres-1", []), "bigbluebutton") + + def test_the_short_token_is_stripped_too(self) -> None: + self.assertEqual(get_instance("matomo-db", []), "matomo") + + def test_mariadb_uses_the_same_suffix(self) -> None: + self.assertEqual(get_instance("matomo-database", []), "matomo") + + +class TestApplicationContainers(unittest.TestCase): + def test_a_bare_application_name_is_not_a_database(self) -> None: + """Returning the name unchanged here would offer the application as a + second engine for its own dedicated database's instance.""" + self.assertIsNone(get_instance("discourse", [])) + + def test_a_swarm_application_task_is_not_a_database(self) -> None: + self.assertIsNone(get_instance("discourse_discourse.1.k3f9x2", [])) + + def test_an_application_that_merely_starts_with_a_token_is_not_split(self) -> None: + self.assertIsNone(get_instance("dbeaver", [])) + + +if __name__ == "__main__": + unittest.main()