fix(network/firewall): merge overlapping CIDRs and never persist a rejected ruleset

[alex-au]

Description

Adding a CIDR to a host-firewall rule that already holds one covering it — the documented
add --name k8s-node --cidr 10.0.0.5/32 against a rule holding 10.0.0.0/24 — made the nft
apply fail, and left the persisted artifacts holding a document that no longer loads at boot:

$ sudo solo-provisioner network firewall add --name k8s-node --cidr 10.0.0.5/32

  Error: failed to execute command
    Cause: os.systemd_operation_error: service solo-provisioner-network-nft.service start failed: failed

$ sudo journalctl -u solo-provisioner-network-nft.service -n 5
sh[29448]: /etc/solo-provisioner/network-weaver-host-firewall.nft:24:75-85: Error: conflicting intervals specified
sh[29448]:         set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24, 10.0.0.9/32 }; }
sh[29448]:                                                                     ~~~~~~~~~~~  ^^^^^^^^^^^

The loud part is the CLI error. The quiet part is the damage: the artifacts were written
before the apply, the unit has no ExecStop so the live table stayed intact, and the host
then came up at next boot with no weaver firewall at all — no management allowlist, no block
list. Three coupled fixes, all three of which the issue asks for.

1. Address sets carry auto-merge

The address sets were declared flags interval without auto-merge, so nft refuses two
overlapping elements outright. The port sets in the same template already carry it, which is
why --port never hit this and --cidr did. With auto-merge the narrower prefix folds into
the one that covers it.

The declarative config keeps both entries — 10.0.0.0/24 and 10.0.0.5/32 — so removing
the wider prefix later correctly leaves the narrower one in force. Only the kernel folds them.
That makes the kernel read-back lossy for addresses the way it already was for ports:
firewall show dumps the kernel and prints the folded form, while firewall show --output yaml reads the config and prints what the operator authored.

2. The ruleset is dry-run before anything is persisted

applyAndPersist rendered, wrote network-weaver-host-firewall.yaml, wrote .nft, and then
restarted the unit. It now renders, dry-runs the document through nft -c -f, and only writes
if that passes. A rejected ruleset therefore never reaches disk, and the artifact that replays
at boot is always one that loads.

The dry run goes through a new Check(ctx, path) on the existing Runner seam in nft.go,
alongside List and Delete — tests already substitute a fake Runner, so the package still
builds and unit-tests on macOS. Past the dry run the config is still written before the nft
artifact, preserving the existing invariant that a crash between the two writes loses the
kernel state rather than the operator's intent.

This is deliberately not apply-then-persist: inverting the order would have lost the intent
while leaving the ruleset live, with nothing left to re-derive it from.

3. One bad apply no longer wedges every later command

The shared unit is restarted on every firewall and policy mutation, so a run of failed applies
tripped systemd's default start rate limit and left every subsequent network firewall
command failing with the same opaque error until someone ran systemctl reset-failed. In the
UAT that found this, one real failure became 18. The unit now sets StartLimitIntervalSec=0 —
there is nothing to rate-limit, it is a oneshot that loads a file and exits.

EnsureNetworkNftUnit stat-and-skipped the unit file, which would have stranded every
already-provisioned host on the unit that shipped when it was first provisioned. It now
compares the on-disk unit against the embedded copy and rewrites on drift. The same change is
in internal/network/policy/service_linux.go, which writes the same shared path, so whichever
plane runs first converges the host rather than the two fighting.

Files changed

File	Change
internal/templates/files/network/network-weaver-host-firewall.nft.tmpl	auto-merge on all eight address set declarations; rewritten rationale comment covering addresses as well as ports
internal/templates/files/network/solo-provisioner-network-nft.service	StartLimitIntervalSec=0
internal/network/firewall/nft.go	Check(ctx, path) added to Runner; execRunner runs nft -c -f and distinguishes a refused ruleset from a broken host
internal/network/firewall/manager.go	applyAndPersist dry-runs before either write; new check helper stages the document beside the real artifact
internal/network/firewall/service_linux.go	EnsureNetworkNftUnit rewrites on content drift instead of stat-and-skip
internal/network/policy/service_linux.go	Same drift handling for the shared unit
internal/network/firewall/firewall_test.go	Check on fakeRunner (with a rejection hook); five new tests
internal/network/firewall/allow_test.go	Set-declaration assertions updated; the legacy-artifact fixture deliberately left un-merged
internal/network/firewall/testdata/*.golden.nft	Regenerated (2 files)
cmd/cli/commands/network/firewall/firewall_test.go, internal/workflows/steps/step_network_firewall_test.go	Check on their fake Runners
docs/dev/traffic-shaper.md, docs/quickstart.md	Set-flag semantics, the dry-run guarantee, and the start-limit note

Deliberately out of scope

internal/network/policy/render.go:221-222 declares the workload-policy address sets with the
identical missing auto-merge. It is not fixed here: those sets are mutated incrementally
by the daemon (add element / delete element), and auto-merge would make a later per-element
delete of a folded prefix fail. Worse, policy set membership is never persisted, and
Manager.Create snapshots it via ListElements before its destructive delete/recreate — with
auto-merge that snapshot reads back folded, so the restore would permanently overwrite the
authored membership. The host firewall is safe to auto-merge precisely because the opposite is
true of it: the YAML config is authoritative, and every mutation re-renders and reloads the whole
table.

Same symptom, genuinely different fix. Filed as #1006.

Also not done: rejecting overlapping CIDRs up front in Rule.AddCIDRs. The issue allows either
behaviour; folding is the friendlier one and keeps the config declarative.

Review guide

Worth a close look:

• manager.go applyAndPersist — the dry run must come before both writes. If it moved
  below the config write, a rejected ruleset would still clobber the operator's intent.
• manager.go check — the document is staged in the nft artifact's own directory, not the
  system temp dir, so the check runs on the same filesystem and permissions the real load sees.
• nft.go Check — a non-zero exit is IllegalFormat (the ruleset is wrong); anything else is
  ExternalError with the cause attached (the host is wrong). Getting these backwards would
  mis-style the error in the doctor layer.
• allow_test.go — the inline legacy fixture in TestManager_LoadsFromLegacyNftWhenConfigMissing
  intentionally keeps the pre-auto-merge rendering. Parse must handle both, and
  TestRoundTrip_RenderParseRender covers the new one.

Test plan

task lint                                     # 0 issues
task test:unit                                # full suite, exit 0
go test ./internal/network/...                # 308 pass

New unit coverage in internal/network/firewall:

Test	Pins
TestRender_OverlappingCIDRsShareAnAutoMergeSet	Every address set renders with auto-merge, allow rules included
TestManager_AddCoveredCIDR	The issue's repro succeeds, and the config keeps both prefixes
TestManager_RejectedRulesetIsNeverPersisted	A refused document leaves both artifacts byte-identical and never restarts the unit
TestManager_ChecksTheDocumentItPersists	The dry run sees exactly the document that lands on disk
TestNetworkNftUnit_HasNoStartLimit	StartLimitIntervalSec=0 survives future template edits

Manual UAT

⚠️ Set mgmt.cidrs to a range that contains your own SSH source address. The input chain
is policy drop; a mgmt allowlist that excludes your client will lock you out of the box the
moment the ruleset applies. The 192.168.50.0/24 below is the UTM VM network — substitute
your own.

Case 1 — the reported repro now succeeds

sudo tee /root/rules.yaml >/dev/null <<'EOF'
version: 1
mgmt:
  cidrs: ["192.168.50.0/24"]
  ports: ["22"]
blocked:
  cidrs: []
in_cluster:
  cidrs: []
allow:
  - name: k8s-node
    cidrs: ["10.0.0.0/24"]
    ports: ["6443"]
EOF

sudo solo-provisioner network firewall create --from-file /root/rules.yaml --force
sudo solo-provisioner network firewall add --name k8s-node --cidr 10.0.0.5/32

Expected: the add succeeds. Before this PR it failed with service ... start failed.

Case 2 — the persisted artifact still loads

sudo nft -c -f /etc/solo-provisioner/network-weaver-host-firewall.nft; echo "exit=$?"

Expected: exit=0, no output. Before this PR this failed with conflicting intervals specified, which is what made the next boot come up bare.

Case 3 — config keeps intent, kernel shows the fold

sudo solo-provisioner network firewall show --output yaml | grep -A4 'k8s-node'
sudo nft list table inet weaver-host-firewall | grep -A5 'set k8s-node '

Expected: the YAML holds both 10.0.0.0/24 and 10.0.0.5/32; the kernel set reads back
as elements = { 10.0.0.0/24 } alone. This asymmetry is the intended trade — verify both halves.

Then confirm the narrower prefix is real rather than swallowed, by removing the wider one:

sudo solo-provisioner network firewall remove --name k8s-node --cidr 10.0.0.0/24
sudo nft list table inet weaver-host-firewall | grep -A5 'set k8s-node '

Expected: the kernel set is now elements = { 10.0.0.5/32 }. Restore it with
add --name k8s-node --cidr 10.0.0.0/24 before continuing.

Case 4 — the firewall survives a reboot

sudo reboot
# after it comes back:
sudo nft list tables | grep weaver-host-firewall
sudo systemctl status solo-provisioner-network-nft.service --no-pager

Expected: the table is present and the unit is active (exited). Before this PR the table
was gone — this is the failure the whole PR exists to prevent.

Case 5 — a rejected ruleset leaves disk untouched

Force a document nft will refuse, by hand-editing the config to something invalid, then
snapshotting the artifacts around a mutation:

sudo cp /etc/solo-provisioner/network-weaver-host-firewall.nft  /root/before.nft
sudo cp /etc/solo-provisioner/network-weaver-host-firewall.yaml /root/before.yaml

# a rule name that renders a set name nft cannot parse
sudo sed -i 's/name: k8s-node/name: k8s node/' /etc/solo-provisioner/network-weaver-host-firewall.yaml
sudo solo-provisioner network firewall add --name mgmt --cidr 198.51.100.4/32

sudo diff /root/before.nft /etc/solo-provisioner/network-weaver-host-firewall.nft && echo 'nft artifact unchanged'
sudo nft -c -f /etc/solo-provisioner/network-weaver-host-firewall.nft; echo "still loads: exit=$?"
sudo cp /root/before.yaml /etc/solo-provisioner/network-weaver-host-firewall.yaml   # restore

Expected: the command errors, the error text names the nft complaint and says nothing was
written, diff reports no change, and nft -c -f still exits 0. If the config validator rejects
the hand-edit before the render is reached, that is also an acceptable pass — the point is that
the on-disk .nft never degrades.

Case 6 — no start-limit wedge

systemctl show solo-provisioner-network-nft.service -p StartLimitIntervalSec

Expected: StartLimitIntervalSec=0. On a host provisioned by an older build, confirm the
unit converges rather than staying stale — check it before running any mutation, then again
after:

grep StartLimitIntervalSec /usr/lib/systemd/system/solo-provisioner-network-nft.service || echo 'absent (pre-upgrade)'
sudo solo-provisioner network firewall add --name mgmt --cidr 198.51.100.5/32
grep StartLimitIntervalSec /usr/lib/systemd/system/solo-provisioner-network-nft.service

Expected: absent before, present after — this is the upgrade path that stat-and-skip would
have blocked.

Verification already done

nft -c -f against nftables v1.1.3 on Debian 13 (arm64), on both renderings of the same
overlapping ruleset:

=== BEFORE (no auto-merge on address sets, overlapping /32):
/tmp/before.nft:33:75-85: Error: conflicting intervals specified
	set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24, 10.0.0.5/32 }; }
	                                                            ~~~~~~~~~~~  ^^^^^^^^^^^
exit=1
=== AFTER (auto-merge, same overlap):
exit=0

And the kernel read-back, loaded as a standalone set-only probe table (no hooked chains, so the
VM's own SSH was never at risk):

$ sudo nft list table inet wv-1002-probe
table inet wv-1002-probe {
	set k8s-node {
		type ipv4_addr
		flags interval
		auto-merge
		elements = { 10.0.0.0/24 }
	}
}

The full create --from-file → add --cidr → reboot walkthrough above has not been run
end-to-end — it needs a VM whose mgmt.cidrs covers its own SSH source, which the available VM
did not have. Cases 1–6 are written to be run by a reviewer on such a host.

Risks

• nft -c -f on every mutation adds an exec to a path that previously had none. A host
  missing the nft binary now fails the mutation outright instead of half-succeeding — arguably
  correct, but it is a new failure mode. The error names the binary and the check.
• auto-merge makes the kernel read-back lossy for addresses. Anything downstream that reads
  nft list and expects to recover the authored CIDR list will now see folded prefixes.
  parse.go is unaffected — it parses the artifact we render, not kernel output — and
  show --output yaml reads the config.
• Rewriting the shared unit on drift touches a file the policy plane also owns. Both call
  sites get the same logic here, so they converge; an unchanged unit is still a no-op with no
  write and no daemon-reload.

Stacking note

Based on 00996-collapse-icmp-path-health-accepts (#997), not main — the code this fixes
(the declarative config, named allow rules, the current applyAndPersist) lives on that branch
via #999 and has not reached main yet. This PR targets #997's branch and will retarget to
main automatically when #997 merges.

The bug itself is older than #999 — the address sets have lacked auto-merge since fd3a73b —
but named allow rules make it far easier to reach, since operators get many more sets to
add --cidr into.

Related Issues

• Fixes fix(network/firewall): overlapping CIDRs break the nft apply and leave an unloadable ruleset persisted #1002

[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

Fixes the host-firewall failure mode where adding an overlapping CIDR causes nft to reject the ruleset and (previously) persisted an unloadable artifact, potentially leaving the host without the weaver firewall on next boot. The PR also hardens the apply path by validating rulesets before persisting and prevents systemd start-limit wedging for the shared oneshot unit.

Changes:

• Add auto-merge to all host-firewall address sets so overlapping CIDRs are accepted by nftables.
• Dry-run (nft -c -f) the rendered ruleset before persisting config/artifacts and restarting the loader unit.
• Disable systemd start limiting for the shared oneshot unit and ensure the unit file is rewritten on content drift; update tests and docs accordingly.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/templates/files/network/network-weaver-host-firewall.nft.tmpl	Add auto-merge to address set declarations and expand rationale comment.
internal/templates/files/network/solo-provisioner-network-nft.service	Disable start rate limiting (StartLimitIntervalSec=0) with rationale.
internal/network/firewall/nft.go	Extend Runner with Check and implement nft -c -f dry-run.
internal/network/firewall/manager.go	Validate rendered ruleset before writing config/artifact and restarting unit.
internal/network/firewall/service_linux.go	Rewrite shared unit on drift (compare embedded vs on-disk).
internal/network/policy/service_linux.go	Apply the same unit drift reconciliation from the policy plane.
internal/network/firewall/firewall_test.go	Add fake Check support and new tests covering overlap + dry-run semantics.
internal/network/firewall/allow_test.go	Update assertions for new set declaration shape (auto-merge).
internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft	Regenerate golden output to include auto-merge and updated comments.
internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft	Regenerate golden output for allow-rules rendering with auto-merge.
cmd/cli/commands/network/firewall/firewall_test.go	Add Check to test runner and avoid writing config to real /etc in tests.
internal/workflows/steps/step_network_firewall_test.go	Add Check stub to step test runner.
docs/quickstart.md	Document overlapping CIDR behavior and “reject-before-write” guarantee.
docs/dev/traffic-shaper.md	Document auto-merge semantics, dry-run-before-persist, and start-limit change.

Suppressed comments (2)

internal/network/firewall/service_linux.go:66

• EnsureNetworkNftUnit can now rewrite the existing systemd unit on drift, but writeEmbedded uses os.WriteFile (truncate+write) which is not atomic. A crash or concurrent writers (firewall vs policy plane) could leave a torn unit file under /usr/lib/systemd/system. Other unit installers in this repo use atomicWriteFile to avoid this (e.g. internal/network/shape/service_linux.go). Consider using the package's atomicWriteFile here as well.

	if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil {
		return errorx.ExternalError.Wrap(err, "failed to create %s", filepath.Dir(destPath))
	}
	if err := os.WriteFile(destPath, content, 0o644); err != nil {
		return errorx.ExternalError.Wrap(err, "failed to write %s", destPath)

internal/network/policy/service_linux.go:45

• defaultEnsureService can now rewrite the shared network-nft unit on content drift, but it writes the unit with os.WriteFile (truncate+write), which is not atomic. A crash or concurrent writers can leave a torn unit file. The policy package already has an atomicWriteFile helper (used for persisted artifacts) that can be reused here to make unit updates durable.

	if err := os.MkdirAll(filepath.Dir(NetworkNftServiceUnitPath), 0o755); err != nil {
		return errorx.ExternalError.Wrap(err, "failed to create %s", filepath.Dir(NetworkNftServiceUnitPath))
	}
	if err := os.WriteFile(NetworkNftServiceUnitPath, content, 0o644); err != nil {
		return errorx.ExternalError.Wrap(err, "failed to write %s", NetworkNftServiceUnitPath)

---

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

[brunodam]

LGTM