more stuff

internal/acl/acl.go

@@ -327,6 +327,12 @@ func matchPTR(n, suffix string) bool {
 	if suffix == "" || len(n) < len(suffix) || !strings.HasSuffix(n, suffix) {
 		return false
 	}
+	// A suffix written with its own leading dot (".lan.") is the natural way to
+	// say "anything under lan" and the console's own hint suggests it. Its dot
+	// is the label boundary, so there is nothing more to check.
+	if suffix[0] == '.' {
+		return true
+	}
 	// The suffix has to start a label: it is either the whole name, or the
 	// character in front of it is a dot.
 	rest := n[:len(n)-len(suffix)]

internal/acl/acl_test.go

@@ -381,6 +381,12 @@ func TestMatchPTRLabelBoundary(t *testing.T) {
 		{"empty suffix", "host.example.com.", "", false},
 		{"empty name", "", "example.com.", false},
 		{"a hyphen is not a label boundary", "host-example.com.", "example.com.", false},
+		// The console suggests ".lan" as a suffix; the leading dot was being
+		// counted as part of the label, so it never matched anything.
+		{"leading-dot suffix matches a name under it", "host.lan.", ".lan.", true},
+		{"leading-dot suffix matches deeper names", "a.b.lan.", ".lan.", true},
+		{"leading-dot suffix does not match the apex", "lan.", ".lan.", false},
+		{"leading-dot suffix still respects the label", "hostlan.", ".lan.", false},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

internal/acl/explain.go

@@ -0,0 +1,414 @@
+package acl
+
+import (
+	"fmt"
+	"net/netip"
+	"strings"
+
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
+)
+
+// Finding is one observation about a group, tied to the field it concerns so
+// an editor can show it next to the input.
+type Finding struct {
+	Field   string `json:"field"`
+	Message string `json:"message"`
+}
+
+// Analysis is what Analyze says about a group that is structurally valid.
+//
+// It exists for people who do not know what the settings do. The engine's
+// behaviour is not always what a setting's name suggests (an allow list is an
+// allowlist-only mode, a rate limit of 0 exempts the group from the default),
+// so the analysis states the effective behaviour in plain words and flags the
+// settings that are legal but almost certainly not what was meant.
+type Analysis struct {
+	// Summary is the group in plain English, one sentence per line.
+	Summary []string `json:"summary"`
+	// Warnings are legal settings that probably do not do what was intended.
+	Warnings []Finding `json:"warnings"`
+	// Notes are facts worth knowing that are not mistakes.
+	Notes []Finding `json:"notes"`
+	// Unreachable is set when no client can ever land in the group, because
+	// the groups above it claim everything it selects.
+	Unreachable bool `json:"unreachable"`
+}
+
+// Context is the group under review and everything around it.
+type Context struct {
+	// Groups is the full evaluation order, with the group under review in it.
+	Groups []config.Group
+	// Index is the position of the group under review within Groups.
+	Index int
+	// DefaultRateLimit is acl.default_rate_limit (queries/second/client).
+	DefaultRateLimit int
+	// DefaultPolicy is what a client that matches no group gets.
+	DefaultPolicy string
+	// Upstreams are the configured upstream addresses. Nil means unknown, and
+	// the upstream check is skipped.
+	Upstreams []string
+}
+
+// Analyze explains Groups[Index]. It does not validate: a group that fails
+// validation is reported by the caller's own checks, and Analyze copes with
+// whatever it is handed.
+func Analyze(c Context) Analysis {
+	a := Analysis{Summary: []string{}, Warnings: []Finding{}, Notes: []Finding{}}
+	if c.Index < 0 || c.Index >= len(c.Groups) {
+		return a
+	}
+	g := c.Groups[c.Index]
+	warn := func(field, format string, args ...any) {
+		a.Warnings = append(a.Warnings, Finding{field, fmt.Sprintf(format, args...)})
+	}
+	note := func(field, format string, args ...any) {
+		a.Notes = append(a.Notes, Finding{field, fmt.Sprintf(format, args...)})
+	}
+
+	a.Summary = append(a.Summary, whoSentence(g), c.orderSentence())
+
+	// What the group does.
+	switch g.Policy {
+	case "deny":
+		a.Summary = append(a.Summary, "Every query from these clients is refused (REFUSED). Nothing below applies to them.")
+		for _, ignored := range []struct {
+			field string
+			set   bool
+		}{
+			{"allow_domains", len(g.AllowDomains) > 0}, {"deny_domains", len(g.DenyDomains) > 0},
+			{"skip_blocklist", g.SkipBlocklist}, {"skip_cache", g.SkipCache},
+			{"rate_limit", g.RateLimit > 0}, {"max_concurrent", g.MaxConcurrent > 0},
+			{"upstreams", len(g.Upstreams) > 0},
+		} {
+			if ignored.set {
+				note(ignored.field, "Ignored: this group's policy is deny, which refuses the query before anything else is looked at.")
+			}
+		}
+	case "block_only":
+		a.Summary = append(a.Summary, "Queries are resolved normally, and names on a blocklist are blocked.")
+		warn("policy", "block_only currently behaves exactly like allow: both resolve everything and both apply the blocklists. Only deny changes what the group can do. Pick allow unless you are keeping block_only for a reason.")
+	default:
+		a.Summary = append(a.Summary, "Queries are resolved normally, and names on a blocklist are blocked.")
+	}
+
+	if g.Policy != "deny" {
+		a.analyzeBehaviour(c, g, warn, note)
+	}
+	a.analyzeMatch(g, warn, note)
+	a.analyzeDomains(g, warn, note)
+	a.analyzeOrder(c, warn)
+	return a
+}
+
+func (a *Analysis) analyzeBehaviour(c Context, g config.Group, warn, note func(string, string, ...any)) {
+	if len(g.AllowDomains) > 0 {
+		a.Summary = append(a.Summary, fmt.Sprintf("Allowlist mode: ONLY %s can be looked up. Every other name is answered as blocked.", joinShort(g.AllowDomains)))
+		note("allow_domains", "A non-empty allow list turns the group into allowlist-only mode. It does not exempt names from the blocklists; use \"Skip blocklists\" for that.")
+	}
+	if len(g.DenyDomains) > 0 {
+		a.Summary = append(a.Summary, fmt.Sprintf("%s is always blocked for these clients, whatever the blocklists say.", joinShort(g.DenyDomains)))
+	}
+	if g.SkipBlocklist {
+		a.Summary = append(a.Summary, "Blocklists are ignored for these clients: ads and trackers resolve normally.")
+	}
+	if g.SkipCache {
+		a.Summary = append(a.Summary, "Nothing is served from or stored in the cache for these clients; every query goes upstream.")
+		note("skip_cache", "Every query from this group costs an upstream round trip, so it is slower and adds load. Use it for debugging, not as a default.")
+	}
+	switch {
+	case g.RateLimit > 0:
+		a.Summary = append(a.Summary, fmt.Sprintf("Each client address may send %d queries per second; above that, queries are refused.", g.RateLimit))
+	case c.DefaultRateLimit > 0:
+		a.Summary = append(a.Summary, "There is no rate limit for these clients.")
+		warn("rate_limit", "0 means unlimited here, not \"use the default\": this group is exempt from the default limit of %d queries per second per client.", c.DefaultRateLimit)
+	}
+	if g.MaxConcurrent > 0 {
+		a.Summary = append(a.Summary, fmt.Sprintf("A client may have at most %d queries in flight; the excess get SERVFAIL.", g.MaxConcurrent))
+	}
+	if g.LogQueries {
+		a.Summary = append(a.Summary, "Every query from these clients is written to the access log, even when logging is otherwise sampled or limited to blocked queries.")
+	}
+	if len(g.Upstreams) > 0 {
+		a.Summary = append(a.Summary, fmt.Sprintf("Queries go to %s instead of the default upstreams.", joinShort(g.Upstreams)))
+		note("upstreams", "Conditional-forwarding zones (upstream.forward_zones) still take precedence over a group's upstreams.")
+		if c.Upstreams != nil {
+			known := map[string]bool{}
+			for _, u := range c.Upstreams {
+				known[strings.ToLower(u)] = true
+			}
+			var unknown []string
+			for _, u := range g.Upstreams {
+				if !known[strings.ToLower(u)] {
+					unknown = append(unknown, u)
+				}
+			}
+			switch {
+			case len(unknown) == len(g.Upstreams):
+				warn("upstreams", "None of these addresses is a configured upstream, so the setting is ignored and this group uses ALL upstreams. Use an address exactly as it appears on the Upstreams page.")
+			case len(unknown) > 0:
+				warn("upstreams", "%s is not a configured upstream and is ignored.", joinShort(unknown))
+			}
+		}
+	}
+}
+
+func (a *Analysis) analyzeMatch(g config.Group, warn, note func(string, string, ...any)) {
+	if g.Match.Any {
+		warn("match.any", "This group matches every client, so any addresses, suffixes or tags below it are ignored, and so are all groups after it.")
+	}
+	for _, s := range g.Match.CIDRs {
+		p, err := netip.ParsePrefix(strings.TrimSpace(s))
+		if err != nil {
+			continue
+		}
+		if p != p.Masked() {
+			warn("match.cidrs", "%s has host bits set; it is treated as %s.", s, p.Masked())
+		}
+		if (p.Addr().Is4() && p.Bits() < 8) || (!p.Addr().Is4() && p.Bits() < 16) {
+			warn("match.cidrs", "%s is a very wide range: it covers a large share of the address space, probably more than you meant.", s)
+		}
+	}
+	if len(g.Match.PTRSuffixes) > 0 || len(g.Match.Tags) > 0 {
+		note("match", "Reverse-DNS suffixes and tags depend on the client's PTR record. The first queries from a client whose name is not yet cached are matched on address alone, so use a CIDR when the match must be reliable.")
+	}
+	for _, t := range g.Match.Tags {
+		if strings.Contains(t, ".") {
+			warn("match.tags", "Tag %q contains a dot. A tag is a single label of the client's reverse name (\"laptop\" for laptop.lan.example.net), so this never matches. Use a reverse-DNS suffix for a domain.", t)
+		}
+	}
+	for _, s := range g.Match.PTRSuffixes {
+		if strings.ContainsAny(s, "*/ ") {
+			warn("match.ptr_suffixes", "%q is not a plain domain suffix; wildcards and paths are not understood here, so this never matches.", s)
+		}
+	}
+}
+
+func (a *Analysis) analyzeDomains(g config.Group, warn, note func(string, string, ...any)) {
+	lint := func(field string, patterns []string) {
+		for _, raw := range patterns {
+			p := strings.ToLower(strings.TrimSpace(raw))
+			switch {
+			case p == "":
+			case strings.Contains(p, "://") || strings.Contains(p, "/"):
+				warn(field, "%q looks like a URL. Use only the host name, e.g. example.com.", raw)
+			case strings.ContainsAny(p, " \t"):
+				warn(field, "%q contains a space, so it never matches a name.", raw)
+			case strings.HasPrefix(p, "."):
+				warn(field, "%q starts with a dot, which is matched literally and never matches. Write *.%s to cover the name and everything under it.", raw, strings.TrimLeft(p, "."))
+			case p == "*" || p == "**" || p == "*.*":
+				if field == "deny_domains" {
+					warn(field, "%q blocks every name for this group.", raw)
+				} else {
+					warn(field, "%q allows every name, so the allow list restricts nothing.", raw)
+				}
+			case strings.Contains(strings.TrimPrefix(strings.TrimPrefix(p, "**."), "*."), "*"):
+				warn(field, "%q uses a wildcard somewhere other than the start, which is not supported and never matches. Only a leading *. or **. works.", raw)
+			}
+		}
+	}
+	lint("allow_domains", g.AllowDomains)
+	lint("deny_domains", g.DenyDomains)
+
+	if len(g.AllowDomains) > 0 || len(g.DenyDomains) > 0 {
+		note("deny_domains", "*.example.com and **.example.com mean the same thing in these lists: the name itself and everything under it. Write example.com for the exact name only.")
+	}
+	if len(g.AllowDomains) > 0 && len(g.DenyDomains) > 0 {
+		allow, deny := NewDomainSet(g.AllowDomains), NewDomainSet(g.DenyDomains)
+		for _, p := range g.AllowDomains {
+			name := strings.TrimPrefix(strings.TrimPrefix(strings.ToLower(strings.TrimSpace(p)), "**."), "*.")
+			if name != "" && deny.Match(name) && allow.Match(name) {
+				warn("allow_domains", "%q is also covered by the deny list, and deny wins, so it stays blocked.", p)
+			}
+		}
+	}
+}
+
+// analyzeOrder reports groups that shadow this one, and groups this one would
+// shadow. Evaluation is first match wins, so a group whose clients are all
+// claimed higher up is dead weight.
+func (a *Analysis) analyzeOrder(c Context, warn func(string, string, ...any)) {
+	g := c.Groups[c.Index]
+	if g.Match.Any {
+		// The group is the catch-all: the warning on match.any covers it, and
+		// everything after it is reported below.
+	} else if covered, by, all := coverage(g, c.Groups[:c.Index]); all {
+		a.Unreachable = true
+		warn("match", "No client can ever land in this group: everything it selects is already claimed by %q above it, which wins.", by)
+	} else {
+		for _, item := range covered {
+			warn("match", "%s is already claimed by %q above this group, so those clients use that group's settings instead.", item.item, item.by)
+		}
+	}
+	for j := c.Index + 1; j < len(c.Groups); j++ {
+		later := c.Groups[j]
+		with := coverageAll(later, c.Groups[:j])
+		without := coverageAll(later, append(append([]config.Group(nil), c.Groups[:c.Index]...), c.Groups[c.Index+1:j]...))
+		if with && !without {
+			warn("match", "Saving this group makes %q unreachable: it now claims everything that group selects.", later.Name)
+		}
+	}
+}
+
+type claimed struct{ item, by string }
+
+// coverage reports which of g's selectors a group in prior already claims,
+// and whether that is all of them (so g can never match anyone).
+func coverage(g config.Group, prior []config.Group) (items []claimed, first string, all bool) {
+	total := len(g.Match.CIDRs) + len(g.Match.PTRSuffixes) + len(g.Match.Tags)
+	if total == 0 {
+		return nil, "", false
+	}
+	for _, p := range prior {
+		if p.Match.Any {
+			return nil, p.Name, true
+		}
+	}
+	for _, s := range g.Match.CIDRs {
+		c, err := netip.ParsePrefix(strings.TrimSpace(s))
+		if err != nil {
+			continue
+		}
+		for _, p := range prior {
+			if by := coveringCIDR(c, p); by {
+				items = append(items, claimed{s, p.Name})
+				break
+			}
+		}
+	}
+	for _, s := range g.Match.PTRSuffixes {
+		sfx := normSuffix(s)
+		for _, p := range prior {
+			if coveringSuffix(sfx, p) {
+				items = append(items, claimed{s, p.Name})
+				break
+			}
+		}
+	}
+	for _, t := range g.Match.Tags {
+		t = strings.ToLower(strings.TrimSpace(t))
+		for _, p := range prior {
+			if containsFold(p.Match.Tags, t) {
+				items = append(items, claimed{"tag " + t, p.Name})
+				break
+			}
+		}
+	}
+	if len(items) == total {
+		first = items[0].by
+		return items, first, true
+	}
+	return items, "", false
+}
+
+func coverageAll(g config.Group, prior []config.Group) bool {
+	if g.Match.Any {
+		return false
+	}
+	_, _, all := coverage(g, prior)
+	return all
+}
+
+func coveringCIDR(c netip.Prefix, p config.Group) bool {
+	for _, s := range p.Match.CIDRs {
+		q, err := netip.ParsePrefix(strings.TrimSpace(s))
+		if err != nil {
+			continue
+		}
+		if q.Bits() <= c.Bits() && q.Contains(c.Masked().Addr()) {
+			return true
+		}
+	}
+	return false
+}
+
+func coveringSuffix(sfx string, p config.Group) bool {
+	for _, s := range p.Match.PTRSuffixes {
+		earlier := normSuffix(s)
+		// Anything under sfx is under earlier when sfx ends with earlier at a
+		// label boundary; the same test the matcher itself uses.
+		if matchPTR(sfx, earlier) {
+			return true
+		}
+	}
+	return false
+}
+
+func normSuffix(s string) string {
+	s = strings.ToLower(strings.TrimSpace(s))
+	if s != "" && !strings.HasSuffix(s, ".") {
+		s += "."
+	}
+	return s
+}
+
+func containsFold(list []string, want string) bool {
+	for _, s := range list {
+		if strings.EqualFold(strings.TrimSpace(s), want) {
+			return true
+		}
+	}
+	return false
+}
+
+func whoSentence(g config.Group) string {
+	if g.Match.Any {
+		return "Applies to every client."
+	}
+	var parts []string
+	if len(g.Match.CIDRs) > 0 {
+		parts = append(parts, "whose address is in "+joinShort(g.Match.CIDRs))
+	}
+	if len(g.Match.PTRSuffixes) > 0 {
+		parts = append(parts, "whose reverse-DNS name ends in "+joinShort(g.Match.PTRSuffixes))
+	}
+	if len(g.Match.Tags) > 0 {
+		parts = append(parts, "tagged "+joinShort(g.Match.Tags))
+	}
+	if len(parts) == 0 {
+		return "Selects no clients yet."
+	}
+	return "Applies to clients " + strings.Join(parts, ", or ") + "."
+}
+
+func (c Context) orderSentence() string {
+	pos, n := c.Index+1, len(c.Groups)
+	switch {
+	case pos == 1 && n == 1:
+		return "It is the only group; clients that match nothing get the default policy (" + c.defaultPolicy() + ")."
+	case pos == 1:
+		return fmt.Sprintf("It is checked first of %d groups; a client matching it never reaches the others.", n)
+	default:
+		return fmt.Sprintf("It is checked %s of %d groups, after the %d above it; clients matching none get the default policy (%s).", ordinal(pos), n, pos-1, c.defaultPolicy())
+	}
+}
+
+func (c Context) defaultPolicy() string {
+	if c.DefaultPolicy == "" {
+		return "allow"
+	}
+	return c.DefaultPolicy
+}
+
+func ordinal(n int) string {
+	switch n {
+	case 2:
+		return "second"
+	case 3:
+		return "third"
+	}
+	return fmt.Sprintf("number %d", n)
+}
+
+// joinShort lists up to three items and counts the rest.
+func joinShort(items []string) string {
+	clean := make([]string, 0, len(items))
+	for _, s := range items {
+		if s = strings.TrimSpace(s); s != "" {
+			clean = append(clean, s)
+		}
+	}
+	if len(clean) > 3 {
+		return strings.Join(clean[:3], ", ") + fmt.Sprintf(" and %d more", len(clean)-3)
+	}
+	return strings.Join(clean, ", ")
+}

internal/acl/explain_test.go

@@ -0,0 +1,172 @@
+package acl
+
+import (
+	"strings"
+	"testing"
+
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
+)
+
+func named(name, policy string, m config.Match) config.Group {
+	return config.Group{Name: name, Policy: policy, Match: m}
+}
+
+func analyzeAt(idx int, defRate int, groups ...config.Group) Analysis {
+	return Analyze(Context{Groups: groups, Index: idx, DefaultRateLimit: defRate, DefaultPolicy: "allow"})
+}
+
+func has(fs []Finding, field, contains string) bool {
+	for _, f := range fs {
+		if f.Field == field && strings.Contains(f.Message, contains) {
+			return true
+		}
+	}
+	return false
+}
+
+func TestAnalyzeAllowListIsAllowlistMode(t *testing.T) {
+	g := named("kids", "allow", config.Match{CIDRs: []string{"10.0.5.0/24"}})
+	g.AllowDomains = []string{"*.school.example"}
+	a := analyzeAt(0, 0, g)
+	if !has(a.Notes, "allow_domains", "allowlist-only") {
+		t.Errorf("notes = %+v, want the allowlist-only explanation", a.Notes)
+	}
+	if !strings.Contains(strings.Join(a.Summary, " "), "ONLY") {
+		t.Errorf("summary = %v, want it to say only the listed names resolve", a.Summary)
+	}
+}
+
+func TestAnalyzeBlockOnlyIsTheSameAsAllow(t *testing.T) {
+	a := analyzeAt(0, 0, named("g", "block_only", config.Match{CIDRs: []string{"10.0.0.0/24"}}))
+	if !has(a.Warnings, "policy", "exactly like allow") {
+		t.Errorf("warnings = %+v, want the block_only warning", a.Warnings)
+	}
+}
+
+func TestAnalyzeRateLimitZeroExemptsFromTheDefault(t *testing.T) {
+	g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}})
+	if a := analyzeAt(0, 50, g); !has(a.Warnings, "rate_limit", "exempt") {
+		t.Errorf("warnings = %+v, want the exemption warning", a.Warnings)
+	}
+	if a := analyzeAt(0, 0, g); len(a.Warnings) != 0 {
+		t.Errorf("no default limit, yet warnings = %+v", a.Warnings)
+	}
+	g.RateLimit = 20
+	if a := analyzeAt(0, 50, g); has(a.Warnings, "rate_limit", "exempt") {
+		t.Errorf("an explicit limit must not warn about exemption: %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeDenyIgnoresTheRest(t *testing.T) {
+	g := named("g", "deny", config.Match{CIDRs: []string{"10.0.0.0/24"}})
+	g.RateLimit = 5
+	g.SkipBlocklist = true
+	a := analyzeAt(0, 50, g)
+	if !has(a.Notes, "rate_limit", "Ignored") || !has(a.Notes, "skip_blocklist", "Ignored") {
+		t.Errorf("notes = %+v, want ignored-setting notes", a.Notes)
+	}
+	if has(a.Warnings, "rate_limit", "exempt") {
+		t.Errorf("a denied group has no rate limit to be exempt from: %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeShadowing(t *testing.T) {
+	wide := named("lan", "allow", config.Match{CIDRs: []string{"10.0.0.0/8"}})
+	narrow := named("cam", "deny", config.Match{CIDRs: []string{"10.1.2.0/24"}})
+
+	a := analyzeAt(1, 0, wide, narrow)
+	if !a.Unreachable || !has(a.Warnings, "match", `"lan"`) {
+		t.Errorf("narrow range after a wider one: unreachable=%v warnings=%+v", a.Unreachable, a.Warnings)
+	}
+
+	// The other order is the useful one and must stay quiet.
+	if a := analyzeAt(0, 0, narrow, wide); a.Unreachable || len(a.Warnings) > 1 {
+		t.Errorf("specific-before-general flagged: unreachable=%v warnings=%+v", a.Unreachable, a.Warnings)
+	}
+
+	// Saving the wide group above the narrow one buries the narrow one.
+	a = analyzeAt(0, 0, wide, narrow)
+	if !has(a.Warnings, "match", `makes "cam" unreachable`) {
+		t.Errorf("warnings = %+v, want the newly-unreachable warning", a.Warnings)
+	}
+}
+
+func TestAnalyzePartialOverlapNamesTheClaimedRange(t *testing.T) {
+	first := named("a", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}})
+	second := named("b", "allow", config.Match{CIDRs: []string{"10.0.0.128/25", "192.168.0.0/24"}})
+	a := analyzeAt(1, 0, first, second)
+	if a.Unreachable {
+		t.Fatal("one range is still free; the group is reachable")
+	}
+	if !has(a.Warnings, "match", "10.0.0.128/25 is already claimed by \"a\"") {
+		t.Errorf("warnings = %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeReverseNameAndTagShadowing(t *testing.T) {
+	first := named("a", "allow", config.Match{PTRSuffixes: []string{"lan.example."}, Tags: []string{"laptop"}})
+	second := named("b", "allow", config.Match{PTRSuffixes: []string{".lab.lan.example."}, Tags: []string{"Laptop"}})
+	if a := analyzeAt(1, 0, first, second); !a.Unreachable {
+		t.Errorf("a suffix under an earlier suffix plus the same tag is fully claimed: %+v", a)
+	}
+}
+
+func TestAnalyzeGroupAfterACatchAllIsUnreachable(t *testing.T) {
+	all := named("all", "allow", config.Match{Any: true, CIDRs: []string{"10.0.0.0/8"}})
+	g := named("g", "allow", config.Match{CIDRs: []string{"192.168.0.0/24"}})
+	a := analyzeAt(1, 0, all, g)
+	if !a.Unreachable {
+		t.Error("nothing after a match-any group can be reached")
+	}
+	if b := analyzeAt(0, 0, all, g); !has(b.Warnings, "match.any", "every client") {
+		t.Errorf("warnings = %+v, want the match-any warning", b.Warnings)
+	}
+}
+
+func TestAnalyzeCIDRHygiene(t *testing.T) {
+	a := analyzeAt(0, 0, named("g", "allow", config.Match{CIDRs: []string{"192.168.1.5/24", "0.0.0.0/0"}}))
+	if !has(a.Warnings, "match.cidrs", "host bits") || !has(a.Warnings, "match.cidrs", "very wide") {
+		t.Errorf("warnings = %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeDomainPatternMistakes(t *testing.T) {
+	g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}})
+	g.AllowDomains = []string{"https://example.com/a", ".example.org", "ads.*.com", "*.fine.example", "*"}
+	g.DenyDomains = []string{"*.fine.example"}
+	a := analyzeAt(0, 0, g)
+	for _, want := range []string{"looks like a URL", "starts with a dot", "not supported", "allows every name", "deny wins"} {
+		if !has(a.Warnings, "allow_domains", want) {
+			t.Errorf("missing allow_domains warning containing %q in %+v", want, a.Warnings)
+		}
+	}
+}
+
+func TestAnalyzeTagWithADotNeverMatches(t *testing.T) {
+	a := analyzeAt(0, 0, named("g", "allow", config.Match{Tags: []string{"lan.example"}}))
+	if !has(a.Warnings, "match.tags", "single label") {
+		t.Errorf("warnings = %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeUnknownUpstreams(t *testing.T) {
+	g := named("g", "allow", config.Match{CIDRs: []string{"10.0.0.0/24"}})
+	g.Upstreams = []string{"9.9.9.9:53"}
+	a := Analyze(Context{Groups: []config.Group{g}, Upstreams: []string{"1.1.1.1:53"}})
+	if !has(a.Warnings, "upstreams", "uses ALL upstreams") {
+		t.Errorf("warnings = %+v", a.Warnings)
+	}
+	g.Upstreams = []string{"1.1.1.1:53"}
+	if a := Analyze(Context{Groups: []config.Group{g}, Upstreams: []string{"1.1.1.1:53"}}); has(a.Warnings, "upstreams", "") {
+		t.Errorf("a configured upstream warned: %+v", a.Warnings)
+	}
+	if a := Analyze(Context{Groups: []config.Group{g}}); has(a.Warnings, "upstreams", "") {
+		t.Errorf("unknown upstream list must skip the check: %+v", a.Warnings)
+	}
+}
+
+func TestAnalyzeToleratesABadIndex(t *testing.T) {
+	if a := Analyze(Context{Index: 3}); len(a.Summary) != 0 {
+		t.Errorf("summary = %v", a.Summary)
+	}
+}

internal/api/zones.go

@@ -33,8 +33,9 @@ type ZonesResponse struct {
 	Zones []localzone.Zone `json:"zones"`
 	// Files are every master file the zones were compiled from.
 	Files []string `json:"files"`
-	// ManagedDir is the directory the API may rewrite, "" when the console
-	// has no managed zone directory configured (blocklist.managed_zones).
+	// ManagedDir says where a zone edit is stored, for the console to show:
+	// the console fills it with the Kubernetes namespace its DNSZone objects
+	// live in. The resolver itself leaves it empty, because it cannot write.
 	ManagedDir string `json:"managed_dir"`
 	// Writable is whether a write to ManagedDir would succeed.
 	Writable bool `json:"writable"`

internal/console/console.go

@@ -100,6 +100,7 @@ func (c *Console) Handler() (http.Handler, error) {
 	// Reads that combine the cluster's objects with the resolver's view.
 	mux.Handle("GET "+base+"/zones", c.api.Protect(http.HandlerFunc(c.handleZones), false))
 	mux.Handle("GET "+base+"/clients", c.api.Protect(http.HandlerFunc(c.handleClients), false))
+	mux.Handle("POST "+base+"/clients/validate", c.api.Protect(http.HandlerFunc(c.handleClientsValidate), false))
 	mux.Handle("GET "+base+"/blocklist-sources", c.api.Protect(http.HandlerFunc(c.handleListSources), false))
 	mux.Handle("PUT "+base+"/blocklist-sources/{name}", c.api.Protect(http.HandlerFunc(c.handleListSourcePut), true))
 	mux.Handle("DELETE "+base+"/blocklist-sources/{name}", c.api.Protect(http.HandlerFunc(c.handleListSourceDelete), true))

internal/console/hooks.go

@@ -56,7 +56,8 @@ type clientsView struct {
 		config.Group
 		Source string `json:"source"`
 	} `json:"groups"`
-	DefaultPolicy string `json:"default_policy"`
+	DefaultPolicy    string `json:"default_policy"`
+	DefaultRateLimit int    `json:"default_rate_limit"`
 }
 
 // sameGroup compares two groups by what they configure, so an empty list and a

internal/console/validate.go

@@ -0,0 +1,193 @@
+package console
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/netip"
+
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/acl"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/api"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubeedit"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubesource"
+)
+
+// validateRequest is a ClientGroup as the editor has it, before it is saved.
+type validateRequest struct {
+	Group config.Group `json:"group"`
+	// TestIP, when set, says which group an ordinary client at that address
+	// would land in once this group is saved.
+	TestIP string `json:"test_ip,omitempty"`
+}
+
+// validateResponse separates what blocks a save (errors) from what is legal
+// but probably unintended (warnings) and from plain information (notes).
+type validateResponse struct {
+	OK          bool            `json:"ok"`
+	Errors      []acl.Finding   `json:"errors"`
+	Warnings    []acl.Finding   `json:"warnings"`
+	Notes       []acl.Finding   `json:"notes"`
+	Summary     []string        `json:"summary"`
+	Unreachable bool            `json:"unreachable"`
+	Position    int             `json:"position"`
+	Total       int             `json:"total"`
+	Test        *validateTestIP `json:"test,omitempty"`
+}
+
+type validateTestIP struct {
+	IP      string `json:"ip"`
+	Valid   bool   `json:"valid"`
+	Group   string `json:"group"`
+	Default bool   `json:"default"`
+	Note    string `json:"note"`
+}
+
+// handleClientsValidate checks a group without saving it. It applies the same
+// checks the save applies (kubesource.CheckGroup, the object-name rule) and
+// then says what the group would do and what is probably wrong with it, in the
+// context of the groups that already exist. A group that is not already saved
+// is placed last, which is where a save puts it.
+func (c *Console) handleClientsValidate(w http.ResponseWriter, r *http.Request) {
+	var in validateRequest
+	if err := api.DecodeJSON(r, &in); err != nil {
+		api.WriteError(w, http.StatusBadRequest, "malformed request body")
+		return
+	}
+	current, meta, err := c.currentGroups(r.Context())
+	if err != nil {
+		api.WriteError(w, http.StatusBadGateway, err.Error())
+		return
+	}
+
+	out := validateResponse{Errors: []acl.Finding{}, Warnings: []acl.Finding{}, Notes: []acl.Finding{}, Summary: []string{}}
+	name := in.Group.Name
+	if err := kubeedit.CheckGroupName(name); err != nil {
+		out.Errors = append(out.Errors, acl.Finding{Field: "name", Message: err.Error()})
+	}
+	spec := kubeedit.SpecFromGroup(in.Group, 0)
+	for _, fe := range kubesource.CheckGroupAll(name, spec) {
+		out.Errors = append(out.Errors, acl.Finding{Field: fe.Field, Message: fe.Msg})
+	}
+	if have, ok := meta.fileGroups[name]; ok && !sameGroup(have, in.Group) {
+		out.Errors = append(out.Errors, acl.Finding{Field: "name", Message: "A group with this name is defined in the resolver's config file and cannot be changed here. Pick another name."})
+	}
+
+	groups, idx := place(current, in.Group)
+	out.Position, out.Total = idx+1, len(groups)
+	out.OK = len(out.Errors) == 0
+
+	// The analysis is useful even for a group that cannot be saved yet (it
+	// explains what the typed-so-far group would do), but it only makes sense
+	// once the group selects something.
+	an := acl.Analyze(acl.Context{
+		Groups: groups, Index: idx,
+		DefaultRateLimit: meta.defaultRateLimit, DefaultPolicy: meta.defaultPolicy,
+		Upstreams: meta.upstreams,
+	})
+	out.Summary, out.Warnings, out.Notes, out.Unreachable = an.Summary, an.Warnings, an.Notes, an.Unreachable
+
+	if in.TestIP != "" {
+		out.Test = testIP(in.TestIP, groups, meta.defaultPolicy)
+	}
+	api.WriteJSON(w, http.StatusOK, out)
+}
+
+// place puts g in the list: in its own slot when a group of that name exists,
+// otherwise last, which is where a save adds it.
+func place(list []config.Group, g config.Group) ([]config.Group, int) {
+	out := append([]config.Group(nil), list...)
+	for i := range out {
+		if out[i].Name == g.Name && g.Name != "" {
+			out[i] = g
+			return out, i
+		}
+	}
+	return append(out, g), len(out)
+}
+
+func testIP(raw string, groups []config.Group, defaultPolicy string) *validateTestIP {
+	res := &validateTestIP{IP: raw}
+	ip, err := netip.ParseAddr(raw)
+	if err != nil {
+		res.Note = "Not an IP address."
+		return res
+	}
+	res.Valid = true
+	d := acl.Compile(config.ACL{DefaultPolicy: defaultPolicy, Groups: groups}).Match(acl.Client{IP: ip})
+	res.Group, res.Default = d.Group, d.Default
+	switch {
+	case d.Default:
+		res.Note = "No group matches this address by IP; it gets the default policy (" + string(d.Policy) + ")."
+	default:
+		res.Note = "Matched on address. Clients can also match by reverse-DNS name or tag, which cannot be tested here."
+	}
+	return res
+}
+
+type groupsMeta struct {
+	fileGroups       map[string]config.Group
+	defaultPolicy    string
+	defaultRateLimit int
+	upstreams        []string
+}
+
+// currentGroups is the evaluation order the resolver will use, made consistent
+// with the cluster: the same merge handleClients does, as typed groups.
+func (c *Console) currentGroups(ctx context.Context) ([]config.Group, groupsMeta, error) {
+	meta := groupsMeta{fileGroups: map[string]config.Group{}}
+	var view clientsView
+	if err := c.opt.Runtime.Get(ctx, "/api/clients", &view); err != nil {
+		return nil, meta, err
+	}
+	objs, err := c.opt.Editor.Groups(ctx)
+	if err != nil {
+		return nil, meta, errors.New("cluster: " + err.Error())
+	}
+	meta.defaultPolicy, meta.defaultRateLimit = view.DefaultPolicy, view.DefaultRateLimit
+
+	inCluster := map[string]bool{}
+	for _, o := range objs {
+		inCluster[o.Name] = true
+	}
+	var out []config.Group
+	seen := map[string]bool{}
+	for _, g := range view.Groups {
+		switch g.Source {
+		case "history":
+			continue
+		case "kubernetes":
+			if !inCluster[g.Name] {
+				continue
+			}
+		default:
+			meta.fileGroups[g.Name] = g.Group
+		}
+		seen[g.Name] = true
+		out = append(out, g.Group)
+	}
+	for _, o := range objs {
+		if !seen[o.Name] {
+			g := config.Group{Name: o.Name, Comment: o.Spec.Comment, Match: o.Spec.Match, Policy: o.Spec.Policy,
+				AllowDomains: o.Spec.AllowDomains, DenyDomains: o.Spec.DenyDomains, SkipBlocklist: o.Spec.SkipBlocklist,
+				SkipCache: o.Spec.SkipCache, RateLimit: o.Spec.RateLimit, MaxConcurrent: o.Spec.MaxConcurrent,
+				LogQueries: o.Spec.LogQueries, Upstreams: o.Spec.Upstreams}
+			out = append(out, g)
+		}
+	}
+
+	// Upstreams are a nicety for one warning; an unreachable or older resolver
+	// must not make validation fail.
+	var ups struct {
+		Servers []struct {
+			Addr string `json:"addr"`
+		} `json:"servers"`
+	}
+	if err := c.opt.Runtime.Get(ctx, "/api/upstreams", &ups); err == nil {
+		meta.upstreams = []string{}
+		for _, s := range ups.Servers {
+			meta.upstreams = append(meta.upstreams, s.Addr)
+		}
+	}
+	return out, meta, nil
+}

internal/console/validate_test.go

@@ -0,0 +1,181 @@
+package console
+
+import (
+	"context"
+	"encoding/json"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	dynfake "k8s.io/client-go/dynamic/fake"
+
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubeedit"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubesource"
+)
+
+// validator builds a console whose cluster holds the given groups and whose
+// resolver reports one group of its own, defined in its config file.
+func validator(t *testing.T, saved ...config.Group) *Console {
+	t.Helper()
+	cl := dynfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(),
+		map[schema.GroupVersionResource]string{
+			kubesource.ZoneGVR: "DNSZoneList", kubesource.GroupGVR: "ClientGroupList",
+			kubesource.ListGVR: "BlockListList", kubesource.OverrideGVR: "AddressOverrideList",
+			kubesource.RuleGVR: "DomainRuleList",
+		})
+	ed := kubeedit.New(cl, "dns")
+	if len(saved) > 0 {
+		if _, err := ed.SyncGroups(context.Background(), saved, nil, "test"); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	resolver := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/clients":
+			groups := []map[string]any{{
+				"name": "from-file", "policy": "allow", "source": "",
+				"match": map[string]any{"cidrs": []string{"172.16.0.0/12"}},
+			}}
+			for _, g := range saved {
+				groups = append(groups, map[string]any{"name": g.Name, "policy": g.Policy, "source": "kubernetes", "match": g.Match})
+			}
+			json.NewEncoder(w).Encode(map[string]any{"groups": groups, "default_policy": "allow", "default_rate_limit": 50})
+		case "/api/upstreams":
+			json.NewEncoder(w).Encode(map[string]any{"servers": []map[string]any{{"addr": "1.1.1.1:53"}}})
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	t.Cleanup(resolver.Close)
+	rt, err := NewRuntime(resolver.URL, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	return &Console{opt: Options{Runtime: rt, Editor: ed}, log: slog.Default()}
+}
+
+func validate(t *testing.T, c *Console, body string) validateResponse {
+	t.Helper()
+	rec := httptest.NewRecorder()
+	c.handleClientsValidate(rec, httptest.NewRequest(http.MethodPost, "/api/clients/validate", strings.NewReader(body)))
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d: %s", rec.Code, rec.Body)
+	}
+	var out validateResponse
+	if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
+		t.Fatalf("%v: %s", err, rec.Body)
+	}
+	return out
+}
+
+func hasField(list []string, f string) bool {
+	for _, s := range list {
+		if s == f {
+			return true
+		}
+	}
+	return false
+}
+
+func errFields(r validateResponse) []string {
+	var out []string
+	for _, e := range r.Errors {
+		out = append(out, e.Field)
+	}
+	return out
+}
+
+func TestValidateReportsEveryErrorAtOnce(t *testing.T) {
+	c := validator(t)
+	r := validate(t, c, `{"group":{"name":"Bad Name","policy":"sometimes","match":{"cidrs":["10.0.0.0/33"]},"rate_limit":-1}}`)
+	if r.OK {
+		t.Fatal("a group with four problems validated")
+	}
+	for _, want := range []string{"name", "policy", "match.cidrs", "rate_limit"} {
+		if !hasField(errFields(r), want) {
+			t.Errorf("no error for %q; got %+v", want, r.Errors)
+		}
+	}
+}
+
+func TestValidateAcceptsAGoodGroupAndExplainsIt(t *testing.T) {
+	c := validator(t)
+	r := validate(t, c, `{"group":{"name":"kids","policy":"allow","match":{"cidrs":["10.0.5.0/24"]},"allow_domains":["*.school.example"]}}`)
+	if !r.OK || len(r.Errors) != 0 {
+		t.Fatalf("errors: %+v", r.Errors)
+	}
+	if r.Position != 2 || r.Total != 2 {
+		t.Errorf("a new group goes last: position %d of %d", r.Position, r.Total)
+	}
+	if len(r.Summary) < 3 {
+		t.Errorf("summary = %v", r.Summary)
+	}
+	// 0 rate limit under a default of 50 is the surprise worth flagging.
+	found := false
+	for _, w := range r.Warnings {
+		found = found || w.Field == "rate_limit"
+	}
+	if !found {
+		t.Errorf("warnings = %+v, want the rate-limit exemption", r.Warnings)
+	}
+}
+
+func TestValidateEditingKeepsThePosition(t *testing.T) {
+	a := config.Group{Name: "a", Policy: "allow", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}}
+	b := config.Group{Name: "b", Policy: "allow", Match: config.Match{CIDRs: []string{"192.168.0.0/16"}}}
+	c := validator(t, a, b)
+	r := validate(t, c, `{"group":{"name":"a","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}}}`)
+	if r.Position != 2 || r.Total != 3 { // from-file, a, b
+		t.Errorf("position %d of %d", r.Position, r.Total)
+	}
+}
+
+func TestValidateRefusesAFileGroupName(t *testing.T) {
+	c := validator(t)
+	r := validate(t, c, `{"group":{"name":"from-file","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}}}`)
+	if r.OK || !hasField(errFields(r), "name") {
+		t.Errorf("a group the resolver's file owns validated: %+v", r)
+	}
+}
+
+func TestValidateFlagsAGroupBuriedUnderAWiderOne(t *testing.T) {
+	wide := config.Group{Name: "wide", Policy: "allow", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}}
+	c := validator(t, wide)
+	r := validate(t, c, `{"group":{"name":"narrow","policy":"deny","match":{"cidrs":["10.1.0.0/16"]}}}`)
+	if !r.OK || !r.Unreachable {
+		t.Errorf("ok=%v unreachable=%v warnings=%+v", r.OK, r.Unreachable, r.Warnings)
+	}
+}
+
+func TestValidateTestIP(t *testing.T) {
+	g := config.Group{Name: "lan", Policy: "deny", Match: config.Match{CIDRs: []string{"10.0.0.0/8"}}}
+	c := validator(t, g)
+	r := validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"10.1.2.3"}`)
+	if r.Test == nil || r.Test.Group != "lan" || r.Test.Default {
+		t.Errorf("test = %+v", r.Test)
+	}
+	r = validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"8.8.8.8"}`)
+	if r.Test == nil || !r.Test.Default {
+		t.Errorf("test = %+v", r.Test)
+	}
+	r = validate(t, c, `{"group":{"name":"lan","policy":"deny","match":{"cidrs":["10.0.0.0/8"]}},"test_ip":"nope"}`)
+	if r.Test == nil || r.Test.Valid {
+		t.Errorf("test = %+v", r.Test)
+	}
+}
+
+func TestValidateUnreachableResolverIsABadGateway(t *testing.T) {
+	c := validator(t)
+	c.opt.Runtime, _ = NewRuntime("http://127.0.0.1:1", "")
+	rec := httptest.NewRecorder()
+	c.handleClientsValidate(rec, httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"group":{"name":"x"}}`)))
+	if rec.Code != http.StatusBadGateway {
+		t.Errorf("status %d", rec.Code)
+	}
+}

internal/kubeedit/groups.go

@@ -129,3 +129,12 @@ func (e *Editor) SyncGroups(ctx context.Context, desired []config.Group, foreign
 	}
 	return res, nil
 }
+
+// CheckGroupName reports why name cannot be a ClientGroup's object name, so a
+// validation endpoint can say it before a save does.
+func CheckGroupName(name string) error {
+	if name == "" {
+		return fmt.Errorf("a group needs a name")
+	}
+	return checkName("group", name)
+}

internal/kubesource/convert.go

@@ -46,14 +46,44 @@ func convertGroup(u *unstructured.Unstructured) (group, error) {
 	return groupFromSpec(u.GetName(), s)
 }
 
+// FieldError is a validation failure tied to one field of a ClientGroup, so an
+// editor can show the message next to the input that caused it. Field is the
+// JSON path inside the group ("policy", "match.cidrs", "rate_limit"); "name"
+// and "match" are used for the whole-object cases.
+type FieldError struct {
+	Field string
+	Msg   string
+}
+
+func (e FieldError) Error() string { return e.Msg }
+
 // CheckGroup validates a ClientGroup spec the way the runtime will, so an
-// editor can refuse a bad one before it is written.
+// editor can refuse a bad one before it is written. It reports the first
+// problem; CheckGroupAll reports every one.
 func CheckGroup(name string, s ClientGroupSpec) (config.Group, error) {
-	g, err := groupFromSpec(name, s)
-	return g.cfg, err
+	g, errs := checkGroup(name, s)
+	if len(errs) > 0 {
+		return g.cfg, errs[0]
+	}
+	return g.cfg, nil
+}
+
+// CheckGroupAll is CheckGroup for an editor that wants to show every problem
+// at once instead of one per save.
+func CheckGroupAll(name string, s ClientGroupSpec) []FieldError {
+	_, errs := checkGroup(name, s)
+	return errs
 }
 
 func groupFromSpec(name string, s ClientGroupSpec) (group, error) {
+	g, errs := checkGroup(name, s)
+	if len(errs) > 0 {
+		return group{}, errs[0]
+	}
+	return g, nil
+}
+
+func checkGroup(name string, s ClientGroupSpec) (group, []FieldError) {
 	g := config.Group{
 		Name:          name,
 		Comment:       s.Comment,
@@ -68,14 +98,18 @@ func groupFromSpec(name string, s ClientGroupSpec) (group, error) {
 		LogQueries:    s.LogQueries,
 		Upstreams:     s.Upstreams,
 	}
+	var errs []FieldError
+	bad := func(field, format string, args ...any) {
+		errs = append(errs, FieldError{Field: field, Msg: fmt.Sprintf(format, args...)})
+	}
 	switch g.Policy {
 	case "allow", "deny", "block_only":
 	default:
-		return group{}, fmt.Errorf("policy must be allow, deny or block_only (got %q)", g.Policy)
+		bad("policy", "policy must be allow, deny or block_only (got %q)", g.Policy)
 	}
 	for _, cidr := range g.Match.CIDRs {
 		if _, _, err := net.ParseCIDR(cidr); err != nil {
-			return group{}, fmt.Errorf("bad CIDR %q", cidr)
+			bad("match.cidrs", "bad CIDR %q", cidr)
 		}
 	}
 	// Copy before normalising: the slice belongs to the decoded object.
@@ -85,18 +119,22 @@ func groupFromSpec(name string, s ClientGroupSpec) (group, error) {
 			g.Match.PTRSuffixes[i] = s + "."
 		}
 	}
-	if !g.Match.Any && len(g.Match.CIDRs) == 0 && len(g.Match.PTRSuffixes) == 0 && len(g.Match.Tags) == 0 {
-		return group{}, fmt.Errorf("match selects no clients: set cidrs, ptr_suffixes, tags or any")
-	}
-	if g.Match.Any && len(g.Match.CIDRs) == 0 && len(g.Match.PTRSuffixes) == 0 && len(g.Match.Tags) == 0 {
+	selectsSomething := len(g.Match.CIDRs) > 0 || len(g.Match.PTRSuffixes) > 0 || len(g.Match.Tags) > 0
+	switch {
+	case !g.Match.Any && !selectsSomething:
+		bad("match", "match selects no clients: set cidrs, ptr_suffixes, tags or any")
+	case g.Match.Any && !selectsSomething:
 		// config.Validate refuses this shape for every group. Refusing it here
 		// rejects one object instead of the whole merged configuration.
-		return group{}, fmt.Errorf("match.any alone is not accepted; use a catch-all in the YAML acl.groups")
+		bad("match.any", "match.any alone is not accepted; use a catch-all in the YAML acl.groups")
 	}
-	if g.RateLimit < 0 || g.MaxConcurrent < 0 {
-		return group{}, fmt.Errorf("rate_limit and max_concurrent must not be negative")
+	if g.RateLimit < 0 {
+		bad("rate_limit", "rate_limit must not be negative")
 	}
-	return group{cfg: g, priority: s.Priority}, nil
+	if g.MaxConcurrent < 0 {
+		bad("max_concurrent", "max_concurrent must not be negative")
+	}
+	return group{cfg: g, priority: s.Priority}, errs
 }
 
 func convertList(u *unstructured.Unstructured) (config.ListSource, error) {