developer
9881d2a095
The console's TypeScript types are hand-written and apiFetch ends in
`JSON.parse(text) as T`, so nothing in the toolchain compared them with the
Go structs. A field that lost its json tag, or a response a struct never had,
showed up as an empty table in a browser and nowhere else: the Go suite was
green and the console was wrong. internal/app/contract_test.go now asserts the
exact JSON key set of every admin endpoint, driven through the real router
with the real hook implementations, plus a test that drives the two write
endpoints with the exact payload web/src builds.
The drift it found, all four fixed here:
blocklist.Source and SourceMeta had no json tags at all, so GET /api/blocklists
sent Go field names ("URL"/"Format") where the console expects snake_case: the
Sources table rendered blank rows and its per-row toggle posted
`name: undefined`. Tagged to match ParseStats.
acl.GroupSummary was flat where the console reads a nested `match` object — the
same shape config.Group has — so every group card read "matches nothing". The
four match fields are nested now, under a new acl.Match.
PUT /api/clients answered 400 `json: unknown field "ptr_suffixes"` to the
console's payload, because config.Group and config.Match carried yaml tags only
and the decoder runs with DisallowUnknownFields. Both structs have json tags
now; the same applies to config.UpstreamServer and UpstreamBootstrap.
PUT /api/upstreams answered 400 `json: unknown field "health"`, because the
console round-trips the whole server object it read, runtime health fields
included. They are accepted and ignored, because the server owns them, and they
are `yaml:"-"` so settings.yaml does not gain runtime state.
Also fixed, each with a test:
A duration on the wire was a nanosecond count. config.Duration had a
MarshalYAML but no MarshalJSON, so bootstraps[].refresh arrived as 30000000000
while the console typed the field as a string, and healthcheck_interval was a
float of seconds in one place and a string in another. Durations on the wire
are duration strings everywhere now — the same spelling config.yaml uses — and a
bare JSON number is still accepted and means seconds, not nanoseconds.
TestDurationJSONForm used to pin the old behaviour and now pins the new one,
saying in its comment that the old behaviour was the bug.
A log row's key set depended on the row. api.LogEntry was omitempty on
client_group, response, upstream and list, so a query with no ACL group
arrived without the key and the console rendered "undefined" where the column
should be empty. The contract test can only pin one shape if the server emits
one, so nothing on that struct is omitempty any more.
A dead peer stayed in the cluster peer list forever. Cluster.seen was only ever
added to and deleted from on an explicit `bye`, so a node that crashed — or
whose Valkey connection died — was reported as a live peer for the life of the
process, and its stale blocklist digest was flagged as divergence forever.
Peers() now reaps peers unheard for peer_ttl*4 and stamps each state with the
receiver's own clock, so clock skew between two machines can neither reap a
healthy peer nor keep a dead one. Discover() ages out the same way on the
publisher's own last_seen stamp, because the keyed Valkey record outlives a
crash: a live node keeps refreshing the key's TTL, so a dead node's field stayed
readable for the life of the fleet.
peer_ttl: 0 would have crashed the process. time.NewTicker(PeerTTL/2) panics on
a non-positive period and `EXPIRE key 0` deletes the heartbeat key, so a config
that left peer_ttl unset would have taken the process down at Start, or made
every instance believe it is alone in the fleet. Both go through peerTTL() and
peerDeadline(), which fall back to the default.
Two cluster tests raced on the wire, not on the product, and one was
environment-dependent rather than wrong. A node that joined after another's
heartbeat was in flight hears about it on the next tick, so "b does not see a"
read immediately after "a sees b" is a coin flip; the divergence test waited for
a digest on the wrong node's view of the fleet; and 127.0.0.1:1 is only
unbindable outside a container that sets
net.ipv4.ip_unprivileged_port_start=0, where it binds and the partial-bind test
fails with both transports up. That test now occupies a port with a live
listener in the test process, which is a deterministic EADDRINUSE on every
machine and every uid.
internal/server/counters_test.go covers the counter-key layer that no other
test touched: the "<transport>.<field>" keyspace, that the aggregate folds only
the known fields, and that the snapshot is safe while counters move.
Two pinned behaviours changed on purpose, each with a test that says so:
An RPZ record did not cover the name it names. rpzRule gave a bare owner name
the subdomain form (*.name), so analytics.example.org. in a policy zone blocked
cdn.analytics.example.org. and left analytics.example.org. resolving normally —
and an rpz-passthru. record did not allowlist its own domain either. RPZ
semantics say the record applies to the domain and everything under it, so a
bare owner now takes the apex form **.name; an owner written with an explicit *.
is still strict, so a zone that wants subdomain-only behaviour can still ask for
it. This changes which queries get blocked for an existing policy zone, which is
why it wanted a decision. TestExampleRPZZoneCoversTheApexItNames pins both
halves and the example zone now says which half it relies on.
metrics.OutcomeLocal had no producer. The engine reported a local-zone answer as
qlog.ActionAllow, which outcomeFor mapped to OutcomeUpstream, so every locally
answered query was counted as an upstream one and the outcome="local" series was
never published — the dashboard's local column was permanently empty.
Response.Local is now set on the local-zone path and outcomeFor consults it
first. That moves published counters, so the README's metrics section says so
too.
496 lines
31 KiB
Markdown
496 lines
31 KiB
Markdown
# fancy-dns — development status and continuation plan
|
||
|
||
Last updated after the round that made the **API response contract a test**
|
||
(`internal/app/contract_test.go` pins the exact JSON key set of every admin
|
||
endpoint, and fixed the four drift items it exposed), decided the two pinned
|
||
open questions, and finished the cluster package's peer-reaping gap. `AGENTS.md`
|
||
holds the decisions, invariants and working rules; this file holds state and the
|
||
plan.
|
||
|
||
## Goal
|
||
|
||
A lightweight, high-performance recursive DNS server with:
|
||
|
||
- **Go backend** — sharded LRU cache backed by bbolt, blocklist replication from
|
||
multiple formats, per-client ACLs, query logging, Prometheus metrics.
|
||
- **Secure DNS** — DNS-over-TLS (RFC 7858) and DNS-over-HTTPS (RFC 8484).
|
||
DNSSEC validation was explicitly scoped **out**; AD/CD/extended-RCODE are
|
||
passed through only.
|
||
- **React + Tailwind admin console** embedded in the binary.
|
||
- **Config as zone files + YAML**, hot-reloadable, with **Valkey/Redis** for
|
||
cross-instance sync (chosen over AMQP).
|
||
|
||
## Architecture decisions (settled with the user)
|
||
|
||
| 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 | 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 |
|
||
| Scale-out | StatefulSet, no HPA | an HPA cannot drive a StatefulSet |
|
||
| Unknown YAML keys | Ignore, loudly documented | `KnownFields(true)` was considered and rejected as a behaviour change for anyone carrying extra keys. Pinned by `TestLoadIgnoresUnknownKeys`. |
|
||
|
||
## Package 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 (see below), 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,
|
||
and the response contract the console is written against
|
||
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.
|
||
|
||
## Status
|
||
|
||
### Green, end to end
|
||
|
||
`gofmt` clean, `go build ./...` clean, `go vet ./...` clean, and
|
||
`go test -race -count=1 -timeout 300s ./...` green across every package.
|
||
|
||
| Package | top-level tests | notes |
|
||
|---|---|---|
|
||
| `internal/acl` | 26 | group compilation, matching, first-group-wins |
|
||
| `internal/api` | 55 (156 cases) | the whole route table, three auth modes, CORS, rate limiting, SSE, settings, error branches, pinned limitations |
|
||
| `internal/app` | 25 + the contract suite | wiring, hooks, **the API response contract**, integration |
|
||
| `internal/blocklist` | 28 | four parsers, the set, the manager, the shipped example zone |
|
||
| `internal/cache` | 27 | LRU, TTL policy, serve-stale, bbolt round-trip |
|
||
| `internal/cluster` | 30 | Valkey lease, pub/sub, divergence, dead-peer reaping |
|
||
| `internal/config` | 22 | Duration/Size, `${VAR}`, unknown keys, the JSON wire form, the shipped example configs |
|
||
| `internal/engine` | 26 | the pipeline end to end, plus the pins |
|
||
| `internal/localzone` | 18 | wildcards, delegation, CNAME following |
|
||
| `internal/metrics` | 10 | the metric names, which are the contract with the dashboards |
|
||
| `internal/qlog` | 18 | ring, bbolt, top domains, sampling |
|
||
| `internal/resolver` | 10 | pool, breakers, health, policies |
|
||
| `internal/server` | 36 | four listeners, UDP deadline reset, truncation, DoH limits, identity, shutdown, stats |
|
||
| `internal/zonefile` | 29 | the master-file parser and the shipped example zone |
|
||
| `web` | 0 | typecheck and `oxlint` clean (0 errors), no test runner |
|
||
| `cmd/fancy-dns` | 0 | exercised by `make smoke` and the CI smoke job instead |
|
||
|
||
### The commands that matter
|
||
|
||
- `make check-go` — the inner loop (~1.3 s): gofmt check, vet, compile. Uses
|
||
`go-build`, not `build`, because `build` depends on `web` and shells out to npm.
|
||
- `make check` — `check-go` + the suite. What CI runs.
|
||
- `make smoke` — **new.** Boots the real binary with the console embedded on
|
||
unprivileged ports (the same `sed` rewrite CI uses), then checks `-check`,
|
||
`/api/healthz`, the embedded console, the unauthenticated 401, an
|
||
authenticated `whoami` and `/api/metrics`. Catches a broken embed, a bad
|
||
default and an auth regression in one command.
|
||
- `make web-check` — console typecheck and production build.
|
||
- `make bench` — cache and blocklist benchmarks.
|
||
|
||
### Test coverage is no longer the interesting gap
|
||
|
||
The API **response contract** is. The console's types are hand-written and
|
||
`apiFetch` ends in `JSON.parse(text) as T`, so nothing in the toolchain compared
|
||
them with the Go structs; a field that lost its `json` tag, or a response a
|
||
struct never had, showed up as an empty table in a browser and nowhere else.
|
||
`internal/app` now has contract tests that assert the exact JSON key set of
|
||
every admin endpoint — see the fix list.
|
||
|
||
### Decided this round (behaviour changed on purpose, each with a test)
|
||
|
||
- **An RPZ record did not cover the name it names.** `rpzRule` gave a bare
|
||
owner name the subdomain form (`*.name`), so `analytics.example.org.` in a
|
||
policy zone blocked `cdn.analytics.example.org.` and left
|
||
`analytics.example.org.` resolving normally, and an `rpz-passthru.` record
|
||
did not allowlist its own domain either. RPZ semantics say the record applies
|
||
to the domain and everything under it, so a bare owner now takes the apex
|
||
form `**.name`; an owner written with an explicit `*.` is still strict, so a
|
||
zone that wants subdomain-only behaviour can ask for it. This changes which
|
||
queries get blocked for an existing policy zone — re-read your zones if they
|
||
relied on the old behaviour. `TestExampleRPZZoneCoversTheApexItNames` pins
|
||
both halves, `TestParseRPZ` pins the compiled forms, and the example zone now
|
||
says which half it relies on.
|
||
- **`metrics.OutcomeLocal` had no producer.** The engine reported a local-zone
|
||
answer as `qlog.ActionAllow`, which `outcomeFor` mapped to
|
||
`OutcomeUpstream`, so every locally answered query was counted as an upstream
|
||
one and the dashboard's local column was permanently empty. `Response.Local`
|
||
is now set on the local-zone path and `outcomeFor` consults it first, so a
|
||
local answer publishes `outcome="local"` and is *not* counted as upstream.
|
||
This moved published counters, which is why it was a decision: the README's
|
||
metrics section and the dashboard notes are updated in the same change.
|
||
`TestLocallyAnsweredQueryIsCountedAsLocal` replaces the pin.
|
||
|
||
## Fixed this round
|
||
|
||
- **A dead peer stayed in the cluster peer list forever.** `Cluster.seen` was
|
||
only ever added to and deleted from on an explicit `bye`, so a node that
|
||
crashed — or whose Valkey connection died — was reported as a live peer for
|
||
the life of the process, and its stale blocklist digest was flagged as
|
||
divergence forever. `Peers()` now reaps peers unheard for `peer_ttl * 4`
|
||
(four missed heartbeats) and stamps each state with the receiver's own clock,
|
||
so clock skew between two machines can neither reap a healthy peer nor keep a
|
||
dead one. `Discover()` ages out the same way on the publisher's own
|
||
`last_seen` stamp, because the keyed Valkey record outlives a crash: a live
|
||
node keeps refreshing the key's TTL, so a dead node's field stayed readable
|
||
for the life of the fleet. `TestDeadPeerAgesOutOfPeers` (which was failing on
|
||
the tree as handed over — the reaping it asked for did not exist) now passes.
|
||
- **`peer_ttl: 0` would have crashed the process.** `time.NewTicker(PeerTTL/2)`
|
||
panics on a non-positive period, and `EXPIRE key 0` *deletes* the heartbeat
|
||
key, so a config that left `peer_ttl` unset would have taken the process down
|
||
at `Start` (or made every instance believe it is alone in the fleet). Both go
|
||
through `peerTTL()`/`peerDeadline()`, which fall back to the default.
|
||
- **Two cluster tests raced on the wire, not on the product.** A node that
|
||
joined after another's heartbeat was in flight hears about it on the next
|
||
tick, so "b does not see a", read immediately after "a sees b", is a coin
|
||
flip; and `TestStatusReportsDivergentPeers` waited for a digest on the wrong
|
||
node's view of the fleet. Both tests were wrong, not the code.
|
||
- **The partial-bind test only worked unprivileged.**
|
||
`TestStartReportsAPartialFailureButServes` used `127.0.0.1:1` as a port that
|
||
cannot be bound, which is not true in any container with
|
||
`net.ipv4.ip_unprivileged_port_start=0` — it binds, and the test failed with
|
||
both transports up. It now occupies a port with a live listener in the test
|
||
process, which is a deterministic `EADDRINUSE` on every machine and every
|
||
uid.
|
||
- **Four API/console contract breaks, all found by the new contract test:**
|
||
`blocklist.Source` and `SourceMeta` had no `json` tags at all, so the Sources
|
||
table rendered blank rows and its per-row toggle posted `name: undefined`;
|
||
`acl.GroupSummary` was flat where the console reads a nested `match`, so every
|
||
group card read "matches nothing"; `PUT /api/clients` answered 400
|
||
`json: unknown field "ptr_suffixes"` to the payload the console sends, because
|
||
`config.Group` and `config.Match` had yaml tags only; and `PUT /api/upstreams`
|
||
answered 400 `json: unknown field "health"`, because the console round-trips
|
||
the whole server object it read, runtime health fields included. The write
|
||
structs now carry json tags, the health fields are accepted and ignored (the
|
||
server owns them), and `acl.Match` is nested.
|
||
- **A duration on the wire was a nanosecond count.** `config.Duration` had a
|
||
`MarshalYAML` but no `MarshalJSON`, so `bootstraps[].refresh` arrived as
|
||
`30000000000` while the console typed the field as a string, and
|
||
`healthcheck_interval` was a float of seconds in one place and a string in
|
||
another. Durations on the wire are duration strings everywhere now, which is
|
||
the same spelling config.yaml uses; a bare JSON number is still accepted and
|
||
means **seconds**, not nanoseconds. `TestDurationJSONForm` pins all of it and
|
||
says the old behaviour was the bug.
|
||
- **A log row's key set depended on the row.** `api.LogEntry` was `omitempty`
|
||
on `client_group`, `response`, `upstream` and `list`, so a query with no ACL
|
||
group arrived without the key and the console rendered "undefined" where the
|
||
column should be empty. The contract test can only pin one shape if the
|
||
server emits one, so nothing on that struct is `omitempty` any more.
|
||
|
||
- **Every query from a client that asks for EDNS0 NSID got SERVFAIL.**
|
||
`dns.EDNS0_NSID.Nsid` is a *hex string* in miekg/dns v1.1.73 — `pack`
|
||
hex-decodes it, `unpack` hex-encodes it — and the engine put the literal
|
||
`"fancy-dns"` in it. `hex.DecodeString("fancy-dns")` fails, so the forwarded
|
||
upstream query could not be built, the exchange failed, and the client got
|
||
SERVFAIL. A one-line bug with total blast radius on a documented feature.
|
||
Fixed with `hex.EncodeToString`, and pinned from both sides:
|
||
`TestClientNSIDIsAnswered` (a client asking for NSID gets a real answer, the
|
||
option we forward upstream decodes to our own name, and `dns.hide_identity`
|
||
drops it) and `TestPinnedEDNS0NSIDIsHex` (the library contract, so a future
|
||
bump that changes `Nsid` to raw bytes fails here with an explanation instead
|
||
of silently breaking the feature).
|
||
- **The login rate limiter was applied to every route except the one it exists
|
||
for.** `allowRequest` is the only reader of `limitState.lockedTill` and it ran
|
||
behind `authed`, while the login route was registered through `public`. So N
|
||
bad API keys gave N × 401 and never a 429, and the lockout instead locked the
|
||
caller out of the *rest* of the API — brute-force protection is the one thing
|
||
the auth surface most needs, and it was the one thing that did not work. There
|
||
is now a `limited()` wrapper (the limiter only, no auth) on the login route;
|
||
`allowRequest` stays the single reader of the lockout state. Pinned by
|
||
`TestLoginBruteForceEventuallyHitsTheRateLimiter`,
|
||
`TestLoginLockoutExpiresWithTheClock`, `TestLoginLockoutIsPerIP`,
|
||
`TestLoginLockoutDisabledWithoutFailures` and the `TestAuthedRouteRateLimit*`
|
||
family, so both routes stay honest.
|
||
- **A successful login reset the attacker's request budget.**
|
||
`clearLoginFailures` deleted the whole per-IP `limitState`, which also reset
|
||
the per-minute window, so one valid key in the middle of a brute-force burst
|
||
handed back a fresh budget. It now clears only `failures` and `lockedTill`,
|
||
and `TestSuccessfulLoginDoesNotResetTheRequestWindow` /
|
||
`TestSuccessfulLoginClearsTheFailureCounter` pin both halves.
|
||
- **DoH GET had no size cap.** The POST path is capped by `readCapped` at
|
||
`dns.MaxMsgSize`; the GET path handed `?dns=` straight to a base64 decode and an
|
||
`Unpack`, so the only limit was net/http's `MaxHeaderBytes` — roughly a
|
||
megabyte of attacker-chosen base64 decoded per request, unauthenticated,
|
||
against a 64 KiB cap on the neighbouring path. A DNS message cannot exceed
|
||
65535 bytes, so the cap is `base64.RawURLEncoding.EncodedLen(dns.MaxMsgSize)`
|
||
and cannot refuse a conforming client. Pinned by
|
||
`TestDoHGETQueryStringOverTheCapIsRefused` (413) and
|
||
`TestDoHServesALargeButLegalQuery` (a message just under the wire limit is
|
||
still served, so the cap cannot be tightened carelessly).
|
||
- **A clean `Stop` sometimes returned "use of closed network connection".** The
|
||
DoH listener has two owners: `Stop`'s close list, and `http.Server.Shutdown`,
|
||
which tracks the same `net.Listener` because `serveDoH` hands it to
|
||
`srv.Serve`. Whichever lost that race made an orderly shutdown look like a
|
||
failed one. Closing an already-closed listener is not a failure, so `Stop` now
|
||
ignores `net.ErrClosed` and only reports a real close error. It reproduced 3
|
||
times in 3 runs with DoH enabled and never with it disabled, which is what
|
||
made it findable; `TestStopIsIdempotent` is the pin.
|
||
- **A per-transport `dropped` counter did not exist.** The DoH drop fix above
|
||
was invisible in the UI: `HandlerStats.Dropped` is an aggregate that cannot
|
||
say which transport dropped, and `TransportStats` had no `Dropped` field at
|
||
all, even though all four transports bump the counter. Added, with the
|
||
reasoning in the field's comment.
|
||
- **A DoH drop was not counted as a drop.**
|
||
`dropped` when `res.Drop`; the DoH path only reached `classify`, which maps
|
||
`drop` to `errors`, so `HandlerStats.Dropped` under-counted DoH. One line, in
|
||
the same position as the other two.
|
||
- **Both shipped zone files matched nothing.** `blocklist.local_zones` and the
|
||
`rpz`-format list source are read with the origin taken from the file's base
|
||
name (`zonefile.OriginFromPath`: `local.example.zone` → `local.`), so the
|
||
relative owner names in the examples were being qualified with that — a
|
||
`*.doubleclick.net` rule blocked `*.doubleclick.net.local.`, and the local
|
||
zone mixed relative owners with absolute ones. An operator copying either file
|
||
got a server that answered nothing and blocked nothing. Both examples now
|
||
spell every name out in full, and
|
||
`TestExampleLocalZoneIsOriginIndependent` (zonefile) plus
|
||
`TestExampleRPZZoneBlocksWhatItSays` (blocklist, through the real
|
||
`parseRPZFiles` path) fail if they are "tidied up" back to relative names.
|
||
- **The shipped configs were never loaded by anything.**
|
||
`deploy/config.example.yaml` and the ConfigMap in `deploy/k8s/00-config.yaml`
|
||
are now loaded through `config.Load` (which ends in `Validate`) and
|
||
spot-checked, including that the k8s `${FANCYDNS_API_KEY}` still expands and
|
||
still *fails* when the Secret is missing, and that the memory envelope matches
|
||
the StatefulSet's 1Gi limit.
|
||
- **The admin API's response shapes had drifted from the console's types** —
|
||
verified against a running binary, not inferred. `blocklist.Source` has no
|
||
`json` tags at all, so the Sources table rendered blank rows and its
|
||
per-row toggle posted `name: undefined`; `acl.GroupSummary` is flat where the
|
||
console expects a nested `match`, so every group card read "matches nothing";
|
||
and **both write endpoints answered 400 to the payload the console actually
|
||
sends** (`json: unknown field "ptr_suffixes"` on `PUT /api/clients`,
|
||
`unknown field "health"` on `PUT /api/upstreams`). The Go side is being fixed
|
||
with contract tests that assert the exact key set of every endpoint; the
|
||
console side is the next piece of work.
|
||
- **The GitHub Actions image job could never tag an image.** It used
|
||
`${{ gitea.sha }}` in a workflow under `.github/`, and the `gitea` context
|
||
only exists on Gitea; the image is also published by
|
||
`.gitea/workflows/build.yaml`, which is the pipeline with registry access. It
|
||
now tags with `github.sha` and only logs in and pushes when this repository
|
||
actually has a registry token, so a GitHub runner builds and stops instead of
|
||
failing.
|
||
- **`api.constantTimeEqual` had no callers**, and the `authenticator` doc
|
||
comment described a constant-time "API key scan" that does not exist. The real
|
||
design is better than the comment (API keys are compared as SHA-256 digests
|
||
through a hash-keyed map, so the plaintext is never stored) — the dead
|
||
function is gone and the comment now describes the code.
|
||
- **`maxGETResponse` was dead code** whose comment claimed a guarantee the
|
||
handler did not provide. Deleted; the DoH response is written as packed.
|
||
|
||
## Pinned, not fixed (each one has a test that says so out loud)
|
||
|
||
The standing decision for the round after this one was: **pin, do not change**.
|
||
Every item below is behaviour someone may want to change deliberately; each has
|
||
a test that fails when it changes, and says what to do when it does.
|
||
|
||
- **Unknown config keys are silently ignored** (a settled decision, see the
|
||
table above). `TestLoadIgnoresUnknownKeys` in `internal/config` also pins the
|
||
asymmetry that makes this surprising: a typo is invisible, while a known key
|
||
with a bad *type* is a startup failure.
|
||
- **Auth configuration is frozen at `New`.** `api.base_path`,
|
||
`api.trusted_proxies` and the authenticator (`auth.mode`, `api_keys`) are
|
||
captured once; a reload re-reads CORS only. Writing a new API key through
|
||
`PUT /api/settings` answers `ok` while the running server keeps the old key.
|
||
`TestPinnedWrittenAuthConfigIsNotAppliedUntilRestart` and
|
||
`TestCORSOriginsAreReadAtHandlerBuild` hold this in place until someone decides
|
||
whether a reload should re-apply it.
|
||
- **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.
|
||
`TestDoHClientIdentityIsTheProxyNotTheForwardedForHeader` pins it, with the
|
||
reason honouring `X-Forwarded-For` without `trusted_proxies` would break the
|
||
ACL and rate-limit invariants.
|
||
- **`api.cors_origins: ["*"]` echoes the request's `Origin` and sets
|
||
`Access-Control-Allow-Credentials: true`.** Deliberate (a literal `*` is
|
||
ignored by browsers when credentials are involved), but it does mean any
|
||
website can drive the admin API with whatever the browser has cached for that
|
||
host. The preflight also answers 204 for any origin and any path.
|
||
`TestCORSWildcardEchoesTheOrigin` and `TestCORSPreflight` pin all of it.
|
||
- **A partial bind failure is invisible.** `serveDoT`/`serveDoH` record a failed
|
||
bind in an error list only, so `ListenerInfo.Err` is never set and the admin
|
||
API cannot show which address failed.
|
||
`TestStartReportsAPartialFailureButServes` pins that `Start` returns nil
|
||
whenever any listener came up.
|
||
- **A question-less message is answered on UDP and kills the connection on
|
||
TCP/DoT.** `TestUDPQuestionlessMessageReachesTheEngine` pins the UDP half (the
|
||
engine turns it into a FORMERR); the stream path returns instead. Probably
|
||
right for both, but it is an asymmetry worth knowing about.
|
||
- **A UDP query in flight at `Stop` can be lost.** The stream paths register
|
||
with `s.wait`, so `Stop` waits for them; `servePacket` does not, so a UDP
|
||
query already inside the pipeline when the listener closes may never be
|
||
answered. That is arguable — UDP has no connection to hang up on and the
|
||
client will retry — but the asymmetry with TCP/DoT is not pinned anywhere, and
|
||
it should be a decision rather than an accident.
|
||
- **`doh.sent` is never bumped.** The DoH handler writes its answer through
|
||
`net/http`, so the `sent` counter is only meaningful on UDP/TCP/DoT, and the
|
||
console's transport table shows `sent: 0` for DoH. Pinned as current
|
||
behaviour by `TestCountersFollowTheEngineOutcome` with a comment saying why.
|
||
- **`api.metrics.path` is ignored**
|
||
always `${base_path}/metrics`); `POST /api/blocklists/refresh` returned
|
||
`{"names": null}`; an empty `config_yaml` in the settings form is not valid (it
|
||
decodes onto `Default()`, which enables a blocklist with no sources); `"1Gi"`
|
||
is not a valid size — only `GiB`, `G`, `GB` — even though that is how k8s
|
||
spells it.
|
||
|
||
## Continuation plan, in order
|
||
|
||
1. **The console side of the contract** — the Go side is authoritative now
|
||
(`internal/app/contract_test.go`); `web/src/lib/types.ts` and the pages have
|
||
to be brought onto it. The known items: the Cache page's "Lookups"/"Last
|
||
seen" columns read `count`/`last_seen` where the server sends `hits` (there
|
||
is no last-seen data at all), the Dashboard reads `qps`/`total_queries` where
|
||
the server sends `queries_per_sec`/`queries_total` and never sends a
|
||
`latency` block, `whoami.can_write`/`read_only` are fetched and never used so
|
||
a read-only key still shows every Save control, `blocked_pct` arrives as
|
||
0..100 from the dashboard and 0..1 over SSE, and the upstreams page has to
|
||
cope with durations being strings on the wire.
|
||
2. **Finish the remaining server tests** — the counter-key layer is now covered
|
||
by `internal/server/counters_test.go`; what is left is a look at each
|
||
transport's shutdown ordering with a slow query in flight, and a `Stop`
|
||
goroutine-leak assertion.
|
||
3. **Decide the rest of the pinned list**, one at a time, each with a changelog
|
||
note: the frozen auth config, the DoH client identity, CORS wildcards, the
|
||
UDP query lost at `Stop`, `doh.sent` never bumped.
|
||
4. **Stop hand-writing the console's types.** The contract tests make the Go
|
||
side authoritative; the cheapest durable answer is generating
|
||
`web/src/lib/types.ts` from the Go structs (or an OpenAPI document emitted by
|
||
the server) so `apiFetch` is typed against something real instead of
|
||
`JSON.parse(text) as T`. `internal/app/dump_shapes_test.go` (`DUMP_SHAPES=1`)
|
||
is the stopgap that already exists.
|
||
|
||
## 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.
|
||
- **Cache invalidation on rule changes flushes the cache** rather than guessing
|
||
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 — and now also expect `internal/app`'s
|
||
contract test to fail, which is the point of it.
|
||
- **API keys are compared by SHA-256 hash, constant-time**, and
|
||
`X-Forwarded-For` is honoured *only* from `trusted_proxies`.
|
||
- **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.
|
||
- **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 (the domains list ships `rule` for
|
||
exactly this).
|
||
- **pprof is loopback-only by construction**, not by policy, so no config flag
|
||
can turn it into a network-reachable unauthenticated endpoint.
|
||
- **A master file's origin is its base name**, so a relative owner name in a
|
||
zone is qualified with the file's name, not with the zone you meant. Absolute
|
||
names are the only spelling that survives a rename.
|
||
|
||
## Test-hygiene traps
|
||
|
||
- `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. `//go:embed all:dist` still
|
||
builds while the directory is empty, so a console rebuild in the middle of
|
||
someone else's `go build` is not fatal.
|
||
- **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 the failure is silent and presents
|
||
as a client timeout, so build a multi-kilobyte answer out of many short
|
||
strings 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.** A plain
|
||
value such as `"fancy-dns"` fails with `dns: overflow packing opt`, and a long
|
||
NSID packs fine (unlike `TXT`). Always hex-encode on the way in.
|
||
- `gofmt -l internal/` is the fastest way to see that a file does not parse.
|
||
- The console has **no test runner at all** — only `tsc -b --noEmit` and
|
||
`oxlint`. A field-name drift between the API and the console is invisible to
|
||
both, which is why the Go side now has contract tests.
|
||
|
||
## Fixed earlier and worth not re-breaking
|
||
|
||
A running list, kept because each of these was invisible until it was found:
|
||
|
||
- `config.ParseSize` ignored the unit multiplier entirely (`512MiB` parsed as
|
||
512 bytes), so every memory budget in the tree was off by a factor of the
|
||
unit. `size_test.go` pins the units, the shadowing cases, the range and the
|
||
truncating rounding.
|
||
- A cache miss and a stale hit moved only the Prometheus counter and not the
|
||
atomics the console's `Totals` reads, so the Cache page read zero for the
|
||
life of the process.
|
||
- `finalize` — the EDNS0/UDP-size step that turns an oversized answer into a
|
||
question-only TC reply — ran only on the cache-hit path, so a multi-kilobyte
|
||
answer from upstream, from the local zone or from a sinkhole was left as a
|
||
datagram a 512-byte UDP client silently dropped.
|
||
- The serve-stale prefetch gate was a 64-slot channel, so a burst of stale hits
|
||
on one hot name launched up to 64 duplicate upstream exchanges instead of the
|
||
one its own comment promised. It is a per-key latch now.
|
||
- The upstream UDP client read buffer was hard-coded to 1232 while the engine
|
||
advertises `dns.max_udp_size`, so an operator who raised it got SERVFAIL for
|
||
every answer over 1232 bytes.
|
||
- Every relative RDATA name in a master file pointed at the DNS root: the
|
||
qualification pass ran *after* `dns.NewRR`, so `www CNAME host` became
|
||
`CNAME host.`. It is now a token-level pass driven by a table of which RDATA
|
||
positions hold a name.
|
||
- The cache snapshot never round-tripped: `Load` decoded the record header out of
|
||
the *key's* remainder instead of the value, so every restart was a cold cache.
|
||
There is now an `encodeRecord`/`decodeRecord` pair with a version byte and a
|
||
CRC-32C trailer, and `Load` drops expired entries and rewrites the TTL to the
|
||
time actually left.
|
||
- `${VAR}` expansion never worked inside quotes — the only form anyone writes —
|
||
because the reference check sat below the quote branches in the same switch.
|
||
The admin API key was the literal string `"${FANCYDNS_API_KEY}"`.
|
||
- ACL PTR suffix matching crossed label boundaries, so a group scoped to
|
||
`example.com.` also swallowed `ample.com.`; the ACL `*` catch-all was dropped
|
||
from the rendered domain patterns; `Upstream.Success` never closed an open
|
||
breaker; `Exchange` fanned out in parallel even under `policy: failover`; the
|
||
cache byte budget was never fed to the LRU.
|
||
- **`TestRebindUnderConcurrentQueries` was a flake** (2 runs in 10) because it
|
||
asserted a reader had to observe two specific addresses. It now pins the
|
||
copy-on-write invariant itself.
|
||
- CI could not have passed (`go.mod` requires Go 1.27.1, CI and the Dockerfile
|
||
pinned 1.23), the Dockerfile clobbered the console by copying the source tree
|
||
after the bundle, and the k8s set could not have worked (`hostNetwork: true`
|
||
with `replicas: 2` and no SO_REUSEPORT).
|
||
- `$TTL` was ignored by the zone parser; every record got the SOA minimum.
|
||
- Blocklist: leading BOM stripped, `!allow` works, `SourceMeta` exists for a
|
||
rule-only source, `Pattern()` reports the bare name while `WireName()` gives
|
||
the wildcard form, RPZ inherits the zone default TTL, hosts files complain
|
||
about a missing address, adblock handles CRLF and over-long lines.
|
||
- `serveUDP` called `net.ResolveUDPAddr` per packet; the peer already is a
|
||
`*net.UDPAddr`.
|