fix(network/policy): handle overlapping CIDRs in workload policy address sets

[alex-au]

Description

Workload-policy address sets are declared flags interval with no auto-merge, so nft refuses any membership write that puts a CIDR into a set alongside one that covers it:

Error: conflicting intervals specified
        set bn-publisher { type ipv4_addr; flags interval;
                           elements = { 10.0.0.0/24, 10.0.0.5/32 }; }

Reachable from network policy create, add and set, and from the traffic-shaper daemon's poll loop.

Containment is now handled in Go rather than by the kernel. Because CIDR prefixes form a tree, containment is the only possible overlap between two prefixes and the only thing nft rejects — so no interval arithmetic is needed, only a containment filter. Operator-authored paths reject and name both prefixes; the daemon's derived path drops covered prefixes, because it fully replaces each set every tick and never deletes an individual element.

network policy remove also stops leaking nft's bare element does not exist.

Why not just add auto-merge?

This is the obvious fix and it is deliberately not taken. Measured against nftables v1.0.6:

auto-merge does resolve the example above — but it also merges adjacency, and merged adjacency is usually not CIDR syntax at all:

10.0.0.1/32 + 10.0.0.2/32   ->  10.0.0.1-10.0.0.2      # a range
10.0.0.0/24 + 10.0.1.0/25   ->  10.0.0.0-10.0.1.127

Two consecutive peer IPs is the daemon's most ordinary input from statusz. That range form breaks the Go side:

netip.ParsePrefix("10.0.0.1-10.0.0.2")   -> err: no '/'
sanity.ValidateCIDR("10.0.0.1-10.0.0.2") -> invalid CIDR
parseElement(...)                        -> parsed=false

DiffElements(desired=[10.0.0.1/32, 10.0.0.2/32], live=[10.0.0.1-10.0.0.2])
  -> Adds=[10.0.0.1 10.0.0.2] Deletes=[10.0.0.1-10.0.0.2] Empty=false   # forever

So auto-merge would introduce per-tick churn for input that is perfectly valid and non-overlapping. It also breaks delete element outright — delete { 10.0.0.5/32 } after a fold returns Error: element does not exist; nft does not split the interval.

