developer
b6d70a50fe
build / build-push (push) Successful in 22s
internal/server was the last package whose shutdown was tested by assumption: four transports, three shutdown shapes, and every one of them can strand a goroutine that a client is holding open. Both bugs below were live in the tree as handed over and neither had a test. An idle client could hold the whole shutdown open. The stream accept loop waits for its connections before Stop returns, and a connection parked in ReadMsg with no deadline in front of it only ends when the client goes away — ctx.Err() is checked between reads, so cancelling the context cannot reach a read that is already blocked. One connection that connected and said nothing was enough to make Stop sit there for the caller's entire grace period (15s in main) and then return context.DeadlineExceeded, which the app reports as "shutdown completed with errors"; and the cache snapshot the app takes right after Stop never happened, because the orchestrator had given up first. It is a shutdown DoS that costs the attacker one connection. The fix is a read deadline in the past on every accepted connection when the context is cancelled, and deliberately not a close: a query already inside the pipeline is past its read, so its answer still goes out, which is the invariant TestInFlightQueryIsAnsweredAcrossStop pins. The accept loop keeps a small registry of live connections for this — the accept path, not the query path, and no query ever takes that mutex. A UDP query in flight at Stop was lost and nothing waited for it. The stream paths register their connections with s.wait; servePacket did not register at all, so a UDP query already inside the pipeline was gone twice over: nothing waited for it, and the answer had nowhere to go once it was ready, because a datagram is written to the very socket the query arrived on. The visible damage was not the lost answer (the client retries) but the app, which snapshots the cache and closes the store the moment Stop returns — a query still in the pipeline could be writing to a store on its way out. Packets are now counted in s.wait, and the UDP listener's close wakes the read loop with an expired deadline and then waits, bounded by udpDrainGrace (250ms), for the loop and the packets it spawned before closing the socket. The bound is deliberate: a shutdown may not spend its grace period on one slow upstream, and past the bound the answer is dropped, which is what the client would see anyway. Stop returning did not mean the process was done, so nothing could tell a leaking shutdown from a clean one. TestStopLeavesNoGoroutineRunningServerCode counts goroutines whose stack still has a frame in the package — not runtime.NumGoroutine(), which in a test binary is net/http, crypto/tls and the race detector talking over each other — and it found the idle-connection bug above. TestStopIsNotBlockedByAnIdleStreamConnection sets a 60s DoT idle timeout on purpose so a config value cannot hide the regression behind a lucky tick, and TestInFlightQueryIsAnsweredAcrossStop now covers all four transports. harness.idleStream exists because a test that only dials a stream transport races the accept loop: Stop can close the listener while the connection is still in the backlog, the server never accepts it, and a shutdown assertion that needs a live server-side goroutine passes for the wrong reason. AGENTS.md gains the two durable invariants (no step in the shutdown may wait for a client; every query goroutine is registered with s.wait, UDP included) and the two test-hygiene traps. STATUS.md drops the UDP query lost at Stop from its pinned list, since it is now a test rather than an accident, and the plan starts at the pinned decisions that are actually left.
224 lines
13 KiB
Markdown
224 lines
13 KiB
Markdown
# 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.
|
|
- **No step in the shutdown may wait for a client.** `Stop` is: cancel, close
|
|
every listener (not interruptible — releasing the ports is the work the next
|
|
process start needs), then wait for the goroutines (interruptible by the
|
|
caller's context). The wait is the step that breaks when something depends on
|
|
a client: a connection parked in a read with no deadline in front of it never
|
|
notices the cancellation, and one idle connection is enough to spend the whole
|
|
grace period and then report a context error. Reach those connections with a
|
|
read deadline in the past, **not** a close — a query already inside the
|
|
pipeline is past its read, and its answer still has to go out.
|
|
- **Every query goroutine is registered with `s.wait`, UDP included**, so `Stop`
|
|
returning means nothing is still inside the engine: the app snapshots the
|
|
cache and closes the store immediately afterwards, and a query still in flight
|
|
would be writing to a store on its way out. A UDP answer has nowhere else to
|
|
go, so that transport also gets a short bounded drain before its socket
|
|
closes — see `udpDrainGrace` in `internal/server/server.go`.
|
|
- **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 settled decision, pinned by
|
|
`TestLoadIgnoresUnknownKeys` in `internal/config`: `yaml`
|
|
`KnownFields(true)` was considered and rejected as a behaviour change for
|
|
anyone carrying extra keys). If a config key seems to have no effect, check
|
|
the spelling against the struct tags before anything else. The leniency
|
|
covers unknown keys only — a known key with a bad *type* is still a startup
|
|
failure.
|
|
- **`dns.EDNS0_NSID.Nsid` is hex on the wire.** A human-readable value there
|
|
builds a message that will not pack, so the client gets SERVFAIL. See the
|
|
test-hygiene traps below.
|
|
|
|
## 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.
|
|
- **A test that only dials a stream transport races the accept loop.** A
|
|
shutdown can close the listener while the connection is still in the backlog,
|
|
so the server never accepts it and the test passes for the wrong reason.
|
|
Exchange one query first, then leave the connection idle
|
|
(`harness.idleStream`).
|
|
- **Count leaked goroutines by stack, not by `runtime.NumGoroutine()`.** The
|
|
test binary shares its process with net/http, crypto/tls and the race
|
|
detector, so a baseline count is noise. "Is any goroutine still running code
|
|
in this package?" is a property of the product — and it needs the caller's own
|
|
goroutine filtered out by id, or every run finds exactly one.
|
|
- 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`.
|
|
- A `dns.Msg` carrying a `TXT` string over 255 bytes **does not pack** in
|
|
miekg/dns v1.1.73 — the error is `dns: string exceeded 255 bytes in txt`, and
|
|
255 is the exact boundary. On a fake upstream's write path that failure is
|
|
silent and presents as a client timeout or a SERVFAIL several layers away.
|
|
Build a multi-kilobyte answer out of many strings under 255 bytes, and assert
|
|
the fake's write error before blaming the client.
|
|
- **`dns.EDNS0_NSID.Nsid` is a hex string, not the bytes on the wire.** The
|
|
library hex-decodes it in `pack` and hex-encodes it in `unpack`, so a plain
|
|
value such as `"fancy-dns"` fails with `dns: overflow packing opt` and a
|
|
long NSID packs fine (unlike `TXT`). This bit the engine: an unencoded
|
|
literal made every query from an NSID-using client SERVFAIL. Always
|
|
hex-encode on the way in; `TestPinnedEDNS0NSIDIsHex` in
|
|
`internal/engine/pins_test.go` pins the contract.
|
|
- `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.
|