From 3cde6eb3dacfe2e49c3a588782d2eb188c514721 Mon Sep 17 00:00:00 2001 From: didericis Date: Sun, 12 Jul 2026 17:33:27 -0400 Subject: [PATCH] refactor(firecracker): single-source the network-pool defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pool params (size, IP base, iface prefix, nft table) were triplicated — hardcoded in netpool.py, scripts/firecracker-netpool.sh, and nix/firecracker-netpool.nix — plus the IP math (3x) and the nft ruleset (2x). Nothing enforced agreement; changing the base (ce3fad9, off CGNAT) forced a coordinated three-file edit, and a missed one would silently provision a range the launcher doesn't expect. Collapse to one source of truth: * netpool.defaults.env — a plain KEY=VALUE file (bash-sourceable, systemd EnvironmentFile-compatible, Python- and Nix-parseable) holding the four defaults. A BOT_BOTTLE_FC_* env var still overrides any key. * netpool.py reads it for the Python defaults (missing file = hard error, not confusing empty defaults). * the shell script falls back to it (no literal `:-8` / `10.243.0.0`), and its `up` is now non-destructive/idempotent (only creates a missing TAP), so re-running never cuts a live VM. * the Nix module readFile-parses it for its option defaults and delegates bring-up to the SAME shell script (dropping its duplicate IP math, nft ruleset, and TAP loop) — passing every value as Environment= so the store-detached script never needs the file. Net: defaults 3x -> 1x, nft ruleset 2x -> 1x, TAP loop 2x -> 1x. The one remaining IP-math dup (Python launch-addressing vs bash bring-up) is justified — different runtimes. Tests now guard the invariant (Python reads the shared file; the script/module hold no literals) instead of pinning duplicated strings. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01WBMWTEtQdJ4W5UrWuLHCck --- .../backend/firecracker/netpool.defaults.env | 17 ++ bot_bottle/backend/firecracker/netpool.py | 49 +++++- nix/firecracker-netpool.nix | 160 ++++++++---------- scripts/firecracker-netpool.sh | 59 ++++--- tests/unit/test_firecracker_backend.py | 69 +++++--- 5 files changed, 214 insertions(+), 140 deletions(-) create mode 100644 bot_bottle/backend/firecracker/netpool.defaults.env diff --git a/bot_bottle/backend/firecracker/netpool.defaults.env b/bot_bottle/backend/firecracker/netpool.defaults.env new file mode 100644 index 0000000..7532b3d --- /dev/null +++ b/bot_bottle/backend/firecracker/netpool.defaults.env @@ -0,0 +1,17 @@ +# Firecracker network-pool defaults — the SINGLE source of these values. +# +# Read by every consumer so they can't drift: +# * netpool.py — parses this for the Python defaults (below). +# * scripts/firecracker-netpool.sh — falls back to these when the +# matching BOT_BOTTLE_FC_* env var is unset. +# * nix/firecracker-netpool.nix — readFile-parses this for its option +# defaults, then passes the resolved values back as Environment=. +# +# Plain KEY=VALUE (no quoting, no inline comments, no spaces around `=`) +# so it is bash-sourceable, systemd EnvironmentFile-compatible, and +# trivially parseable from Python and Nix. A real BOT_BOTTLE_FC_* env +# var of the same name always overrides the value here. +BOT_BOTTLE_FC_POOL_SIZE=8 +BOT_BOTTLE_FC_IP_BASE=10.243.0.0 +BOT_BOTTLE_FC_IFACE_PREFIX=bbfc +BOT_BOTTLE_FC_NFT_TABLE=bot_bottle_fc diff --git a/bot_bottle/backend/firecracker/netpool.py b/bot_bottle/backend/firecracker/netpool.py index 5dc7c5b..09bb596 100644 --- a/bot_bottle/backend/firecracker/netpool.py +++ b/bot_bottle/backend/firecracker/netpool.py @@ -4,11 +4,13 @@ config renderers (shell command + NixOS module) shown to operators. The Firecracker backend needs a privileged one-time network setup: a pool of point-to-point TAP devices (owned by the invoking user, so `./cli.py start` never needs root) and a dedicated nftables table that -isolates every VM. This module is the single source of truth for the -pool parameters — the shell script (`scripts/firecracker-netpool.sh`), -the NixOS module (`nix/firecracker-netpool.nix`), and the backend's -fail-closed preflight all derive from these constants so they can't -drift. +isolates every VM. The pool parameters live in exactly one place — +`netpool.defaults.env`, a plain KEY=VALUE file next to this module — +and every consumer reads *that*: this module (below), the shell script +(`scripts/firecracker-netpool.sh`), and the NixOS module +(`nix/firecracker-netpool.nix`). A `BOT_BOTTLE_FC_*` env var of the +same name always overrides the file, and the backend's fail-closed +preflight derives from these accessors, so nothing can drift. Topology (per slot i): * TAP ``bbfc{i}`` — no shared bridge, so no docker0 / virbr0 / cni0 @@ -43,18 +45,47 @@ from typing import IO from ...log import die +# The pool defaults live in one shared file (see module docstring); the +# shell script and NixOS module read the same file, so the values can't +# drift. This is a packaged data file — a missing/broken install is a +# hard error, surfaced here rather than as confusing empty defaults. +DEFAULTS_FILE = Path(__file__).with_name("netpool.defaults.env") + + +def _load_defaults() -> dict[str, str]: + out: dict[str, str] = {} + for raw in DEFAULTS_FILE.read_text().splitlines(): + line = raw.strip() + if not line or line.startswith("#") or "=" not in line: + continue + key, _, value = line.partition("=") + out[key.strip()] = value.strip() + return out + + +_DEFAULTS = _load_defaults() + + +def _cfg(key: str) -> str: + """A `BOT_BOTTLE_FC_*` env var overrides the shared-file default.""" + try: + return os.environ.get(key) or _DEFAULTS[key] + except KeyError: + die(f"{key} is missing from {DEFAULTS_FILE.name} (broken install)") + + # Interface names are capped at 15 chars (IFNAMSIZ-1); "bbfc" + a small # index stays well under that and is distinctive enough to grep for. -IFACE_PREFIX = os.environ.get("BOT_BOTTLE_FC_IFACE_PREFIX", "bbfc") -NFT_TABLE = "bot_bottle_fc" +IFACE_PREFIX = _cfg("BOT_BOTTLE_FC_IFACE_PREFIX") +NFT_TABLE = _cfg("BOT_BOTTLE_FC_NFT_TABLE") def pool_size() -> int: - return int(os.environ.get("BOT_BOTTLE_FC_POOL_SIZE", "8")) + return int(_cfg("BOT_BOTTLE_FC_POOL_SIZE")) def ip_base() -> str: - return os.environ.get("BOT_BOTTLE_FC_IP_BASE", "10.243.0.0") + return _cfg("BOT_BOTTLE_FC_IP_BASE") # Sidecar ports the VM reaches at its host-side TAP IP. Kept in sync diff --git a/nix/firecracker-netpool.nix b/nix/firecracker-netpool.nix index 699e775..5f25f27 100644 --- a/nix/firecracker-netpool.nix +++ b/nix/firecracker-netpool.nix @@ -1,104 +1,70 @@ # bot-bottle Firecracker network pool — declarative NixOS module. # # The one-time privileged setup the Firecracker backend needs: a pool of -# user-owned point-to-point TAP devices plus a fail-closed nftables table -# that confines every microVM to its own sidecar. +# user- (or group-) owned point-to-point TAP devices plus a fail-closed +# nftables table that confines every microVM to its own sidecar. # # NON-INVASIVE BY DESIGN. It does NOT flip `networking.nftables.enable` # (which would switch your whole host firewall backend) or # `systemd.network.enable` (which would hand your interfaces to -# systemd-networkd). Instead a single systemd oneshot service brings the -# pool up on boot — the declarative equivalent of -# `sudo ./scripts/firecracker-netpool.sh up`. The `inet ` table -# is independent (its own hooks at priority -10), so it coexists with an -# iptables `networking.firewall`, Docker, ufw, firewalld, etc. +# systemd-networkd). Instead a single systemd oneshot brings the pool up +# on boot by running the SAME bring-up script as every other install +# path (`scripts/firecracker-netpool.sh`) — so the TAP/nft logic lives in +# exactly one place. The `inet ` table is independent (its own +# hooks at priority -10), so it coexists with an iptables +# `networking.firewall`, Docker, ufw, firewalld, etc. # -# The oneshot is only restarted when this config changes, so a -# `nixos-rebuild switch` doesn't tear down TAPs out from under running -# VMs unless you actually changed the pool. +# The oneshot is only restarted when this config changes, and the shared +# script is non-destructive (it never tears down an existing TAP), so a +# `nixos-rebuild switch` won't cut TAPs out from under running VMs. # -# The option defaults MUST match the backend constants in -# bot_bottle/backend/firecracker/netpool.py (pool size, IP base, iface -# prefix, table name). The backend reads the same values at launch from -# the BOT_BOTTLE_FC_* env vars; if you override an option here, override -# the matching env var for the CLI too (or set writeEnvFile = true below -# to have this module emit them). Keep the two sides in lockstep. +# Single source of the pool defaults: bot_bottle/backend/firecracker/ +# netpool.defaults.env. This module readFile-parses it for the option +# defaults below, then passes the resolved values back to the script as +# Environment=, so the host pool and the CLI launcher can't drift. { config, lib, pkgs, ... }: let cfg = config.services.bot-bottle-firecracker; - # --- IPv4 <-> int (full 32-bit carry math) ------------------------- - toOctets = s: map lib.toInt (lib.splitString "." s); - ipToInt = s: - let o = toOctets s; in - (lib.elemAt o 0) * 16777216 - + (lib.elemAt o 1) * 65536 - + (lib.elemAt o 2) * 256 - + (lib.elemAt o 3); - intToIp = n: + # --- shared single-source defaults --------------------------------- + # Parse the KEY=VALUE defaults file (the same one netpool.py and the + # shell script read). Pure eval — just readFile, no import-from- + # derivation — so it works for both flake and channel consumers. + readDefaults = file: let - b0 = n / 16777216; r0 = n - b0 * 16777216; - b1 = r0 / 65536; r1 = r0 - b1 * 65536; - b2 = r1 / 256; - b3 = r1 - b2 * 256; - in "${toString b0}.${toString b1}.${toString b2}.${toString b3}"; + lines = lib.splitString "\n" (builtins.readFile file); + keep = l: l != "" && !(lib.hasPrefix "#" l) && lib.hasInfix "=" l; + toPair = l: + let parts = lib.splitString "=" l; + in lib.nameValuePair (lib.head parts) + (lib.concatStringsSep "=" (lib.tail parts)); + in lib.listToAttrs (map toPair (lib.filter keep lines)); - baseInt = ipToInt cfg.ipBase; + defaults = readDefaults ../bot_bottle/backend/firecracker/netpool.defaults.env; - # Slot i: host = base + 2i (the VM's gateway), on its own /31. - slots = lib.genList (i: { - iface = "${cfg.ifacePrefix}${toString i}"; - hostIp = intToIp (baseInt + 2 * i); - }) cfg.poolSize; + # The one bring-up implementation, shared with the sudo/systemd paths. + netpoolScript = ../scripts/firecracker-netpool.sh; - ip = "${pkgs.iproute2}/bin/ip"; - nft = "${pkgs.nftables}/bin/nft"; + # /31 alignment == an even final octet (only bit 0 matters for base+2i). + lastOctet = lib.toInt (lib.last (lib.splitString "." cfg.ipBase)); - # Idempotent ruleset: (create-if-absent → delete → recreate) is atomic - # within one `nft -f`, so re-running `up` always lands a clean table. - nftFile = pkgs.writeText "bot-bottle-fc.nft" '' - table inet ${cfg.tableName} - delete table inet ${cfg.tableName} - table inet ${cfg.tableName} { - chain forward { - type filter hook forward priority -10; policy accept; - iifname != "${cfg.ifacePrefix}*" return - ct state established,related accept - ct status dnat accept - drop - } - chain input { - type filter hook input priority -10; policy accept; - iifname != "${cfg.ifacePrefix}*" return - ct state established,related accept - drop - } - } - ''; + # The script needs ip/nft/sysctl + the usual coreutils. It gets every + # pool value via the unit's Environment=, so it never reads the shared + # defaults file (which isn't beside it once copied to the store). + runtimePath = with pkgs; [ iproute2 nftables procps coreutils gnused ]; - # A non-owner user can't open a user-owned TAP, but any member of a - # TAP's owning GROUP can (the kernel checks owner-uid OR owning-group). - # So `group` lets an interactive user and a CI-runner user share one - # pool; `owner` is the single-user default. - tapOwn = if cfg.group != null then "group ${cfg.group}" else "user ${cfg.owner}"; + ownEnv = + if cfg.group != null + then { BOT_BOTTLE_FC_GROUP = cfg.group; } + else { BOT_BOTTLE_FC_OWNER = cfg.owner; }; - upScript = pkgs.writeShellScript "bot-bottle-fc-netpool-up" '' - set -eu - ${lib.concatMapStringsSep "\n" (s: '' - ${ip} link show ${s.iface} >/dev/null 2>&1 || ${ip} tuntap add dev ${s.iface} mode tap ${tapOwn} - ${ip} addr replace ${s.hostIp}/31 dev ${s.iface} - ${ip} link set ${s.iface} up - '') slots} - ${nft} -f ${nftFile} - ''; - - downScript = pkgs.writeShellScript "bot-bottle-fc-netpool-down" '' - ${nft} delete table inet ${cfg.tableName} 2>/dev/null || true - ${lib.concatMapStringsSep "\n" (s: '' - ${ip} link show ${s.iface} >/dev/null 2>&1 && ${ip} tuntap del dev ${s.iface} mode tap || true - '') slots} - ''; + unitEnv = { + BOT_BOTTLE_FC_POOL_SIZE = toString cfg.poolSize; + BOT_BOTTLE_FC_IP_BASE = cfg.ipBase; + BOT_BOTTLE_FC_IFACE_PREFIX = cfg.ifacePrefix; + BOT_BOTTLE_FC_NFT_TABLE = cfg.tableName; + } // ownEnv; in { options.services.bot-bottle-firecracker = { @@ -106,25 +72,28 @@ in poolSize = lib.mkOption { type = lib.types.ints.positive; - default = 8; + default = lib.toInt defaults.BOT_BOTTLE_FC_POOL_SIZE; + defaultText = lib.literalMD "the shared `netpool.defaults.env` value"; description = "Number of pool slots (concurrent bottles). Must match BOT_BOTTLE_FC_POOL_SIZE."; }; ipBase = lib.mkOption { type = lib.types.str; - default = "10.243.0.0"; + default = defaults.BOT_BOTTLE_FC_IP_BASE; + defaultText = lib.literalMD "the shared `netpool.defaults.env` value"; description = '' Base IPv4 of the /31 pool; slot i uses host = base + 2i. Must - be /31-aligned (even final address) and must match - BOT_BOTTLE_FC_IP_BASE. Default is an obscure RFC-1918 /16 that - dodges docker/libvirt/k8s/LAN and, deliberately, Tailscale's - 100.64.0.0/10 CGNAT range. + be /31-aligned (even final octet) and must match + BOT_BOTTLE_FC_IP_BASE. The shared default is an obscure RFC-1918 + /16 that dodges docker/libvirt/k8s/LAN and, deliberately, + Tailscale's 100.64.0.0/10 CGNAT range. ''; }; ifacePrefix = lib.mkOption { type = lib.types.str; - default = "bbfc"; + default = defaults.BOT_BOTTLE_FC_IFACE_PREFIX; + defaultText = lib.literalMD "the shared `netpool.defaults.env` value"; description = "TAP interface name prefix. Must match BOT_BOTTLE_FC_IFACE_PREFIX."; }; @@ -153,7 +122,8 @@ in tableName = lib.mkOption { type = lib.types.str; - default = "bot_bottle_fc"; + default = defaults.BOT_BOTTLE_FC_NFT_TABLE; + defaultText = lib.literalMD "the shared `netpool.defaults.env` value"; description = "nftables table name for the isolation boundary. Must match netpool.NFT_TABLE."; }; @@ -171,7 +141,7 @@ in config = lib.mkIf cfg.enable { assertions = [ { - assertion = lib.mod baseInt 2 == 0; + assertion = lib.mod lastOctet 2 == 0; message = "services.bot-bottle-firecracker.ipBase must be /31-aligned (even final octet); got ${cfg.ipBase}."; } { @@ -183,17 +153,20 @@ in # VM->sidecar traffic is DNAT'd and forwarded, so forwarding must be on. boot.kernel.sysctl."net.ipv4.ip_forward" = 1; - # One oneshot brings up the whole pool (TAPs + independent nft table). - # No networking.nftables.enable / systemd.network.enable — see header. + # One oneshot brings up the whole pool (TAPs + independent nft table) + # by running the shared bring-up script. No networking.nftables.enable + # / systemd.network.enable — see header. systemd.services."bot-bottle-firecracker-netpool" = { description = "bot-bottle Firecracker TAP pool + nft isolation table"; wantedBy = [ "multi-user.target" ]; after = [ "network-pre.target" ]; + path = runtimePath; + environment = unitEnv; serviceConfig = { Type = "oneshot"; RemainAfterExit = true; - ExecStart = upScript; - ExecStop = downScript; + ExecStart = "${pkgs.bash}/bin/bash ${netpoolScript} up"; + ExecStop = "${pkgs.bash}/bin/bash ${netpoolScript} down"; }; }; @@ -202,6 +175,7 @@ in BOT_BOTTLE_FC_POOL_SIZE=${toString cfg.poolSize} BOT_BOTTLE_FC_IP_BASE=${cfg.ipBase} BOT_BOTTLE_FC_IFACE_PREFIX=${cfg.ifacePrefix} + BOT_BOTTLE_FC_NFT_TABLE=${cfg.tableName} ''; }; }; diff --git a/scripts/firecracker-netpool.sh b/scripts/firecracker-netpool.sh index d08fcde..cf080c9 100755 --- a/scripts/firecracker-netpool.sh +++ b/scripts/firecracker-netpool.sh @@ -33,24 +33,44 @@ # sudo ./scripts/firecracker-netpool.sh down # ./scripts/firecracker-netpool.sh status # -# Env overrides (must match the backend's util.py constants): -# BOT_BOTTLE_FC_POOL_SIZE number of slots (default 8) -# BOT_BOTTLE_FC_IP_BASE base IPv4 of the /31 pool (default 10.243.0.0) -# BOT_BOTTLE_FC_IFACE_PREFIX TAP name prefix (default bbfc) -# BOT_BOTTLE_FC_OWNER owning user (default $SUDO_USER or $USER) -# BOT_BOTTLE_FC_GROUP owning group; if set, TAPs are group-owned -# instead of user-owned, so any group member -# (e.g. an interactive user + a CI runner -# user) can open the pool. Overrides OWNER. +# Pool params default to the shared single-source file +# (bot_bottle/backend/firecracker/netpool.defaults.env); a matching +# env var overrides its key: +# BOT_BOTTLE_FC_POOL_SIZE number of slots +# BOT_BOTTLE_FC_IP_BASE base IPv4 of the /31 pool +# BOT_BOTTLE_FC_IFACE_PREFIX TAP name prefix +# BOT_BOTTLE_FC_NFT_TABLE isolation table name +# BOT_BOTTLE_FC_OWNER owning user (default $SUDO_USER or $USER) +# BOT_BOTTLE_FC_GROUP owning group; if set, TAPs are group-owned +# instead of user-owned, so any group member +# (e.g. an interactive user + a CI runner +# user) can open the pool. Overrides OWNER. set -euo pipefail -POOL_SIZE="${BOT_BOTTLE_FC_POOL_SIZE:-8}" -IP_BASE="${BOT_BOTTLE_FC_IP_BASE:-10.243.0.0}" -PREFIX="${BOT_BOTTLE_FC_IFACE_PREFIX:-bbfc}" +# The single source of the pool defaults, shared with netpool.py and the +# Nix module. The Nix path passes every value as env (the script runs +# from the store, detached from this file), so this lookup only matters +# on the direct/sudo path where the script sits in the repo tree. +_SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +_DEFAULTS="$_SCRIPT_DIR/../bot_bottle/backend/firecracker/netpool.defaults.env" +_default() { # value of KEY=... from the shared file; empty if unavailable + [ -f "$_DEFAULTS" ] || return 0 + sed -n "s/^$1=//p" "$_DEFAULTS" | tail -1 +} + +POOL_SIZE="${BOT_BOTTLE_FC_POOL_SIZE:-$(_default BOT_BOTTLE_FC_POOL_SIZE)}" +IP_BASE="${BOT_BOTTLE_FC_IP_BASE:-$(_default BOT_BOTTLE_FC_IP_BASE)}" +PREFIX="${BOT_BOTTLE_FC_IFACE_PREFIX:-$(_default BOT_BOTTLE_FC_IFACE_PREFIX)}" +TABLE="${BOT_BOTTLE_FC_NFT_TABLE:-$(_default BOT_BOTTLE_FC_NFT_TABLE)}" OWNER="${BOT_BOTTLE_FC_OWNER:-${SUDO_USER:-$USER}}" GROUP="${BOT_BOTTLE_FC_GROUP:-}" -TABLE="bot_bottle_fc" + +# Fail loudly rather than provisioning a half/empty range if a value +# resolved to nothing (env unset AND the shared file unreadable). +for _v in POOL_SIZE IP_BASE PREFIX TABLE; do + [ -n "${!_v}" ] || { echo "error: $_v unresolved (set BOT_BOTTLE_FC_* or fix $_DEFAULTS)" >&2; exit 1; } +done # Sidecar ports (must match the backend). egress=9099, supervise=9100, # git-http=9420. Reached by the VM at its host-side TAP IP. @@ -94,12 +114,13 @@ cmd_up() { for i in $(seq 0 $((POOL_SIZE-1))); do local dev host dev="$(iface "$i")" ; host="$(host_ip "$i")" - if ip link show "$dev" >/dev/null 2>&1; then - ip link set "$dev" down 2>/dev/null || true - ip tuntap del dev "$dev" mode tap 2>/dev/null || true - fi - ip tuntap add dev "$dev" mode tap "${own_args[@]}" - ip addr add "$host/31" dev "$dev" + # Non-destructive + idempotent: only create a missing TAP (tearing + # an existing one down would cut a running VM), and `addr replace` + # is safe to re-run. Ownership is fixed at creation, so to change + # owner/group run `down` then `up`. + ip link show "$dev" >/dev/null 2>&1 \ + || ip tuntap add dev "$dev" mode tap "${own_args[@]}" + ip addr replace "$host/31" dev "$dev" ip link set "$dev" up echo " $dev host=$host guest=$(guest_ip "$i") $own_desc" done diff --git a/tests/unit/test_firecracker_backend.py b/tests/unit/test_firecracker_backend.py index 737835d..55b1937 100644 --- a/tests/unit/test_firecracker_backend.py +++ b/tests/unit/test_firecracker_backend.py @@ -67,10 +67,24 @@ class TestNetpoolRenderers(unittest.TestCase): self.assertNotIn("networking.nftables.enable = true", mod) self.assertNotIn("systemd.network.enable = true", mod) self.assertIn("bot-bottle-firecracker-netpool", mod) # the oneshot - self.assertIn(netpool.NFT_TABLE, mod) - self.assertIn('iifname != "${cfg.ifacePrefix}*" return', mod) # Supports group-owned TAPs (shared pool for a multi-user host). - self.assertIn("group ${cfg.group}", mod) + self.assertIn("BOT_BOTTLE_FC_GROUP", mod) + + def test_nixos_module_delegates_and_holds_no_literals(self): + # Single-source discipline: the module must NOT re-implement the + # bring-up (it delegates to the shared script) and must NOT hard- + # code the pool defaults (it readFile-parses the shared .env), so + # nothing can drift from netpool.py / the shell script. + root = Path(__file__).resolve().parents[2] + mod = (root / "nix" / "firecracker-netpool.nix").read_text() + # Delegation to the one bring-up implementation + shared defaults. + self.assertIn("scripts/firecracker-netpool.sh", mod) + self.assertIn("netpool.defaults.env", mod) + self.assertIn("BOT_BOTTLE_FC_NFT_TABLE", mod) + # No duplicated literals or nft ruleset. + self.assertNotIn(netpool.ip_base(), mod) # 10.243.0.0 + self.assertNotIn(netpool.NFT_TABLE, mod) # bot_bottle_fc + self.assertNotIn("ct status dnat", mod) # the nft ruleset def test_shell_setup_reflects_overrides(self): with patch.dict(os.environ, {"BOT_BOTTLE_FC_POOL_SIZE": "3"}): @@ -262,26 +276,43 @@ class TestBottleAgentArgv(unittest.TestCase): self.assertIn("USER=node", argv) -class TestSetupScriptConsistency(unittest.TestCase): - """The shell setup script duplicates the pool defaults; keep them in - lockstep with the Python constants so the two setup paths agree.""" +class TestNetpoolDefaultsSingleSource(unittest.TestCase): + """The pool defaults live in one shared file (netpool.defaults.env); + Python parses it and the shell script + Nix module read the same file, + so the three setup paths can't drift.""" - def _script(self) -> str: - root = Path(__file__).resolve().parent.parent.parent - return (root / "scripts" / "firecracker-netpool.sh").read_text() + def _root(self) -> Path: + return Path(__file__).resolve().parents[2] - def test_defaults_match_python(self): - script = self._script() + def _shared_defaults(self) -> dict[str, str]: + text = netpool.DEFAULTS_FILE.read_text() + out: dict[str, str] = {} + for raw in text.splitlines(): + line = raw.strip() + if line and not line.startswith("#") and "=" in line: + k, _, v = line.partition("=") + out[k.strip()] = v.strip() + return out + + def test_python_reads_the_shared_file(self): + d = self._shared_defaults() with patch.dict(os.environ, {}, clear=True): - self.assertIn(f'POOL_SIZE:-{netpool.pool_size()}', script) - self.assertIn(f'BOT_BOTTLE_FC_IP_BASE:-{netpool.ip_base()}', script) - self.assertIn(f'IFACE_PREFIX:-{netpool.IFACE_PREFIX}', script) + self.assertEqual(int(d["BOT_BOTTLE_FC_POOL_SIZE"]), netpool.pool_size()) + self.assertEqual(d["BOT_BOTTLE_FC_IP_BASE"], netpool.ip_base()) + # Module constants resolve through the same shared file. + self.assertEqual(d["BOT_BOTTLE_FC_IFACE_PREFIX"], netpool.IFACE_PREFIX) + self.assertEqual(d["BOT_BOTTLE_FC_NFT_TABLE"], netpool.NFT_TABLE) - def test_table_name_and_ports_match(self): - script = self._script() - self.assertIn(f'TABLE="{netpool.NFT_TABLE}"', script) - for port in netpool.SIDECAR_PORTS: - self.assertIn(str(port), script) + def test_env_var_overrides_the_shared_default(self): + with patch.dict(os.environ, {"BOT_BOTTLE_FC_IP_BASE": "10.99.0.0"}): + self.assertEqual("10.99.0.0", netpool.ip_base()) + + def test_script_defers_to_shared_file_without_literals(self): + script = (self._root() / "scripts" / "firecracker-netpool.sh").read_text() + # Delegates its defaults to the shared file, not hardcoded values. + self.assertIn("netpool.defaults.env", script) + self.assertIn("BOT_BOTTLE_FC_POOL_SIZE:-$(_default BOT_BOTTLE_FC_POOL_SIZE)", script) + self.assertIn('TABLE="${BOT_BOTTLE_FC_NFT_TABLE:-$(_default BOT_BOTTLE_FC_NFT_TABLE)}"', script) class TestBootArgs(unittest.TestCase):