diff --git a/docs/api.md b/docs/api.md index d49ae62..28170b8 100644 --- a/docs/api.md +++ b/docs/api.md @@ -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 diff --git a/docs/architecture.md b/docs/architecture.md index 0426546..60df93e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -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. @@ -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 diff --git a/src/hosts/service.py b/src/hosts/service.py index 6a41d9b..13c9909 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -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 @@ -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 = ( @@ -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, @@ -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 @@ -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() diff --git a/src/hosts/tests/test_secrets.py b/src/hosts/tests/test_secrets.py index ae1399c..fef9cd6 100644 --- a/src/hosts/tests/test_secrets.py +++ b/src/hosts/tests/test_secrets.py @@ -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 = { @@ -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, diff --git a/src/secrets_exchange/secrets.py b/src/secrets_exchange/secrets.py index d869c93..7581484 100644 --- a/src/secrets_exchange/secrets.py +++ b/src/secrets_exchange/secrets.py @@ -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 @@ -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.""" @@ -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: @@ -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: