Files
svcforge/services/api/deps.py
T
Nguyen Minh Phuc c76154aeaa
ci / lint (push) Successful in 34s
ci / unit (push) Successful in 1m41s
ci / types (push) Successful in 1m41s
ci / dockerfile (push) Successful in 18s
ci / security (push) Successful in 1m27s
ci / chart (push) Failing after 1m11s
ci / integration (push) Successful in 1m10s
ci / image (api) (push) Has been skipped
ci / image (reconciler) (push) Has been skipped
ci / image (worker) (push) Has been skipped
ci / bump (push) Has been skipped
review: fix 26 findings from a 4-agent audit
CORRECTNESS
- lost-lease race: complete()/fail() did not check ownership, so a worker whose
  lease expired could mark a task done while another worker was running it, or
  requeue a task someone else owned. Reproduced, fixed with a CAS on
  (state, locked_by), pinned by two regression tests.
- worker died on report failure: _run_one's docstring claimed no exception
  escapes the TaskGroup; fail()/complete() were outside the guarded block, so a
  DB blip cancelled every sibling provision on the pod.
- claim query used an INNER join, which could strand a just-claimed task and
  report 'queue empty'. LEFT join.
- InstanceRepo.set_error bypassed the state machine and had no callers. Deleted.
- handle_deprovision ignored its CAS result, so a wrong-state instance kept a
  dangling endpoint and got re-provisioned by the drift check 60s later.
- handle_verify re-notified on every retry: five pages for one halt.

DEPLOY-BREAKING
- the migration Job could never succeed: no Dockerfile copied migrations/, and
  migrate.py resolved the path relative to the source tree, which only works for
  an editable install. Added COPY + SVCFORGE_MIGRATIONS_DIR.
- ServiceMonitor selector did not match the Service: API metrics never scraped.
- SvcforgeReconcilerStale fired permanently from every pod, because the gauge is
  module-level and every service exports it as 0. Scoped to the reconciler job.
- SvcforgeTaskFailed latched forever on a monotonic counter. Now increase()[15m].
- the digest guard accepted the all-zeros placeholder.
- worker terminationGracePeriodSeconds was 60s against a 600s helm timeout.

DEAD CODE THAT SHOULD NOT HAVE BEEN
- adapters/k8s.py was never called, so tenant namespaces were never created and
  the first provision for a new team would fail. Wired into handle_provision.
- adapters/redis.py was never imported by any service. Rate limiting is now wired
  into the API, failing open.
- Settings.check_production() had no callers. Given an explicit environment and
  called from every entrypoint.

OBSERVABILITY
- the API never called obs.setup(): no JSON logs, no trace correlation, log_json
  silently inert.
- LogNotifier's structured fields were discarded by the stdlib->structlog bridge.
- bind_task_context cleared the 'service' binding for the life of every task.
- split tasks_failed into task_attempts_failed and tasks_dead_lettered.

SECURITY
- trivy correctly blocked the worker/reconciler images: helm 3.16.2 and kubectl
  1.31.2 carry CRITICAL Go stdlib CVEs. Bumped to helm 3.21.3 and kubectl 1.35.3,
  which also closes a four-minor skew against the v1.35.3 cluster.

TESTS THAT COULD NOT FAIL
- the concurrency cap test passed on a fully serial worker.
- the alert/metric cross-check asserted a hardcoded list instead of reading the
  chart, so it could not catch a rename on the chart side.
- fixed OTel tracer-provider pollution between test files.

DOCS
- ARCHITECTURE.md: mermaid diagrams, user stories, and the helm-vs-ArgoCD
  guarantee (verified with --dry-run=server).
- AGENTS.md + CLAUDE.md.
- prose sweep for back-and-forth phrasing across 19 files.
2026-07-18 12:13:49 +00:00

195 lines
7.6 KiB
Python

