Contribution Date
Contribution Project
Contribution Details
A metrics endpoint that cannot map your API, and a limiter that cannot be spoofed
Go-live items 3 and 4, paired because they are both "the thing that has been declared and
never wired".
## `/ready`, and what it deliberately does not check
`/health` already answers liveness and says nothing on purpose. `/ready` answers the other
question — should traffic come here — by touching what a request touches: Postgres and the
object store, **both** checked rather than short-circuiting on the first failure, because
during a wide outage "the database is down" tells you nothing about whether the store is
also down.
It skips the search index, and that is a decision rather than an omission: a tenant's index
opens lazily and is rebuildable, so failing readiness over it would pull a replica out of
rotation for something that stops no upload, download or metadata write.
## `/metrics` is fail-closed, and 404 rather than 401
No `server.metrics_token`, no endpoint. A wrong token gets the same 404, so a scan cannot
tell a deployment with metrics switched off from one behind a credential.
The argument is already written in this repo against `/health`: it is the first thing
anybody scans, and metrics are worse than a version string — route templates are a map of
the entire API surface and per-route counts are a usage profile. The default that ends up on
the public internet is the one that serves openly and trusts a firewall.
## Cardinality is the whole design
The registry is hand-written in `dam-telemetry`: four series, about two hundred visible
lines, no global recorder to install and no ordering question about when. If the metric set
outgrows one screen the `metrics` crates are the right answer and this should be deleted
rather than extended.
The route label is axum's `MatchedPath` — the template — and never the URI. A label per
asset id is a million series and a monitoring system that falls over, so there is a case
asserting a thousand requests through one template stay one series. An unmatched request
collapses to a single bucket, because labelling 404s with their URIs would let anybody
create unbounded series by requesting random paths. Status is a class rather than a code:
the questions asked of it are answered by the class, and the code is in the log.
`damrs_jobs` is refreshed on scrape rather than on a timer — queue depth is a fact about the
database, not something this process accumulates — and a failure reading it does not fail
the scrape. An endpoint that 500s because one gauge is unavailable goes dark exactly when
the database is the thing going wrong. `state="dead"` is the series worth alerting on: a
worker failing every derivative is indistinguishable from outside from one with nothing to
do.
Lock poisoning is recovered from rather than propagated. The worst case is a counter off by
one, and letting the observability layer panic the process is backwards.
## The limiter protects the public routes and nothing else
`governor` has been a declared dependency commented "per-tenant rate limiting" and never
called. It now covers `/d/{token}`, `/share/{token}` and `/portal/{key}` — the routes that
take no API key — keyed by client address.
The authenticated API is deliberately not address-keyed. A company sits behind one or two
egress addresses, so that would be a limit on the *customer*: the whole art department
sharing a bucket and one bulk upload starving everybody's thumbnails. Authenticated traffic
has a revocable credential and per-tenant quotas, which are the right instruments.
Burst is separate from the sustained rate because a grid loads sixty thumbnails at once, and
a limiter tuned only on the rate throttles the first screen every user ever sees. Off by
default: a guessed number either does nothing or throttles a real page load, and neither is
discovered until it is in front of users.
`X-Forwarded-For` is trusted only as far as `trusted_proxy_hops` says, counting from the
**right**, because those entries are what each proxy appended and a client cannot forge
them. Taking the leftmost entry — the usual mistake — lets anybody claim a fresh bucket per
request, or put somebody else's address in and exhaust theirs. There is a case for each hop
count and one for a header shorter than claimed, which falls back to the socket rather than
promoting a client-supplied value.
## One correction
The middleware first took `ConnectInfo` by value while its own comment claimed a missing
address was allowed. A required extractor *rejects*, so a forgotten
`into_make_service_with_connect_info` in `damd` would have been a 500 on every public route
rather than a lenient one — the comment described the opposite of the code. It reads the
extension directly now, and a case asserts both halves: engaged when a peer is present,
allowing when it is not.
Verified live: three requests through the burst then 429s with `Retry-After` on the public
route, six of six 200s on the authenticated API, `/ready` reporting both dependencies, and
`/metrics` 404 without a token and correct with one — including `route="/assets/{asset_id}"`
for a request to `/assets/nope`.
`dam-api` also gains the `cfg_attr(test, allow(...))` every other crate carries; it had no
unit tests until the throttle got some.
Contribution Issue Link
Files count
0
Patches count
1