The host firewall gets away with auto-merge (#1002/#1004) for a structural reason: there the kernel is write-only. firewall.Runner has no element-level operations, Manager.load reads the authoritative YAML (falling back to the .nft file we rendered, not nft list), and the only kernel content read is runner.List inside Show, which is printed and never parsed. The policy plane is the opposite — membership is never persisted, the kernel is the only copy, and ListElements feeds snapshot/restore, the daemon's diff, and show.

Measured nft behaviour

The design rests on these, verified in a privileged container rather than inferred:

Case	flags interval, no auto-merge
adjacent /25 + /25	ACCEPT — so adjacency merging is unnecessary
exact duplicate	ACCEPT — so only strict containment conflicts, and idempotent re-add keeps working
contained /24 + /32	REJECT conflicting intervals specified
delete element of a non-member	ERROR, and the batch is atomic — removes nothing
nft -f with any error	atomic — table and membership left fully intact

The last row is why no nft -c dry-run was added: unlike the firewall (which persisted its boot artifact before applying, the actual bug in #1002), the policy plane already applies before persisting, so a dry-run would be dead code.

Files changed

File	Change
internal/network/policy/cidrset.go	new — covers, containmentPair, PruneContainedCIDRs, rejectContainment, rejectMissingMembers
internal/network/policy/cidrset_test.go	new — helper tables plus manager-level behaviour
internal/network/policy/manager.go	Create/Add reject; Remove pre-checks membership; applySet gains membershipSource; new liveMembership
internal/network/policy/render.go	comment recording why auto-merge is absent here (no behaviour change)
internal/network/policy/policy_test.go	fake enforces nft's containment + missing-element rules; canonical delete matching
internal/blocknode/shaper/policy_map.go	desiredElements prunes, so the reconciler's diff settles
internal/blocknode/shaper/policy_map_test.go	no-churn, digest, and compound-set tests

Review guide

Worth a close look:

• cidrset.go — covers() is strict containment (outer.Bits() < inner.Bits()). An exact duplicate must not be a conflict, or the idempotent re-add that works today would start failing.
• cidrset.go — the sortCIDRElems doc comment argues why comparing each candidate against only the last retained element is sufficient. That is the one non-obvious algorithmic step.
• cidrset.go — parseCIDRElems drops compound <ip> . <port> and unparseable tokens, so neither path can touch them (AC#4).
• PruneContainedCIDRs returns a subset in input order — never rewrites, merges, or invents a prefix. This is what keeps ListElements round-tripping (AC#2).
• manager.go Create — rejection happens before withLock and before any kernel or disk write.
• manager.go Remove — the pre-check runs before the first DeleteElements, so a rejected batch removes nothing.
• policy_map.go desiredElements — pruning here is what makes the diff settle; removing it reintroduces a per-tick re-apply.
• Golden .nft fixtures are unchanged.

Note: the policy package still uses plain errorx rather than the newer errx + reason-code contract in CLAUDE.md. The package has zero errx usage today, so the new errors match their surroundings; adopting errx package-wide is a separate cleanup.

Tests:

go test -race -tags='!integration' ./internal/network/policy/... ./internal/blocknode/shaper/...
task lint

Mutation-checked, so the suite demonstrably bites: forcing covers() to false fails 21 tests; reverting the desiredElements prune fails the 2 churn/digest tests; forcing rejectMissingMembers to nil fails the 5 Remove tests.

Manual UAT

Prereq: a host with the workload-policy table live (after block node install, or network policy create).

SP=solo-provisioner; P=bn-publisher
sudo $SP network policy show --name $P          # record starting membership for cleanup

1. Covered CIDR rejected, both prefixes named

sudo $SP network policy add --name $P --cidr 10.0.0.0/24     # OK
sudo $SP network policy add --name $P --cidr 10.0.0.5/32     # REJECTED

Expected — not nft's text:

policy "bn-publisher" already permits 10.0.0.5/32 through its existing member 10.0.0.0/24:
an nft address set rejects overlapping entries, and 10.0.0.0/24 already matches every address
in 10.0.0.5/32, so nothing needs to be added. To narrow the policy, remove 10.0.0.0/24 first.

2. Reverse direction rejected, with different advice

sudo $SP network policy add --name $P --cidr 10.20.0.5/32    # OK
sudo $SP network policy add --name $P --cidr 10.20.0.0/24    # REJECTED
# expect "...covers the existing member 10.20.0.5/32... Remove 10.20.0.5/32 first..."

3. Exact re-add and adjacency still work (must NOT be rejected)

sudo $SP network policy add --name $P --cidr 10.0.0.0/24                          # OK, idempotent
sudo $SP network policy add --name $P --cidr 10.30.0.0/25 --cidr 10.30.0.128/25   # OK
sudo nft list set inet weaver-workload-policy $P
# expect BOTH /25s present as separate elements, NOT merged into 10.30.0.0/24

4. Overlapping list in one invocation rejected, prior membership intact

sudo $SP network policy set --name $P --cidrs 10.40.0.0/16,10.40.5.0/24   # REJECTED
sudo nft list set inet weaver-workload-policy $P                         # membership unchanged

5. remove gives a legible error

sudo $SP network policy remove --name $P --cidr 10.0.0.5/32
# expect "...covered by 10.0.0.0/24... Removing part of a member is not supported..."
# NOT "Error: element does not exist"

sudo $SP network policy remove --name $P --cidr 192.168.99.0/24
# expect "...is not a member of policy... network policy show --name bn-publisher"

6. A batch containing one absent entry removes nothing

sudo $SP network policy add --name $P --cidr 10.0.1.0/24
sudo $SP network policy remove --name $P --cidr 10.0.1.0/24 --cidr 192.168.99.0/24
# expect REJECTED, naming 192.168.99.0/24
sudo nft list set inet weaver-workload-policy $P    # expect 10.0.1.0/24 STILL PRESENT

7. Removing an exact member still works (AC#3)

sudo $SP network policy remove --name $P --cidr 10.0.1.0/24   # OK
sudo nft list set inet weaver-workload-policy $P              # 10.0.1.0/24 gone

8. Daemon neither wedges nor churns

sudo systemctl restart solo-provisioner-daemon
sudo journalctl -u solo-provisioner-daemon -f | grep -iE 'policy|conflicting|covered'
# expect NO "conflicting intervals specified"
# expect membership sets are not re-applied every tick once settled
# if upstream supplies a covered CIDR, expect a Debug line:
#   "dropped CIDRs already covered by another member of the same policy set"

9. Cleanup — restore the membership recorded in step 0 with network policy set.

Risks

• Operators now see our error instead of nft's for an overlapping add — same outcome, better message.
• Cross-plane inconsistency: firewall add --cidr folds an overlap silently (fix(network/firewall): merge overlapping CIDRs and never persist a rejected ruleset #1004) while policy add rejects it. Deliberate, for the structural reason above; the error text explains it. Worth revisiting if feat(daemon): persist statusz-derived nft set membership so it survives a reboot #990 gives the policy plane a persisted membership record.
• Daemon pruning changes what lands in the kernel when upstream supplies overlapping CIDRs. Previously the whole reconcile errored, so this is strictly more available, and the dropped prefix is redundant by definition — the covering member already matches every address in it.
• Latent, not introduced here: the host firewall's auto-merge is only safe while nothing compares live table content against the persisted config. Adding firewall drift detection would hit this same range-form problem.
• Rollback: revert. Touches only internal/network/policy plus one function in internal/blocknode/shaper.

Related Issues

• Closes fix(network/policy): workload policy address sets reject overlapping CIDRs #1006

[swirlds-automation]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[Copilot]

Pull request overview

This PR fixes nftables flags interval set update failures caused by overlapping (strictly containing) CIDR prefixes in workload-policy address sets, by detecting/pruning containment conflicts in Go rather than relying on nft auto-merge semantics that would introduce lossy range folding and perpetual reconcile churn.

Changes:

• Added CIDR containment detection utilities to reject overlapping operator-authored updates and prune redundant daemon-derived membership.
• Updated policy Manager paths (Create/Add/Set/Remove, daemon apply paths) to enforce the new containment/missing-member behavior while preserving atomicity expectations.
• Updated traffic-shaper desired membership computation to prune covered prefixes so diffing settles (no per-tick re-apply).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/network/policy/cidrset.go	New containment/pruning helpers for CIDR interval-set semantics and improved operator-facing errors.
internal/network/policy/cidrset_test.go	New unit + manager-level tests covering containment detection, pruning, and operator/daemon behavior.
internal/network/policy/manager.go	Enforces containment checks for operator paths, prunes on daemon paths, and adds live membership prechecks for safer mutations.
internal/network/policy/policy_test.go	Enhances the fake nft runner to mirror kernel containment and missing-element atomic failure semantics.
internal/network/policy/render.go	Documents why auto-merge is intentionally not enabled for workload-policy sets.
internal/blocknode/shaper/policy_map.go	Prunes covered desired endpoints so reconciler diffs settle and digest reflects applied kernel membership.
internal/blocknode/shaper/policy_map_test.go	Adds regression tests for no-churn settling, digest stability, and compound-set non-pruning behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[brunodam]

LGTM