feat(network/firewall): declare a named allow rule from the CLI without a config file - #1015
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
This PR extends the host-firewall CLI so operators can declare a named allow rule without authoring a full declarative YAML file, enabling a safe declare-then-populate workflow that renders no accept rules until a rule is complete.
Changes:
- Add
network firewall create-allow-ruleto declare an (initially empty) named allow rule from the CLI. - Relax allow-rule validation to permit declared-but-incomplete rules, and warn at apply-time when allow rules render nothing.
- Add
--proto/--icmp-echosupport tonetwork firewall set, and update docs/tests to cover the new lifecycle.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/network/firewall/rule.go | Adds allow-rule “incomplete” predicate and relaxes validation for allow rules; tightens validation for reserved blocks. |
| internal/network/firewall/table.go | Adds IncompleteAllowRules() helper to surface apply-time warnings for declared-but-empty allow rules. |
| internal/network/firewall/manager.go | Introduces CreateRule, extends Update with Proto/ICMPEcho pointers, and warns on incomplete allow rules during apply. |
| internal/network/firewall/parse.go | Updates Parse docs to reflect the CLI-declaration path for named allow rules. |
| internal/network/firewall/allow_test.go | Adds model/manager rendering + lifecycle tests covering incomplete rules and CLI-vs-file equivalence. |
| cmd/cli/commands/network/firewall/create_allow_rule.go | New Cobra command implementing create-allow-rule. |
| cmd/cli/commands/network/firewall/firewall.go | Registers the new verb and adds shared flag vars for proto/icmp-echo. |
| cmd/cli/commands/network/firewall/set.go | Adds --proto / --icmp-echo flag handling via pointer semantics in fw.Update. |
| cmd/cli/commands/network/firewall/create.go | Updates --from-file help text to no longer claim it’s the only declaration path. |
| cmd/cli/commands/network/firewall/firewall_test.go | Adds end-to-end CLI tests for declare/redeclare, proto/icmp-echo on set, and typo safety. |
| docs/quickstart.md | Documents create-allow-rule, new set flags, and list/range forms. |
| docs/dev/traffic-shaper.md | Updates developer docs to include the new declaration workflow and invariants. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Manual UAT — executedRan the full runbook from the PR description on a freshly reset UTM VM (Debian, arm64), VM at
The parts worth showingUAT-1 — an incomplete rule really does render nothing. After Sets exist and are empty; zero accept rules. Then one The range survived as one element, ports came out sorted, and the echo accept (handle 33) sits UAT-2 — the order that was impossible before. UAT-3 — the typo guard, with a nice touch. All three verbs fail and list the real rule Nothing was created ( UAT-4 — AC#4, byte-identical. Built The emitted YAML also confirms the "only persist what was set" design — UAT-6 — every rejection, including both bugs this PR fixes: UAT-7 — no regressions. All five pre- Each rejection left the live table unchanged ( UAT-8 — boot persistence. A dual-stack UDP + Fully restored by Bonus — the recovery path #1010 will depend on. Deleted Reworded message is correct, and management access is recovered from the Still deferredThe sequence ending in |
2c4d027 to
d4a1aed
Compare
`create --from-file` was the only way to bring a named allow rule into existence, so admitting one monitoring host meant learning a schema and authoring a file that states the whole table -- which is fully declarative, removes any rule it omits, and can fall back to an empty management allowlist under the default-drop policy. Add `network firewall create-allow-rule --name <rule>`, which declares a rule that `add` then populates. Both `--cidr` and `--port` already take lists applied in one transaction, so a rule goes live in a single follow-up command. Expose the two Rule fields that had no flag, `--proto` and `--icmp-echo`, on the new verb and on `set`, so every field of every rule is now reachable from the CLI rather than only from a file. Declaring stays a separate verb from `add`: an unknown --name on add/remove/set still fails, so a typo edits nothing instead of quietly creating a second rule alongside the intended one. Re-declaring an existing name is create-if-missing, matching `create`. This required relaxing Rule.Validate, which rejected an allow rule with no CIDRs, or with no ports and no icmp_echo. A declared-but-unpopulated rule has to be representable for the sequence to work in any order. It is fail-closed: the template gates every emission on the address and port sets being non-empty, so an incomplete rule renders no accept rule at all -- verified against a live kernel. applyAndPersist warns about them instead. Also reject --proto/--icmp-echo on the reserved blocks, which previously accepted and silently ignored them: in_cluster took both, and mgmt and in_cluster accepted proto=tcp because they only rejected a mismatching value. The renderer fixes all three to TCP and the config schema carries no proto field for them, so any value accepted here reported a change that never happened. And fix create-allow-rule --name mgmt reporting "already exists" rather than naming the reserved block -- the reserved-name check has to run ahead of the create-if-missing branch, since those blocks always exist. Refs #1009 Signed-off-by: alex-au <alex.w.aus@gmail.com>
d4a1aed to
acbd22f
Compare
Description
network firewall create --from-filewas the only way to bring a named allow rule intoexistence, as its own help text stated. Everything else about the host firewall is reachable
from the CLI — the three reserved blocks via
create/set/add, and membership inside anexisting named rule via
add/remove/set --name. Only the act of declaring a rule had noCLI form.
That made the file mandatory for a case it was not designed for. An operator who simply wants
to admit their own monitoring host had to learn a schema and author a file that states the
whole table:
--from-fileis fully declarative, so a partial file silently removes rulesand can fall back to an empty management allowlist under the default-drop policy. It also
blocked the interactive path — a prompted flow cannot offer “add an allow rule” when the
underlying capability only accepts a file.
This adds
network firewall create-allow-rule, so the whole lifecycle is CLI-reachable:--cidrand--portalready accepted lists applied in one transaction, so step 2 needs oneaddrather than one per element — which is why the declare verb deliberately takes no--cidr/--portof its own. A real rule has too many of both to fold into the declare line.create --from-fileremains valid and unchanged.The invariant this changes
Rule.Validaterejected an allow rule with no CIDRs (“delete the rule rather than emptyingit”) and one with no ports and no
icmp_echo. A declared-but-unpopulated rule could not existunder those checks, so declare-then-populate was impossible in any order. Both checks are
dropped for allow rules and replaced by a warning on apply.
This is fail-closed rather than fail-open: the template gates every emission on the address
and port sets being non-empty, so an incomplete rule renders no accept rule at all. That
was verified against a real kernel rather than assumed — a table carrying all four incomplete
shapes (declared-empty, cidrs-only, ports-only, echo-without-cidrs) plus one complete rule
produced empty set declarations only, passed
nft -c -f, and loaded live with accepts emittedsolely for the complete rule.
Reserved blocks keep every existing restriction; the relaxation is scoped to the
default:branch of the
switch r.Name.Two bugs found while building this
in_clusteracceptedprotoandicmp_echoand silently ignored both — the templatehardcodes TCP for all three reserved blocks and gives them no echo accept. Newly rejected,
which matters now that
set --protocan reach them.create-allow-rule --name mgmtinitially reported “already exists” and succeeded, becausethe reserved blocks always exist and hit the create-if-missing branch. The reserved-name
check now runs first. Caught by a test, not by review.
Related Issues
--from-file): its acceptance criterion “every field of every rule,reserved and named, is settable through CLI verbs alone” is satisfied by this PR —
Nameviacreate-allow-rule --name,CIDRs/Portsviaadd/remove/set, andProto/ICMPEchovia the new flags. Nothing is file-only after this. The stale “declared in the config file”
strings were reworded to name
create-allow-ruleinstead of--from-file, which isalready refactor(network/firewall): remove --from-file and make the CLI the only host-firewall input #1010’s end state, so that work is not thrown away.
ending in
block node reconfigure) needs it. See Deferred at the end.Changes
internal/network/firewall/rule.goin_clusternow rejectsproto/icmp_echo; newincomplete()predicateinternal/network/firewall/table.goIncompleteAllowRules()for the apply-time warninginternal/network/firewall/manager.goCreateRule;UpdategainsProto/ICMPEchopointers; warning inapplyAndPersist; unknown-name errors name the new verbinternal/network/firewall/parse.gocmd/.../firewall/create_allow_rule.gocreate-allow-ruleverbcmd/.../firewall/firewall.gocmd/.../firewall/set.go--proto/--icmp-echocmd/.../firewall/create.go--from-filehelp no longer claims to be the only waydocs/quickstart.md,docs/dev/traffic-shaper.mdsetflags, and the list formReview guide
Key invariants to check:
CreateRulerejects reserved names before the exists check (manager.go). Reversingthese two is the bug called out above.
CreateRuleis built onwithLock, notmutate.mutatealways callsapplyAndPersist, so the already-exists no-op would restart the nft unit for nothing. A testasserts the apply count does not move.
Update.Proto/.ICMPEchoare pointers.""is a real proto setting andfalsea realecho setting, so only “flag was given” distinguishes them from “leave alone”. Existing callers
pass nil and are unaffected.
--proto/--icmp-echoare read only whenChanged(). LeavingProtoempty keeps it outof
show --output yaml, so theshow→create --from-fileround-trip stays byte-stable.mgmt/blocked/in_clusterrestrictions areunchanged or tightened.
resetFlagStatecovers the two new vars in the CLI tests, or state leaks between cases.Test plan
These packages import the Linux-only
internal/mountand do not build on macOS:Full
./internal/... ./cmd/... ./pkg/...suite passes;golangci-lintreports 0 issues.New tests:
TestTable_IncompleteAllowRulesAreLegalAndRenderNothing,TestRender_CLIDeclaredMatchesFileDeclared,TestManager_CreateRule,TestManager_SetProtoAndICMPEcho,TestManager_UnknownRuleNameStillFails,TestCreateAllowRuleCmd,TestCreateAllowRuleCmd_Rejections,TestCreateAllowRuleCmd_ForceRedeclares,TestSetCmd_ProtoAndICMPEcho,TestUnknownRuleNameNeverDeclares.Manual UAT
Run on the BN VM as root, from a console you did not open through a rule you are about to
change. Several cases render a default-drop input chain; keep a second non-SSH console open.
Replace
192.168.50.0/24with the subnet your host reaches the VM on, or you will lockyourself out.
UAT-1 — declare, populate, delete with no file (the headline AC)
Expect: sets
rudder_server/rudder_server6declared and empty, nosaddr @rudder_serveraccept anywhere, and a warning that the rule renders nothing yet.Expect, from the single
add:The range must survive as one element, and the echo accept must sit above the
limit rate over 10/second dropmeter (nft -a list chain inet weaver-host-firewall input_icmp_ipv4— compare handle numbers).
sudo solo-provisioner network firewall delete --name rudder_server # no new verb neededUAT-2 — populate in any order; nothing renders until complete
Both orders must work. Before this PR the first
addfailed outright.UAT-3 — a typo never declares (AC#3)
Each error should list the known rules and point at
create-allow-rule.UAT-4 — CLI-declared renders identically to file-declared (AC#4)
Expect no diff, and the one
adminrule split across families —@adminand@admin6.UAT-5 — create-if-missing and
--forceUAT-6 —
--proto/--icmp-echoonset, and reserved-block rejectionUAT-7 — no regressions in the pre-existing surface
Re-run UAT-2 (reserved blocks required) and UAT-6 (backward compatibility) from the #999
runbook unchanged.
create --from-filemust behave exactly as before, and every pre---nameinvocation must still work:
sudo solo-provisioner network firewall add --mgmt-cidr 10.1.0.0/16 sudo solo-provisioner network firewall add --in-cluster-port 9100 sudo solo-provisioner network firewall set --mgmt-cidrs 192.168.50.0/24 --in-cluster-ports 6443,4244UAT-8 — boot persistence
sudo reboot sudo nft list table inet weaver-host-firewall | grep adminDeferred
The final acceptance case — the full sequence ending in
sudo solo-provisioner block node reconfigure, confirming every rule survives — depends on #1003 / PR #1005, which is greenbut not yet merged. Standalone
network firewallverbs write no machine state, so todayreconfiguredeletes the firewall it finds. To be run after rebasing ontomainonce #1005lands.
Risks
Rule.Validaterelaxation is the only behaviour change outside the new verb. Itremoves two loud errors that also caught typo'd config files.
--from-filenow warns ratherthan errors on a half-written allow rule, and
remove-ing a rule's last CIDR no longer fails— the rule goes dormant instead. Both are fail-closed. The first disappears entirely under
refactor(network/firewall): remove --from-file and make the CLI the only host-firewall input #1010. Rollback is a one-hunk revert in
rule.go.in_clusternewly rejectsproto/icmp_echo. A hand-written config that set either onin_clusterwas previously accepted and ignored, and will now fail to load. That config wasalready not doing what it said.
Reverting the PR restores
--from-file-only declaration with no migration.🤖 Generated with Claude Code