diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..0f7900e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,178 @@ +# AGENTS.md — working on fancy-dns + +Project-specific knowledge for anyone (human or agent) changing this repo. +General state and the continuation plan live in `STATUS.md`; read that first for +what is done and what is next. This file is the part that should *not* change +every week: the decisions, the invariants, and the rules that have been learned +the hard way. + +## What this is + +A recursive DNS server in Go with an embedded React console. Config is YAML plus +RFC 1035 master files, hot-reloaded over SIGHUP. Blocklists in four formats, a +sharded LRU cache with a bbolt snapshot for warm restart, per-client ACLs, +query logging, Prometheus metrics, and optional Valkey/Redis for cross-instance +sync. + +## Layout + +``` +cmd/fancy-dns/main.go entrypoint: flags, signals, SIGHUP reload +internal/ + 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) + cache/ Key, Entry, sharded LRU, TTL policy, serve-stale, bbolt Store + resolver/ Upstream pool: health, circuit breakers, parallel/failover/random + acl/ compiled groups, CIDR/PTR/tag matching, per-group domain policy + localzone/ authoritative data from master files, wildcards, delegations + engine/ THE QUERY PIPELINE, reverse-PTR cache + server/ UDP, TCP, DoT, DoH listeners; all funnel into one handler + qlog/ bbolt query-log ring + live in-memory ring + SSE fanout + 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, memory envelope + web/ go:embed of the built console +web/ Vite + React + TS + Tailwind console +deploy/ example config, example zones, k8s manifests +``` + +### Query pipeline order (engine.Handle) + +``` +validate → ACL deny → per-client concurrency → global concurrency → rate limit + → group allow/deny domains → local zone → blocklist → cache → upstream + → cache put → finalize (EDNS0, UDP truncation) → metrics + qlog +``` + +Cache is a single shard-local lookup; only a miss touches the network. + +## Decisions already made — do not relitigate + +| Decision | Choice | Why | +|---|---|---| +| Scope | Full production | clustering, metrics, ACLs, query logs, hot reload, Docker, k8s, CI | +| Config store | YAML + DNS master files | reviewable/diffable in git | +| Cluster sync | Valkey (Redis protocol) | simple, mature Go client, easy local dev | +| Secure DNS | DoT + DoH, **no DNSSEC validation** | deliberately out of scope; AD/CD/extended-RCODE are passed 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 | +| Scale-out | StatefulSet, **no HPA** | an HPA cannot drive a StatefulSet; scale by adding replicas | +| Ingress | A separate ClusterIP Service | a headless Service has no virtual IP for an Ingress to route to | +| Profiling | pprof refused unless admin is loopback | the handlers sit outside the API's auth middleware | + +## Invariants + +Break these and you break the design, not just the test. + +- **Everything the query path reads is behind an `atomic.Pointer` holder** — ACL, + blocklist, localzone, upstream pool. Readers load the pointer once and work + against a value that cannot change under them. **A lock on the hot path is a + regression.** Prefer building a new immutable snapshot and swapping the + pointer. +- **Cache entries are 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. +- **The cache is bounded by bytes as well as by entry count.** `cache.size` alone + bounds nothing: one cached ANY or TXT answer can be tens of kilobytes. +- **Serve-stale** answers are served with TTL 1. An expired upstream answer is + never cached. A truncated answer is never cached — it would be served + truncated forever. +- **The UDP read deadline is reset every iteration.** A deadline set once expires + for good and turns the loop into a spin that never reads another packet. +- **Only the Valkey lease holder fetches blocklists**, so N instances do not hit + a mirror N times. Followers serve their compiled copy. +- **Blocklist changes flush the cache** rather than guessing which subtrees are + affected (`invalidateBlocklistCache()` in `internal/app/hooks.go`). +- **API keys are compared by SHA-256 hash, constant-time**, and + `X-Forwarded-For` is honoured *only* from `trusted_proxies`. Honouring it from + anyone lets a client spoof its identity past the ACL and the rate limiter. +- **The API response field names are shared with the frontend.** If you rename + one, grep `web/src` too. +- **`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. +- **The Go heap does not account for goroutine stacks, runtime metadata, or the + bbolt mmap.** `memory.reserve` exists to cover them; the cache snapshot and + query log are mmapped and live outside the heap entirely. +- **Unknown config keys are silently ignored** (a known open decision — + `yaml` `KnownFields(true)` would reject them). If a config key seems to have no + effect, check the spelling against the struct tags before anything else. + +## Working rules + +**Never leave the tree red.** You have a hard budget and no way to detect you are +close to it. Whatever is on disk at the end of any round must compile and pass. +Get a few tests running early and keep them running. If you are running long, +stop adding tests and make the existing ones pass — a smaller green suite is worth +far more than a larger red one. + +**Scope your edits.** Check `git status` before you start; other work may be in +flight. Stage only the packages you touched. `.codesearch/` is tracked but +gitignored — never stage it. If you edit a package someone else is also editing, +say so in your report rather than silently overwriting. + +**Verify before you claim.** `make check-go` is the fast loop (gofmt, vet, +compile, ~1s). `make check` adds the full test suite. Push only a green tree. + +**Commit and push when the task says so.** Read `git log --oneline -5` and +`git show --stat HEAD` first and match the style: an imperative subject line, then +one paragraph per bug covering root cause and fix. A commit that says *what* was +changed without saying *why* is not finished work. + +**When a test fails, decide which of three things is wrong before editing +anything:** the test, the implementation, or the specification. All three happen +here. Say which one you concluded and why. Do not relax an assertion to make a +suite pass — if an existing assertion pins buggy behaviour, change it and say so +explicitly in the commit message, because that decision belongs in the history. + +**Report honestly.** If you did not finish, say what is unfinished and what state +the tree is in. A partial result that is accurately described is usable; a +partial result described as done is not. + +## Test-hygiene traps + +- `dns.NewRR` needs a TTL column: `dns.NewRR("host.example.com")` does not parse. + A failure there is a bad fixture, not a product bug. +- A test that closes a stop channel *after* `wg.Wait()` deadlocks the whole + package and looks like "the suite hangs". Close first, then wait. +- Assert on invariants, not on what a concurrent goroutine happened to observe. + "A reader must have seen address X" is a flake; "every list a reader sees is one + of the published ones" is a real property and cannot flake. +- Spin up `dns.Server` fakes on `127.0.0.1:0`: `net.Listen`/`net.ListenPacket` to + find the port, `ActivateAndServe` in a goroutine, stop in `t.Cleanup`. +- `internal/app/integration_test.go` already builds a fully wired app. Reuse its + harness rather than rebuilding one. +- `tsc -b` replays **cached diagnostics** from `node_modules/.tmp/*.tsbuildinfo`. + After a large frontend edit: `rm node_modules/.tmp/*.tsbuildinfo`. +- `vite build` empties `outDir` and **deletes `internal/web/dist/.gitkeep`**. + Restore it or the directory disappears from git. +- The Go binary embeds `internal/web/dist`, so `npm ci && npm run build` must run + before a fresh clone will build a binary with the console in it. +- `make build` depends on `web` and shells out to npm. `make go-build` skips the + console and is what you want in an inner loop. + +## Security-sensitive areas + +These have been wrong before. Changes here need a test that fails without the fix. + +- **Auth** (`internal/api/auth.go`): API key comparison must stay constant-time; + the key is hashed before comparison so the plaintext is not in a long-lived + string. +- **Client identity**: `X-Forwarded-For` is only trusted from `trusted_proxies`, + and `httpRemoteAddr` (used by the DoH handler) deliberately does **not** + consult it at all. Behind an Ingress every DoH client therefore lands in the + proxy's ACL group — fail-closed if that group is restrictive, fail-open if it + is permissive. Pin this behaviour with a test before changing it. +- **Config expansion**: `${VAR}` is expanded in `Load` and an unset variable + with no default is fatal, so a missing Secret is a crash loop rather than an + unauthenticated console. The expansion works inside quoted scalars — that is + the form operators write — and skips YAML comments. Do not "simplify" it into a + plain `os.ExpandEnv`; that silently breaks the API key path. +- **pprof** is loopback-only by construction, so no config flag can turn it into + a network-reachable unauthenticated endpoint. Keep it that way. +- **Amplification**: the DoH/UDP handlers cap the client's advertised EDNS0 + buffer. Do not remove the cap. diff --git a/STATUS.md b/STATUS.md index bbfc956..ef71092 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,7 +1,7 @@ # fancy-dns — development status and continuation plan -Last updated after the env-expansion fix, the memory envelope and `make -check-go`. Everything below reflects the tree at `4c00bb4`. +Last updated when the whole tree went green. `AGENTS.md` holds the decisions, +invariants and working rules; this file holds state and the plan. ## Goal @@ -27,6 +27,7 @@ A lightweight, high-performance recursive DNS server with: | 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 | +| Scale-out | StatefulSet, no HPA | an HPA cannot drive a StatefulSet | ## Package layout @@ -65,49 +66,35 @@ Cache is a single shard-local lookup; only a miss touches the network. ## Status -### Green +### Green — the whole tree passes `gofmt` clean, `go build ./...` clean, `go vet` clean. -`go test -race ./internal/... -count=1` is green for **acl, app, blocklist, cache, -config, qlog, resolver, zonefile**. +`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. -`make check-go` is the inner loop (1.3 s): gofmt check, vet, compile. It uses +`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`. -### 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.) +`api`, `cluster`, **`engine`**, `metrics`, `server`, `web`. -### Fixed earlier and worth not re-breaking +`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. +### Fixed recently and worth not re-breaking + +- **Every relative RDATA name in a master file pointed at the DNS root.** The + qualification pass ran *after* `dns.NewRR`, and the library resolves RDATA + names against the root, so `www CNAME host` became `CNAME host.` — the alias + pointed at the root, and so did every relative NS, MX, PTR, SRV, NAPTR and SOA + target in every zone file. It is now a token-level pass driven by a table of + which RDATA positions hold a name; dotted names stay absolute, so the RPZ + targets `.` and `rpz-drop.` and the null MX/SRV targets are untouched. - **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. @@ -119,28 +106,34 @@ config, qlog, resolver, zonefile**. 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 + `"${FANCYDNS_API_KEY}"`. The k8s manifest 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 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. +- **The ACL `*` catch-all was dropped** from the rendered domain patterns, so the + engine's per-query recompile lost the "everything" case. +- **`Upstream.Success` never closed an open breaker.** A recovered upstream 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`**, because the + parallel path was only taken when exactly one upstream was chosen. +- **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, wired to + `debug.SetMemoryLimit`. The reserve covers what the Go heap does not account + for: goroutine stacks, runtime metadata, and the bbolt mmap. - **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. +- **`$TTL` was ignored** by the zone parser; every record got the SOA minimum. +- **`TestRebindUnderConcurrentQueries` was a flake** (2 runs in 10) — it asserted + a reader had to observe two specific addresses, which is a claim about what a + goroutine happened to see. It now pins the copy-on-write invariant itself: + every observed snapshot must equal one of the published lists. - **`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`. @@ -149,19 +142,16 @@ config, qlog, resolver, zonefile**. - **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. + kustomization. The HPA is gone. - **`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. ### Open decisions - **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. + wants `blocklist:` produced a plausible-looking server, and two test fixtures + were caught by this rather than by what they were testing. `KnownFields(true)` + would reject it, but that is a behaviour change for anyone carrying extra keys. - **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 @@ -170,17 +160,23 @@ config, qlog, resolver, zonefile**. ## Continuation plan, in order -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, 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, +1. **`internal/engine` suite** — 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 + rate-limit refusals, serve-stale with its single background refresh, + truncation against a 512-byte client buffer, and that the metrics and qlog + 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). -4. `internal/metrics` + `internal/cluster`: the collectors, the rolling rings, - the Valkey lease, pub/sub sync, divergence detection. `miniredis` is the +3. **`internal/server` suite** — 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`. @@ -215,21 +211,20 @@ config, qlog, resolver, zonefile**. - **`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 +- **pprof is loopback-only by construction**, not by policy, so no config 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. +- `dns.NewRR` needs a TTL column: `dns.NewRR("host.example.com")` does not + parse. That is a fixture bug, not a product bug. +- A test that closes a stop channel *after* `wg.Wait()` deadlocks the whole + package and looks like "the suite hangs". Close first, then wait. +- Assert on invariants, not on what a concurrent goroutine happened to observe. + "A reader must have seen address X" is a flake; "every list a reader sees is one + of the published ones" is a real property and cannot flake. - `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. -- `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. +- `gofmt -l internal/` is the fastest way to see that a file does not parse.