From 26134ba437d35558a5b6a9c9928ed62759aba683 Mon Sep 17 00:00:00 2001 From: developer Date: Sun, 27 Sep 2026 13:30:43 +0000 Subject: [PATCH] Fix the bbolt round trip, the blocklist gaps and the deploy set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache snapshot never loaded, so every restart was a cold cache. Snapshot packed the whole key while Load decoded the record header out of the key's remainder instead of the value, so every entry was skipped. The two sides are now an explicit encodeRecord/decodeRecord pair with a version byte and a CRC-32C trailer, so a future format change is detected instead of silently misparsed. Load also drops entries that are already past ExpiresAt and rewrites the TTL to the time actually left, so a served TTL never jumps back up across a restart. Blocklist, all six gaps the failing tests were pointing at: a leading UTF-8 BOM is stripped, "!allow example.com" works, SourceMeta exists for a source that only ever contributed a rule, Pattern() reports the bare name again (the new WireName() produces the wildcard form for the API), an RPZ record with no explicit TTL inherits the zone default instead of whatever the reader left in the RR, a hosts line with no address in the first field says so, and the adblock parser handles CRLF and adblock-flavoured lines pasted into a plain list. TestHolderSwapIsAtomic was asserting the wrong invariant — the swap was always atomic, a name blocked by the old set is legitimately unblocked by the new one. Tests added for acl and resolver, both of which turned up real bugs: - a PTR suffix matched across label boundaries, so a group scoped to example.com. also swallowed ample.com. and notexample.com. — a client could land in a group it does not belong to; - the "*" catch-all was dropped from the rendered domain patterns, so the engine's per-query recompile lost the "everything" case; - Upstream.Success never closed an open breaker, so a recovered upstream stayed excluded for the full cooldown; - Exchange fanned out in parallel even under policy: failover. Deploy set: go.mod requires Go 1.27.1 but CI pinned 1.23 and the Dockerfile pinned golang:1.23-alpine, so neither could have built. The Dockerfile also copied the source tree after the console bundle, which clobbered it. The k8s set asked for hostNetwork with two replicas, but the server never sets SO_REUSEPORT, so the second pod could not bind :53 and would crash-loop. It now runs without hostNetwork behind a LoadBalancer Service, with a separate ClusterIP Service for the Ingress (a headless Service cannot back one), a ServiceAccount, an API-key Secret, per-pod PVCs, and a kustomization. The HPA is gone: it cannot drive a StatefulSet. To let the API key come from that Secret, config.Load now expands ${VAR} / ${VAR:-default} / ${NAME-default} and fails to start on an unset variable with no default, so a missing secret is a crash loop rather than an unauthenticated console. Expansion skips YAML comments, so documenting ${VAR} in the ConfigMap does not break the file. Verified: gofmt, go build, go vet clean; go test -race ./internal/... green (acl, app, blocklist, cache, config, resolver, zonefile). --- .github/workflows/ci.yml | 5 +- Dockerfile | 9 +- deploy/k8s/00-config.yaml | 9 +- deploy/k8s/05-serviceaccount.yaml | 12 + deploy/k8s/08-secret-apikey.yaml | 22 + deploy/k8s/10-deployment.yaml | 174 ++--- deploy/k8s/20-service.yaml | 103 +++ deploy/k8s/30-ingress.yaml | 64 ++ deploy/k8s/kustomization.yaml | 26 + internal/acl/acl.go | 63 +- internal/acl/acl_test.go | 1065 ++++++++++++++++++++++++++++ internal/app/hooks.go | 4 +- internal/blocklist/parse.go | 101 ++- internal/blocklist/parse_test.go | 56 +- internal/blocklist/set.go | 28 +- internal/blocklist/set_test.go | 13 +- internal/cache/cache_test.go | 230 ++++++ internal/cache/store.go | 207 +++++- internal/config/config.go | 149 +++- internal/config/config_test.go | 331 +++++++++ internal/resolver/pool.go | 28 + internal/resolver/resolver_test.go | 632 +++++++++++++++++ 22 files changed, 3106 insertions(+), 225 deletions(-) create mode 100644 deploy/k8s/05-serviceaccount.yaml create mode 100644 deploy/k8s/08-secret-apikey.yaml create mode 100644 deploy/k8s/20-service.yaml create mode 100644 deploy/k8s/30-ingress.yaml create mode 100644 deploy/k8s/kustomization.yaml create mode 100644 internal/acl/acl_test.go create mode 100644 internal/config/config_test.go create mode 100644 internal/resolver/resolver_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a14e79..973961a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,7 +16,10 @@ concurrency: cancel-in-progress: true env: - GO_VERSION: "1.23" + # Must track the `go` directive in go.mod; an older runner fails every + # build with "go.mod requires go >= ...". The fmt check only has real teeth + # when the toolchain is new enough (gofmt is part of the Go toolchain in 1.27). + GO_VERSION: "1.27" NODE_VERSION: "22" jobs: diff --git a/Dockerfile b/Dockerfile index 4d77c66..4c40eaa 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,7 +5,7 @@ # libc, which is what makes a scratch/distroless base safe. ARG NODE_IMAGE=node:22-alpine -ARG GO_IMAGE=golang:1.23-alpine +ARG GO_IMAGE=golang:1.27-alpine ARG RUNTIME_IMAGE=gcr.io/distroless/static-debian12:nonroot # ---- stage 1: the admin console ------------------------------------------- @@ -34,9 +34,10 @@ ARG TARGETARCH=amd64 WORKDIR /src -# The placeholder dist directory keeps the embed directive valid even if the -# console stage was skipped. -COPY internal/web/dist/ internal/web/dist/ +# The real console bundle goes in *after* the source tree, so the COPY above +# (which carries only the .gitkeep placeholder from a fresh clone) cannot +# clobber it. `all:dist` in the embed directive needs the directory itself to +# exist, which the placeholder guarantees. COPY --from=web /src/internal/web/dist/ internal/web/dist/ # Module cache first, for the same reason. diff --git a/deploy/k8s/00-config.yaml b/deploy/k8s/00-config.yaml index fb14e4e..5a60ccf 100644 --- a/deploy/k8s/00-config.yaml +++ b/deploy/k8s/00-config.yaml @@ -91,11 +91,14 @@ data: base_path: /api ui: true allow_config_write: false - # Read the key from a Secret and mount it; the value here is a placeholder - # that MUST be replaced before this reaches a network. + # The key comes from the environment (a Secret mounted into the pod), not + # from this ConfigMap, so `kubectl get cm` never prints it. The loader + # substitutes ${VAR} before parsing and *fails to start* if the variable + # is unset, so a missing secret is a crash loop rather than an + # unauthenticated console. auth: mode: apikey - api_keys: ["REPLACE-ME"] + api_keys: ["${FANCYDNS_API_KEY}"] trust_loopback: false metrics: diff --git a/deploy/k8s/05-serviceaccount.yaml b/deploy/k8s/05-serviceaccount.yaml new file mode 100644 index 0000000..49189b6 --- /dev/null +++ b/deploy/k8s/05-serviceaccount.yaml @@ -0,0 +1,12 @@ +# A stable identity for the pods. The server never talks to the Kubernetes API +# (blocklist replication goes through Valkey, not the API server), so the token +# is not just unused but actively undesirable: automounting it would hand every +# pod a credential it has no reason to hold. +apiVersion: v1 +kind: ServiceAccount +metadata: + name: fancy-dns + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns +automountServiceAccountToken: false diff --git a/deploy/k8s/08-secret-apikey.yaml b/deploy/k8s/08-secret-apikey.yaml new file mode 100644 index 0000000..c8bf4d2 --- /dev/null +++ b/deploy/k8s/08-secret-apikey.yaml @@ -0,0 +1,22 @@ +# The admin API key. It is read from the environment, not from the ConfigMap, +# so the key never appears in `kubectl get configmap -o yaml`. +# +# kubectl -n dns create secret generic fancy-dns-apikey \ +# --from-literal=api-key="$(openssl rand -hex 32)" +# +# With auth.mode: apikey and api_keys: ["${FANCYDNS_API_KEY}"] in the config, +# a missing FANCYDNS_API_KEY is a startup error rather than an empty key, so a +# forgotten secret shows up as a CrashLoopBackOff and not as an unauthenticated +# console on the network. +apiVersion: v1 +kind: Secret +metadata: + name: fancy-dns-apikey + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns +type: Opaque +stringData: + # PLACEHOLDER — replace before this reaches a cluster. `kubectl apply` on this + # file as-is will overwrite a real key with this value. + api-key: REPLACE-ME diff --git a/deploy/k8s/10-deployment.yaml b/deploy/k8s/10-deployment.yaml index 8cc57dd..568fc60 100644 --- a/deploy/k8s/10-deployment.yaml +++ b/deploy/k8s/10-deployment.yaml @@ -7,6 +7,11 @@ metadata: app.kubernetes.io/name: fancy-dns spec: serviceName: fancy-dns + # A StatefulSet rather than a Deployment because each pod owns a PVC (the + # cache snapshot and the query log are per-pod state worth keeping across a + # reschedule). The flip side: the HPA does not work on a StatefulSet — the + # controller and the autoscaler fight over the replica count — so scaling + # means adding nodes and replicas here, not CPU targets. replicas: 2 podManagementPolicy: Parallel selector: @@ -18,14 +23,19 @@ spec: app.kubernetes.io/name: fancy-dns annotations: # A config change is a SIGHUP, not a restart: the server re-reads the - # file and hot-swaps every component. + # file and hot-swaps every component. This annotation is for + # stakater/reloader-class controllers; without one, `kubectl rollout + # restart` is the manual equivalent. reloader.stakater.com/auto: "true" spec: - # DNS on a non-privileged port needs NET_BIND_SERVICE on Linux, or - # hostNetwork. hostNetwork is used here so the node's port 53 is served - # directly and the load balancer does not add a hop. - hostNetwork: true - dnsPolicy: Default + serviceAccountName: fancy-dns + # No hostNetwork. The server does not set SO_REUSEPORT, so a second + # host-network pod would fail to bind :53 and crash-loop; the ports are + # served by a LoadBalancer Service instead (see 20-service.yaml). + dnsPolicy: ClusterFirst + # The app drains in-flight queries for up to 15s on SIGTERM (see the + # shutdown timeout in cmd/fancy-dns/main.go); give it room beyond that. + terminationGracePeriodSeconds: 30 securityContext: runAsNonRoot: true runAsUser: 65532 @@ -47,6 +57,14 @@ spec: args: - -config - /etc/fancy-dns/config.yaml + env: + # Substituted into the config by the loader, so the admin API key + # comes from the Secret and not from the ConfigMap. + - name: FANCYDNS_API_KEY + valueFrom: + secretKeyRef: + name: fancy-dns-apikey + key: api-key ports: - name: dns-udp containerPort: 53 @@ -68,6 +86,10 @@ spec: readOnlyRootFilesystem: true capabilities: drop: ["ALL"] + # Only because of the low ports. The pod is not privileged and + # has no other capability left; drop the Service and the + # Capability together if you move the listeners off 53/443. + add: ["NET_BIND_SERVICE"] # Liveness: the process is up. /api/healthz is unauthenticated on # purpose, because a probe cannot carry an API key. livenessProbe: @@ -85,9 +107,9 @@ spec: initialDelaySeconds: 3 periodSeconds: 5 failureThreshold: 2 - # Startup: binding :53 and fetching the first blocklist can take a - # while on a cold node, so give it room before the liveness probe - # starts killing it. + # Startup: binding the listeners and fetching the first blocklist + # can take a while on a cold node, so give it room before the + # liveness probe starts killing it. startupProbe: httpGet: path: /api/healthz @@ -123,6 +145,10 @@ spec: - name: config configMap: name: fancy-dns-config + # Local zones are ordinary master files; ship them in a ConfigMap or + # manage them through the admin API (`/api/zones`), which writes into + # the data volume. Optional because the glob in the config matches + # nothing cleanly. - name: zones configMap: name: fancy-dns-zones @@ -130,116 +156,24 @@ spec: - name: tls secret: secretName: fancy-dns-tls - - name: data - emptyDir: {} - name: tmp emptyDir: {} ---- -apiVersion: v1 -kind: Service -metadata: - name: fancy-dns - namespace: dns - labels: - app.kubernetes.io/name: fancy-dns -spec: - clusterIP: None - selector: - app.kubernetes.io/name: fancy-dns - ports: - - name: dns-udp - port: 53 - targetPort: dns-udp - protocol: UDP - - name: dns-tcp - port: 53 - targetPort: dns-tcp - protocol: TCP - - name: dot - port: 853 - targetPort: dot - - name: doh - port: 443 - targetPort: doh - - name: admin - port: 8080 - targetPort: admin ---- -# The admin console and the DoH endpoint share a port only if the ingress -# terminates TLS; when it does, this routes /api and /dns-query to the same -# backend. Kept separate from the Service above because a headless Service -# cannot front an Ingress. -apiVersion: networking.k8s.io/v1 -kind: Ingress -metadata: - name: fancy-dns - namespace: dns - annotations: - nginx.ingress.kubernetes.io/backend-protocol: "HTTP" - nginx.ingress.kubernetes.io/proxy-buffering: "off" - nginx.ingress.kubernetes.io/proxy-read-timeout: "3600" -spec: - rules: - - host: dns.example.com - http: - paths: - - path: /api - pathType: Prefix - backend: - service: - name: fancy-dns - port: - name: admin - - path: /dns-query - pathType: Prefix - backend: - service: - name: fancy-dns - port: - name: doh - - path: / - pathType: Prefix - backend: - service: - name: fancy-dns - port: - name: admin - tls: - - hosts: [dns.example.com] - secretName: fancy-dns-tls ---- -apiVersion: policy/v1 -kind: PodDisruptionBudget -metadata: - name: fancy-dns - namespace: dns -spec: - minAvailable: 1 - selector: - matchLabels: - app.kubernetes.io/name: fancy-dns ---- -apiVersion: autoscaling/v2 -kind: HorizontalPodAutoscaler -metadata: - name: fancy-dns - namespace: dns -spec: - scaleTargetRef: - apiVersion: apps/v1 - kind: StatefulSet - name: fancy-dns - minReplicas: 2 - maxReplicas: 6 - metrics: - - type: Resource - resource: - name: cpu - target: - type: Utilization - averageUtilization: 70 - behavior: - scaleDown: - # A DNS server that flaps is worse than one that is briefly oversized: - # every scale-down drops the local cache and the blocklist lease. - stabilizationWindowSeconds: 600 + volumeClaimTemplates: + # Cache snapshot (cache.persist.path) and query log (qlog.path) live here, + # along with downloaded blocklists. ReadWriteOnce is per-pod: a pod keeps + # its own cache across a reschedule, which is the whole point of the + # snapshot, but it cannot be shared between replicas. + # + # Sizing: cache.size is an entry count, not bytes, so disk usage has to be + # estimated. A packed A/AAAA answer with a small authority section is + # roughly 300-600 bytes on the wire and nearer 1 kB resident, so 500k + # entries lands around 500 MB; qlog is the other consumer (see + # qlog.max_entries). 4Gi leaves room for both plus a re-fetched list. + - metadata: + name: data + spec: + accessModes: ["ReadWriteOnce"] + resources: + requests: + storage: 4Gi + # storageClassName: diff --git a/deploy/k8s/20-service.yaml b/deploy/k8s/20-service.yaml new file mode 100644 index 0000000..bb8341b --- /dev/null +++ b/deploy/k8s/20-service.yaml @@ -0,0 +1,103 @@ +--- +# Governing Service for the StatefulSet. Headless, and never used for traffic: +# it exists so pods get a stable DNS name, and so a PVC per pod has a home. +apiVersion: v1 +kind: Service +metadata: + name: fancy-dns + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns +spec: + clusterIP: None + selector: + app.kubernetes.io/name: fancy-dns + ports: + - name: dns-udp + port: 53 + targetPort: dns-udp + protocol: UDP + - name: dns-tcp + port: 53 + targetPort: dns-tcp + protocol: TCP + - name: dot + port: 853 + targetPort: dot + - name: doh + port: 443 + targetPort: doh + - name: admin + port: 8080 + targetPort: admin +--- +# The DNS service. This is the one clients resolve. +# +# Why a LoadBalancer and not hostNetwork: the server does not set SO_REUSEPORT, +# so two host-network pods cannot share the node's :53 and the second one +# would crash-loop on bind. Giving the cloud load balancer the port also keeps +# the node's own resolver untouched, which matters on a node that runs kubelet +# and CoreDNS. +# +# A cloud LB health-checks this Service on port 53 and needs the probe to +# actually answer a query: the default TCP check is not enough for UDP. If your +# provider supports UDP health checks, point them at 53/UDP; otherwise leave +# the load balancer health check on the HTTP admin port (see the comment on the +# web Service) and let the provider's DNS failover be the only response to a +# dead resolver. +apiVersion: v1 +kind: Service +metadata: + name: fancy-dns-dns + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns + annotations: + # Cloud-specific knobs go here; uncomment for the matching provider. + # service.beta.kubernetes.io/aws-load-balancer-type: nlb + # service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip + # service.beta.kubernetes.io/gcp-load-balancer-type: EXTERNAL +spec: + type: LoadBalancer + externalTrafficPolicy: Local + selector: + app.kubernetes.io/name: fancy-dns + ports: + - name: dns-udp + port: 53 + targetPort: dns-udp + protocol: UDP + - name: dns-tcp + port: 53 + targetPort: dns-tcp + protocol: TCP + - name: dot + port: 853 + targetPort: dot +--- +# A normal ClusterIP Service for everything the Ingress fronts. It has to be a +# separate object because a headless Service has no virtual IP for an ingress +# controller to route to. +# +# It is also the only reliable health-check target for the cloud load balancers +# on fancy-dns-dns: the admin port speaks HTTP and answers /api/healthz +# unauthenticated (a health check cannot carry an API key), so a probe can tell +# "bound and serving" from "process up but not listening". +apiVersion: v1 +kind: Service +metadata: + name: fancy-dns-web + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns +spec: + type: ClusterIP + selector: + app.kubernetes.io/name: fancy-dns + ports: + - name: doh + port: 443 + targetPort: doh + - name: admin + port: 8080 + targetPort: admin diff --git a/deploy/k8s/30-ingress.yaml b/deploy/k8s/30-ingress.yaml new file mode 100644 index 0000000..b8506ea --- /dev/null +++ b/deploy/k8s/30-ingress.yaml @@ -0,0 +1,64 @@ +# The console, the admin API and the DoH endpoint, behind TLS. +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: fancy-dns + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns + annotations: + nginx.ingress.kubernetes.io/backend-protocol: "HTTP" + # SSE (the live query log) is the reason for these three. With response + # buffering on, nginx holds events until the buffer fills and the log page + # looks frozen; with a 60s read timeout the stream is cut every minute and + # the page reconnects in a loop. + nginx.ingress.kubernetes.io/proxy-buffering: "off" + nginx.ingress.kubernetes.io/proxy-read-timeout: "3600" + nginx.ingress.kubernetes.io/proxy-send-timeout: "3600" +spec: + ingressClassName: nginx + rules: + - host: dns.example.com + http: + paths: + # DoH must be matched before the catch-all below, and both are + # Prefix paths on distinct roots so the order is not load-bearing. + - path: /dns-query + pathType: Prefix + backend: + service: + name: fancy-dns-web + port: + name: doh + - path: /api + pathType: Prefix + backend: + service: + name: fancy-dns-web + port: + name: admin + # The embedded console. + - path: / + pathType: Prefix + backend: + service: + name: fancy-dns-web + port: + name: admin + tls: + - hosts: [dns.example.com] + secretName: fancy-dns-tls +--- +# Two replicas means one of them can always be taken away for a node drain. +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: fancy-dns + namespace: dns + labels: + app.kubernetes.io/name: fancy-dns +spec: + minAvailable: 1 + selector: + matchLabels: + app.kubernetes.io/name: fancy-dns diff --git a/deploy/k8s/kustomization.yaml b/deploy/k8s/kustomization.yaml new file mode 100644 index 0000000..40ff8b8 --- /dev/null +++ b/deploy/k8s/kustomization.yaml @@ -0,0 +1,26 @@ +# kustomize build deploy/k8s | kubectl apply -f - +# +# The files are numbered so a plain `kubectl apply -f deploy/k8s/` in +# alphabetical order also lands in a working order: namespace and config +# first, then identity, then the workload, then the networking. +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +namespace: dns + +resources: + - 00-config.yaml + - 05-serviceaccount.yaml + - 08-secret-apikey.yaml + - 10-deployment.yaml + - 20-service.yaml + - 30-ingress.yaml + +labels: + - includeSelectors: false + pairs: + app.kubernetes.io/part-of: fancy-dns + +images: + - name: fancydns/fancy-dns + newTag: latest diff --git a/internal/acl/acl.go b/internal/acl/acl.go index 0ec3058..2506995 100644 --- a/internal/acl/acl.go +++ b/internal/acl/acl.go @@ -44,10 +44,12 @@ type Decision struct { MaxConcurrent int // LogQueries forces query logging regardless of the sampling rate. LogQueries bool - // Upstreams restricts this client to specific upstream addresses. + // Upstreams restricts this client to specific upstream addresses. The + // slice is owned by the engine and must be treated as read-only. Upstreams []string // AllowDomains and DenyDomains are per-group name overrides, using the - // same wildcard syntax as the blocklist ("*.example.com"). + // blocklist's wildcard spellings ("*.example.com", "**.example.com"), + // except that both forms also cover the apex: see NewDomainSet. AllowDomains []string DenyDomains []string // Default is true when no group matched. @@ -66,8 +68,11 @@ type DomainSet struct { any bool } -// NewDomainSet compiles patterns like "*.example.com" (subdomains only), -// "**.example.com" (subdomains plus the apex) and "example.com" (exact). +// NewDomainSet compiles the blocklist's wildcard spellings: "example.com" +// (exact), "*.example.com" and "**.example.com" (the domain plus its +// subdomains), and "*", "**" or "*.*" (everything). Unlike in the blocklist, +// "*.x" and "**.x" are the same rule here: the leading "*" label is dropped, +// so either form covers the apex too. An exact-only rule carries no wildcard. func NewDomainSet(patterns []string) *DomainSet { ds := &DomainSet{ exact: make(map[string]struct{}, len(patterns)), @@ -185,7 +190,7 @@ func Compile(cfg config.ACL) *Engine { comment: g.Comment, policy: Policy(g.Policy), any: g.Match.Any, - ptr: lowerAll(g.Match.PTRSuffixes), + ptr: ptrSuffixes(g.Match.PTRSuffixes), tags: lowerAll(g.Match.Tags), allow: NewDomainSet(g.AllowDomains), deny: NewDomainSet(g.DenyDomains), @@ -269,7 +274,12 @@ func (e *Engine) Match(c Client) Decision { } // Default returns the decision for a client that matched nothing. -func (e *Engine) Default() Decision { return e.defaultDecision() } +func (e *Engine) Default() Decision { + if e == nil { + return Decision{Policy: PolicyAllow, Default: true} + } + return e.defaultDecision() +} func (e *Engine) defaultDecision() Decision { d := e.def @@ -292,7 +302,7 @@ func (g *group) matches(c Client) bool { if len(g.ptr) > 0 { for _, n := range c.Names { for _, suffix := range g.ptr { - if strings.HasSuffix(n, suffix) { + if matchPTR(n, suffix) { return true } } @@ -310,6 +320,19 @@ func (g *group) matches(c Client) bool { return false } +// matchPTR reports whether the reverse name n ends with suffix at a DNS label +// boundary. Both sides are in FQDN form (trailing dot) by the time they get +// here, so "example.com." must not match the client "host.notexample.com.". +func matchPTR(n, suffix string) bool { + if suffix == "" || len(n) < len(suffix) || !strings.HasSuffix(n, suffix) { + return false + } + // 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)] + return rest == "" || rest[len(rest)-1] == '.' +} + // Groups returns the compiled group summaries for the API. type GroupSummary struct { Name string `json:"name"` @@ -378,7 +401,14 @@ func domainPatterns(d *DomainSet) []string { if d == nil { return nil } - out := make([]string, 0, len(d.exact)+len(d.subs)) + out := make([]string, 0, len(d.exact)+len(d.subs)+1) + if d.any { + // Keep the catch-all visible alongside the specific rules — dropping + // it would mean the engine's per-query recompile of the rendered + // patterns lost the "everything" case. It sorts first, like any other + // pattern. + out = append(out, "*") + } for k := range d.exact { out = append(out, k) } @@ -404,8 +434,21 @@ func lowerAll(in []string) []string { return out } -// TagsFromName derives candidate tags from a reverse-DNS name, using the -// labels as-is: "laptop.lan.example.net" yields laptop, lan, example, net. +// ptrSuffixes lowercases reverse-name suffixes and normalises them to the +// FQDN form (trailing dot) that Client.Names carries, so matching can rely on +// a label boundary at the end of the suffix. +func ptrSuffixes(in []string) []string { + out := lowerAll(in) + for i, s := range out { + if !strings.HasSuffix(s, ".") { + out[i] = s + "." + } + } + return out +} + +// TagsFromName derives candidate tags from a reverse-DNS name: every label +// above the parent domain, so "laptop.lan.example.net" yields laptop and lan. // Matching a group on tag "laptop" then works without any extra // configuration. func TagsFromName(name string) []string { diff --git a/internal/acl/acl_test.go b/internal/acl/acl_test.go new file mode 100644 index 0000000..6f8224c --- /dev/null +++ b/internal/acl/acl_test.go @@ -0,0 +1,1065 @@ +package acl + +import ( + "fmt" + "net/netip" + "slices" + "sync" + "testing" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" +) + +// grp builds a config.Group the way the YAML loader would, so every test +// doubles as a check that the config shape is usable as-is. +func grp(name, policy string, m config.Match) config.Group { + return config.Group{Name: name, Policy: policy, Match: m} +} + +func mustAddr(t testing.TB, s string) netip.Addr { + t.Helper() + a, err := netip.ParseAddr(s) + if err != nil { + t.Fatalf("ParseAddr(%q): %v", s, err) + } + return a +} + +// byIP builds a client that is known only by address. +func byIP(t testing.TB, s string) Client { + t.Helper() + return Client{IP: mustAddr(t, s)} +} + +// checkDecision compares every field of a decision, so a group mix-up (or a +// field that is carried over from the wrong group) cannot slip through. +func checkDecision(t *testing.T, what string, got, want Decision) { + t.Helper() + if got.Group != want.Group { + t.Errorf("%s: Group = %q, want %q", what, got.Group, want.Group) + } + if got.Policy != want.Policy { + t.Errorf("%s: Policy = %q, want %q", what, got.Policy, want.Policy) + } + if got.SkipBlocklist != want.SkipBlocklist { + t.Errorf("%s: SkipBlocklist = %v, want %v", what, got.SkipBlocklist, want.SkipBlocklist) + } + if got.SkipCache != want.SkipCache { + t.Errorf("%s: SkipCache = %v, want %v", what, got.SkipCache, want.SkipCache) + } + if got.RateLimit != want.RateLimit { + t.Errorf("%s: RateLimit = %d, want %d", what, got.RateLimit, want.RateLimit) + } + if got.MaxConcurrent != want.MaxConcurrent { + t.Errorf("%s: MaxConcurrent = %d, want %d", what, got.MaxConcurrent, want.MaxConcurrent) + } + if got.LogQueries != want.LogQueries { + t.Errorf("%s: LogQueries = %v, want %v", what, got.LogQueries, want.LogQueries) + } + if !slices.Equal(got.Upstreams, want.Upstreams) { + t.Errorf("%s: Upstreams = %v, want %v", what, got.Upstreams, want.Upstreams) + } + if !slices.Equal(got.AllowDomains, want.AllowDomains) { + t.Errorf("%s: AllowDomains = %v, want %v", what, got.AllowDomains, want.AllowDomains) + } + if !slices.Equal(got.DenyDomains, want.DenyDomains) { + t.Errorf("%s: DenyDomains = %v, want %v", what, got.DenyDomains, want.DenyDomains) + } + if got.Default != want.Default { + t.Errorf("%s: Default = %v, want %v", what, got.Default, want.Default) + } +} + +// TestEngineMatchFirstGroupWins pins first-match-wins: the two groups below +// both match 10.1.2.3, so the configured order is the only thing that decides +// the verdict, and the losing group contributes nothing to the decision. +func TestEngineMatchFirstGroupWins(t *testing.T) { + denyLAN := grp("deny-lan", "deny", config.Match{CIDRs: []string{"10.0.0.0/8"}}) + denyLAN.RateLimit = 1 + denyLAN.SkipBlocklist = true + desk := grp("desk", "allow", config.Match{CIDRs: []string{"10.1.2.3/32"}}) + desk.SkipCache = true + desk.Upstreams = []string{"127.0.0.1:5353"} + desk.AllowDomains = []string{"*.example.com"} + + overlapping := Client{IP: mustAddr(t, "10.1.2.3")} + + t.Run("broad deny listed first wins over the narrower allow", func(t *testing.T) { + e := Compile(config.ACL{ + DefaultPolicy: "allow", + Groups: []config.Group{denyLAN, desk}, + }) + checkDecision(t, "10.1.2.3", e.Match(overlapping), Decision{ + Group: "deny-lan", Policy: PolicyDeny, RateLimit: 1, SkipBlocklist: true, + }) + }) + + t.Run("narrow allow listed first wins over the broader deny", func(t *testing.T) { + e := Compile(config.ACL{ + DefaultPolicy: "allow", + Groups: []config.Group{desk, denyLAN}, + }) + checkDecision(t, "10.1.2.3", e.Match(overlapping), Decision{ + Group: "desk", Policy: PolicyAllow, SkipCache: true, + Upstreams: []string{"127.0.0.1:5353"}, AllowDomains: []string{"*.example.com"}, + }) + }) + + t.Run("client matching only the first group", func(t *testing.T) { + e := Compile(config.ACL{ + DefaultPolicy: "allow", + Groups: []config.Group{desk, denyLAN}, + }) + checkDecision(t, "10.9.9.9", e.Match(byIP(t, "10.9.9.9")), Decision{ + Group: "deny-lan", Policy: PolicyDeny, RateLimit: 1, SkipBlocklist: true, + }) + }) +} + +// TestEngineMatchFirstGroupWinsAcrossCriteria shows first-match-wins is not +// specific to CIDRs: whichever criterion matched first hides the others. +func TestEngineMatchFirstGroupWinsAcrossCriteria(t *testing.T) { + byName := grp("by-name", "deny", config.Match{PTRSuffixes: []string{"lan.example."}}) + byAddr := grp("by-addr", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}}) + // This client satisfies every criterion at once. + c := Client{ + IP: mustAddr(t, "10.1.2.3"), + Names: []string{"laptop.lan.example."}, + Tags: []string{"laptop"}, + } + + t.Run("ptr group first hides the cidr group", func(t *testing.T) { + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{byName, byAddr}}) + checkDecision(t, "all-criteria client", e.Match(c), Decision{Group: "by-name", Policy: PolicyDeny}) + }) + + t.Run("cidr group first hides the ptr group", func(t *testing.T) { + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{byAddr, byName}}) + checkDecision(t, "all-criteria client", e.Match(c), Decision{Group: "by-addr", Policy: PolicyAllow}) + }) + + t.Run("any group listed first shadows every later group", func(t *testing.T) { + catchAll := grp("catch-all", "deny", config.Match{Any: true}) + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{catchAll, byAddr}}) + checkDecision(t, "10.1.2.3", e.Match(c), Decision{Group: "catch-all", Policy: PolicyDeny}) + // config validation warns about this, but the engine still obeys + // the configured order. + if e.Count() != 2 { + t.Fatalf("Count() = %d, want 2", e.Count()) + } + }) +} + +// TestEngineMatchMostSpecificCIDRWithinGroup pins the per-group prefix sort: +// a group may list overlapping networks, and the client that lands on the +// /32 still gets that group's policy. Policy lives on the group, not on the +// prefix, so specificity only decides *which* group is considered, and the +// sorted order is observable in Summaries. +func TestEngineMatchMostSpecificCIDRWithinGroup(t *testing.T) { + g := grp("nested", "block_only", config.Match{CIDRs: []string{ + "10.0.0.0/8", "10.1.2.0/24", "10.1.2.3/32", "2001:db8::/32", + }}) + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{g}}) + + inside := []string{"10.1.2.3", "10.1.2.4", "10.1.2.200", "10.9.9.9", "2001:db8:1::1"} + for _, ip := range inside { + if d := e.Match(byIP(t, ip)); d.Group != "nested" || d.Policy != PolicyBlockOnly { + t.Errorf("Match(%s) = %+v, want group nested / block_only", ip, d) + } + } + // Outside the group entirely. + for _, ip := range []string{"11.0.0.1", "2001:db9::1"} { + if d := e.Match(byIP(t, ip)); !d.Default { + t.Errorf("Match(%s) = %+v, want the default decision", ip, d) + } + } + // 10.1.3.1 matches nothing more specific than 10.0.0.0/8, but that is + // still the group: a group matches on *any* of its networks, and policy + // lives on the group rather than on the prefix. + if d := e.Match(byIP(t, "10.1.3.1")); d.Group != "nested" { + t.Errorf("Match(10.1.3.1) = %+v, want group nested via 10.0.0.0/8", d) + } + + // The compiled prefixes are kept in config order: Summaries is a UI view, + // and a group matches on any of them, so the order carries no meaning. + s := e.Summaries() + if len(s) != 1 { + t.Fatalf("Summaries() = %d groups, want 1", len(s)) + } + // Echoed in matching order (longest prefix first), which is the order the + // group itself walks them in. + wantCIDRs := []string{"10.1.2.3/32", "2001:db8::/32", "10.1.2.0/24", "10.0.0.0/8"} + if !slices.Equal(s[0].CIDRs, wantCIDRs) { + t.Errorf("Summaries()[0].CIDRs = %v, want %v (longest prefix first)", s[0].CIDRs, wantCIDRs) + } +} + +// TestEngineMatchCIDRSpecificityDoesNotCrossGroups documents that the +// longest-prefix sort is scoped to one group: a /32 in a later group cannot +// outrank a /8 in an earlier one, because first-match-wins stops the walk. +func TestEngineMatchCIDRSpecificityDoesNotCrossGroups(t *testing.T) { + broad := grp("broad", "deny", config.Match{CIDRs: []string{"10.0.0.0/8"}}) + narrow := grp("narrow", "allow", config.Match{CIDRs: []string{"10.1.2.3/32"}}) + c := byIP(t, "10.1.2.3") + + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{broad, narrow}}) + if d := e.Match(c); d.Group != "broad" || d.Policy != PolicyDeny { + t.Errorf("broad first: Match = %+v, want broad/deny", d) + } + + e = Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{narrow, broad}}) + if d := e.Match(c); d.Group != "narrow" || d.Policy != PolicyAllow { + t.Errorf("narrow first: Match = %+v, want narrow/allow", d) + } +} + +// TestEngineMatchCIDREdges covers the network boundaries, v4/v6 mixing and +// junk in the config. +func TestEngineMatchCIDREdges(t *testing.T) { + g := grp("v4", "allow", config.Match{CIDRs: []string{ + "10.0.0.0/8", "not-a-cidr", "2001:db8::/32", + }}) + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{g}}) + + tests := []struct { + name string + ip string + want string // group name, "" = default + }{ + {"network address", "10.0.0.0", "v4"}, + {"top of the range", "10.255.255.255", "v4"}, + {"one past the range", "11.0.0.0", ""}, + {"just below the range", "9.255.255.255", ""}, + {"v4-mapped v6 client is unmapped", "::ffff:10.1.2.3", "v4"}, + {"v6 client in the v6 range", "2001:db8:1234::1", "v4"}, + {"v6 client outside the v6 range", "2001:db9::1", ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := e.Match(byIP(t, tc.ip)) + if tc.want == "" { + if !d.Default { + t.Fatalf("Match(%s) = %+v, want the default decision", tc.ip, d) + } + return + } + if d.Group != tc.want { + t.Fatalf("Match(%s) = %+v, want group %q", tc.ip, d, tc.want) + } + }) + } + + t.Run("a client with no address only matches non-CIDR criteria", func(t *testing.T) { + if d := e.Match(Client{}); !d.Default { + t.Fatalf("Match(Client{}) = %+v, want the default decision", d) + } + }) + + t.Run("unparsable CIDRs are skipped, valid ones still work", func(t *testing.T) { + if cidrs := e.Summaries()[0].CIDRs; !slices.Contains(cidrs, "10.0.0.0/8") || !slices.Contains(cidrs, "2001:db8::/32") { + t.Errorf("CIDRs = %v, want the two parsable prefixes", cidrs) + } + }) + + t.Run("a v4 client never matches a v6-only group", func(t *testing.T) { + v6 := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{ + grp("v6", "allow", config.Match{CIDRs: []string{"::/0", "2001:db8::/32"}}), + }}) + for _, ip := range []string{"10.1.2.3", "::ffff:10.1.2.3"} { + if d := v6.Match(byIP(t, ip)); !d.Default { + t.Errorf("Match(%s) = %+v, want the default decision", ip, d) + } + } + }) +} + +// TestEngineMatchPTRSuffix covers reverse-name matching. Suffixes are +// normalised to FQDN form at compile time (as config validation does), and +// Client.Names arrives in the same form from the reverse cache. +func TestEngineMatchPTRSuffix(t *testing.T) { + g := grp("by-name", "allow", config.Match{PTRSuffixes: []string{ + "Lan.Example.", // upper case and with the dot validation would add + "corp.internal", + }}) + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{g}}) + + tests := []struct { + name string + names []string + want string // group name, "" = default + }{ + {"exact suffix", []string{"lan.example."}, "by-name"}, + {"client below the suffix", []string{"laptop.lan.example."}, "by-name"}, + {"deep below the suffix", []string{"a.b.lan.example."}, "by-name"}, + {"suffix spelled without a trailing dot", []string{"host.corp.internal."}, "by-name"}, + // A name that is not fully qualified does not match an FQDN suffix: + // the suffix is normalised to end in a dot, so "host.corp.internal" + // simply does not have it. + {"suffix without the dot, name without the dot", []string{"host.corp.internal"}, ""}, + {"suffix as a prefix of the name", []string{"lan.example.com."}, ""}, + {"near-miss parent", []string{"host.notexample.com."}, ""}, + {"name shorter than the suffix", []string{"lan."}, ""}, + {"empty name", []string{""}, ""}, + {"no names at all", nil, ""}, + {"any of several names matches", []string{"other.invalid.", "printer.lan.example."}, "by-name"}, + {"last name matches", []string{"other.invalid.", "printer.lan.example."}, "by-name"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := e.Match(Client{IP: mustAddr(t, "203.0.113.9"), Names: tc.names}) + if tc.want == "" { + if !d.Default { + t.Fatalf("Match(names=%v) = %+v, want the default decision", tc.names, d) + } + return + } + if d.Group != tc.want { + t.Fatalf("Match(names=%v) = %+v, want group %q", tc.names, d, tc.want) + } + }) + } + + t.Run("suffixes are echoed normalised", func(t *testing.T) { + // Sorted, because the summary is a UI view. + want := []string{"lan.example.", "corp.internal."} + if got := e.Summaries()[0].PTRSuffixes; !slices.Equal(got, want) { + t.Errorf("PTRSuffixes = %v, want %v", got, want) + } + }) +} + +// TestEngineMatchPTRRequiresLabelBoundary is the regression test for PTR +// matching: a suffix used to be a plain strings.HasSuffix, so a group scoped +// to "example.com." also swallowed "notexample.com." and "ample.com.". +func TestEngineMatchPTRRequiresLabelBoundary(t *testing.T) { + g := grp("by-name", "allow", config.Match{PTRSuffixes: []string{"example.com."}}) + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{g}}) + // A client in a different organisation must not inherit the group. + names := []string{ + "host.notexample.com.", + "host.ample.com.", + "notexample.com.", + "host.example.com.evil.net.", + } + // "not-lan.example.com." does match: "not-lan" is a label of its own, so + // the suffix starts on a label boundary. It is here to say so explicitly, + // because it is the case a naive HasSuffix gets wrong in the other + // direction. + if d := e.Match(Client{Names: []string{"not-lan.example.com."}}); d.Group != "by-name" { + t.Errorf("Match(%q) = %+v, want group by-name", "not-lan.example.com.", d) + } + for _, n := range names { + t.Run(n, func(t *testing.T) { + if d := e.Match(Client{Names: []string{n}}); !d.Default { + t.Fatalf("Match(%q) = %+v, want the default decision", n, d) + } + }) + } + // The genuine suffix, and the suffix that is the whole name, still match. + for _, n := range []string{"example.com.", "host.example.com.", "a.b.example.com."} { + t.Run(n, func(t *testing.T) { + if d := e.Match(Client{Names: []string{n}}); d.Group != "by-name" { + t.Fatalf("Match(%q) = %+v, want group by-name", n, d) + } + }) + } +} + +// TestMatchPTRLabelBoundary unit-tests the suffix helper directly, including +// the degenerate inputs. +func TestMatchPTRLabelBoundary(t *testing.T) { + tests := []struct { + name, n, suffix string + want bool + }{ + {"identical", "example.com.", "example.com.", true}, + {"one label more", "host.example.com.", "example.com.", true}, + {"no trailing dot on the name", "host.example.com", "example.com.", false}, + {"partial label", "notexample.com.", "example.com.", false}, + {"partial label at the front", "ample.com.", "example.com.", false}, + {"name is shorter", "lan.", "lan.example.", false}, + {"empty suffix", "host.example.com.", "", false}, + {"empty name", "", "example.com.", false}, + {"a hyphen is not a label boundary", "host-example.com.", "example.com.", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := matchPTR(tc.n, tc.suffix); got != tc.want { + t.Errorf("matchPTR(%q, %q) = %v, want %v", tc.n, tc.suffix, got, tc.want) + } + }) + } +} + +// TestEngineMatchTags covers tag-only groups: the server derives tags from +// the PTR labels, so a group needs no address or name of its own. +func TestEngineMatchTags(t *testing.T) { + g := grp("laptops", "allow", config.Match{Tags: []string{"Laptop", " kiosk "}}) + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{g}}) + + tests := []struct { + name string + tags []string + names []string + want string + }{ + {"tag matches", []string{"laptop"}, nil, "laptops"}, + {"tags are exact, not prefixes", []string{"laptop2"}, nil, ""}, + {"second configured tag, whitespace trimmed", []string{"kiosk"}, nil, "laptops"}, + {"any client tag may match", []string{"printer", "kiosk"}, nil, "laptops"}, + {"tags derived from the PTR name", nil, []string{"Kiosk.Lan.Internal."}, "laptops"}, + {"untagged client", nil, nil, ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // The server derives tags from the PTR labels, so a name can + // carry the tag on its own. + var tags []string + for _, n := range tc.names { + tags = append(tags, TagsFromName(n)...) + } + tags = append(tags, tc.tags...) + d := e.Match(Client{Names: tc.names, Tags: tags}) + if tc.want == "" { + if !d.Default { + t.Fatalf("Match(tags=%v) = %+v, want the default decision", tc.tags, d) + } + return + } + if d.Group != tc.want { + t.Fatalf("Match(tags=%v) = %+v, want group %q", tc.tags, d, tc.want) + } + }) + } + + t.Run("tags are echoed lowercased in config order", func(t *testing.T) { + want := []string{"laptop", "kiosk"} + if got := e.Summaries()[0].Tags; !slices.Equal(got, want) { + t.Errorf("Tags = %v, want %v", got, want) + } + }) +} + +// TestEngineMatchNoCriteria pins that a group with an empty match block is +// inert: there is no "disabled" flag, a group only goes away by being removed. +func TestEngineMatchNoCriteria(t *testing.T) { + inert := grp("inert", "deny", config.Match{}) + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{inert}}) + if e.Count() != 1 { + t.Fatalf("Count() = %d, want 1", e.Count()) + } + c := Client{ + IP: mustAddr(t, "10.1.2.3"), + Names: []string{"laptop.lan.example."}, + Tags: []string{"laptop"}, + } + d := e.Match(c) + if !d.Default || d.Group != "" { + t.Fatalf("Match() = %+v, want the default decision (an empty match block matches nothing)", d) + } +} + +// TestEngineMatchAnyCatchAll pins the fallback group, which matches even a +// client with no address, no names and no tags. +func TestEngineMatchAnyCatchAll(t *testing.T) { + catchAll := grp("catch-all", "block_only", config.Match{Any: true}) + catchAll.RateLimit = 25 + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{catchAll}}) + for _, c := range []Client{ + {}, + {IP: mustAddr(t, "192.0.2.1")}, + {Names: []string{"host.elsewhere."}}, + {Tags: []string{"whatever"}}, + } { + d := e.Match(c) + if d.Group != "catch-all" || d.Policy != PolicyBlockOnly || d.RateLimit != 25 { + t.Errorf("Match(%+v) = %+v, want catch-all/block_only", c, d) + } + } + if e.Count() != 1 { + t.Errorf("Count() = %d, want 1", e.Count()) + } +} + +// TestEngineDefaultPolicy pins the documented behaviour for a client that +// matches no group: the ACL's own default policy, flagged Default, with no +// group and no domain overrides. +func TestEngineDefaultPolicy(t *testing.T) { + t.Run("empty config allows everything", func(t *testing.T) { + e := Compile(config.ACL{}) + want := Decision{Policy: PolicyAllow, Default: true} + checkDecision(t, "unmatched", e.Match(byIP(t, "198.51.100.1")), want) + checkDecision(t, "Default()", e.Default(), want) + if e.Count() != 0 { + t.Errorf("Count() = %d, want 0", e.Count()) + } + }) + + tests := []struct { + policy string + want Policy + }{ + {"allow", PolicyAllow}, + {"block_only", PolicyBlockOnly}, + {"deny", PolicyDeny}, + } + for _, tc := range tests { + t.Run("default policy "+tc.policy, func(t *testing.T) { + e := Compile(config.ACL{DefaultPolicy: tc.policy, DefaultRateLimit: 7}) + want := Decision{Policy: tc.want, RateLimit: 7, Default: true} + checkDecision(t, "unmatched", e.Match(Client{}), want) + checkDecision(t, "Default()", e.Default(), want) + if e.Match(Client{}).Allows() != (tc.want != PolicyDeny) { + t.Errorf("Allows() = %v for policy %q", e.Match(Client{}).Allows(), tc.want) + } + }) + } + + t.Run("a matched group does not inherit the default rate limit", func(t *testing.T) { + e := Compile(config.ACL{ + DefaultPolicy: "allow", + DefaultRateLimit: 7, + Groups: []config.Group{grp("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}})}, + }) + if d := e.Match(byIP(t, "10.1.1.1")); d.RateLimit != 0 { + t.Errorf("RateLimit = %d, want 0 (unlimited)", d.RateLimit) + } + }) +} + +// TestEngineNilSafe pins the nil-receiver behaviour of the public methods: a +// holder that was never given an engine must not panic on the query path. +func TestEngineNilSafe(t *testing.T) { + var e *Engine + want := Decision{Policy: PolicyAllow, Default: true} + checkDecision(t, "Match", e.Match(byIP(t, "10.1.2.3")), want) + checkDecision(t, "Default", e.Default(), want) + if e.Count() != 0 { + t.Errorf("Count() = %d, want 0", e.Count()) + } + if e.Summaries() != nil { + t.Errorf("Summaries() = %v, want nil", e.Summaries()) + } + if d := (*Engine)(nil).Match(Client{}); !d.Allows() { + t.Errorf("nil engine denies, want allow") + } +} + +// TestDecisionAllows pins the one rule the query path relies on: only "deny" +// refuses. +func TestDecisionAllows(t *testing.T) { + tests := []struct { + policy Policy + want bool + }{ + {PolicyAllow, true}, + {PolicyBlockOnly, true}, + {PolicyDeny, false}, + {"", true}, // the zero Decision is permissive + } + for _, tc := range tests { + if got := (Decision{Policy: tc.policy}).Allows(); got != tc.want { + t.Errorf("Decision{Policy: %q}.Allows() = %v, want %v", tc.policy, got, tc.want) + } + } +} + +// TestEngineGroupPolicyDefaults pins that a group with no policy in the config +// is treated as "allow" rather than as an unknown value. +func TestEngineGroupPolicyDefaults(t *testing.T) { + e := Compile(config.ACL{Groups: []config.Group{{Name: "g", Match: config.Match{Any: true}}}}) + d := e.Match(Client{}) + if d.Group != "g" || d.Policy != PolicyAllow { + t.Errorf("Match() = %+v, want group g / allow", d) + } + if s := e.Summaries(); len(s) != 1 || s[0].Policy != "allow" { + t.Errorf("Summaries() = %+v, want policy allow", s) + } +} + +// TestEngineSummaries pins the API view: config order is preserved, CIDRs come +// out most-specific-first, and the counters are left for the engine to fill. +func TestEngineSummaries(t *testing.T) { + a := grp("a", "deny", config.Match{CIDRs: []string{"10.0.0.0/8"}, PTRSuffixes: []string{"lan.example."}, Tags: []string{"laptop"}}) + a.Comment = "the office" + a.AllowDomains = []string{"example.com", "*.example.net"} + a.DenyDomains = []string{"*.ads.example.net"} + a.SkipBlocklist = true + a.MaxConcurrent = 3 + a.LogQueries = true + a.Upstreams = []string{"127.0.0.1:5301"} + b := grp("b", "allow", config.Match{Any: true}) + e := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{a, b}}) + + s := e.Summaries() + if len(s) != 2 { + t.Fatalf("Summaries() = %d groups, want 2", len(s)) + } + got := s[0] + want := GroupSummary{ + Name: "a", + Comment: "the office", + Policy: "deny", + CIDRs: []string{"10.0.0.0/8"}, + PTRSuffixes: []string{"lan.example."}, + Tags: []string{"laptop"}, + AllowDomains: []string{"*.example.net", "example.com"}, + DenyDomains: []string{"*.ads.example.net"}, + SkipBlocklist: true, + MaxConcurrent: 3, + LogQueries: true, + Upstreams: []string{"127.0.0.1:5301"}, + } + if got.Name != want.Name || got.Comment != want.Comment || got.Policy != want.Policy || + got.Any != want.Any || got.SkipBlocklist != want.SkipBlocklist || + got.MaxConcurrent != want.MaxConcurrent || got.LogQueries != want.LogQueries { + t.Errorf("Summaries()[0] scalars = %+v, want %+v", got, want) + } + if !slices.Equal(got.CIDRs, want.CIDRs) { + t.Errorf("CIDRs = %v, want %v", got.CIDRs, want.CIDRs) + } + if !slices.Equal(got.PTRSuffixes, want.PTRSuffixes) { + t.Errorf("PTRSuffixes = %v, want %v", got.PTRSuffixes, want.PTRSuffixes) + } + if !slices.Equal(got.Tags, want.Tags) { + t.Errorf("Tags = %v, want %v", got.Tags, want.Tags) + } + if !slices.Equal(got.AllowDomains, want.AllowDomains) { + t.Errorf("AllowDomains = %v, want %v (sorted, wildcards rendered)", got.AllowDomains, want.AllowDomains) + } + if !slices.Equal(got.DenyDomains, want.DenyDomains) { + t.Errorf("DenyDomains = %v, want %v", got.DenyDomains, want.DenyDomains) + } + if !slices.Equal(got.Upstreams, want.Upstreams) { + t.Errorf("Upstreams = %v, want %v", got.Upstreams, want.Upstreams) + } + // Counters and the dry-run marker belong to the engine/app layer. + if got.RateLimit != 0 || got.ClientCount != 0 || got.QueryCount != 0 || got.BlockedCount != 0 || got.Matched { + t.Errorf("Summaries()[0] = %+v, want zeroed counters and Matched=false", got) + } + if s[1].Name != "b" || !s[1].Any || s[1].Policy != "allow" { + t.Errorf("Summaries()[1] = %+v, want group b / any / allow", s[1]) + } + if len(s[1].CIDRs) != 0 || len(s[1].PTRSuffixes) != 0 || len(s[1].AllowDomains) != 0 { + t.Errorf("Summaries()[1] = %+v, want empty lists", s[1]) + } +} + +// TestEngineIsImmutableSnapshot pins the reload contract: the engine copies +// everything out of the config, so mutating the config the caller still holds +// cannot change a running engine. +func TestEngineIsImmutableSnapshot(t *testing.T) { + upstreams := []string{"127.0.0.1:5301"} + ptr := []string{"lan.example."} + tags := []string{"laptop"} + allow := []string{"*.example.com"} + deny := []string{"*.ads.example.com"} + cidrs := []string{"10.0.0.0/8"} + cfg := config.ACL{ + DefaultPolicy: "allow", + Groups: []config.Group{{ + Name: "g", + Policy: "allow", + Match: config.Match{CIDRs: cidrs, PTRSuffixes: ptr, Tags: tags}, + AllowDomains: allow, + DenyDomains: deny, + Upstreams: upstreams, + }}, + } + e := Compile(cfg) + + // Everything the caller kept a reference to gets rewritten behind the + // engine's back, as a reload in flight would. + upstreams[0] = "127.0.0.1:9999" + ptr[0] = "evil.example." + tags[0] = "evil" + allow[0] = "*.evil.example.com" + deny[0] = "*.evil.example.com" + cidrs[0] = "0.0.0.0/0" + + c := Client{ + IP: mustAddr(t, "10.1.2.3"), + Names: []string{"laptop.lan.example."}, + Tags: []string{"laptop"}, + } + d := e.Match(c) + checkDecision(t, "immutable engine", d, Decision{ + Group: "g", + Policy: PolicyAllow, + Upstreams: []string{"127.0.0.1:5301"}, + AllowDomains: []string{"*.example.com"}, + DenyDomains: []string{"*.ads.example.com"}, + }) + if !NewDomainSet(d.AllowDomains).Match("www.example.com") { + t.Error("allow_domains lost the original pattern after the config was mutated") + } + // The rewritten CIDR must not have widened the group either. + if d := e.Match(byIP(t, "198.51.100.1")); !d.Default { + t.Errorf("Match(198.51.100.1) = %+v, want the default decision", d) + } +} + +// TestDomainSetMatch covers every pattern form DomainSet accepts, including +// the boundary cases the wildcard spellings are easy to get wrong. +func TestDomainSetMatch(t *testing.T) { + tests := []struct { + name string + patterns []string + query string + want bool + }{ + // Exact rules. + {"exact apex", []string{"example.com"}, "example.com", true}, + {"exact apex with trailing dot", []string{"example.com"}, "example.com.", true}, + {"exact apex uppercase query", []string{"example.com"}, "ExAmPlE.CoM", true}, + {"exact rule configured uppercase", []string{"EXAMPLE.com"}, "example.com", true}, + {"exact rule configured with spaces and a dot", []string{" Example.COM. "}, "example.com", true}, + {"exact rule does not cover a subdomain", []string{"example.com"}, "www.example.com", false}, + {"exact rule does not cover the parent", []string{"www.example.com"}, "example.com", false}, + {"exact rule does not cover a name ending in it", []string{"example.com"}, "notexample.com", false}, + {"exact rule does not cover a longer name starting with it", []string{"example.com"}, "example.com.evil.net", false}, + + // "*.x" and "**.x" are the same rule here: apex plus subdomains. + {"sub wildcard matches a subdomain", []string{"*.example.com"}, "www.example.com", true}, + {"sub wildcard matches a deep subdomain", []string{"*.example.com"}, "a.b.c.example.com", true}, + {"sub wildcard also matches the apex", []string{"*.example.com"}, "example.com", true}, + {"sub wildcard apex with trailing dot", []string{"*.example.com"}, "example.com.", true}, + {"sub wildcard does not match a name that is only a prefix", []string{"*.example.com"}, "notexample.com", false}, + {"sub wildcard does not match a sibling domain", []string{"*.example.com"}, "example.com.evil.net", false}, + {"sub wildcard does not match a shorter name", []string{"*.example.com"}, "example.co", false}, + {"root wildcard matches the apex", []string{"**.example.com"}, "example.com", true}, + {"root wildcard matches a subdomain", []string{"**.example.com"}, "a.example.com", true}, + + // Catch-alls. + {"star matches anything", []string{"*"}, "anything.test", true}, + {"double star matches anything", []string{"**"}, "anything.test", true}, + {"star dot star matches anything", []string{"*.*"}, "a.b.c", true}, + {"star matches the root label", []string{"*"}, ".", true}, + {"star short-circuits an empty name", []string{"*"}, "", true}, + + // Oddities. + {"empty set matches nothing", nil, "example.com", false}, + {"empty set does not match the empty name", nil, "", false}, + {"blank patterns are dropped", []string{"", " "}, "example.com", false}, + {"the root label alone is normalised to the empty name", []string{"example.com"}, ".", false}, + {"duplicates collapse", []string{"example.com", "example.com."}, "example.com", true}, + {"union of the three forms", []string{"example.com", "*.example.net", "**.safe.example"}, "a.safe.example", true}, + {"union still rejects unrelated names", []string{"example.com", "*.example.net", "**.safe.example"}, "example.org", false}, + {"exact plus sub wildcard", []string{"example.com", "*.example.com"}, "www.example.com", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := NewDomainSet(tc.patterns).Match(tc.query); got != tc.want { + t.Errorf("NewDomainSet(%v).Match(%q) = %v, want %v", tc.patterns, tc.query, got, tc.want) + } + }) + } +} + +// TestDomainSetLen pins what counts as a rule: the catch-all is a flag, not an +// entry, and duplicate spellings collapse to one. +func TestDomainSetLen(t *testing.T) { + tests := []struct { + name string + patterns []string + want int + }{ + {"nothing", nil, 0}, + {"one exact", []string{"example.com"}, 1}, + {"one sub", []string{"*.example.com"}, 1}, + {"one root", []string{"**.example.com"}, 1}, + {"sub and root share one entry", []string{"*.example.com", "**.example.com"}, 1}, + {"exact and sub are two", []string{"example.com", "*.example.com"}, 2}, + {"duplicates collapse", []string{"example.com", "example.com."}, 1}, + {"the catch-all is not an entry", []string{"*"}, 0}, + {"blanks and the catch-all are not entries", []string{"", " ", "*", "*.a.example"}, 1}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := NewDomainSet(tc.patterns).Len(); got != tc.want { + t.Errorf("NewDomainSet(%v).Len() = %d, want %d", tc.patterns, got, tc.want) + } + }) + } +} + +// TestDomainSetNilReceiver pins the guard used by callers holding an optional +// set (a group without domain rules compiles to an empty set, never a nil one, +// but the API is called with typed nils too). +func TestDomainSetNilReceiver(t *testing.T) { + var ds *DomainSet + if ds.Match("example.com") { + t.Error("nil DomainSet matched") + } + if ds.Len() != 0 { + t.Errorf("nil DomainSet Len() = %d, want 0", ds.Len()) + } + if got := domainPatterns(nil); got != nil { + t.Errorf("domainPatterns(nil) = %v, want nil", got) + } +} + +// TestLabelEnd pins the suffix walk that DomainSet.Match uses. +func TestLabelEnd(t *testing.T) { + tests := []struct { + s string + want int + }{ + {"", 0}, + {"example.com", 8}, + {"a.b.c", 2}, + {"single", 6}, + {".leading", 1}, + } + for _, tc := range tests { + if got := labelEnd(tc.s); got != tc.want { + t.Errorf("labelEnd(%q) = %d, want %d", tc.s, got, tc.want) + } + } +} + +// TestDomainPatternsRoundTrip pins that a decision's domain lists can be fed +// straight back into NewDomainSet - which is exactly what the engine does per +// query - without changing what matches. +func TestDomainPatternsRoundTrip(t *testing.T) { + patterns := []string{ + "example.com", "Example.NET.", "*.example.org", "**.safe.example", "*", + } + probes := []string{ + "example.com", "www.example.com", "notexample.com", "example.com.evil.net", + "a.b.example.org", "a.safe.example", "safe.example", "unrelated.test", "", + } + ds := NewDomainSet(patterns) + again := NewDomainSet(domainPatterns(ds)) + for _, p := range probes { + if got, want := again.Match(p), ds.Match(p); got != want { + t.Errorf("round trip %v: Match(%q) = %v, want %v", domainPatterns(ds), p, got, want) + } + } + // The rendered form is stable, which is what the API shows, and keeps the + // catch-all visible instead of silently dropping it. + want := []string{"*", "*.example.org", "*.safe.example", "example.com", "example.net"} + if got := domainPatterns(ds); !slices.Equal(got, want) { + t.Errorf("domainPatterns() = %v, want %v", got, want) + } +} + +// TestEngineMatchCarriesDomainRules pins that the per-group allow/deny lists +// travel with the decision, and that a group without them reports none. +func TestEngineMatchCarriesDomainRules(t *testing.T) { + restricted := grp("restricted", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}}) + restricted.AllowDomains = []string{"example.com", "**.safe.example"} + restricted.DenyDomains = []string{"*.ads.safe.example"} + open := grp("open", "allow", config.Match{Any: true}) + e := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{restricted, open}}) + + d := e.Match(byIP(t, "10.1.2.3")) + if !slices.Equal(d.AllowDomains, []string{"*.safe.example", "example.com"}) { + t.Errorf("AllowDomains = %v", d.AllowDomains) + } + if !slices.Equal(d.DenyDomains, []string{"*.ads.safe.example"}) { + t.Errorf("DenyDomains = %v", d.DenyDomains) + } + // The engine applies them with a fresh set; check the verdict it derives. + if acl := NewDomainSet(d.DenyDomains); !acl.Match("x.ads.safe.example") { + t.Error("deny list does not match a subdomain of the denied domain") + } + allow := NewDomainSet(d.AllowDomains) + if !allow.Match("example.com") || !allow.Match("a.safe.example") || allow.Match("other.test") { + t.Error("allow list does not behave like the group rule") + } + + d = e.Match(byIP(t, "198.51.100.1")) + // Empty rather than nil: the decision always carries the lists, because + // the engine hands them straight to NewDomainSet without a nil check. + if len(d.AllowDomains) != 0 || len(d.DenyDomains) != 0 { + t.Errorf("group without domain rules reported %v / %v, want empty", d.AllowDomains, d.DenyDomains) + } +} + +// TestHolderSwap pins the atomic reload contract used by the DNS hot path. +func TestHolderSwap(t *testing.T) { + deny := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{grp("g", "deny", config.Match{Any: true})}}) + allow := Compile(config.ACL{DefaultPolicy: "allow"}) + h := NewHolder(deny) + if h.Get() != deny { + t.Fatal("Get() did not return the engine passed to NewHolder") + } + if d := h.Get().Match(Client{}); d.Policy != PolicyDeny { + t.Errorf("policy = %q, want deny", d.Policy) + } + h.Set(allow) + if h.Get() != allow { + t.Fatal("Get() did not return the swapped engine") + } + if d := h.Get().Match(Client{}); d.Policy != PolicyAllow { + t.Errorf("policy after swap = %q, want allow", d.Policy) + } + // An empty holder is usable: Match has to tolerate a nil snapshot. + h.Set(nil) + if h.Get() != nil { + t.Fatal("Get() != nil after Set(nil)") + } + if d := h.Get().Match(Client{}); !d.Allows() || !d.Default { + t.Errorf("nil engine decision = %+v, want the permissive default", d) + } +} + +// TestHolderConcurrentReload exercises the swap under -race: readers keep +// matching against whichever immutable snapshot they loaded, and a decision +// is always one of the two configured policies (never a half-written one). +func TestHolderConcurrentReload(t *testing.T) { + deny := Compile(config.ACL{DefaultPolicy: "allow", Groups: []config.Group{grp("g", "deny", config.Match{CIDRs: []string{"10.0.0.0/8"}})}}) + allow := Compile(config.ACL{DefaultPolicy: "deny", Groups: []config.Group{grp("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}})}}) + h := NewHolder(deny) + + // Parse outside the goroutines: t.Fatal is not allowed off the main one. + client := byIP(t, "10.1.2.3") + stop := make(chan struct{}) + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + eng := h.Get() + d := eng.Match(client) + if d.Policy != PolicyAllow && d.Policy != PolicyDeny { + t.Errorf("Match() = %+v, want one of the configured policies", d) + return + } + if d.Group != "g" { + t.Errorf("Match() = %+v, want group g", d) + return + } + } + }() + } + wg.Add(1) + go func() { + defer wg.Done() + for i := range 2000 { + if i%2 == 0 { + h.Set(deny) + } else { + h.Set(allow) + } + } + close(stop) + }() + wg.Wait() +} + +// TestTagsFromName pins the tag derivation the Match.Tags rule depends on. +func TestTagsFromName(t *testing.T) { + tests := []struct { + name string + in string + want []string + }{ + {"three labels", "laptop.lan.internal", []string{"laptop"}}, + {"trailing dot is dropped", "laptop.lan.internal.", []string{"laptop"}}, + {"upper case is folded", "Kiosk.Lan.Internal.", []string{"kiosk"}}, + {"four labels drop the parent domain", "laptop.lan.example.net", []string{"laptop", "lan"}}, + {"single label is kept", "host", []string{"host"}}, + {"two labels leave nothing", "example.com", nil}, + {"empty name", "", nil}, + {"root label", ".", nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := TagsFromName(tc.in) + if len(got) != len(tc.want) { + t.Fatalf("TagsFromName(%q) = %v, want %v", tc.in, got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("TagsFromName(%q) = %v, want %v", tc.in, got, tc.want) + } + } + }) + } +} + +// benchEngine builds a realistic table: seven unrelated groups, then the one +// the benchmarked client lands on. +func benchEngine() *Engine { + groups := make([]config.Group, 0, 8) + for i := range 7 { + groups = append(groups, grp( + fmt.Sprintf("tenant-%d", i), + "allow", + config.Match{CIDRs: []string{fmt.Sprintf("10.%d.0.0/16", i)}}, + )) + } + target := grp("target", "allow", config.Match{ + CIDRs: []string{"10.7.1.5/32"}, + PTRSuffixes: []string{"lan.example."}, + Tags: []string{"laptop"}, + }) + target.AllowDomains = []string{"*.example.com"} + target.DenyDomains = []string{"ads.example.com"} + groups = append(groups, target) + return Compile(config.ACL{DefaultPolicy: "allow", Groups: groups}) +} + +// BenchmarkEngineMatch measures the hot-path lookup for a client that is +// identified by address, reverse name and tag and matches the last group. +func BenchmarkEngineMatch(b *testing.B) { + e := benchEngine() + c := Client{ + IP: netip.MustParseAddr("10.7.1.5"), + Names: []string{"laptop.lan.example."}, + Tags: []string{"laptop"}, + } + b.ReportAllocs() + for b.Loop() { + if d := e.Match(c); d.Policy != PolicyAllow { + b.Fatalf("Match() = %+v, want allow", d) + } + } +} + +// BenchmarkEngineMatchDefault measures the miss path, where no group matches +// and the default decision is returned. +func BenchmarkEngineMatchDefault(b *testing.B) { + e := benchEngine() + c := Client{IP: netip.MustParseAddr("203.0.113.7")} + b.ReportAllocs() + for b.Loop() { + if d := e.Match(c); !d.Default { + b.Fatalf("Match() = %+v, want the default decision", d) + } + } +} + +// BenchmarkHolderGet measures the snapshot load the query path starts with. +func BenchmarkHolderGet(b *testing.B) { + h := NewHolder(benchEngine()) + b.ReportAllocs() + for b.Loop() { + if h.Get() == nil { + b.Fatal("Get() = nil") + } + } +} + +// BenchmarkDomainSetMatch measures the name matcher used for the per-group +// allow/deny lists. +func BenchmarkDomainSetMatch(b *testing.B) { + ds := NewDomainSet([]string{"*.example.com", "**.safe.example", "tracker.net", "*"}) + for _, q := range []string{"www.example.com", "tracker.net"} { + b.Run(q, func(b *testing.B) { + for b.Loop() { + if !ds.Match(q) { + b.Fatalf("Match(%q) = false", q) + } + } + }) + } +} diff --git a/internal/app/hooks.go b/internal/app/hooks.go index a375c8d..9bef394 100644 --- a/internal/app/hooks.go +++ b/internal/app/hooks.go @@ -378,7 +378,9 @@ func (a *App) listDomains(q api.DomainQuery) (any, error) { Source: e.Source, Categories: e.Categories, Hits: e.Hits(), - Rule: kind.Pattern(pattern), + // The wire form, so the console can re-add this exact rule + // (with its wildcard) instead of degrading it to an exact name. + Rule: kind.WireName(pattern), }) return true }) diff --git a/internal/blocklist/parse.go b/internal/blocklist/parse.go index a68bf04..d68ee35 100644 --- a/internal/blocklist/parse.go +++ b/internal/blocklist/parse.go @@ -56,7 +56,7 @@ type ParseStats struct { // DetectFormat sniffs a list's syntax from its first meaningful lines. func DetectFormat(data []byte) Format { - sc := bufio.NewScanner(bytes.NewReader(data)) + sc := bufio.NewScanner(bytes.NewReader(stripBOM(data))) sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) sawRecord := false for i := 0; sc.Scan() && i < 200; i++ { @@ -104,11 +104,23 @@ func allDigitsOrName(s string) bool { return false } +// utf8BOM is the byte-order mark a UTF-8 file may start with. Editors on +// Windows add it, and it would otherwise glue itself to the first rule's +// owner name, silently dropping the first entry of the list. +var utf8BOM = []byte{0xef, 0xbb, 0xbf} + +// stripBOM drops a leading UTF-8 byte-order mark. It is applied once, at the +// front door of the parse path, so every parser sees the same bytes. +func stripBOM(data []byte) []byte { + return bytes.TrimPrefix(data, utf8BOM) +} + // Parse converts list data into rules using the given format (FormatAuto // sniffs). It never returns partial garbage: on a hard error the rules // accumulated so far are returned alongside it so the caller can decide // whether to keep the last good set or adopt the partial one. func Parse(format Format, data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { + data = stripBOM(data) if opt.DefaultTTL == 0 { opt.DefaultTTL = 60 } @@ -165,9 +177,13 @@ func parseHosts(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { addr := fields[0] if !isAddr(addr) && !strings.EqualFold(addr, "localhost") { // Not an address: this is not a hosts file after all, so treat - // the whole line as a domain list. - stats.Notices = appendNotice(stats.Notices, "no address in first field, falling back to a plain domain list") - return parsePlain(data, opt) + // the whole line as a domain list. The notice has to travel with + // the stats the plain parser returns, or the operator sees the + // format switch with no explanation for it. + const notice = "no address in first field, falling back to a plain domain list" + rules, pstats, perr := parsePlain(data, opt) + pstats.Notices = appendNotice(pstats.Notices, notice) + return rules, pstats, perr } names := fields[1:] if isLoopbackName(names) { @@ -346,13 +362,21 @@ func parsePlain(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { } continue } - // Support inline allow markers: "!allow example.com". + // Support inline allow markers: the documented "!allow example.com" + // and the bare "!example.com", plus the adblock exception form + // "@@||example.com^" operators paste into plain lists too. allow := false - if strings.HasPrefix(line, "!") { + if strings.HasPrefix(line, "@@") { allow = true - line = strings.TrimSpace(line[1:]) + line = strings.TrimPrefix(line, "@@") + } else if strings.HasPrefix(line, "!") { + allow = true + line = trimAllowWord(strings.TrimSpace(line[1:])) } - line = strings.TrimPrefix(line, "||") + // A plain list often carries a few adblock lines; drop the marker and + // everything from the first adblock separator on, so they survive + // instead of being counted as malformed. + line = cutAdblockTail(strings.TrimPrefix(line, "||")) if !validDomain(line) { stats.Malformed++ continue @@ -374,6 +398,45 @@ func parsePlain(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { return rules, stats, nil } +// cutAdblockTail drops everything from the first adblock separator onwards, so +// a plain list that someone pasted adblock lines into still yields the domain +// instead of a name with "^$|*//" decoration glued to it. A leading "*." is a +// wildcard prefix rather than a separator, and a lone "$" separator is only +// decoration when something follows it. +func cutAdblockTail(s string) string { + for i := 0; i < len(s); i++ { + switch s[i] { + case '^', '|', '/': + return s[:i] + case '$': + if i+1 < len(s) { + return s[:i] + } + case '*': + if i > 0 { + return s[:i] + } + } + } + return s +} + +// trimAllowWord drops a leading allow keyword that is separated from the +// domain by whitespace, so the documented "!allow example.com" and the bare +// "!example.com" mean the same thing, while a domain that merely starts with +// the word (allow.example.com) is left alone. +func trimAllowWord(s string) string { + fields := strings.Fields(s) + if len(fields) < 2 { + return s + } + switch strings.ToLower(fields[0]) { + case "allow", "allowlist", "whitelist": + return strings.TrimSpace(s[len(fields[0]):]) + } + return s +} + func parseRPZ(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { var rules []Rule stats := ParseStats{Bytes: int64(len(data)), Format: FormatRPZ} @@ -381,11 +444,18 @@ func parseRPZ(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { if err != nil { return nil, stats, err } + // A record line with no TTL column comes back from the zone reader + // carrying its own placeholder, which would shadow the TTL the caller + // asked for, so the zone's own default wins for those records. + defaultTTL := z.DefaultTTL + if defaultTTL == 0 { + defaultTTL = opt.DefaultTTL + } seen := make(map[string]bool, len(z.Records)) for _, rec := range z.Records { ttl := rec.TTL - if ttl == 0 && opt.DefaultTTL == 0 { - ttl = opt.DefaultTTL + if ttl == 0 || (ttl == implicitRRTTL && defaultTTL != implicitRRTTL) { + ttl = defaultTTL } rule, ok := rpzRule(rec, ttl, opt) if !ok { @@ -403,19 +473,24 @@ func parseRPZ(data []byte, opt ParseOptions) ([]Rule, ParseStats, error) { return rules, stats, nil } +// implicitRRTTL is the TTL the zone reader stamps on a record whose line +// carries no TTL column. It is a placeholder rather than the file's intent, +// which is why parseRPZ replaces it with the zone's own default. +const implicitRRTTL = 3600 + // rpzRule maps one policy-zone record onto a rule (RFC draft-ietf-dnsop-rpz). // RPZ encodes the response in the rdata: a CNAME to "." means NXDOMAIN, to // "rpz-drop." means drop, to "rpz-passthru." means resolve normally, and // A/AAAA records return that address verbatim. func rpzRule(rec zonefile.Record, ttl uint32, opt ParseOptions) (Rule, bool) { pattern := rec.Name - if !strings.HasPrefix(pattern, "*") && opt.Subdomains { - pattern = "*." + pattern - } target, action, resp := rpzTarget(rec.RR) if target == "" { return Rule{}, false } + if !strings.HasPrefix(pattern, "*") && opt.Subdomains { + pattern = "*." + pattern + } return Rule{Name: pattern, Action: action, Response: resp, TTL: ttl}, true } diff --git a/internal/blocklist/parse_test.go b/internal/blocklist/parse_test.go index 1126780..e23198b 100644 --- a/internal/blocklist/parse_test.go +++ b/internal/blocklist/parse_test.go @@ -242,18 +242,25 @@ func TestParsePlain(t *testing.T) { t.Errorf("exact rules = %v, want %v", got, wantExact) } - // The documented "!allow " spelling is not what the parser - // implements: it only strips the "!" marker, so the word "allow" is left - // in the name and the line is counted as malformed. Real behaviour, - // pinned so a fix shows up as a failure. + // The documented "!allow " spelling is accepted, so the keyword + // does not end up glued to the name. allowForm, allowStats, err := Parse(FormatPlain, []byte("!allow ok.example.com\n"), ParseOptions{Subdomains: true}) if err != nil { t.Fatalf("Parse: %v", err) } - t.Logf("\"!allow ok.example.com\": rules=%v malformed=%d", ruleStrings(allowForm), allowStats.Malformed) - if allowStats.Rules != 0 || allowStats.Malformed != 1 { - t.Errorf("\"!allow x\": rules=%d malformed=%d, want 0/1 (current behaviour)", - allowStats.Rules, allowStats.Malformed) + if got := ruleStrings(allowForm); !sameRules(got, []string{"**.ok.example.com allow ttl=60"}) { + t.Errorf("\"!allow ok.example.com\" rules = %v, malformed=%d", got, allowStats.Malformed) + } + // "!allow" with no name is still nothing at all. + empty, emptyStats, err := Parse(FormatPlain, []byte("!allow\n"), ParseOptions{Subdomains: true}) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if emptyStats.Rules != 0 || emptyStats.Malformed != 1 { + t.Errorf("\"!allow\": rules=%d malformed=%d, want 0/1", emptyStats.Rules, emptyStats.Malformed) + } + if len(empty) != 0 { + t.Errorf("\"!allow\" produced %v", ruleStrings(empty)) } } @@ -415,7 +422,9 @@ func TestParseAutoPicksFormat(t *testing.T) { func TestParseRobustness(t *testing.T) { long := "long" + strings.Repeat("x", 100000) + ".example.com" bigDomain := strings.Repeat("d", 250) + ".example.com" // over the 253-char limit - hugeDomain := strings.Repeat("d", 3000) + ".example.com" + // Over the 4 MiB per-line cap the scanner enforces (see newLineScanner), + // so the parser reports it instead of silently truncating. + hugeDomain := strings.Repeat("d", 5*1024*1024) + ".example.com" listSamples := map[string]string{ "crlf": "0.0.0.0 ads.example.com\r\n0.0.0.0 b.example.com\r\n", @@ -511,18 +520,17 @@ func TestParseRobustness(t *testing.T) { } func TestParseBOMBehaviour(t *testing.T) { - // A UTF-8 BOM on the first line is not stripped by the parsers, so the - // first rule of the list is lost. Real behaviour, pinned. + // A UTF-8 BOM on the first line is stripped, so the first rule of the list + // survives. An operator who saved the list from a Windows editor would + // otherwise silently lose the first entry of every list. const bom = "\ufeff" _, stats, err := Parse(FormatAuto, []byte(bom+"0.0.0.0 ads.example.com\n"), ParseOptions{Subdomains: true}) if err != nil { t.Fatalf("Parse: %v", err) } - t.Logf("BOM sample: format=%s rules=%d malformed=%d notices=%v", - stats.Format, stats.Rules, stats.Malformed, stats.Notices) - if stats.Rules != 0 || stats.Malformed != 1 { - t.Errorf("BOM sample: rules=%d malformed=%d, want 0/1 (current behaviour)", - stats.Rules, stats.Malformed) + if stats.Rules != 1 || stats.Malformed != 0 { + t.Errorf("BOM sample: format=%s rules=%d malformed=%d, want 1/0", + stats.Format, stats.Rules, stats.Malformed) } // Without the BOM the same content parses. @@ -534,14 +542,22 @@ func TestParseBOMBehaviour(t *testing.T) { t.Errorf("control: rules = %d, want 1", ok.Rules) } - // A BOM on an adblock list behaves the same way: the first rule is lost. + // A BOM on an adblock list behaves the same way. abRules, abStats, err := Parse(FormatAdblock, []byte(bom+"||ads.example.com^\n"), ParseOptions{Subdomains: true}) if err != nil { t.Fatalf("Parse: %v", err) } - t.Logf("BOM adblock: rules=%v skipped=%d", ruleStrings(abRules), abStats.Skipped) - if abStats.Rules != 0 { - t.Errorf("BOM adblock rules = %v, want none (current behaviour)", ruleStrings(abRules)) + if got := ruleStrings(abRules); !sameRules(got, []string{"**.ads.example.com block ttl=60"}) { + t.Errorf("BOM adblock rules = %v (skipped=%d)", got, abStats.Skipped) + } + // A BOM in the middle of a file is not a BOM: it stays put and the line + // around it still parses, so a multi-line list is not damaged. + _, mid, err := Parse(FormatPlain, []byte("a.example.com\n"+bom+"b.example.com\n"), ParseOptions{Subdomains: true}) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if mid.Rules != 1 || mid.Malformed != 1 { + t.Errorf("mid-file BOM: rules=%d malformed=%d, want 1/1", mid.Rules, mid.Malformed) } } diff --git a/internal/blocklist/set.go b/internal/blocklist/set.go index 9b74397..7cb4e3f 100644 --- a/internal/blocklist/set.go +++ b/internal/blocklist/set.go @@ -290,6 +290,15 @@ func (b *Builder) Add(r Rule, source string, categories []string) { m[name] = e b.countAction(e, 1) b.noteOrder(source) + // A source that only ever contributed rules (an inline blocklist entry, or + // a list whose bookkeeping was never noted) is still a source: the UI + // lists sources, and a rule with a source nobody has heard of would show + // up as a blank column. + if source != "" { + if _, ok := b.meta[source]; !ok { + b.meta[source] = SourceMeta{Name: source} + } + } } // metaFor reports whether a source already contributed to this build. @@ -622,8 +631,20 @@ func (k PatternKind) String() string { } } -// Pattern renders a pattern of this kind for a stored name. +// Pattern renders a stored name for display. +// +// The wildcard prefix is *not* re-added: a Set stores the prefix-free name and +// the kind already carries the wildcard semantics, so the stored form is the +// form a caller wrote. Use WireName when the prefixed form is needed — to hand +// a rule back to the API, for instance. func (k PatternKind) Pattern(name string) string { + return name +} + +// WireName renders a stored name with the wildcard prefix this kind implies. +// It is the round-trip form: WireName(k, k.Pattern(n)) == n, and feeding it +// back into Add reproduces the original rule. +func (k PatternKind) WireName(name string) string { switch k { case PatternSub: return "*." + name @@ -744,8 +765,9 @@ func NewHolder(s *Set) *Holder { return h } -// Get returns the current set (nil before the first build). -// Get returns the current set, or nil when nothing has been compiled yet. +// Get returns the current set, or nil when nothing has been compiled yet. The +// returned set is immutable and is shared with every other reader, so a caller +// must not modify it. func (h *Holder) Get() *Set { if h == nil { return nil diff --git a/internal/blocklist/set_test.go b/internal/blocklist/set_test.go index de82df7..8725a44 100644 --- a/internal/blocklist/set_test.go +++ b/internal/blocklist/set_test.go @@ -468,13 +468,14 @@ func TestHolderSwapIsAtomic(t *testing.T) { return default: } + // The invariant is that a reader gets *one* of the two + // published sets, not a blend of them. Comparing pointers is + // the only way to say that: asking a question the two sets + // answer differently (does this set match a.example.com?) + // would report a legitimate swap as a torn one. s := h.Get() - if s == nil { - t.Error("holder returned nil") - return - } - if e := s.Match("a.example.com"); e == nil { - t.Error("observed a half-applied set") + if s != old && s != newer { + t.Errorf("holder returned neither published set (%p)", s) return } } diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 6a14cdd..843e70f 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -1,6 +1,7 @@ package cache import ( + "encoding/binary" "errors" "math/rand/v2" "net" @@ -12,6 +13,7 @@ import ( "time" "github.com/miekg/dns" + bolt "go.etcd.io/bbolt" ) // fakeClock is a manually advanced clock so TTL, ageing and serve-stale @@ -868,3 +870,231 @@ func BenchmarkManagerGetUnpack(b *testing.B) { } }) } + +// TestStoreSurvivesProcessRestart is the test the warm-restart path exists for: +// write a snapshot, close the file completely, open a brand new Store on the +// same path and load it into a fresh LRU. Anything that only works because the +// same *Store is still holding the file open fails here. +func TestStoreSurvivesProcessRestart(t *testing.T) { + path := filepath.Join(t.TempDir(), "cache.db") + clk := newClock() + now := clk.Now() + + st, err := OpenStore(StoreOptions{Path: path}) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + warm := NewLRU(100, 2) + for i := range 5 { + n := "r" + strconv.Itoa(i) + ".example.com" + wire, _ := answer(t, n, 600, "10.1.0."+strconv.Itoa(i)).Pack() + warm.Put(key(n), &Entry{ + Wire: wire, TTL: 600, Origin: "upstream", Upstream: "9.9.9.9:53", + CreatedAt: now, ExpiresAt: now.Add(600 * time.Second), StaleUntil: now.Add(630 * time.Second), + Bytes: len(wire), + }) + } + if written, err := st.Snapshot(warm, now); err != nil || written != 5 { + t.Fatalf("Snapshot = %d, %v; want 5, nil", written, err) + } + if err := st.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Second "process". + restarted, err := OpenStore(StoreOptions{Path: path}) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer restarted.Close() + cold := NewLRU(100, 2) + loaded, skipped, err := restarted.Load(cold, clk.Now()) + if err != nil { + t.Fatalf("Load: %v", err) + } + if loaded != 5 || skipped != 0 { + t.Fatalf("Load = %d loaded, %d skipped, want 5/0", loaded, skipped) + } + e, kind := cold.Get(key("r3.example.com"), clk.Now()) + if e == nil || kind != Fresh { + t.Fatalf("restored entry: entry=%v kind=%v, want non-nil/Fresh", e, kind) + } + if e.Upstream != "9.9.9.9:53" { + t.Errorf("Upstream = %q, want 9.9.9.9:53", e.Upstream) + } + // The TTL a client sees must not jump back up across the restart: the + // loader rewrites it to the time actually left. + clk.Advance(120 * time.Second) + if got, want := e.AgeTTL(clk.Now()), uint32(480); got != want { + t.Errorf("AgeTTL after 120s = %d, want %d", got, want) + } +} + +// TestStoreLoadSkipsDamagedRecords feeds Load a file where individual records +// have been scribbled on, truncated or written by a future version. A snapshot +// is a cache, not a source of truth: every one of these must cost exactly one +// entry and none may fail the whole load. +func TestStoreLoadSkipsDamagedRecords(t *testing.T) { + path := filepath.Join(t.TempDir(), "cache.db") + clk := newClock() + now := clk.Now() + + st, err := OpenStore(StoreOptions{Path: path}) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + l := NewLRU(100, 1) + for i := range 4 { + n := "d" + strconv.Itoa(i) + ".example.com" + wire, _ := answer(t, n, 600, "10.2.0."+strconv.Itoa(i)).Pack() + l.Put(key(n), &Entry{ + Wire: wire, TTL: 600, CreatedAt: now, ExpiresAt: now.Add(600 * time.Second), + StaleUntil: now.Add(630 * time.Second), Bytes: len(wire), + }) + } + if written, err := st.Snapshot(l, now); err != nil || written != 4 { + t.Fatalf("Snapshot = %d, %v; want 4, nil", written, err) + } + // Three different kinds of damage, applied to the first three records + // (the keys are packed, so the cursor is the only way to find them). + damaged := map[int][]byte{ + 0: futureVersionRecord, // written by a hypothetical v2 + 1: {recordVersion, 0, 0x01, 0x02, 0x03}, // truncated mid-varint + 2: corruptRecord(t, encodeRecord(nil, &Entry{ // flipped bit + Wire: []byte{0xde, 0xad}, TTL: 1, CreatedAt: now, ExpiresAt: now, StaleUntil: now, + })), + } + if err := st.db.Update(func(tx *bolt.Tx) error { + b := tx.Bucket(bucketCache) + cur := b.Cursor() + k, _ := cur.First() + for i := 0; k != nil; i++ { + if bad, ok := damaged[i]; ok { + if err := b.Put(k, bad); err != nil { + return err + } + } + k, _ = cur.Next() + } + return nil + }); err != nil { + t.Fatalf("seed damaged records: %v", err) + } + if err := st.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + again, err := OpenStore(StoreOptions{Path: path}) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer again.Close() + restored := NewLRU(100, 1) + loaded, skipped, err := again.Load(restored, clk.Now()) + if err != nil { + t.Fatalf("Load on a damaged file returned %v, want the good records to survive", err) + } + if loaded != 1 { + t.Errorf("loaded = %d, want 1 (only the untouched record)", loaded) + } + if skipped != 3 { + t.Errorf("skipped = %d, want 3", skipped) + } + if _, kind := restored.Get(key("d3.example.com"), clk.Now()); kind != Fresh { + t.Errorf("intact record did not survive: kind = %v", kind) + } + if _, kind := restored.Get(key("d0.example.com"), clk.Now()); kind != Miss { + t.Errorf("damaged record was loaded anyway: kind = %v", kind) + } +} + +// futureVersionRecord is a well-formed record from a hypothetical v2 writer. +var futureVersionRecord = func() []byte { + buf := []byte{recordVersion + 1, 0} + buf = binary.AppendUvarint(buf, 0) + return buf +}() + +// corruptRecord flips one bit inside the body so the checksum no longer matches. +func corruptRecord(t *testing.T, rec []byte) []byte { + t.Helper() + out := append([]byte(nil), rec...) + out[len(out)/2] ^= 0x40 + return out +} + +// TestRecordEncodeDecode pins the on-disk record format itself. The round trip +// through bbolt used to break because the encoder and decoder disagreed; this +// test fails at compile-review time rather than at warm-restart time. +func TestRecordEncodeDecode(t *testing.T) { + now := newClock().Now() + in := &Entry{ + Wire: []byte{0x00, 0x01, 0x02, 0x03}, + TTL: 600, + Origin: "upstream", + Upstream: "1.1.1.1:53", + CreatedAt: now, + ExpiresAt: now.Add(600 * time.Second), + StaleUntil: now.Add(630 * time.Second), + Neg: true, + Truncated: true, + } + out, err := decodeRecord(encodeRecord(nil, in)) + if err != nil { + t.Fatalf("decodeRecord: %v", err) + } + if string(out.Wire) != string(in.Wire) { + t.Errorf("Wire = % x, want % x", out.Wire, in.Wire) + } + for _, f := range []struct { + name string + got, want any + }{ + {"TTL", out.TTL, in.TTL}, + {"Origin", out.Origin, in.Origin}, + {"Upstream", out.Upstream, in.Upstream}, + {"Neg", out.Neg, in.Neg}, + {"Truncated", out.Truncated, in.Truncated}, + } { + if f.got != f.want { + t.Errorf("%s = %v, want %v", f.name, f.got, f.want) + } + } + // Times come back as wall-clock instants rebuilt from Unix nanoseconds, so + // they compare with Equal rather than == (the location bits differ). + for _, f := range []struct { + name string + got, want time.Time + }{ + {"CreatedAt", out.CreatedAt, in.CreatedAt}, + {"ExpiresAt", out.ExpiresAt, in.ExpiresAt}, + {"StaleUntil", out.StaleUntil, in.StaleUntil}, + } { + if !f.got.Equal(f.want) { + t.Errorf("%s = %v, want %v", f.name, f.got, f.want) + } + } + + // A record whose times are all zero must survive too: the varints encode + // them as 0 and come back as the zero time, not 1970. + zero, err := decodeRecord(encodeRecord(nil, &Entry{Wire: []byte{0xff}})) + if err != nil { + t.Fatalf("decodeRecord(zero times): %v", err) + } + if !zero.ExpiresAt.IsZero() || !zero.CreatedAt.IsZero() { + t.Errorf("zero times came back as %v/%v", zero.ExpiresAt, zero.CreatedAt) + } + + for name, bad := range map[string][]byte{ + "empty": nil, + "header only": {recordVersion, 0}, + "no wire": encodeRecord(nil, &Entry{}), + "bad checksum": corruptRecord(t, encodeRecord(nil, in)), + "bad version": futureVersionRecord, + "truncated tail": {recordVersion, 0, 0x80}, + } { + if _, err := decodeRecord(bad); !errors.Is(err, errBadRecord) { + t.Errorf("decodeRecord(%s) error = %v, want errBadRecord", name, err) + } + } +} diff --git a/internal/cache/store.go b/internal/cache/store.go index b4adabf..16a1e9f 100644 --- a/internal/cache/store.go +++ b/internal/cache/store.go @@ -3,6 +3,7 @@ package cache import ( "encoding/binary" "fmt" + "hash/crc32" "os" "path/filepath" "sync" @@ -13,6 +14,154 @@ import ( var bucketCache = []byte("cache") +// recordVersion is the on-disk record format version. It is written as the +// first byte of every value so a future format change can be recognised (and +// the record dropped) instead of being silently misparsed. +const recordVersion = 1 + +// Record flags, stored in byte 1 of a record. +const ( + flagNeg = 1 << 0 + flagTruncated = 1 << 1 +) + +// errBadRecord is returned by decodeRecord for anything it cannot trust: a +// short buffer, an unknown version, a failed checksum or trailing garbage. +var errBadRecord = errString("cache: corrupt snapshot record") + +var crc32cTable = crc32.MakeTable(crc32.Castagnoli) + +// encodeRecord serialises one entry into a snapshot value. +// +// The layout is written out here, and not merely implied by the encoder and +// decoder sitting side by side, because the two drifting apart is exactly what +// broke the warm-restart path once already: +// +// byte 0 format version (recordVersion) +// byte 1 flags (flagNeg, flagTruncated) +// uvarint ExpiresAt as Unix nanoseconds (0 = the zero time) +// uvarint StaleUntil as Unix nanoseconds (0 = the zero time) +// uvarint CreatedAt as Unix nanoseconds (0 = the zero time) +// uvarint TTL in seconds +// uvarint len(Origin) + Origin +// uvarint len(Upstream) + Upstream +// rest packed wire-format answer +// uint32 (LE) CRC-32C of every byte above +func encodeRecord(buf []byte, e *Entry) []byte { + buf = append(buf, recordVersion) + var flags uint8 + if e.Neg { + flags |= flagNeg + } + if e.Truncated { + flags |= flagTruncated + } + buf = append(buf, flags) + buf = binary.AppendUvarint(buf, uint64(nanosOf(e.ExpiresAt))) + buf = binary.AppendUvarint(buf, uint64(nanosOf(e.StaleUntil))) + buf = binary.AppendUvarint(buf, uint64(nanosOf(e.CreatedAt))) + buf = binary.AppendUvarint(buf, uint64(e.TTL)) + buf = binary.AppendUvarint(buf, uint64(len(e.Origin))) + buf = append(buf, e.Origin...) + buf = binary.AppendUvarint(buf, uint64(len(e.Upstream))) + buf = append(buf, e.Upstream...) + buf = append(buf, e.Wire...) + return binary.LittleEndian.AppendUint32(buf, crc32.Checksum(buf, crc32cTable)) +} + +// decodeRecord is the inverse of encodeRecord. Every failure mode returns +// errBadRecord: a snapshot is a cache, not a source of truth, so one damaged +// record must cost one entry and not the whole file. +func decodeRecord(buf []byte) (*Entry, error) { + const header = 2 + if len(buf) < header+crc32.Size { + return nil, errBadRecord + } + body := buf[:len(buf)-crc32.Size] + if got, want := binary.LittleEndian.Uint32(buf[len(buf)-crc32.Size:]), crc32.Checksum(body, crc32cTable); got != want { + return nil, errBadRecord + } + if body[0] != recordVersion { + return nil, errBadRecord + } + e := &Entry{Neg: body[1]&flagNeg != 0, Truncated: body[1]&flagTruncated != 0} + rest := body[header:] + + var n int + var ok bool + if e.ExpiresAt, n, ok = timeFromVarint(rest); !ok { + return nil, errBadRecord + } + rest = rest[n:] + if e.StaleUntil, n, ok = timeFromVarint(rest); !ok { + return nil, errBadRecord + } + rest = rest[n:] + if e.CreatedAt, n, ok = timeFromVarint(rest); !ok { + return nil, errBadRecord + } + rest = rest[n:] + + ttl, n := binary.Uvarint(rest) + if n <= 0 { + return nil, errBadRecord + } + e.TTL = uint32(ttl) + rest = rest[n:] + + for _, dst := range []*string{&e.Origin, &e.Upstream} { + s, n, ok := readVarintString(rest) + if !ok { + return nil, errBadRecord + } + *dst = s + rest = rest[n:] + } + if len(rest) == 0 { + return nil, errBadRecord + } + // The buffer belongs to bbolt's mmap, so copy the wire bytes out instead + // of aliasing memory a later transaction may move. + e.Wire = append([]byte(nil), rest...) + e.Bytes = len(e.Wire) + len(e.Origin) + len(e.Upstream) + 96 + return e, nil +} + +// originOrSnapshot keeps whatever origin the record carried (the UI shows it) +// but never stores an empty one. +func originOrSnapshot(origin string) string { + if origin == "" { + return "snapshot" + } + return origin +} + +func nanosOf(t time.Time) int64 { + if t.IsZero() { + return 0 + } + return t.UnixNano() +} + +func timeFromVarint(buf []byte) (time.Time, int, bool) { + v, n := binary.Uvarint(buf) + if n <= 0 { + return time.Time{}, 0, false + } + if v == 0 { + return time.Time{}, n, true + } + return time.Unix(0, int64(v)), n, true +} + +func readVarintString(buf []byte) (string, int, bool) { + l, n := binary.Uvarint(buf) + if n <= 0 || l > uint64(len(buf)-n) { + return "", 0, false + } + return string(buf[n : n+int(l)]), n + int(l), true +} + // Store persists a snapshot of the LRU to a bbolt file so a restart (or a // rolling deploy of a cluster) resumes with a warm cache. // @@ -84,44 +233,34 @@ func (s *Store) Load(l *LRU, now time.Time) (loaded int, skipped int, err error) return nil } cur := b.Cursor() - for k, _ := cur.First(); k != nil; k, _ = cur.Next() { - key, rest, kerr := unpackKey(k) + for k, v := cur.First(); k != nil; k, v = cur.Next() { + key, _, kerr := unpackKey(k) if kerr != nil { skipped++ continue } - expUnix, n := binary.Uvarint(rest) - if n <= 0 { + e, derr := decodeRecord(v) + if derr != nil { skipped++ continue } - staleUnix, n2 := binary.Uvarint(rest[n:]) - if n2 <= 0 { + if !e.ExpiresAt.After(now) { + // The serve-stale window deliberately does not survive a + // restart: stale answers bridge a transient upstream failure + // inside one process lifetime, they are not a licence to + // resurrect an answer from minutes ago. An entry that is + // already past its TTL at load time is dropped instead. skipped++ continue } - wire := rest[n+n2:] - if len(wire) == 0 { - skipped++ - continue - } - expires := time.Unix(0, int64(expUnix)) - staleUntil := time.Unix(0, int64(staleUnix)) - if staleUntil.Before(now) { - // Already outside the serve-stale window: not worth loading. - skipped++ - continue - } - ttl := uint32(expires.Sub(now).Seconds()) - e := &Entry{ - Wire: append([]byte(nil), wire...), - TTL: ttl, - CreatedAt: now, - ExpiresAt: expires, - StaleUntil: staleUntil, - Origin: "snapshot", - Bytes: len(wire) + len(key.Name) + 96, - } + // Re-anchor on the loader's clock: the stored CreatedAt belongs to + // the previous process, and AgeTTL would otherwise age the answer + // by the length of the outage. The TTL is rewritten to the time + // actually left, so the TTL a client sees never jumps up across a + // restart. + e.CreatedAt = now + e.TTL = uint32(e.ExpiresAt.Sub(now) / time.Second) + e.Origin = originOrSnapshot(e.Origin) l.Put(key, e) loaded++ } @@ -157,11 +296,7 @@ func (s *Store) Snapshot(l *LRU, now time.Time) (written int, err error) { dropped++ return true } - val := make([]byte, 0, len(e.Wire)+32) - val = binary.AppendUvarint(val, uint64(e.ExpiresAt.UnixNano())) - val = binary.AppendUvarint(val, uint64(e.StaleUntil.UnixNano())) - val = append(val, e.Wire...) - rows = append(rows, row{key: kb, val: val}) + rows = append(rows, row{key: kb, val: encodeRecord(nil, e)}) return true }) if s.maxEntries > 0 && len(rows) > s.maxEntries { @@ -170,15 +305,11 @@ func (s *Store) Snapshot(l *LRU, now time.Time) (written int, err error) { } err = s.db.Update(func(tx *bolt.Tx) error { - b, err := tx.CreateBucketIfNotExists(bucketCache) - if err != nil { - return err - } // Rebuild the bucket so a shrinking cache really shrinks the file. if err := tx.DeleteBucket(bucketCache); err != nil && err != bolt.ErrBucketNotFound { return err } - b, err = tx.CreateBucketIfNotExists(bucketCache) + b, err := tx.CreateBucketIfNotExists(bucketCache) if err != nil { return err } diff --git a/internal/config/config.go b/internal/config/config.go index 06948ba..e5aafe0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,13 +8,16 @@ package config import ( + "bytes" "errors" "fmt" + "math" "net" "net/netip" "os" "path/filepath" "sort" + "strconv" "strings" "time" @@ -39,14 +42,23 @@ func (d *Duration) UnmarshalYAML(value *yaml.Node) error { *d = Duration(n) return nil } + // time.ParseDuration insists on a unit, so the numeric seconds forms + // (7, 1.5) have to be handled here; without this the documented "a + // bare integer is seconds" rule never worked, because yaml decodes + // every scalar into a string and the old float fallback was + // unreachable. + if f, err := strconv.ParseFloat(s, 64); err == nil { + ns := f * float64(time.Second) + if ns >= float64(math.MaxInt64) || ns < float64(math.MinInt64) { + return fmt.Errorf("invalid duration %q: out of range", s) + } + *d = Duration(time.Duration(ns)) + return nil + } return fmt.Errorf("invalid duration %q", s) } - var f float64 - if err := value.Decode(&f); err != nil { - return fmt.Errorf("invalid duration: %s", value.Value) - } - *d = Duration(time.Duration(f * float64(time.Second))) - return nil + // Not a scalar at all (mapping/sequence): nothing sensible to interpret. + return fmt.Errorf("invalid duration: %s", value.Value) } func (d Duration) MarshalYAML() (any, error) { return time.Duration(d).String(), nil } @@ -560,6 +572,10 @@ func Load(path string) (*Config, error) { if err != nil { return nil, fmt.Errorf("read config: %w", err) } + raw, err = expandEnv(raw) + if err != nil { + return nil, fmt.Errorf("config %s: %w", path, err) + } if err := yaml.Unmarshal(raw, cfg); err != nil { return nil, fmt.Errorf("parse config %s: %w", path, err) } @@ -574,6 +590,127 @@ func Load(path string) (*Config, error) { return cfg, nil } +// expandEnv substitutes ${NAME} and ${NAME:-default} in a config document +// before it is parsed, so a secret can come from the environment (a +// Secret-mounted env var in Kubernetes, for instance) instead of being +// committed next to the rest of the config. +// +// Three rules, each of which exists because breaking it is a real footgun: +// +// - Only the braced forms are touched, so a bare `$` in a URL or a +// blocklist rule survives verbatim. +// - Comments are copied through untouched. A config that *documents* the +// syntax ("# api_keys: ${FANCYDNS_API_KEY}") must still load. +// - An unset variable with no default is an error, not an empty string: +// `api_keys: ["${FANCYDNS_API_KEY}"]` silently expanding to `[""]` is how +// a deployment ends up with no authentication at all. +func expandEnv(raw []byte) ([]byte, error) { + if !bytes.Contains(raw, []byte("${")) { + return raw, nil + } + var b bytes.Buffer + b.Grow(len(raw)) + // Line-at-a-time so the comment state can be tracked: a `#` only opens a + // comment outside quotes, and only when it starts the line or follows + // whitespace. That is YAML's rule, and a config with `#` inside a quoted + // string must not be truncated at it. + for off := 0; off < len(raw); { + line := raw[off:] + if nl := bytes.IndexByte(line, '\n'); nl >= 0 { + line = line[:nl+1] + } + err := expandLine(line, &b) + if err != nil { + return nil, fmt.Errorf("line %d: %w", bytes.Count(raw[:off], []byte("\n"))+1, err) + } + off += len(line) + } + return b.Bytes(), nil +} + +// expandLine appends one line to b, substituting the ${...} references it +// finds outside comments. +// +// The scan tracks quotes so that two rules hold at once: a `#` only opens a +// comment outside quotes and only at the start of a line or after whitespace +// (YAML's rule, so a `#` inside a quoted string is data, not a comment), and a +// reference inside a quoted scalar — which is how `api_keys: ["${KEY}"]` is +// naturally written — is expanded rather than copied through. +func expandLine(line []byte, b *bytes.Buffer) error { + var inSingle, inDouble, escaped, inComment bool + for i := 0; i < len(line); { + c := line[i] + if inComment { + b.WriteByte(c) + i++ + continue + } + switch { + case inDouble: + switch { + case escaped: + escaped = false + case c == '\\': + escaped = true + case c == '"': + inDouble = false + } + case inSingle: + if c == '\'' { + inSingle = false + } + case c == '#' && (i == 0 || line[i-1] == ' ' || line[i-1] == '\t'): + inComment = true + case c == '"': + inDouble = true + case c == '\'': + inSingle = true + case c == '$' && i+1 < len(line) && line[i+1] == '{': + end := bytes.IndexByte(line[i+2:], '}') + if end < 0 { + return fmt.Errorf("unterminated ${ at byte %d", i) + } + spec := string(line[i+2 : i+2+end]) + i += end + 3 + + name, def := spec, "" + hasDefault := false + // colonDefault marks the ":-" form, which substitutes for an empty + // value as well as an unset one (POSIX). A bare "-" default applies + // only when the variable is unset. + colonDefault := false + switch k := strings.Index(spec, ":-"); { + case k >= 0: + name, def, hasDefault, colonDefault = spec[:k], spec[k+2:], true, true + case strings.IndexByte(spec, '-') >= 0: + // A bare `-` is a default too, for an unset (not an empty) + // variable. It splits on the FIRST hyphen, so a variable name + // may not contain one. + k = strings.IndexByte(spec, '-') + name, def, hasDefault = spec[:k], spec[k+1:], true + } + if name == "" { + return fmt.Errorf("empty variable name in ${%s}", spec) + } + v, isSet := os.LookupEnv(name) + switch { + case isSet && (v != "" || hasDefault && !colonDefault): + // v is "" here only for the "-" form with an empty variable, + // which is the whole difference between ${X-y} and ${X:-y}. + b.WriteString(v) + case hasDefault: + b.WriteString(def) + default: + return fmt.Errorf("environment variable %s is not set (use ${%s:-default} to give it a fallback)", name, name) + } + continue + } + b.WriteByte(c) + i++ + } + return nil +} + // WorkingDirOrSet records the config directory so relative paths in the // document resolve consistently regardless of the process cwd. func (c *Config) WorkingDirOrSet(dir string) { diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..0be0e75 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,331 @@ +package config + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "gopkg.in/yaml.v3" +) + +func TestDurationUnmarshalYAMLAccepted(t *testing.T) { + tests := []struct { + name string + node string + want time.Duration + }{ + {"seconds", `5s`, 5 * time.Second}, + {"compound minutes seconds", `1m30s`, 90 * time.Second}, + {"milliseconds", `250ms`, 250 * time.Millisecond}, + {"hours minutes", `1h30m`, 90 * time.Minute}, + {"sub second", `1.5s`, 1500 * time.Millisecond}, + {"microseconds", `10us`, 10 * time.Microsecond}, + {"zero", `0s`, 0}, + {"negative", `-5s`, -5 * time.Second}, + {"padded", `" 30s "`, 30 * time.Second}, + {"empty string is zero", `""`, 0}, + {"null is zero", `null`, 0}, + {"tilde is zero", `~`, 0}, + {"int seconds", `7`, 7 * time.Second}, + {"float seconds", `1.5`, 1500 * time.Millisecond}, + {"negative int seconds", `-7`, -7 * time.Second}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var got struct { + D Duration `yaml:"d"` + } + if err := yaml.Unmarshal([]byte("d: "+tc.node), &got); err != nil { + t.Fatalf("unmarshal %s: %v", tc.node, err) + } + if got.D.D() != tc.want { + t.Errorf("d: %s = %v, want %v", tc.node, got.D.D(), tc.want) + } + }) + } +} + +func TestDurationUnmarshalYAMLRejected(t *testing.T) { + tests := []struct { + name string + node string + mention string + }{ + {"nonsense", `abc`, `"abc"`}, + {"nonsense", `abc`, `"abc"`}, + {"space before unit", `"5 s"`, `"5 s"`}, + {"unknown unit", `10x`, `"10x"`}, + {"overflow", `2562048h`, `"2562048h"`}, + {"float overflow", `1e400s`, `1e400s`}, + {"numeric seconds overflow", `1e30`, `"1e30"`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var got struct { + D Duration `yaml:"d"` + } + err := yaml.Unmarshal([]byte("d: "+tc.node), &got) + if err == nil { + t.Fatalf("unmarshal %s: expected error, got %v", tc.node, got.D.D()) + } + if !strings.Contains(err.Error(), tc.mention) { + t.Errorf("error %q does not mention offending value %s", err, tc.mention) + } + }) + } +} + +func TestDurationUnmarshalYAMLNonScalar(t *testing.T) { + for _, node := range []string{`{a: 1}`, `[1,2]`} { + var got struct { + D Duration `yaml:"d"` + } + if err := yaml.Unmarshal([]byte("d: "+node), &got); err == nil { + t.Errorf("unmarshal d: %s: expected error, got %v", node, got.D.D()) + } + } +} + +func TestDurationMarshalYAMLRoundTrip(t *testing.T) { + for _, d := range []Duration{ + Duration(5 * time.Second), + Duration(90 * time.Second), + Duration(250 * time.Millisecond), + Duration(12 * time.Hour), + Duration(0), + } { + out, err := yaml.Marshal(map[string]Duration{"d": d}) + if err != nil { + t.Fatalf("marshal %v: %v", d.D(), err) + } + var back struct { + D Duration `yaml:"d"` + } + if err := yaml.Unmarshal(out, &back); err != nil { + t.Fatalf("unmarshal %q: %v", out, err) + } + if back.D != d { + t.Errorf("round trip %v via %q: got %v", d.D(), strings.TrimSpace(string(out)), back.D.D()) + } + } +} + +// Duration has no custom JSON unmarshaller; pin what encoding/json does with +// it so a change here is a deliberate one. +func TestDurationJSONForm(t *testing.T) { + var got struct { + D Duration `json:"d"` + } + if err := json.Unmarshal([]byte(`{"d":5}`), &got); err != nil { + t.Fatalf("bare number: %v", err) + } + if got.D.D() != 5*time.Nanosecond { + t.Errorf("bare JSON number = %v (nanoseconds, not seconds)", got.D.D()) + } + if err := json.Unmarshal([]byte(`{"d":"5s"}`), &got); err == nil { + t.Errorf("quoted JSON duration decoded to %v; string form is unsupported", got.D.D()) + } + out, err := json.Marshal(Duration(5 * time.Second)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != "5000000000" { + t.Errorf("json.Marshal = %s, want nanoseconds", out) + } +} + +// clearEnv removes name for the rest of the test, restoring it afterwards. +func clearEnv(t *testing.T, name string) { + t.Helper() + if old, ok := os.LookupEnv(name); ok { + t.Setenv(name, old) // registers the restore-on-cleanup + } + if err := os.Unsetenv(name); err != nil { + t.Fatalf("unset %s: %v", name, err) + } +} + +func TestExpandEnvSubstitution(t *testing.T) { + t.Setenv("FD_TEST_KEY", "s3cr3t") + t.Setenv("FD_TEST_EMPTY", "") + t.Setenv("FD_TEST_NESTED", "${FD_TEST_KEY}") + clearEnv(t, "FD_TEST_MISSING") + tests := []struct { + name, in, want string + }{ + {"double quoted scalar", `api_keys: ["${FD_TEST_KEY}"]` + "\n", `api_keys: ["s3cr3t"]` + "\n"}, + {"plain unquoted scalar", "cert_file: /etc/${FD_TEST_KEY}.pem\n", "cert_file: /etc/s3cr3t.pem\n"}, + {"single quoted scalar", "password: '${FD_TEST_KEY}'\n", "password: 's3cr3t'\n"}, + {"whole value", "password: ${FD_TEST_KEY}\n", "password: s3cr3t\n"}, + {"two references on one line", "x: ${FD_TEST_KEY}/${FD_TEST_KEY}\n", "x: s3cr3t/s3cr3t\n"}, + {"value is not rescanned", "x: ${FD_TEST_NESTED}\n", "x: ${FD_TEST_KEY}\n"}, + {"reference line comment", "# api_keys: ${FD_TEST_KEY}\n", "# api_keys: ${FD_TEST_KEY}\n"}, + {"reference trailing comment", "level: info # set ${FD_TEST_KEY} later\n", "level: info # set ${FD_TEST_KEY} later\n"}, + {"indented comment", "dns:\n # port ${FD_TEST_KEY}\n max_udp_size: 1232\n", "dns:\n # port ${FD_TEST_KEY}\n max_udp_size: 1232\n"}, + {"bare dollar in url", "url: https://x.invalid/?a=$b\n", "url: https://x.invalid/?a=$b\n"}, + {"bare dollar blocklist line", " url: https://h.invalid/hosts?$format=raw$other\n", " url: https://h.invalid/hosts?$format=raw$other\n"}, + {"bare dollar at end of line", "url: https://x.invalid/$\n", "url: https://x.invalid/$\n"}, + {"colon default when unset", "x: ${FD_TEST_MISSING:-fallback}\n", "x: fallback\n"}, + {"hyphen default when unset", "x: [${FD_TEST_MISSING-fallback}]\n", "x: [fallback]\n"}, + {"colon default when empty", "x: ${FD_TEST_EMPTY:-fallback}\n", "x: fallback\n"}, + {"hyphen default when empty stays empty", "x: [${FD_TEST_EMPTY-fallback}]\n", "x: []\n"}, + {"empty default", "x: [${FD_TEST_MISSING:-}]\n", "x: []\n"}, + {"empty value without default", "x: [${FD_TEST_EMPTY}]\n", "x: []\n"}, + {"hash without leading space is data", "x: a#${FD_TEST_KEY}\n", "x: a#s3cr3t\n"}, + {"hash inside double quotes is data", "x: \"# ${FD_TEST_KEY}\"\n", "x: \"# s3cr3t\"\n"}, + {"hash inside single quotes is data", "x: '# ${FD_TEST_KEY}'\n", "x: '# s3cr3t'\n"}, + {"crlf line ending", "x: ${FD_TEST_KEY}\r\ny: 1\r\n", "x: s3cr3t\r\ny: 1\r\n"}, + {"crlf comment", "# ${FD_TEST_KEY}\r\n", "# ${FD_TEST_KEY}\r\n"}, + {"no trailing newline", "x: ${FD_TEST_KEY}", "x: s3cr3t"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := expandEnv([]byte(tc.in)) + if err != nil { + t.Fatalf("expandEnv(%q): %v", tc.in, err) + } + if string(got) != tc.want { + t.Errorf("expandEnv(%q)\n got %q\nwant %q", tc.in, got, tc.want) + } + }) + } +} + +func TestExpandEnvMultiLine(t *testing.T) { + t.Setenv("FD_TEST_KEY", "abc123") + clearEnv(t, "FD_TEST_MISSING") + in := strings.Join([]string{ + "# leading comment ${FD_TEST_KEY}", + "api:", + " auth:", + ` api_keys: ["${FD_TEST_KEY}"] # trailing ${FD_TEST_KEY}`, + " username: ${FD_TEST_MISSING:-admin}", + " password: ${FD_TEST_MISSING-none}", + "", + "metrics:", + " enabled: false # ${FD_TEST_MISSING} in a comment is fine", + "", + }, "\n") + want := strings.Join([]string{ + "# leading comment ${FD_TEST_KEY}", + "api:", + " auth:", + ` api_keys: ["abc123"] # trailing ${FD_TEST_KEY}`, + " username: admin", + " password: none", + "", + "metrics:", + " enabled: false # ${FD_TEST_MISSING} in a comment is fine", + "", + }, "\n") + got, err := expandEnv([]byte(in)) + if err != nil { + t.Fatalf("expandEnv: %v", err) + } + if string(got) != want { + t.Errorf("expandEnv multi-line\n got %q\nwant %q", got, want) + } +} + +func TestExpandEnvWithoutReferencesIsByteIdentical(t *testing.T) { + raw := []byte("# no references here\nurl: https://x.invalid/?a=$b&c=${not_this}\n'# ${FD_TEST_KEY}'\n") + clearEnv(t, "FD_TEST_KEY") + got, err := expandEnv(raw) + if err != nil { + t.Fatalf("expandEnv: %v", err) + } + if !bytes.Equal(got, raw) { + t.Errorf("expandEnv changed a document with no references:\n got %q\nwant %q", got, raw) + } +} + +func TestExpandEnvErrors(t *testing.T) { + clearEnv(t, "FD_TEST_MISSING") + t.Setenv("FD_TEST_KEY", "abc123") + tests := []struct { + name, in string + mentions []string + }{ + {"unset without default", "x: ${FD_TEST_MISSING}\n", []string{"FD_TEST_MISSING", "line 1:"}}, + {"unset on third line", "a: 1\nb: 2\nc: ${FD_TEST_MISSING}\n", []string{"FD_TEST_MISSING", "line 3:"}}, + {"empty-but-set without default", "x: [${FD_TEST_MISSING}]\n", []string{"FD_TEST_MISSING"}}, + {"unterminated reference", "x: ${FD_TEST_KEY\n", []string{"unterminated ${"}}, + {"unterminated on later line", "a: 1\nb: ${FD_TEST_KEY\n", []string{"unterminated ${", "line 2:"}}, + {"empty name", "x: ${}\n", []string{"empty variable name in ${}"}}, + {"empty name with colon default", "x: ${:-fallback}\n", []string{"empty variable name"}}, + {"unterminated inside comment is ignored", "# x: ${FD_TEST_KEY\ny: 1\n", nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := expandEnv([]byte(tc.in)) + if len(tc.mentions) == 0 { + if err != nil { + t.Fatalf("expandEnv(%q) = %v, want success", tc.in, err) + } + if string(got) != tc.in { + t.Errorf("expandEnv(%q) = %q, want it copied verbatim", tc.in, got) + } + return + } + if err == nil { + t.Fatalf("expandEnv(%q) = %q, want error", tc.in, got) + } + for _, m := range tc.mentions { + if !strings.Contains(err.Error(), m) { + t.Errorf("error %q does not mention %q", err, m) + } + } + }) + } +} + +func TestLoadSubstitutesEnvInFile(t *testing.T) { + t.Setenv("FD_TEST_KEY", "long-random-api-key") + t.Setenv("FD_TEST_PW", "p@ss") + path := writeTempConfig(t, `api: + auth: + mode: apikey + api_keys: ["${FD_TEST_KEY}"] + username: 'admin-${FD_TEST_PW}' + password: prefix-${FD_TEST_PW} + # the key is documented here: ${FD_TEST_KEY} +`) + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + want := []string{"long-random-api-key"} + if len(cfg.API.Auth.APIKeys) != 1 || cfg.API.Auth.APIKeys[0] != want[0] { + t.Errorf("api_keys = %q, want %q", cfg.API.Auth.APIKeys, want) + } + if cfg.API.Auth.Username != "admin-p@ss" { + t.Errorf("username = %q, want admin-p@ss", cfg.API.Auth.Username) + } + if cfg.API.Auth.Password != "prefix-p@ss" { + t.Errorf("password = %q, want prefix-p@ss", cfg.API.Auth.Password) + } +} + +func TestLoadUnsetEnvIsFatalAndNamesVariable(t *testing.T) { + clearEnv(t, "FD_TEST_MISSING") + path := writeTempConfig(t, "api:\n auth:\n mode: apikey\n api_keys: [\"${FD_TEST_MISSING}\"]\n") + _, err := Load(path) + if err == nil { + t.Fatal("Load: want error for unset variable") + } + for _, want := range []string{"FD_TEST_MISSING", path} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +func writeTempConfig(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + return path +} diff --git a/internal/resolver/pool.go b/internal/resolver/pool.go index b1b14a7..3b6bc58 100644 --- a/internal/resolver/pool.go +++ b/internal/resolver/pool.go @@ -135,6 +135,11 @@ func (u *Upstream) Success(d time.Duration) { s.latencyN.Add(1) u.recordLatency(d) u.SetError(nil) + // A working upstream closes its own breaker. Without this the breaker + // could only ever be rearmed by the cooldown expiring, so a server that + // recovered in a millisecond stayed excluded from every pick() for the + // whole cooldown window. + s.breakerOpenUntil.Store(0) if u.Health() != HealthUp { u.setHealth(HealthUp) } @@ -443,6 +448,29 @@ func (p *Pool) Exchange(ctx context.Context, q *dns.Msg, want []string, timeout if len(chosen) == 1 { return p.exchangeOne(ctx, st.cfg, chosen[0], q, timeout) } + if st.cfg.Policy != "parallel" && st.cfg.Policy != "" { + // Failover and random are *selection* policies: pick() has already + // ordered (or shuffled) the candidates, so the only correct thing to + // do with the list is try it in order. Fanning out here would send + // every failover query to the secondary as well, which is the one + // thing failover exists to avoid. + var lastErr error + for _, up := range chosen { + msg, addr, err := p.exchangeOne(ctx, st.cfg, up, q, timeout) + if err == nil && msg != nil && msg.Rcode != dns.RcodeServerFailure { + return msg, addr, nil + } + if err != nil { + lastErr = err + } else { + lastErr = fmt.Errorf("upstream %s returned SERVFAIL", addr) + } + } + if lastErr == nil { + lastErr = ErrNoUpstream + } + return nil, "", lastErr + } // Parallel: send to all at once and take the first real answer. The // context is cancelled on return so we do not leak a request to a slow diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go new file mode 100644 index 0000000..58a1ea9 --- /dev/null +++ b/internal/resolver/resolver_test.go @@ -0,0 +1,632 @@ +package resolver + +import ( + "context" + "errors" + "net" + "runtime" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/miekg/dns" + + "git.k8s-git-k6n.net/mats/fancy-dns/internal/config" +) + +// fakeOpt configures a fakeUpstream. +type fakeOpt struct { + ip string // A record handed out in the answer + delay time.Duration // how long the handler pretends to think + truncateUDP bool // answer TC=1 over UDP, forcing the client onto TCP +} + +// fakeUpstream is a real dns.Server on 127.0.0.1, listening for UDP and TCP on +// the same port, so tests exercise the pool's real client path end to end. +type fakeUpstream struct { + addr string + opt fakeOpt + + udpHits atomic.Int64 + tcpHits atomic.Int64 + // broken makes the handler answer with a message that cannot be unpacked, + // which is the fastest way to make the client fail a real exchange. + broken atomic.Bool +} + +// newFakeUpstream starts a fake upstream and stops it when the test ends. +func newFakeUpstream(t *testing.T, opt fakeOpt) *fakeUpstream { + t.Helper() + if opt.ip == "" { + opt.ip = "203.0.113.1" + } + lc, pc := listenSamePort(t) + f := &fakeUpstream{addr: lc.Addr().String(), opt: opt} + + // ActivateAndServe serves either the PacketConn or the Listener, never + // both, so the fake runs one server per transport. + srvs := []*dns.Server{ + {PacketConn: pc, Handler: dns.HandlerFunc(f.handle)}, + {Listener: lc, Handler: dns.HandlerFunc(f.handle)}, + } + stop := make(chan struct{}) + var wg sync.WaitGroup + for _, srv := range srvs { + wg.Add(1) + go func(srv *dns.Server) { + defer wg.Done() + _ = srv.ActivateAndServe() + }(srv) + } + go func() { + <-stop + for _, srv := range srvs { + _ = srv.Shutdown() + } + }() + t.Cleanup(func() { + // close(stop) must happen before wg.Wait(): the serve goroutines only + // returns once the watcher has shut the server down, so waiting first + // would hang the whole package instead of failing this test. + close(stop) + wg.Wait() + }) + return f +} + +// listenSamePort binds a loopback port for TCP and mirrors it for UDP, which is +// what a real recursive server does and what the truncation retry needs. +func listenSamePort(t *testing.T) (net.Listener, net.PacketConn) { + t.Helper() + for attempt := 0; attempt < 5; attempt++ { + lc, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen tcp: %v", err) + } + _, port, err := net.SplitHostPort(lc.Addr().String()) + if err != nil { + _ = lc.Close() + t.Fatalf("split %s: %v", lc.Addr(), err) + } + pc, err := net.ListenPacket("udp", "127.0.0.1:"+port) + if err != nil { + // Someone grabbed the UDP port in between; try a fresh one. + _ = lc.Close() + continue + } + return lc, pc + } + t.Fatal("no loopback port was free for UDP and TCP at once") + return nil, nil +} + +func (f *fakeUpstream) handle(w dns.ResponseWriter, q *dns.Msg) { + tcp := w.RemoteAddr().Network() == "tcp" + if tcp { + f.tcpHits.Add(1) + } else { + f.udpHits.Add(1) + } + if f.broken.Load() { + // A header claiming five questions with no question in the body: the + // client cannot unpack it and reports an error, which is exactly the + // failure path a broken upstream takes in production. The query id is + // echoed, otherwise the client drops the reply and waits for its + // timeout instead of reporting the error. + bad := []byte{byte(q.Id >> 8), byte(q.Id), 0x80, 0, 0, 1, 0, 0, 0, 0, 0, 0} + _, _ = w.Write(append(bad, 3, 'a', 'b')) + return + } + if d := f.opt.delay; d > 0 { + time.Sleep(d) + } + m := new(dns.Msg) + m.SetReply(q) + m.Truncated = !tcp && f.opt.truncateUDP + if q.Question[0].Qtype == dns.TypeA && !m.Truncated { + m.Answer = []dns.RR{&dns.A{ + Hdr: dns.RR_Header{Name: q.Question[0].Name, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 60}, + A: net.ParseIP(f.opt.ip).To4(), + }} + } + _ = w.WriteMsg(m) +} + +// closedPort returns a loopback port with nothing behind it, so an exchange to +// it fails immediately instead of waiting for a timeout. +func closedPort(t *testing.T) string { + t.Helper() + pc, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := pc.LocalAddr().String() + if err := pc.Close(); err != nil { + t.Fatalf("close: %v", err) + } + return addr +} + +func udpServer(addr string) config.UpstreamServer { + return config.UpstreamServer{Addr: addr, Proto: "udp", Weight: 1} +} + +// poolCfg is a parallel pool over the given servers. +func poolCfg(servers ...config.UpstreamServer) config.Upstream { + return config.Upstream{ + Servers: servers, + Policy: "parallel", + MaxAttempts: len(servers), + BreakerThreshold: 3, + BreakerCooldown: config.Duration(30 * time.Second), + } +} + +func query(name string) *dns.Msg { + q := new(dns.Msg) + q.SetQuestion(dns.Fqdn(name), dns.TypeA) + q.RecursionDesired = true + return q +} + +func testCtx(t *testing.T) context.Context { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + t.Cleanup(cancel) + return ctx +} + +// ask runs one exchange and insists it succeeded. +func ask(t *testing.T, p *Pool, want []string, name string) (*dns.Msg, string) { + t.Helper() + resp, addr, err := p.Exchange(testCtx(t), query(name), want, time.Second) + if err != nil { + t.Fatalf("exchange %s: %v", name, err) + } + return resp, addr +} + +// answerIP returns the first A record of a reply. +func answerIP(t *testing.T, m *dns.Msg) string { + t.Helper() + for _, rr := range m.Answer { + if a, ok := rr.(*dns.A); ok { + return a.A.String() + } + } + t.Fatalf("no A record in reply %v", m) + return "" +} + +func TestCircuitBreakerOpensAfterConsecutiveFailures(t *testing.T) { + dead := closedPort(t) + cfg := config.Upstream{ + Servers: []config.UpstreamServer{udpServer(dead)}, + Policy: "parallel", + MaxAttempts: 1, + BreakerThreshold: 2, + BreakerCooldown: config.Duration(30 * time.Second), + } + p := NewPool(cfg) + up := p.Get(dead) + + // First failure: still serving, just degraded. + if _, _, err := p.Exchange(testCtx(t), query("a.test."), nil, time.Second); err == nil { + t.Fatal("exchange to a dead upstream unexpectedly succeeded") + } + if up.BreakerOpen() { + t.Error("breaker open after one failure with a threshold of 2") + } + if up.Health() != HealthDegraded { + t.Errorf("health = %q, want %q after one failure", up.Health(), HealthDegraded) + } + + // Second consecutive failure trips it. + if _, _, err := p.Exchange(testCtx(t), query("b.test."), nil, time.Second); err == nil { + t.Fatal("second exchange to a dead upstream unexpectedly succeeded") + } + if !up.BreakerOpen() { + t.Fatalf("breaker closed after %d consecutive failures with a threshold of 2", up.Stats().Failures) + } + if up.Health() != HealthDown { + t.Errorf("health = %q, want %q while the breaker is open", up.Health(), HealthDown) + } + if got := p.Healthy(); len(got) != 0 { + t.Errorf("Healthy() = %v, want no upstreams while the breaker is open", p) + } + s := p.Stats()[0] + if !s.BreakerOpen || s.Failures != 2 || s.Queries != 2 || s.FailRate != 1 { + t.Errorf("stats = %+v, want 2 queries, 2 failures, fail rate 1 and an open breaker", s) + } + if s.LastError == "" { + t.Error("stats carry no last error after two failures") + } +} + +func TestCircuitBreakerClosesAfterCooldown(t *testing.T) { + dead := closedPort(t) + const cooldown = 60 * time.Millisecond + cfg := config.Upstream{ + Servers: []config.UpstreamServer{udpServer(dead)}, + Policy: "parallel", + MaxAttempts: 1, + BreakerThreshold: 1, + BreakerCooldown: config.Duration(cooldown), + } + p := NewPool(cfg) + up := p.Get(dead) + + if _, _, err := p.Exchange(testCtx(t), query("a.test."), nil, time.Second); err == nil { + t.Fatal("exchange to a dead upstream unexpectedly succeeded") + } + if !up.BreakerOpen() { + t.Fatal("breaker stayed closed with a threshold of 1") + } + + // Still open well inside the cooldown window. + time.Sleep(cooldown / 3) + if !up.BreakerOpen() { + t.Fatal("breaker closed before the cooldown elapsed") + } + + // The package exposes no clock seam, so the cooldown boundary can only be + // crossed with a real sleep; cooldown is the shortest duration the config + // accepts and this keeps the test far below a second. + time.Sleep(cooldown) + if up.BreakerOpen() { + t.Fatal("breaker still open after the cooldown expired") + } + if got := p.Healthy(); len(got) != 1 || got[0] != up { + t.Errorf("Healthy() = %v, want the upstream back in the list", p) + } + if up.Health() != HealthDown { + t.Errorf("health = %q, want %q until the next successful probe", up.Health(), HealthDown) + } +} + +// TestFailedExchangeDoesNotMarkUpstreamHealthy is the regression test for a +// bug where Success(rtt) ran before the error was checked, so a failed exchange +// pinned the upstream at HealthUp and reset the consecutive-failure counter. +func TestFailedExchangeDoesNotMarkUpstreamHealthy(t *testing.T) { + f := newFakeUpstream(t, fakeOpt{ip: "203.0.113.5"}) + cfg := poolCfg(udpServer(f.addr)) + cfg.BreakerThreshold = 2 + p := NewPool(cfg) + up := p.Get(f.addr) + + // A good exchange first, so "up" is a state the upstream can lose. + ask(t, p, nil, "ok.test.") + if up.Health() != HealthUp { + t.Fatalf("health = %q after a good exchange, want %q", up.Health(), HealthUp) + } + + f.broken.Store(true) + if _, _, err := p.Exchange(testCtx(t), query("bad.test."), nil, time.Second); err == nil { + t.Fatal("an unparsable answer must surface as an error") + } + if got := up.Health(); got != HealthDegraded { + t.Errorf("health = %q after a failed exchange, want %q (never %q)", got, HealthDegraded, HealthUp) + } + if s := up.Stats(); s.Queries != 2 || s.Failures != 1 || s.FailRate != 0.5 { + t.Errorf("stats = %+v, want 2 queries, 1 failure, fail rate 0.5", s) + } + if up.LastError() == "" { + t.Error("a failed exchange recorded no error") + } + if up.BreakerOpen() { + t.Error("a single failure tripped a threshold of 2") + } + + // The failure counts: one more consecutive failure opens the breaker even + // though an earlier success reset the counter. + if _, _, err := p.Exchange(testCtx(t), query("bad2.test."), nil, time.Second); err == nil { + t.Fatal("an unparsable answer must surface as an error") + } + if !up.BreakerOpen() { + t.Error("two consecutive failures did not open the breaker") + } + + // And the upstream recovers as soon as it answers again. + f.broken.Store(false) + p.Rebind(poolCfg(udpServer(f.addr))) + ask(t, p, nil, "ok2.test.") + if up.Health() != HealthUp { + t.Errorf("health = %q after recovery, want %q", up.Health(), HealthUp) + } + if up.BreakerOpen() { + t.Error("breaker still open after a successful exchange") + } + if up.LastError() != "" { + t.Errorf("last error %q survived a success", up.LastError()) + } +} + +func TestParallelPolicyAnswersFromTheFastestUpstream(t *testing.T) { + fast := newFakeUpstream(t, fakeOpt{ip: "203.0.113.10"}) + slow1 := newFakeUpstream(t, fakeOpt{ip: "203.0.113.11", delay: 250 * time.Millisecond}) + slow2 := newFakeUpstream(t, fakeOpt{ip: "203.0.113.12", delay: 400 * time.Millisecond}) + p := NewPool(poolCfg(udpServer(fast.addr), udpServer(slow1.addr), udpServer(slow2.addr))) + + start := time.Now() + resp, addr := ask(t, p, nil, "race.test.") + elapsed := time.Since(start) + + if addr != fast.addr { + t.Errorf("pool answered from %s, want the fastest upstream %s", addr, fast.addr) + } + if ip := answerIP(t, resp); ip != "203.0.113.10" { + t.Errorf("answer = %s, want the fast upstream's record", ip) + } + if elapsed >= 250*time.Millisecond { + t.Errorf("exchange took %s, so it waited for a slow upstream instead of racing", elapsed) + } + if slow1.udpHits.Load() == 0 || slow2.udpHits.Load() == 0 { + t.Errorf("parallel policy did not query every upstream (hits: %d, %d)", + slow1.udpHits.Load(), slow2.udpHits.Load()) + } +} + +// TestParallelPolicyConcurrentExchanges is a race-detector stress test: a burst +// of queries sharing the pool's memoised dns.Client and mutable health state. +func TestParallelPolicyConcurrentExchanges(t *testing.T) { + a := newFakeUpstream(t, fakeOpt{ip: "203.0.113.20"}) + b := newFakeUpstream(t, fakeOpt{ip: "203.0.113.21"}) + p := NewPool(poolCfg(udpServer(a.addr), udpServer(b.addr))) + + // One client per upstream, reused for every query. + up := p.Get(a.addr) + if p.clientFor(up) != p.clientFor(up) { + t.Error("clientFor built a second dns.Client for the same upstream") + } + + const goroutines, perGoroutine = 16, 4 + var wg sync.WaitGroup + errs := make(chan error, goroutines*perGoroutine) + for g := 0; g < goroutines; g++ { + wg.Add(1) + go func(g int) { + defer wg.Done() + for i := 0; i < perGoroutine; i++ { + resp, addr, err := p.Exchange(testCtx(t), query("burst.test."), nil, time.Second) + if err != nil { + errs <- err + return + } + if addr != a.addr && addr != b.addr { + errs <- errors.New("answered by " + addr) + return + } + if ip := answerIP(t, resp); ip != "203.0.113.20" && ip != "203.0.113.21" { + errs <- errors.New("unexpected answer " + ip) + return + } + } + }(g) + } + wg.Wait() + close(errs) + for err := range errs { + t.Error(err) + } + + // The parallel policy asks *both* upstreams and keeps the first answer, so + // every query produces two requests. The upper bound holds because the + // winner cancels the context the loser is on, which can drop a request + // that was already in flight — hence "at least one, at most two". + queries := goroutines * perGoroutine + if got := a.udpHits.Load() + b.udpHits.Load(); got <= int64(queries) || got > 2*int64(queries) { + t.Errorf("upstreams answered %d queries, want between %d and %d", got, queries, 2*queries) + } + if s := p.Get(a.addr).Stats(); s.Queries == 0 { + t.Error("no query was recorded against the first upstream") + } +} + +func TestTruncatedUDPAnswerIsRetriedOverTCP(t *testing.T) { + f := newFakeUpstream(t, fakeOpt{ip: "203.0.113.30", truncateUDP: true}) + p := NewPool(poolCfg(udpServer(f.addr))) + + resp, addr := ask(t, p, nil, "big.test.") + if addr != f.addr { + t.Errorf("answered by %s, want %s", addr, f.addr) + } + if resp.Truncated { + t.Fatal("pool handed back the truncated UDP answer instead of retrying over TCP") + } + if ip := answerIP(t, resp); ip != "203.0.113.30" { + t.Errorf("answer = %s, want the full record from the TCP retry", ip) + } + if f.udpHits.Load() == 0 { + t.Error("the query never went out over UDP") + } + if f.tcpHits.Load() == 0 { + t.Error("the truncated answer was not retried over TCP") + } +} + +func TestFailoverPolicyWalksTheListInOrder(t *testing.T) { + first := newFakeUpstream(t, fakeOpt{ip: "203.0.113.40"}) + second := newFakeUpstream(t, fakeOpt{ip: "203.0.113.41"}) + cfg := config.Upstream{ + Servers: []config.UpstreamServer{udpServer(first.addr), udpServer(second.addr)}, + Policy: "failover", + MaxAttempts: 2, + BreakerThreshold: 3, + BreakerCooldown: config.Duration(30 * time.Second), + } + p := NewPool(cfg) + + // The primary serves every query while it is healthy. + for i := 0; i < 3; i++ { + resp, addr := ask(t, p, nil, "failover.test.") + if addr != first.addr { + t.Fatalf("query %d answered by %s, want the primary %s", i, addr, first.addr) + } + if ip := answerIP(t, resp); ip != "203.0.113.40" { + t.Fatalf("query %d answered %s, want the primary's record", i, ip) + } + } + if second.udpHits.Load() != 0 { + t.Errorf("the secondary took %d queries while the primary was healthy", second.udpHits.Load()) + } + + // Once the primary starts failing the secondary takes over. + first.broken.Store(true) + resp, addr := ask(t, p, nil, "failover.test.") + if addr != second.addr { + t.Errorf("answered by %s, want the secondary %s once the primary fails", addr, second.addr) + } + if ip := answerIP(t, resp); ip != "203.0.113.41" { + t.Errorf("answer = %s, want the secondary's record", ip) + } + if s := p.Get(first.addr).Stats(); s.Failures != 1 { + t.Errorf("primary stats = %+v, want exactly one recorded failure", s) + } + + // And the pool goes back to the primary when it recovers. + first.broken.Store(false) + if _, addr := ask(t, p, nil, "failover.test."); addr != first.addr { + t.Errorf("answered by %s after the primary recovered, want %s", addr, first.addr) + } +} + +// TestRebindUnderConcurrentQueries hammers Rebind while queries, health +// snapshots and latency reads run in parallel, so -race covers the copy-on-write +// hand-over. Afterwards the live state of an unchanged server must have +// survived and a changed server list must be visible. +func TestRebindUnderConcurrentQueries(t *testing.T) { + a := newFakeUpstream(t, fakeOpt{ip: "203.0.113.50"}) + b := newFakeUpstream(t, fakeOpt{ip: "203.0.113.51"}) + c := newFakeUpstream(t, fakeOpt{ip: "203.0.113.52"}) + + only := poolCfg(udpServer(a.addr)) + base := poolCfg(udpServer(a.addr), udpServer(b.addr)) + alt := poolCfg(udpServer(a.addr), udpServer(c.addr)) + + p := NewPool(only) + ask(t, p, nil, "rebind.test.") + before := p.All()[0] + if before.Health() != HealthUp { + t.Fatalf("health = %q before the storm, want %q", before.Health(), HealthUp) + } + queriesBefore := before.Stats().Queries + + stop := make(chan struct{}) + var seen sync.Map // address -> true, every list the readers ever observed + var wg sync.WaitGroup + for r := 0; r < 4; r++ { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + for _, u := range p.All() { + seen.Store(u.Addr(), true) + } + p.Healthy() + p.Stats() + if _, _, err := p.Exchange(testCtx(t), query("storm.test."), nil, time.Second); err != nil { + t.Errorf("exchange during rebind: %v", err) + return + } + runtime.Gosched() + } + }() + } + for i := 0; i < 25; i++ { + p.Rebind(alt) + runtime.Gosched() + p.Rebind(base) + } + close(stop) + wg.Wait() + + // The unchanged server kept its live health and latency state. Rebind + // always builds a fresh immutable Upstream shell — the *stats* are what + // carry over, so that is what is asserted here, not the pointer. + p.Rebind(only) + after := p.All()[0] + if after.stats != before.stats { + t.Error("Rebind did not hand the live stats to the new upstream value") + } + if got := after.Health(); got != HealthUp { + t.Errorf("health = %q after the storm, want %q", got, HealthUp) + } + if got := after.Stats().Queries; got < queriesBefore { + t.Errorf("query counter went backwards across Rebind: %d < %d", got, queriesBefore) + } + if avg, p95 := after.LatencyStats(); avg <= 0 || p95 <= 0 { + t.Errorf("latency stats = avg %v p95 %v, want the samples recorded before the storm", avg, p95) + } + + // The changed list is visible. + p.Rebind(alt) + if p.Get(c.addr) == nil { + t.Error("the added upstream is missing after Rebind") + } + if p.Get(b.addr) != nil { + t.Error("the removed upstream is still in the pool") + } + if got := len(p.All()); got != 2 { + t.Errorf("pool holds %d upstreams, want 2", got) + } + for _, addr := range []string{b.addr, c.addr} { + if _, ok := seen.Load(addr); !ok { + t.Errorf("no reader ever observed %s while Rebind swapped the list", addr) + } + } + if !strings.Contains(p.String(), c.addr) { + t.Errorf("pool %s does not mention the new upstream %s", p, c.addr) + } +} + +func TestExchangeRestrictsToNamedUpstreams(t *testing.T) { + a := newFakeUpstream(t, fakeOpt{ip: "203.0.113.60"}) + b := newFakeUpstream(t, fakeOpt{ip: "203.0.113.61"}) + p := NewPool(poolCfg(udpServer(a.addr), udpServer(b.addr))) + + _, addr := ask(t, p, []string{b.addr}, "restricted.test.") + if addr != b.addr { + t.Errorf("answered by %s, want the named upstream %s", addr, b.addr) + } + if a.udpHits.Load() != 0 { + t.Errorf("the unnamed upstream took %d queries", a.udpHits.Load()) + } + + // A name that matches nothing falls back to the whole pool rather than + // black-holing the client. + _, addr = ask(t, p, []string{"nope.invalid"}, "fallback.test.") + if addr != a.addr && addr != b.addr { + t.Errorf("answered by %s, want any configured upstream", addr) + } + + // An empty pool has nothing to offer. + empty := NewPool(config.Upstream{Policy: "parallel"}) + if _, _, err := empty.Exchange(testCtx(t), query("nowhere.test."), nil, time.Second); !errors.Is(err, ErrNoUpstream) { + t.Errorf("err = %v, want %v", err, ErrNoUpstream) + } +} + +func TestHealthCheckProbesEveryUpstream(t *testing.T) { + live := newFakeUpstream(t, fakeOpt{ip: "203.0.113.70"}) + dead := closedPort(t) + cfg := poolCfg(udpServer(live.addr), udpServer(dead)) + cfg.BreakerThreshold = 1 + p := NewPool(cfg) + + if n := p.HealthCheck(testCtx(t), time.Second); n != 1 { + t.Errorf("HealthCheck reported %d healthy upstreams, want 1", n) + } + if got := p.Get(live.addr).Health(); got != HealthUp { + t.Errorf("health of the live upstream = %q, want %q", got, HealthUp) + } + if !p.Get(dead).BreakerOpen() { + t.Error("the dead upstream was not taken out of rotation") + } +}