From 2b844742281eaceae69aac80a73647806817b058 Mon Sep 17 00:00:00 2001 From: developer Date: Sun, 27 Sep 2026 13:32:17 +0000 Subject: [PATCH] Rewrite STATUS.md to match the tree after the bbolt/blocklist/acl/resolver fixes Records the four bugs the first end-to-end run surfaced, what is green, what is still without a suite, and what is in flight, so the next session does not start from the previous plan. --- STATUS.md | 284 ++++++++++++++++++++++++------------------------------ 1 file changed, 128 insertions(+), 156 deletions(-) diff --git a/STATUS.md b/STATUS.md index bcfa032..0e95299 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,8 +1,7 @@ # fancy-dns — development status and continuation plan -Last updated: end of the race-fix / test-coverage session. Everything below -reflects the working tree, including the known-failing tests and the exact -commands to reproduce them. +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. ## Goal @@ -26,13 +25,15 @@ A lightweight, high-performance recursive DNS server with: | Cluster sync | Valkey (Redis protocol) | simple, mature Go client, easy local dev | | 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 | ## 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 type, 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 @@ -65,156 +66,125 @@ Cache is a single shard-local lookup; only a miss touches the network. ### Done and verified -- `go build ./...`, `go vet ./...` and `gofmt -l` are all clean. -- **`go test -race ./internal/app/` is green — zero data races.** The two - `-race`-only failures and all ten reported races are fixed (see below). -- `go test ./internal/zonefile/` is green (the full parser suite). -- Frontend **builds and type-checks**: `npx tsc -b --noEmit` and `npm run build` - both exit 0, `npx oxlint` reports 0 errors / 10 warnings, and the real bundle - lands in `internal/web/dist/`. The `//go:embed all:dist` picks it up with no - Go change. -- One real frontend bug fixed along the way: four pages used a `toast(string)` - duck-typed shim while `useToast()` takes `ToastOptions`, so every mutation - toast rendered an empty title. +- `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. -### Fixed this session (races and their root causes) +### The four bugs from the first end-to-end run -All of these were found by `-race` and fixed at the source, not with extra -locking: +Found by running the binary and querying it, not by a test. All four are being +fixed in `internal/zonefile` + `internal/localzone`: -1. **`cache.Entry.Hits` → `atomic.Uint32`.** A counter on an entry shared by - every reader raced the LRU walk used by the API. The struct is now never - copied by value. -2. **`LRU.perShard` → `atomic.Int64`.** `SetCapacity` (a config reload, from the - API goroutine) rewrote the per-shard budget while every query shard was - inserting. -3. **`LRU.SetCapacity` now evicts.** Shrinking the budget changed the number but - left the old entries resident until they happened to be overwritten; a - reload that lowers `cache.size` could not actually reclaim memory. It now - trims each shard to the new budget and counts the evictions. -4. **`resolver.Pool` rewritten as copy-on-write.** This was the bulk of the - races: `Rebind` mutated `p.ups` / `p.cfg` under a mutex that the query path - never took, so `Exchange`, `Healthy`, `All` and the `dns.Client` memo map - were all racing. The pool now holds an immutable `poolState` behind - `atomic.Pointer`; `Rebind` builds the next state and swaps it with one - store. `Upstream` is immutable and holds a shared `*upstreamStats`, so the - live health, latency and breaker state of an unchanged server survives a - reload while a query in flight keeps a consistent view. -5. **`resolver.Pool.Exchange` no longer marks dead upstreams healthy.** A - pre-existing bug: `exchangeOne` called `up.Success(rtt)` before checking the - error, so a dead upstream was marked up, had its consecutive-failure count - reset and had `LastError` cleared on every failed exchange — **the circuit - breaker could never trip through the query path.** -6. **`qlog` live ring → `[]atomic.Pointer[liveEntry]`.** `Log` wrote - `l.live[pos%len]` from every query goroutine while `Recent` walked the ring - under a read lock the writers never took. Publishing is now one atomic word. -7. **`blocklist.Holder.Get` is nil-safe**, and `Builder` keeps an `ordered` set - so a source appears once in `Sources()` whether it was added by a rule or by - `NoteSource` (it used to be appended twice). -8. **`cache.packKey` fully qualifies the name before packing.** Keys are stored - canonically (lowercase, no trailing dot) but the wire format needs a - trailing dot, so `dns.PackDomainName` rejected every key. +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. -### Known failures (fix these next) +### Fixed earlier and worth not re-breaking -Run: `go test -race -timeout 120s ./internal/... -count=1` +- **`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. +- **`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. -1. **bbolt snapshot round trip is broken — the warm-restart path does not work.** - `internal/cache/store.go` `Snapshot` writes 25 entries and `Load` skips all - 25 (`TestStoreSnapshotAndLoad`, `TestStoreLoadSkipsExpiredEntries`, - `TestStoreTruncateAndStats` show `loaded = 0, skipped = N`). The write side - and the read side disagree about the record layout — most likely the - `binary.Uvarint` header that `Load` decodes (`expUnix`, `staleUnix`) is not - what `Snapshot` emits. **This is the highest-value remaining bug**: without - it every restart is a cold cache. Start at `Snapshot` (around the `put` - call) and `Load` (`internal/cache/store.go:78`). -2. **`internal/blocklist` — 5 failing tests, all real implementation gaps**, not - test bugs. Do not "fix" them by relaxing the assertions; they are the spec: - - `TestSetIntrospection`: `SourceMeta("b")` is missing for a source that was - only ever mentioned by a rule, and `Pattern()` reports the internal `*.` / - `**.` form where the caller wrote a bare domain. - - `TestHolderSwapIsAtomic`: reports a half-applied set. The swap is a single - `atomic.Pointer` store, so either the test's invariant is wrong or - `Holder` publishes before the set is fully built — check which before - changing anything. - - `TestParseHosts`: a hosts line whose first field is not an address should - emit the notice "no address in first field, falling back to a plain - domain list"; it silently produces nothing. - - `TestParseRPZ`: bare RPZ owner names are sniffed as `FormatPlain` (the - format sniffer only knows the classes `IN`/`CH`/`HS`), and the - no-subdomain form is compiled as a wildcard. - - `TestParseRobustness`: CRLF input to the adblock parser produces no rules - at all, and an over-long line is accepted instead of rejected. - - Also unreported by a test but found by inspection: no parser strips a - leading UTF-8 BOM, and the documented inline allow marker - `!allow example.com` does not work (only `!example.com` does). -3. **No tests yet for `acl`, `localzone`, `resolver`, `config`, `qlog`, - `engine`, `server`, `api`, `metrics`, `cluster`.** `internal/acl` and - `internal/localzone` were started but the agent ran out of budget; - `internal/resolver` was not started at all. +### Open decision -### Not started / incomplete deliverables +- **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. -- **`README.md` is still a 0-byte file.** Everything else in the deploy set - exists (written in a parallel pass) and is tracked: `Makefile` (4 KB), - `Dockerfile`, `.gitignore`, `.github/workflows/ci.yml` (6 KB), - `deploy/config.example.yaml`, `deploy/local.example.zone`, `deploy/zones/`. - None of it has been *reviewed against the code* — the CI workflow and the - Makefile targets (`make web`, `make race`, `make image`) need a - `make -n`/dry-run pass, and every config key, port and CLI flag the docs - mention has to be checked against `internal/config` and `cmd/fancy-dns`. -- **`deploy/k8s/` has only two of the seven expected manifests** - (`00-config.yaml`, `10-deployment.yaml`). Still missing: Service, PVC, - ServiceAccount + Secret (the API key), Ingress (needs - `proxy_buffering off` for the SSE log stream), namespace and kustomization. -- End-to-end verification of the console against a running binary: the bundle - compiles and embeds, but nobody has clicked through the pages. -- `internal/web/dist` is gitignored except `.gitkeep`, so a fresh clone cannot - build the binary without running `npm ci && npm run build` first. Either CI - builds the bundle before `go build`, or the ignore rule comes off. +### 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`. ## Continuation plan, in order -### Step 1 — the bbolt round trip (highest priority) - -```bash -go test -race ./internal/cache/ -run 'TestStore' -count=1 -v -``` - -Read `Snapshot` and `Load` in `internal/cache/store.go` as a pair and make the -record format explicit — a small `encodeRecord`/`decodeRecord` pair with one -version byte, so a future change cannot silently desync them the way this one -did. Then add a `t.TempDir()` round-trip test that survives a process restart -and a partially written file. - -### Step 2 — the five blocklist gaps - -Fix the implementation against the existing assertions (see item 2 above), then -`go test -race ./internal/blocklist/ -count=1` to green. The BOM strip and the -`!allow` marker are the two an operator is most likely to hit. - -### Step 3 — the remaining unit tests - -Same checklist as before, minus what is now covered (`blocklist`, `cache`, -`zonefile` have suites; `cache` and `blocklist` have benchmarks). Priority: -`resolver` (breaker open/close, parallel race, TCP fallback on truncation, -failover, `Rebind` under `-race` — with a `dns.Server` fake on `127.0.0.1:0`), -`acl` (first-match-wins, most-specific CIDR, PTR suffix, `DomainSet` wildcard -forms), `localzone` (delegation cut, closest-wildcard per RFC 1034 §4.3.3, -NODATA vs NXDOMAIN), `config` (`Duration` parsing, `Validate`, the store hooks). - -### Step 4 — review the deploy set and write the README - -The console pages exist and build; verify them in a browser against a running -binary. Then read `Makefile`, `Dockerfile` and `.github/workflows/ci.yml` -against the code and fix what does not match, add the missing k8s manifests -(Service, PVC, ServiceAccount + Secret, Ingress with `proxy_buffering off` for -`/api/logs/stream`, namespace, kustomization), and write `README.md` from the -source: config reference, zone-file semantics, blocklist formats, API routes, -the `trusted_proxies` / `metrics.protect` / `auth.mode` security caveats, and -the limitations (no DNSSEC validation, no zone transfer, no dynamic update). -CI must run `npm ci && npm run build` before `go build`, because the Go build -embeds `internal/web/dist`. +1. Land the three in-flight pieces, then re-run + `go test -race ./internal/... -count=1 -timeout 300s`. +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). +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. ## Design notes worth preserving @@ -222,33 +192,35 @@ embeds `internal/web/dist`. 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. This is - the pattern the whole race cleanup settled on: prefer a new immutable - snapshot over a lock. + 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. A newly blocked name must not keep serving its - cached good answer. `invalidateBlocklistCache()` in `internal/app/hooks.go`. + which subtrees are affected. `invalidateBlocklistCache()` in + `internal/app/hooks.go`. - **API responses share the field names in `internal/app/hooks.go`.** If you 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` — otherwise a - client could spoof its identity past the ACL and the rate limiter. -- **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 packet (a real bug caught by `TestBlocklistToggle`). + `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 + 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. ## Test-hygiene traps - `tsc -b` replays **cached diagnostics** from `node_modules/.tmp/*.tsbuildinfo`. - After a large edit, `rm node_modules/.tmp/*.tsbuildinfo` or you will debug - errors that no longer exist. + 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.