From 66f36cfac6992ef1940535ca274fdb70b058cc94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mats=20T=C3=B6rnberg?= Date: Fri, 16 Oct 2026 14:49:50 +0200 Subject: [PATCH] more stuff --- README.md | 55 ++- STATUS.md | 747 +++++------------------------- deploy/OUTSIDE-CLUSTER.md | 292 ++++++++++++ deploy/compose/README.md | 5 + deploy/compose/config.yaml | 9 +- internal/acl/acl.go | 6 + internal/acl/acl_test.go | 6 + internal/acl/explain.go | 414 +++++++++++++++++ internal/acl/explain_test.go | 172 +++++++ internal/api/zones.go | 5 +- internal/console/console.go | 1 + internal/console/hooks.go | 3 +- internal/console/validate.go | 193 ++++++++ internal/console/validate_test.go | 181 ++++++++ internal/kubeedit/groups.go | 9 + internal/kubesource/convert.go | 64 ++- web/src/lib/api.ts | 12 + web/src/lib/types.ts | 35 ++ web/src/pages/Clients.tsx | 513 ++++++++++++++------ 19 files changed, 1905 insertions(+), 817 deletions(-) create mode 100644 deploy/OUTSIDE-CLUSTER.md create mode 100644 internal/acl/explain.go create mode 100644 internal/acl/explain_test.go create mode 100644 internal/console/validate.go create mode 100644 internal/console/validate_test.go diff --git a/README.md b/README.md index b921ffb..b8322cd 100644 --- a/README.md +++ b/README.md @@ -275,15 +275,15 @@ file. | `acl.groups` | list | empty | evaluated top to bottom, first match wins | | `acl.groups[].name` / `comment` | string | — | `name` required and unique | | `acl.groups[].match.cidrs` | list of CIDR | empty | matched against the client address (v4 or v6) | -| `acl.groups[].match.ptr_suffixes` | list | empty | matched against reverse-DNS names of the client, on a label boundary; a missing trailing dot is added | +| `acl.groups[].match.ptr_suffixes` | list | empty | matched against reverse-DNS names of the client, on a label boundary; a missing trailing dot is added. `lan.example.` covers the name and everything under it; `.lan.example.` (leading dot) covers only what is under it | | `acl.groups[].match.tags` | list | empty | labels derived from the reverse name (e.g. `laptop` for `laptop.lan.internal.`) | | `acl.groups[].match.any` | bool | `false` | catch-all; must be the last group | -| `acl.groups[].policy` | `allow` \| `deny` \| `block_only` | `allow` | `deny` answers REFUSED and wins over the local zone | -| `acl.groups[].allow_domains` / `deny_domains` | list | empty | per-group name policy with the blocklist wildcards; `deny_domains` is checked first | -| `acl.groups[].skip_blocklist` | bool | `false` | the group opts out of blocklist filtering | +| `acl.groups[].policy` | `allow` \| `deny` \| `block_only` | `allow` | `deny` answers REFUSED and wins over the local zone. **`block_only` behaves exactly like `allow` today**: both resolve normally and both apply the blocklists | +| `acl.groups[].allow_domains` / `deny_domains` | list | empty | per-group name policy with the blocklist wildcards (`*.x` and `**.x` both cover `x` itself here). **A non-empty `allow_domains` is allowlist-only mode**: every name not in it is blocked for the group; it does not exempt names from the blocklists (that is `skip_blocklist`). `deny_domains` is checked first and wins | +| `acl.groups[].skip_blocklist` | bool | `false` | the group opts out of blocklist filtering (local zones and address overrides are unaffected) | | `acl.groups[].skip_cache` | bool | `false` | the group only gets fresh lookups | | `acl.groups[].rate_limit` | int | inherit | queries/second for this group | -| `acl.groups[].max_concurrent` | int | `0` (off) | in-flight cap for this client | +| `acl.groups[].max_concurrent` | int | `0` (off) | in-flight cap for this client; the excess get SERVFAIL | | `acl.groups[].log_queries` | bool | `false` | log every query from this group regardless of `access_log` selection | | `acl.groups[].upstreams` | list of addrs | empty | restrict this group to named upstreams; a list that matches nothing falls back to the whole pool | @@ -521,6 +521,7 @@ is on. | POST | `/api/domains` | yes | add an inline rule | | POST | `/api/dns/test` | — | run one query through the live pipeline (`{"name","type"}`) and report the rcode, action, answers, upstream and latency — the console's "is my DNS answering" test, and a failed lookup is a 200 with a result, not a failed request | | DELETE | `/api/domains/{name…}?action=&list=` | yes | remove a rule | +| POST | `/api/clients/validate` | — | console only. Check a client group without saving it: `{"group":{…},"test_ip":"…"}` answers `errors` (what blocks a save, with the field), `warnings` (legal but probably unintended: a group buried under a wider one, a rate limit of 0 under a default limit, an allowlist), `notes`, a plain-English `summary`, the group's `position` in the evaluation order, and which group `test_ip` would land in | | GET | `/api/zones` | — | every local zone with its records, delegations, source files, whether each is an editable cluster object, the record types the console writes, and why the store is stale (`error`) | | POST | `/api/zones/records` | yes | add a record (`{"zone","name","type","ttl","value"}`; a name without a trailing dot is relative to the zone) | | PUT | `/api/zones/records` | yes | replace the whole RRset of one name and type | @@ -580,7 +581,7 @@ docker run --rm -p 53:53/udp -p 53:53/tcp -p 8080:8080 \ The `Dockerfile` builds the console in a Node stage, then a static `CGO_ENABLED=0` binary with it embedded, and ships it on `gcr.io/distroless/static-debian12:nonroot`. It declares `VOLUME -/var/lib/fancy-dns` (cache snapshot, query log, downloaded lists) and copies +/var/lib/fancy-dns` (cache snapshot, downloaded lists, the saved cluster snapshot) and copies `deploy/config.example.yaml` plus `deploy/zones/` to `/etc/fancy-dns/`; there is no `config.yaml` in the image, so its `CMD` (`-config /etc/fancy-dns/config.yaml`) requires you to mount one. @@ -626,7 +627,7 @@ Things to know before you apply it: * **Per-pod PVC sizing.** The StatefulSet uses a 4 Gi `ReadWriteOnce` template. `cache.size` is an *entry count*, not bytes: a packed answer is roughly 300–600 bytes on the wire and nearer 1 kB resident, so 500k entries - lands around 500 MB, and `qlog.max_entries` is the other consumer. The PVC + lands around 500 MB, and the downloaded blocklists are the other consumer. The PVC cannot be shared between replicas, and being a StatefulSet also means an HPA cannot drive the replica count. * Probes all hit `/api/healthz` on the admin port, which is unauthenticated on @@ -771,72 +772,70 @@ Found while writing this file; each is a to-do, not something fixed here. 6. **`internal/config/config.go:229` — `cache.prewarm` is dead.** `App.prewarm` exists (`internal/app/app.go:220`) but nothing calls it, while `deploy/config.example.yaml:97` ships `prewarm: []`. -7. **`internal/config/config.go:358` — `qlog.log_client_names` is dead.** -8. **`internal/config/config.go:373` — `cluster.peer_sync` is dead**, and +7. **`internal/config/config.go:373` — `cluster.peer_sync` is dead**, and **`cluster.advertise`** (`:371`, "the address other instances dial for the admin API") is only echoed in heartbeats: no code dials a peer. -9. **`internal/config/config.go:411` — `api.allow_blocklist_write` is dead.** +8. **`internal/config/config.go:411` — `api.allow_blocklist_write` is dead.** Only `allow_config_write` is enforced (`internal/api/handlers.go: handleSettingsPut`). -10. **`internal/config/config.go:418` — `metrics.path` is dead.** The route is +9. **`internal/config/config.go:418` — `metrics.path` is dead.** The route is built from `api.base_path` (`internal/api/api.go:routes`: `GET metrics`), so `metrics.path: /metrics` in `deploy/config.example.yaml:196` and in `deploy/k8s/00-config.yaml` has no effect; the endpoint is `/api/metrics`. -11. **`internal/config/config.go` — `dns.doh.transport.alt_svc` (`:157`) and +10. **`internal/config/config.go` — `dns.doh.transport.alt_svc` (`:157`) and `.mode` (`:156`), `dns.dot.upstream` (`:129`), `dns.edns0.cookie_support` (`:166`), `dns.edns0.client_subnet` (`:162`), `dns.edns0.ipv4_bits` / `ipv6_bits` (`:163-164`) are all dead.** `serveDoH` always speaks h2 with an h2c fallback (no HTTP/3, no Alt-Svc), there is no encrypted-upstream DoT switch (`upstream.servers[].proto: tls` is the way to get that), and ECS is forwarded verbatim whenever `dns.edns0.preserve` is on. -12. **`internal/config/config.go:266` — `blocklist.parallelism` is dead**; +11. **`internal/config/config.go:266` — `blocklist.parallelism` is dead**; `RefreshAll` compiles single-threaded. -13. **`internal/config/config.go` (`ListSource.Timeout`) and +12. **`internal/config/config.go` (`ListSource.Timeout`) and `server.timeouts.list_fetch`** — the fetch timeout is never applied: `blocklist.NewManager` hardcodes a 120 s `http.Client` and `internal/blocklist/manager.go:fetch` never consults `l.Timeout`. -14. **`internal/config/config.go` (`UpstreamBootstrap.Refresh`)** — bootstrap +13. **`internal/config/config.go` (`UpstreamBootstrap.Refresh`)** — bootstrap names are resolved once per pool rebuild; `refresh` never triggers a re-resolution. -15. **`internal/config/config.go` (`Valkey.TLS`)** — `cluster.valkey.tls` is +14. **`internal/config/config.go` (`Valkey.TLS`)** — `cluster.valkey.tls` is never applied; `internal/cluster/cluster.go:dial` builds a plain `redis.Client` with no `TLSConfig`. -17. **`internal/zonefile/zonefile.go:1-8` and `:238-243` — the package comment +16. **`internal/zonefile/zonefile.go:1-8` and `:238-243` — the package comment advertises `$INCLUDE`, the parser rejects it** ("`$INCLUDE … is not supported; use blocklist.local_zones or a list source instead`"), and `Options.MaxIncludeDepth` is never set by any caller. Either implement it or correct the comment and drop the field. -18. **`Makefile:113` — `make run` claims it runs +17. **`Makefile:113` — `make run` claims it runs `deploy/config.example.yaml` "on unprivileged ports", but that file binds `0.0.0.0:53` and `[::]:53`** (`deploy/config.example.yaml:23-24`). Unprivileged use needs the same `sed` rewrite the CI smoke test does (`.github/workflows/ci.yml`, "Smoke test the real binary"), or a port you can bind. -20. **`deploy/k8s/20-service.yaml` — the load-balancer comment says to leave +19. **`deploy/k8s/20-service.yaml` — the load-balancer comment says to leave the LB health check on the HTTP admin port, but the `fancy-dns-dns` LoadBalancer Service publishes only 53/UDP, 53/TCP and 853/TCP.** The admin port is on the separate `fancy-dns-web` ClusterIP Service, which the LB cannot health-check. Pick a provider with UDP health checks or use DNS-level failover. -21. **`deploy/k8s/00-config.yaml` enables `dot` and `doh` against +20. **`deploy/k8s/00-config.yaml` enables `dot` and `doh` against `/etc/fancy-dns/tls/tls.crt` while the Secret it mounts ships empty `tls.crt` / `tls.key`.** Applying the kustomization unchanged leaves both listeners unable to build a `tls.Config`; a comment says so, but an example that cannot come up as-is is still a footgun. -22. **`api.rate_limit.per_minute` defaults to `60` in `config.Default()` but +21. **`api.rate_limit.per_minute` defaults to `60` in `config.Default()` but `deploy/config.example.yaml:189` says `600`**, and - `deploy/config.example.yaml` sets `qlog.log_cached: false` where the default - is `true`. Harmless, but the example is not a dump of the defaults and - readers will assume it is. -23. **`api.auth.mode: apikey` with an emptied `api_keys` list and + `deploy/config.example.yaml` is not a dump of the defaults, and readers will + assume it is. +22. **`api.auth.mode: apikey` with an emptied `api_keys` list and `trust_loopback: true` is indistinguishable from `none`.** `Validate` only rejects the combination with `trust_loopback: false`; the unsafe case is reported solely as a warning from `POST /api/settings/validate`. -24. **`config.Load` does not use `yaml.Decoder.KnownFields`**, so unknown keys +23. **`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. **RESOLVED — an RPZ record did not cover the name it names.** +24. **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 @@ -849,7 +848,7 @@ Found while writing this file; each is a to-do, not something fixed here. `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 +25. **`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 are involved, so echoing is the only way a wildcard can work with a session diff --git a/STATUS.md b/STATUS.md index e1bead2..7d9066a 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,657 +1,138 @@ -# fancy-dns — development status and continuation plan +# fancy-dns — status and continuation plan -## Console authentication update +`AGENTS.md` holds the decisions, invariants and working rules. This file holds +state and the plan. History is in `git log`; the old per-round fix lists were +removed because each fix is explained in its commit and pinned by a test. -OIDC login now starts automatically, with server-owned state and PKCE. Go -exchanges the authorization code, verifies the access token, and stores it in -an in-memory session addressed by a Secure, HttpOnly, SameSite=Lax cookie. -Cookie mutations require an exact configured console Origin. Sessions reverify -the access token and cannot outlive its signed expiry. Logout revokes the local -session and offers an explicit sign-in button rather than immediately logging -back in. API keys and bearer tokens remain available for machine clients. -`api.auth.oidc_redirect_uri` names the registered HTTPS callback; when omitted, -an HTTPS audience supplies the origin with `/auth/oidc/callback`. The current -AuthClient already registers that callback, so provider CORS changes are no -longer needed. Pending logins and sessions are process-local; a restart requires -sign-in, and multiple replicas need sticky routing (as existing basic sessions do). -No refresh token is requested; expiry starts another provider sign-in using SSO. +Last updated after the round that added client-group validation. The cluster-object +architecture, the separate console and the web UI are deployed. -Console cleanup removes the unused bootstrap refresh editor, hides Save when -configuration writes are unavailable, removes script instructions from login, -and fixes the shell's identity/read-only display by moving it inside its provider. -Validation: `make check` passes, including race tests; frontend lint and production -build pass with warnings. These changes have not been deployed. +## What exists -Last updated after the round that finished the **server shutdown paths**: -every transport's ordering with a query in flight is now a test, and it found -two real bugs — an idle client could hold a shutdown for the whole grace -period, and a UDP query in flight was neither waited for nor answered. The -round before that brought the console onto the Go side of the API contract, so -both halves of that are done. `AGENTS.md` holds the decisions, invariants and -working rules; this file holds state and the plan. +Two binaries, one repository: -## Timeout enforcement update - -Foreground queries now enforce `server.timeouts.query` at `engine.Handle`, -including all failover attempts. Reloads update the limit atomically; earlier -caller deadlines and cancellation are preserved. `server.timeouts.upstream` -bounds each complete upstream attempt, with UDP and its TCP truncation retry -sharing one budget. Per-attempt client copies avoid mutating memoised clients, -and cancellation closes the private connection to interrupt blocked reads. -Caller cancellation (including parallel-race losers) does not degrade upstream -health. An exhausted fallback budget returns an error; an immediate TCP failure -still preserves the truncated UDP answer for the client to retry. - -Regression tests cover silent UDP/TCP/TLS upstreams, active-read cancellation, -fallback budgeting, failover, configured overall deadlines and hot reload. -The observed transient `.local` search-expansion incident remains unexplained: -these timeout defects bound stalls but do not establish why upstream replies -were absent at the time. The orchestrator's absolute RabbitMQ hostname remains -the independent search-expansion mitigation. - -## Conditional forwarding update - -`upstream.forward_zones` now routes an apex and its descendants to dedicated -upstreams, using the most specific case-insensitive label suffix. Routes reuse -health/breaker tracking and UDP-to-TCP retry, never fall back to public servers, -and apply to background cache refreshes too. Authoritative local zones, ACLs, -and blocklists retain their precedence. Upstream reloads flush the cache; console -upstream edits preserve the YAML-only routes and do not promote their targets to -default servers. The Kubernetes config routes `local` to `10.10.0.1:53` over UDP. -Direct UDP and TCP probes for `fastpi.local` both returned `10.10.11.117`. -This repository change still needs a rebuilt image and rollout to become live. - -## Goal - -A lightweight, high-performance recursive DNS server with: - -- **Go backend** — sharded LRU cache backed by bbolt, blocklist replication from - multiple formats, per-client ACLs, query logging, Prometheus metrics. -- **Secure DNS** — DNS-over-TLS (RFC 7858) and DNS-over-HTTPS (RFC 8484). - DNSSEC validation was explicitly scoped **out**; AD/CD/extended-RCODE are - passed through only. -- **React + Tailwind admin console** embedded in the binary. -- **Config as zone files + YAML**, hot-reloadable, with **Valkey/Redis** for - cross-instance sync (chosen over AMQP). - -## Architecture decisions (settled with the user) - -| Decision | Choice | Why | +| Binary | Does | Talks to | |---|---|---| -| Scope | Full production | clustering, metrics, ACLs, query logs, hot reload, Docker, k8s, CI | -| Config store | YAML + DNS master files | reviewable/diffable in git | -| Cluster sync | Valkey (Redis protocol) | simple, mature Go client, easy local dev | -| Secure DNS | DoT + DoH, no DNSSEC validation | avoids signer/anchor machinery; pass-through only | -| Hot cache | Sharded LRU in memory, bbolt for snapshots | no I/O on the hot path; warm restart | -| k8s networking | No hostNetwork, LoadBalancer Service | the server never sets SO_REUSEPORT, so two host-network pods cannot share :53 | -| Memory | Soft GOMEMLIMIT, cache byte budget | a cgroup kill is an outage; a cache miss is not | -| Scale-out | StatefulSet, no HPA | an HPA cannot drive a StatefulSet | -| Unknown YAML keys | Ignore, loudly documented | `KnownFields(true)` was considered and rejected as a behaviour change for anyone carrying extra keys. Pinned by `TestLoadIgnoresUnknownKeys`. | +| `fancy-dns` | The resolver: UDP/TCP/DoT/DoH, cache, blocklists, ACL groups, local zones, access log, metrics, a read-only admin API. Reads `DNSZone`, `ClientGroup`, `BlockList`, `AddressOverride` and `DomainRule` objects with a watch (`-kube`) and keeps a snapshot so it can start without the cluster. | Upstreams; optionally the Kubernetes API and Valkey. | +| `fancy-dns-console` | The web UI and the edit API. The only writer of the cluster objects; authenticates (API key, basic, OIDC with PKCE); proxies every other `/api` route to the resolver. Not on the DNS path. | The resolver's admin API and the Kubernetes API. | -## Package layout +Operational docs: `README.md` (reference), `deploy/MIGRATION.md` (ConfigMap +deployment to cluster objects), `deploy/OUTSIDE-CLUSTER.md` (a backup resolver +and console on a node outside the cluster), `deploy/compose/README.md` +(file-configured Docker backup). -``` -cmd/fancy-dns/main.go entrypoint: flags, signals, SIGHUP reload -internal/ - config/ Config struct, Duration, Size, Validate, atomic Store + hooks, - ${VAR} environment expansion in Load - zonefile/ RFC 1035 master-file parser ($TTL/$ORIGIN, parens, serial math) - blocklist/ Set (exact/sub/root maps), 4 parsers, Manager (fetch+compile+swap) - cache/ Key, Entry, sharded LRU, TTL policy, serve-stale, bbolt Store - resolver/ Upstream pool: health, circuit breakers, parallel/failover/random - acl/ compiled groups, CIDR/PTR/tag matching, per-group domain policy - localzone/ authoritative data from master files, wildcards, delegations - engine/ THE QUERY PIPELINE (see below), reverse-PTR cache - server/ UDP, TCP, DoT, DoH listeners; all funnel into one handler - qlog/ bbolt query-log ring + live in-memory ring + SSE fanout - metrics/ Prometheus collectors + rolling time-series rings - cluster/ Valkey pub/sub, leader lease, divergence detection - api/ admin REST + SSE, auth, CORS, rate limiting - app/ wiring, lifecycle, API hook implementations, memory envelope, - and the response contract the console is written against - web/ go:embed of the built console -web/ Vite + React + TS + Tailwind console -deploy/ example config, example zones, k8s manifests -``` +## Verified -### Query pipeline order (engine.Handle) +`make check` (gofmt, vet, compile, `go test -race ./...`) passes on every +package. The web console typechecks and `oxlint` reports no errors; the +remaining warnings are `set-state-in-effect` in `AppShell`, `Login`, `Domains` +and `Zones` and an unused parameter in `ui/Select.tsx`. The console has no test +runner, so a UI change is verified by `tsc` and by looking at it. -``` -validate → ACL deny → per-client concurrency → global concurrency → rate limit - → group allow/deny domains → local zone → blocklist → cache → upstream - → cache put → finalize (EDNS0, UDP truncation) → metrics + qlog -``` +## This round -Cache is a single shard-local lookup; only a miss touches the network. +- **Client-group validation.** `POST /api/clients/validate` (console only) + checks a group without saving it. It applies the same checks as a save, now + reported per field and all at once (`kubesource.CheckGroupAll`, + `kubesource.FieldError`), then runs `acl.Analyze`, which explains the group in + plain English and warns about legal-but-unintended settings: a group buried + under a wider one (and the reverse: a save that buries a later group), CIDRs + with host bits or a huge range, an allowlist, `block_only`, a rate limit of 0 + under a default limit, unknown upstreams, domain patterns that never match, + tags with a dot. `test_ip` says which group an address would land in. + Tests: `internal/acl/explain_test.go`, `internal/console/validate_test.go`. +- **Clients page.** Rebuilt around that endpoint: templates for the common + cases, sections ("Which devices?", "What happens to their queries?", + advanced), live validation with errors next to the field, a "What this group + will do" panel, and a try-an-address box. The "Match any client" switch is + gone: the console cannot store a catch-all group (`match.any` alone is + refused for cluster objects), so it could only produce an error. +- **The old help text was wrong, in ways that mattered.** It said + `allow_domains` "skip the blocklists"; in fact a non-empty list is + allowlist-only mode (everything else is blocked). It said `block_only` means + "no recursion"; in fact it is identical to `allow`. It suggested `.lan` as a + reverse-DNS suffix, which never matched anything (see the next item). It said + tags match "ECS/DNS tag metadata"; they are labels of the client's reverse + name. README's `acl` table is corrected to match. +- **`matchPTR` rejected a suffix with a leading dot.** `.lan.` was compared at a + label boundary that the leading dot had already consumed, so it matched + nothing. A leading dot now means "anything under". Pinned in + `TestMatchPTRLabelBoundary`. This changes which clients a group with such a + suffix matches (from nobody to the intended ones). +- **Docs.** This file, `deploy/OUTSIDE-CLUSTER.md`, and removal of the dead + `qlog` and `managed_zones` references from README, the Compose config, and a + comment in `internal/api/zones.go`. -## Status +## Decisions waiting for you -### Green, end to end +1. ~~`block_only`.~~ **Decided: removed from the UI.** It did exactly what `allow` + does. The API and CRD still accept it so stored objects keep working; the + console shows it only on a group that already has it, and the validator warns. +2. ~~Catch-all group.~~ **Decided: no change.** "Everyone else" is the default policy in + the resolver's YAML (`acl.default_policy`). The console does not set it and + cluster objects are never catch-alls. +3. The pinned behaviours below, one at a time. -`gofmt` clean, `go build ./...` clean, `go vet ./...` clean, and -`go test -race -count=1 -timeout 300s ./...` green across every package. +## Pinned, not fixed -| Package | top-level tests | notes | -|---|---|---| -| `internal/acl` | 26 | group compilation, matching, first-group-wins | -| `internal/api` | 55 (156 cases) | the whole route table, three auth modes, CORS, rate limiting, SSE, settings, error branches, pinned limitations | -| `internal/app` | 30 (incl. 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` | 20 | Valkey lease, pub/sub, divergence, dead-peer reaping | -| `internal/config` | 22 | Duration/Size, `${VAR}`, unknown keys, the JSON wire form, the shipped example configs | -| `internal/engine` | 26 | the pipeline end to end, plus the pins | -| `internal/localzone` | 18 | wildcards, delegation, CNAME following | -| `internal/metrics` | 10 | the metric names, which are the contract with the dashboards | -| `internal/qlog` | 18 | ring, bbolt, top domains, sampling | -| `internal/resolver` | 10 | pool, breakers, health, policies | -| `internal/server` | 57 | four listeners, UDP deadline reset, truncation, DoH limits, identity, counters, **shutdown and leak** | -| `internal/zonefile` | 29 | the master-file parser and the shipped example zone | -| `web` | 0 | typecheck and `oxlint` clean (0 errors), no test runner | -| `cmd/fancy-dns` | 0 | exercised by `make smoke` and the CI smoke job instead | +Each has a test that fails when it changes and says what to do then. -### The commands that matter +- **Unknown config keys are silently ignored** (`TestLoadIgnoresUnknownKeys`). +- **Auth config is frozen at startup.** `api.base_path`, `trusted_proxies` and + the authenticator are read once; a reload re-reads CORS only + (`TestPinnedWrittenAuthConfigIsNotAppliedUntilRestart`, + `TestCORSOriginsAreReadAtHandlerBuild`). +- **DoH clients behind an Ingress all look like the proxy**, because DoH + deliberately ignores `X-Forwarded-For` + (`TestDoHClientIdentityIsTheProxyNotTheForwardedForHeader`). +- **`api.cors_origins: ["*"]` echoes the request's Origin with credentials** + (`TestCORSWildcardEchoesTheOrigin`, `TestCORSPreflight`). +- **A partial bind failure is invisible** to the admin API + (`TestStartReportsAPartialFailureButServes`). +- **A question-less message is answered on UDP and drops the connection on + TCP/DoT** (`TestUDPQuestionlessMessageReachesTheEngine`). +- **`doh.sent` is never bumped** (`TestCountersFollowTheEngineOutcome`). -- `make check-go` — the inner loop (~1.3 s): gofmt check, vet, compile. Uses - `go-build`, not `build`, because `build` depends on `web` and shells out to npm. -- `make check` — `check-go` + the suite. What CI runs. -- `make smoke` — **new.** Boots the real binary with the console embedded on - unprivileged ports (the same `sed` rewrite CI uses), then checks `-check`, - `/api/healthz`, the embedded console, the unauthenticated 401, an - authenticated `whoami` and `/api/metrics`. Catches a broken embed, a bad - default and an auth regression in one command. -- `make web-check` — console typecheck and production build. -- `make bench` — cache and blocklist benchmarks. +The README's "Known inconsistencies" section lists more dead config keys and +stale line references; its line numbers have drifted and have not been +re-audited. -### The contract gap is closed on both sides +## Plan, in order -The API **response contract** used to be the interesting gap. The console's -types are hand-written and `apiFetch` ends in `JSON.parse(text) as T`, so -nothing in the toolchain compared them with the Go structs; a field that lost -its `json` tag, or a response a struct never had, showed up as an empty table in -a browser and nowhere else. `internal/app` now has contract tests that assert -the exact JSON key set of every admin endpoint (see the fix list), and the -console has been brought onto them — so the two sides agree *today*, but only -because someone compared them once. The durable fix is still open: see plan -item 2. +1. Resolve the decisions above. +2. **Stop hand-writing the console's types.** `web/src/lib/types.ts` is + hand-written and `apiFetch` ends in `JSON.parse(text) as T`; the Go contract + tests make the server authoritative today, but only because someone compared + them once. Generate the types from the Go structs (or emit an OpenAPI + document). `internal/app/dump_shapes_test.go` (`DUMP_SHAPES=1`) is the + stopgap. +3. **Test the console package beyond validation.** `internal/console` has one + test file (the validator). The cluster-consistent merge in `handleClients` + and `putACL` is the part that has caused lost updates before. +4. **Decide whether the shutdown grace belongs in config.** `main` hard-codes + 15s for `Stop`, of which 250ms is the UDP drain, and the Kubernetes + `terminationGracePeriodSeconds` is a second copy of the same number. +5. **Look at the Zones and Clients pages in a browser.** Neither the Zones page + nor this round's Clients page has been seen rendered by the person who wrote + it; both are typechecked and built only. +6. Re-audit README "Known inconsistencies" against the code. -### The server's shutdown is now pinned behaviour, not folklore +## Commands -It was the last part of `internal/server` tested by assumption. Four -transports, three shutdown shapes (a datagram socket, a stream accept loop with -configurable idle timeouts, and an `http.Server` that drains politely), and -every one of them can strand a goroutine a client is holding open. The -invariants that came out of it are in AGENTS.md; the two bugs it found are in -the fix list below. - -### 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 - -- **The console could not see local zones at all, so it could not manage - overrides or delegations.** `blocklist.local_zones` compiled a store that - nothing could read back: there was no API for a zone, its records, its - delegations or the file each record came from, and no way to change one - without editing YAML and reloading. The Domains page is the blocklist's - inline rules, not local data, so nothing in the console covered this half of - what the server does. There is now a **Zones** page and the six endpoints - behind it (`GET /api/zones`, `POST`/`PUT`/`DELETE /api/zones/records`, - `POST`/`DELETE /api/zones/delegations`, `DELETE /api/zones?zone=`). - - The write side is the interesting part, because the only place a zone can be - changed without a human editing YAML is a file on disk: - - - **The console owns `blocklist.managed_zones` and nothing else.** One - `.zone` per zone, holding the SOA, the apex NS set and the records - added there. It is compiled by the same `localzone.Load` as the operator's - files, so a record added from the console is served on the next query with - no reload. Files in `local_zones` are listed and shown read-only, and the - config key defaults to empty so a deployment that never sets it cannot have - zone data rewritten behind its back. `TestZoneWritesNeedAManagedDirectory` - pins that half. - - **The writer proves its own output.** A record is validated by the - project's own master-file parser (so `www` means `www.` and a bare - CNAME target is completed, the same way it is in a zone file), the file is - rendered, and that rendering is re-parsed and compared with the records it - meant to write *before* the file is renamed into place. A round-trip that - drops a record is a writer bug found there rather than by an operator - noticing a zone that stopped answering. - - **A zone that cannot answer is refused, not created.** No SOA means no - NXDOMAIN or NODATA authority, and no apex NS means nothing to delegate to - and no server to name, so a new zone has to start with one of the two. The - SOA's primary follows the apex NS set, the serial moves on every write, and - a record at or below a delegation is refused — the store answers those names - as delegated and never reads the file, so it would be data that looks alive - and never answers. - - **The engine's gate was the config list, not the store.** - `engine.applyConfig` only published local-record metrics when - `blocklist.local_zones` was non-empty, so a zone the console created was - served but uncounted. The gate is now the store's own record count. - - `Zone`/`ZoneRecord`/`ZoneDelegation` are type *aliases* in `internal/api` - rather than copies: one set of json tags, pinned by - `TestZoneEndpointKeySets` at every level, with the record list filled from a - real write so the assertion is about a record and not about an envelope. - - Verified end to end against a running instance: a record added through the - API answers authoritatively (`aa`) on the next query, a cut stops the parent - answering below it, and deleting the zone hands the name back. The console - page itself was not looked at in a browser in this round — the browser was - unavailable — so it is typechecked and built, not visually verified. - -- **A client that connected and said nothing held the whole shutdown open.** - The stream accept loop waits for its connections before `Stop` returns, and a - connection parked in `ReadMsg` with no deadline in front of it only ends when - the client goes away — `ctx.Err()` is checked *between* reads, so cancelling - the context cannot reach a read that is already blocked. One idle TCP - connection (or one DoT connection, with `timeouts.tls` set high) was enough to - make `Stop` sit there for the caller's entire grace period — 15s in `main` — - and then return `context.DeadlineExceeded`, which the app reports as - "shutdown completed with errors". Worse, the cache snapshot the app takes - right after `Stop` never happened: the orchestrator had already given up. It - is a shutdown DoS that costs the attacker one connection, and nothing tested - it. The fix is a read deadline in the past on every accepted connection when - the context is cancelled, and deliberately *not* a close: a query already - inside the pipeline is past its read, so its answer still goes out, which is - the invariant `TestInFlightQueryIsAnsweredAcrossStop` pins. - `TestStopIsNotBlockedByAnIdleStreamConnection` sets a 60s DoT idle timeout on - purpose, so a config value cannot hide the regression behind a lucky tick. -- **A UDP query in flight at `Stop` was lost, and nothing waited for it.** The - stream paths register their connections with `s.wait`, so `Stop` waits for - them; `servePacket` did not register at all, so a UDP query already inside - the pipeline when the socket closed was gone twice over — nothing waited for - it, and the answer had nowhere to go even once it was ready, because a - datagram is written to the very socket the query arrived on. The visible - damage was not the lost answer (the client retries) but the app: it snapshots - the cache and closes the store the moment `Stop` returns, so a query still in - the pipeline could be writing to a store that was about to close. Packets are - now counted in `s.wait`, and the UDP listener's close waits (bounded by - `udpDrainGrace`, 250ms) for the read loop and the packets it spawned before it - closes the socket. A shutdown must not spend its grace period on one slow - upstream, hence the bound: past it the answer is dropped, which is what the - client would see anyway. Pinned on all four transports by - `TestInFlightQueryIsAnsweredAcrossStop/{tcp,dot,udp,doh}`. -- **`Stop` returning did not mean the process was done.** Nothing asserted that - the server's goroutines were gone, so a shutdown that leaked one was - indistinguishable from one that did not. `TestStopLeavesNoGoroutineRunningServerCode` - counts goroutines whose stack still has a frame in this package — not - `runtime.NumGoroutine()`, which in a test binary is net/http, crypto/tls and - the race detector talking over each other — and it found the idle-connection - bug above, since the check is the only way to see a goroutine that has no - client left to answer. - -- **A dead peer stayed in the cluster peer list forever.** `Cluster.seen` was - only ever added to and deleted from on an explicit `bye`, so a node that - crashed — or whose Valkey connection died — was reported as a live peer for - the life of the process, and its stale blocklist digest was flagged as - divergence forever. `Peers()` now reaps peers unheard for `peer_ttl * 4` - (four missed heartbeats) and stamps each state with the receiver's own clock, - so clock skew between two machines can neither reap a healthy peer nor keep a - dead one. `Discover()` ages out the same way on the publisher's own - `last_seen` stamp, because the keyed Valkey record outlives a crash: a live - node keeps refreshing the key's TTL, so a dead node's field stayed readable - for the life of the fleet. `TestDeadPeerAgesOutOfPeers` (which was failing on - the tree as handed over — the reaping it asked for did not exist) now passes. -- **`peer_ttl: 0` would have crashed the process.** `time.NewTicker(PeerTTL/2)` - panics on a non-positive period, and `EXPIRE key 0` *deletes* the heartbeat - key, so a config that left `peer_ttl` unset would have taken the process down - at `Start` (or made every instance believe it is alone in the fleet). Both go - through `peerTTL()`/`peerDeadline()`, which fall back to the default. -- **Two cluster tests raced on the wire, not on the product.** A node that - joined after another's heartbeat was in flight hears about it on the next - tick, so "b does not see a", read immediately after "a sees b", is a coin - flip; and `TestStatusReportsDivergentPeers` waited for a digest on the wrong - node's view of the fleet. Both tests were wrong, not the code. -- **The partial-bind test only worked unprivileged.** - `TestStartReportsAPartialFailureButServes` used `127.0.0.1:1` as a port that - cannot be bound, which is not true in any container with - `net.ipv4.ip_unprivileged_port_start=0` — it binds, and the test failed with - both transports up. It now occupies a port with a live listener in the test - process, which is a deterministic `EADDRINUSE` on every machine and every - uid. -- **Four API/console contract breaks, all found by the new contract test:** - `blocklist.Source` and `SourceMeta` had no `json` tags at all, so the Sources - table rendered blank rows and its per-row toggle posted `name: undefined`; - `acl.GroupSummary` was flat where the console reads a nested `match`, so every - group card read "matches nothing"; `PUT /api/clients` answered 400 - `json: unknown field "ptr_suffixes"` to the payload the console sends, because - `config.Group` and `config.Match` had yaml tags only; and `PUT /api/upstreams` - answered 400 `json: unknown field "health"`, because the console round-trips - the whole server object it read, runtime health fields included. The write - structs now carry json tags, the health fields are accepted and ignored (the - server owns them), and `acl.Match` is nested. -- **A duration on the wire was a nanosecond count.** `config.Duration` had a - `MarshalYAML` but no `MarshalJSON`, so `bootstraps[].refresh` arrived as - `30000000000` while the console typed the field as a string, and - `healthcheck_interval` was a float of seconds in one place and a string in - another. Durations on the wire are duration strings everywhere now, which is - the same spelling config.yaml uses; a bare JSON number is still accepted and - means **seconds**, not nanoseconds. `TestDurationJSONForm` pins all of it and - says the old behaviour was the bug. -- **A log row's key set depended on the row.** `api.LogEntry` was `omitempty` - on `client_group`, `response`, `upstream` and `list`, so a query with no ACL - group arrived without the key and the console rendered "undefined" where the - column should be empty. The contract test can only pin one shape if the - server emits one, so nothing on that struct is `omitempty` any more. - -- **Every query from a client that asks for EDNS0 NSID got SERVFAIL.** - `dns.EDNS0_NSID.Nsid` is a *hex string* in miekg/dns v1.1.73 — `pack` - hex-decodes it, `unpack` hex-encodes it — and the engine put the literal - `"fancy-dns"` in it. `hex.DecodeString("fancy-dns")` fails, so the forwarded - upstream query could not be built, the exchange failed, and the client got - SERVFAIL. A one-line bug with total blast radius on a documented feature. - Fixed with `hex.EncodeToString`, and pinned from both sides: - `TestClientNSIDIsAnswered` (a client asking for NSID gets a real answer, the - option we forward upstream decodes to our own name, and `dns.hide_identity` - drops it) and `TestPinnedEDNS0NSIDIsHex` (the library contract, so a future - bump that changes `Nsid` to raw bytes fails here with an explanation instead - of silently breaking the feature). -- **The login rate limiter was applied to every route except the one it exists - for.** `allowRequest` is the only reader of `limitState.lockedTill` and it ran - behind `authed`, while the login route was registered through `public`. So N - bad API keys gave N × 401 and never a 429, and the lockout instead locked the - caller out of the *rest* of the API — brute-force protection is the one thing - the auth surface most needs, and it was the one thing that did not work. There - is now a `limited()` wrapper (the limiter only, no auth) on the login route; - `allowRequest` stays the single reader of the lockout state. Pinned by - `TestLoginBruteForceEventuallyHitsTheRateLimiter`, - `TestLoginLockoutExpiresWithTheClock`, `TestLoginLockoutIsPerIP`, - `TestLoginLockoutDisabledWithoutFailures` and the `TestAuthedRouteRateLimit*` - family, so both routes stay honest. -- **A successful login reset the attacker's request budget.** - `clearLoginFailures` deleted the whole per-IP `limitState`, which also reset - the per-minute window, so one valid key in the middle of a brute-force burst - handed back a fresh budget. It now clears only `failures` and `lockedTill`, - and `TestSuccessfulLoginDoesNotResetTheRequestWindow` / - `TestSuccessfulLoginClearsTheFailureCounter` pin both halves. -- **DoH GET had no size cap.** The POST path is capped by `readCapped` at - `dns.MaxMsgSize`; the GET path handed `?dns=` straight to a base64 decode and an - `Unpack`, so the only limit was net/http's `MaxHeaderBytes` — roughly a - megabyte of attacker-chosen base64 decoded per request, unauthenticated, - against a 64 KiB cap on the neighbouring path. A DNS message cannot exceed - 65535 bytes, so the cap is `base64.RawURLEncoding.EncodedLen(dns.MaxMsgSize)` - and cannot refuse a conforming client. Pinned by - `TestDoHGETQueryStringOverTheCapIsRefused` (413) and - `TestDoHServesALargeButLegalQuery` (a message just under the wire limit is - still served, so the cap cannot be tightened carelessly). -- **A clean `Stop` sometimes returned "use of closed network connection".** The - DoH listener has two owners: `Stop`'s close list, and `http.Server.Shutdown`, - which tracks the same `net.Listener` because `serveDoH` hands it to - `srv.Serve`. Whichever lost that race made an orderly shutdown look like a - failed one. Closing an already-closed listener is not a failure, so `Stop` now - ignores `net.ErrClosed` and only reports a real close error. It reproduced 3 - times in 3 runs with DoH enabled and never with it disabled, which is what - made it findable; `TestStopIsIdempotent` is the pin. -- **A per-transport `dropped` counter did not exist.** The DoH drop fix above - was invisible in the UI: `HandlerStats.Dropped` is an aggregate that cannot - say which transport dropped, and `TransportStats` had no `Dropped` field at - all, even though all four transports bump the counter. Added, with the - reasoning in the field's comment. -- **A DoH drop was not counted as a drop.** - `dropped` when `res.Drop`; the DoH path only reached `classify`, which maps - `drop` to `errors`, so `HandlerStats.Dropped` under-counted DoH. One line, in - the same position as the other two. -- **Both shipped zone files matched nothing.** `blocklist.local_zones` and the - `rpz`-format list source are read with the origin taken from the file's base - name (`zonefile.OriginFromPath`: `local.example.zone` → `local.`), so the - relative owner names in the examples were being qualified with that — a - `*.doubleclick.net` rule blocked `*.doubleclick.net.local.`, and the local - zone mixed relative owners with absolute ones. An operator copying either file - got a server that answered nothing and blocked nothing. Both examples now - spell every name out in full, and - `TestExampleLocalZoneIsOriginIndependent` (zonefile) plus - `TestExampleRPZZoneBlocksWhatItSays` (blocklist, through the real - `parseRPZFiles` path) fail if they are "tidied up" back to relative names. -- **The shipped configs were never loaded by anything.** - `deploy/config.example.yaml` and the ConfigMap in `deploy/k8s/00-config.yaml` - are now loaded through `config.Load` (which ends in `Validate`) and - spot-checked, including that the k8s `${FANCYDNS_API_KEY}` still expands and - still *fails* when the Secret is missing, and that the memory envelope matches - the StatefulSet's 1Gi limit. -- **The admin API's response shapes had drifted from the console's types** — - verified against a running binary, not inferred. `blocklist.Source` has no - `json` tags at all, so the Sources table rendered blank rows and its - per-row toggle posted `name: undefined`; `acl.GroupSummary` is flat where the - console expects a nested `match`, so every group card read "matches nothing"; - and **both write endpoints answered 400 to the payload the console actually - sends** (`json: unknown field "ptr_suffixes"` on `PUT /api/clients`, - `unknown field "health"` on `PUT /api/upstreams`). The Go side is being fixed - with contract tests that assert the exact key set of every endpoint; the - console side is the next piece of work. -- **The GitHub Actions image job could never tag an image.** It used - `${{ gitea.sha }}` in a workflow under `.github/`, and the `gitea` context - only exists on Gitea; the image is also published by - `.gitea/workflows/build.yaml`, which is the pipeline with registry access. It - now tags with `github.sha` and only logs in and pushes when this repository - actually has a registry token, so a GitHub runner builds and stops instead of - failing. -- **`api.constantTimeEqual` had no callers**, and the `authenticator` doc - comment described a constant-time "API key scan" that does not exist. The real - design is better than the comment (API keys are compared as SHA-256 digests - through a hash-keyed map, so the plaintext is never stored) — the dead - function is gone and the comment now describes the code. -- **`maxGETResponse` was dead code** whose comment claimed a guarantee the - handler did not provide. Deleted; the DoH response is written as packed. - -## Pinned, not fixed (each one has a test that says so out loud) - -The standing decision for the round after this one was: **pin, do not change**. -Every item below is behaviour someone may want to change deliberately; each has -a test that fails when it changes, and says what to do when it does. - -- **Unknown config keys are silently ignored** (a settled decision, see the - table above). `TestLoadIgnoresUnknownKeys` in `internal/config` also pins the - asymmetry that makes this surprising: a typo is invisible, while a known key - with a bad *type* is a startup failure. -- **Auth configuration is frozen at `New`.** `api.base_path`, - `api.trusted_proxies` and the authenticator (`auth.mode`, `api_keys`) are - captured once; a reload re-reads CORS only. Writing a new API key through - `PUT /api/settings` answers `ok` while the running server keeps the old key. - `TestPinnedWrittenAuthConfigIsNotAppliedUntilRestart` and - `TestCORSOriginsAreReadAtHandlerBuild` hold this in place until someone decides - whether a reload should re-apply it. -- **DoH clients behind an Ingress all present as the proxy.** `httpRemoteAddr` - uses the raw `RemoteAddr` and never consults `trusted_proxies`, so every DoH - client lands in the Ingress's ACL group: fail-closed if that group is - restrictive, fail-open if it is permissive. - `TestDoHClientIdentityIsTheProxyNotTheForwardedForHeader` pins it, with the - reason honouring `X-Forwarded-For` without `trusted_proxies` would break the - ACL and rate-limit invariants. -- **`api.cors_origins: ["*"]` echoes the request's `Origin` and sets - `Access-Control-Allow-Credentials: true`.** Deliberate (a literal `*` is - ignored by browsers when credentials are involved), but it does mean any - website can drive the admin API with whatever the browser has cached for that - host. The preflight also answers 204 for any origin and any path. - `TestCORSWildcardEchoesTheOrigin` and `TestCORSPreflight` pin all of it. -- **A partial bind failure is invisible.** `serveDoT`/`serveDoH` record a failed - bind in an error list only, so `ListenerInfo.Err` is never set and the admin - API cannot show which address failed. - `TestStartReportsAPartialFailureButServes` pins that `Start` returns nil - whenever any listener came up. -- **A question-less message is answered on UDP and kills the connection on - TCP/DoT.** `TestUDPQuestionlessMessageReachesTheEngine` pins the UDP half (the - engine turns it into a FORMERR); the stream path returns instead. Probably - right for both, but it is an asymmetry worth knowing about. -- **`doh.sent` is never bumped.** The DoH handler writes its answer through - `net/http`, so the `sent` counter is only meaningful on UDP/TCP/DoT, and the - console's transport table shows `sent: 0` for DoH. Pinned as current - behaviour by `TestCountersFollowTheEngineOutcome` with a comment saying why. -- **`api.metrics.path` is ignored** - always `${base_path}/metrics`); `POST /api/blocklists/refresh` returned - `{"names": null}`; an empty `config_yaml` in the settings form is not valid (it - decodes onto `Default()`, which enables a blocklist with no sources); `"1Gi"` - is not a valid size — only `GiB`, `G`, `GB` — even though that is how k8s - spells it. - -## Continuation plan, in order - -1. **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 - question-less message asymmetry, `doh.sent` never bumped. (The UDP query lost - at `Stop` left this list when it was fixed.) -2. **Stop hand-writing the console's types.** The contract tests make the Go - side authoritative; the cheapest durable answer is generating - `web/src/lib/types.ts` from the Go structs (or an OpenAPI document emitted by - the server) so `apiFetch` is typed against something real instead of - `JSON.parse(text) as T`. `internal/app/dump_shapes_test.go` (`DUMP_SHAPES=1`) - is the stopgap that already exists. -3. **Decide whether the shutdown grace belongs in config.** `main` hard-codes - 15s for the whole `Stop`, and the drain inside it is 250ms of that. A - Kubernetes `terminationGracePeriodSeconds` and a resolver's own notion of - "long enough to answer what is in flight" are the same number, and right now - only one of them is visible. The four-transport invariant is pinned, so - changing the bound is cheap — but the knobs do not exist yet. -4. **Read the pinned "not fixed" list in this file for anything that has since - become untrue.** Each entry claims a behaviour a reader may act on; a claim - that no longer matches the code is worse than a missing one. - -## Design notes worth preserving - -- **Cache stores packed wire bytes, not `*dns.Msg`.** A hit costs one `Unpack` - and yields a private copy, so serving never mutates shared state and TTL - ageing is free. Same trade dnsmasq makes. -- **Everything the query path reads is behind an `atomic.Pointer` holder** — ACL, - blocklist, localzone, and now the upstream pool. Readers load the pointer once - and work against a value that cannot change under them. Prefer a new immutable - snapshot over a lock. -- **Cache invalidation on rule changes flushes the cache** rather than guessing - which subtrees are affected (`invalidateBlocklistCache` in - `internal/app/hooks.go`). -- **API responses share the field names in `internal/app/hooks.go`.** If you - rename one, grep the frontend too — and now also expect `internal/app`'s - contract test to fail, which is the point of it. -- **API keys are compared by SHA-256 hash, constant-time**, and - `X-Forwarded-For` is honoured *only* from `trusted_proxies`. -- **The UDP listener resets its read deadline every iteration.** A deadline set - once expires for good and turns the loop into a spin that never reads another - packet (caught by `TestBlocklistToggle`). -- **Shutdown order is: cancel, close every listener, then wait.** The close is - not interruptible by the caller's context, because releasing the ports is the - work the next process start needs. Everything after it is interruptible, and - a cancelled context is expected to be able to cut the *wait* short — which is - why no step in the shutdown may depend on a client doing anything. -- **Only the Valkey lease holder fetches lists**, so N instances do not hit a - mirror N times. -- **Serve-stale** answers get TTL 1; an expired upstream answer is never cached; - a truncated answer is never cached (it would be served truncated forever). -- **`PatternKind.Pattern` is the display form; `WireName` is the wildcard form.** - The internal `*.` / `**.` prefixes must not leak into the UI, but the API - needs them to re-add a rule unchanged (the domains list ships `rule` for - exactly this). -- **pprof is loopback-only by construction**, not by policy, so no config flag - can turn it into a network-reachable unauthenticated endpoint. -- **A master file's origin is its base name**, so a relative owner name in a - zone is qualified with the file's name, not with the zone you meant. Absolute - names are the only spelling that survives a rename. +- `make check-go`: the inner loop (~1s): gofmt, vet, compile (no npm). +- `make check`: `check-go` plus `go test -race ./...`; what CI runs. +- `make smoke`: boots the real binary with the console embedded and checks the + API, metrics and auth. +- `make web-check`: console typecheck and production build. +- `make build`: both binaries with the console embedded (needs npm). +- `make bench`: cache and blocklist benchmarks. ## Test-hygiene traps -- `dns.NewRR` needs a TTL column: `dns.NewRR("host.example.com")` does not - parse. That is a fixture bug, not a product bug. -- A test that closes a stop channel *after* `wg.Wait()` deadlocks the whole - package and looks like "the suite hangs". Close first, then wait. -- Assert on invariants, not on what a concurrent goroutine happened to observe. - "A reader must have seen address X" is a flake; "every list a reader sees is - one of the published ones" is a real property and cannot flake. -- **A test that only dials a stream transport races the accept loop.** Stop can - close the listener while the connection is still in the backlog, the server - never accepts it, and any shutdown assertion that needs a live server-side - goroutine passes for the wrong reason. Exchange one query first, then leave - the connection alone — that is what `harness.idleStream` is for. -- **Count leaked goroutines by stack, not by `runtime.NumGoroutine()`.** A test - binary shares its process with net/http, crypto/tls and the race detector; a - baseline count is noise. "Does any goroutine still have a frame in this - package?" is a property of the product, and it needs the caller's own - goroutine filtered out by id or every run finds exactly one. -- `tsc -b` replays **cached diagnostics** from `node_modules/.tmp/*.tsbuildinfo`. - After a large edit, `rm node_modules/.tmp/*.tsbuildinfo`. -- `vite build` empties `outDir` and **deletes `internal/web/dist/.gitkeep`**. - Restore it or the directory disappears from git. `//go:embed all:dist` still - builds while the directory is empty, so a console rebuild in the middle of - someone else's `go build` is not fatal. -- **A `TXT` string over 255 bytes does not pack** in miekg/dns v1.1.73 — the - error is `dns: string exceeded 255 bytes in txt`, and 255 is the exact - boundary. On a fake upstream's write path the failure is silent and presents - as a client timeout, so build a multi-kilobyte answer out of many short - strings and assert the fake's write error before blaming the client. -- **`dns.EDNS0_NSID.Nsid` is a hex string, not the bytes on the wire.** A plain - value such as `"fancy-dns"` fails with `dns: overflow packing opt`, and a long - NSID packs fine (unlike `TXT`). Always hex-encode on the way in. -- `gofmt -l internal/` is the fastest way to see that a file does not parse. -- The console has **no test runner at all** — only `tsc -b --noEmit` and - `oxlint`. A field-name drift between the API and the console is invisible to - both, which is why the Go side now has contract tests. - -## Fixed earlier and worth not re-breaking - -A running list, kept because each of these was invisible until it was found: - -- `config.ParseSize` ignored the unit multiplier entirely (`512MiB` parsed as - 512 bytes), so every memory budget in the tree was off by a factor of the - unit. `size_test.go` pins the units, the shadowing cases, the range and the - truncating rounding. -- A cache miss and a stale hit moved only the Prometheus counter and not the - atomics the console's `Totals` reads, so the Cache page read zero for the - life of the process. -- `finalize` — the EDNS0/UDP-size step that turns an oversized answer into a - question-only TC reply — ran only on the cache-hit path, so a multi-kilobyte - answer from upstream, from the local zone or from a sinkhole was left as a - datagram a 512-byte UDP client silently dropped. -- The serve-stale prefetch gate was a 64-slot channel, so a burst of stale hits - on one hot name launched up to 64 duplicate upstream exchanges instead of the - one its own comment promised. It is a per-key latch now. -- The upstream UDP client read buffer was hard-coded to 1232 while the engine - advertises `dns.max_udp_size`, so an operator who raised it got SERVFAIL for - every answer over 1232 bytes. -- Every relative RDATA name in a master file pointed at the DNS root: the - qualification pass ran *after* `dns.NewRR`, so `www CNAME host` became - `CNAME host.`. It is now a token-level pass driven by a table of which RDATA - positions hold a name. -- The cache snapshot never round-tripped: `Load` decoded the record header out of - the *key's* remainder instead of the value, so every restart was a cold cache. - There is now an `encodeRecord`/`decodeRecord` pair with a version byte and a - CRC-32C trailer, and `Load` drops expired entries and rewrites the TTL to the - time actually left. -- `${VAR}` expansion never worked inside quotes — the only form anyone writes — - because the reference check sat below the quote branches in the same switch. - The admin API key was the literal string `"${FANCYDNS_API_KEY}"`. -- ACL PTR suffix matching crossed label boundaries, so a group scoped to - `example.com.` also swallowed `ample.com.`; the ACL `*` catch-all was dropped - from the rendered domain patterns; `Upstream.Success` never closed an open - breaker; `Exchange` fanned out in parallel even under `policy: failover`; the - cache byte budget was never fed to the LRU. -- **`TestRebindUnderConcurrentQueries` was a flake** (2 runs in 10) because it - asserted a reader had to observe two specific addresses. It now pins the - copy-on-write invariant itself. -- CI could not have passed (`go.mod` requires Go 1.27.1, CI and the Dockerfile - pinned 1.23), the Dockerfile clobbered the console by copying the source tree - after the bundle, and the k8s set could not have worked (`hostNetwork: true` - with `replicas: 2` and no SO_REUSEPORT). -- `$TTL` was ignored by the zone parser; every record got the SOA minimum. -- Blocklist: leading BOM stripped, `!allow` works, `SourceMeta` exists for a - rule-only source, `Pattern()` reports the bare name while `WireName()` gives - the wildcard form, RPZ inherits the zone default TTL, hosts files complain - about a missing address, adblock handles CRLF and over-long lines. -- `serveUDP` called `net.ResolveUDPAddr` per packet; the peer already is a - `*net.UDPAddr`. +These are in `AGENTS.md` (TXT strings over 255 bytes, `EDNS0_NSID` is hex, +`dns.NewRR` needs a TTL, closing a stop channel after `wg.Wait()`, counting +goroutines by stack, `tsc -b` replaying cached diagnostics, `vite build` +deleting `internal/web/dist/.gitkeep`). One more from this round: a Go test that +needs a Kubernetes client should use `dynamic/fake` with +`NewSimpleDynamicClientWithCustomListKinds` and list every GVR in +`kubesource.allGVRs`, or a `List` panics. diff --git a/deploy/OUTSIDE-CLUSTER.md b/deploy/OUTSIDE-CLUSTER.md new file mode 100644 index 0000000..326f288 --- /dev/null +++ b/deploy/OUTSIDE-CLUSTER.md @@ -0,0 +1,292 @@ +# Running fancy-dns outside the cluster + +This is how to run a second resolver on a machine that is not a pod, for +example the controller node, as a backup for the in-cluster one. It uses the +same code and the same cluster objects (`DNSZone`, `ClientGroup`, `BlockList`, +`AddressOverride`, `DomainRule`), so an edit made in the console reaches both +resolvers and nobody keeps a second copy of the configuration. + +There are two binaries and you probably only want the first: + +| Binary | Role | On the controller node | +|---|---|---| +| `fancy-dns` | The resolver. Answers DNS, reads the cluster objects with a watch, never writes them. | **Yes**: this is the backup. | +| `fancy-dns-console` | The web UI. The only thing that writes the objects, and it proxies the rest to a resolver. Not on the DNS path. | Optional. See [the console](#the-console-outside-the-cluster). | + +If you meant something else by "the client", say so; the rest of this file +assumes the resolver. + +## How it behaves when the cluster is gone + +This is the reason to run it, so know it first. + +| Situation | What the resolver does | +|---|---| +| Cluster reachable | Serves the cluster objects live and saves a snapshot of them to `/kube-snapshot.json` after every change. | +| Cluster unreachable at start | Waits 30 seconds, then starts from the saved snapshot and keeps retrying in the background. `GET /api/status` lists `kubernetes: not synced, serving the last saved cluster configuration` under `components.degraded` (and `GET /api/healthz`, which needs no key, reports `degraded`). | +| Cluster unreachable at start, **never synced before** | Starts with the YAML file only: no cluster zones, address overrides, groups or lists. `GET /api/status` says `not synced and no saved snapshot`. Start it once while the cluster is up, and keep its data directory. | +| Cluster goes away while running | Keeps serving what it has and reconnects when the API server returns. | +| The kubeconfig is missing or unreadable | **Refuses to start.** A resolver that cannot build a client at all is a configuration error, not a degraded cluster. Test the kubeconfig (below) before relying on it. | + +Because of the snapshot, the backup needs a data directory that survives +reboots. That is also where the cache snapshot lives, so it is the same +directory you would have anyway. + +Plain DNS to the backup does not depend on the console, on Valkey, or on the +in-cluster resolver being up. + +## 1. Give it read-only access to the objects + +Do not copy the cluster's admin kubeconfig onto a DNS server if you can avoid it: +the resolver only ever needs to read five resource types in one namespace. +`deploy/k8s/crds/rbac.yaml` already defines that Role (`fancy-dns-objects`). +Bind a dedicated service account to it and give that account a long-lived token: + +```yaml +# kubectl apply -f this-file.yaml +apiVersion: v1 +kind: ServiceAccount +metadata: {name: fancy-dns-backup, namespace: dns} +--- +apiVersion: v1 +kind: Secret +metadata: + name: fancy-dns-backup-token + namespace: dns + annotations: {kubernetes.io/service-account.name: fancy-dns-backup} +type: kubernetes.io/service-account-token +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: {name: fancy-dns-objects-backup, namespace: dns} +subjects: [{kind: ServiceAccount, name: fancy-dns-backup, namespace: dns}] +roleRef: {apiGroup: rbac.authorization.k8s.io, kind: Role, name: fancy-dns-objects} +``` + +Build a kubeconfig from it. Run this where you have admin `kubectl`, then copy +the file to the node as `/etc/fancy-dns/kubeconfig`: + +```sh +SERVER=$(kubectl config view --minify -o jsonpath='{.clusters[0].cluster.server}') +CA=$(kubectl -n dns get secret fancy-dns-backup-token -o jsonpath='{.data.ca\.crt}') +TOKEN=$(kubectl -n dns get secret fancy-dns-backup-token -o jsonpath='{.data.token}' | base64 -d) + +cat > fancy-dns.kubeconfig < +short +``` + +`components.degraded` should be empty. The two `dig`s tell you it resolves and +that it has the cluster's data. Then simulate the outage you bought this for: +block the API server from the node (or stop the resolver and start it with the +cluster unreachable), and confirm the same names still answer and +`components.degraded` now names the snapshot. + +Finally point clients at it as a **second** nameserver (DHCP option 6, or the +router's secondary). It is a backup, not a load balancer: most stub resolvers +only try the second server after the first times out. + +## The console outside the cluster + +Optional. You only need it on the node if you want to edit while the in-cluster +console is unavailable, and it can only save while the API server is reachable, +since an edit is a write to the cluster. Reads of statistics come from whichever +resolver you point it at. + +It needs a **write** Role, which is a separate account from the resolver's. +`deploy/k8s/console/console.yaml` defines the Role `fancy-dns-console`; bind a +second service account to it the same way as above (name it +`fancy-dns-console-ext`, build `/etc/fancy-dns/console.kubeconfig`). + +`/etc/fancy-dns/console.yaml` (see `deploy/console.example.yaml`): + +```yaml +blocklist: {enabled: false} +api: + enabled: true + listen: ["127.0.0.1:8081"] + base_path: /api + ui: true + auth: + mode: apikey + api_keys: ["${FANCYDNS_CONSOLE_KEY}"] + trust_loopback: false +``` + +`/etc/fancy-dns/console.env`: + +```sh +FANCYDNS_CONSOLE_KEY= +FANCYDNS_RUNTIME_KEY= +``` + +Run it with a unit like the resolver's, minus the port-53 capability: + +```ini +ExecStart=/usr/local/bin/fancy-dns-console \ + -config /etc/fancy-dns/console.yaml \ + -runtime-url http://127.0.0.1:8080 \ + -kubeconfig /etc/fancy-dns/console.kubeconfig \ + -kube-namespace dns +``` + +Keep it on loopback and reach it with an SSH tunnel +(`ssh -L 8081:127.0.0.1:8081 controller`) or an authenticated TLS reverse proxy. +The UI can change DNS for every client you have; do not publish it with an API +key on plain HTTP. + +## Troubleshooting + +| Symptom | Likely cause | +|---|---| +| Service exits immediately: `no usable Kubernetes credentials` | `-kubeconfig` points at a file that is missing, unreadable by the `fancy-dns` user, or malformed. | +| Starts, `components.degraded` says `not synced` | The API server is unreachable or the token is rejected (`kubectl --kubeconfig … get clientgroups`), or the CRDs are not installed. | +| `…not synced and no saved snapshot` and cluster data missing | First start with the cluster down. Start it once with the cluster up. Also check the data directory is writable. | +| `kubernetes: objects rejected` in `components.degraded` | An object failed validation (or this binary is older than the cluster's). The last good version stays in force. Which objects, and why: `jq .problems /var/lib/fancy-dns/kube-snapshot.json`. | +| Cluster answers differ from the backup's | The upstream/forward-zone config differs (it is YAML, not an object), or the backup's cache holds older answers. `POST /api/cache/flush` on it. | +| `bind: address already in use` | Something owns port 53; see [Port 53](#port-53). | +| Console: `cluster: …forbidden` | It is using the read-only kubeconfig; it needs the write Role. | diff --git a/deploy/compose/README.md b/deploy/compose/README.md index e579b6f..e22e5b9 100644 --- a/deploy/compose/README.md +++ b/deploy/compose/README.md @@ -5,6 +5,11 @@ This Compose deployment follows `deploy/k8s/00-config.yaml` and of pulling `registry.k6n.net/fancydns`, avoiding a bootstrap dependency on the cluster registry DNS. +> **Following the cluster's configuration.** This Compose setup is configured by +> files and has no link to the cluster's `ClientGroup`, `DNSZone` and other +> objects. To run a backup resolver that reads them (and keeps serving a saved +> copy when the cluster is down), see [`../OUTSIDE-CLUSTER.md`](../OUTSIDE-CLUSTER.md). + ## Prepare and start From the repository root: diff --git a/deploy/compose/config.yaml b/deploy/compose/config.yaml index 34f23e2..6812166 100644 --- a/deploy/compose/config.yaml +++ b/deploy/compose/config.yaml @@ -63,7 +63,6 @@ cache: blocklist: enabled: true default_action: nxdomain - managed_zones: /var/lib/fancy-dns/managed-zones local_zones: ["/etc/fancy-dns/zones/*.zone"] lists: - name: edge-router-x-address-overrides @@ -83,12 +82,12 @@ acl: default_policy: allow groups: [] -qlog: +# One JSON line per blocked or failed query on stdout (docker logs). The +# resolver keeps no query log of its own; the metrics count everything. +access_log: enabled: true - path: /var/lib/fancy-dns/queries.db - max_entries: 200000 - sample_rate: 1.0 log_blocked: true + log_errors: true log_cached: false api: diff --git a/internal/acl/acl.go b/internal/acl/acl.go index 0cdfa2b..d4ffa88 100644 --- a/internal/acl/acl.go +++ b/internal/acl/acl.go @@ -327,6 +327,12 @@ func matchPTR(n, suffix string) bool { if suffix == "" || len(n) < len(suffix) || !strings.HasSuffix(n, suffix) { return false } + // A suffix written with its own leading dot (".lan.") is the natural way to + // say "anything under lan" and the console's own hint suggests it. Its dot + // is the label boundary, so there is nothing more to check. + if suffix[0] == '.' { + return true + } // The suffix has to start a label: it is either the whole name, or the // character in front of it is a dot. rest := n[:len(n)-len(suffix)] diff --git a/internal/acl/acl_test.go b/internal/acl/acl_test.go index cf7abd8..340b99d 100644 --- a/internal/acl/acl_test.go +++ b/internal/acl/acl_test.go @@ -381,6 +381,12 @@ func TestMatchPTRLabelBoundary(t *testing.T) { {"empty suffix", "host.example.com.", "", false}, {"empty name", "", "example.com.", false}, {"a hyphen is not a label boundary", "host-example.com.", "example.com.", false}, + // The console suggests ".lan" as a suffix; the leading dot was being + // counted as part of the label, so it never matched anything. + {"leading-dot suffix matches a name under it", "host.lan.", ".lan.", true}, + {"leading-dot suffix matches deeper names", "a.b.lan.", ".lan.", true}, + {"leading-dot suffix does not match the apex", "lan.", ".lan.", false}, + {"leading-dot suffix still respects the label", "hostlan.", ".lan.", false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/acl/explain.go b/internal/acl/explain.go new file mode 100644 index 0000000..26a9299 --- /dev/null +++ b/internal/acl/explain.go @@ -0,0 +1,414 @@ +package acl + +import ( + "fmt" + "net/netip" + "strings" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" +) + +// Finding is one observation about a group, tied to the field it concerns so +// an editor can show it next to the input. +type Finding struct { + Field string `json:"field"` + Message string `json:"message"` +} + +// Analysis is what Analyze says about a group that is structurally valid. +// +// It exists for people who do not know what the settings do. The engine's +// behaviour is not always what a setting's name suggests (an allow list is an +// allowlist-only mode, a rate limit of 0 exempts the group from the default), +// so the analysis states the effective behaviour in plain words and flags the +// settings that are legal but almost certainly not what was meant. +type Analysis struct { + // Summary is the group in plain English, one sentence per line. + Summary []string `json:"summary"` + // Warnings are legal settings that probably do not do what was intended. + Warnings []Finding `json:"warnings"` + // Notes are facts worth knowing that are not mistakes. + Notes []Finding `json:"notes"` + // Unreachable is set when no client can ever land in the group, because + // the groups above it claim everything it selects. + Unreachable bool `json:"unreachable"` +} + +// Context is the group under review and everything around it. +type Context struct { + // Groups is the full evaluation order, with the group under review in it. + Groups []config.Group + // Index is the position of the group under review within Groups. + Index int + // DefaultRateLimit is acl.default_rate_limit (queries/second/client). + DefaultRateLimit int + // DefaultPolicy is what a client that matches no group gets. + DefaultPolicy string + // Upstreams are the configured upstream addresses. Nil means unknown, and + // the upstream check is skipped. + Upstreams []string +} + +// Analyze explains Groups[Index]. It does not validate: a group that fails +// validation is reported by the caller's own checks, and Analyze copes with +// whatever it is handed. +func Analyze(c Context) Analysis { + a := Analysis{Summary: []string{}, Warnings: []Finding{}, Notes: []Finding{}} + if c.Index < 0 || c.Index >= len(c.Groups) { + return a + } + g := c.Groups[c.Index] + warn := func(field, format string, args ...any) { + a.Warnings = append(a.Warnings, Finding{field, fmt.Sprintf(format, args...)}) + } + note := func(field, format string, args ...any) { + a.Notes = append(a.Notes, Finding{field, fmt.Sprintf(format, args...)}) + } + + a.Summary = append(a.Summary, whoSentence(g), c.orderSentence()) + + // What the group does. + switch g.Policy { + case "deny": + a.Summary = append(a.Summary, "Every query from these clients is refused (REFUSED). Nothing below applies to them.") + for _, ignored := range []struct { + field string + set bool + }{ + {"allow_domains", len(g.AllowDomains) > 0}, {"deny_domains", len(g.DenyDomains) > 0}, + {"skip_blocklist", g.SkipBlocklist}, {"skip_cache", g.SkipCache}, + {"rate_limit", g.RateLimit > 0}, {"max_concurrent", g.MaxConcurrent > 0}, + {"upstreams", len(g.Upstreams) > 0}, + } { + if ignored.set { + note(ignored.field, "Ignored: this group's policy is deny, which refuses the query before anything else is looked at.") + } + } + case "block_only": + a.Summary = append(a.Summary, "Queries are resolved normally, and names on a blocklist are blocked.") + warn("policy", "block_only currently behaves exactly like allow: both resolve everything and both apply the blocklists. Only deny changes what the group can do. Pick allow unless you are keeping block_only for a reason.") + default: + a.Summary = append(a.Summary, "Queries are resolved normally, and names on a blocklist are blocked.") + } + + if g.Policy != "deny" { + a.analyzeBehaviour(c, g, warn, note) + } + a.analyzeMatch(g, warn, note) + a.analyzeDomains(g, warn, note) + a.analyzeOrder(c, warn) + return a +} + +func (a *Analysis) analyzeBehaviour(c Context, g config.Group, warn, note func(string, string, ...any)) { + if len(g.AllowDomains) > 0 { + a.Summary = append(a.Summary, fmt.Sprintf("Allowlist mode: ONLY %s can be looked up. Every other name is answered as blocked.", joinShort(g.AllowDomains))) + note("allow_domains", "A non-empty allow list turns the group into allowlist-only mode. It does not exempt names from the blocklists; use \"Skip blocklists\" for that.") + } + if len(g.DenyDomains) > 0 { + a.Summary = append(a.Summary, fmt.Sprintf("%s is always blocked for these clients, whatever the blocklists say.", joinShort(g.DenyDomains))) + } + if g.SkipBlocklist { + a.Summary = append(a.Summary, "Blocklists are ignored for these clients: ads and trackers resolve normally.") + } + if g.SkipCache { + a.Summary = append(a.Summary, "Nothing is served from or stored in the cache for these clients; every query goes upstream.") + note("skip_cache", "Every query from this group costs an upstream round trip, so it is slower and adds load. Use it for debugging, not as a default.") + } + switch { + case g.RateLimit > 0: + a.Summary = append(a.Summary, fmt.Sprintf("Each client address may send %d queries per second; above that, queries are refused.", g.RateLimit)) + case c.DefaultRateLimit > 0: + a.Summary = append(a.Summary, "There is no rate limit for these clients.") + warn("rate_limit", "0 means unlimited here, not \"use the default\": this group is exempt from the default limit of %d queries per second per client.", c.DefaultRateLimit) + } + if g.MaxConcurrent > 0 { + a.Summary = append(a.Summary, fmt.Sprintf("A client may have at most %d queries in flight; the excess get SERVFAIL.", g.MaxConcurrent)) + } + if g.LogQueries { + a.Summary = append(a.Summary, "Every query from these clients is written to the access log, even when logging is otherwise sampled or limited to blocked queries.") + } + if len(g.Upstreams) > 0 { + a.Summary = append(a.Summary, fmt.Sprintf("Queries go to %s instead of the default upstreams.", joinShort(g.Upstreams))) + note("upstreams", "Conditional-forwarding zones (upstream.forward_zones) still take precedence over a group's upstreams.") + if c.Upstreams != nil { + known := map[string]bool{} + for _, u := range c.Upstreams { + known[strings.ToLower(u)] = true + } + var unknown []string + for _, u := range g.Upstreams { + if !known[strings.ToLower(u)] { + unknown = append(unknown, u) + } + } + switch { + case len(unknown) == len(g.Upstreams): + warn("upstreams", "None of these addresses is a configured upstream, so the setting is ignored and this group uses ALL upstreams. Use an address exactly as it appears on the Upstreams page.") + case len(unknown) > 0: + warn("upstreams", "%s is not a configured upstream and is ignored.", joinShort(unknown)) + } + } + } +} + +func (a *Analysis) analyzeMatch(g config.Group, warn, note func(string, string, ...any)) { + if g.Match.Any { + warn("match.any", "This group matches every client, so any addresses, suffixes or tags below it are ignored, and so are all groups after it.") + } + for _, s := range g.Match.CIDRs { + p, err := netip.ParsePrefix(strings.TrimSpace(s)) + if err != nil { + continue + } + if p != p.Masked() { + warn("match.cidrs", "%s has host bits set; it is treated as %s.", s, p.Masked()) + } + if (p.Addr().Is4() && p.Bits() < 8) || (!p.Addr().Is4() && p.Bits() < 16) { + warn("match.cidrs", "%s is a very wide range: it covers a large share of the address space, probably more than you meant.", s) + } + } + if len(g.Match.PTRSuffixes) > 0 || len(g.Match.Tags) > 0 { + note("match", "Reverse-DNS suffixes and tags depend on the client's PTR record. The first queries from a client whose name is not yet cached are matched on address alone, so use a CIDR when the match must be reliable.") + } + for _, t := range g.Match.Tags { + if strings.Contains(t, ".") { + warn("match.tags", "Tag %q contains a dot. A tag is a single label of the client's reverse name (\"laptop\" for laptop.lan.example.net), so this never matches. Use a reverse-DNS suffix for a domain.", t) + } + } + for _, s := range g.Match.PTRSuffixes { + if strings.ContainsAny(s, "*/ ") { + warn("match.ptr_suffixes", "%q is not a plain domain suffix; wildcards and paths are not understood here, so this never matches.", s) + } + } +} + +func (a *Analysis) analyzeDomains(g config.Group, warn, note func(string, string, ...any)) { + lint := func(field string, patterns []string) { + for _, raw := range patterns { + p := strings.ToLower(strings.TrimSpace(raw)) + switch { + case p == "": + case strings.Contains(p, "://") || strings.Contains(p, "/"): + warn(field, "%q looks like a URL. Use only the host name, e.g. example.com.", raw) + case strings.ContainsAny(p, " \t"): + warn(field, "%q contains a space, so it never matches a name.", raw) + case strings.HasPrefix(p, "."): + warn(field, "%q starts with a dot, which is matched literally and never matches. Write *.%s to cover the name and everything under it.", raw, strings.TrimLeft(p, ".")) + case p == "*" || p == "**" || p == "*.*": + if field == "deny_domains" { + warn(field, "%q blocks every name for this group.", raw) + } else { + warn(field, "%q allows every name, so the allow list restricts nothing.", raw) + } + case strings.Contains(strings.TrimPrefix(strings.TrimPrefix(p, "**."), "*."), "*"): + warn(field, "%q uses a wildcard somewhere other than the start, which is not supported and never matches. Only a leading *. or **. works.", raw) + } + } + } + lint("allow_domains", g.AllowDomains) + lint("deny_domains", g.DenyDomains) + + if len(g.AllowDomains) > 0 || len(g.DenyDomains) > 0 { + note("deny_domains", "*.example.com and **.example.com mean the same thing in these lists: the name itself and everything under it. Write example.com for the exact name only.") + } + if len(g.AllowDomains) > 0 && len(g.DenyDomains) > 0 { + allow, deny := NewDomainSet(g.AllowDomains), NewDomainSet(g.DenyDomains) + for _, p := range g.AllowDomains { + name := strings.TrimPrefix(strings.TrimPrefix(strings.ToLower(strings.TrimSpace(p)), "**."), "*.") + if name != "" && deny.Match(name) && allow.Match(name) { + warn("allow_domains", "%q is also covered by the deny list, and deny wins, so it stays blocked.", p) + } + } + } +} + +// analyzeOrder reports groups that shadow this one, and groups this one would +// shadow. Evaluation is first match wins, so a group whose clients are all +// claimed higher up is dead weight. +func (a *Analysis) analyzeOrder(c Context, warn func(string, string, ...any)) { + g := c.Groups[c.Index] + if g.Match.Any { + // The group is the catch-all: the warning on match.any covers it, and + // everything after it is reported below. + } else if covered, by, all := coverage(g, c.Groups[:c.Index]); all { + a.Unreachable = true + warn("match", "No client can ever land in this group: everything it selects is already claimed by %q above it, which wins.", by) + } else { + for _, item := range covered { + warn("match", "%s is already claimed by %q above this group, so those clients use that group's settings instead.", item.item, item.by) + } + } + for j := c.Index + 1; j < len(c.Groups); j++ { + later := c.Groups[j] + with := coverageAll(later, c.Groups[:j]) + without := coverageAll(later, append(append([]config.Group(nil), c.Groups[:c.Index]...), c.Groups[c.Index+1:j]...)) + if with && !without { + warn("match", "Saving this group makes %q unreachable: it now claims everything that group selects.", later.Name) + } + } +} + +type claimed struct{ item, by string } + +// coverage reports which of g's selectors a group in prior already claims, +// and whether that is all of them (so g can never match anyone). +func coverage(g config.Group, prior []config.Group) (items []claimed, first string, all bool) { + total := len(g.Match.CIDRs) + len(g.Match.PTRSuffixes) + len(g.Match.Tags) + if total == 0 { + return nil, "", false + } + for _, p := range prior { + if p.Match.Any { + return nil, p.Name, true + } + } + for _, s := range g.Match.CIDRs { + c, err := netip.ParsePrefix(strings.TrimSpace(s)) + if err != nil { + continue + } + for _, p := range prior { + if by := coveringCIDR(c, p); by { + items = append(items, claimed{s, p.Name}) + break + } + } + } + for _, s := range g.Match.PTRSuffixes { + sfx := normSuffix(s) + for _, p := range prior { + if coveringSuffix(sfx, p) { + items = append(items, claimed{s, p.Name}) + break + } + } + } + for _, t := range g.Match.Tags { + t = strings.ToLower(strings.TrimSpace(t)) + for _, p := range prior { + if containsFold(p.Match.Tags, t) { + items = append(items, claimed{"tag " + t, p.Name}) + break + } + } + } + if len(items) == total { + first = items[0].by + return items, first, true + } + return items, "", false +} + +func coverageAll(g config.Group, prior []config.Group) bool { + if g.Match.Any { + return false + } + _, _, all := coverage(g, prior) + return all +} + +func coveringCIDR(c netip.Prefix, p config.Group) bool { + for _, s := range p.Match.CIDRs { + q, err := netip.ParsePrefix(strings.TrimSpace(s)) + if err != nil { + continue + } + if q.Bits() <= c.Bits() && q.Contains(c.Masked().Addr()) { + return true + } + } + return false +} + +func coveringSuffix(sfx string, p config.Group) bool { + for _, s := range p.Match.PTRSuffixes { + earlier := normSuffix(s) + // Anything under sfx is under earlier when sfx ends with earlier at a + // label boundary; the same test the matcher itself uses. + if matchPTR(sfx, earlier) { + return true + } + } + return false +} + +func normSuffix(s string) string { + s = strings.ToLower(strings.TrimSpace(s)) + if s != "" && !strings.HasSuffix(s, ".") { + s += "." + } + return s +} + +func containsFold(list []string, want string) bool { + for _, s := range list { + if strings.EqualFold(strings.TrimSpace(s), want) { + return true + } + } + return false +} + +func whoSentence(g config.Group) string { + if g.Match.Any { + return "Applies to every client." + } + var parts []string + if len(g.Match.CIDRs) > 0 { + parts = append(parts, "whose address is in "+joinShort(g.Match.CIDRs)) + } + if len(g.Match.PTRSuffixes) > 0 { + parts = append(parts, "whose reverse-DNS name ends in "+joinShort(g.Match.PTRSuffixes)) + } + if len(g.Match.Tags) > 0 { + parts = append(parts, "tagged "+joinShort(g.Match.Tags)) + } + if len(parts) == 0 { + return "Selects no clients yet." + } + return "Applies to clients " + strings.Join(parts, ", or ") + "." +} + +func (c Context) orderSentence() string { + pos, n := c.Index+1, len(c.Groups) + switch { + case pos == 1 && n == 1: + return "It is the only group; clients that match nothing get the default policy (" + c.defaultPolicy() + ")." + case pos == 1: + return fmt.Sprintf("It is checked first of %d groups; a client matching it never reaches the others.", n) + default: + return fmt.Sprintf("It is checked %s of %d groups, after the %d above it; clients matching none get the default policy (%s).", ordinal(pos), n, pos-1, c.defaultPolicy()) + } +} + +func (c Context) defaultPolicy() string { + if c.DefaultPolicy == "" { + return "allow" + } + return c.DefaultPolicy +} + +func ordinal(n int) string { + switch n { + case 2: + return "second" + case 3: + return "third" + } + return fmt.Sprintf("number %d", n) +} + +// joinShort lists up to three items and counts the rest. +func joinShort(items []string) string { + clean := make([]string, 0, len(items)) + for _, s := range items { + if s = strings.TrimSpace(s); s != "" { + clean = append(clean, s) + } + } + if len(clean) > 3 { + return strings.Join(clean[:3], ", ") + fmt.Sprintf(" and %d more", len(clean)-3) + } + return strings.Join(clean, ", ") +} diff --git a/internal/acl/explain_test.go b/internal/acl/explain_test.go new file mode 100644 index 0000000..b14ced9 --- /dev/null +++ b/internal/acl/explain_test.go @@ -0,0 +1,172 @@ +package acl + +import ( + "strings" + "testing" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" +) + +func named(name, policy string, m config.Match) config.Group { + return config.Group{Name: name, Policy: policy, Match: m} +} + +func analyzeAt(idx int, defRate int, groups ...config.Group) Analysis { + return Analyze(Context{Groups: groups, Index: idx, DefaultRateLimit: defRate, DefaultPolicy: "allow"}) +} + +func has(fs []Finding, field, contains string) bool { + for _, f := range fs { + if f.Field == field && strings.Contains(f.Message, contains) { + return true + } + } + return false +} + +func TestAnalyzeAllowListIsAllowlistMode(t *testing.T) { + g := named("kids", "allow", config.Match{CIDRs: []string{"10.0.5.0/24"}}) + g.AllowDomains = []string{"*.school.example"} + a := analyzeAt(0, 0, g) + if !has(a.Notes, "allow_domains", "allowlist-only") { + t.Errorf("notes = %+v, want the allowlist-only explanation", a.Notes) + } + if !strings.Contains(strings.Join(a.Summary, " "), "ONLY") { + t.Errorf("summary = %v, want it to say only the listed names resolve", a.Summary) + } +} + +func TestAnalyzeBlockOnlyIsTheSameAsAllow(t *testing.T) { + a := analyzeAt(0, 0, named("g", "block_only", config.Match{CIDRs: []string{"10.0.0.0/24"}})) + if !has(a.Warnings, "policy", "exactly like allow") { + t.Errorf("warnings = %+v, want the block_only warning", a.Warnings) + } +} + +func TestAnalyzeRateLimitZeroExemptsFromTheDefault(t *testing.T) { + g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}}) + if a := analyzeAt(0, 50, g); !has(a.Warnings, "rate_limit", "exempt") { + t.Errorf("warnings = %+v, want the exemption warning", a.Warnings) + } + if a := analyzeAt(0, 0, g); len(a.Warnings) != 0 { + t.Errorf("no default limit, yet warnings = %+v", a.Warnings) + } + g.RateLimit = 20 + if a := analyzeAt(0, 50, g); has(a.Warnings, "rate_limit", "exempt") { + t.Errorf("an explicit limit must not warn about exemption: %+v", a.Warnings) + } +} + +func TestAnalyzeDenyIgnoresTheRest(t *testing.T) { + g := named("g", "deny", config.Match{CIDRs: []string{"10.0.0.0/24"}}) + g.RateLimit = 5 + g.SkipBlocklist = true + a := analyzeAt(0, 50, g) + if !has(a.Notes, "rate_limit", "Ignored") || !has(a.Notes, "skip_blocklist", "Ignored") { + t.Errorf("notes = %+v, want ignored-setting notes", a.Notes) + } + if has(a.Warnings, "rate_limit", "exempt") { + t.Errorf("a denied group has no rate limit to be exempt from: %+v", a.Warnings) + } +} + +func TestAnalyzeShadowing(t *testing.T) { + wide := named("lan", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}}) + narrow := named("cam", "deny", config.Match{CIDRs: []string{"10.1.2.0/24"}}) + + a := analyzeAt(1, 0, wide, narrow) + if !a.Unreachable || !has(a.Warnings, "match", `"lan"`) { + t.Errorf("narrow range after a wider one: unreachable=%v warnings=%+v", a.Unreachable, a.Warnings) + } + + // The other order is the useful one and must stay quiet. + if a := analyzeAt(0, 0, narrow, wide); a.Unreachable || len(a.Warnings) > 1 { + t.Errorf("specific-before-general flagged: unreachable=%v warnings=%+v", a.Unreachable, a.Warnings) + } + + // Saving the wide group above the narrow one buries the narrow one. + a = analyzeAt(0, 0, wide, narrow) + if !has(a.Warnings, "match", `makes "cam" unreachable`) { + t.Errorf("warnings = %+v, want the newly-unreachable warning", a.Warnings) + } +} + +func TestAnalyzePartialOverlapNamesTheClaimedRange(t *testing.T) { + first := named("a", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}}) + second := named("b", "allow", config.Match{CIDRs: []string{"10.0.0.128/25", "192.168.0.0/24"}}) + a := analyzeAt(1, 0, first, second) + if a.Unreachable { + t.Fatal("one range is still free; the group is reachable") + } + if !has(a.Warnings, "match", "10.0.0.128/25 is already claimed by \"a\"") { + t.Errorf("warnings = %+v", a.Warnings) + } +} + +func TestAnalyzeReverseNameAndTagShadowing(t *testing.T) { + first := named("a", "allow", config.Match{PTRSuffixes: []string{"lan.example."}, Tags: []string{"laptop"}}) + second := named("b", "allow", config.Match{PTRSuffixes: []string{".lab.lan.example."}, Tags: []string{"Laptop"}}) + if a := analyzeAt(1, 0, first, second); !a.Unreachable { + t.Errorf("a suffix under an earlier suffix plus the same tag is fully claimed: %+v", a) + } +} + +func TestAnalyzeGroupAfterACatchAllIsUnreachable(t *testing.T) { + all := named("all", "allow", config.Match{Any: true, CIDRs: []string{"10.0.0.0/8"}}) + g := named("g", "allow", config.Match{CIDRs: []string{"192.168.0.0/24"}}) + a := analyzeAt(1, 0, all, g) + if !a.Unreachable { + t.Error("nothing after a match-any group can be reached") + } + if b := analyzeAt(0, 0, all, g); !has(b.Warnings, "match.any", "every client") { + t.Errorf("warnings = %+v, want the match-any warning", b.Warnings) + } +} + +func TestAnalyzeCIDRHygiene(t *testing.T) { + a := analyzeAt(0, 0, named("g", "allow", config.Match{CIDRs: []string{"192.168.1.5/24", "0.0.0.0/0"}})) + if !has(a.Warnings, "match.cidrs", "host bits") || !has(a.Warnings, "match.cidrs", "very wide") { + t.Errorf("warnings = %+v", a.Warnings) + } +} + +func TestAnalyzeDomainPatternMistakes(t *testing.T) { + g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}}) + g.AllowDomains = []string{"https://example.com/a", ".example.org", "ads.*.com", "*.fine.example", "*"} + g.DenyDomains = []string{"*.fine.example"} + a := analyzeAt(0, 0, g) + for _, want := range []string{"looks like a URL", "starts with a dot", "not supported", "allows every name", "deny wins"} { + if !has(a.Warnings, "allow_domains", want) { + t.Errorf("missing allow_domains warning containing %q in %+v", want, a.Warnings) + } + } +} + +func TestAnalyzeTagWithADotNeverMatches(t *testing.T) { + a := analyzeAt(0, 0, named("g", "allow", config.Match{Tags: []string{"lan.example"}})) + if !has(a.Warnings, "match.tags", "single label") { + t.Errorf("warnings = %+v", a.Warnings) + } +} + +func TestAnalyzeUnknownUpstreams(t *testing.T) { + g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}}) + g.Upstreams = []string{"9.9.9.9:53"} + a := Analyze(Context{Groups: []config.Group{g}, Upstreams: []string{"1.1.1.1:53"}}) + if !has(a.Warnings, "upstreams", "uses ALL upstreams") { + t.Errorf("warnings = %+v", a.Warnings) + } + g.Upstreams = []string{"1.1.1.1:53"} + if a := Analyze(Context{Groups: []config.Group{g}, Upstreams: []string{"1.1.1.1:53"}}); has(a.Warnings, "upstreams", "") { + t.Errorf("a configured upstream warned: %+v", a.Warnings) + } + if a := Analyze(Context{Groups: []config.Group{g}}); has(a.Warnings, "upstreams", "") { + t.Errorf("unknown upstream list must skip the check: %+v", a.Warnings) + } +} + +func TestAnalyzeToleratesABadIndex(t *testing.T) { + if a := Analyze(Context{Index: 3}); len(a.Summary) != 0 { + t.Errorf("summary = %v", a.Summary) + } +} diff --git a/internal/api/zones.go b/internal/api/zones.go index ce9adc1..c19b10f 100644 --- a/internal/api/zones.go +++ b/internal/api/zones.go @@ -33,8 +33,9 @@ type ZonesResponse struct { Zones []localzone.Zone `json:"zones"` // Files are every master file the zones were compiled from. Files []string `json:"files"` - // ManagedDir is the directory the API may rewrite, "" when the console - // has no managed zone directory configured (blocklist.managed_zones). + // ManagedDir says where a zone edit is stored, for the console to show: + // the console fills it with the Kubernetes namespace its DNSZone objects + // live in. The resolver itself leaves it empty, because it cannot write. ManagedDir string `json:"managed_dir"` // Writable is whether a write to ManagedDir would succeed. Writable bool `json:"writable"` diff --git a/internal/console/console.go b/internal/console/console.go index 3aadd2a..ba1abd3 100644 --- a/internal/console/console.go +++ b/internal/console/console.go @@ -100,6 +100,7 @@ func (c *Console) Handler() (http.Handler, error) { // Reads that combine the cluster's objects with the resolver's view. mux.Handle("GET "+base+"/zones", c.api.Protect(http.HandlerFunc(c.handleZones), false)) mux.Handle("GET "+base+"/clients", c.api.Protect(http.HandlerFunc(c.handleClients), false)) + mux.Handle("POST "+base+"/clients/validate", c.api.Protect(http.HandlerFunc(c.handleClientsValidate), false)) mux.Handle("GET "+base+"/blocklist-sources", c.api.Protect(http.HandlerFunc(c.handleListSources), false)) mux.Handle("PUT "+base+"/blocklist-sources/{name}", c.api.Protect(http.HandlerFunc(c.handleListSourcePut), true)) mux.Handle("DELETE "+base+"/blocklist-sources/{name}", c.api.Protect(http.HandlerFunc(c.handleListSourceDelete), true)) diff --git a/internal/console/hooks.go b/internal/console/hooks.go index 10df747..1246693 100644 --- a/internal/console/hooks.go +++ b/internal/console/hooks.go @@ -56,7 +56,8 @@ type clientsView struct { config.Group Source string `json:"source"` } `json:"groups"` - DefaultPolicy string `json:"default_policy"` + DefaultPolicy string `json:"default_policy"` + DefaultRateLimit int `json:"default_rate_limit"` } // sameGroup compares two groups by what they configure, so an empty list and a diff --git a/internal/console/validate.go b/internal/console/validate.go new file mode 100644 index 0000000..53d8dc9 --- /dev/null +++ b/internal/console/validate.go @@ -0,0 +1,193 @@ +package console + +import ( + "context" + "errors" + "net/http" + "net/netip" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/acl" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/api" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/kubeedit" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/kubesource" +) + +// validateRequest is a ClientGroup as the editor has it, before it is saved. +type validateRequest struct { + Group config.Group `json:"group"` + // TestIP, when set, says which group an ordinary client at that address + // would land in once this group is saved. + TestIP string `json:"test_ip,omitempty"` +} + +// validateResponse separates what blocks a save (errors) from what is legal +// but probably unintended (warnings) and from plain information (notes). +type validateResponse struct { + OK bool `json:"ok"` + Errors []acl.Finding `json:"errors"` + Warnings []acl.Finding `json:"warnings"` + Notes []acl.Finding `json:"notes"` + Summary []string `json:"summary"` + Unreachable bool `json:"unreachable"` + Position int `json:"position"` + Total int `json:"total"` + Test *validateTestIP `json:"test,omitempty"` +} + +type validateTestIP struct { + IP string `json:"ip"` + Valid bool `json:"valid"` + Group string `json:"group"` + Default bool `json:"default"` + Note string `json:"note"` +} + +// handleClientsValidate checks a group without saving it. It applies the same +// checks the save applies (kubesource.CheckGroup, the object-name rule) and +// then says what the group would do and what is probably wrong with it, in the +// context of the groups that already exist. A group that is not already saved +// is placed last, which is where a save puts it. +func (c *Console) handleClientsValidate(w http.ResponseWriter, r *http.Request) { + var in validateRequest + if err := api.DecodeJSON(r, &in); err != nil { + api.WriteError(w, http.StatusBadRequest, "malformed request body") + return + } + current, meta, err := c.currentGroups(r.Context()) + if err != nil { + api.WriteError(w, http.StatusBadGateway, err.Error()) + return + } + + out := validateResponse{Errors: []acl.Finding{}, Warnings: []acl.Finding{}, Notes: []acl.Finding{}, Summary: []string{}} + name := in.Group.Name + if err := kubeedit.CheckGroupName(name); err != nil { + out.Errors = append(out.Errors, acl.Finding{Field: "name", Message: err.Error()}) + } + spec := kubeedit.SpecFromGroup(in.Group, 0) + for _, fe := range kubesource.CheckGroupAll(name, spec) { + out.Errors = append(out.Errors, acl.Finding{Field: fe.Field, Message: fe.Msg}) + } + if have, ok := meta.fileGroups[name]; ok && !sameGroup(have, in.Group) { + out.Errors = append(out.Errors, acl.Finding{Field: "name", Message: "A group with this name is defined in the resolver's config file and cannot be changed here. Pick another name."}) + } + + groups, idx := place(current, in.Group) + out.Position, out.Total = idx+1, len(groups) + out.OK = len(out.Errors) == 0 + + // The analysis is useful even for a group that cannot be saved yet (it + // explains what the typed-so-far group would do), but it only makes sense + // once the group selects something. + an := acl.Analyze(acl.Context{ + Groups: groups, Index: idx, + DefaultRateLimit: meta.defaultRateLimit, DefaultPolicy: meta.defaultPolicy, + Upstreams: meta.upstreams, + }) + out.Summary, out.Warnings, out.Notes, out.Unreachable = an.Summary, an.Warnings, an.Notes, an.Unreachable + + if in.TestIP != "" { + out.Test = testIP(in.TestIP, groups, meta.defaultPolicy) + } + api.WriteJSON(w, http.StatusOK, out) +} + +// place puts g in the list: in its own slot when a group of that name exists, +// otherwise last, which is where a save adds it. +func place(list []config.Group, g config.Group) ([]config.Group, int) { + out := append([]config.Group(nil), list...) + for i := range out { + if out[i].Name == g.Name && g.Name != "" { + out[i] = g + return out, i + } + } + return append(out, g), len(out) +} + +func testIP(raw string, groups []config.Group, defaultPolicy string) *validateTestIP { + res := &validateTestIP{IP: raw} + ip, err := netip.ParseAddr(raw) + if err != nil { + res.Note = "Not an IP address." + return res + } + res.Valid = true + d := acl.Compile(config.ACL{DefaultPolicy: defaultPolicy, Groups: groups}).Match(acl.Client{IP: ip}) + res.Group, res.Default = d.Group, d.Default + switch { + case d.Default: + res.Note = "No group matches this address by IP; it gets the default policy (" + string(d.Policy) + ")." + default: + res.Note = "Matched on address. Clients can also match by reverse-DNS name or tag, which cannot be tested here." + } + return res +} + +type groupsMeta struct { + fileGroups map[string]config.Group + defaultPolicy string + defaultRateLimit int + upstreams []string +} + +// currentGroups is the evaluation order the resolver will use, made consistent +// with the cluster: the same merge handleClients does, as typed groups. +func (c *Console) currentGroups(ctx context.Context) ([]config.Group, groupsMeta, error) { + meta := groupsMeta{fileGroups: map[string]config.Group{}} + var view clientsView + if err := c.opt.Runtime.Get(ctx, "/api/clients", &view); err != nil { + return nil, meta, err + } + objs, err := c.opt.Editor.Groups(ctx) + if err != nil { + return nil, meta, errors.New("cluster: " + err.Error()) + } + meta.defaultPolicy, meta.defaultRateLimit = view.DefaultPolicy, view.DefaultRateLimit + + inCluster := map[string]bool{} + for _, o := range objs { + inCluster[o.Name] = true + } + var out []config.Group + seen := map[string]bool{} + for _, g := range view.Groups { + switch g.Source { + case "history": + continue + case "kubernetes": + if !inCluster[g.Name] { + continue + } + default: + meta.fileGroups[g.Name] = g.Group + } + seen[g.Name] = true + out = append(out, g.Group) + } + for _, o := range objs { + if !seen[o.Name] { + g := config.Group{Name: o.Name, Comment: o.Spec.Comment, Match: o.Spec.Match, Policy: o.Spec.Policy, + AllowDomains: o.Spec.AllowDomains, DenyDomains: o.Spec.DenyDomains, SkipBlocklist: o.Spec.SkipBlocklist, + SkipCache: o.Spec.SkipCache, RateLimit: o.Spec.RateLimit, MaxConcurrent: o.Spec.MaxConcurrent, + LogQueries: o.Spec.LogQueries, Upstreams: o.Spec.Upstreams} + out = append(out, g) + } + } + + // Upstreams are a nicety for one warning; an unreachable or older resolver + // must not make validation fail. + var ups struct { + Servers []struct { + Addr string `json:"addr"` + } `json:"servers"` + } + if err := c.opt.Runtime.Get(ctx, "/api/upstreams", &ups); err == nil { + meta.upstreams = []string{} + for _, s := range ups.Servers { + meta.upstreams = append(meta.upstreams, s.Addr) + } + } + return out, meta, nil +} diff --git a/internal/console/validate_test.go b/internal/console/validate_test.go new file mode 100644 index 0000000..8df4f34 --- /dev/null +++ b/internal/console/validate_test.go @@ -0,0 +1,181 @@ +package console + +import ( + "context" + "encoding/json" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynfake "k8s.io/client-go/dynamic/fake" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/kubeedit" + "git.k8s-git-k6n.net/mats/fancy-dns/internal/kubesource" +) + +// validator builds a console whose cluster holds the given groups and whose +// resolver reports one group of its own, defined in its config file. +func validator(t *testing.T, saved ...config.Group) *Console { + t.Helper() + cl := dynfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), + map[schema.GroupVersionResource]string{ + kubesource.ZoneGVR: "DNSZoneList", kubesource.GroupGVR: "ClientGroupList", + kubesource.ListGVR: "BlockListList", kubesource.OverrideGVR: "AddressOverrideList", + kubesource.RuleGVR: "DomainRuleList", + }) + ed := kubeedit.New(cl, "dns") + if len(saved) > 0 { + if _, err := ed.SyncGroups(context.Background(), saved, nil, "test"); err != nil { + t.Fatal(err) + } + } + + resolver := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/clients": + groups := []map[string]any{{ + "name": "from-file", "policy": "allow", "source": "", + "match": map[string]any{"cidrs": []string{"172.16.0.0/12"}}, + }} + for _, g := range saved { + groups = append(groups, map[string]any{"name": g.Name, "policy": g.Policy, "source": "kubernetes", "match": g.Match}) + } + json.NewEncoder(w).Encode(map[string]any{"groups": groups, "default_policy": "allow", "default_rate_limit": 50}) + case "/api/upstreams": + json.NewEncoder(w).Encode(map[string]any{"servers": []map[string]any{{"addr": "1.1.1.1:53"}}}) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(resolver.Close) + rt, err := NewRuntime(resolver.URL, "") + if err != nil { + t.Fatal(err) + } + return &Console{opt: Options{Runtime: rt, Editor: ed}, log: slog.Default()} +} + +func validate(t *testing.T, c *Console, body string) validateResponse { + t.Helper() + rec := httptest.NewRecorder() + c.handleClientsValidate(rec, httptest.NewRequest(http.MethodPost, "/api/clients/validate", strings.NewReader(body))) + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body) + } + var out validateResponse + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatalf("%v: %s", err, rec.Body) + } + return out +} + +func hasField(list []string, f string) bool { + for _, s := range list { + if s == f { + return true + } + } + return false +} + +func errFields(r validateResponse) []string { + var out []string + for _, e := range r.Errors { + out = append(out, e.Field) + } + return out +} + +func TestValidateReportsEveryErrorAtOnce(t *testing.T) { + c := validator(t) + r := validate(t, c, `{"group":{"name":"Bad Name","policy":"sometimes","match":{"cidrs":["10.0.0.0/33"]},"rate_limit":-1}}`) + if r.OK { + t.Fatal("a group with four problems validated") + } + for _, want := range []string{"name", "policy", "match.cidrs", "rate_limit"} { + if !hasField(errFields(r), want) { + t.Errorf("no error for %q; got %+v", want, r.Errors) + } + } +} + +func TestValidateAcceptsAGoodGroupAndExplainsIt(t *testing.T) { + c := validator(t) + r := validate(t, c, `{"group":{"name":"kids","policy":"allow","match":{"cidrs":["10.0.5.0/24"]},"allow_domains":["*.school.example"]}}`) + if !r.OK || len(r.Errors) != 0 { + t.Fatalf("errors: %+v", r.Errors) + } + if r.Position != 2 || r.Total != 2 { + t.Errorf("a new group goes last: position %d of %d", r.Position, r.Total) + } + if len(r.Summary) < 3 { + t.Errorf("summary = %v", r.Summary) + } + // 0 rate limit under a default of 50 is the surprise worth flagging. + found := false + for _, w := range r.Warnings { + found = found || w.Field == "rate_limit" + } + if !found { + t.Errorf("warnings = %+v, want the rate-limit exemption", r.Warnings) + } +} + +func TestValidateEditingKeepsThePosition(t *testing.T) { + a := config.Group{Name: "a", Policy: "allow", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}} + b := config.Group{Name: "b", Policy: "allow", Match: config.Match{CIDRs: []string{"192.168.0.0/16"}}} + c := validator(t, a, b) + r := validate(t, c, `{"group":{"name":"a","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}}}`) + if r.Position != 2 || r.Total != 3 { // from-file, a, b + t.Errorf("position %d of %d", r.Position, r.Total) + } +} + +func TestValidateRefusesAFileGroupName(t *testing.T) { + c := validator(t) + r := validate(t, c, `{"group":{"name":"from-file","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}}}`) + if r.OK || !hasField(errFields(r), "name") { + t.Errorf("a group the resolver's file owns validated: %+v", r) + } +} + +func TestValidateFlagsAGroupBuriedUnderAWiderOne(t *testing.T) { + wide := config.Group{Name: "wide", Policy: "allow", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}} + c := validator(t, wide) + r := validate(t, c, `{"group":{"name":"narrow","policy":"deny","match":{"cidrs":["10.1.0.0/16"]}}}`) + if !r.OK || !r.Unreachable { + t.Errorf("ok=%v unreachable=%v warnings=%+v", r.OK, r.Unreachable, r.Warnings) + } +} + +func TestValidateTestIP(t *testing.T) { + g := config.Group{Name: "lan", Policy: "deny", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}} + c := validator(t, g) + r := validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"10.1.2.3"}`) + if r.Test == nil || r.Test.Group != "lan" || r.Test.Default { + t.Errorf("test = %+v", r.Test) + } + r = validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"8.8.8.8"}`) + if r.Test == nil || !r.Test.Default { + t.Errorf("test = %+v", r.Test) + } + r = validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"nope"}`) + if r.Test == nil || r.Test.Valid { + t.Errorf("test = %+v", r.Test) + } +} + +func TestValidateUnreachableResolverIsABadGateway(t *testing.T) { + c := validator(t) + c.opt.Runtime, _ = NewRuntime("http://127.0.0.1:1", "") + rec := httptest.NewRecorder() + c.handleClientsValidate(rec, httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"group":{"name":"x"}}`))) + if rec.Code != http.StatusBadGateway { + t.Errorf("status %d", rec.Code) + } +} diff --git a/internal/kubeedit/groups.go b/internal/kubeedit/groups.go index 7cfbb25..c5f9e26 100644 --- a/internal/kubeedit/groups.go +++ b/internal/kubeedit/groups.go @@ -129,3 +129,12 @@ func (e *Editor) SyncGroups(ctx context.Context, desired []config.Group, foreign } return res, nil } + +// CheckGroupName reports why name cannot be a ClientGroup's object name, so a +// validation endpoint can say it before a save does. +func CheckGroupName(name string) error { + if name == "" { + return fmt.Errorf("a group needs a name") + } + return checkName("group", name) +} diff --git a/internal/kubesource/convert.go b/internal/kubesource/convert.go index d9329ac..0bb9d79 100644 --- a/internal/kubesource/convert.go +++ b/internal/kubesource/convert.go @@ -46,14 +46,44 @@ func convertGroup(u *unstructured.Unstructured) (group, error) { return groupFromSpec(u.GetName(), s) } +// FieldError is a validation failure tied to one field of a ClientGroup, so an +// editor can show the message next to the input that caused it. Field is the +// JSON path inside the group ("policy", "match.cidrs", "rate_limit"); "name" +// and "match" are used for the whole-object cases. +type FieldError struct { + Field string + Msg string +} + +func (e FieldError) Error() string { return e.Msg } + // CheckGroup validates a ClientGroup spec the way the runtime will, so an -// editor can refuse a bad one before it is written. +// editor can refuse a bad one before it is written. It reports the first +// problem; CheckGroupAll reports every one. func CheckGroup(name string, s ClientGroupSpec) (config.Group, error) { - g, err := groupFromSpec(name, s) - return g.cfg, err + g, errs := checkGroup(name, s) + if len(errs) > 0 { + return g.cfg, errs[0] + } + return g.cfg, nil +} + +// CheckGroupAll is CheckGroup for an editor that wants to show every problem +// at once instead of one per save. +func CheckGroupAll(name string, s ClientGroupSpec) []FieldError { + _, errs := checkGroup(name, s) + return errs } func groupFromSpec(name string, s ClientGroupSpec) (group, error) { + g, errs := checkGroup(name, s) + if len(errs) > 0 { + return group{}, errs[0] + } + return g, nil +} + +func checkGroup(name string, s ClientGroupSpec) (group, []FieldError) { g := config.Group{ Name: name, Comment: s.Comment, @@ -68,14 +98,18 @@ func groupFromSpec(name string, s ClientGroupSpec) (group, error) { LogQueries: s.LogQueries, Upstreams: s.Upstreams, } + var errs []FieldError + bad := func(field, format string, args ...any) { + errs = append(errs, FieldError{Field: field, Msg: fmt.Sprintf(format, args...)}) + } switch g.Policy { case "allow", "deny", "block_only": default: - return group{}, fmt.Errorf("policy must be allow, deny or block_only (got %q)", g.Policy) + bad("policy", "policy must be allow, deny or block_only (got %q)", g.Policy) } for _, cidr := range g.Match.CIDRs { if _, _, err := net.ParseCIDR(cidr); err != nil { - return group{}, fmt.Errorf("bad CIDR %q", cidr) + bad("match.cidrs", "bad CIDR %q", cidr) } } // Copy before normalising: the slice belongs to the decoded object. @@ -85,18 +119,22 @@ func groupFromSpec(name string, s ClientGroupSpec) (group, error) { g.Match.PTRSuffixes[i] = s + "." } } - if !g.Match.Any && len(g.Match.CIDRs) == 0 && len(g.Match.PTRSuffixes) == 0 && len(g.Match.Tags) == 0 { - return group{}, fmt.Errorf("match selects no clients: set cidrs, ptr_suffixes, tags or any") - } - if g.Match.Any && len(g.Match.CIDRs) == 0 && len(g.Match.PTRSuffixes) == 0 && len(g.Match.Tags) == 0 { + selectsSomething := len(g.Match.CIDRs) > 0 || len(g.Match.PTRSuffixes) > 0 || len(g.Match.Tags) > 0 + switch { + case !g.Match.Any && !selectsSomething: + bad("match", "match selects no clients: set cidrs, ptr_suffixes, tags or any") + case g.Match.Any && !selectsSomething: // config.Validate refuses this shape for every group. Refusing it here // rejects one object instead of the whole merged configuration. - return group{}, fmt.Errorf("match.any alone is not accepted; use a catch-all in the YAML acl.groups") + bad("match.any", "match.any alone is not accepted; use a catch-all in the YAML acl.groups") } - if g.RateLimit < 0 || g.MaxConcurrent < 0 { - return group{}, fmt.Errorf("rate_limit and max_concurrent must not be negative") + if g.RateLimit < 0 { + bad("rate_limit", "rate_limit must not be negative") } - return group{cfg: g, priority: s.Priority}, nil + if g.MaxConcurrent < 0 { + bad("max_concurrent", "max_concurrent must not be negative") + } + return group{cfg: g, priority: s.Priority}, errs } func convertList(u *unstructured.Unstructured) (config.ListSource, error) { diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index f7c931d..c78931a 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -11,7 +11,9 @@ import type { AddDomainResponse, BlocklistsResponse, CacheResponse, + ClientGroupInput, ClientsPayload, + GroupValidation, ClientsResponse, DomainCheckResponse, DomainFilters, @@ -370,6 +372,16 @@ export function putClients(payload: ClientsPayload): Promise { }) } +export function validateClientGroup( + group: ClientGroupInput, + testIp?: string, +): Promise { + return apiFetch('/api/clients/validate', { + method: 'POST', + body: JSON.stringify({ group, ...(testIp ? { test_ip: testIp } : {}) }), + }) +} + export function getUpstreams(): Promise { return apiFetch('/api/upstreams') } diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index 389c527..61d7769 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -345,6 +345,41 @@ export interface ClientsPayload { default_policy: DefaultPolicy } +// --- POST /api/clients/validate --------------------------------------------- + +/** One observation about a group, tied to the field it concerns. */ +export interface GroupFinding { + /** A JSON path inside the group: "policy", "match.cidrs", "rate_limit". */ + field: string + message: string +} + +/** + * What the server says about a group before it is saved. `errors` block the + * save; `warnings` are legal settings that probably are not what was meant; + * `notes` are facts worth knowing. + */ +export interface GroupValidation { + ok: boolean + errors: GroupFinding[] + warnings: GroupFinding[] + notes: GroupFinding[] + /** The group in plain English, one sentence per entry. */ + summary: string[] + /** No client can ever land in the group: the ones above it claim them all. */ + unreachable: boolean + /** 1-based place in the evaluation order, and the number of groups. */ + position: number + total: number + test?: { + ip: string + valid: boolean + group: string + default: boolean + note: string + } +} + // --- GET /api/upstreams ---------------------------------------------------- /** diff --git a/web/src/pages/Clients.tsx b/web/src/pages/Clients.tsx index c4bdf97..7dae02f 100644 --- a/web/src/pages/Clients.tsx +++ b/web/src/pages/Clients.tsx @@ -7,12 +7,21 @@ * user sees what is wrong next to the input rather than in a toast alone. */ -import { useMemo, useState } from 'react' -import { Copy, Pencil, Plus, Trash2, Users } from 'lucide-react' +import { useEffect, useMemo, useRef, useState } from 'react' +import type { ReactNode } from 'react' +import { AlertTriangle, Check, Copy, Info, Pencil, Plus, Trash2, Users } from 'lucide-react' -import { ApiError, getClients, putClients } from '../lib/api' +import { ApiError, getClients, putClients, validateClientGroup } from '../lib/api' import { useWhoami } from '../lib/session.ts' -import type { ClientGroup, ClientGroupInput, ClientsPayload, DefaultPolicy, GroupPolicy } from '../lib/types' +import type { + ClientGroup, + ClientGroupInput, + ClientsPayload, + DefaultPolicy, + GroupFinding, + GroupPolicy, + GroupValidation, +} from '../lib/types' import { useApi, useAsyncAction } from '../hooks/useApi' import { fromLines, toLines, formatNumber } from '../lib/format' import { @@ -45,14 +54,63 @@ function notify( } const POLICIES = [ - { value: 'allow', label: 'Allow — resolve everything' }, - { value: 'block_only', label: 'Block only — no recursion, blocklists apply' }, - { value: 'deny', label: 'Deny — refuse everything' }, + { value: 'allow', label: 'Allow — resolve normally, blocklists apply' }, + { value: 'deny', label: 'Deny — refuse every query from these clients' }, +] + +/** + * block_only does exactly what allow does, so it is not offered. A group that + * already has it keeps showing it, rather than silently changing on edit. + */ +const LEGACY_POLICY = { value: 'block_only', label: 'Block only (old setting, same as Allow)' } + +function policyOptions(current: string) { + return current === 'block_only' ? [...POLICIES, LEGACY_POLICY] : POLICIES +} + +/** + * Starting points for the common cases. Each fills in the behaviour and leaves + * the "who" (name, addresses) to the user, which is the part only they know. + */ +const TEMPLATES: { id: string; title: string; text: string; patch: Partial }[] = [ + { + id: 'trusted', + title: 'Trusted devices', + text: 'Never blocked: ads and trackers resolve normally.', + patch: { policy: 'allow', skip_blocklist: true, comment: 'Trusted devices: blocklists are skipped' }, + }, + { + id: 'restricted', + title: 'Only some sites', + text: 'Clients can look up only the sites you list. Everything else is blocked.', + patch: { + policy: 'allow', + allow_domains: '*.example.com', + comment: 'Allowlist: only the listed sites resolve', + }, + }, + { + id: 'guests', + title: 'Guests', + text: 'Normal filtering, with a cap on queries per second.', + patch: { policy: 'allow', rate_limit: '20', comment: 'Guests: rate limited' }, + }, + { + id: 'cutoff', + title: 'Cut a device off', + text: 'Refuses every query from these clients.', + patch: { policy: 'deny', comment: 'Cut off: all queries refused' }, + }, + { + id: 'debug', + title: 'Debug a device', + text: 'Logs every query and bypasses the cache so each lookup is fresh.', + patch: { policy: 'allow', log_queries: true, skip_cache: true, comment: 'Debugging: logged, uncached' }, + }, ] const DEFAULT_POLICIES = [ { value: 'allow', label: 'Allow' }, - { value: 'block_only', label: 'Block only' }, { value: 'deny', label: 'Deny' }, ] @@ -238,6 +296,53 @@ export default function Clients() { const effectiveDefault = defaultDraft ?? defaultPolicy ?? String(data?.default_policy ?? 'allow') + // Live check of the draft against the server: the same validation a save + // applies, plus a plain-English reading of what the group will do. + const [check, setCheck] = useState(null) + const [checkFailed, setCheckFailed] = useState(null) + const [testIp, setTestIp] = useState('') + const pristine = useRef('') + const checkSeq = useRef(0) + + useEffect(() => { + if (!editorOpen) return + const touched = JSON.stringify(draft) !== pristine.current + // Do not shout about an empty name at someone who has just opened the form. + if (!touched && draft.name.trim() === '') return + const seq = ++checkSeq.current + const handle = window.setTimeout(() => { + validateClientGroup(toGroup(draft), testIp.trim() || undefined) + .then((res) => { + if (seq !== checkSeq.current) return + setCheck(res) + setCheckFailed(null) + }) + .catch((err: unknown) => { + if (seq !== checkSeq.current) return + setCheckFailed(err instanceof Error ? err.message : 'validation unavailable') + }) + }, 350) + return () => window.clearTimeout(handle) + }, [draft, editorOpen, testIp]) + + const openDraft = (next: GroupDraft, index: number | null): void => { + pristine.current = JSON.stringify(next) + checkSeq.current++ + setCheck(null) + setCheckFailed(null) + setTestIp('') + setEditingIndex(index) + setDraft(next) + setFieldErrors([]) + setEditorOpen(true) + } + + /** Errors the server found on one field of the draft, ready for an input. */ + const checkError = (...fields: string[]): string | undefined => { + const hit = check?.errors.filter((f) => fields.includes(f.field)) + return hit && hit.length > 0 ? hit.map((f) => f.message).join(' ') : undefined + } + const save = useAsyncAction(async (payload: ClientsPayload) => { try { return await putClients(payload) @@ -282,30 +387,18 @@ export default function Clients() { return true } - const openNew = (): void => { - setEditingIndex(null) - setDraft(EMPTY_DRAFT) - setFieldErrors([]) - setEditorOpen(true) - } + const openNew = (): void => openDraft(EMPTY_DRAFT, null) const openEdit = (index: number): void => { const group = groups[index] - if (!group) return - setEditingIndex(index) - setDraft(fromGroup(group)) - setFieldErrors([]) - setEditorOpen(true) + if (group) openDraft(fromGroup(group), index) } const duplicate = (index: number): void => { const group = groups[index] if (!group) return const copy = fromGroup(group) - setEditingIndex(null) - setDraft({ ...copy, name: `${copy.name}-copy` }) - setFieldErrors([]) - setEditorOpen(true) + openDraft({ ...copy, name: `${copy.name}-copy` }, null) } const saveGroup = async (): Promise => { @@ -314,6 +407,10 @@ export default function Clients() { notify(toast, 'fail', 'a group needs a name') return } + if (check && !check.ok) { + notify(toast, 'fail', 'fix the highlighted problems first') + return + } const group = toGroup(draft) const next = groups.map(toInput) if (editingIndex !== null && next[editingIndex]) { @@ -354,7 +451,7 @@ export default function Clients() {
void saveDefault(e.target.value)} - options={DEFAULT_POLICIES} + options={effectiveDefault === 'block_only' ? [...DEFAULT_POLICIES, LEGACY_POLICY] : DEFAULT_POLICIES} />
@@ -424,6 +521,7 @@ export default function Clients() { {matchSummary(group)}

+ {(group.allow_domains?.length ?? 0) > 0 ? allowlist only : null} {group.skip_blocklist ? skip blocklist : null} {group.skip_cache ? skip cache : null} {group.log_queries ? log : null} @@ -457,153 +555,215 @@ export default function Clients() { open={editorOpen} onClose={() => setEditorOpen(false)} title={editingIndex === null ? 'New client group' : `Edit ${draft.name}`} - description="Groups are matched top to bottom; the first match wins." + description="A group is a set of devices that share rules. Each query is checked against the groups from the top; the first group that matches the device decides." size="lg" footer={ -
+
+ {check && !check.ok ? ( + Fix the problems marked in red to save. + ) : null} -
} > -
-
+
+ {editingIndex === null ? ( +
+

Start from

+
+ {TEMPLATES.map((t) => ( + + ))} +
+
+ ) : null} + + +
+ setDraft((d) => ({ ...d, name: e.target.value }))} + error={checkError('name') ?? fieldError('name')} + autoComplete="off" + spellCheck={false} + /> + setDraft((d) => ({ ...d, comment: e.target.value }))} + /> +
+ + + +