From 9863dd1cdd49f35c0c8952d104c045a93d019fe9 Mon Sep 17 00:00:00 2001 From: abrown38 Date: Wed, 21 Oct 2026 12:57:18 +0100 Subject: [PATCH] Quotas: carry what was used when a rule counts differently model.QuotaCarry says what becomes of a quota's use when its rule changes how it counts; the firewall's apply follows it, comparing with the rules in force: per address to a total starts at the addresses' sum, wider groups add up their addresses, and what can't be carried (a total to per address, per source to per destination, finer groups) starts again, with the engine warning quota_restarts on that change. Before, per source to per destination kept the sources' use as destinations', and regrouping left old entries behind. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++ docs/rules.md | 14 ++++++ docs/validation.md | 1 + internal/engine/engine.go | 18 ++++++++ internal/engine/engine_test.go | 46 +++++++++++++++++++ internal/model/objects.go | 39 +++++++++++++++++ internal/model/quota_test.go | 26 +++++++++++ internal/nft/nft.go | 80 ++++++++++++++++++++++++++++++++++ internal/nft/nft_test.go | 56 +++++++++++++++++++++++- scripts/e2e.sh | 9 ++++ 10 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 internal/model/quota_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e136dde..7dfcc13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,12 @@ All notable changes to Housecarl are documented here. The format follows ### Fixed +- Changing how a quota counts lost or muddled what it used: per address + to a total started at nothing, per source to per destination kept the + sources' use as if it were the destinations', and other groups left + the old entries behind. Now a total starts at the addresses' sum, + wider groups add up their addresses, and what can't be carried starts + again with a warning (`quota_restarts`) on the change. - `hc stats top devices` listed each device twice (once for what it received, once for what it sent); it is one row with both again. - `hc rule add --before/--after/--first` made two changes (create at the diff --git a/docs/rules.md b/docs/rules.md index 45d8907..15f52a9 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -215,6 +215,20 @@ A per-address quota counts an address: a device that gets a new one starts again. With Steward's reservations IPv4 addresses stay; IPv6 is grouped by /64 (below). +Changing the amount, `over` or the resets keeps what was used. Changing +how the quota counts carries what can be carried: + +| Change | What was used | +|---|---| +| per address to one total | the addresses' sum | +| to wider groups (IPv6 /64 to /56) | added up per new group | +| a total to per address | starts again: a total can't be split | +| per source to per destination (or back) | starts again | +| to finer groups (/64 to /128) | starts again: a group can't be split | + +A change that starts a quota again says so (warning `quota_restarts`), +when it is staged for review and in `hc` (also with `-n`). + **Rules with a quota or a byte rate count every packet.** The built-in that accepts established connections comes before your rules, so a rule there only sees each connection's first packet. Rules that count traffic diff --git a/docs/validation.md b/docs/validation.md index b6d54bb..89bc2f7 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -97,6 +97,7 @@ These apply to rules, source NAT and redirects. | `rule_group` | error | a grouping prefix outside 1-32 (IPv4) or 1-128 (IPv6) | | `rule_traffic_accept` | error | a rule with a quota or a byte rate that accepts: it is checked before established connections are accepted, so it can only drop, reject or log | | `rule_every_packet_accept` | error | a rule checked against every packet (`every_packet`) that accepts: it comes before established connections are accepted, so it can only drop, reject or log | +| `quota_restarts` | warning | a change makes a rule count its quota so that what it used can't carry over (a total to per address, per source to per destination, finer groups): it starts again. Only on the change that does it | | `flowtable_traffic` | warning | a forward rule checked against every packet (a quota, a byte rate, `every_packet`) while the flowtable is on: it only sees each connection's first packets | | `reject_with` | error | `tcp-reset` on a rule that matches more than TCP, or with an action other than reject | | `log_prefix` | error | a log prefix over 100 characters | diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 01adbcd..514ce1f 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -191,6 +191,7 @@ func (e *Engine) commit(ctx context.Context, p *auth.Principal, description stri // Only report warnings the change brings; old ones are on the status // page. res.Warnings = newProblems(validate.Check(e.state, e.env()), problems) + res.Warnings = append(res.Warnings, quotaRestarts(e.state, next)...) if e.Firewall != nil { if err := e.Firewall.Check(ctx, next); err != nil { return res, &Error{Kind: KindFirewall, Message: "nftables refused the ruleset: " + err.Error()} @@ -230,6 +231,23 @@ func (e *Engine) commit(ctx context.Context, p *auth.Principal, description stri return res, nil } +// quotaRestarts warns of the rules whose quota starts again because they +// now count it in a way what they used can't be carried to. +func quotaRestarts(before, after *model.State) []validate.Problem { + was := map[int64]*model.Rule{} + for _, r := range before.Rules { + was[r.ID] = r + } + var out []validate.Problem + for _, r := range after.Rules { + if carry, why := model.QuotaCarry(was[r.ID], r); carry == model.QuotaRestarts { + out = append(out, validate.Problem{Severity: validate.Warning, Code: "quota_restarts", Object: r.Label(), + Message: "its quota starts again: " + why}) + } + } + return out +} + func newProblems(before, after []validate.Problem) []validate.Problem { seen := map[string]bool{} for _, p := range before { diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index c78f3fc..cf76966 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -173,3 +173,49 @@ func TestPositionsStayStableUntilCrowded(t *testing.T) { t.Errorf("order %v", order) } } + +// TestQuotaRestartsWarns: a change that counts a quota so what it used +// can't carry over warns; one that can (to a total, wider groups) doesn't. +func TestQuotaRestartsWarns(t *testing.T) { + e := newEngine(t) + ctx := context.Background() + var id int64 + if _, err := e.Apply(ctx, admin, "", Options{}, func(st *model.State, newID func() int64) error { + id = newID() + st.Rules = append(st.Rules, &model.Rule{ID: id, Chain: "forward", Action: "drop", Quota: &model.Quota{Bytes: 1 << 30, Over: true, PerSource: true}}) + return nil + }); err != nil { + t.Fatal(err) + } + change := func(f func(r *model.Rule)) []string { + res, err := e.Apply(ctx, admin, "", Options{DryRun: true}, func(st *model.State, _ func() int64) error { + f(st.ByID(model.KindRule, id).(*model.Rule)) + return nil + }) + if err != nil { + t.Fatal(err) + } + var codes []string + for _, w := range res.Warnings { + if w.Code == "quota_restarts" { + codes = append(codes, w.Message) + } + } + return codes + } + if w := change(func(r *model.Rule) { r.Quota.PerSource, r.Quota.PerDestination = false, true }); len(w) != 1 || !strings.Contains(w[0], "per source can't be counted per destination") { + t.Errorf("per source to per destination: %v", w) + } + if w := change(func(r *model.Rule) { r.Quota.PerSource = false }); len(w) != 0 { + t.Errorf("to a total (summed): %v", w) + } + if w := change(func(r *model.Rule) { r.GroupV6 = 56 }); len(w) != 0 { + t.Errorf("wider groups: %v", w) + } + if w := change(func(r *model.Rule) { r.GroupV6 = 128 }); len(w) != 1 { + t.Errorf("finer groups: %v", w) + } + if w := change(func(r *model.Rule) { r.Quota.Bytes = 2 << 30 }); len(w) != 0 { + t.Errorf("another amount: %v", w) + } +} diff --git a/internal/model/objects.go b/internal/model/objects.go index 5df8ad5..80b27e1 100644 --- a/internal/model/objects.go +++ b/internal/model/objects.go @@ -274,6 +274,45 @@ func (r *Rule) Groups() (v4, v6 int) { return v4, v6 } +// What becomes of what a rule's quota used when the rule changes +// (QuotaCarry). +const ( + QuotaKept = "" // counted as before: carried as it is + QuotaSummed = "summed" // per address to a total: the addresses' sum + QuotaRegrouped = "regrouped" // per address in wider groups: added up per group + QuotaRestarts = "restarts" // can't be carried: starts again +) + +// QuotaCarry says what becomes of what old's quota used when the rule +// becomes new (both the same rule, with quotas), and for QuotaRestarts +// why. The amount, over and the resets don't matter: only how it counts. +func QuotaCarry(old, new *Rule) (carry, why string) { + if old == nil || new == nil || old.Quota == nil || new.Quota == nil { + return QuotaKept, "" + } + was, is := old.Quota.PerAddress(), new.Quota.PerAddress() + words := map[string]string{"saddr": "per source", "daddr": "per destination", "": "in total"} + switch { + case was == "" && is == "": + return QuotaKept, "" + case is == "": + return QuotaSummed, "" + case was == "": + return QuotaRestarts, "a total can't be split by address, so each address starts at nothing" + case was != is: + return QuotaRestarts, "what was counted " + words[was] + " can't be counted " + words[is] + } + o4, o6 := old.Groups() + n4, n6 := new.Groups() + switch { + case o4 == n4 && o6 == n6: + return QuotaKept, "" + case n4 > o4 || n6 > o6: + return QuotaRestarts, "addresses are grouped more finely now, and a group's use can't be split" + } + return QuotaRegrouped, "" +} + // PortForward publishes a service on an inside host: incoming connections // to a public address and port are sent on (DNAT) to To[:ToPort], let // through the forward chain, and, with Hairpin, also work from inside. diff --git a/internal/model/quota_test.go b/internal/model/quota_test.go new file mode 100644 index 0000000..22c1010 --- /dev/null +++ b/internal/model/quota_test.go @@ -0,0 +1,26 @@ +package model + +import "testing" + +func TestQuotaCarry(t *testing.T) { + rule := func(per string, v4, v6 int) *Rule { + return &Rule{GroupV4: v4, GroupV6: v6, Quota: &Quota{Bytes: 1, PerSource: per == "src", PerDestination: per == "dst"}} + } + for _, c := range []struct { + old, new *Rule + want string + }{ + {rule("", 0, 0), rule("", 0, 0), QuotaKept}, + {rule("src", 0, 0), rule("src", 32, 64), QuotaKept}, // the defaults, written out + {rule("src", 0, 0), rule("", 0, 0), QuotaSummed}, + {rule("", 0, 0), rule("src", 0, 0), QuotaRestarts}, + {rule("src", 0, 0), rule("dst", 0, 0), QuotaRestarts}, + {rule("src", 0, 0), rule("src", 24, 56), QuotaRegrouped}, + {rule("src", 0, 0), rule("src", 24, 128), QuotaRestarts}, + {nil, rule("src", 0, 0), QuotaKept}, + } { + if got, _ := QuotaCarry(c.old, c.new); got != c.want { + t.Errorf("%+v -> %+v: %q, want %q", c.old, c.new, got, c.want) + } + } +} diff --git a/internal/nft/nft.go b/internal/nft/nft.go index de49101..35d9522 100644 --- a/internal/nft/nft.go +++ b/internal/nft/nft.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "net" + "net/netip" "os" "os/exec" "path/filepath" @@ -424,6 +425,9 @@ type Firewall struct { restore *QuotaUsage // adjust are quota changes the next apply makes (AdjustQuota). adjust []QuotaAdjust + // quotaRules are the rules with a quota in the ruleset in force, by + // ID: what their usage was counted by (nil before the first apply). + quotaRules map[int64]*model.Rule // flowDevs are the flowtable's devices in the ruleset in force, by // name, with their interface index then (0: left out, missing). flowDevs map[string]int @@ -510,6 +514,73 @@ func (u *QuotaUsage) apply(adj []QuotaAdjust) { } } +// carry keeps what quotas used when their rules now count differently +// (model.QuotaCarry): per address to a total, the sum; to wider groups, +// the groups' sums; what can't be carried starts again. was are the +// rules in force before (nil: nothing to compare with). +func (u *QuotaUsage) carry(was map[int64]*model.Rule, st *model.State) { + for _, r := range st.Rules { + old := was[r.ID] + carry, _ := model.QuotaCarry(old, r) + if carry == model.QuotaKept { + continue + } + name := render.QuotaName(r.ID) + sets := []string{name + "_v4", name + "_v6"} + switch carry { + case model.QuotaSummed: + var sum int64 + for _, set := range sets { + for _, e := range u.QuotaSets[set] { + sum += e.Used + } + } + if u.Quotas == nil { + u.Quotas = map[string]int64{} + } + u.Quotas[name] = sum + case model.QuotaRegrouped: + v4, v6 := r.Groups() + for _, set := range sets { + byKey := map[string]int64{} + var order []string + for _, e := range u.QuotaSets[set] { + a, err := netip.ParseAddr(e.Addr) + if err != nil { + continue + } + bits := v4 + if a.Is6() { + bits = v6 + } + p, _ := a.Prefix(bits) + k := p.Addr().String() + if _, ok := byKey[k]; !ok { + order = append(order, k) + } + byKey[k] += e.Used + } + if len(order) == 0 { + continue + } + var es []render.Element + for _, k := range order { + es = append(es, render.Element{Addr: k, Used: byKey[k]}) + } + u.QuotaSets[set] = es + } + continue + } + // What was counted the other way goes: a total, or the entries. + if carry != model.QuotaSummed { + delete(u.Quotas, name) + } + for _, set := range sets { + delete(u.QuotaSets, set) + } + } +} + // ResetQuota starts a rule's quota again: the named quota, or the // per-address quota sets (both families). func (f *Firewall) ResetQuota(ctx context.Context, ruleID int64, perAddress bool) error { @@ -579,6 +650,7 @@ func (f *Firewall) Apply(ctx context.Context, st *model.State) error { usage = old.QuotaUsage } usage.merge(f.restore) + usage.carry(f.quotaRules, st) usage.apply(f.adjust) f.adjust = nil opt.Quotas, opt.QuotaSets = usage.Quotas, usage.QuotaSets @@ -589,6 +661,14 @@ func (f *Firewall) Apply(ctx context.Context, st *model.State) error { return err } f.restore = nil + f.quotaRules = map[int64]*model.Rule{} + for _, r := range st.Rules { + if r.Quota != nil { + c, q := *r, *r.Quota // only the quota and the grouping are read + c.Quota = &q + f.quotaRules[r.ID] = &c + } + } f.flowDevs = map[string]int{} for _, d := range st.Settings.FlowtableDevices { if opt.Interfaces == nil || opt.Interfaces[d] { diff --git a/internal/nft/nft_test.go b/internal/nft/nft_test.go index 9ee965c..45ba76d 100644 --- a/internal/nft/nft_test.go +++ b/internal/nft/nft_test.go @@ -197,6 +197,21 @@ func TestQuotasInContainer(t *testing.T) { if t2, d2 := used(); t2 > total-1000 || t2 < total-2000 || d2 != 0 { t.Errorf("after giving back: total %d (was %d, gave 1000), per destination %d (was %d, reset)", t2, total, d2, dev) } + // Counted in total now: what the addresses used carries over. + ping("127.0.0.3") + _, dev = used() + st.Rules[1].Quota.PerDestination = false + if err := fw.Apply(ctx, st); err != nil { + t.Fatal(err) + } + tb, _ := fw.Table(ctx) + if tb.Quotas["quota_r2"] < dev || dev == 0 { + t.Errorf("per destination to a total: %d, was %d", tb.Quotas["quota_r2"], dev) + } + st.Rules[1].Quota.PerDestination = true + if err := fw.Apply(ctx, st); err != nil { + t.Fatal(err) + } // Reset by hand. if err := fw.ResetQuota(ctx, 1, false); err != nil { t.Fatal(err) @@ -211,7 +226,7 @@ func TestQuotasInContainer(t *testing.T) { // saved usage comes back. ping("127.0.0.2") ping("127.0.0.3") - tb, _ := fw.Table(ctx) + tb, _ = fw.Table(ctx) saved := tb.QuotaUsage exec.Command("docker", "exec", name, "nft", "delete", "table", "inet", "housecarl").Run() fw.RestoreQuotas(&saved) @@ -240,6 +255,45 @@ func TestQuotaAdjust(t *testing.T) { } } +// TestQuotaCarry: usage follows a rule counting its quota differently. +func TestQuotaCarry(t *testing.T) { + perSource := &model.Rule{ID: 2, Quota: &model.Quota{Bytes: 1 << 30, PerSource: true}} + usage := func() QuotaUsage { + return QuotaUsage{Quotas: map[string]int64{}, QuotaSets: map[string][]render.Element{ + "quota_r2_v4": {{Addr: "10.0.0.1", Used: 100}, {Addr: "10.0.0.2", Used: 50}}, + "quota_r2_v6": {{Addr: "2001:db8:0:1::", Used: 7}, {Addr: "2001:db8:0:2::", Used: 3}}}} + } + was := map[int64]*model.Rule{2: perSource} + with := func(f func(r *model.Rule)) *model.State { + r := *perSource + q := *perSource.Quota + r.Quota = &q + f(&r) + return &model.State{Rules: []*model.Rule{&r}} + } + u := usage() + u.carry(was, with(func(r *model.Rule) { r.Quota.PerSource = false })) + if u.Quotas["quota_r2"] != 160 || len(u.QuotaSets["quota_r2_v4"]) != 0 { + t.Errorf("to a total: %+v", u) + } + u = usage() + u.carry(was, with(func(r *model.Rule) { r.GroupV4, r.GroupV6 = 24, 48 })) + want := map[string][]render.Element{"quota_r2_v4": {{Addr: "10.0.0.0", Used: 150}}, "quota_r2_v6": {{Addr: "2001:db8::", Used: 10}}} + if !reflect.DeepEqual(u.QuotaSets, want) { + t.Errorf("wider groups: %+v", u.QuotaSets) + } + u = usage() + u.carry(was, with(func(r *model.Rule) { r.Quota.PerSource, r.Quota.PerDestination = false, true })) + if len(u.QuotaSets) != 0 { + t.Errorf("per destination now: %+v", u.QuotaSets) + } + u = usage() + u.carry(nil, with(func(r *model.Rule) { r.Quota.PerSource = false })) + if len(u.QuotaSets["quota_r2_v4"]) != 2 || u.Quotas["quota_r2"] != 0 { + t.Errorf("nothing to compare with (the first apply): %+v", u) + } +} + // TestDeviceCountsInContainer: the per-device counters are read back and // survive a reload. func TestDeviceCountsInContainer(t *testing.T) { diff --git a/scripts/e2e.sh b/scripts/e2e.sh index 74dcbe7..db86699 100755 --- a/scripts/e2e.sh +++ b/scripts/e2e.sh @@ -342,6 +342,15 @@ run "the device is back" 0 send 1000 run "the reset is audited" 0 H audit --action quota. contains "quota.reset" contains "quota.give" +# Counted in total now: what the device used carries over; per +# destination can't take it, and says so. +run "within the quota" 0 send 300000 +run "to one total" 0 H rule update "$QR" --quota 1mbytes,over +not_contains "quota_restarts" +run "the total carried over" 0 H quotas +contains "e2e quota) *[0-9.]* KB" +run "per destination starts again" 0 H rule update "$QR" --quota 1mbytes,over,per-destination +contains "quota_restarts" run "the quota rule goes" 0 H rule delete -y "$QR" # ---- the zone matrix, of the imported ruleset