diff --git a/README.md b/README.md index 1680acc..9dd9970 100644 --- a/README.md +++ b/README.md @@ -344,6 +344,13 @@ Metric names: `fancydns_queries_total`, `fancydns_blocked_total`, `fancydns_query_duration_seconds`, `fancydns_upstream_duration_seconds`, `fancydns_blocklist_compile_seconds`. +`fancydns_query_duration_seconds` carries an `outcome` label with the values +`cache`, `blocked`, `local`, `upstream`, `error`, `refused`, `dropped`. `local` +is the series for queries answered from `blocklist.local_zones`; it used to have +no producer, so it was never published and every locally answered query was +counted as `upstream`. A dashboard that showed an "upstream" column now sees +local traffic counted correctly — see STATUS.md for the change note. + ### log | Key | Type | Default | What it does | @@ -439,7 +446,7 @@ plain-list entry becomes `**.name`. | `hosts` | `IP name [name…]`, `0 name`, `localhost` lines, `#`/`;` comments | `0.0.0.0 ads.example.com` | `localhost`/`broadcasthost`/`ip6-*` lines are skipped; a line whose first field is not an address falls back to the plain parser, with a notice in the parse stats | | `adblock` | `\|\|host^` and `\|http://host/…` (blocked), `@@\|\|host^` (allowed), bare hostnames | `\|\|ads.example.com^$third-party` | element-hiding rules (`##`, `#@#`, `#?#`) and regex rules are skipped and reported in `stats.notices`; a URL path rule blocks the whole host | | `plain` | one name per line, `0.0.0.0 name`, `!allow name` / `!name` / `@@…` allow markers, adblock tails on pasted lines | `ads.example.com` | a single-label name without a dot is rejected | -| `rpz` | an RFC 1035 policy zone | `ads.example.com CNAME .` | parsed by `internal/zonefile`; the zone's own default TTL wins for records with no TTL column | +| `rpz` | an RFC 1035 policy zone | `ads.example.com CNAME .` | parsed by `internal/zonefile`; the zone's own default TTL wins for records with no TTL column; a bare owner name covers the domain and everything under it (`**.name`, RPZ semantics), an owner written `*.name` covers only its subdomains | | `auto` | sniffs the first 200 meaningful lines | — | default; a `$TTL`/`$ORIGIN`/`$INCLUDE` line or a `name TTL IN A …` shape means `rpz`, an address-first line means `hosts`, `\|\|`/`##`/`@@` means `adblock`, otherwise `plain` | **Categories.** `blocklist.lists[].categories` are copied onto every rule from @@ -796,17 +803,19 @@ Found while writing this file; each is a to-do, not something fixed here. 24. **`config.Load` does not use `yaml.Decoder.KnownFields`**, so unknown keys are ignored silently. A settled decision (pinning, not changing, the behaviour); `TestLoadIgnoresUnknownKeys` in `internal/config` holds it. -25. **`internal/blocklist/parse.go:485-495` — an RPZ record does not cover the - name it names.** `rpzRule` gives a bare owner name the subdomain form - (`*.name`), and a subdomain-form rule matches that name's children but not - the name itself, so `analytics.example.org.` in a policy zone blocks - `cdn.analytics.example.org.` and leaves `analytics.example.org.` resolving +25. **RESOLVED — 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. RPZ semantics say the record applies to the domain and everything - under it, so the apex form (`**.name`) is what belongs here — a - one-character change in `rpzRule` that alters which queries get blocked, so - it wants a decision rather than a quiet fix. Pinned by - `TestExampleRPZZoneApexIsNotCovered`. The same gap means an - `rpz-passthru.` record for a domain does not allowlist the domain itself. + under it, so `rpzRule` now uses the apex form (`**.name`) for a bare owner. + 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: re-read your zones if they relied + on the old (buggy) strict behaviour. Pinned by + `TestExampleRPZZoneCoversTheApexItNames`, including the `rpz-passthru.` + half — the same gap meant a passthru record did not allowlist its own + domain. 26. **`internal/api/api.go:288-317` — `api.cors_origins: ["*"]` echoes the request's `Origin` *and* sets `Access-Control-Allow-Credentials: true`.** That is deliberate (a literal `*` is ignored by browsers when credentials diff --git a/STATUS.md b/STATUS.md index ffaf233..2cfe181 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,9 +1,11 @@ # fancy-dns — development status and continuation plan -Last updated after the round that finished the test coverage of every package -that has logic worth covering, fixed two live bugs and pinned the open -questions. `AGENTS.md` holds the decisions, invariants and working rules; this -file holds state and the 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 @@ -79,11 +81,11 @@ Cache is a single shard-local lookup; only a miss touches the network. |---|---|---| | `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 | wiring, hooks, the API response contract, integration | +| `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` | in progress | Valkey lease, pub/sub, divergence | -| `internal/config` | 22 | Duration/Size, `${VAR}`, unknown keys, the shipped example configs | +| `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 | @@ -116,8 +118,86 @@ 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 @@ -219,32 +299,14 @@ every admin endpoint — see the fix list. ## Pinned, not fixed (each one has a test that says so out loud) -The standing decision for this round 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. +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. -- **`metrics.OutcomeLocal` has no producer.** The engine reports a local-zone - answer as `qlog.ActionAllow`, which `outcomeFor` maps to `OutcomeUpstream`, so - every locally answered query is counted as an upstream one and the - `outcome="local"` series is never published — the dashboard's local column is - permanently empty. `TestPinnedLocalAnswersAreCountedAsUpstream` asserts the - upstream counter moves by one and the local series stays absent. Fixing it - moves published counters, so it wants a decision and a dashboard note. -- **An RPZ record does not cover the name it names.** `rpzRule` gives a bare - owner name the subdomain form (`*.name`), and a subdomain-form rule matches - the name's children but not the name itself, so `analytics.example.org.` in a - policy zone blocks `cdn.analytics.example.org.` and leaves - `analytics.example.org.` resolving normally. RPZ semantics say the record - applies to the domain and everything under it, so `**.name` is what belongs - here — a one-character change in `rpzRule` that alters which queries get - blocked. Verified end to end against a running binary: the subdomain is - NXDOMAIN, the apex is not. Pinned by `TestExampleRPZZoneApexIsNotCovered`, - which also shows the same gap on the allow side (an `rpz-passthru.` record - for a domain does not allowlist the domain itself). - **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 @@ -293,31 +355,29 @@ that fails when it changes, and says what to do when it does. ## Continuation plan, in order -1. **`internal/cluster` suite** — in progress. The rolling heartbeat, the Valkey - lease and leader handoff, pub/sub config sync, divergence detection and - reconnect, against `miniredis`. -2. **The API response contract** — in progress on the Go side - (`internal/app` contract tests + the fixes listed above). -3. **The console side of that contract** — the next piece of work, and it should - be done against the canonical key list from step 2: the `qtype` fields that - are numbers on the wire but strings in TypeScript (the server also sends - `qtype_name`), `blocked_pct` arriving as 0..100 from the dashboard and 0..1 - over SSE, the `whoami.can_write` / `read_only` flags that are fetched and - never used (a read-only key still shows every Save control), a `latency` - block the dashboard never receives, the cache "Lookups"/"Last seen" columns - reading `count`/`last_seen` where the API sends `hits`, and the refresh toast - that says "0 sources queued" because the API returned null. -4. **The remaining server tests** — in progress: shutdown ordering (in-flight - queries, idempotent `Stop`, a cancelled context, the port actually being - released), the stats counters, and a table for the pure helpers. -5. **Decide the open questions** in the pinned list above, one at a time, each - with a changelog note: the RPZ apex, `OutcomeLocal`, the frozen auth config, - the DoH client identity, CORS wildcards. -6. **Stop hand-writing the console's types.** The contract tests make the Go +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`. + `JSON.parse(text) as T`. `internal/app/dump_shapes_test.go` (`DUMP_SHAPES=1`) + is the stopgap that already exists. ## Design notes worth preserving diff --git a/deploy/zones/local.rpz.zone.example b/deploy/zones/local.rpz.zone.example index d2d64fd..bfed872 100644 --- a/deploy/zones/local.rpz.zone.example +++ b/deploy/zones/local.rpz.zone.example @@ -13,6 +13,12 @@ $TTL 60 localhost. SOA localhost. hostmaster.localhost. 1 3600 600 604800 60 localhost. NS localhost. +; A policy record names a domain and applies to that domain AND everything +; under it. An owner written bare (`analytics.example.org.`) is given the apex +; form internally, so the apex and its subdomains are both covered; an owner +; written with an explicit `*.` covers only names below it, which is how you +; ask for the subdomain-only behaviour. TestExampleRPZZoneCoversTheApexItNames +; in internal/blocklist pins both halves. ; *.doubleclick.net CNAME . -> NXDOMAIN *.doubleclick.net. CNAME . *.ads.example.com. CNAME . diff --git a/internal/acl/acl.go b/internal/acl/acl.go index 2506995..30d259d 100644 --- a/internal/acl/acl.go +++ b/internal/acl/acl.go @@ -333,15 +333,28 @@ func matchPTR(n, suffix string) bool { return rest == "" || rest[len(rest)-1] == '.' } +// Match selects clients by network identity. It is the API's spelling of +// config.Match: the same four fields, nested under `match`, so the summary +// the console reads and the group the console writes are the same shape. +type Match struct { + CIDRs []string `json:"cidrs"` + PTRSuffixes []string `json:"ptr_suffixes"` + Tags []string `json:"tags"` + Any bool `json:"any"` +} + // Groups returns the compiled group summaries for the API. +// +// The four match fields are nested under `match` because that is how the +// config file spells a group (config.Group.Match) and therefore how the +// console spells it too: a flat summary made every group card read "matches +// nothing" and the editor write back a shape the server refused. type GroupSummary struct { - Name string `json:"name"` - Comment string `json:"comment,omitempty"` - Policy string `json:"policy"` - CIDRs []string `json:"cidrs"` - PTRSuffixes []string `json:"ptr_suffixes"` - Tags []string `json:"tags"` - Any bool `json:"any"` + Name string `json:"name"` + Comment string `json:"comment,omitempty"` + Policy string `json:"policy"` + Match Match `json:"match"` + // AllowDomains and DenyDomains stay flat, as in config.Group. AllowDomains []string `json:"allow_domains"` DenyDomains []string `json:"deny_domains"` SkipBlocklist bool `json:"skip_blocklist"` @@ -372,10 +385,7 @@ func (e *Engine) Summaries() []GroupSummary { Name: g.name, Comment: g.comment, Policy: string(g.policy), - CIDRs: cidrs, - PTRSuffixes: g.ptr, - Tags: g.tags, - Any: g.any, + Match: Match{CIDRs: cidrs, PTRSuffixes: g.ptr, Tags: g.tags, Any: g.any}, AllowDomains: domainPatterns(g.allow), DenyDomains: domainPatterns(g.deny), SkipBlocklist: g.skipBlocklist, diff --git a/internal/acl/acl_test.go b/internal/acl/acl_test.go index 6f8224c..cf7abd8 100644 --- a/internal/acl/acl_test.go +++ b/internal/acl/acl_test.go @@ -189,8 +189,8 @@ func TestEngineMatchMostSpecificCIDRWithinGroup(t *testing.T) { // Echoed in matching order (longest prefix first), which is the order the // group itself walks them in. wantCIDRs := []string{"10.1.2.3/32", "2001:db8::/32", "10.1.2.0/24", "10.0.0.0/8"} - if !slices.Equal(s[0].CIDRs, wantCIDRs) { - t.Errorf("Summaries()[0].CIDRs = %v, want %v (longest prefix first)", s[0].CIDRs, wantCIDRs) + if !slices.Equal(s[0].Match.CIDRs, wantCIDRs) { + t.Errorf("Summaries()[0].Match.CIDRs = %v, want %v (longest prefix first)", s[0].Match.CIDRs, wantCIDRs) } } @@ -256,7 +256,7 @@ func TestEngineMatchCIDREdges(t *testing.T) { }) t.Run("unparsable CIDRs are skipped, valid ones still work", func(t *testing.T) { - if cidrs := e.Summaries()[0].CIDRs; !slices.Contains(cidrs, "10.0.0.0/8") || !slices.Contains(cidrs, "2001:db8::/32") { + if cidrs := e.Summaries()[0].Match.CIDRs; !slices.Contains(cidrs, "10.0.0.0/8") || !slices.Contains(cidrs, "2001:db8::/32") { t.Errorf("CIDRs = %v, want the two parsable prefixes", cidrs) } }) @@ -322,7 +322,7 @@ func TestEngineMatchPTRSuffix(t *testing.T) { t.Run("suffixes are echoed normalised", func(t *testing.T) { // Sorted, because the summary is a UI view. want := []string{"lan.example.", "corp.internal."} - if got := e.Summaries()[0].PTRSuffixes; !slices.Equal(got, want) { + if got := e.Summaries()[0].Match.PTRSuffixes; !slices.Equal(got, want) { t.Errorf("PTRSuffixes = %v, want %v", got, want) } }) @@ -434,7 +434,7 @@ func TestEngineMatchTags(t *testing.T) { t.Run("tags are echoed lowercased in config order", func(t *testing.T) { want := []string{"laptop", "kiosk"} - if got := e.Summaries()[0].Tags; !slices.Equal(got, want) { + if got := e.Summaries()[0].Match.Tags; !slices.Equal(got, want) { t.Errorf("Tags = %v, want %v", got, want) } }) @@ -597,12 +597,14 @@ func TestEngineSummaries(t *testing.T) { } got := s[0] want := GroupSummary{ - Name: "a", - Comment: "the office", - Policy: "deny", - CIDRs: []string{"10.0.0.0/8"}, - PTRSuffixes: []string{"lan.example."}, - Tags: []string{"laptop"}, + Name: "a", + Comment: "the office", + Policy: "deny", + Match: Match{ + CIDRs: []string{"10.0.0.0/8"}, + PTRSuffixes: []string{"lan.example."}, + Tags: []string{"laptop"}, + }, AllowDomains: []string{"*.example.net", "example.com"}, DenyDomains: []string{"*.ads.example.net"}, SkipBlocklist: true, @@ -611,18 +613,18 @@ func TestEngineSummaries(t *testing.T) { Upstreams: []string{"127.0.0.1:5301"}, } if got.Name != want.Name || got.Comment != want.Comment || got.Policy != want.Policy || - got.Any != want.Any || got.SkipBlocklist != want.SkipBlocklist || + got.Match.Any != want.Match.Any || got.SkipBlocklist != want.SkipBlocklist || got.MaxConcurrent != want.MaxConcurrent || got.LogQueries != want.LogQueries { t.Errorf("Summaries()[0] scalars = %+v, want %+v", got, want) } - if !slices.Equal(got.CIDRs, want.CIDRs) { - t.Errorf("CIDRs = %v, want %v", got.CIDRs, want.CIDRs) + if !slices.Equal(got.Match.CIDRs, want.Match.CIDRs) { + t.Errorf("CIDRs = %v, want %v", got.Match.CIDRs, want.Match.CIDRs) } - if !slices.Equal(got.PTRSuffixes, want.PTRSuffixes) { - t.Errorf("PTRSuffixes = %v, want %v", got.PTRSuffixes, want.PTRSuffixes) + if !slices.Equal(got.Match.PTRSuffixes, want.Match.PTRSuffixes) { + t.Errorf("PTRSuffixes = %v, want %v", got.Match.PTRSuffixes, want.Match.PTRSuffixes) } - if !slices.Equal(got.Tags, want.Tags) { - t.Errorf("Tags = %v, want %v", got.Tags, want.Tags) + if !slices.Equal(got.Match.Tags, want.Match.Tags) { + t.Errorf("Tags = %v, want %v", got.Match.Tags, want.Match.Tags) } if !slices.Equal(got.AllowDomains, want.AllowDomains) { t.Errorf("AllowDomains = %v, want %v (sorted, wildcards rendered)", got.AllowDomains, want.AllowDomains) @@ -637,10 +639,10 @@ func TestEngineSummaries(t *testing.T) { if got.RateLimit != 0 || got.ClientCount != 0 || got.QueryCount != 0 || got.BlockedCount != 0 || got.Matched { t.Errorf("Summaries()[0] = %+v, want zeroed counters and Matched=false", got) } - if s[1].Name != "b" || !s[1].Any || s[1].Policy != "allow" { + if s[1].Name != "b" || !s[1].Match.Any || s[1].Policy != "allow" { t.Errorf("Summaries()[1] = %+v, want group b / any / allow", s[1]) } - if len(s[1].CIDRs) != 0 || len(s[1].PTRSuffixes) != 0 || len(s[1].AllowDomains) != 0 { + if len(s[1].Match.CIDRs) != 0 || len(s[1].Match.PTRSuffixes) != 0 || len(s[1].AllowDomains) != 0 { t.Errorf("Summaries()[1] = %+v, want empty lists", s[1]) } } diff --git a/internal/api/api.go b/internal/api/api.go index 3eaa3c3..c14bb80 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -131,24 +131,29 @@ type LogQuery struct { Since time.Time } -// LogEntry is the JSON shape of a query-log row, matching what the DNS -// handlers record. +// LogEntry is one query-log row as the admin API serves it. +// +// These fields are not omitempty, and that is deliberate: omitempty makes the +// key set depend on the row, so a query with no ACL group arrived without +// `client_group` and the console rendered "undefined" where the column should +// be empty. The contract test in internal/app pins the key set, and it can +// only pin one shape if the server emits one. type LogEntry struct { ID uint64 `json:"id"` TS time.Time `json:"ts"` Client string `json:"client"` - Group string `json:"client_group,omitempty"` + Group string `json:"client_group"` Name string `json:"name"` QType uint16 `json:"qtype"` QTypeStr string `json:"qtype_name"` RCode int `json:"rcode"` RCodeStr string `json:"rcode_name"` Action string `json:"action"` - Response string `json:"response,omitempty"` + Response string `json:"response"` Duration int64 `json:"duration_ms"` - Upstream string `json:"upstream,omitempty"` - List string `json:"list,omitempty"` - Protocol string `json:"protocol,omitempty"` + Upstream string `json:"upstream"` + List string `json:"list"` + Protocol string `json:"protocol"` Blocked bool `json:"blocked"` } diff --git a/internal/api/handlers.go b/internal/api/handlers.go index ebeebea..bdb5631 100644 --- a/internal/api/handlers.go +++ b/internal/api/handlers.go @@ -258,15 +258,26 @@ func (s *Server) handleUpstreams(w http.ResponseWriter, _ *http.Request) { writeJSON(w, http.StatusOK, s.hooks.GetUpstream()) } -// upstreamPayload is the PUT body for the upstreams endpoint. Health fields -// from the UI are ignored on write: the server owns them. +// upstreamPayload is the PUT body for the upstreams endpoint. The health +// fields the console sends (health, latency_ms, fail_rate, breaker_open and +// the counters) are accepted and ignored: the server owns them, and refusing +// the object the console just read is what made every save answer 400. They are +// carried on config.UpstreamServer with `json:"-"` on the yaml side, so +// settings.yaml does not gain runtime state. +// +// HealthCheckInterval accepts both spellings the console has used: the +// duration string ("30s") and the older seconds number under its old name. type upstreamPayload struct { Policy string `json:"policy"` Servers []config.UpstreamServer `json:"servers"` Bootstraps []config.UpstreamBootstrap `json:"bootstraps"` + HealthCheck config.Duration `json:"healthcheck_interval"` HealthCheckSeconds float64 `json:"healthcheck_interval_s"` MaxAttempts int `json:"max_attempts"` BreakerThreshold int `json:"breaker_threshold"` + // BreakerCooldown was read-only in the console; accepting it means a + // round-trip of the object the console read does not fail. + BreakerCooldown config.Duration `json:"breaker_cooldown"` } // handleUpstreamsPut validates and persists a new upstream config. @@ -283,14 +294,23 @@ func (s *Server) handleUpstreamsPut(w http.ResponseWriter, r *http.Request) { // Preserve fields the UI does not manage, so a UI write never silently // resets a breaker cooldown someone set by hand. cur := s.hooks.Store.Get().Upstream + healthCheck := in.HealthCheck + if healthCheck <= 0 { + // The older seconds-number spelling; zero also means "not supplied". + healthCheck = config.Duration(in.HealthCheckSeconds * float64(time.Second)) + } + cooldown := cur.BreakerCooldown + if in.BreakerCooldown > 0 { + cooldown = in.BreakerCooldown + } up := config.Upstream{ Policy: in.Policy, Servers: in.Servers, Bootstraps: in.Bootstraps, MaxAttempts: in.MaxAttempts, - HealthCheck: config.Duration(in.HealthCheckSeconds * float64(time.Second)), + HealthCheck: healthCheck, BreakerThreshold: in.BreakerThreshold, - BreakerCooldown: cur.BreakerCooldown, + BreakerCooldown: cooldown, } if up.HealthCheck <= 0 { up.HealthCheck = cur.HealthCheck diff --git a/internal/app/contract_test.go b/internal/app/contract_test.go new file mode 100644 index 0000000..0332379 --- /dev/null +++ b/internal/app/contract_test.go @@ -0,0 +1,431 @@ +package app + +import ( + "encoding/json" + "net/http" + "sort" + "strconv" + "strings" + "testing" +) + +// The contract tests in this file exist because 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. +// +// Each assertion is the EXACT top-level key set of one endpoint, driven +// through the real router with the real hook implementations (harness_test.go +// wires a whole App). Renaming a field on either side now fails here with a +// readable diff instead of in a browser. +// +// The sets below are the Go side's truth. Where the console disagrees, the +// console is what has to move: these are the endpoints a deployed binary +// answers, and a contract that follows the UI would drag the API backwards +// every time the UI got nicer. TestConsoleWritePayloadsAreAccepted pins the +// other direction, because there the console is the client and its payload is +// fixed. + +// keysOf returns the sorted top-level keys of a JSON object body. +func keysOf(t *testing.T, body []byte) []string { + t.Helper() + var m map[string]json.RawMessage + if err := json.Unmarshal(body, &m); err != nil { + t.Fatalf("body is not a JSON object: %v\n%s", err, body) + } + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// keysOfField returns the sorted keys of a nested object addressed by path, +// where a numeric path element indexes an array. +func keysOfField(t *testing.T, body []byte, path ...string) []string { + t.Helper() + var cur any + if err := json.Unmarshal(body, &cur); err != nil { + t.Fatalf("body is not JSON: %v\n%s", err, body) + } + for i, p := range path { + switch node := cur.(type) { + case map[string]any: + next, ok := node[p] + if !ok { + t.Fatalf("%s: no such key %q at depth %d\n%s", strings.Join(path, "."), p, i, body) + } + cur = next + case []any: + idx, err := strconv.Atoi(p) + if err != nil || idx >= len(node) { + t.Fatalf("%s: %q is not a valid index (len %d)", strings.Join(path, "."), p, len(node)) + } + cur = node[idx] + default: + t.Fatalf("%s: %q is not addressable at depth %d", strings.Join(path, "."), p, i) + } + } + obj, ok := cur.(map[string]any) + if !ok { + t.Fatalf("%s: is not an object", strings.Join(path, ".")) + } + out := make([]string, 0, len(obj)) + for k := range obj { + out = append(out, k) + } + sort.Strings(out) + return out +} + +func wantKeys(want ...string) []string { + out := append([]string(nil), want...) + sort.Strings(out) + return out +} + +func requireKeys(t *testing.T, what string, got, want []string) { + t.Helper() + if strings.Join(got, ",") == strings.Join(want, ",") { + return + } + t.Errorf("%s JSON keys drifted.\n got: %s\nwant: %s", what, + strings.Join(got, ", "), strings.Join(want, ", ")) + t.Log("A key the console does not read is harmless but means the Go side grew " + + "a field nobody agreed on: put it in web/src/lib/types.ts or take it off " + + "the wire. A key the console reads and this test does not list is a field " + + "that works today and will break the console the moment the struct changes.") +} + +// TestAdminEndpointKeySets is the table: one entry per admin read endpoint the +// console calls, with its exact key set. +func TestAdminEndpointKeySets(t *testing.T) { + h := newHarness(t, nil) + // One query, so the log and the cache-top tables have a row to shape. + // Asserting a key set against an empty list proves only the envelope. + h.query("hot.test.", 1) + h.query("hot.test.", 1) + + t.Run("status", func(t *testing.T) { + code, body := h.get("/api/status") + if code != http.StatusOK { + t.Fatalf("GET /api/status = %d: %s", code, body) + } + requireKeys(t, "GET /api/status", keysOf(t, body), + wantKeys("components", "config", "started", "uptime_s", "version")) + }) + + t.Run("dashboard", func(t *testing.T) { + code, body := h.get("/api/dashboard?range=1h") + if code != http.StatusOK { + t.Fatalf("GET /api/dashboard = %d: %s", code, body) + } + requireKeys(t, "GET /api/dashboard", keysOf(t, body), wantKeys( + "blocked_pct", "blocked_total", "blocklist_rules", "cache_entries", + "cache_hit_rate", "config_version", "degraded_reason", "errors_total", + "generation", "health", "inflight", "listeners", "node_id", + "queries_per_sec", "queries_total", "recent", "series", "top_blocked", + "top_queried", "transports", "upstreams", "uptime_seconds", "version", + )) + requireKeys(t, "GET /api/dashboard recent[0]", keysOfField(t, body, "recent", "0"), wantKeys( + "action", "blocked", "client", "client_group", "duration_ms", "id", + "list", "name", "protocol", "qtype", "qtype_name", "rcode", "rcode_name", + "response", "ts", "upstream", + )) + }) + + t.Run("blocklists", func(t *testing.T) { + code, body := h.get("/api/blocklists") + if code != http.StatusOK { + t.Fatalf("GET /api/blocklists = %d: %s", code, body) + } + requireKeys(t, "GET /api/blocklists", keysOf(t, body), + wantKeys("digest", "errors", "items", "stats")) + // items[0] is where drift item 1 bit: blocklist.Source had no json + // tags at all, so the Sources table rendered blank rows and its + // per-row toggle posted `name: undefined`. + requireKeys(t, "GET /api/blocklists items[0]", keysOfField(t, body, "items", "0"), + wantKeys("digest", "format", "meta", "name", "stats")) + requireKeys(t, "GET /api/blocklists items[0].meta", keysOfField(t, body, "items", "0", "meta"), + wantKeys("bytes", "duration", "enabled", "err", "etag", "format", + "http_status", "last_mod", "name", "path", "rules", "stale", + "updated_at", "url")) + }) + + // The rules the console's table reads all come from /api/domains, which + // serves both block and allow rules. + t.Run("domains", func(t *testing.T) { + code, body := h.get("/api/domains?per_page=5") + if code != http.StatusOK { + t.Fatalf("GET /api/domains = %d: %s", code, body) + } + requireKeys(t, "GET /api/domains", keysOf(t, body), + wantKeys("items", "page", "per_page", "total", "truncated")) + requireKeys(t, "GET /api/domains items[0]", keysOfField(t, body, "items", "0"), + wantKeys("action", "categories", "hits", "name", "rule", "source")) + }) + + t.Run("clients", func(t *testing.T) { + code, body := h.get("/api/clients") + if code != http.StatusOK { + t.Fatalf("GET /api/clients = %d: %s", code, body) + } + requireKeys(t, "GET /api/clients", keysOf(t, body), + wantKeys("default_policy", "default_rate_limit", "groups")) + // Drift item 2: the summary was flat where the console reads `match`, + // so every group card read "matches nothing" and the editor sent a + // shape the server refused. `comment` stays omitempty: a group without + // one must not carry a key the console would have to special-case. + requireKeys(t, "GET /api/clients groups[0]", keysOfField(t, body, "groups", "0"), + wantKeys("allow_domains", "blocked_count", "client_count", + "deny_domains", "log_queries", "match", "max_concurrent", "matched", + "name", "policy", "query_count", "rate_limit", "skip_blocklist", + "skip_cache", "upstreams")) + requireKeys(t, "GET /api/clients groups[0].match", keysOfField(t, body, "groups", "0", "match"), + wantKeys("any", "cidrs", "ptr_suffixes", "tags")) + }) + + t.Run("upstreams", func(t *testing.T) { + code, body := h.get("/api/upstreams") + if code != http.StatusOK { + t.Fatalf("GET /api/upstreams = %d: %s", code, body) + } + requireKeys(t, "GET /api/upstreams", keysOf(t, body), wantKeys( + "bootstraps", "breaker_cooldown", "breaker_threshold", + "healthcheck_interval", "max_attempts", "policy", "servers", + )) + requireKeys(t, "GET /api/upstreams servers[0]", keysOfField(t, body, "servers", "0"), + wantKeys("addr", "avg_ms", "breaker_open", "fail_rate", "failures", + "health", "latency_ms", "p95_ms", "proto", "queries", "server_name", + "weight")) + }) + + t.Run("logs", func(t *testing.T) { + code, body := h.get("/api/logs?limit=10") + if code != http.StatusOK { + t.Fatalf("GET /api/logs = %d: %s", code, body) + } + requireKeys(t, "GET /api/logs", keysOf(t, body), + wantKeys("has_more", "items", "limit", "offset", "total")) + // Every key on every row. These were omitempty, so a row without an + // ACL group or a blocklist arrived without the key and the console + // rendered "undefined" instead of an empty cell. + requireKeys(t, "GET /api/logs items[0]", keysOfField(t, body, "items", "0"), wantKeys( + "action", "blocked", "client", "client_group", "duration_ms", "id", + "list", "name", "protocol", "qtype", "qtype_name", "rcode", "rcode_name", + "response", "ts", "upstream", + )) + }) + + t.Run("cache", func(t *testing.T) { + code, body := h.get("/api/cache") + if code != http.StatusOK { + t.Fatalf("GET /api/cache = %d: %s", code, body) + } + requireKeys(t, "GET /api/cache", keysOf(t, body), wantKeys( + "bytes", "bytes_human", "enabled", "entries", "evictions", "hit_rate", + "hits", "max_ttl_ms", "min_ttl_ms", "misses", "negative_ttl_ms", + "persisted", "serve_stale_ms", "shards", "size", "stale_hits", + )) + requireKeys(t, "GET /api/cache persisted", keysOfField(t, body, "persisted"), wantKeys( + "bytes", "enabled", "entries", "errors", "last_snapshot", "loaded", + "path", "writes", + )) + }) + + t.Run("cache top", func(t *testing.T) { + code, body := h.get("/api/cache/top?limit=5") + if code != http.StatusOK { + t.Fatalf("GET /api/cache/top = %d: %s", code, body) + } + requireKeys(t, "GET /api/cache/top", keysOf(t, body), wantKeys("items")) + // There is no `count` and no `last_seen`: the cache tracks a hit + // counter and a byte count per name, and the console's "Lookups" and + // "Last seen" columns were reading fields that never existed. + requireKeys(t, "GET /api/cache/top items[0]", keysOfField(t, body, "items", "0"), + wantKeys("bytes", "hits", "name", "qtype", "qtype_name")) + }) + + t.Run("settings", func(t *testing.T) { + code, body := h.get("/api/settings") + if code != http.StatusOK { + t.Fatalf("GET /api/settings = %d: %s", code, body) + } + requireKeys(t, "GET /api/settings", keysOf(t, body), wantKeys( + "config_yaml", "defaults_yaml", "effective_yaml", "last_error", + "path", "reload_available", "version", "writable", + )) + }) + + t.Run("whoami", func(t *testing.T) { + code, body := h.get("/api/whoami") + if code != http.StatusOK { + t.Fatalf("GET /api/whoami = %d: %s", code, body) + } + requireKeys(t, "GET /api/whoami", keysOf(t, body), + wantKeys("auth_mode", "base_path", "can_write", "mode", "name", "read_only")) + }) +} + +// TestWriteEndpointKeySets pins the small write responses. +func TestWriteEndpointKeySets(t *testing.T) { + h := newHarness(t, nil) + + t.Run("blocklist refresh", func(t *testing.T) { + code, body := h.do(http.MethodPost, "/api/blocklists/refresh", `{"names":[]}`) + if code != http.StatusOK { + t.Fatalf("POST /api/blocklists/refresh = %d: %s", code, body) + } + requireKeys(t, "POST /api/blocklists/refresh", keysOf(t, body), wantKeys("names", "started")) + // The toast used to say "0 sources queued": names is a slice, and a + // nil slice marshals to null, which the console counted as zero. + var got struct { + Names []string `json:"names"` + } + if err := json.Unmarshal(body, &got); err != nil { + t.Fatal(err) + } + if got.Names == nil { + t.Errorf("names is null, so the console's source count is a lie: %s", body) + } + }) + + t.Run("cache flush", func(t *testing.T) { + code, body := h.do(http.MethodPost, "/api/cache/flush", "") + if code != http.StatusOK { + t.Fatalf("POST /api/cache/flush = %d: %s", code, body) + } + requireKeys(t, "POST /api/cache/flush", keysOf(t, body), wantKeys("entries", "ok")) + }) + + t.Run("acl save", func(t *testing.T) { + code, body := h.do(http.MethodPut, "/api/clients", `{"groups":[],"default_policy":"allow"}`) + if code != http.StatusOK { + t.Fatalf("PUT /api/clients = %d: %s", code, body) + } + requireKeys(t, "PUT /api/clients", keysOf(t, body), wantKeys("ok", "version")) + }) + + t.Run("upstream save", func(t *testing.T) { + code, body := h.do(http.MethodPut, "/api/upstreams", + `{"policy":"parallel","servers":[{"addr":"127.0.0.1:5300","proto":"udp","weight":1}],`+ + `"bootstraps":[],"max_attempts":1,"breaker_threshold":5,"healthcheck_interval":"30s"}`) + if code != http.StatusOK { + t.Fatalf("PUT /api/upstreams = %d: %s", code, body) + } + requireKeys(t, "PUT /api/upstreams", keysOf(t, body), wantKeys("ok", "version")) + }) +} + +// TestConsoleWritePayloadsAreAccepted drives the two write endpoints with the +// exact payload web/src builds. Both answered 400 with `json: unknown field +// ...`, and no hand-written Go test found it: the suite never sent what the +// console actually sends. +func TestConsoleWritePayloadsAreAccepted(t *testing.T) { + h := newHarness(t, nil) + + t.Run("clients", func(t *testing.T) { + // web/src/pages/Clients.tsx toGroup(): match is nested, and the + // console sends the ClientGroup shape without the server-owned + // counters (client_count, query_count, blocked_count, matched). + const body = `{"groups":[{ + "name":"console", + "comment":"from the console", + "policy":"allow", + "match":{"cidrs":["10.0.0.0/8"],"ptr_suffixes":["lan.example."],"tags":["laptop"],"any":false}, + "allow_domains":["*.example.net"], + "deny_domains":["*.ads.example.net"], + "skip_blocklist":false, + "skip_cache":false, + "rate_limit":10, + "max_concurrent":0, + "log_queries":true, + "upstreams":[] + }],"default_policy":"allow"}` + code, res := h.do(http.MethodPut, "/api/clients", body) + if code != http.StatusOK { + t.Fatalf("PUT /api/clients with the console payload = %d: %s", code, res) + } + // And it must round-trip: what the console sent comes back in the + // shape it sent, or the editor empties itself on refetch. + _, list := h.get("/api/clients") + var got struct { + Groups []struct { + Name string `json:"name"` + Match struct { + CIDRs []string `json:"cidrs"` + Tags []string `json:"tags"` + } `json:"match"` + } `json:"groups"` + } + if err := json.Unmarshal(list, &got); err != nil { + t.Fatal(err) + } + if len(got.Groups) != 1 { + t.Fatalf("the saved group did not come back: %s", list) + } + if got.Groups[0].Name != "console" { + t.Errorf("group name = %q, want console", got.Groups[0].Name) + } + if len(got.Groups[0].Match.CIDRs) != 1 || got.Groups[0].Match.CIDRs[0] != "10.0.0.0/8" { + t.Errorf("match.cidrs = %v, want [10.0.0.0/8]; the nested shape must survive", got.Groups[0].Match.CIDRs) + } + if len(got.Groups[0].Match.Tags) != 1 { + t.Errorf("match.tags = %v, want [laptop]", got.Groups[0].Match.Tags) + } + }) + + t.Run("upstreams", func(t *testing.T) { + // web/src/pages/Upstreams.tsx save(): the console sends the whole + // server object it read, including the runtime health fields, with a + // comment saying the server ignores them. Refusing them is drift + // item 4 — every save in the console answered 400. + const body = `{"policy":"failover","max_attempts":2,"breaker_threshold":5, + "healthcheck_interval_s":30, + "servers":[{"addr":"127.0.0.1:5300","proto":"udp","server_name":"", + "insecure_skip_verify":false,"weight":1,"comment":"", + "health":"down","latency_ms":0,"fail_rate":0,"breaker_open":false, + "queries":0,"avg_ms":0,"p95_ms":0}], + "bootstraps":[{"name":"quad9","addresses":["9.9.9.9"],"refresh":"24h"}]}` + code, res := h.do(http.MethodPut, "/api/upstreams", body) + if code != http.StatusOK { + t.Fatalf("PUT /api/upstreams with the console payload = %d: %s", code, res) + } + // The duration string form must survive the round trip too, or the + // console's duration inputs show a nanosecond count. + _, list := h.get("/api/upstreams") + var got struct { + HealthCheck json.RawMessage `json:"healthcheck_interval"` + Cooldown json.RawMessage `json:"breaker_cooldown"` + Servers []struct { + Addr string `json:"addr"` + } `json:"servers"` + Bootstraps []struct { + Name string `json:"name"` + Refresh json.RawMessage `json:"refresh"` + } `json:"bootstraps"` + } + if err := json.Unmarshal(list, &got); err != nil { + t.Fatal(err) + } + if string(got.HealthCheck) != `"30s"` { + t.Errorf("healthcheck_interval = %s, want \"30s\"", got.HealthCheck) + } + if string(got.Cooldown) == "" { + t.Error("breaker_cooldown disappeared from the response") + } + if len(got.Servers) != 1 || got.Servers[0].Addr != "127.0.0.1:5300" { + t.Fatalf("servers = %+v, want the saved 127.0.0.1:5300", got.Servers) + } + if len(got.Bootstraps) != 1 || got.Bootstraps[0].Name != "quad9" { + t.Fatalf("bootstraps = %+v, want the saved quad9", got.Bootstraps) + } + // Go's canonical spelling of 24h, which is what Duration.MarshalJSON + // writes and what the console's duration parser accepts. + if string(got.Bootstraps[0].Refresh) != `"24h0m0s"` { + t.Errorf("bootstraps[0].refresh = %s, want a duration string", got.Bootstraps[0].Refresh) + } + }) +} diff --git a/internal/app/dump_shapes_test.go b/internal/app/dump_shapes_test.go new file mode 100644 index 0000000..5c6cd2a --- /dev/null +++ b/internal/app/dump_shapes_test.go @@ -0,0 +1,56 @@ +package app + +import ( + "encoding/json" + "net/http" + "os" + "sort" + "testing" +) + +// TestDumpShapes prints the real shape of every admin endpoint. It is a +// development aid, not an assertion: run it with +// +// go test ./internal/app/ -run TestDumpShapes -v +// +// when changing a response struct, and diff the output against +// web/src/lib/types.ts. Delete it once the contract test covers everything. +func TestDumpShapes(t *testing.T) { + if os.Getenv("DUMP_SHAPES") == "" { + t.Skip("set DUMP_SHAPES=1") + } + h := newHarness(t, nil) + h.query("hot.test.", 1) + for _, p := range []string{ + "/api/status", "/api/dashboard?range=1h", "/api/blocklists", + "/api/domains?per_page=3", "/api/clients", "/api/upstreams", + "/api/logs?limit=3", "/api/cache", "/api/cache/top?limit=3", + "/api/settings", "/api/whoami", + } { + code, body := h.get(p) + var keys []string + var m map[string]json.RawMessage + if json.Unmarshal(body, &m) == nil { + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + } + t.Logf("%-28s %d %v", p, code, keys) + var pretty any + if json.Unmarshal(body, &pretty) == nil { + b, _ := json.MarshalIndent(pretty, " ", " ") + if len(b) > 1200 { + b = b[:1200] + } + t.Logf("%s", b) + } + } + for _, m := range []struct{ method, path, body string }{ + {http.MethodPost, "/api/blocklists/refresh", `{"names":[]}`}, + {http.MethodPost, "/api/cache/flush", ""}, + } { + code, body := h.do(m.method, m.path, m.body) + t.Logf("%s %s -> %d %s", m.method, m.path, code, body) + } +} diff --git a/internal/app/hooks.go b/internal/app/hooks.go index 9bef394..459882e 100644 --- a/internal/app/hooks.go +++ b/internal/app/hooks.go @@ -535,13 +535,17 @@ func (a *App) getUpstreams() any { servers = append(servers, s) } return map[string]any{ - "policy": cfg.Upstream.Policy, - "servers": servers, + "policy": cfg.Upstream.Policy, + "servers": servers, + // Durations on the wire are duration strings, everywhere: this is the + // same spelling config.yaml uses and the same one Duration.MarshalJSON + // writes for bootstraps[].refresh. It used to be a float of seconds + // while the console declared the field a string. "bootstraps": cfg.Upstream.Bootstraps, - "healthcheck_interval": cfg.Upstream.HealthCheck.D().Seconds(), + "healthcheck_interval": cfg.Upstream.HealthCheck, "max_attempts": cfg.Upstream.MaxAttempts, "breaker_threshold": cfg.Upstream.BreakerThreshold, - "breaker_cooldown": cfg.Upstream.BreakerCooldown.D().Seconds(), + "breaker_cooldown": cfg.Upstream.BreakerCooldown, } } diff --git a/internal/blocklist/example_test.go b/internal/blocklist/example_test.go index f31f585..9666f42 100644 --- a/internal/blocklist/example_test.go +++ b/internal/blocklist/example_test.go @@ -32,13 +32,13 @@ func TestExampleRPZZoneBlocksWhatItSays(t *testing.T) { {"*.doubleclick.net.", ActionNXDomain, ""}, {"*.ads.example.com.", ActionNXDomain, ""}, {"*.tracker.example.net.", ActionNXDomain, ""}, - // A bare owner name in a policy zone is given the subdomain form, - // which covers the name's children but not the name itself; see - // TestExampleRPZZoneApexIsNotCovered. - {"*.analytics.example.org.", ActionNXDomain, ""}, + // A bare owner name takes the apex form, so the record covers the + // domain and everything under it; see + // TestExampleRPZZoneCoversTheApexItNames. + {"**.analytics.example.org.", ActionNXDomain, ""}, {"*.noisy-cdn.example.", ActionDrop, ""}, - {"*.metrics.internal.", ActionPassthrough, ""}, - {"*.blocked.example.", ActionRedirect, "192.0.2.10"}, + {"**.metrics.internal.", ActionPassthrough, ""}, + {"**.blocked.example.", ActionRedirect, "192.0.2.10"}, } got := make([]ruleWant, 0, len(rules)) for _, r := range rules { @@ -103,20 +103,18 @@ func TestExampleRPZZoneMatchesTheNamesItNames(t *testing.T) { } } -// TestExampleRPZZoneApexIsNotCovered pins a real gap rather than an intention. +// TestExampleRPZZoneCoversTheApexItNames pins RPZ semantics on the shipped +// policy zone: a record names a domain, and it applies to that domain AND +// everything below it. // -// An RPZ record names the domain it applies to, and in RPZ semantics that means -// the domain and everything below it. rpzRule gives a bare owner name the -// subdomain form ("*.name"), and a subdomain-form rule does not match the name -// itself — so `analytics.example.org.` in a policy zone blocks -// sub.analytics.example.org. and leaves analytics.example.org. resolving -// normally. The apex form ("**.name") is what covers both. -// -// Not changed here: it alters which queries get blocked for anyone already -// running a policy zone, and the previous round's decision was to pin the -// open questions rather than change behaviour. If this is fixed, the fix is one -// character in rpzRule ("*." -> "**.") plus this test and the STATUS.md entry. -func TestExampleRPZZoneApexIsNotCovered(t *testing.T) { +// This was a real gap, pinned rather than fixed for several rounds: rpzRule +// gave a bare owner name the strict-subdomain form ("*.name"), so +// `analytics.example.org.` blocked cdn.analytics.example.org. and left +// analytics.example.org. resolving normally. DECIDED: rpzRule now uses the +// apex form ("**.name") for a bare owner, which is what RFC 7766 says. An +// owner written with an explicit "*." is still strict, so a zone that wants +// subdomain-only behaviour can ask for it. +func TestExampleRPZZoneCoversTheApexItNames(t *testing.T) { path := filepath.Join("..", "..", "deploy", "zones", "local.rpz.zone.example") rules, _, err := parseRPZFiles([]string{path}) if err != nil { @@ -128,16 +126,33 @@ func TestExampleRPZZoneApexIsNotCovered(t *testing.T) { } set := b.Build() - for _, name := range []string{"doubleclick.net.", "ads.example.com.", "analytics.example.org."} { - if e := set.Match(name); e != nil { - t.Errorf("%s matched %v; a policy record for a domain is expected to cover "+ - "the domain itself as well as its subdomains (see STATUS.md)", name, e) + // The record for analytics.example.org. was written with a bare owner, so + // it covers the apex as well as its children. + for _, name := range []string{"analytics.example.org.", "cdn.analytics.example.org."} { + e := set.Match(name) + if e == nil { + t.Errorf("%s is not matched by the example policy zone", name) + continue + } + if e.IsAllow() { + t.Errorf("%s matched an allow rule, want a block", name) } } - // The same gap on the other side: an rpz-passthru. for the apex does not - // allowlist the apex, so a name on the blocklist stays blocked. - if e := set.Match("metrics.internal."); e != nil && e.IsAllow() { - t.Errorf("metrics.internal. matched the allow rule %v; the apex of an "+ - "rpz-passthru. record is not covered", e) + // The same on the allow side: an rpz-passthru. for a bare owner must + // allowlist the apex too, or a name on the blocklist stays blocked. + if e := set.Match("metrics.internal."); e == nil || !e.IsAllow() { + t.Errorf("metrics.internal. = %v, want the rpz-passthru allow rule to cover the apex", e) + } + // A bare-owner redirect covers its apex as well, which is the point of + // pointing a name at a landing page. + if e := set.Match("blocked.example."); e == nil || e.Action != ActionRedirect { + t.Errorf("blocked.example. = %v, want the redirect to cover the apex", e) + } + // The example's other records carry an explicit "*." owner, so they stay + // strict and the pin above is not masking a blanket change of behaviour. + for _, name := range []string{"doubleclick.net.", "ads.example.com."} { + if e := set.Match(name); e != nil { + t.Errorf("%s matched %v; an explicit \"*.\" owner must stay strict", name, e) + } } } diff --git a/internal/blocklist/manager.go b/internal/blocklist/manager.go index c4fea9d..bc039ba 100644 --- a/internal/blocklist/manager.go +++ b/internal/blocklist/manager.go @@ -24,13 +24,13 @@ import ( // Source is a compiled list plus the bookkeeping the UI shows. type Source struct { - Name string - Format Format - Meta SourceMeta - Stats ParseStats + Name string `json:"name"` + Format Format `json:"format"` + Meta SourceMeta `json:"meta"` + Stats ParseStats `json:"stats"` // Digest identifies the compiled content, so instances can tell whether // they hold the same rules without shipping the rules around. - Digest string + Digest string `json:"digest"` } // State is the compiled blocklist plus its per-source detail. diff --git a/internal/blocklist/parse.go b/internal/blocklist/parse.go index d68ee35..595403b 100644 --- a/internal/blocklist/parse.go +++ b/internal/blocklist/parse.go @@ -489,7 +489,13 @@ func rpzRule(rec zonefile.Record, ttl uint32, opt ParseOptions) (Rule, bool) { return Rule{}, false } if !strings.HasPrefix(pattern, "*") && opt.Subdomains { - pattern = "*." + pattern + // RPZ semantics: a record names a domain and applies to that domain + // AND everything under it, so a bare owner name takes the apex form + // "**.". The subdomain form "*." would block sub.name and leave name + // itself resolving, which is not what a policy record says. An owner + // written as "*." is left exactly as written, so a zone that wants + // strict-subdomain behaviour can still ask for it. + pattern = "**." + pattern } return Rule{Name: pattern, Action: action, Response: resp, TTL: ttl}, true } diff --git a/internal/blocklist/parse_test.go b/internal/blocklist/parse_test.go index e23198b..a2449b2 100644 --- a/internal/blocklist/parse_test.go +++ b/internal/blocklist/parse_test.go @@ -281,15 +281,17 @@ func TestParseRPZ(t *testing.T) { t.Fatalf("Parse: %v", err) } want := []string{ - // A bare owner name becomes the STRICT-subdomain form "*." (RPZ - // semantics: the policy covers what is under the name), unlike the - // hosts parser which uses "**.". Zone owner names keep their - // trailing dot; Builder.Add strips it at compile time. - "*.example.com. nxdomain ttl=3600", + // A bare owner name becomes the APEX form "**." (RPZ semantics: the + // policy covers the name and everything under it), unlike the hosts + // parser's strict "*." and the hosts parser's "**." for a leading + // dot. An owner written with an explicit wildcard is left alone. Zone + // owner names keep their trailing dot; Builder.Add strips it at + // compile time. + "**.example.com. nxdomain ttl=3600", "*.sub.example.org. drop ttl=3600", "**.both.example.net. passthrough ttl=3600", - "*.redirect.example.gov. redirect ttl=3600 resp=127.0.0.2", - "*.redirect6.example.gov. redirect ttl=3600 resp=::1", + "**.redirect.example.gov. redirect ttl=3600 resp=127.0.0.2", + "**.redirect6.example.gov. redirect ttl=3600 resp=::1", } if got := ruleStrings(rules); !sameRules(got, want) { t.Errorf("rules = %v,\nwant %v", got, want) @@ -318,19 +320,27 @@ func TestParseRPZ(t *testing.T) { if err != nil { t.Fatalf("Parse: %v", err) } - if got := ruleStrings(cname); !sameRules(got, []string{"*.shop.example.com. cname ttl=60 resp=parked.example.net."}) { + if got := ruleStrings(cname); !sameRules(got, []string{"**.shop.example.com. cname ttl=60 resp=parked.example.net."}) { t.Errorf("cname rules = %v", got) } - // The compiled forms behave as documented: "*." skips the apex while - // "**." covers it. + // The compiled forms behave as documented: a bare owner name covers the + // apex AND its children, an explicit "*." covers only children. s := mustBuild(rules...) - if e := s.Match("example.com"); e != nil { - t.Errorf("RPZ \"example.com CNAME .\" matched the apex: %q, want nil (strict \"*.\")", e.Name) + if e := s.Match("example.com"); e == nil || e.Action != ActionNXDomain { + t.Errorf("example.com: %+v, want nxdomain (a bare owner name covers the apex)", e) } if e := s.Match("www.example.com"); e == nil || e.Action != ActionNXDomain { t.Errorf("www.example.com: %+v, want nxdomain", e) } + // "*.sub.example.org" was written with an explicit wildcard, so it stays + // strict: the subdomain is blocked, the apex is not. + if e := s.Match("sub.example.org"); e != nil { + t.Errorf("sub.example.org: %+v, want nil (an explicit \"*.\" owner is strict)", e) + } + if e := s.Match("deep.sub.example.org"); e == nil || e.Action != ActionDrop { + t.Errorf("deep.sub.example.org: %+v, want drop", e) + } if e := s.Match("both.example.net"); e == nil || e.Action != ActionPassthrough { t.Errorf("both.example.net: %+v, want passthrough (\"**.\" covers the apex)", e) } diff --git a/internal/blocklist/set.go b/internal/blocklist/set.go index 7cb4e3f..9c9215c 100644 --- a/internal/blocklist/set.go +++ b/internal/blocklist/set.go @@ -196,20 +196,20 @@ type RuleCounts struct { // SourceMeta describes one contributing source. type SourceMeta struct { - Name string - Format string - Rules int - Bytes int64 - ETag string - LastMod string - UpdatedAt time.Time - Duration time.Duration - Err string - Enabled bool - URL string - Path string - Stale bool - HTTPStatus int + Name string `json:"name"` + Format string `json:"format"` + Rules int `json:"rules"` + Bytes int64 `json:"bytes"` + ETag string `json:"etag"` + LastMod string `json:"last_mod"` + UpdatedAt time.Time `json:"updated_at"` + Duration time.Duration `json:"duration"` + Err string `json:"err"` + Enabled bool `json:"enabled"` + URL string `json:"url"` + Path string `json:"path"` + Stale bool `json:"stale"` + HTTPStatus int `json:"http_status"` } // Builder accumulates rules and produces a Set. diff --git a/internal/cluster/cluster.go b/internal/cluster/cluster.go index 3721c25..f5c2d21 100644 --- a/internal/cluster/cluster.go +++ b/internal/cluster/cluster.go @@ -43,6 +43,12 @@ const ( EventBye EventKind = "bye" ) +// defaultPeerTTL mirrors config.Default()'s cluster.peer_ttl. It is the +// fallback for a config that leaves it unset: the ticker and the key expiry +// both divide by it, and a zero period panics NewTicker and turns the key +// expiry into a delete. +const defaultPeerTTL = 15 * time.Second + // Event is a cluster message. type Event struct { Kind EventKind `json:"kind"` @@ -70,6 +76,25 @@ type NodeState struct { Listeners []string `json:"listeners"` // Leader is true when this node holds the list-refresh lease. Leader bool `json:"leader"` + // LastSeen is when this node was last alive, as recorded by whoever holds + // this state: the publishing node stamps it into the keyed record it + // leaves in Valkey, and a receiver stamps it again with its own clock when + // the heartbeat arrives. It exists so a state can be aged out (see + // Cluster.Peers) and so the console can show how stale a peer is. A zero + // value means "unknown" — a record written by a build without this field — + // and is treated as live rather than stale, so a rolling upgrade never + // empties the peer list. + LastSeen time.Time `json:"last_seen,omitempty"` +} + +// peer is a node's last published state plus the local clock reading when it +// arrived. seenAt is deliberately kept out of the wire format and out of any +// comparison against a remote clock: ageing is decided by how long *this* +// process has heard nothing, so clock skew between two machines can neither +// reap a healthy peer nor keep a dead one. +type peer struct { + state NodeState + seenAt time.Time } // Handler receives cluster events from peers. @@ -113,7 +138,7 @@ type Cluster struct { lastEvent atomic.Int64 // seen is the last state per node id. seenMu sync.RWMutex - seen map[string]NodeState + seen map[string]peer stop chan struct{} wg sync.WaitGroup @@ -130,7 +155,7 @@ func New(opt Options) *Cluster { log: log.With("component", "cluster"), opt: opt, node: opt.Version, - seen: make(map[string]NodeState), + seen: make(map[string]peer), stop: make(chan struct{}), } if cfg := opt.Config.Get(); cfg != nil && cfg.Cluster.NodeID != "" { @@ -276,7 +301,10 @@ func (c *Cluster) runSession(ctx context.Context) error { c.connected.Store(true) c.log.Info("cluster subscribed", "channel", c.channel(EventHeartbeat)) - hb := time.NewTicker(c.cfg.Get().Cluster.PeerTTL.D() / 2) + // c.peerTTL() rather than the raw config value: time.NewTicker panics on a + // non-positive period, so a peer_ttl of 0 would take the process down at + // Start rather than falling back to the default. + hb := time.NewTicker(c.peerTTL() / 2) defer hb.Stop() ch := sub.Channel() @@ -298,6 +326,10 @@ func (c *Cluster) runSession(ctx context.Context) error { continue } c.connected.Store(true) + // Age out peers nobody has heard from in peerDeadline. The reap + // also runs from Peers, so this is about not growing the map + // while nobody is reading the API — not about correctness. + c.reapStalePeers() c.renewLease(ctx) case msg, ok := <-ch: if !ok { @@ -312,9 +344,7 @@ func (c *Cluster) runSession(ctx context.Context) error { } c.lastEvent.Store(time.Now().Unix()) if ev.State != nil { - c.seenMu.Lock() - c.seen[ev.NodeID] = *ev.State - c.seenMu.Unlock() + c.remember(ev.NodeID, *ev.State) } if ev.Kind == EventBye { c.seenMu.Lock() @@ -369,9 +399,54 @@ func (c *Cluster) State() *NodeState { st.BlocklistBuilt = built st.ConfigDigest = cfgDigest } + // Stamped last, so a StateFunc cannot overwrite it with its own idea of + // when this node was last alive. + st.LastSeen = time.Now() return st } +// peerTTL is the configured heartbeat interval, or the default when it is +// unset — a zero peer_ttl must not make every peer instantly stale. +func (c *Cluster) peerTTL() time.Duration { + if c.cfg != nil { + if d := c.cfg.Get().Cluster.PeerTTL.D(); d > 0 { + return d + } + } + return defaultPeerTTL +} + +// peerDeadline is how long a peer may go unheard before it is reaped: four +// missed heartbeats. Long enough that a GC pause, a blocked scheduler or a +// Valkey restart does not empty the peer list; short enough that a crashed pod +// is gone from the UI before an operator goes looking for it. +func (c *Cluster) peerDeadline() time.Duration { return c.peerTTL() * 4 } + +// remember records a peer's state together with the time it arrived. +func (c *Cluster) remember(id string, st NodeState) { + c.seenMu.Lock() + c.seen[id] = peer{state: st, seenAt: time.Now()} + c.seenMu.Unlock() +} + +// reapStalePeers forgets peers unheard for longer than peerDeadline and +// reports how many went. It runs on the heartbeat tick so the map cannot grow +// without bound when nobody reads the API, and from Peers so a reader can never +// observe a dead node. +func (c *Cluster) reapStalePeers() int { + cutoff := time.Now().Add(-c.peerDeadline()) + c.seenMu.Lock() + defer c.seenMu.Unlock() + n := 0 + for id, p := range c.seen { + if p.seenAt.Before(cutoff) { + delete(c.seen, id) + n++ + } + } + return n +} + // publishState broadcasts this instance's heartbeat. It reports whether the // write reached Valkey: the heartbeat is the only liveness signal this package // has, because go-redis's Subscribe reconnects transparently and runSession @@ -392,7 +467,10 @@ func (c *Cluster) publishState(ctx context.Context) error { // Also keep a keyed record so a late-joining instance can discover the // existing set without waiting a full heartbeat interval. pipe.HSet(ctx, c.heartbeatKey(), c.node, mustJSON(st)) - pipe.Expire(ctx, c.heartbeatKey(), c.cfg.Get().Cluster.PeerTTL.D()*4) + // c.peerDeadline() so an unset peer_ttl cannot turn this into + // EXPIRE key 0, which *deletes* the key and would make every instance + // believe it is the only one in the fleet. + pipe.Expire(ctx, c.heartbeatKey(), c.peerDeadline()) if _, err := pipe.Exec(ctx); err != nil { return err } @@ -407,12 +485,22 @@ func mustJSON(v any) string { return string(b) } -// Peers returns the last known state of other instances. +// Peers returns the last known state of every other instance heard from within +// peerDeadline. A node that stops heartbeating is dropped: a peer list that +// only ever grows reports a fleet that is not there any more, and a dead peer +// carrying an old blocklist digest is flagged as divergent forever. func (c *Cluster) Peers() []NodeState { + // Reap here as well as on the tick, so what a caller reads is reaped + // whether or not the maintain loop is still ticking — it is not, during a + // Valkey outage, and that is exactly when a peer that died at the same + // moment is most likely to be missed. + c.reapStalePeers() c.seenMu.RLock() defer c.seenMu.RUnlock() out := make([]NodeState, 0, len(c.seen)) - for _, s := range c.seen { + for _, p := range c.seen { + s := p.state + s.LastSeen = p.seenAt out = append(out, s) } sort.Slice(out, func(i, j int) bool { return out[i].NodeID < out[j].NodeID }) @@ -605,12 +693,19 @@ func (c *Cluster) Discover(ctx context.Context) []NodeState { ids = append(ids, id) } sort.Strings(ids) + cutoff := time.Now().Add(-c.peerDeadline()) for _, id := range ids { if id == c.node { continue } var st NodeState if err := json.Unmarshal([]byte(fields[id]), &st); err == nil { + // The hash outlives a crash: a live node keeps refreshing the + // key's TTL, so a dead node's field would stay readable for the + // life of the fleet. Age it out on the publisher's own stamp. + if !st.LastSeen.IsZero() && st.LastSeen.Before(cutoff) { + continue + } out = append(out, st) } } diff --git a/internal/cluster/cluster_test.go b/internal/cluster/cluster_test.go index 912ee18..4bfc597 100644 --- a/internal/cluster/cluster_test.go +++ b/internal/cluster/cluster_test.go @@ -488,8 +488,8 @@ func TestStatusReportsDivergentPeers(t *testing.T) { // And a peer with no blocklist at all (empty digest) is not divergence: // the rule requires both sides to be non-empty. db.set("", 0) - waitFor(t, "b's state to be republished with an empty digest", 3*time.Second, func() bool { - return peerDigest(b, a.NodeID()) == "" + waitFor(t, "a's view of b to be republished with an empty digest", 3*time.Second, func() bool { + return peerDigest(a, b.NodeID()) == "" }) if got := a.Status(ctx).Divergent; len(got) != 0 { t.Errorf("a peer with no blocklist is reported divergent: %v", got) @@ -599,13 +599,19 @@ func TestHeartbeatAppearsInPeersWithItsIdentity(t *testing.T) { if hasPeer(a.Peers(), a.NodeID()) { t.Error("a node appears in its own peer list") } - if !hasPeer(b.Peers(), a.NodeID()) { - t.Error("b does not see a in Peers()") - } + // The other direction has to be waited for rather than read: a joined + // *after* a's heartbeat was already in flight, so a hearing b is + // immediate while b hears a only on a's next tick. Reading b here is a + // race that loses almost every run, and it has nothing to do with the + // behaviour under test. + waitFor(t, "b to see a in Peers()", 3*time.Second, func() bool { + return hasPeer(b.Peers(), a.NodeID()) + }) // The blocklist digest travels with the heartbeat: that is what the - // divergence rule compares. - waitFor(t, "a to learn b's blocklist digest", 3*time.Second, func() bool { - return peerDigest(a, b.NodeID()) == "rules-a" || peerDigest(a, b.NodeID()) != "" + // divergence rule compares. b has no digests configured, so a's view of + // b is legitimately empty and b's view of a is the one that must arrive. + waitFor(t, "b to learn a's blocklist digest", 3*time.Second, func() bool { + return peerDigest(b, a.NodeID()) == "rules-a" }) if got := peerDigest(a, b.NodeID()); got != "" { t.Errorf("unexpected digest %q from a node with no digests configured", got) diff --git a/internal/config/config.go b/internal/config/config.go index d03fb28..37de19d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,6 +9,7 @@ package config import ( "bytes" + "encoding/json" "errors" "fmt" "math" @@ -61,6 +62,37 @@ func (d *Duration) UnmarshalYAML(value *yaml.Node) error { return fmt.Errorf("invalid duration: %s", value.Value) } +// MarshalJSON writes the Go duration string ("30s"), matching MarshalYAML and +// what the console's types declare. Without it a config.Duration on the wire +// became a nanosecond count, so `bootstraps[].refresh` arrived as 30000000000 +// and the console's duration field was nonsense. +func (d Duration) MarshalJSON() ([]byte, error) { + return json.Marshal(time.Duration(d).String()) +} + +// UnmarshalJSON accepts the duration string the server writes, and a bare +// number as SECONDS — not nanoseconds, which is what encoding/json would do by +// default and what nobody means. Accepting the number keeps a hand-written +// client (`{"healthcheck_interval": 30}`) working instead of silently meaning +// 30ns. +func (d *Duration) UnmarshalJSON(b []byte) error { + var s string + if err := json.Unmarshal(b, &s); err == nil { + v, err := time.ParseDuration(strings.TrimSpace(s)) + if err != nil { + return fmt.Errorf("invalid duration %q: %w", s, err) + } + *d = Duration(v) + return nil + } + var n float64 + if err := json.Unmarshal(b, &n); err != nil { + return fmt.Errorf("duration must be a string or a number of seconds: %s", b) + } + *d = Duration(n * float64(time.Second)) + return nil +} + func (d Duration) MarshalYAML() (any, error) { return time.Duration(d).String(), nil } func (d Duration) D() time.Duration { return time.Duration(d) } @@ -186,28 +218,46 @@ type Upstream struct { Bootstraps []UpstreamBootstrap `yaml:"bootstraps"` } +// UpstreamServer is one resolver. The json tags exist for the same reason as +// config.Group's: PUT /api/upstreams decodes the console's payload with +// DisallowUnknownFields, and the console round-trips the whole server object +// it read from GET /api/upstreams — including the runtime health fields the +// server owns. Accepting them (and ignoring them) is the contract; refusing +// them made every save in the console answer 400. type UpstreamServer struct { // Addr is host:port; host may be an IP or a name resolved via Bootstraps. - Addr string `yaml:"addr"` + Addr string `yaml:"addr" json:"addr"` // Proto is "udp", "tcp" or "tls" (DoT, 8.4.1). - Proto string `yaml:"proto"` + Proto string `yaml:"proto" json:"proto"` // ServerName / InsecureSkipVerify are only used with proto: tls. - ServerName string `yaml:"server_name"` - InsecureSkipVerify bool `yaml:"insecure_skip_verify"` + ServerName string `yaml:"server_name" json:"server_name"` + InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"` // HealthPath is a hostname used for health probes instead of the Addr host. - HealthName string `yaml:"health_name"` + HealthName string `yaml:"health_name" json:"health_name"` // Weight biases random selection (default 1). - Weight int `yaml:"weight"` + Weight int `yaml:"weight" json:"weight"` // MaxFails before the breaker opens; 0 uses the global threshold. - MaxFails int `yaml:"max_fails"` - Comment string `yaml:"comment,omitempty"` + MaxFails int `yaml:"max_fails" json:"max_fails"` + Comment string `yaml:"comment,omitempty" json:"comment,omitempty"` + // Health and the counters below are read-only runtime state. They are + // accepted on a write and ignored, because the server owns them; the + // console sends back the object it read. + Health string `yaml:"-" json:"health,omitempty"` + LatencyMS float64 `yaml:"-" json:"latency_ms,omitempty"` + AvgMS float64 `yaml:"-" json:"avg_ms,omitempty"` + P95MS float64 `yaml:"-" json:"p95_ms,omitempty"` + FailRate float64 `yaml:"-" json:"fail_rate,omitempty"` + Queries uint64 `yaml:"-" json:"queries,omitempty"` + Failures uint64 `yaml:"-" json:"failures,omitempty"` + BreakerOpen bool `yaml:"-" json:"breaker_open,omitempty"` + LastError string `yaml:"-" json:"last_error,omitempty"` } type UpstreamBootstrap struct { - Name string `yaml:"name"` - Addresses []string `yaml:"addresses"` + Name string `yaml:"name" json:"name"` + Addresses []string `yaml:"addresses" json:"addresses"` // Refresh re-resolves periodically (for dyn names). - Refresh Duration `yaml:"refresh"` + Refresh Duration `yaml:"refresh" json:"refresh"` } type Cache struct { @@ -304,42 +354,47 @@ type ACL struct { // Group is a named set of clients sharing a policy. Groups are evaluated // top-to-bottom; the first match wins. +// +// The json tags are the same as the yaml ones and they are not decoration: +// PUT /api/clients decodes the admin console's payload into this struct with +// DisallowUnknownFields, and the console sends snake_case. A struct with only +// yaml tags made every write answer 400 `json: unknown field "ptr_suffixes"`. type Group struct { - Name string `yaml:"name"` - Comment string `yaml:"comment,omitempty"` - Match Match `yaml:"match"` + Name string `yaml:"name" json:"name"` + Comment string `yaml:"comment,omitempty" json:"comment,omitempty"` + Match Match `yaml:"match" json:"match"` // Policy is "allow" (full resolution), "block_only" (blocklists still // apply, everything else resolves) or "deny" (REFUSED). - Policy string `yaml:"policy"` + Policy string `yaml:"policy" json:"policy"` // AllowDomains / DenyDomains are per-group overrides; the names support // the same wildcards as blocklists ("*.example.com", "**.example.com"). - AllowDomains []string `yaml:"allow_domains"` - DenyDomains []string `yaml:"deny_domains"` + AllowDomains []string `yaml:"allow_domains" json:"allow_domains"` + DenyDomains []string `yaml:"deny_domains" json:"deny_domains"` // BlocklistToggle lets a group opt out of blocklist filtering. - SkipBlocklist bool `yaml:"skip_blocklist"` + SkipBlocklist bool `yaml:"skip_blocklist" json:"skip_blocklist"` // CacheToggle lets a group bypass the cache (fresh lookups only). - SkipCache bool `yaml:"skip_cache"` + SkipCache bool `yaml:"skip_cache" json:"skip_cache"` // RateLimit overrides ACL.DefaultRateLimit for this group. - RateLimit int `yaml:"rate_limit"` + RateLimit int `yaml:"rate_limit" json:"rate_limit"` // MaxConcurrent overrides Server.MaxConcurrentPerClient. - MaxConcurrent int `yaml:"max_concurrent"` + MaxConcurrent int `yaml:"max_concurrent" json:"max_concurrent"` // LogQueries forces query logging for this group regardless of sampling. - LogQueries bool `yaml:"log_queries"` - Upstreams []string `yaml:"upstreams"` + LogQueries bool `yaml:"log_queries" json:"log_queries"` + Upstreams []string `yaml:"upstreams" json:"upstreams"` } // Match selects clients by network identity. type Match struct { // CIDRs are matched against the client address (v4 or v6). - CIDRs []string `yaml:"cidrs"` + CIDRs []string `yaml:"cidrs" json:"cidrs"` // Networks match DNS clients identified by a resolved PTR of their // reverse name; the name must end with one of the suffixes. - PTRSuffixes []string `yaml:"ptr_suffixes"` + PTRSuffixes []string `yaml:"ptr_suffixes" json:"ptr_suffixes"` // Tags are matched against the client tag set derived from PTR labels, // e.g. "laptop" for "laptop.lan.internal.". - Tags []string `yaml:"tags"` + Tags []string `yaml:"tags" json:"tags"` // Always matches (useful as a catch-all fallback group). - Any bool `yaml:"any"` + Any bool `yaml:"any" json:"any"` } type QueryLog struct { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d3f55f4..7492fbf 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -114,8 +114,18 @@ func TestDurationMarshalYAMLRoundTrip(t *testing.T) { } } -// Duration has no custom JSON unmarshaller; pin what encoding/json does with -// it so a change here is a deliberate one. +// TestDurationJSONForm pins the wire form of a Duration. This test used to +// pin the encoding/json default — a bare nanosecond count, and a quoted +// duration as an error — because a config.Duration had no custom JSON +// marshaller at all. +// +// DECIDED: that default was wrong on both sides. The admin console declares +// these fields as strings (`bootstraps[].refresh`, `healthcheck_interval`), the +// same spelling config.yaml uses, and a nanosecond count on the wire is +// something no client means. Duration now marshals to "5s" and unmarshals from +// a duration string; a bare number is accepted as SECONDS rather than +// nanoseconds, so a hand-written client asking for `{"healthcheck_interval":30}` +// gets 30s and not 30ns. func TestDurationJSONForm(t *testing.T) { var got struct { D Duration `json:"d"` @@ -123,18 +133,35 @@ func TestDurationJSONForm(t *testing.T) { if err := json.Unmarshal([]byte(`{"d":5}`), &got); err != nil { t.Fatalf("bare number: %v", err) } - if got.D.D() != 5*time.Nanosecond { - t.Errorf("bare JSON number = %v (nanoseconds, not seconds)", got.D.D()) + if got.D.D() != 5*time.Second { + t.Errorf("bare JSON number = %v, want 5s (a number is seconds, not nanoseconds)", got.D.D()) } - if err := json.Unmarshal([]byte(`{"d":"5s"}`), &got); err == nil { - t.Errorf("quoted JSON duration decoded to %v; string form is unsupported", got.D.D()) + if err := json.Unmarshal([]byte(`{"d":"5s"}`), &got); err != nil { + t.Fatalf("quoted duration: %v", err) + } + if got.D.D() != 5*time.Second { + t.Errorf("quoted JSON duration = %v, want 5s", got.D.D()) + } + if err := json.Unmarshal([]byte(`{"d":"nonsense"}`), &got); err == nil { + t.Error("an unparseable duration string decoded without an error") + } + if err := json.Unmarshal([]byte(`{"d":true}`), &got); err == nil { + t.Error("a boolean decoded as a duration without an error") } out, err := json.Marshal(Duration(5 * time.Second)) if err != nil { t.Fatalf("marshal: %v", err) } - if string(out) != "5000000000" { - t.Errorf("json.Marshal = %s, want nanoseconds", out) + if string(out) != `"5s"` { + t.Errorf("json.Marshal = %s, want \"5s\"", out) + } + // The yaml form is unchanged, so a config file and the API agree. + y, err := Duration(90 * time.Second).MarshalYAML() + if err != nil { + t.Fatalf("MarshalYAML: %v", err) + } + if y != "1m30s" { + t.Errorf("MarshalYAML = %v, want 1m30s", y) } } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index eb8a0d0..469d985 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -80,6 +80,13 @@ type Response struct { CacheKind string // Upstream is the address that answered. Upstream string + // Local reports that the answer came from the local zone rather than from + // the cache or an upstream. It is what makes metrics.OutcomeLocal + // reachable: an authoritative local answer is a real, distinct outcome + // and the dashboard has a column for it, but nothing set it, so every + // locally answered query was counted as an upstream one and the + // outcome="local" series was never published. + Local bool // Duration is the total handling time. Duration time.Duration // Client is the resolved client string. @@ -333,6 +340,11 @@ func (e *Engine) Handle(ctx context.Context, req Request) Response { if local, ok := e.answerLocal(q, store); ok { res.Msg = e.finalize(local, q, req) res.Action = qlog.ActionAllow + // Authoritative local data is ActionAllow for the query log (the + // query was permitted and answered), but it is its own metrics + // outcome: counting it as upstream inflated the upstream column + // with queries that never left the process. + res.Local = true e.finish(req, res, start, decision) return res } @@ -448,7 +460,7 @@ func (e *Engine) finish(req Request, res Response, start time.Time, d acl.Decisi if res.Duration == 0 { res.Duration = time.Since(start) } - metrics.Record(req.Transport, outcomeFor(res.Action), res.Duration) + metrics.Record(req.Transport, outcomeFor(res), res.Duration) if res.Action == qlog.ActionAllow && res.CacheHit { metrics.Record(req.Transport, metrics.OutcomeCache, res.Duration) } @@ -475,8 +487,15 @@ func (e *Engine) finish(req Request, res Response, start time.Time, d acl.Decisi e.logger.Log(entry) } -func outcomeFor(a qlog.Action) string { - switch a { +// outcomeFor maps a finished query onto its metrics outcome. The action alone +// is not enough: ActionAllow covers both an upstream answer and a local-zone +// answer, and the two are different things for an operator reading the +// dashboard, so Response.Local is consulted first. +func outcomeFor(res Response) string { + if res.Local { + return metrics.OutcomeLocal + } + switch res.Action { case qlog.ActionCached: return metrics.OutcomeCache case qlog.ActionBlock: diff --git a/internal/engine/pins_test.go b/internal/engine/pins_test.go index a2a022b..8ea3ee1 100644 --- a/internal/engine/pins_test.go +++ b/internal/engine/pins_test.go @@ -45,35 +45,41 @@ func outcomeCount(t *testing.T, outcome string) uint64 { return total } -// TestPinnedLocalAnswersAreCountedAsUpstream pins the gap STATUS.md calls +// TestPinnedLocalAnswersAreCountedAsUpstream pins the gap STATUS.md called // "metrics.OutcomeLocal has no producer": a query answered from the local // zone reports qlog.ActionAllow, and outcomeFor maps ActionAllow to -// OutcomeUpstream, so every locally answered query is counted as an upstream -// one. The outcome="local" series is therefore never published and the -// dashboard's local column is permanently empty. +// OutcomeUpstream, so every locally answered query was counted as an upstream +// one. The outcome="local" series was therefore never published and the +// dashboard's local column was permanently empty. // -// If this test fails, something started producing OutcomeLocal (or the -// mapping changed): that moves published counters, so it is a deliberate -// change — update the Grafana panels and the ServiceMonitor queries, and -// remove this pin. Do not "fix" it silently. -func TestPinnedLocalAnswersAreCountedAsUpstream(t *testing.T) { +// DECIDED (round with the console contract): the producer now exists. +// Response.Local is set on the local-zone path and outcomeFor consults it, so +// a local answer is counted as outcome="local" and is NOT counted as upstream. +// This moved published counters, which is why it was a decision and not a +// silent fix. TestLocallyAnsweredQueryIsCountedAsLocal replaces this pin; the +// dashboards and STATUS.md were updated in the same change. +func TestLocallyAnsweredQueryIsCountedAsLocal(t *testing.T) { h := newHarness(t, hopt{localZone: testZone}) - before := outcomeCount(t, metrics.OutcomeUpstream) + upBefore := outcomeCount(t, metrics.OutcomeUpstream) + localBefore := outcomeCount(t, metrics.OutcomeLocal) res := h.ask("host.local.", dns.TypeA) requireAction(t, res, qlog.ActionAllow) if got := answerA(res.Msg); got != "192.168.1.5" { t.Fatalf("answer = %q, want the local zone's 192.168.1.5", got) } requireNoUpstreamHit(t, h, "host.local.") - - if got := outcomeCount(t, metrics.OutcomeUpstream) - before; got != 1 { - t.Fatalf("a locally answered query moved the upstream outcome by %d, want 1", got) + if !res.Local { + t.Error("Response.Local is false for a locally answered query, so the " + + "outcome would be counted as upstream again") } - if got := outcomeCount(t, metrics.OutcomeLocal); got != 0 { - t.Fatalf("metrics.OutcomeLocal now has %d observations; it is no longer "+ - "dead. Publishing published counters is a decision: update the "+ - "dashboards and STATUS.md, then delete this pin.", got) + + if got := outcomeCount(t, metrics.OutcomeLocal) - localBefore; got != 1 { + t.Errorf("a locally answered query moved the local outcome by %d, want 1", got) + } + if got := outcomeCount(t, metrics.OutcomeUpstream) - upBefore; got != 0 { + t.Errorf("a locally answered query moved the upstream outcome by %d, want 0: "+ + "it never reached an upstream", got) } } diff --git a/internal/server/config_test.go b/internal/server/config_test.go index 708a182..c3503eb 100644 --- a/internal/server/config_test.go +++ b/internal/server/config_test.go @@ -90,11 +90,20 @@ func TestEnabledDoTWithoutCertIsAFailure(t *testing.T) { // serving on the survivors is the difference between one broken transport and // an outage. func TestStartReportsAPartialFailureButServes(t *testing.T) { - // Port 1 is privileged: an unprivileged test process cannot bind it, which - // is a deterministic bind failure without a port race. + // A port already held by a listener in this test process is the + // deterministic bind failure. "127.0.0.1:1" looks more convincing but is + // not: an unprivileged process cannot bind it *by default*, and plenty of + // container runtimes set net.ipv4.ip_unprivileged_port_start=0, where it + // succeeds and the test passes for the wrong reason — or fails, with the + // only signal being that both transports came up. + blocker, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("occupy a port: %v", err) + } + t.Cleanup(func() { _ = blocker.Close() }) cfg := config.Default() cfg.DNS.UDP = []string{"127.0.0.1:0"} - cfg.DNS.TCP = []string{"127.0.0.1:1"} + cfg.DNS.TCP = []string{blocker.Addr().String()} cfg.DNS.DoT = config.DoT{Enabled: false} cfg.DNS.DoH = config.DoH{Enabled: false} srv := New(Options{Engine: newFakeHandler(), Config: config.NewStore(cfg, ""), Logger: discardLogger()}) diff --git a/internal/server/counters_test.go b/internal/server/counters_test.go new file mode 100644 index 0000000..f2f0c5c --- /dev/null +++ b/internal/server/counters_test.go @@ -0,0 +1,209 @@ +package server + +import ( + "sync" + "testing" +) + +// TestBumpAndLoadKeySemantics is a table over the counter keyspace. The keys +// are the only thing that binds a counter to a transport, and two callers can +// disagree about the spelling of a transport: bump() deliberately normalises +// an empty transport to "udp" (the pre-transport default) while load() does +// not normalise at all, so a caller that asks for the wrong spelling silently +// reads 0 rather than failing. Pinning both directions here is what stops that +// from turning into a counter that is written but never shown. +func TestBumpAndLoadKeySemantics(t *testing.T) { + cases := []struct { + name string + transport string + field string + wantKey string + }{ + {name: "udp queries", transport: "udp", field: "queries", wantKey: "udp.queries"}, + {name: "tcp sent", transport: "tcp", field: "sent", wantKey: "tcp.sent"}, + {name: "dot truncated", transport: "dot", field: "truncated", wantKey: "dot.truncated"}, + {name: "doh dropped", transport: "doh", field: "dropped", wantKey: "doh.dropped"}, + // An empty transport is the historical spelling of "the default + // transport", and classify()/servePacket() predate the per-transport + // split, so an empty one must not create a ".queries" key that + // ByTransport can never find. + {name: "empty transport falls back to udp", transport: "", field: "queries", wantKey: "udp.queries"}, + {name: "unknown transport is kept verbatim", transport: "quic", field: "queries", wantKey: "quic.queries"}, + {name: "unknown field is kept verbatim", transport: "udp", field: "nonsense", wantKey: "udp.nonsense"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := new(Server) + s.bump(tc.transport, tc.field) + + // The key is the whole lookup path, so assert on what was + // actually stored rather than inferring it from a read. + if _, ok := s.counters.Load(tc.wantKey); !ok { + keys := make([]string, 0, 4) + s.counters.Range(func(k, _ any) bool { + keys = append(keys, k.(string)) + return true + }) + t.Fatalf("bump(%q, %q) stored %v, want key %q", tc.transport, tc.field, keys, tc.wantKey) + } + // The normalised key is what a reader of the default transport + // sees. load() does not normalise, which is deliberate: it is + // always called with a literal transport from the four-transport + // list, so a caller that invented a spelling reads 0 rather than + // silently getting another transport's numbers. + if got := s.load(tc.wantKey[:len(tc.wantKey)-len(tc.field)-1], tc.field); got != 1 { + t.Errorf("load(key transport, %q) = %d, want 1", tc.field, got) + } + // Counters accumulate, they do not reset. + s.bump(tc.transport, tc.field) + if got := s.load(tc.wantKey[:len(tc.wantKey)-len(tc.field)-1], tc.field); got != 2 { + t.Errorf("after two bumps load(%q) = %d, want 2", tc.field, got) + } + // A field nobody wrote reads 0 instead of aliasing a sibling. + if got := s.load(tc.wantKey[:len(tc.wantKey)-len(tc.field)-1], tc.field+"x"); got != 0 { + t.Errorf("load(%q) = %d, want 0", tc.field+"x", got) + } + }) + } +} + +// TestStatsAggregatesOnlyKnownFields is the other half of the key contract: a +// key whose suffix is not a HandlerStats field (sent, truncated, or garbage) +// must be ignored by the aggregate instead of being miscounted into it. +func TestStatsAggregatesOnlyKnownFields(t *testing.T) { + cases := []struct { + field string + weight func(*HandlerStats) uint64 + }{ + {field: "queries", weight: func(s *HandlerStats) uint64 { return s.Queries }}, + {field: "blocked", weight: func(s *HandlerStats) uint64 { return s.Blocked }}, + {field: "cached", weight: func(s *HandlerStats) uint64 { return s.Cached }}, + {field: "errors", weight: func(s *HandlerStats) uint64 { return s.Errors }}, + {field: "dropped", weight: func(s *HandlerStats) uint64 { return s.Dropped }}, + } + for _, tc := range cases { + t.Run(tc.field, func(t *testing.T) { + s := new(Server) + s.bump("tcp", tc.field) + s.bump("tcp", tc.field) + got := s.Stats() + if v := tc.weight(&got); v != 2 { + t.Errorf("Stats().%s = %d, want 2", tc.field, v) + } + // The per-transport view is the transport-scoped mirror of the + // same bumps and must agree. + ts := s.ByTransport()["tcp"] + switch tc.field { + case "queries": + if ts.Queries != 2 { + t.Errorf("ByTransport tcp.Queries = %d, want 2", ts.Queries) + } + case "blocked": + if ts.Blocked != 2 { + t.Errorf("ByTransport tcp.Blocked = %d, want 2", ts.Blocked) + } + case "cached": + if ts.Cached != 2 { + t.Errorf("ByTransport tcp.Cached = %d, want 2", ts.Cached) + } + case "errors": + if ts.Errors != 2 { + t.Errorf("ByTransport tcp.Errors = %d, want 2", ts.Errors) + } + case "dropped": + if ts.Dropped != 2 { + t.Errorf("ByTransport tcp.Dropped = %d, want 2", ts.Dropped) + } + } + }) + } + + // Fields the aggregate does not know about stay out of it but do show up + // per transport, which is how the console's transport tab works. + s := new(Server) + s.bump("udp", "sent") + s.bump("udp", "truncated") + s.bump("udp", "nonsense") + if got := s.Stats(); got.Queries+got.Blocked+got.Cached+got.Errors+got.Dropped != 0 { + t.Errorf("Stats() = %+v, want every aggregate field 0", got) + } + ts := s.ByTransport()["udp"] + if ts.Sent != 1 || ts.Truncated != 1 { + t.Errorf("ByTransport udp = %+v, want Sent 1 Truncated 1", ts) + } +} + +// TestStatsSnapshotIsSafeWhileCountersMove hammers bump() from many +// goroutines while Stats() and ByTransport() are read. Under -race this is the +// real assertion; the final check is an invariant (the aggregate equals the +// sum over transports) rather than a claim about what a reader happened to +// observe mid-flight, so it cannot flake. +func TestStatsSnapshotIsSafeWhileCountersMove(t *testing.T) { + const ( + writers = 8 + bumpsPerWriter = 200 + readers = 4 + ) + s := new(Server) + transports := []string{"udp", "tcp", "dot", "doh"} + + var start sync.WaitGroup + start.Add(1) + done := make(chan struct{}) + + // Writers and readers wait on separate groups: the readers stop only when + // `done` is closed, and `done` is closed once the writers are finished, so + // folding both into one WaitGroup would wait for readers that are waiting + // for the very Wait that waits for them. + var writersWG, readersWG sync.WaitGroup + for w := 0; w < writers; w++ { + writersWG.Add(1) + go func(w int) { + defer writersWG.Done() + transport := transports[w%len(transports)] + start.Wait() + for i := 0; i < bumpsPerWriter; i++ { + s.bump(transport, "queries") + s.bump(transport, "sent") + } + }(w) + } + for r := 0; r < readers; r++ { + readersWG.Add(1) + go func() { + defer readersWG.Done() + start.Wait() + for { + select { + case <-done: + return + default: + } + _ = s.Stats() + _ = s.ByTransport() + } + }() + } + start.Done() + writersWG.Wait() + close(done) + readersWG.Wait() + + total := s.Stats() + var sum uint64 + for _, tr := range transports { + ts := s.ByTransport()[tr] + sum += ts.Queries + if ts.Queries != ts.Sent { + t.Errorf("transport %q: Queries %d != Sent %d", tr, ts.Queries, ts.Sent) + } + } + if sum != total.Queries { + t.Errorf("sum of per-transport queries = %d, aggregate = %d", sum, total.Queries) + } + // One "queries" bump and one "sent" bump per iteration, so the aggregate + // is writers*bumpsPerWriter queries and the same number of sends. + if want := uint64(writers * bumpsPerWriter); total.Queries != want { + t.Errorf("aggregate queries = %d, want %d", total.Queries, want) + } +}