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") + } +}