diff --git a/CHANGELOG.md b/CHANGELOG.md index f1e8722..77a7a57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,16 @@ All notable changes to Housecarl are documented here. The format follows ### Fixed +- A history entry's ruleset diff showed the object it changed as moved: + stepping back put the old form at the end of its list, so updating + one port forward (turning hairpin off) also showed its forward and + DNAT lines moving down. The diff is now only what the change did, and + its sides are named `before N` and `after N` instead of `now`. +- Ruleset diffs numbered the hunks after one with a moved line too early + on the new side (`@@ -76,9 +74,9 @@` for `+76,9`), so the chain named + after `@@` could be the one before (`chain output` for a hunk in + `chain dstnat`). go-udiff miscounts them; Housecarl now numbers them + itself. - The dashboard's bar for a per-address quota showed only the busiest address, so one address that used up its allowance filled it (100%). The bar is now all the addresses together, and the dashboard and the diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 45459dd..334e287 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -335,6 +335,8 @@ func Rewind(st *model.State, h *model.HistoryEntry) error { return err } } + // put appends what it brings back; its position puts it in place. + st.Sort() return nil } diff --git a/internal/server/diff_test.go b/internal/server/diff_test.go index ddc536e..3adc0e4 100644 --- a/internal/server/diff_test.go +++ b/internal/server/diff_test.go @@ -1,6 +1,8 @@ package server import ( + "encoding/json" + "fmt" "strings" "testing" @@ -70,3 +72,68 @@ func TestHistoryDiff(t *testing.T) { t.Errorf("unknown entry: %d", code) } } + +// TestHunkStarts: after a hunk with unchanged lines between its edits (a +// moved line), the later hunks' new sides start where they do. +func TestHunkStarts(t *testing.T) { + var a, b strings.Builder + for i := 1; i <= 20; i++ { + fmt.Fprintf(&a, "%d\n", i) + switch i { + case 4: + case 6: + b.WriteString("6\n4\n") + case 14: + default: + fmt.Fprintf(&b, "%d\n", i) + } + } + diff := hunkStarts(udiff.Unified("a", "b", a.String(), b.String())) + var heads []string + for _, l := range strings.Split(diff, "\n") { + if strings.HasPrefix(l, "@@") { + heads = append(heads, l) + } + } + // diff -u: the move nets nothing, the deleted 14 one line. + if want := []string{"@@ -1,9 +1,9 @@", "@@ -11,7 +11,6 @@"}; strings.Join(heads, "|") != strings.Join(want, "|") { + t.Fatalf("hunk headers %q, want %q\n%s", heads, want, diff) + } +} + +// TestHistoryDiffUpdate: an update's diff is only what it changed; the +// object stepped back to its old form stays where it was. +func TestHistoryDiffUpdate(t *testing.T) { + e := newEnv(t) + e.call(e.token, "POST", "/zones", model.Zone{Name: "lan", Interfaces: []string{"eth1"}}, nil) + e.call(e.token, "POST", "/zones", model.Zone{Name: "wan", Interfaces: []string{"wan"}, External: true}, nil) + var ids []int64 + for i, port := range []string{"8096", "443", "50300", "7777", "25"} { + var mr api.MutationResponse + e.call(e.token, "POST", "/forwards", map[string]any{"name": "f" + port, "ports": port, "to": fmt.Sprintf("10.0.10.%d", 30+i), + "public_addresses": []string{"203.0.113.1"}, "hairpin": true}, &mr) + var f model.PortForward + if err := json.Unmarshal(mr.Object, &f); err != nil { + t.Fatal(err) + } + ids = append(ids, f.ID) + } + var mr api.MutationResponse + if code := e.call(e.token, "PATCH", "/forwards/"+itoa(ids[2]), map[string]any{"hairpin": false}, &mr); code != 200 { + t.Fatalf("update: %d", code) + } + var out struct{ Diff string } + if code := e.call(e.token, "GET", "/history/"+itoa(mr.HistoryID)+"/diff", nil, &out); code != 200 { + t.Fatalf("diff: %d", code) + } + var changed []string + for _, l := range strings.Split(out.Diff, "\n") { + if (strings.HasPrefix(l, "+") || strings.HasPrefix(l, "-")) && !strings.HasPrefix(l, "+++") && !strings.HasPrefix(l, "---") { + changed = append(changed, l) + } + } + if len(changed) != 1 || !strings.HasPrefix(changed[0], "-") || !strings.Contains(changed[0], "-hairpin") || + !strings.Contains(out.Diff, "(before "+itoa(mr.HistoryID)+")") || !strings.Contains(out.Diff, "@@ chain srcnat") { + t.Errorf("entry %d's diff, want only the hairpin line gone, in chain srcnat:\n%s", mr.HistoryID, out.Diff) + } +} diff --git a/internal/server/h_config.go b/internal/server/h_config.go index dc05939..5c9bc53 100644 --- a/internal/server/h_config.go +++ b/internal/server/h_config.go @@ -3,6 +3,7 @@ package server import ( "encoding/json" "errors" + "fmt" "net/http" "net/netip" "sort" @@ -98,11 +99,71 @@ func (s *Server) respondChange(w http.ResponseWriter, r *http.Request, p *auth.P // rulesetDiff is the unified diff of the rulesets two configurations // make: the file housecarl-ruleset.service loads, now and after. func (s *Server) rulesetDiff(before, after *model.State) string { + return s.labeledDiff(before, after, "now", "after") +} + +// labeledDiff is rulesetDiff with its own names for the two sides. +func (s *Server) labeledDiff(before, after *model.State, from, to string) string { if before == nil || after == nil { return "" } now, then := s.fw.Render(before).Text, s.fw.Render(after).Text - return blockHeaders(udiff.Unified("ruleset.nft (now)", "ruleset.nft (after)", now, then), now, then) + diff := hunkStarts(udiff.Unified("ruleset.nft ("+from+")", "ruleset.nft ("+to+")", now, then)) + return blockHeaders(diff, now, then) +} + +// hunkStarts numbers each hunk's new side again, from its old side and +// the lines the hunks before it added and removed. go-udiff (v0.4.1) +// leaves out of its count the unchanged lines between two edits of one +// hunk, so after a hunk like a moved line every later one started too +// early ("@@ -76,9 +74,9 @@" for "+76,9"), and blockHeaders named the +// chain before it. +func hunkStarts(diff string) string { + lines := strings.SplitAfter(diff, "\n") + delta := 0 + for i := 0; i < len(lines); i++ { + f := strings.Fields(lines[i]) + if !strings.HasPrefix(lines[i], "@@ ") || len(f) < 4 || f[3] != "@@" { + continue + } + start, _, _ := strings.Cut(f[1][1:], ",") + from, err := strconv.Atoi(start) + if err != nil { + continue + } + oldN, newN := 0, 0 + for _, l := range lines[i+1:] { + if strings.HasPrefix(l, "@@ ") { + break + } + switch { + case strings.HasPrefix(l, "-"): + oldN++ + case strings.HasPrefix(l, "+"): + newN++ + case strings.HasPrefix(l, " "): + oldN++ + newN++ + } + } + // An empty side is numbered by the line before it. + first := from + if oldN == 0 { + first++ + } + to := first + delta + if newN == 0 { + to-- + } + side := "+" + strconv.Itoa(to) + if newN > 1 { + side += "," + strconv.Itoa(newN) + } + f[2] = side + lines[i] = strings.Join(f, " ") + "\n" + delta += newN - oldN + } + return strings.Join(lines, "") } // blockHeaders names, after each hunk's @@ line, the chain (or other @@ -705,7 +766,8 @@ func (s *Server) historyDiff(w http.ResponseWriter, r *http.Request, p *auth.Pri return err } } - writeJSON(w, http.StatusOK, map[string]string{"diff": s.rulesetDiff(st, after)}) + before, done := fmt.Sprintf("before %d", id), fmt.Sprintf("after %d", id) + writeJSON(w, http.StatusOK, map[string]string{"diff": s.labeledDiff(st, after, before, done)}) return nil }