diff --git a/STATUS.md b/STATUS.md index ef71092..35c359b 100644 --- a/STATUS.md +++ b/STATUS.md @@ -72,7 +72,8 @@ Cache is a single shard-local lookup; only a miss touches the network. `go test -race ./internal/... -count=1 -timeout 300s` is **green across every package that has a suite**: `acl`, `app`, `blocklist`, `cache`, `config`, -`localzone`, `qlog`, `resolver`, `zonefile` — roughly 190 tests. +`engine`, `localzone`, `metrics`, `qlog`, `resolver`, `zonefile` — roughly 230 +tests — and `api` joined the list with 78 cases of its own. `make check-go` is the inner loop (1.3 s: gofmt check, vet, compile). It uses `go-build`, not `build`, because `build` depends on `web` and shells out to npm. @@ -80,11 +81,55 @@ package that has a suite**: `acl`, `app`, `blocklist`, `cache`, `config`, ### No test suite yet -`api`, `cluster`, **`engine`**, `metrics`, `server`, `web`. +`cluster`, `server`, `web`. -`engine` is the notable one: it is the query pipeline every packet goes through -and it has no test file at all. An attempt at one did not land, so treat it as -greenfield rather than an extension of an existing suite. +`internal/engine` is now covered — 23 tests over a harness (`harness_test.go`) +that builds a wired engine with a fake upstream on loopback, a local zone, a +blocklist, a cache and a query log — and `internal/config` has a `Size` suite. +`internal/metrics` has a counter/gauge/exposition suite (9 tests) that pins the +metric names, which are the contract with the Grafana dashboards and the +ServiceMonitor. `internal/api` is covered too: every route in the table (each +asserted to reach its hook, so a new route without a case is a route nothing +tests), method/path rejection, a read-route sweep that asserts nothing mutated, +panic recovery, the base path, all three auth modes, read-only keys and +`allow_config_write: false`, loopback trust, and the hashed API keys. +`internal/server` is the one in progress. + +## Fixed in the last round (the engine and `config.Size` work) + +- **`config.ParseSize` ignored the unit multiplier entirely.** `scaleSize` took + the number and returned `Size(n)`; `mult` never entered the arithmetic, so + `512MiB` parsed as **512 bytes** and `1GiB` as 1 byte. Every memory budget + written in units — `memory.limit`, `memory.reserve`, `memory.cache_max_bytes`, + the cache byte budget in the app wiring — was off by a factor of the unit, + and `EffectiveLimit`'s 64 MiB floor then quietly replaced the operator's + number. The memory envelope was inert for anyone who wrote a unit, which is + everyone: a bare number is bytes. The fix is one multiplication; + `size_test.go` now pins `1.5GiB`, `0.5KiB`, the unit shadowing cases (`MiB` + must not match `M`), the out-of-range error and the truncating rounding. +- **A cache miss and a stale hit moved only the Prometheus counter.** + `metrics.RecordMiss` and `metrics.RecordStaleHit` bumped the collector and + not the atomics that `Totals` reads, so the Cache page's "misses" and "stale + hits" read 0 for the life of the process while the Grafana panel was right — + exactly the disagreement `Record`'s doc comment says cannot happen. +- **The uncached path never truncated.** `finalize` — the EDNS0/UDP-size step + that turns an oversized answer into a question-only TC reply — was called + only on the *cache hit* path. A multi-kilobyte answer from upstream, from the + local zone or from a sinkhole left here as a datagram no 512-byte UDP client + can receive, and the client silently dropped it. All four reply paths now + finalize. +- **The serve-stale prefetch gate was not single-flight.** + `LRU.prefetchGate` was `make(chan struct{}, 64)`, so a burst of stale hits on + one hot name launched up to 64 duplicate upstream exchanges instead of the + one its own doc comment promised. It is a per-key latch now (a + `map[Key]struct{}` under a mutex): distinct stale names still refresh in + parallel, one name does not. `TryPrefetch`/`ReleasePrefetch` take the key. +- **The upstream UDP client read buffer was hard-coded to 1232** while the + engine advertises `dns.max_udp_size` upstream. An operator who raised + `max_udp_size` above 1232 got a SERVFAIL for *every* answer over 1232 bytes: + the datagram was read truncated and failed to unpack with "dns: overflowing + header size". `resolver.NewPoolWithUDPSize` takes the advertised size and the + app passes `cfg.DNS.MaxUDPSize`; `NewPool` keeps the 1232 default. ### Fixed recently and worth not re-breaking @@ -146,6 +191,32 @@ greenfield rather than an extension of an existing suite. - **`serveUDP` called `net.ResolveUDPAddr` per packet**; the peer already is a `*net.UDPAddr`. +## New findings from the `api` suite, unfixed + +- **The login rate limiter never applies to the login endpoint.** `handleLogin` + is registered through `public`, which bypasses `authed` and therefore + `allowRequest` — the only reader of `lockedTill`, and `handleLogin` the only + writer of `failures`. So `api.rate_limit.per_minute`, documented as "requests + per minute per IP **for the login endpoint**", applies to every *other* route + instead: posting 100 bad keys to `/api/auth/login` returns 100 × 401 and never + a 429, and the lockout only ever locks the caller out of the rest of the API. + Brute-force protection is the one thing the auth surface most needs. The suite + asserts nothing about this, so the fix is a free choice: register login + through the limiter, or move the lockout check into the login handler. +- **Auth configuration is frozen at `New`.** `api.base_path`, + `api.trusted_proxies` and the authenticator (`auth.mode`, `api_keys`) are + captured once in `New`; a reload re-reads CORS only. Writing a new API key + through the settings endpoint therefore answers `ok` while the running server + keeps the old key, and a `trusted_proxies` change needs a restart. A config + write endpoint that cannot apply its own auth change is an operational trap. + Pin the current behaviour with a test before changing it. +- **`api.constantTimeEqual` has no callers**, and the `authenticator` doc comment + describes a constant-time "API key scan" that does not exist. The real design + is better than the comment: API keys are compared as SHA-256 digests through a + hash-keyed map and the key itself is never stored, which satisfies the + AGENTS.md invariant. The comment is wrong, not the code; delete the dead + function and fix the comment. + ### Open decisions - **Unknown config keys are silently ignored.** Writing `block:` where the struct @@ -157,10 +228,30 @@ greenfield rather than an extension of an existing suite. client lands in the Ingress's ACL group. Fail-closed if that group is restrictive, fail-open if it is permissive. Pin the behaviour with a test before changing it. +- **A character-string longer than 255 bytes cannot be packed by this version + of `miekg/dns` (v1.1.73).** `TXT.len()` reports a >255-byte string as zero + length and `EDNS0_NSID.len()` is likewise wrong, so `Msg.Pack` fails with + "dns: overflow packing opt" — or, on the server write path, silently fails + and the query times out. Two consequences: a fixture that wants a + multi-kilobyte answer has to build many short strings (the engine harness + uses 15 x 200), and a **real DoT/DoH/UDP client that sends an NSID option + gets a SERVFAIL**, because the engine forwards its own NSID for such a query + and the message will not pack. Not fixed: it needs a library bump or a + deliberate decision to answer NSID locally rather than forward it. +- **`"1Gi"` is not a valid size** — only `GiB`, `G`, `GB`. The parser is + deliberately strict, but k8s spells it `1Gi` and the k8s manifests are the + likeliest place an operator writes a memory budget. Left as is and pinned by + a test; worth a decision. +- **`metrics.OutcomeLocal` has no producer.** The engine reports a local-zone + answer as `ActionAllow`, which `finish` maps to the *upstream* outcome, so + locally answered queries inflate the upstream counter and the dashboard's + local column is always empty. Not changed: fixing it moves published + counters, so it wants a decision and a changelog note. ## Continuation plan, in order -1. **`internal/engine` suite** — the largest uncovered surface and the one every +1. ~~**`internal/engine` suite**~~ — **done**, 23 tests. The original ask was + the largest uncovered surface and the one every packet touches. Pin the pipeline order end to end: ACL deny before anything else, local zone before blocklist, blocklist before cache, a blocklist hit served and cached without touching the upstream, the concurrency and @@ -169,19 +260,25 @@ greenfield rather than an extension of an existing suite. hooks fire exactly once per query and not at all for a query rejected before the pipeline reached them. Reuse the harness in `internal/app/integration_test.go`. -2. **`internal/api` suite** — every route, the three auth modes, the constant-time - API-key comparison, `X-Forwarded-For` honoured *only* from `trusted_proxies`, - CORS, rate limiting, and the SSE stream (open a subscriber, assert delivery, - unsubscribe, assert no goroutine leak). -3. **`internal/server` suite** — all four listeners behind one handler, +2. ~~**`internal/api` suite**~~ — **done, partly** (78 cases): the route table, + the three auth modes, the hashed-key invariant, `trusted_proxies` and + loopback trust, read-only keys, `allow_config_write: false`, panic recovery. + **Not covered**, and the harness already carries what they need + (`fake.stream`/`unsub` for SSE, `fake.fail` for error paths, `harness.srv.now` + as an advanceable clock): CORS, the rate limiter, the SSE + open/unsubscribe/no-leak case, `/metrics` (enabled/protected/501), and the + settings PUT/validate/reload happy paths. +3. **`internal/server` suite** — in progress. All four listeners behind one handler, per-listener config, the UDP read-deadline reset, shutdown ordering. -4. **`internal/metrics` + `internal/cluster`** — the collectors, the rolling - rings, the Valkey lease, pub/sub sync, divergence detection. `miniredis` is the - obvious test server. -5. Tests for `config.Size` — the newest parsing code in the repo and the most - likely to be wrong at a boundary like `1.5GiB`. -6. Wire `memory:` into `deploy/k8s/00-config.yaml`, matched to the StatefulSet's - 1Gi limit. +4. **`internal/metrics` + `internal/cluster`** — the collectors are **done** (9 + tests, the metric names pinned as the contract with the dashboards); what is + left is `cluster`: the rolling rings, the Valkey lease, pub/sub sync, + divergence detection. `miniredis` is the obvious test server. +5. ~~Tests for `config.Size`~~ — **done**, and the unit-multiplier bug it was + supposed to catch was real. +6. ~~Wire `memory:` into `deploy/k8s/00-config.yaml`, matched to the + StatefulSet's 1Gi limit.~~ — **done**: `1GiB` limit, `256MiB` reserve, + `384MiB` cache budget. 7. Decide on `KnownFields(true)`. 8. Re-run the console end to end; the API surface has changed several times. @@ -227,4 +324,8 @@ greenfield rather than an extension of an existing suite. After a large edit, `rm node_modules/.tmp/*.tsbuildinfo`. - `vite build` empties `outDir` and **deletes `internal/web/dist/.gitkeep`**. Restore it or the directory disappears from git. +- **`dns.Msg` with a character-string over 255 bytes does not pack** in + miekg/dns v1.1.73 (`TXT` of 4 KiB, any `EDNS0_NSID`). The failure is silent on + a server write path, so it presents as a client-side timeout; build a big + answer out of many strings under 255 bytes instead. - `gofmt -l internal/` is the fastest way to see that a file does not parse. diff --git a/deploy/k8s/00-config.yaml b/deploy/k8s/00-config.yaml index ecb124e..4a4a799 100644 --- a/deploy/k8s/00-config.yaml +++ b/deploy/k8s/00-config.yaml @@ -50,6 +50,22 @@ data: proto: tls server_name: dns.quad9.net + # Memory envelope. The container limit (1Gi, in the StatefulSet) is the + # hard ceiling; this tells the process about it so the collector works to + # a target instead of being OOM-killed. `limit` is the soft GOMEMLIMIT and + # must be at or below the container limit; `reserve` covers what the Go + # heap does not account for — goroutine stacks, runtime metadata, and the + # mmapped cache snapshot and query log. + # + # The cache byte budget is derived (60% of limit - reserve) unless it is + # set explicitly. cache.size below is an *entry count*, so this is what + # actually bounds the cache's memory: 500k entries of a few hundred bytes + # each is well past 384MiB, and the byte budget is what will bite first. + memory: + limit: 1GiB + reserve: 256MiB + cache_max_bytes: 384MiB + cache: enabled: true size: 500000