Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ service account that first used it. Another one reusing it gets `409`.
provider has `HOST_LIMITS` live hosts. A live host is not in `error` and
has not expired.

`POST /hosts` returns `422` with `SECRET_MISSING` when a secret is missing.
The detail is the issuer's reason, for example that the GitHub App is not
on the repository. A retry gets the same answer.

## Refresh a host secret

`POST /hosts/{host_id}/secrets/{service}/refresh` makes the exchange drop
Expand Down
12 changes: 8 additions & 4 deletions docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ a provider into the contract?*

`POST /hosts` provisions inline on the event loop and returns
`201 Created` with the active host, or `502 Bad Gateway` with the row
left in `error` state. States live in `hosts.models.HostStatus`:
left in `error` state. When a secret is missing, the response is `422`
with the issuer's reason, not `502`. A retry gets the same answer. States
live in `hosts.models.HostStatus`:
`provisioning → creating_network → creating_vm → bootstrapping →
active`, with `error` as the terminal failure.

Expand All @@ -120,9 +122,11 @@ the request headers, and the refresh interval. Drukbox never stores a fetched
token. The exchange fetches it on demand, keeps it in memory, and fetches again
when less than a minute of its life remains. The issuer answers
`{"value": "…", "expires_at": "…"}`. `expires_at` is optional, and without it
the refresh interval sets the expiry. Any other answer is a failure. While a
issuer fails, the exchange serves the last value it holds, as long as that
value is still valid. With nothing valid in memory it answers `503`.
the refresh interval sets the expiry. When the secret is missing, the issuer
answers `4xx` with `{"detail": "…"}`, the reason. Any other answer is a
failure. While an issuer fails, the exchange serves the last value it holds,
as long as that value is still valid. With nothing valid in memory it answers
`503`.

A provider that holds the value never asks the exchange. For it, a timer
fetches a fresh value when less than a minute of the pushed one remains and
Expand Down
63 changes: 29 additions & 34 deletions src/hosts/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
)
from providers.registry import get_provider_names, get_vm_provider
from secrets_exchange.client import SecretsExchange
from secrets_exchange.secrets import IssuerError, Secret
from secrets_exchange.secrets import IssuerError, MissingSecretError, Secret
from templates.exceptions import TemplateNotAvailableError, UnknownTemplateError
from templates.models import Template, TemplateStatus

