Add AGENTS.md; bring STATUS.md up to a green tree
AGENTS.md is the part that should not churn: the settled decisions with their reasons, the invariants that hold the design together, the working rules, and the security-sensitive areas that have been wrong before. A new agent should read it and STATUS.md and know what not to re-litigate. Deliberately excluded: anything about how to search, read or patch. Those are being automated per round, and writing advice about them into the repo would rot. What is here instead is project-specific and cannot be automated away — for example that a PTR suffix must match on a label boundary, that a truncated answer must never be cached, and that the UDP read deadline has to be reset every iteration or the loop spins. The working rules are the part worth arguing for. Never leave the tree red, because nobody can detect the budget running out: three of five subagents left broken files, and each cost a round to discover. Report honestly, and when a test fails, decide first whether the test, the implementation or the specification is wrong — all three happen here, and the decision belongs in the commit message. STATUS.md now records a fully green tree: nine packages with suites, all passing under -race. The plan starts with internal/engine, which is the largest uncovered surface and the one every packet goes through.
This commit is contained in:
1 parent
16c8ad27b9
commit
bbd8692bab
2 files changed
+247
-74
No files matched your search
@@ -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.
|
||||
Reference in new issue
Block a user