From b097e10bef70d96ad0c18228a148e3eab07d555a Mon Sep 17 00:00:00 2001 From: adam Date: Sun, 11 Oct 2026 17:06:22 +0100 Subject: [PATCH] Keep options in a fixed order, and say which block a diff hunk is in Options of a block (server options, a view, a zone) are listed and written grouped by topic, alphabetical within a group, so the same option is in the same place everywhere. Top-level statements keep the order they were added in. named.conf diffs add the enclosing blocks to each hunk header, like git's function context: @@ -50,6 +49,8 @@ view "internal" {. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 ++ docs/api.md | 4 +- docs/concepts.md | 10 +- internal/engine/deploy.go | 3 + internal/engine/diffcontext.go | 136 ++++++++++++++++++++++++++++ internal/engine/diffcontext_test.go | 63 +++++++++++++ internal/model/model.go | 8 +- internal/model/optorder.go | 55 +++++++++++ internal/model/optorder_test.go | 40 ++++++++ 9 files changed, 326 insertions(+), 3 deletions(-) create mode 100644 internal/engine/diffcontext.go create mode 100644 internal/engine/diffcontext_test.go create mode 100644 internal/model/optorder.go create mode 100644 internal/model/optorder_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 450e1b1..89748f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,16 @@ All notable changes to Jarl are documented here. The format follows ### Changed +- Options are kept in one fixed order, in named.conf, the web UI and the + CLI: grouped by topic (files, listening, access control, recursion and + forwarding, DNSSEC, transfers and notify, then the rest), alphabetical + within a group. The same option is now in the same place in every view + and zone. The first apply after upgrading reorders named.conf (a diff + that only moves lines). Top-level statements keep their order. +- Diffs of named.conf say which block each change is in, after the `@@` + line numbers (`@@ -50,6 +49,8 @@ view "internal" {`), like git does + with function names. Moving an option from the server's options into a + view no longer looks like two unrelated edits. - Web UI options (server, top-level, view and zone): values with blocks, such as logging, are shown and edited laid out as in named.conf, over several lines with indentation. The editor grows with its content, Tab diff --git a/docs/api.md b/docs/api.md index 47637b9..c958982 100644 --- a/docs/api.md +++ b/docs/api.md @@ -444,7 +444,9 @@ listing with `?changeset=` marks them `pending`. | GET / PATCH | `/settings` | `{"nameservers": [...], "rname": "...", "default_ttl": 3600, "refresh": ..., "retry": ..., "expire": ..., "negative_ttl": ..., "serial_policy": "date"}` | Option values are named.conf syntax without the final semicolon. They are -checked against the BIND 9.20 grammar for that scope. +checked against the BIND 9.20 grammar for that scope. `position` only +orders top-level statements; the options of a block are always listed and +written in Jarl's fixed order (docs/concepts.md). ### Catalogs and search diff --git a/docs/concepts.md b/docs/concepts.md index 2fac872..1da5337 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -68,7 +68,9 @@ both, using a single mechanism: * `POST /api/v1/apply` (CLI: `jarl apply -f changes.yaml`) applies a list of operations as one changeset in a single request. * `?dry_run=1` (CLI: `--dry-run`) runs the whole pipeline except installing - files and reloading named, and shows the problems and the diff. + files and reloading named, and shows the problems and the diff. Each + part of a named.conf diff says which block it is in, after the `@@` + line numbers: `@@ -50,6 +49,8 @@ view "internal" {`. **Why bundling matters.** Some changes only make sense together. Turning `www` from an A record into a CNAME means deleting the A record and adding @@ -159,6 +161,12 @@ RFC 1982 arithmetic. Jarl checks them before named does. Statements that Jarl manages itself (`zone`, `view`, `acl`, `key`, a zone's `type`/`file`, a view's `match-clients`, ...) can't be set as raw options. + Options of a block are kept in one fixed order, the same in named.conf, + the web UI and the CLI: grouped (files, listening, access control, + recursion and forwarding, DNSSEC, transfers and notify, then the rest), + alphabetical within a group. So the same option is in the same place in + every view and zone. Top-level statements stay in the order they were + added. * **Settings** are Jarl's defaults for new zones: name servers, SOA contact and timers, default TTL, serial policy. diff --git a/internal/engine/deploy.go b/internal/engine/deploy.go index bf8f370..a93b902 100644 --- a/internal/engine/deploy.go +++ b/internal/engine/deploy.go @@ -335,5 +335,8 @@ func fileDiff(path string, old []byte, existed bool, new []byte, removed, secret if d != "" && !strings.HasSuffix(d, "\n") { d += "\n" } + if strings.HasSuffix(path, ".conf") { + d = labelHunks(d, string(old), string(new)) + } return d } diff --git a/internal/engine/diffcontext.go b/internal/engine/diffcontext.go new file mode 100644 index 0000000..ae91df8 --- /dev/null +++ b/internal/engine/diffcontext.go @@ -0,0 +1,136 @@ +package engine + +import ( + "regexp" + "strconv" + "strings" +) + +// hunkHeader matches a unified diff hunk header; the line counts are left +// out when they are 1. +var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@$`) + +// labelHunks adds the blocks a hunk's first change is in to its header, +// like git does with function names, so a change to named.conf says where +// it is: +// +// @@ -50,6 +49,8 @@ view "internal" { +// +// Removed lines are looked up in the old file, added ones in the new. +func labelHunks(diff, old, new string) string { + if !strings.Contains(diff, "\n@@ ") { + return diff + } + oldCtx, newCtx := blockContext(old), blockContext(new) + lines := strings.SplitAfter(diff, "\n") + for i, line := range lines { + m := hunkHeader.FindStringSubmatch(strings.TrimSuffix(line, "\n")) + if m == nil { + continue + } + o, _ := strconv.Atoi(m[1]) + n, _ := strconv.Atoi(m[2]) + label := "" + hunk: + for _, l := range lines[i+1:] { + switch { + case strings.HasPrefix(l, "-"): + label = at(oldCtx, o) + break hunk + case strings.HasPrefix(l, "+"): + label = at(newCtx, n) + break hunk + case strings.HasPrefix(l, " "): + o, n = o+1, n+1 + case strings.HasPrefix(l, `\`): + default: + break hunk + } + } + if label != "" { + lines[i] = strings.TrimSuffix(line, "\n") + " " + label + "\n" + } + } + return strings.Join(lines, "") +} + +// at returns the context of line n (1-based), or "". +func at(ctx []string, n int) string { + if n < 1 || n > len(ctx) { + return "" + } + return ctx[n-1] +} + +// blockContext returns, for each line of a named.conf file, the blocks +// open at its start: `view "internal" { zone "example.com" {`, or "" at +// the top level. It knows named.conf's strings and comments. +func blockContext(text string) []string { + var ( + out []string + stack []string + stmt strings.Builder // the current statement, up to a { or ; + inStr bool + inBlock bool // in a /* */ comment + ) + label := func() string { + if len(stack) == 0 { + return "" + } + return strings.Join(stack, " { ") + " {" + } + for _, line := range strings.SplitAfter(text, "\n") { + if line == "" { + continue + } + out = append(out, label()) + for i := 0; i < len(line); i++ { + c := line[i] + switch { + case inBlock: + if c == '*' && i+1 < len(line) && line[i+1] == '/' { + inBlock = false + i++ + } + case inStr: + stmt.WriteByte(c) + if c == '\\' && i+1 < len(line) { + stmt.WriteByte(line[i+1]) + i++ + } else if c == '"' { + inStr = false + } + case c == '"': + inStr = true + stmt.WriteByte(c) + case c == '#' || c == '/' && i+1 < len(line) && line[i+1] == '/': + i = len(line) // a comment to the end of the line + case c == '/' && i+1 < len(line) && line[i+1] == '*': + inBlock = true + i++ + case c == '{': + stack = append(stack, shortLabel(stmt.String())) + stmt.Reset() + case c == '}': + if len(stack) > 0 { + stack = stack[:len(stack)-1] + } + stmt.Reset() + case c == ';': + stmt.Reset() + default: + stmt.WriteByte(c) + } + } + } + return out +} + +// shortLabel tidies the text before a { for a hunk header. +func shortLabel(s string) string { + s = strings.Join(strings.Fields(s), " ") + if len(s) > 60 { + s = s[:57] + "..." + } + return s +} diff --git a/internal/engine/diffcontext_test.go b/internal/engine/diffcontext_test.go new file mode 100644 index 0000000..7279c75 --- /dev/null +++ b/internal/engine/diffcontext_test.go @@ -0,0 +1,63 @@ +package engine + +import ( + "strings" + "testing" +) + +func TestLabelHunks(t *testing.T) { + old := `// header +options { + directory "/var/cache/bind"; + forwarders { 1.1.1.1; 9.9.9.9; }; + listen-on { any; }; + recursion no; + version "a { b"; # a { in a string and comments: /* { + dnssec-validation auto; +}; + +view "internal" { + match-clients { 10.0.0.0/8; }; + recursion yes; + allow-transfer { localhost; }; + /* a { comment + over lines */ + zone "example.com" { + type primary; + file "example.com.db"; + notify yes; + }; + zone "other.example" { type primary; file "o.db"; }; +}; +` + new := strings.Replace(old, "\tforwarders { 1.1.1.1; 9.9.9.9; };\n", "", 1) + new = strings.Replace(new, "\tallow-transfer { localhost; };\n", "\tallow-transfer { localhost; };\n\tforward first;\n\tforwarders { 1.1.1.1; 9.9.9.9; };\n", 1) + new = strings.Replace(new, "\t\tnotify yes;\n", "\t\tnotify no;\n", 1) + d := fileDiff("/etc/bind/named.conf", []byte(old), true, []byte(new), false, false) + var headers []string + for _, l := range strings.Split(d, "\n") { + if strings.HasPrefix(l, "@@") { + headers = append(headers, l) + } + } + want := []string{ + `@@ -1,7 +1,6 @@ options {`, + `@@ -12,12 +11,14 @@ view "internal" {`, + } + if strings.Join(headers, "\n") != strings.Join(want, "\n") { + t.Errorf("headers:\n%s\n\ndiff:\n%s", strings.Join(headers, "\n"), d) + } + + // The first change of a hunk decides, and a change inside a zone + // names both blocks. + new2 := strings.Replace(old, "\t\tnotify yes;\n", "\t\tnotify no;\n", 1) + d = fileDiff("/etc/bind/named.conf", []byte(old), true, []byte(new2), false, false) + if !strings.Contains(d, ` @@ view "internal" { zone "example.com" {`+"\n") { + t.Errorf("zone change:\n%s", d) + } + // Zone files aren't named.conf syntax. + d = fileDiff("/var/lib/bind/example.com.db", []byte(old), true, []byte(new2), false, false) + if strings.Contains(d, "@@ view") { + t.Errorf("zone file labelled:\n%s", d) + } +} diff --git a/internal/model/model.go b/internal/model/model.go index d2392c2..3988ccd 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -506,7 +506,9 @@ func (s *State) ServedRecordsByZone() map[int64][]*Record { return out } -// ScopedOptions returns the options of a scope, in position order. +// ScopedOptions returns the options of a scope: in a fixed order by name +// for a block (see optionLess), in the order they were added for +// top-level statements, which are written as entered. func (s *State) ScopedOptions(scope string, scopeID int64) []*Option { var out []*Option for _, o := range s.Options { @@ -514,6 +516,10 @@ func (s *State) ScopedOptions(scope string, scopeID int64) []*Option { out = append(out, o) } } + if scope != ScopeTopLevel { + sort.Slice(out, func(i, j int) bool { return optionLess(out[i], out[j]) }) + return out + } sort.Slice(out, func(i, j int) bool { if out[i].Position != out[j].Position { return out[i].Position < out[j].Position diff --git a/internal/model/optorder.go b/internal/model/optorder.go new file mode 100644 index 0000000..cbc02a1 --- /dev/null +++ b/internal/model/optorder.go @@ -0,0 +1,55 @@ +package model + +// optionGroups orders the options of a block (options, a view, a zone) the +// same way everywhere: in named.conf, the web UI and the CLI. Related +// options are grouped; within a group they are alphabetical, and options +// not listed here come last, alphabetically. named doesn't care about the +// order of options in a block. +var optionGroups = [][]string{ + // Files and identity. + {"bindkeys-file", "directory", "dump-file", "hostname", "managed-keys-directory", "memstatistics-file", "pid-file", "recursing-file", "secroots-file", "server-id", "session-keyfile", "statistics-file", "version"}, + // Listening and source addresses. + {"listen-on", "listen-on-v6", "notify-source", "notify-source-v6", "query-source", "query-source-v6", "transfer-source", "transfer-source-v6"}, + // Access control. + {"allow-query", "allow-query-cache", "allow-query-cache-on", "allow-query-on", "allow-recursion", "allow-recursion-on", "allow-update", "allow-update-forwarding", "blackhole", "deny-answer-addresses", "deny-answer-aliases", "update-policy"}, + // Recursion, forwarding and the cache. + {"forward", "forwarders", "max-cache-size", "max-cache-ttl", "max-ncache-ttl", "minimal-responses", "prefetch", "qname-minimization", "rate-limit", "recursion", "response-policy", "serve-stale", "stale-answer-enable", "stale-answer-ttl", "stale-cache-enable"}, + // DNSSEC. + {"dnssec-accept-expired", "dnssec-must-be-secure", "dnssec-policy", "dnssec-validation", "inline-signing", "key-directory", "trust-anchor-telemetry", "validate-except"}, + // Transfers and notify. + {"allow-notify", "allow-transfer", "also-notify", "catalog-zones", "ixfr-from-differences", "masters", "max-transfer-idle-in", "max-transfer-idle-out", "max-transfer-time-in", "max-transfer-time-out", "notify", "notify-to-soa", "primaries", "provide-ixfr", "request-ixfr", "serial-query-rate", "transfer-format", "transfers-in", "transfers-out", "transfers-per-ns"}, +} + +var optionGroup = func() map[string]int { + m := map[string]int{} + for i, g := range optionGroups { + for _, n := range g { + m[n] = i + } + } + return m +}() + +// optionLess orders two options of the same block: by group, then name, +// then (for an option given more than once, like listen-on) the order they +// were added in. +func optionLess(a, b *Option) bool { + ga, ok := optionGroup[a.Name] + if !ok { + ga = len(optionGroups) + } + gb, ok := optionGroup[b.Name] + if !ok { + gb = len(optionGroups) + } + if ga != gb { + return ga < gb + } + if a.Name != b.Name { + return a.Name < b.Name + } + if a.Position != b.Position { + return a.Position < b.Position + } + return a.ID < b.ID +} diff --git a/internal/model/optorder_test.go b/internal/model/optorder_test.go new file mode 100644 index 0000000..fb65ee6 --- /dev/null +++ b/internal/model/optorder_test.go @@ -0,0 +1,40 @@ +package model + +import ( + "strings" + "testing" +) + +func TestScopedOptionsOrder(t *testing.T) { + s := NewState() + add := func(scope string, names ...string) { + for i, n := range names { + id := int64(len(s.Options) + 1) + s.Options[id] = &Option{ID: id, Scope: scope, Name: n, Position: i} + } + } + // Added in an arbitrary order; listen-on twice. + add(ScopeView, "zzz-unknown", "forwarders", "listen-on", "recursion", "dnssec-validation", "aaa-unknown", "allow-query", "forward", "directory", "listen-on", "allow-transfer") + var got []string + for _, o := range s.ScopedOptions(ScopeView, 0) { + got = append(got, o.Name) + } + want := "directory listen-on listen-on allow-query forward forwarders recursion dnssec-validation allow-transfer aaa-unknown zzz-unknown" + if strings.Join(got, " ") != want { + t.Errorf("order:\n got %s\nwant %s", strings.Join(got, " "), want) + } + // An option given twice keeps the order it was added in. + l := s.ScopedOptions(ScopeView, 0) + if l[1].Position > l[2].Position { + t.Error("repeated listen-on swapped") + } + // Top-level statements stay as entered. + add(ScopeTopLevel, "logging", "acl", "key") + got = nil + for _, o := range s.ScopedOptions(ScopeTopLevel, 0) { + got = append(got, o.Name) + } + if strings.Join(got, " ") != "logging acl key" { + t.Errorf("top level: %v", got) + } +}