-
Notifications
You must be signed in to change notification settings - Fork 0
fix(deploy): reject duplicate codenames before mutating the shared workspace #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,28 @@ async def list_deployments(session: AsyncSession = Depends(_session), | |
| @router.post("/", response_model=DeploymentOut, status_code=status.HTTP_201_CREATED) | ||
| async def create_deployment(payload: DeploymentCreate, | ||
| session: AsyncSession = Depends(_session)): | ||
| # (codename, scenario_label) is unique and also names the workspace | ||
| # directory, so a duplicate would reuse an existing deployment's | ||
| # workspace. Reject it here rather than letting the commit fail: by then | ||
| # the shared directory has already been mutated, and the IntegrityError | ||
| # surfaces as an opaque 500. | ||
| clash = (await session.execute( | ||
| select(Deployment).where( | ||
| Deployment.codename == payload.codename, | ||
| Deployment.scenario_label == payload.scenario_label, | ||
| ) | ||
| )).scalar_one_or_none() | ||
| if clash is not None: | ||
| 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": f"in use by {clash.id}"}], | ||
| ) | ||
| try: | ||
| ws = Workspace.create( | ||
| codename=payload.codename, | ||
|
|
@@ -56,15 +78,33 @@ async def create_deployment(payload: DeploymentCreate, | |
| message=e.message, | ||
| details=[{"field": "workspace_root", "reason": e.message}], | ||
| ) from e | ||
| # Seed the workspace vault password. deploy_trigger reads | ||
| row = Deployment( | ||
| id=uuid.uuid4().hex[:16], | ||
| codename=payload.codename, | ||
| scenario_label=payload.scenario_label, | ||
| project_id=payload.project_id, | ||
| target_host_id=payload.target_host_id, | ||
| catalog_sha=payload.catalog_sha, | ||
| project_sha=payload.project_sha, | ||
| team_count=payload.team_count, | ||
| state="pending", | ||
| workspace_path=str(ws.path), | ||
| ) | ||
| session.add(row) | ||
| await session.commit() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a supplied vault password cannot be written—for example because the disk fills, permissions change, or the process exits after this commit—the deployment row remains permanently committed even though the POST fails or disconnects before seeding the required secret. A retry with the same name then receives the new 409 conflict, and there is no API that updates the password or removes the deployment row, leaving the deployment unusable without manual intervention; reserve the unique name within the transaction but commit only after the required file write succeeds, or compensate by removing the row on failure. Useful? React with 👍 / 👎. |
||
| await session.refresh(row) | ||
|
|
||
| # Everything below mutates the workspace, so it runs only once the row is | ||
| # persisted — a failed create must never touch another deployment's files. | ||
| # | ||
| # Seed the workspace vault password: deploy_trigger reads | ||
| # <ws>/secrets/vault_pass.txt to set ANSIBLE_VAULT_PASSWORD_FILE and to | ||
| # unlock the SSH keys for the run; nothing else writes that file, so a | ||
| # deploy created purely through the API had no way to decrypt anything. | ||
| # The UI collects this on the deploy form. | ||
| # unlock the SSH keys for the run, and nothing else writes it. Absent or | ||
| # empty leaves an operator-seeded file alone. | ||
| if payload.secrets and payload.secrets.get("vault_password"): | ||
| vault_pass_file = ws.path / "secrets" / "vault_pass.txt" | ||
| vault_pass_file.parent.mkdir(parents=True, exist_ok=True) | ||
| # Written before the content so the secret is never briefly world-readable. | ||
| # 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"]) | ||
|
|
@@ -90,21 +130,7 @@ async def create_deployment(payload: DeploymentCreate, | |
| ) | ||
| except Exception: # noqa: BLE001 — best-effort; preflight is source of truth | ||
| pass | ||
| row = Deployment( | ||
| id=uuid.uuid4().hex[:16], | ||
| codename=payload.codename, | ||
| scenario_label=payload.scenario_label, | ||
| project_id=payload.project_id, | ||
| target_host_id=payload.target_host_id, | ||
| catalog_sha=payload.catalog_sha, | ||
| project_sha=payload.project_sha, | ||
| team_count=payload.team_count, | ||
| state="pending", | ||
| workspace_path=str(ws.path), | ||
| ) | ||
| session.add(row) | ||
| await session.commit() | ||
| await session.refresh(row) | ||
|
|
||
| return DeploymentOut.model_validate(row, from_attributes=True) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When two POSTs for the same
(codename, scenario_label)overlap, both can complete this read before either insert commits, so both proceed and the unique constraint rejects one duringsession.commit(). That request still leaks anIntegrityErroras an opaque 500 instead of the promisedDEPLOYMENT_EXISTS409; handle the constraint failure after rollback or use an atomic insertion/reservation rather than relying solely on this check.Useful? React with 👍 / 👎.