Skip to content

refactor(blocknode): unify the firewall and traffic-shaping gated network-feature wiring #947

Description

@alex-au

Problem

The host firewall and traffic-shaping features are siblings — two operator-gated "network plane" features toggled on block node install / reconfigure / upgrade and persisted in state (MachineState.Firewall, BlockNodeState.Shaping). They landed incrementally and, as a result, do the same things in materially different ways. This makes the wiring hard to reason about and every future change (e.g. #945 IPv6, #942 FQDN) has to be applied twice, differently.

Concrete divergences (file:line on main):

  1. "Is it currently on?" uses two different sources of truth. reconfigure seeds the traffic-shaping gate from persisted state (cmd/cli/commands/block/node/reconfigure.go:56, TrafficShapingDisabled) but seeds the firewall gate from a live host probe (reconfigure.go:57firewall.NewManager().IsActive).

  2. Resolved config is threaded two different ways. Firewall pushes its resolved config into the global config singleton (config.OverrideHostConfig, read back via config.Get().Host); shaping threads it through inputs.Custom fields. Same concept, two transport mechanisms.

  3. Persisted-state fallback lives in two different layers. Firewall merges persisted state in the CLI/common layer (cmd/cli/commands/common/host_firewall.gomergeHostFirewallFromState / applyPersistedFirewallContent / SeedHostFirewallFromState); shaping does it in the BLL layer (internal/bll/blocknode/helpers.go:213-238, inside resolveBlocknodeEffectiveInputs).

  4. Duplicated resolver boilerplate. ResolveTrafficShapingConfig (common/traffic_shaping.go:47-96) and ResolveHostFirewallConfig (common/host_firewall.go:127-240) share a near-identical skeleton: force-fetch → effectiveBool(seed) → confirm-prompt guarded by ShouldPrompt && !Changed → a "mismatch guard" loop emitting the same ErrPropertyResolution message shape for feature-only flags. The mismatch-guard blocks are essentially copy-paste differing only in the flag-name slice and the noun.

  5. The enable-step sequence is assembled twice. networkPlaneSteps (internal/bll/blocknode/network_plane.go:36) and NetworkSetupWorkflow (internal/workflows/network_setup.go:33) both emit the same 4-step enable bundle (NetworkPolicyCreate → NftWeaverPersist → TcEgressPersist → TcIngressRecord); install uses one, reconfigure/upgrade the other.

  6. Neither feature flows through the documented RSL EffectiveValue[T] resolution pattern (docs/dev/effective-value-resolution.md). Both use ad-hoc effectiveBool/inline fallbacks, so the "flag > input > persisted-state > default" precedence is spelled out by hand in three different idioms.

  7. Minor: effectiveBool is defined in host_firewall.go but consumed cross-file by traffic_shaping.go (reads as firewall-owned but shaping depends on it); ValidateHostFirewallFlags/ValidateEgressFlags exist but there is no ValidateTrafficShapingFlags (the gate's validation is folded into the mismatch guard); shaping flags are registered through three mechanisms (RegisterTrafficShapingFlags gate, egress.go NIC/rate, inline --shape) vs firewall's single RegisterHostFirewallFlags.

None of this is a functional bug today; it is structural debt that raises the cost and risk of every future network-plane change.

Proposed fix

Unify the two features behind one "gated network feature" abstraction. Sketch (to be refined at triage — do not treat this as a spec):

  • One resolver for the confirm-prompt + mismatch-guard + effective-bool skeleton, parameterized by feature (flag-name set, noun, seed source). Firewall and shaping each supply only their specifics.
  • One source of truth for "currently enabled" — pick persisted-state or live-probe and use it for both (persisted-state is the more consistent choice given both are already persisted; live-probe can be a reconciliation detail).
  • One layer + one mechanism for persisted-state fallback — both in the BLL RSL path, or both in CLI/common; not one each.
  • One desired-state step assembler parameterized by allowTeardown + daemon-restart handling, replacing the networkPlaneSteps / NetworkSetupWorkflow duplication so all three commands share it.
  • Optionally, move both features onto the RSL EffectiveValue[T] pattern per docs/dev/effective-value-resolution.md so precedence is declared once.

Risks

  • Higher blast radius than a pure shaping refactor: this touches the firewall feature and its tests. Sequence it behind the shape-engine cleanup (branch refactor-traffic-shaper-engine) and the render-pipeline refactor.
  • Threading changes (config singleton vs inputs.Custom) interact with resolveBlocknodeEffectiveInputs; see the [[blocknode-effective-inputs-passthrough]] invariant — new fields must be added there or they are silently dropped.
  • Needs a design decision on the persisted-state-vs-live-probe question (item 2/3) before implementation.

Acceptance

  • Firewall and traffic-shaping share a single gated-feature resolver (confirm-prompt + mismatch-guard + effective-bool), with per-feature config supplied as data.
  • "Currently enabled?" is derived from one source of truth for both features across install/reconfigure/upgrade.
  • Persisted-state fallback for both features lives in one layer via one mechanism.
  • The enable/disable step sequence is assembled by one shared function used by install, reconfigure, and upgrade.
  • No behavior change to the install/reconfigure/upgrade contracts described in docs/dev/traffic-shaper-test-plan.md (§2.3.6 enable/disable/upgrade semantics); existing BLL/handler tests pass.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions