fix: stop differential apply rewriting unchanged rules #18

Merged

benvin merged 4 commits from benvin/diff-expr-equality into main 2026-10-03 21:24:49 +10:00

Conversation 3 Commits 4 Files Changed 5

+273 -54

unkin-agent commented 2026-10-03 20:48:30 +10:00

Member

Copy Link

Copy Source

Plan re-added unchanged rules every run (string-compared exprs, kernel-canonicalised Queue/Redir) and applied changes in random order, breaking first-match chains.

• Compare rules by value (reflect.DeepEqual); compile Queue (Total: 1) and Redir (RegisterProtoMax, NF_NAT_RANGE_PROTO_SPECIFIED) in the shape the kernel reads back (verified in a netns).
• Diff per chain in rule order: keep the common prefix/suffix, replace the middle, insert new rules before the first kept rule (InsertRule + Position) or append.
• Sort zone expansion so compiles are deterministic.
• Tests: repeat compiles diff empty, Queue/Redir readback shape, fresh-apply order, mid-chain insert position, expanded port/proto/zone-list rule replaced in place with an empty second plan.

Plan re-added unchanged rules every run (string-compared exprs, kernel-canonicalised Queue/Redir) and applied changes in random order, breaking first-match chains. - Compare rules by value (`reflect.DeepEqual`); compile Queue (`Total: 1`) and Redir (`RegisterProtoMax`, `NF_NAT_RANGE_PROTO_SPECIFIED`) in the shape the kernel reads back (verified in a netns). - Diff per chain in rule order: keep the common prefix/suffix, replace the middle, insert new rules before the first kept rule (`InsertRule` + `Position`) or append. - Sort zone expansion so compiles are deterministic. - Tests: repeat compiles diff empty, Queue/Redir readback shape, fresh-apply order, mid-chain insert position, expanded port/proto/zone-list rule replaced in place with an empty second plan.

unkin-agent added 1 commit 2026-10-03 20:48:30 +10:00

fix: compare rule expressions by value in diff e16d95fb63

unkin-agent commented 2026-10-03 20:49:40 +10:00

Author

Member

Copy Link

Copy Source

• internal/nftables/diff.go:67 — cs.Add/cs.Remove are still built by ranging desiredByTag/currentByTag (map order), so across tags the order is random; rule order within a chain is first-match, so a fresh apply or multi-rule replace can land rules in a random order, and Summary() output is unstable → iterate in a deterministic order (desired chain order then slice index, or sort keys) and assert it in a test.
• internal/nftables/compiler_test.go:1550 — the test never covers the Queue/Redir canonicalisation (no NFQueue or redirect-with-port rule in diffTestConfig) and never compares against a kernel-readback-shaped state, so reverting those two compiler.go changes keeps it green → add an nfqueue rule and a redirect-with-port rule, and compare against a hand-built readback-shaped state (Queue Total 1; Redir Min/Max=1 + PROTO_SPECIFIED).
• internal/nftables/compiler.go:459 — Redir readback normalisation is asserted from the PR text, not verified against a real kernel/nft dump → confirm against a real readback before relying on reflect.DeepEqual.
• nit: PR bundles diff equality, compiler canonicalisation and zone sort; split into diff.go+test and compiler.go+test if you want atomic PRs.

- internal/nftables/diff.go:67 — `cs.Add`/`cs.Remove` are still built by ranging `desiredByTag`/`currentByTag` (map order), so across tags the order is random; rule order within a chain is first-match, so a fresh apply or multi-rule replace can land rules in a random order, and `Summary()` output is unstable → iterate in a deterministic order (desired chain order then slice index, or sort keys) and assert it in a test. - internal/nftables/compiler_test.go:1550 — the test never covers the Queue/Redir canonicalisation (no `NFQueue` or redirect-with-port rule in `diffTestConfig`) and never compares against a kernel-readback-shaped state, so reverting those two compiler.go changes keeps it green → add an nfqueue rule and a redirect-with-port rule, and compare against a hand-built readback-shaped state (Queue Total 1; Redir Min/Max=1 + PROTO_SPECIFIED). - internal/nftables/compiler.go:459 — Redir readback normalisation is asserted from the PR text, not verified against a real kernel/nft dump → confirm against a real readback before relying on `reflect.DeepEqual`. - nit: PR bundles diff equality, compiler canonicalisation and zone sort; split into diff.go+test and compiler.go+test if you want atomic PRs.

unkin-agent added 1 commit 2026-10-03 20:52:03 +10:00

fix: preserve rule order in diff and insert replacements in place df7ebb8efe

unkin-agent commented 2026-10-03 20:53:06 +10:00

Author

Member

Copy Link

Copy Source

No findings.

No findings.

unkin-agent added 1 commit 2026-10-03 21:15:31 +10:00

Merge remote-tracking branch 'origin/main' into benvin/diff-expr-equality ... aa0d3e10c2

# Conflicts:
#	internal/nftables/compiler.go
#	internal/nftables/compiler_test.go

unkin-agent added 1 commit 2026-10-03 21:16:41 +10:00

Merge remote-tracking branch 'origin/main' into benvin/diff-expr-equality ... 1ad025a4b7

# Conflicts:
#	internal/nftables/diff.go

unkin-agent commented 2026-10-03 21:17:45 +10:00

Author

Member

Copy Link

Copy Source

No findings.

No findings.

benvin merged commit 78afe7c242 into main 2026-10-03 21:24:49 +10:00

benvin deleted branch benvin/diff-expr-equality 2026-10-03 21:24:49 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/tomswall#18