"""Dependency injection: how a handler gets a pool, a repo, a catalog, and a team.
Everything expensive — the pool, the JWKS client, the parsed catalog — is built once in
`lifespan` and parked on `app.state`. These functions only hand it out. A `Depends` that
does I/O per request is a `Depends` that does that I/O on every request forever.
"""
from __future__ import annotations
import asyncio
from typing import Annotated, Any
import jwt
from fastapi import Depends, HTTPException, Request, status
from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer
from jwt import PyJWKClient
from svcforge_core.adapters.redis import RateLimiterProto
from svcforge_core.domain.models import CatalogEntry
from svcforge_core.repo.db import DictPool
from svcforge_core.repo.instances import InstanceRepo
from svcforge_core.repo.tasks import TaskRepo
from svcforge_core.settings import Settings
# The algorithm allow-list is the whole point of naming algorithms explicitly.
# `jwt.decode(..., algorithms=...)` without it accepts whatever the *token* claims in its
# own header — including `none`, and including HS256 verified with the RSA public key as
# an HMAC secret. Both are forgery. The list is not configuration.
ALLOWED_ALGORITHMS = ["RS256"]
# What `auth_disabled` returns. Settings.check_production() refuses that flag in prod.
DEV_TEAM = "platform"
TEAM_CLAIM = "team"
# auto_error=False is load-bearing. HTTPBearer(auto_error=True) answers a *missing*
# Authorization header with 403, not 401 — an old FastAPI wart. The spec (and every
# client that knows what to do about it) wants 401, so the error is raised here.
_bearer = HTTPBearer(auto_error=False)
def _unauthorized() -> HTTPException:
"""One shape for every auth failure.
Expired, wrong issuer, wrong audience, bad signature, malformed, no header: all the
same 401 with the same body. Telling a caller *which* one turns the endpoint into an
oracle they can tune a forgery against.
"""
return HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail={"code": "unauthorized", "message": "invalid or missing credentials"},
headers={"WWW-Authenticate": "Bearer"},
)
def get_settings(request: Request) -> Settings:
"""The Settings that create_app() was handed."""
settings: Settings = request.app.state.settings
return settings
async def get_pool(request: Request) -> DictPool:
"""Return the pool that lifespan put on app.state."""
pool: DictPool = request.app.state.pool
return pool
def get_catalog(request: Request) -> dict[str, CatalogEntry]:
"""The catalog, parsed once at startup.
Read from disk per request and a mid-flight edit to catalog.yaml changes the answer
between two requests of the same deploy. Load it at startup; a change is a restart.
"""
catalog: dict[str, CatalogEntry] = request.app.state.catalog
return catalog
def get_instance_repo(pool: Annotated[DictPool, Depends(get_pool)]) -> InstanceRepo:
"""An InstanceRepo bound to the app's pool. Cheap: it is a handle, not a connection."""
return InstanceRepo(pool)
def get_task_repo(pool: Annotated[DictPool, Depends(get_pool)]) -> TaskRepo:
"""A TaskRepo bound to the app's pool."""
return TaskRepo(pool)
async def get_current_team(
request: Request,
creds: Annotated[HTTPAuthorizationCredentials | None, Depends(_bearer)],
settings: Annotated[Settings, Depends(get_settings)],
) -> str:
"""Verify the JWT against the cached JWKS. Check aud/iss/exp and the alg allow-list.
Returns the team claim. Raises HTTPException(401) on any failure — never leaks why.
"""
if settings.auth_disabled:
return DEV_TEAM
if creds is None or not creds.credentials:
raise _unauthorized()
jwks_client: PyJWKClient | None = getattr(request.app.state, "jwks_client", None)
if jwks_client is None:
# Auth is on but there is no key source. Fail closed. Answering 500 here would be
# honest about the cause and would also let a misconfigured deploy be told apart
# from a bad token; 401 is the same answer a forger gets.
raise _unauthorized()
try:
# PyJWKClient keeps its own TTL cache, so this is a dict lookup on the hot path.
# It is only blocking on a cache MISS (key rotation) — hence to_thread, which
# costs a thread hop we take a handful of times a day rather than an event loop
# stalled on someone else's HTTP call once per rotation.
signing_key = await _signing_key(jwks_client, creds.credentials)
claims: dict[str, Any] = jwt.decode(
creds.credentials,
signing_key.key,
algorithms=ALLOWED_ALGORITHMS,
audience=settings.jwt_audience,
issuer=settings.jwt_issuer,
options={
"require": ["exp", "aud", "iss"],
"verify_exp": True,
"verify_aud": True,
"verify_iss": settings.jwt_issuer is not None,
"verify_signature": True,
},
)
except Exception as exc: # deliberate catch-all: every failure becomes one opaque 401
raise _unauthorized() from exc
team = claims.get(TEAM_CLAIM)
if not isinstance(team, str) or not team:
raise _unauthorized()
return team
async def _signing_key(client: PyJWKClient, token: str) -> jwt.PyJWK:
"""Fetch the signing key off the event loop.
PyJWKClient.get_signing_key_from_jwt() does a synchronous urlopen on a cache miss.
Called directly from `async def`, that blocks the loop — every other in-flight request
on this worker stops until the identity provider answers, and if it hangs, so does the
pod, and /readyz keeps saying it is fine.
"""
return await asyncio.to_thread(client.get_signing_key_from_jwt, token)
def get_rate_limiter(request: Request) -> RateLimiterProto | None:
"""The limiter lifespan built, or None when no Redis is configured."""
limiter: RateLimiterProto | None = getattr(request.app.state, "rate_limiter", None)
return limiter
async def rate_limit(
request: Request,
team: Annotated[str, Depends(get_current_team)],
) -> None:
"""Per-team rate limiting. One Redis command per check, and it fails OPEN.
Failing open is the entire policy. Redis holds derived state; losing it must degrade
the platform, never stop it. A limiter that fails closed converts a cache outage into
a total outage, which is a strictly worse incident than the burst it was protecting
against — so `RateLimiter.check` swallows its own errors and returns `allowed=True`.
The 429 below therefore only ever comes from a real, counted overage.
"""
limiter = get_rate_limiter(request)
if limiter is None:
return
result = await limiter.check(team)
if not result.allowed:
raise HTTPException(
status_code=status.HTTP_429_TOO_MANY_REQUESTS,
detail={"code": "rate_limited", "message": "too many requests"},
headers={"Retry-After": str(result.retry_after_s)},
)
async def idempotency_key(request: Request) -> str | None:
"""`Idempotency-Key` handling. Seam only — Module 10 fills this in (Redis store).
Until then the real idempotency anchor is `instances.release_name`, which is unique in
the schema and deterministic from (team, service_type, id).
"""
return request.headers.get("Idempotency-Key")
PoolDep = Annotated[DictPool, Depends(get_pool)]
TeamDep = Annotated[str, Depends(get_current_team)]
InstanceRepoDep = Annotated[InstanceRepo, Depends(get_instance_repo)]
TaskRepoDep = Annotated[TaskRepo, Depends(get_task_repo)]
CatalogDep = Annotated[dict[str, CatalogEntry], Depends(get_catalog)]