diff --git a/STATUS.md b/STATUS.md index bcc667f..bbfc956 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,7 +1,7 @@ # fancy-dns — development status and continuation plan -Last updated after the bbolt/blocklist/acl/resolver fix session and the first -end-to-end run of the console. Everything below reflects the working tree. +Last updated after the env-expansion fix, the memory envelope and `make +check-go`. Everything below reflects the tree at `4c00bb4`. ## Goal @@ -26,13 +26,14 @@ A lightweight, high-performance recursive DNS server with: | Secure DNS | DoT + DoH, no DNSSEC validation | avoids signer/anchor machinery; pass-through only | | Hot cache | Sharded LRU in memory, bbolt for snapshots | no I/O on the hot path; warm restart | | k8s networking | No hostNetwork, LoadBalancer Service | the server never sets SO_REUSEPORT, so two host-network pods cannot share :53 | +| Memory | Soft GOMEMLIMIT, cache byte budget | a cgroup kill is an outage; a cache miss is not | ## Package layout ``` cmd/fancy-dns/main.go entrypoint: flags, signals, SIGHUP reload internal/ - config/ Config struct, Duration type, Validate, atomic Store + hooks, + config/ Config struct, Duration, Size, Validate, atomic Store + hooks, ${VAR} environment expansion in Load zonefile/ RFC 1035 master-file parser ($TTL/$ORIGIN, parens, serial math) blocklist/ Set (exact/sub/root maps), 4 parsers, Manager (fetch+compile+swap) @@ -46,7 +47,7 @@ internal/ metrics/ Prometheus collectors + rolling time-series rings cluster/ Valkey pub/sub, leader lease, divergence detection api/ admin REST + SSE, auth, CORS, rate limiting - app/ wiring, lifecycle, API hook implementations + app/ wiring, lifecycle, API hook implementations, memory envelope web/ go:embed of the built console web/ Vite + React + TS + Tailwind console deploy/ example config, example zones, k8s manifests @@ -64,170 +65,139 @@ Cache is a single shard-local lookup; only a miss touches the network. ## Status -### Done and verified +### Green -- `gofmt -l ./cmd ./internal` empty, `go build ./...` clean, `go vet ./...` clean. -- **`go test -race ./internal/... -count=1` is green** for `acl`, `app`, - `blocklist`, `cache`, `config`, `resolver`, `zonefile`. -- Test suites exist and are green for: `acl` (26 tests + 4 benchmarks), - `blocklist`, `cache` (incl. benchmarks), `config`, `resolver`, `zonefile`. -- `internal/localzone`, `internal/engine` and `internal/qlog` are being written - right now — see "in flight" below. -- `README.md` is being written from the source right now. +`gofmt` clean, `go build ./...` clean, `go vet` clean. -### The four bugs from the first end-to-end run +`go test -race ./internal/... -count=1` is green for **acl, app, blocklist, cache, +config, qlog, resolver, zonefile**. -Found by running the binary and querying it, not by a test. All four are being -fixed in `internal/zonefile` + `internal/localzone`: +`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. +`make check` = `check-go test`. -1. **`$TTL 60` is ignored.** Every record came back with the SOA minimum - (3600). `Zone.DefaultTTL` is documented as the `$TTL` value; RFC 1035 §5.2 - says an explicit `$TTL` wins. -2. **Records are swallowed when the previous record has an odd token count.** - `ns1 A 192.0.2.53` followed by `txt TXT "hello world"` parses as one record - `ns1 TXT "TXT" "hello world"` — the TXT record is lost entirely. Data loss - in `assemble`/`Parse`. -3. **A delegated name answers NXDOMAIN instead of a referral.** With - `sub NS ns1.sub.example.test.`, `host.sub.example.test. A` returned - `NXDOMAIN aa=true`; it must be NOERROR, AA false, NS in Authority. -4. **A wildcard answer keeps the literal wildcard owner.** A query for - `x.wild.example.test. A` returned an answer owned by - `*.wild.example.test.`. RFC 4592 §3.3.1 requires the owner name to be - replaced with the query name. +### Red — one package + +**`internal/localzone`**, 5 failures. One is a real production bug: + +- **A relative CNAME target is emitted unqualified.** A master file line + `www CNAME host` produces `www.example.com. CNAME host.` instead of + `host.example.com.` — the alias points at the DNS root. Per RFC 1035 §5.1 the + target is relative to the zone origin. + + Narrowed, not yet confirmed: `zonefile.assemble` only calls `relative()` on the + *owner* and passes RDATA tokens through untouched, and `qualifyRdata(rr, + z.Origin)` is invoked with the origin that the owner path used successfully. + So the parser looks correct and the defect is more likely in how + `internal/localzone` stores or re-serialises records. **Look there first**; + do not "fix" `qualifyRdata`, it is right. + +- `TestCNAMELoopIsNotFollowed` — bad fixture, not a product bug: + `dns.NewRR("alias.example.com")` has no TTL column so it will not parse. +- `TestLoadOriginDerivation` — a file whose first record has no owner name fails + with `record has no owner name and no previous owner to inherit`; decide + whether the parser should accept it when the origin is known. +- `TestLoadMergesFilesForOneOrigin` — `Count() = 3, want 4`. A record is still + dropped when two files feed one origin. Same class as the bug reproduced by + hand: a record whose first token is consumed as RDATA of the previous one. + +### No test suite yet + +`server`, `api`, `metrics`, `cluster`. (`app` has integration tests that pass; +`engine` and `qlog` now have suites.) ### Fixed earlier and worth not re-breaking -- **`cache.Snapshot`/`Load` never round-tripped.** `Snapshot` packed the whole - key while `Load` decoded the record header out of the *key's* remainder - instead of the value, so all 25 entries were skipped — every restart was a - cold cache. There is now an explicit `encodeRecord`/`decodeRecord` pair with - a version byte and a CRC-32C trailer, so a future format change is detected - rather than silently misparsed. `Load` also drops entries already past - `ExpiresAt` and rewrites the TTL to the time actually left, so a served TTL - never jumps back up across a restart. -- **ACL PTR suffix matched across label boundaries.** A group scoped to - `example.com.` also swallowed `ample.com.` and `notexample.com.` — a client - could land in a group it does not belong to. `matchPTR` now requires the - suffix to start a label, and `ptrSuffixes` normalises the config value to FQDN - form so the check works even for a config that skipped `Validate`. -- **The ACL `*` catch-all was dropped** from the rendered domain patterns, so - the engine's per-query `NewDomainSet` recompile lost the "everything" case. +- **The cache snapshot never round-tripped.** `Snapshot` packed the whole key + while `Load` decoded the record header out of the *key's* remainder instead of + the value, so all 25 entries were skipped — every restart was a cold cache. + There is now an explicit `encodeRecord`/`decodeRecord` pair with a version byte + and a CRC-32C trailer. `Load` drops entries already past `ExpiresAt` and + rewrites the TTL to the time actually left, so a served TTL never jumps back up + across a restart. +- **`${VAR}` expansion never worked inside quotes** — which is the only form + anyone writes. The reference check sat in the same switch as quote tracking, + below the `inDouble`/`inSingle` branches, so `api_keys: ["${KEY}"]` came out of + `Load` unchanged and the admin API key was the literal string + `"${FANCYDNS_API_KEY}"`. The k8s set looked correct and was not. A variable + that is set but empty now expands to empty rather than erroring (the + `${X}` / `${X-y}` / `${X:-y}` distinction); `\${FOO}` is literal. +- **`$TTL` was ignored** by the zone parser; every record got the SOA minimum. +- **ACL PTR suffix matched across label boundaries**, so a group scoped to + `example.com.` also swallowed `ample.com.` — a client could land in a group it + does not belong to. +- **The ACL `*` catch-all was dropped** from the rendered domain patterns. - **`Upstream.Success` never closed an open breaker.** A recovered upstream - stayed excluded for the whole cooldown — the mirror image of the older bug - where a failed exchange marked a dead upstream healthy. -- **`Exchange` fanned out in parallel even under `policy: failover`.** It only - took the parallel path when exactly one upstream was chosen, so a failover - list of two raced both. -- **Blocklist:** a leading UTF-8 BOM is stripped, `!allow example.com` works, - `SourceMeta` exists for a source that only contributed a rule, `Pattern()` - reports the bare name again (`WireName()` produces the wildcard form for the - API), an RPZ record with no explicit TTL inherits the zone default, a hosts - line with no address in the first field says so, the adblock parser handles - CRLF and adblock lines pasted into a plain list, and over-long lines are - reported rather than accepted. -- **`TestHolderSwapIsAtomic` had the wrong invariant**, not the Holder: after a - swap to a set that does not block `a.example.com`, that name is legitimately - unblocked. The swap is a single `atomic.Pointer` store. -- **CI could not have passed.** `go.mod` requires Go 1.27.1; CI pinned 1.23 and - the Dockerfile pinned `golang:1.23-alpine`. Both now track the toolchain. -- **The Dockerfile clobbered the console.** It copied the source tree *after* - the console bundle, and a fresh clone's `internal/web/dist` holds only - `.gitkeep`. -- **The k8s set could not have worked.** `hostNetwork: true` with - `replicas: 2`, and the server never sets SO_REUSEPORT, so the second pod - could not bind :53. It now runs without hostNetwork behind a LoadBalancer - Service, with a separate ClusterIP Service for the Ingress (a headless - Service cannot back one), a ServiceAccount, an API-key Secret, per-pod PVCs, - and a kustomization. The HPA is gone — it cannot drive a StatefulSet. -- **The API key could not come from a Secret**, because the config had no - variable expansion. `config.Load` now expands `${VAR}`, `${VAR:-default}` and - `${NAME-default}` and fails to start on an unset variable with no default, so - a missing secret is a crash loop rather than an unauthenticated console. - Expansion skips YAML comments, so documenting `${VAR}` in the ConfigMap does - not break the file. + stayed excluded for the full cooldown — the mirror of the older bug where a + failed exchange marked a dead upstream healthy. +- **`Exchange` fanned out in parallel even under `policy: failover`.** +- **The cache byte budget was never fed to the LRU.** `maxBytes` was enforced + per shard on insert but had no setter and nothing called one, so `cache.size` + had no predictable memory cost. +- **The memory envelope** (`memory.limit/reserve/cache_max_bytes`) is new. The + reserve exists because the Go heap does not account for goroutine stacks, + runtime metadata, or the bbolt mmap for the cache snapshot and the query log. +- **Blocklist:** leading BOM stripped, `!allow` works, `SourceMeta` exists for a + rule-only source, `Pattern()` reports the bare name (`WireName()` gives the + wildcard form for the API), RPZ inherits the zone default TTL, hosts files + complain about a missing address, adblock handles CRLF and over-long lines. +- **`TestHolderSwapIsAtomic` had the wrong invariant**, not the Holder. +- **CI could not have passed:** `go.mod` requires Go 1.27.1, CI pinned 1.23 and + the Dockerfile pinned `golang:1.23-alpine`. +- **The Dockerfile clobbered the console** — it copied the source tree after the + console bundle. +- **The k8s set could not have worked:** `hostNetwork: true` with `replicas: 2` + and no SO_REUSEPORT. Now a LoadBalancer Service, a separate ClusterIP Service + for the Ingress, a ServiceAccount, an API-key Secret, per-pod PVCs, a + kustomization. The HPA is gone — it cannot drive a StatefulSet. +- **`serveUDP` called `net.ResolveUDPAddr` per packet**; the peer already is a + `*net.UDPAddr`. +- **`api.pprof`** mounts `net/http/pprof`, refused unless every admin address is + loopback — the handlers sit on the app-level mux and would bypass the API's own + auth. -### Memory envelope (added) +### Open decisions -`memory.limit` / `memory.reserve` / `memory.cache_max_bytes` are new. `limit` -becomes a `GOMEMLIMIT` via `debug.SetMemoryLimit`, reduced by `reserve`; the -cache byte budget is wired into the LRU, which already enforced `maxBytes` per -shard but had no setter and was never fed one. - -Rationale: a cgroup OOM kill is the worst outcome available to a resolver — an -outage rather than a cache miss, with no chance to serve stale on the way out. -`GOMEMLIMIT` turns the same limit into a soft one the GC works towards. The -reserve exists because the Go heap does not account for goroutine stacks, -runtime metadata, or the bbolt mmap backing the cache snapshot and the query log -— handing the runtime the full cgroup limit is how you get killed by a mechanism -that believed it had headroom. - -`ParseSize` accepts `512MiB`, `2GiB`, `1000` (bytes), and is strict about junk -(`512MiBx` is an error). `config.Size` has no test coverage yet — **that is the -first thing to add**. - -### Known red - -- `internal/config`: `TestLoadUnsetEnvIsFatalAndNamesVariable` fails. The - fixture now has a blocklist entry, so `Validate` passes, but the expected - `${FD_TEST_MISSING}` expansion error never fires — `Load` returns the - *Validate* error instead. `TestExpandEnvErrors` covers the same - reference-in-a-flow-sequence case and passes, so the difference is specific to - the quoted, flow-sequence form `api_keys: ["${X}"]`. Undiagnosed; do not - assume the expander is fine. -- `internal/qlog` and `internal/localzone` have test files mid-write from - subagents that ran out of budget. `internal/qlog` does not compile - (`internal/... /qlog.go:520: undefined: liveEntry`, `viewsFor: undefined: - Resize`). Fix that before anything else in those packages. - -### Open decision - -- **Unknown config keys are silently ignored.** Writing `block:` where the - struct wants `blocklist:` produced a plausible-looking server instead of an - error. `yaml.Decoder.KnownFields(true)` would reject it, but that is a - behaviour change for anyone carrying extra keys in their config. Not done - unilaterally. - -### No tests yet for - -`server`, `api`, `metrics`, `cluster`. `app` has integration-level tests that -pass; the rest have no suite at all. - -### In flight (subagents — do not edit these files) - -- `internal/zonefile` + `internal/localzone` — repair the truncated - `localzone_test.go` and fix the four bugs above. -- `internal/engine` + `internal/qlog` — first suites for the query pipeline and - the query log. -- `README.md` — written from the source, with a "known inconsistencies" list. - -Still to delegate after those: `server` + `api`, then `metrics` + `cluster`. +- **Unknown config keys are silently ignored.** Writing `block:` where the struct + wants `blocklist:` produced a plausible-looking server. `KnownFields(true)` + would reject it, but that is a behaviour change for anyone carrying extra + keys. Two test fixtures were caught by this, not by the thing they tested. +- **DoH clients behind an Ingress all present as the proxy.** `httpRemoteAddr` + uses the raw `RemoteAddr` and never consults `trusted_proxies`, so every DoH + 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. ## Continuation plan, in order -1. Land the three in-flight pieces, then re-run - `go test -race ./internal/... -count=1 -timeout 300s`. +1. **Unblock `internal/localzone`.** Start with how the Store re-serialises + records; the parser side looks correct. Then the fixture, the origin + derivation and the merge count. 2. `internal/server` suite: all four listeners behind one handler, per-listener - config, the UDP read-deadline reset (see the design notes), shutdown. -3. `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). + config, the UDP read-deadline reset, shutdown ordering. +3. `internal/api` suite: every route, the three auth modes, 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). 4. `internal/metrics` + `internal/cluster`: the collectors, the rolling rings, - the Valkey lease, pub/sub sync, and divergence detection (a `miniredis` - instance is the obvious test server). -5. Decide on `KnownFields(true)`. -6. Re-run the console end to end (`/api/healthz`, `/api/whoami`, a page from - each route) — it was verified once and the API surface has changed since. + 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. +7. Decide on `KnownFields(true)`. +8. Re-run the console end to end; the API surface has changed several times. ## Design notes worth preserving - **Cache stores packed wire bytes, not `*dns.Msg`.** A hit costs one `Unpack` and yields a private copy, so serving never mutates shared state and TTL ageing is free. Same trade dnsmasq makes. -- **Everything the query path reads is behind an `atomic.Pointer` holder** — - ACL, blocklist, localzone, and now the upstream pool. Readers load the pointer - once and work against a value that cannot change under them. Prefer a new - immutable snapshot over a lock. +- **Everything the query path reads is behind an `atomic.Pointer` holder** — ACL, + blocklist, localzone, and now the upstream pool. Readers load the pointer once + and work against a value that cannot change under them. Prefer a new immutable + snapshot over a lock. - **Cache invalidation on rule changes flushes the cache** rather than guessing which subtrees are affected. `invalidateBlocklistCache()` in `internal/app/hooks.go`. @@ -235,25 +205,31 @@ Still to delegate after those: `server` + `api`, then `metrics` + `cluster`. rename one, grep the frontend too. - **API keys are compared by SHA-256 hash, constant-time**, and `X-Forwarded-For` is honoured *only* from `trusted_proxies`. -- **UDP listener resets its read deadline every iteration.** A deadline set once - expires permanently and turns the loop into a spin that never reads another +- **The UDP listener resets its read deadline every iteration.** A deadline set + once expires for good and turns the loop into a spin that never reads another packet (caught by `TestBlocklistToggle`). - **Only the Valkey lease holder fetches lists**, so N instances do not hit a - mirror N times. Followers serve their compiled copy. -- **Blocked/serve-stale behaviour:** a served-stale answer gets TTL 1, an - expired upstream answer is never cached, and a truncated answer is never - cached (it would be served truncated forever). -- **`PatternKind.Pattern` is the display form; `WireName` is the wildcard - form.** The internal `*.` / `**.` prefixes must not leak into the UI, but the - API needs them to re-add a rule unchanged. + mirror N times. +- **Serve-stale** answers get TTL 1; an expired upstream answer is never cached; + a truncated answer is never cached (it would be served truncated forever). +- **`PatternKind.Pattern` is the display form; `WireName` is the wildcard form.** + The internal `*.` / `**.` prefixes must not leak into the UI, but the API + needs them to re-add a rule unchanged. +- **pprof is loopback-only by construction**, not by policy, so no profiler flag + can turn it into a network-reachable unauthenticated endpoint. ## Test-hygiene traps +- **Subagent briefs must require "whatever is on disk when you run out of + budget must compile and pass".** Three of five agents left a red tree; the + check has to be in the brief, because an agent cannot know it is out of budget. - `tsc -b` replays **cached diagnostics** from `node_modules/.tmp/*.tsbuildinfo`. 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. - A test that closes a stop channel *after* `wg.Wait()` deadlocks the whole package and looks like "the suite hangs". Close first, then wait. -- An agent that runs out of budget mid-append leaves a file that does not - parse. `gofmt -l` on the package is the fastest way to see it. +- `gofmt -l internal/` is the fastest way to see that an agent left a file that + does not parse. +- `dns.NewRR` needs a TTL column. `dns.NewRR("host.example.com")` does not + parse; that is a fixture bug, not a product bug.