From c4f723fc6f47f404cfc941dad823f1d57eeb32bb Mon Sep 17 00:00:00 2001 From: Philippe Parage <69145356+pparage@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:46:21 +0200 Subject: [PATCH] fix(deploy): report the real error, and undo a seed whose commit failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more from Codex review of the atomicity fix. Foreign keys are enforced at DB level (PRAGMA foreign_keys=ON), so an unknown project_id or target_host_id reached the flush and raised the same IntegrityError as a name clash — the API answered 'A deployment named BRAVO/demo_lab already exists' for a project that does not exist. Validate both references up front with a proper 404, and translate only a uniqueness violation into 409; anything else keeps its own detail. If the commit failed after the password was written, the row was gone but vault_pass.txt remained. A later create that supplies no password skips the seed branch entirely, so it would silently adopt a secret belonging to a deployment that never existed. Snapshot the prior file and restore it — or remove it when there was none — when the commit fails. All three new tests fail against the previous commit. --- app/routes/v1/deployments/crud.py | 92 +++++++++++++++++++--- tests/routes/test_deployment_vault_seed.py | 81 +++++++++++++++++++ 2 files changed, 163 insertions(+), 10 deletions(-) diff --git a/app/routes/v1/deployments/crud.py b/app/routes/v1/deployments/crud.py index 2ae219c..131774a 100644 --- a/app/routes/v1/deployments/crud.py +++ b/app/routes/v1/deployments/crud.py @@ -18,12 +18,14 @@ from app.core.config import settings from app.core.db import get_session_factory from app.core.errors import Range42Error, WorkspaceNonLocalFsError -from app.core.models import Deployment +from app.core.logging import get_logger +from app.core.models import Deployment, Project, ProxmoxHost from app.core.workspace import Workspace, WorkspaceError from app.schemas.v1.common import Page from app.schemas.v1.deployments import DeploymentCreate, DeploymentOut router = APIRouter() +log = get_logger(__name__) async def _session() -> AsyncSession: @@ -43,6 +45,22 @@ async def list_deployments(session: AsyncSession = Depends(_session), ) +def _restore_vault_pass(path: Path, prior: str | None) -> None: + """Undo a seed whose deployment never persisted. + + Best-effort: a workspace left slightly dirty is recoverable, but raising + here would mask the original failure. + """ + try: + if prior is None: + path.unlink(missing_ok=True) + else: + path.write_text(prior) + except OSError: + log.warning("could not restore vault_pass.txt after a failed create", + path=str(path)) + + @router.post("/", response_model=DeploymentOut, status_code=status.HTTP_201_CREATED) async def create_deployment(payload: DeploymentCreate, session: AsyncSession = Depends(_session)): @@ -68,6 +86,27 @@ async def create_deployment(payload: DeploymentCreate, ), details=[{"field": "codename", "reason": f"in use by {clash.id}"}], ) + + # Validate the foreign keys here too. They are enforced at DB level + # (PRAGMA foreign_keys=ON), so a bad reference would otherwise surface + # from the flush below as an IntegrityError indistinguishable from the + # uniqueness clash — and get reported as "already exists". + for field, model, value in ( + ("project_id", Project, payload.project_id), + ("target_host_id", ProxmoxHost, payload.target_host_id), + ): + found = (await session.execute( + select(model.id).where(model.id == value) + )).scalar_one_or_none() + if found is None: + raise Range42Error( + error="not_found", + code="NOT_FOUND", + status=404, + message=f"{model.__name__} {value} not found", + details=[{"field": field, "reason": "no such row"}], + ) + try: ws = Workspace.create( codename=payload.codename, @@ -102,30 +141,51 @@ async def create_deployment(payload: DeploymentCreate, await session.flush() except IntegrityError as e: await session.rollback() + # Only the workspace-uniqueness constraint means "already exists". + # Anything else (a reference deleted between the checks above and this + # flush, say) must not be dressed up as a name clash. + detail = str(getattr(e, "orig", e)) + if "unique" in detail.lower() or "uq_deployment_workspace" in detail: + raise Range42Error( + error="conflict", + code="DEPLOYMENT_EXISTS", + status=409, + message=( + f"A deployment named {payload.codename}/" + f"{payload.scenario_label} already exists" + ), + details=[{"field": "codename", "reason": "created concurrently"}], + ) from e raise Range42Error( - error="conflict", - code="DEPLOYMENT_EXISTS", + error="invalid_reference", + code="INVALID_REFERENCE", status=409, - message=( - f"A deployment named {payload.codename}/" - f"{payload.scenario_label} already exists" - ), - details=[{"field": "codename", "reason": "created concurrently"}], + message="Deployment could not be persisted", + details=[{"field": "payload", "reason": detail}], ) from e # Seed the workspace vault password: deploy_trigger reads # /secrets/vault_pass.txt to set ANSIBLE_VAULT_PASSWORD_FILE and to # unlock the SSH keys for the run, and nothing else writes it. Absent or # empty leaves an operator-seeded file alone. + vault_pass_file = ws.path / "secrets" / "vault_pass.txt" + prior_secret: str | None = None + seeded = False if payload.secrets and payload.secrets.get("vault_password"): try: - vault_pass_file = ws.path / "secrets" / "vault_pass.txt" + # Remember what was there. If the commit below fails, the row will + # not exist but the file would — and a later create that supplies + # no password skips this branch entirely, silently adopting a + # secret that belongs to a deployment that never existed. + if vault_pass_file.is_file(): + prior_secret = vault_pass_file.read_text() vault_pass_file.parent.mkdir(parents=True, exist_ok=True) # chmod before the content so the secret is never briefly # world-readable. vault_pass_file.touch(mode=0o600, exist_ok=True) vault_pass_file.chmod(0o600) vault_pass_file.write_text(payload.secrets["vault_password"]) + seeded = True except OSError as e: await session.rollback() raise Range42Error( @@ -136,7 +196,19 @@ async def create_deployment(payload: DeploymentCreate, details=[{"field": "secrets.vault_password", "reason": str(e)}], ) from e - await session.commit() + try: + await session.commit() + except Exception as e: + await session.rollback() + if seeded: + _restore_vault_pass(vault_pass_file, prior_secret) + raise Range42Error( + error="workspace_error", + code="DEPLOYMENT_PERSIST_FAILED", + status=500, + message="Deployment could not be persisted", + details=[{"field": "deployment", "reason": str(e)}], + ) from e await session.refresh(row) # Best-effort proxmox token provisioning: requires an app-level vault diff --git a/tests/routes/test_deployment_vault_seed.py b/tests/routes/test_deployment_vault_seed.py index 405d136..d0c8896 100644 --- a/tests/routes/test_deployment_vault_seed.py +++ b/tests/routes/test_deployment_vault_seed.py @@ -200,3 +200,84 @@ def _boom(self, *a, **kw): vault_pass = tmp_path / "ws" / "ALPHA-demo_lab" / "secrets" / "vault_pass.txt" assert vault_pass.read_text() == VAULT_PW + + +@pytest.mark.asyncio +async def test_unknown_project_and_host_report_what_is_actually_wrong( + tmp_path, monkeypatch, +): + """FKs are enforced at DB level, so a bad reference used to surface from + the flush as an IntegrityError and get reported as "already exists".""" + app = await _boot(tmp_path, monkeypatch) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + bad_project = await c.post( + "/v1/deployments/", json=_payload(codename="BRAVO", project_id="nope")) + bad_host = await c.post( + "/v1/deployments/", json=_payload(codename="CHARLIE", target_host_id="nope")) + + for r, missing in ((bad_project, "Project"), (bad_host, "ProxmoxHost")): + assert r.status_code == 404, r.text + assert r.json()["code"] == "NOT_FOUND" + assert missing in r.json()["message"] + assert "already exists" not in r.json()["message"] + + +@pytest.mark.asyncio +async def test_commit_failure_does_not_leave_a_stale_secret(tmp_path, monkeypatch): + """A commit failure after seeding must not leave the password behind. + + The row would not exist, but a later create that supplies no password + skips the seed branch entirely — and would silently adopt a secret + belonging to a deployment that never existed. + """ + app = await _boot(tmp_path, monkeypatch) + vault_pass = tmp_path / "ws" / "ALPHA-demo_lab" / "secrets" / "vault_pass.txt" + + from sqlalchemy.ext.asyncio import AsyncSession + + real_commit = AsyncSession.commit + + async def _boom(self): + raise OSError(5, "I/O error") + + monkeypatch.setattr(AsyncSession, "commit", _boom) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + r = await c.post("/v1/deployments/", + json=_payload(secrets={"vault_password": VAULT_PW})) + assert r.status_code == 500 + assert r.json()["code"] == "DEPLOYMENT_PERSIST_FAILED" + assert not vault_pass.exists(), "a secret was left for a deployment that never existed" + + monkeypatch.setattr(AsyncSession, "commit", real_commit) + + +@pytest.mark.asyncio +async def test_commit_failure_restores_an_operator_seeded_secret( + tmp_path, monkeypatch, +): + """If the workspace already held a password, put it back — do not delete.""" + app = await _boot(tmp_path, monkeypatch) + ws_secrets = tmp_path / "ws" / "ALPHA-demo_lab" / "secrets" + ws_secrets.mkdir(parents=True, exist_ok=True) + vault_pass = ws_secrets / "vault_pass.txt" + vault_pass.write_text("OPERATOR-SEEDED") + + from sqlalchemy.ext.asyncio import AsyncSession + real_commit = AsyncSession.commit + + async def _boom(self): + raise OSError(5, "I/O error") + + monkeypatch.setattr(AsyncSession, "commit", _boom) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + await c.post("/v1/deployments/", + json=_payload(secrets={"vault_password": VAULT_PW})) + assert vault_pass.read_text() == "OPERATOR-SEEDED" + + monkeypatch.setattr(AsyncSession, "commit", real_commit)