Expand Down Expand Up @@ -167,12 +167,8 @@ async def _try_claim_pool_host(
provider: str,
expires_at: datetime | None | EllipsisType,
) -> Host | None:
# Pick a candidate, then atomically claim it with UPDATE ... WHERE
# claimed_at IS NULL ... RETURNING. The WHERE predicate is the actual
# race guard — concurrent claimants resolve to a single winner per
# row regardless of dialect (PG: MVCC + WHERE filter; SQLite: write
# lock + WHERE filter). Losers return None and the caller falls
# through to fresh provisioning.
# The claim's WHERE on claimed_at is the race guard: concurrent
# claimants resolve to one winner per row on both dialects.
now = utc_now()
vm = get_vm_provider(provider)
candidates = (
Expand All @@ -188,32 +184,29 @@ async def _try_claim_pool_host(
if vm.max_lifetime and isinstance(expires_at, datetime):
# A warm host that the provider stops before the requested end is no match.
candidates = candidates.where(Host.created_at >= expires_at - vm.max_lifetime)
candidate = (await self.session.execute(candidates)).scalar_one_or_none()
if not candidate:
return

if expires_at is ...:
expires_at = self._default_lease_expires_at()
# The claim replaces the warm-pool max-age TTL with the caller's lease:
# a concrete window (explicit or the default), or None for a caller
# who deliberately opted into a permanent host.
result = await self.session.execute(
update(Host)
.where(Host.id == candidate.id)
.where(Host.claimed_at.is_(None))
.values(
service_account=service_account,
claimed_at=now,
updated_at=now,
expires_at=vm.lease_end(candidate.created_at, expires_at),
if candidate := (await self.session.execute(candidates)).scalar_one_or_none():
if expires_at is ...:
expires_at = self._default_lease_expires_at()
# The claim replaces the warm-pool max-age TTL with the caller's lease:
# a concrete window (explicit or the default), or None for a caller
# who deliberately opted into a permanent host.
result = await self.session.execute(
update(Host)
.where(Host.id == candidate.id)
.where(Host.claimed_at.is_(None))
.values(
service_account=service_account,
claimed_at=now,
updated_at=now,
expires_at=vm.lease_end(candidate.created_at, expires_at),
)
.returning(Host)
)
.returning(Host)
)
host = result.scalar_one_or_none()
await self.session.commit()
if host:
logger.info("pool: claimed host_id=%s name=%s", host.id, host.name)
return host
host = result.scalar_one_or_none()
await self.session.commit()
if host:
logger.info("pool: claimed host_id=%s name=%s", host.id, host.name)
return host

async def create_host(
self,
Expand Down Expand Up @@ -571,6 +564,9 @@ async def provision(self, host_id: str) -> None:
try:
environment.update(await self.put_secrets(host, vm))
get_persist(environment)
except MissingSecretError as exc:
await self.mark_failed(host, exc)
raise
except (IssuerError, ProviderError, ValueError) as exc:
await self.mark_failed(host, exc)
return
Expand Down Expand Up @@ -689,8 +685,7 @@ async def mark_failed(self, host: Host, exc: Exception) -> None:
host.last_error = f"{type(exc).__name__}: {exc}"
now = utc_now()
# An errored host is dead weight (its VM may be half-created). Expire it
# now so the janitor is the single owner of teardown; the POST caller
# already got last_error in the 502.
# now so the janitor is the single owner of teardown.
host.expires_at = now
host.updated_at = now
await self.session.commit()
Expand Down
20 changes: 20 additions & 0 deletions src/hosts/tests/test_secrets.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from providers.base import VMCreateResult
from providers.capabilities import SecretInjectionCapability
from providers.exceptions import ProviderTransportError
from secrets_exchange.secrets import MissingSecretError

ISSUER = {"url": "https://mint.test/box/github", "headers": {"X-Key": "k"}, "refresh": "1h"}
SECRETS = {
Expand Down Expand Up @@ -222,6 +223,25 @@ async def test_an_issuer_that_gives_no_value_at_boot_fails_provisioning(
create_vm.assert_not_awaited()


@respx.mock
async def test_a_missing_secret_at_boot_fails_provisioning_with_the_issuers_reason(
settings: Settings, create_vm: AsyncMock, stub_provider: StubVMProvider
) -> None:
stub_provider.secrets = RecordingInjection()
respx.get(ISSUER["url"]).respond(
status_code=422, json={"detail": "the GitHub App is not installed on acme/api"}
)

async with async_session_factory() as session:
with pytest.raises(MissingSecretError) as failure:
await HostService(session, settings=settings).create_host(
env={}, secrets=SECRETS, image=None, provider="stub"
)

assert str(failure.value) == "the GitHub App is not installed on acme/api"
create_vm.assert_not_awaited()


@respx.mock
async def test_a_wrong_issuer_answer_at_boot_never_reaches_the_log_or_the_host(
settings: Settings,
Expand Down
13 changes: 12 additions & 1 deletion src/secrets_exchange/secrets.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import httpx
from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, ValidationError

from core.exceptions import AppException
from hosts.models import Host
from providers.exceptions import ProviderError
from providers.registry import get_vm_provider
Expand All @@ -28,6 +29,14 @@ class IssuerError(Exception):
"""The message is safe to log."""


class MissingSecretError(IssuerError, AppException):
"""The secret is missing, and the message is the issuer's reason. A retry gets
the same answer."""

status_code = 422
error_code = "SECRET_MISSING"


class IssuerUnavailableError(Exception):
"""No valid secret exists for the entry."""

Expand All @@ -44,6 +53,8 @@ class Secret(BaseModel):
async def fetch(cls, issuer: dict[str, Any], client: httpx.AsyncClient) -> Self:
try:
response = await client.get(issuer["url"], headers=issuer["headers"])
if response.is_client_error:
raise MissingSecretError(response.json()["detail"])
response.raise_for_status()
secret = cls.model_validate(response.json())
except httpx.LocalProtocolError:
Expand All @@ -55,7 +66,7 @@ async def fetch(cls, issuer: dict[str, Any], client: httpx.AsyncClient) -> Self:
raise IssuerError(type(exc).__name__) from exc
except json.JSONDecodeError:
raise IssuerError("answer is not JSON") from None
except ValidationError:
except (KeyError, ValidationError):
# The chain would carry the answer, and a token with it.
raise IssuerError("answer has the wrong shape") from None
if not secret.expires_at:
Expand Down
Loading