From 93d2fa4d1c3ab6a5570b8a0bfaf5e354bdd99a98 Mon Sep 17 00:00:00 2001 From: alex-au Date: Fri, 14 Aug 2026 11:39:36 +1000 Subject: [PATCH 1/2] fix(blocknode): keep a hand-created host firewall across block node reconfigure A host firewall created with `network firewall create` was silently deleted by the next `block node reconfigure` -- table, both on-disk artifacts, and the management allowlist -- while the command reported success. `reconfigure` seeds its enable/disable choice from MachineState.Firewall, which only the block-node workflow ever wrote. The standalone verbs stopped at the manager, so a hand-created firewall left the record nil -- indistinguishable from "explicitly disabled" -- and the no-flag seed resolved to disabled, scheduling step_network_firewall_delete. Make the two paths agree: - `network firewall create` / `delete --all` record the enable decision into MachineState.Firewall. Best-effort: an nft ruleset already applied to the kernel must not be reported as a failure because a bookkeeping write missed. The delete side matters just as much as create -- a record still saying "enabled" would make the next reconfigure re-create a table just removed. - reconfigure's seed consults the live table via Manager.IsActive, which that method's own doc comment already claimed was wired up. A live table always seeds enabled, so removing an active host firewall now requires an explicit --firewall-enabled=false rather than being the default outcome of an unrelated reconfigure. - The firewall's own config file becomes a content precedence tier above machine state (flag > --config > live firewall > state > default), so a reconfigure's force re-render no longer reverts an urgent `add --name mgmt --cidr ...` back to the allowlist captured at install time. The projection leaves out what HostConfig cannot hold -- IPv6 members and inclusive port ranges -- with a warning rather than silently, or in the IPv6 case failing HostConfig.Validate and aborting the whole reconfigure. Closes #1003 Signed-off-by: alex-au --- cmd/cli/commands/block/node/reconfigure.go | 15 +- cmd/cli/commands/common/host_firewall.go | 181 ++++++++++++++++-- cmd/cli/commands/common/host_firewall_test.go | 130 +++++++++++++ cmd/cli/commands/network/firewall/create.go | 7 + cmd/cli/commands/network/firewall/delete.go | 6 + .../network/firewall/firewall_test.go | 5 + cmd/cli/commands/network/firewall/state.go | 69 +++++++ .../commands/network/firewall/state_test.go | 174 +++++++++++++++++ docs/dev/traffic-shaper.md | 8 + docs/quickstart.md | 16 +- internal/network/firewall/manager.go | 8 +- 11 files changed, 594 insertions(+), 25 deletions(-) create mode 100644 cmd/cli/commands/network/firewall/state.go create mode 100644 cmd/cli/commands/network/firewall/state_test.go diff --git a/cmd/cli/commands/block/node/reconfigure.go b/cmd/cli/commands/block/node/reconfigure.go index 826567ff..48d45ec0 100644 --- a/cmd/cli/commands/block/node/reconfigure.go +++ b/cmd/cli/commands/block/node/reconfigure.go @@ -45,12 +45,13 @@ var ( // Seed the enable/disable prompts from the block node's CURRENT state so a // no-flag / default-accept reconfigure keeps whatever is already deployed // and only an explicit toggle enables or tears a feature down. Both features - // now use the same source of truth — the persisted install/reconfigure - // decision (#947): traffic shaping from BlockNodeState.TrafficShapingDisabled, - // the host firewall from MachineState.Firewall. (The live inet-table probe is - // a reconciliation detail inside NetworkFirewallCreate, not a prompt seed.) - // On an unreadable state file, bias both to enabled so a default-accept never - // tears an established plane down. + // read the persisted install/reconfigure decision (#947): traffic shaping + // from BlockNodeState.TrafficShapingDisabled, the host firewall from + // MachineState.Firewall — except that the firewall's seed also consults the + // live inet table, because the standalone `network firewall` verbs can create + // one without the block node ever recording a decision (#1003). On an + // unreadable state file, bias both to enabled so a default-accept never tears + // an established plane down. stateDefaults, err := state.ReadPromptDefaultsFromDisk() if err != nil { logx.As().Debug().Err(err).Msg("could not read state file for reconfigure seeds; using conservative defaults") @@ -58,7 +59,7 @@ var ( firewallSeed := true currentTrafficShaping := true if err == nil { - firewallSeed = stateDefaults.Firewall != nil && !stateDefaults.Firewall.Disabled + firewallSeed = common.ResolveFirewallSeed(cmd.Context(), stateDefaults.Firewall) currentTrafficShaping = !stateDefaults.BlockNode.TrafficShapingDisabled } diff --git a/cmd/cli/commands/common/host_firewall.go b/cmd/cli/commands/common/host_firewall.go index 02f2b0ec..6ed5efe8 100644 --- a/cmd/cli/commands/common/host_firewall.go +++ b/cmd/cli/commands/common/host_firewall.go @@ -3,6 +3,7 @@ package common import ( + "context" "strconv" "strings" @@ -118,7 +119,8 @@ func hostFirewallFeature() gatedFeature { // host-service ports) and applies it to the global config so the // NetworkFirewallCreate step (wired into the block-node install/reconfigure/ // upgrade workflows) can render the inet weaver-host-firewall table. Precedence per value: -// CLI flag > interactive prompt > config file > built-in default. When the +// CLI flag > interactive prompt > config file > live host firewall > persisted +// state (MachineState.Firewall) > built-in default. When the // session is interactive, any value not supplied on the CLI is presented as a // pre-filled prompt the operator can confirm with Enter. An empty management // allowlist is allowed — the step then skips firewall creation rather than @@ -132,11 +134,10 @@ func hostFirewallFeature() gatedFeature { // seedEnabled is the default the enable/disable choice falls back to when neither // the flag nor an interactive prompt decides it. `install` passes false (opt-in — // a fresh install without the flag installs no firewall), while `reconfigure` -// passes the block node's persisted firewall decision (MachineState.Firewall), so -// a no-flag / default-accept reconfigure keeps the last-chosen state rather than -// silently tearing an established firewall down. It is intentionally NOT derived -// from cfg.Disabled: config.yaml's zero value cannot distinguish "enabled" from -// "never configured". +// passes ResolveFirewallSeed's answer, so a no-flag / default-accept reconfigure +// keeps the last-chosen state rather than silently tearing an established +// firewall down. It is intentionally NOT derived from cfg.Disabled: config.yaml's +// zero value cannot distinguish "enabled" from "never configured". // // It requires RegisterHostFirewallFlags to have been called on cmd. func ResolveHostFirewallConfig(cmd *cobra.Command, args []string, cv *prompt.ChosenValues, seedEnabled bool) error { @@ -169,10 +170,17 @@ func ResolveHostFirewallConfig(cmd *cobra.Command, args []string, cv *prompt.Cho // Fall back to the last-persisted firewall allowlist for any field the operator // did not supply via --config, so a reconfigure that re-enables the firewall // without re-passing --mgmt-cidrs restores the last-known-good allowlist instead - // of skipping with the SSH-lockout guard (issue #932). State sits below the - // config file and above the built-in default; a CLI flag, checked inside the - // effective* helpers below, still wins over all of them. On a fresh host with no - // persisted firewall this is a no-op. + // of skipping with the SSH-lockout guard (issue #932). A CLI flag, checked inside + // the effective* helpers below, still wins over both tiers. On a fresh host with + // neither a live firewall nor persisted state this is a no-op. + // + // The live firewall is consulted first because it is always at least as fresh as + // machine state: every path that writes machine state also re-renders the + // firewall, but the standalone `network firewall` verbs write only the firewall. + // Without this tier a reconfigure's force re-render would revert an urgent + // `network firewall add --name mgmt --cidr …` back to the allowlist recorded at + // install time (issue #1003). + cfg = mergeLiveHostFirewall(cmd.Context(), cfg) cfg = mergeHostFirewallFromState(cfg) // Seed each prompt target with the effective value: the CLI flag when the @@ -286,11 +294,158 @@ func joinInts(in []int) string { return strings.Join(parts, ",") } +// newHostFirewallManager is the seam over the production firewall manager so +// unit tests can substitute one wired to a fake nft runner and temp paths (the +// production manager probes the live kernel, which is Linux-only). +var newHostFirewallManager = func() *firewall.Manager { return firewall.NewManager() } + +// ResolveFirewallSeed answers "did this host want a host firewall?" for +// reconfigure's enable/disable seed — the value used when neither +// --firewall-enabled nor an interactive prompt decides it. +// +// persisted is the block node's recorded decision (MachineState.Firewall), nil +// when nothing was ever recorded. Nil is NOT "disabled": until issue #1003 the +// standalone `network firewall` verbs wrote no state at all, so a firewall +// created with `network firewall create` looked identical to a host that never +// had one — and a no-flag reconfigure resolved that to disabled and deleted it. +// +// So a live inet weaver-host-firewall table always seeds enabled, whatever state +// records. Removing an active host firewall is then only reachable through an +// explicit --firewall-enabled=false or an interactive decline, never as the +// default outcome of an unrelated reconfigure. The converse is left alone: when +// no table is live the recorded decision stands, so a reconfigure still +// re-asserts a firewall that state says should be there. +func ResolveFirewallSeed(ctx context.Context, persisted *models.HostConfig) bool { + recorded := persisted != nil && !persisted.Disabled + if recorded { + return true + } + + active, err := newHostFirewallManager().IsActive(ctx) + if err != nil { + // Not fatal: an unreadable probe just means we fall back to what state + // recorded, which is the pre-#1003 behaviour. + logx.As().Debug().Err(err).Msg("could not probe the live host firewall; seeding from persisted state only") + return recorded + } + if active { + logx.As().Info().Msg( + "a host firewall (inet weaver-host-firewall) is active but the block node has no record of enabling it; " + + "keeping it enabled — pass --firewall-enabled=false to remove it deliberately") + return true + } + return recorded +} + +// mergeLiveHostFirewall fills any host-firewall content field left empty by the +// config file with the corresponding value from the firewall's own persisted +// table (/etc/solo-provisioner/network-weaver-host-firewall.yaml). It is the +// "live firewall" tier of the flag > config > live > state > default precedence. +// Returns cfg unchanged when no firewall is configured on this host. +// +// Only the three reserved blocks map onto models.HostConfig; named allow rules +// have no field there and are instead carried across inside NetworkFirewallCreate, +// which re-reads them from the same table. +func mergeLiveHostFirewall(ctx context.Context, cfg models.HostConfig) models.HostConfig { + t, err := newHostFirewallManager().Table(ctx) + if err != nil { + logx.As().Debug().Err(err).Msg("no live host firewall to seed from; falling back to persisted state") + return cfg + } + live := hostConfigFromTable(t) + return applyPersistedFirewallContent(cfg, &live) +} + +// hostConfigFromTable projects a firewall Table's reserved blocks onto the +// flag-shaped models.HostConfig. +// +// The projection is narrower than the table in three places, and each warns +// rather than dropping silently: a Rule's CIDR list may mix address families +// while HostConfig is IPv4-only (HostConfig.Validate rejects an IPv6 CIDR +// outright, which would turn a reconfigure into a hard error); HostConfig.PodCIDR +// is a single string where InCluster.CIDRs is a list; and the port fields are +// []int where a Rule holds port specs, which may be inclusive ranges +// ("2379-2380"). A value that cannot be carried is left out, so the tier below +// (persisted state, then the built-in default) supplies that field. +func hostConfigFromTable(t *firewall.Table) models.HostConfig { + cfg := models.HostConfig{ + ManagementCIDRs: ipv4Only(t.Mgmt.CIDRs, ruleDescMgmt), + BlockedCIDRs: ipv4Only(t.Blocked.CIDRs, ruleDescBlocked), + InClusterPorts: plainPorts(t.InCluster.Ports, ruleDescInCluster), + } + if sshPorts := plainPorts(t.Mgmt.Ports, ruleDescMgmt); len(sshPorts) > 0 { + cfg.SSHPort = sshPorts[0] + } + if pod := ipv4Only(t.InCluster.CIDRs, ruleDescInCluster); len(pod) > 0 { + cfg.PodCIDR = pod[0] + if len(pod) > 1 { + logx.As().Warn().Strs("cidrs", pod).Msg( + "the live host firewall's in-cluster block holds several pod CIDRs but --pod-cidr carries only one; " + + "only the first is seeded — pass --pod-cidr, or re-apply the full set with " + + "`network firewall set --name in_cluster --cidrs`, if the rest still apply") + } + } + return cfg +} + +// Rule descriptions naming the offending block in the projection warnings. +const ( + ruleDescMgmt = "management" + ruleDescBlocked = "block-list" + ruleDescInCluster = "in-cluster" +) + +// ipv4Only keeps the CIDRs models.HostConfig can hold and warns about the rest. +// The flag-shaped config validates every CIDR as IPv4, so carrying an IPv6 +// member across would fail HostConfig.Validate and abort the whole reconfigure — +// the one outcome worse than not seeding the value at all. +func ipv4Only(cidrs []string, desc string) []string { + var out, skipped []string + for _, c := range cidrs { + if err := sanity.ValidateIPv4CIDR(strings.TrimSpace(c)); err != nil { + skipped = append(skipped, c) + continue + } + out = append(out, c) + } + if len(skipped) > 0 { + logx.As().Warn().Strs("cidrs", skipped).Msg( + "the live host firewall's " + desc + " block holds non-IPv4 addresses, which the flag-shaped config " + + "cannot express; they are not seeded and a re-render would drop them — re-apply them afterwards " + + "with `network firewall set --cidrs`") + } + return out +} + +// plainPorts keeps the port specs expressible as a single int and warns about +// any it had to leave behind, so an inclusive range authored through +// `network firewall set --ports` is never silently lost when the flag-shaped +// config is re-rendered. +func plainPorts(specs []string, desc string) []int { + var out []int + var skipped []string + for _, spec := range specs { + p, err := strconv.Atoi(strings.TrimSpace(spec)) + if err != nil { + skipped = append(skipped, spec) + continue + } + out = append(out, p) + } + if len(skipped) > 0 { + logx.As().Warn().Strs("ports", skipped).Msg( + "the live host firewall's " + desc + " block holds port ranges, which the flag-shaped config cannot " + + "express; they are not seeded and a re-render would drop them — re-apply them afterwards with " + + "`network firewall set --ports`") + } + return out +} + // mergeHostFirewallFromState fills any host-firewall content field left empty by // the config file with the last value persisted in state (machineState.firewall). -// It is the "state" tier of the flag > config > state > default precedence for the -// firewall allowlist: the returned config seeds the effective* helpers, where a -// CLI flag still takes priority. Only the allowlist content is merged — the +// It is the "state" tier of the flag > config > live firewall > state > default +// precedence for the firewall allowlist: the returned config seeds the effective* +// helpers, where a CLI flag still takes priority. Only the allowlist content is merged — the // enable/disable decision is resolved separately (flag / prompt / persisted // state), so the persisted Firewall.Disabled is intentionally ignored here. Returns cfg // unchanged when no firewall was ever persisted or the state read fails. diff --git a/cmd/cli/commands/common/host_firewall_test.go b/cmd/cli/commands/common/host_firewall_test.go index ccb10adb..c200b787 100644 --- a/cmd/cli/commands/common/host_firewall_test.go +++ b/cmd/cli/commands/common/host_firewall_test.go @@ -3,12 +3,54 @@ package common import ( + "context" + "os" + "path/filepath" "testing" + "github.com/hashgraph/solo-weaver/internal/network/firewall" "github.com/hashgraph/solo-weaver/pkg/models" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// probeRunner satisfies firewall.Runner without touching the kernel. Only +// Exists is exercised here — the seed is a pure presence probe, and the content +// tier reads the persisted config rather than the kernel. +type probeRunner struct{ exists bool } + +func (p *probeRunner) List(context.Context) (string, error) { return "", nil } +func (p *probeRunner) Check(context.Context, string) error { return nil } +func (p *probeRunner) Delete(context.Context) error { p.exists = false; return nil } +func (p *probeRunner) Exists(context.Context) (bool, error) { return p.exists, nil } + +// stubFirewallManager points the package's firewall seam at a manager backed by +// a fake nft runner and temp artifact paths. active drives the IsActive probe; +// table, when non-nil, is written to the config path so Manager.Table loads it. +func stubFirewallManager(t *testing.T, active bool, table *firewall.Table) { + t.Helper() + dir := t.TempDir() + configPath := filepath.Join(dir, "network-weaver-host-firewall.yaml") + + if table != nil { + data, err := firewall.FileConfigFromTable(table).Marshal() + require.NoError(t, err) + require.NoError(t, os.WriteFile(configPath, data, 0o600)) + } + + orig := newHostFirewallManager + newHostFirewallManager = func() *firewall.Manager { + return firewall.NewManagerWithConfig(firewall.Config{ + Runner: &probeRunner{exists: active}, + NftPath: filepath.Join(dir, "network-weaver-host-firewall.nft"), + ConfigPath: configPath, + LockPath: filepath.Join(dir, ".applying"), + ApplyViaService: func(context.Context) error { return nil }, + }) + } + t.Cleanup(func() { newHostFirewallManager = orig }) +} + // TestApplyPersistedFirewallContent_ConfigWins verifies the config > state // precedence (issue #932, AC4): fields the operator supplied via --config are // left untouched, and only the fields config left empty are filled from the @@ -49,3 +91,91 @@ func TestApplyPersistedFirewallContent_NilPersistedIsNoOp(t *testing.T) { got := applyPersistedFirewallContent(cfg, nil) assert.Equal(t, cfg, got) } + +// TestResolveFirewallSeed is the truth table behind issue #1003. The row that +// matters most is "nothing recorded + a live table": before the fix that read as +// "disabled" and a no-flag `block node reconfigure` deleted a firewall the +// operator had created with `network firewall create`. +func TestResolveFirewallSeed(t *testing.T) { + enabled := &models.HostConfig{Disabled: false} + disabled := &models.HostConfig{Disabled: true} + + tests := []struct { + name string + persisted *models.HostConfig + active bool + want bool + }{ + {"nothing recorded, table live", nil, true, true}, + {"nothing recorded, no table", nil, false, false}, + {"recorded disabled, table live", disabled, true, true}, + {"recorded disabled, no table", disabled, false, false}, + {"recorded enabled, no table", enabled, false, true}, + {"recorded enabled, table live", enabled, true, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stubFirewallManager(t, tt.active, nil) + assert.Equal(t, tt.want, ResolveFirewallSeed(context.Background(), tt.persisted)) + }) + } +} + +// TestMergeLiveHostFirewall_ConfigWinsStateLoses pins the new precedence tier: +// --config still beats the live firewall, but the live firewall beats whatever +// machine state recorded. Without that ordering a reconfigure's force re-render +// reverts an urgent `network firewall add --name mgmt --cidr …` back to the +// allowlist captured at install time. +func TestMergeLiveHostFirewall_ConfigWinsStateLoses(t *testing.T) { + live := firewall.NewTable() + live.Mgmt.CIDRs = []string{"10.9.0.0/16"} + live.Mgmt.Ports = []string{"2222"} + live.Blocked.CIDRs = []string{"198.51.100.0/24"} + live.InCluster.CIDRs = []string{"10.4.0.0/14"} + live.InCluster.Ports = []string{"4244", "6443"} + stubFirewallManager(t, true, live) + + // The operator pinned only the blocked list via --config. + cfg := models.HostConfig{BlockedCIDRs: []string{"203.0.113.0/24"}} + cfg = mergeLiveHostFirewall(context.Background(), cfg) + + assert.Equal(t, []string{"203.0.113.0/24"}, cfg.BlockedCIDRs, "config must win over the live firewall") + assert.Equal(t, []string{"10.9.0.0/16"}, cfg.ManagementCIDRs, "the live allowlist must fill an unset field") + assert.Equal(t, 2222, cfg.SSHPort) + assert.Equal(t, "10.4.0.0/14", cfg.PodCIDR) + assert.Equal(t, []int{4244, 6443}, cfg.InClusterPorts) + + // State is only consulted for what is still empty after the live tier. + cfg = applyPersistedFirewallContent(cfg, &models.HostConfig{ + ManagementCIDRs: []string{"192.168.50.0/24"}, + SSHPort: 22, + }) + assert.Equal(t, []string{"10.9.0.0/16"}, cfg.ManagementCIDRs, "the live firewall must win over persisted state") + assert.Equal(t, 2222, cfg.SSHPort) +} + +// TestMergeLiveHostFirewall_NoLiveFirewallIsNoOp verifies the common case on a +// host that has never had a firewall: nothing to load, config untouched. +func TestMergeLiveHostFirewall_NoLiveFirewallIsNoOp(t *testing.T) { + stubFirewallManager(t, false, nil) + + cfg := models.HostConfig{ManagementCIDRs: []string{"203.0.113.0/24"}} + assert.Equal(t, cfg, mergeLiveHostFirewall(context.Background(), cfg)) +} + +// TestHostConfigFromTable_SkipsWhatHostConfigCannotHold covers the lossy edges of +// the projection. HostConfig's port fields are []int, so an inclusive range +// authored through `network firewall set --ports` cannot be carried; and its +// CIDR fields are IPv4-only, so seeding an IPv6 member would fail +// HostConfig.Validate and abort the whole reconfigure. Both are left out (with a +// warning) rather than carried across and rejected downstream. +func TestHostConfigFromTable_SkipsWhatHostConfigCannotHold(t *testing.T) { + tbl := firewall.NewTable() + tbl.Mgmt.CIDRs = []string{"192.168.50.0/24", "2001:db8::/32"} + tbl.InCluster.Ports = []string{"4244", "2379-2380", "6443"} + + got := hostConfigFromTable(tbl) + assert.Equal(t, []string{"192.168.50.0/24"}, got.ManagementCIDRs, "only IPv4 members are carried") + assert.Equal(t, []int{4244, 6443}, got.InClusterPorts, "only plain-integer specs are carried") + assert.NoError(t, got.Validate(), "the projection must always be a valid HostConfig") +} diff --git a/cmd/cli/commands/network/firewall/create.go b/cmd/cli/commands/network/firewall/create.go index 1ff78486..f866647c 100644 --- a/cmd/cli/commands/network/firewall/create.go +++ b/cmd/cli/commands/network/firewall/create.go @@ -51,6 +51,13 @@ var createCmd = &cobra.Command{ if err != nil { return err } + + // Record the enable decision even when the create was a no-op (the table + // already existed and --force was not passed): "this host wants a host + // firewall" is true either way, and it is the decision — not the ruleset — + // that a later `block node reconfigure` seeds its enable/disable choice from. + recordHostFirewallDecision(false) + if changed { logx.As().Info().Msg("inet weaver-host-firewall firewall is in the desired state") } diff --git a/cmd/cli/commands/network/firewall/delete.go b/cmd/cli/commands/network/firewall/delete.go index 55d04e58..0eaa9f68 100644 --- a/cmd/cli/commands/network/firewall/delete.go +++ b/cmd/cli/commands/network/firewall/delete.go @@ -56,6 +56,12 @@ var deleteCmd = &cobra.Command{ if err := mgr.Delete(cmd.Context()); err != nil { return err } + + // Record the opt-out so the block-node workflow agrees the host no longer + // wants a firewall. A machine state still saying "enabled" would make the + // next `block node reconfigure` re-create the table just removed here. + recordHostFirewallDecision(true) + logx.As().Info().Msg("inet weaver-host-firewall firewall removed") return nil }, diff --git a/cmd/cli/commands/network/firewall/firewall_test.go b/cmd/cli/commands/network/firewall/firewall_test.go index 1ec3468c..65fe6a84 100644 --- a/cmd/cli/commands/network/firewall/firewall_test.go +++ b/cmd/cli/commands/network/firewall/firewall_test.go @@ -89,6 +89,11 @@ func stubManager(t *testing.T) (nftPath, configPath string) { nftPath = filepath.Join(dir, "network-weaver-host-firewall.nft") configPath = filepath.Join(dir, "network-weaver-host-firewall.yaml") + // The mutating verbs also record the enable decision into machine state + // (issue #1003), so every stubbed verb needs a state file under t.TempDir() + // too — otherwise a unit test would write to the host's real state. + stubStateManager(t) + origMgr, origDetect := newManager, detectPodCIDR newManager = func() *fw.Manager { return fw.NewManagerWithConfig(fw.Config{ diff --git a/cmd/cli/commands/network/firewall/state.go b/cmd/cli/commands/network/firewall/state.go new file mode 100644 index 00000000..20b28acf --- /dev/null +++ b/cmd/cli/commands/network/firewall/state.go @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "github.com/automa-saga/logx" + "github.com/hashgraph/solo-weaver/internal/state" + "github.com/joomcode/errorx" +) + +// stateManager is the seam over the production state manager so command tests +// can substitute one backed by a temp state file. +var stateManager = func() (state.Manager, error) { return state.NewStateManager() } + +// recordHostFirewallDecision records the operator's enable/disable decision into +// MachineState.Firewall so the standalone `network firewall` verbs and the +// block-node workflow share one source of truth. +// +// Without it the two paths disagree: `block node reconfigure` seeds its +// enable/disable choice from the persisted decision, which the standalone verbs +// never wrote, so a firewall created here read as "never configured" — and a +// later no-flag reconfigure resolved that to disabled and deleted it (issue +// #1003). The inverse matters just as much: after `delete --all`, a machine +// state still saying "enabled" would make the next reconfigure re-create the +// firewall the operator just removed. +// +// Only the decision is written. The CIDR/port content stays in the firewall's +// own config file, which ResolveHostFirewallConfig reads directly as a tier +// above machine state — mirroring it here would have to squeeze a Rule's port +// specs into HostConfig's []int and would silently drop any inclusive range. +// +// Best-effort by design: `network firewall create` is node-agnostic and may run +// on a host with no state file at all, and an nft ruleset that has already been +// applied to the kernel must not be reported as a failure because a bookkeeping +// write did not land. Failures are logged, never returned. +func recordHostFirewallDecision(disabled bool) { + if err := writeHostFirewallDecision(disabled); err != nil { + logx.As().Warn().Err(err).Bool("disabled", disabled).Msg( + "could not record the host-firewall decision in runtime state; a later `block node reconfigure` " + + "may not reflect it — re-run with --firewall-enabled to state the decision explicitly") + return + } + logx.As().Debug().Bool("disabled", disabled).Msg("Recorded host-firewall decision into runtime state") +} + +// writeHostFirewallDecision does the load-patch-flush, split out so the error +// path is testable without asserting on log output. +func writeHostFirewallDecision(disabled bool) error { + sm, err := stateManager() + if err != nil { + return errorx.IllegalState.Wrap(err, "failed to create state manager") + } + if err := sm.Refresh(); err != nil && !errorx.IsOfType(err, state.NotFoundError) { + return errorx.IllegalState.Wrap(err, "failed to refresh state") + } + + st := sm.State() + fw := st.MachineState.Firewall + if fw == nil { + fw = &state.HostFirewallState{} + } + fw.Disabled = disabled + st.MachineState.Firewall = fw + + if err := sm.Set(st).FlushState(); err != nil { + return errorx.IllegalState.Wrap(err, "failed to persist state") + } + return nil +} diff --git a/cmd/cli/commands/network/firewall/state_test.go b/cmd/cli/commands/network/firewall/state_test.go new file mode 100644 index 00000000..7e547e5b --- /dev/null +++ b/cmd/cli/commands/network/firewall/state_test.go @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/hashgraph/solo-weaver/internal/state" + "github.com/hashgraph/solo-weaver/pkg/fsx" + "github.com/stretchr/testify/require" +) + +// fakeStateManager is an in-memory state.Manager: Set/FlushState record the +// flushed state instead of writing a file, so a verb's decision write is +// observable without touching the host's real state (or needing the generated +// principal mocks that a real fsx.Manager would chown through). +type fakeStateManager struct { + current state.State + flushed *state.State + refresh error + flushErr error +} + +func (f *fakeStateManager) State() state.State { return f.current } +func (f *fakeStateManager) HasPersistedState() (os.FileInfo, bool, error) { + return nil, false, nil +} +func (f *fakeStateManager) Set(s state.State) state.Writer { f.current = s; return f } +func (f *fakeStateManager) AddActionHistory(state.ActionHistory) state.Writer { + return f +} +func (f *fakeStateManager) FlushState() error { + if f.flushErr != nil { + return f.flushErr + } + snapshot := f.current + f.flushed = &snapshot + return nil +} +func (f *fakeStateManager) FlushActionHistory() error { return nil } +func (f *fakeStateManager) FlushAll() error { return f.FlushState() } +func (f *fakeStateManager) Refresh() error { return f.refresh } +func (f *fakeStateManager) FileManager() fsx.Manager { return nil } + +// stubStateManager points the package's state seam at an in-memory manager and +// returns it, so a test can assert on what a verb flushed. +func stubStateManager(t *testing.T) *fakeStateManager { + t.Helper() + fake := &fakeStateManager{} + + orig := stateManager + stateManager = func() (state.Manager, error) { return fake, nil } + t.Cleanup(func() { stateManager = orig }) + return fake +} + +// flushedFirewall returns the host-firewall record the last flush persisted, +// failing the test when nothing was flushed at all. +func flushedFirewall(t *testing.T, fake *fakeStateManager) *state.HostFirewallState { + t.Helper() + require.NotNil(t, fake.flushed, "expected the verb to flush state") + return fake.flushed.MachineState.Firewall +} + +// TestCreateCmd_RecordsEnableDecision is the core of issue #1003: a firewall +// created through the standalone verb must leave the block node's persisted +// decision saying "enabled", or the next no-flag `block node reconfigure` reads +// the absent record as "disabled" and deletes the table. +func TestCreateCmd_RecordsEnableDecision(t *testing.T) { + stubManager(t) + fake := stubStateManager(t) + + require.NoError(t, run(t, "create", "--mgmt-cidrs", "192.168.50.0/24")) + + fw := flushedFirewall(t, fake) + require.NotNil(t, fw, "create must record a host-firewall decision") + require.False(t, fw.Disabled, "a created firewall must be recorded as enabled") +} + +// TestCreateCmd_RecordsDecisionOnNoOp covers the create-if-missing no-op: the +// supplied flags were not applied, but "this host wants a firewall" is still +// true, and that is the only thing the decision records. +func TestCreateCmd_RecordsDecisionOnNoOp(t *testing.T) { + stubManager(t) + fake := stubStateManager(t) + + require.NoError(t, run(t, "create", "--mgmt-cidrs", "192.168.50.0/24")) + // Second create without --force: the table exists, so nothing is re-rendered. + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + + fw := flushedFirewall(t, fake) + require.NotNil(t, fw) + require.False(t, fw.Disabled) +} + +// TestDeleteCmd_RecordsDisableDecision covers the inverse of #1003: after a +// standalone teardown, a machine state still saying "enabled" would make the +// next reconfigure re-create the table the operator just removed. +func TestDeleteCmd_RecordsDisableDecision(t *testing.T) { + stubManager(t) + fake := stubStateManager(t) + + require.NoError(t, run(t, "create", "--mgmt-cidrs", "192.168.50.0/24")) + require.NoError(t, run(t, "delete", "--all")) + + fw := flushedFirewall(t, fake) + require.NotNil(t, fw) + require.True(t, fw.Disabled, "a deleted firewall must be recorded as disabled") +} + +// TestDeleteCmd_PreservesRecordedContent verifies the decision write leaves the +// last-known-good allowlist in place, so a later bare re-enable still restores it +// (issue #932) instead of skipping with the SSH-lockout guard. +func TestDeleteCmd_PreservesRecordedContent(t *testing.T) { + stubManager(t) + fake := stubStateManager(t) + + // Seed the record the block-node workflow would have written. + fake.current.MachineState.Firewall = &state.HostFirewallState{ + ManagementCIDRs: []string{"192.168.50.0/24"}, + SSHPort: 2222, + } + + require.NoError(t, run(t, "delete", "--all")) + + fw := flushedFirewall(t, fake) + require.NotNil(t, fw) + require.True(t, fw.Disabled) + require.Equal(t, []string{"192.168.50.0/24"}, fw.ManagementCIDRs, "content must survive the decision write") + require.Equal(t, 2222, fw.SSHPort) +} + +// TestDeleteCmd_ByNameLeavesDecisionAlone verifies that removing one allow rule +// is not read as a decision to disable the firewall. +func TestDeleteCmd_ByNameLeavesDecisionAlone(t *testing.T) { + stubManager(t) + fake := stubStateManager(t) + + rules := filepath.Join(t.TempDir(), "rules.yaml") + require.NoError(t, os.WriteFile(rules, []byte(`version: 1 +mgmt: + cidrs: ["192.168.50.0/24"] +blocked: + cidrs: [] +in_cluster: + cidrs: [] +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443"] +`), 0o600)) + + require.NoError(t, run(t, "create", "--from-file", rules)) + fake.flushed = nil // only observe what the rule delete does + + require.NoError(t, run(t, "delete", "--name", "k8s-node")) + require.Nil(t, fake.flushed, "deleting one allow rule is not a decision to disable the firewall") +} + +// TestCreateCmd_SucceedsWhenStateWriteFails is the "bookkeeping must never fail +// an applied ruleset" guarantee: the nft table is already live by the time the +// decision is recorded, so a state-write failure is a warning, not an error. +func TestCreateCmd_SucceedsWhenStateWriteFails(t *testing.T) { + stubManager(t) + + orig := stateManager + stateManager = func() (state.Manager, error) { return nil, errors.New("no state manager") } + t.Cleanup(func() { stateManager = orig }) + + require.NoError(t, run(t, "create", "--mgmt-cidrs", "192.168.50.0/24")) +} diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index df352a4a..ce933207 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -473,6 +473,14 @@ provisioned node. blocks must be stated in it (as must `cidrs` inside `mgmt` and `blocked`) — otherwise a file that forgot `mgmt` would render an empty management allowlist under the default-drop policy. + `create` and `delete --all` also record the enable/disable decision into + `machineState.firewall.disabled`, the same field the block-node workflow + writes, so the standalone verbs and `block node reconfigure` share one source + of truth (issue #1003). Only the decision is mirrored: the ruleset itself stays + in `/etc/solo-provisioner/network-weaver-host-firewall.yaml`, which + `ResolveHostFirewallConfig` reads as a precedence tier *above* machine state — + otherwise a reconfigure's force re-render would revert an urgent + `add --name mgmt --cidr …` back to the allowlist captured at install time. - **`network policy`** (`create`/`add`/`remove`/`set`/`show`/`delete`) — the workload policy plane. `create` takes `--name` (the nft set name), `--stamp` (the HTB class to classify into, which also fixes direction) or `--deny`, plus diff --git a/docs/quickstart.md b/docs/quickstart.md index d8b57582..8914a308 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -449,7 +449,7 @@ sudo solo-provisioner block node reconfigure \ | `--no-restart` | Skip rollout-restart of the block node pod after reconfiguring | `false` | | `--with-reset` | Wipe block node data directories; PVs and PVCs are preserved | `false` | | `--purge-storage` | Delete PersistentVolumes and PersistentVolumeClaims in addition to wiping data (implies --with-reset) | `false` | -| `--firewall-enabled` | Enable or disable the node-level host firewall (`inet weaver-host-firewall` table) on an existing install. Seeded from the firewall's current on-host state, so a no-flag reconfigure keeps it as-is; pass `=false` to tear the table down, `=true` (with `--mgmt-cidrs`) to create it. Same sub-flags as `install` (`--mgmt-cidrs`, `--blocked-cidrs`, `--ssh-port`, `--pod-cidr`, `--in-cluster-ports`). | current state | +| `--firewall-enabled` | Enable or disable the node-level host firewall (`inet weaver-host-firewall` table) on an existing install. Seeded from the firewall's current on-host state — a live table always seeds enabled, however it was created — so a no-flag reconfigure keeps it as-is; pass `=false` to tear the table down, `=true` (with `--mgmt-cidrs`) to create it. Same sub-flags as `install` (`--mgmt-cidrs`, `--blocked-cidrs`, `--ssh-port`, `--pod-cidr`, `--in-cluster-ports`). | current state | | `--traffic-shaping-enabled` | Enable or disable the BN traffic-shaping bundle (network-policy plane + tc HTB shaping + daemon traffic-shaper monitor) on an existing install. Seeded from the persisted install decision, so a no-flag reconfigure keeps it; pass `=true` to create it (with `--egress-interface`/`--link-rate`/`--shape`/`--daemon-bin` as on `install`), `=false` to tear it down. | persisted state | | `--statusz-base-url` | Override the daemon's block-node statusz endpoint with an explicit `http(s)` base URL (e.g. `http://127.0.0.1:8080`) for a port-forward or directly-reachable BN. Merged per-field into `daemon.yaml` (`components.block_node.statusz.base_url`); omitting the flag preserves whatever is already on disk. Only when no `base_url` exists on disk does the daemon fall back to discovering the endpoint from the watched BN pod. | preserved on disk | | `--statusz-poll-interval` | Cadence at which the daemon's block-node traffic-shaper monitor polls statusz, as a positive Go duration (e.g. `5s`, `30s`). Merged per-field into `daemon.yaml` (`components.block_node.statusz.poll_interval`); omitting the flag preserves whatever is already on disk. Only when no `poll_interval` exists on disk does the daemon fall back to its `5s` default. | preserved on disk | @@ -464,7 +464,9 @@ sudo solo-provisioner block node reconfigure \ > `install`, so an operator can turn either feature on or off on an > already-deployed block node without a full `install --force` reinstall. Both > gates are seeded from the block node's **current** state — the host firewall from -> whether the `inet weaver-host-firewall` table exists, traffic shaping from the persisted install +> the last enable/disable decision *and* from whether the `inet weaver-host-firewall` table is +> live (a table that exists always seeds enabled, including one created by hand with +> `network firewall create`), traffic shaping from the persisted install > decision — so a routine reconfigure that doesn't pass the flag (or accepts the > interactive default) never changes enablement. Teardown only happens on an > explicit toggle: answering **No** (or passing `=false`) for a currently-enabled @@ -826,6 +828,16 @@ sudo solo-provisioner network firewall create --from-file rules.yaml --force # > > The reserved blocks cannot be deleted individually — clear their addresses instead (`network firewall set --name mgmt --cidrs ""`). +> **`create` and `delete --all` record the enable decision.** Both write it into the host's runtime +> state (`machineState.firewall.disabled`), so `block node reconfigure` agrees with what you did +> here: a firewall you created by hand survives a later reconfigure instead of being torn down, and +> one you deleted here is not re-created by it. A live table always wins over the recorded decision, +> so removing an active host firewall through `block node reconfigure` needs an explicit +> `--firewall-enabled=false`. The membership verbs (`add`, `remove`, `set`) change no decision — +> `reconfigure` reads their result straight out of +> `/etc/solo-provisioner/network-weaver-host-firewall.yaml`, so an urgent +> `add --name mgmt --cidr …` is not reverted by the next reconfigure. + #### Create a Traffic Policy The `policy` scope is a generic, category-agnostic primitive that manages the `inet weaver-workload-policy` workload traffic plane: named per-category rules that classify traffic into an HTB priority class, or quarantine a set of CIDRs. It is not tied to any specific node type — the CLI takes CIDRs and class names directly (statusz-agnostic); the examples below use the block-node categories because `block node install` is the only caller today. Each `create` renders the rule(s) into the `inet weaver-workload-policy` forward chain, ensures the policy's nft set `@` exists, writes a per-policy registry file under `/etc/solo-provisioner/policies/`, applies the full chain to the live kernel with `nft -f`, and atomically rewrites `/etc/solo-provisioner/network-weaver-workload-policy.nft`. diff --git a/internal/network/firewall/manager.go b/internal/network/firewall/manager.go index 81c4689d..1f2f3f89 100644 --- a/internal/network/firewall/manager.go +++ b/internal/network/firewall/manager.go @@ -193,9 +193,11 @@ func (m *Manager) Table(_ context.Context) (*Table, error) { // IsActive reports whether the inet weaver-host-firewall table is currently present in the // kernel. It is a read-only probe (no lock, no mutation) used by callers that -// need the firewall's current on-host state — e.g. seeding the reconfigure -// enable/disable prompt from ground truth rather than from config.yaml, whose -// zero value cannot distinguish "enabled" from "never configured". +// need the firewall's current on-host state, rather than a recorded decision +// whose absence cannot be distinguished from "disabled": +// common.ResolveFirewallSeed seeds the reconfigure enable/disable choice from it +// so an active firewall is never torn down by default, and NetworkFirewallCreate +// uses it to scope its rollback to a table that step actually introduced. func (m *Manager) IsActive(ctx context.Context) (bool, error) { return m.runner.Exists(ctx) } From ab6a5aff82898d1105a4f651f59b9186461a14f8 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Fri, 14 Aug 2026 21:15:42 +1000 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Wang --- cmd/cli/commands/common/host_firewall.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cli/commands/common/host_firewall.go b/cmd/cli/commands/common/host_firewall.go index 6ed5efe8..f625363b 100644 --- a/cmd/cli/commands/common/host_firewall.go +++ b/cmd/cli/commands/common/host_firewall.go @@ -412,7 +412,7 @@ func ipv4Only(cidrs []string, desc string) []string { logx.As().Warn().Strs("cidrs", skipped).Msg( "the live host firewall's " + desc + " block holds non-IPv4 addresses, which the flag-shaped config " + "cannot express; they are not seeded and a re-render would drop them — re-apply them afterwards " + - "with `network firewall set --cidrs`") + "with `network firewall set --name --cidrs ...`") } return out }