Wire the memory envelope into the k8s ConfigMap; bring STATUS.md up to date
build / build-push (push) Failing after 19s
build / build-push (push) Failing after 19s
The container limit is 1Gi, so declare the envelope next to it: a 1GiB soft heap limit, a 256MiB reserve for what the Go heap does not account for, and an explicit 384MiB cache byte budget. cache.size is an entry count, so the byte budget is what actually bounds the cache now that ParseSize applies its units. STATUS.md records the round: the fixes, the new suites (engine, config.Size, metrics, and the api suite that landed separately), three findings left unfixed -- the login rate limiter never applies to the login endpoint, auth configuration is frozen at New, and api.constantTimeEqual is dead -- and the continuation plan with items 1, 2, 5 and 6 struck.
This commit is contained in:
1 parent
4f49033f1b
commit
cc7466823b
2 files changed
+135
-18
No files matched your search
@@ -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.
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user