From 534136339fe9a10279aab7a2c65e4ddc066557fa Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sun, 30 Aug 2026 17:59:52 +0000 Subject: [PATCH] Add --delete-network-full to fully tear down a network's live state --delete-network only ever removed the config entry, leaving the bridge/iptables/persistent-namespace state behind. Since ensure_network_provisioned() treats "bridge exists" as "already fully provisioned" and skips re-adding anything, a network recreated with the same name after a plain --delete-network silently never got a fresh MASQUERADE rule if the old one had been removed separately by hand -- the bridge itself was still there the whole time. teardown_network_state() (network_bridge.{h,cpp}) is the reverse of ensure_network_provisioned(): for extern, removes the MASQUERADE rule(s) then deletes the bridge; for intern, removes the whole persistent namespace in one step (destroys the bridge inside it too, no separate ip link del needed). Deliberately leaves the IPv4/IPv6 forwarding sysctls alone -- those are global host state shared across every extern network, not per-network. Each step is best-effort (teardown_step(), logging a warning not an error on failure) since a step "failing" because that piece was already gone by hand is the expected case this exists to handle, not a reason to abort -- delete_network_command() doesn't gate the config removal on any of this succeeding, unlike delete_volume_command()'s own -full variant. --delete-network-full wired into cli_args.{h,cpp} the same way --delete-volume-full is. Verified as root via the doas rule: an extern network's bridge and MASQUERADE rule were both confirmed gone after --delete-network-full, and recreating a network with the same name went through provision_bridge() fresh instead of short-circuiting on a stale bridge_exists() check -- fixing exactly the gap reported (a manually removed MASQUERADE rule never came back on delete+recreate). An intern network's persistent namespace was likewise confirmed fully removed and recreatable without conflict. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz --- CLAUDE.md | 47 ++++++++++++++++++++++++++++++++++++++---- README.md | 3 ++- src/cli_args.cpp | 19 +++++++++++++++-- src/cli_args.h | 3 ++- src/commands.cpp | 15 ++++++++++++-- src/network_bridge.cpp | 41 ++++++++++++++++++++++++++++++++++++ src/network_bridge.h | 19 +++++++++++++++++ 7 files changed, 137 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 008de2a..e55c62c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -182,10 +182,20 @@ Source layout (all under `src/`): it's already a pure deterministic function of the name — then the IPv6 subnet — or `"(no ipv6)"` — appended unaligned as the trailing column, nothing follows it). - `delete_network_command()` currently only removes the config entry, the - same as `delete_volume_command()`'s default (non-`-full`) behavior — it - does not tear down the network's live bridge/namespace/iptables state (no - `--delete-network-full` analog exists yet). + `delete_network_command()` takes a `delete_full` bool, same shape as + `delete_volume_command()`'s own: `--delete-network` (`false`) only removes + the config entry, leaving the network's live host-side state untouched; + `--delete-network-full` (`true`) additionally calls + `teardown_network_state()` (`network_bridge.h`, see below) first. Unlike + `delete_volume_command()`'s `-full` variant (which bails out *before* + touching the config if its single `remove_all()` call fails), + `delete_network_command()` doesn't gate the config removal on + `teardown_network_state()`'s success at all — that function is + deliberately best-effort/non-fatal per-step (see its own doc comment), so + a step "failing" because that piece was already gone by hand (exactly + `ensure_network_provisioned()`'s own existence-check caveat, above) is + expected, not a reason to leave a network the user explicitly asked to + delete sitting in the config. `write_config_command()` implements `-w/--write-config`: unlike `create_volume_command()`/`delete_volume_command()`'s use of `write_config_file()` (which only ever persists `AppConfig` fields that are @@ -662,6 +672,35 @@ Source layout (all under `src/`): all came up correctly; a real `intern` network's bridge came up inside its own dedicated namespace with neither forwarding nor a NAT rule, confirming the structural (not merely policy) isolation the design calls for. + `teardown_network_state()` is the reverse: `--delete-network-full` + (`commands.cpp`'s `delete_network_command()`) calls it to tear down + exactly what `ensure_network_provisioned()` stood up. For `extern`: + removes the `ip6tables` MASQUERADE rule (if `ipv6`) then the `iptables` + one, then deletes the bridge itself — via a new `.cpp`-local + `teardown_step()`, the same shape as `run_admin_command()` but logging a + *warning*, not an error, on failure, since a step failing because that + piece was already gone by hand is the expected, common case this exists + to handle (exactly `ensure_network_provisioned()`'s own existence-check + caveat above — this is precisely how a manually-removed MASQUERADE rule + can go from "won't come back on recreate" to "cleanly torn down and + recreated" once `--delete-network-full` exists at all). For `intern`: + removes the whole persistent namespace (`persistent_netns.h`) in one + step, which destroys everything inside it — the bridge included — with + no separate `ip link del` needed. Deliberately never touches the + IPv4/IPv6 forwarding sysctls `provision_bridge()` enables for `extern` — + those are global host state shared across every `extern` network, not + per-network, so turning them off here could break others still relying + on them. **Verified end-to-end on this dev machine (root, via the scoped + `doas` rule)**: an `extern` network's bridge and MASQUERADE rule were + both confirmed gone after `--delete-network-full` (`ip link show` + reporting "Device does not exist"), and recreating a network with the + *same name* afterward correctly went through `provision_bridge()` again + from scratch (confirmed via the debug log) instead of short-circuiting + on a stale `bridge_exists()` check — fixing exactly the gap a user + reported (a manually-removed MASQUERADE rule never came back on + `--delete-network` + recreate, since the old bridge was silently still + there); an `intern` network's persistent namespace was similarly + confirmed fully removed and recreatable without conflict. `probe_veth_support()` (added for the tap+relay fallback, see `docs/networking-design.md`'s addendum and `network_join.{h,cpp}` below): the real target device supports `tun`/`tap` but not `veth` diff --git a/README.md b/README.md index e2f7545..cda1b63 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,8 @@ slocker-lite -V|--version | `--delete-volume-full ` | Like `--delete-volume`, but also recursively deletes the volume's host directory. | | `-n, --network ` | Create/manage a persistent named network: requires exactly one of `--extern` (a real Linux bridge in the host's own namespace, with NAT/forwarding set up so containers on it reach the host's real network) or `--intern` (a bridge inside its own dedicated, routeless namespace, only reachable by other containers on the same network). `--subnet ` overrides the auto-allocated IPv4 range (`10.168.0.0/24`, incrementing per network); `--no-ipv6` disables (and `--subnet6 ` overrides) the auto-allocated IPv6 range, on by default. `--no-veth` forces the tap+relay join fallback even on a kernel that supports veth (useful for testing that path; it's otherwise chosen automatically whenever the running kernel lacks veth support). With `--run`, instead joins `` to the container as its own `eth` interface with an address from the network's subnet; repeatable, no membership limit. Root-only for now. **Known gap**: on a network joined via the tap+relay fallback, peer-to-peer connectivity works, but reaching the network's own gateway (and thus the outside, for `extern`) does not yet — root cause unconfirmed, see [`docs/networking-design.md`](docs/networking-design.md). | | `--list-networks` | List all named networks (see `-n/--network`) with their kind, IPv4 subnet, bridge name, and IPv6 subnet (or `(no ipv6)`). | -| `--delete-network ` | Remove a named network from the config. | +| `--delete-network ` | Remove a named network from the config. Its live bridge/namespace/iptables state is left untouched. | +| `--delete-network-full ` | Like `--delete-network`, but also tears down its live state (MASQUERADE rule(s), the bridge itself for `extern`, or the whole persistent namespace for `intern`), so a network with the same name can be created fresh afterward. | | `-p, --port-forward [:]:` | With `--run`, forward a TCP port from the host into the container. `` is optional, defaulting to the container's sole `--extern` network (an error if it joined more than one without specifying). Repeatable. Reachable via the host's real, externally-facing IP; `localhost`/loopback access has a known NAT-hairpinning limitation (see [`docs/networking-design.md`](docs/networking-design.md)). | | `--list-processes` | List running `--run` sessions found by their pid files under `$XDG_STATE_HOME/slocker-lite/run/`, with their pid, container name, and status (`running` or `exited`). | | `--clean-processes` | Remove stale pid files (see `--list-processes`), and any `-p`/`--port-forward` iptables rules, left behind by sessions that are no longer running (e.g. after a crash). | diff --git a/src/cli_args.cpp b/src/cli_args.cpp index 8683667..f9cbb13 100644 --- a/src/cli_args.cpp +++ b/src/cli_args.cpp @@ -63,9 +63,10 @@ constexpr int network_subnet6 = 273; constexpr int list_networks = 274; constexpr int delete_network = 275; constexpr int network_no_veth = 276; +constexpr int delete_network_full = 277; } // namespace options -constexpr std::array long_options = {{ +constexpr std::array long_options = {{ {"help", no_argument, nullptr, 'h'}, {"version", no_argument, nullptr, 'V'}, {"test", no_argument, nullptr, 't'}, @@ -101,6 +102,7 @@ constexpr std::array long_options = {{ {"no-veth", no_argument, nullptr, options::network_no_veth}, {"list-networks", no_argument, nullptr, options::list_networks}, {"delete-network", required_argument, nullptr, options::delete_network}, + {"delete-network-full", required_argument, nullptr, options::delete_network_full}, {"port-forward", required_argument, nullptr, 'p'}, {nullptr, 0, nullptr, 0}, }}; @@ -125,6 +127,7 @@ void print_usage(const char* prog) { " [--no-ipv6] [--subnet6 ] [--no-veth]\n" " {0} --list-networks\n" " {0} --delete-network \n" + " {0} --delete-network-full \n" " {0} --list-processes\n" " {0} --clean-processes\n" " {0} -w|--write-config\n" @@ -236,7 +239,15 @@ void print_usage(const char* prog) { " with their kind, IPv4 subnet, bridge name,\n" " and IPv6 subnet (or \"(no ipv6)\")\n" " --delete-network \n" - " remove a named network from the config\n" + " remove a named network from the config (its\n" + " live bridge/namespace/iptables state is left\n" + " untouched)\n" + " --delete-network-full \n" + " like --delete-network, but also tears down its\n" + " live state: MASQUERADE rule(s), the bridge\n" + " itself (extern), or the whole persistent\n" + " namespace (intern) -- so a network with the\n" + " same name can be created fresh afterward\n" " -p, --port-forward [:]:\n" " with --run, forward a TCP port from the host\n" " into the container. is optional --\n" @@ -325,6 +336,7 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { case options::clean_processes: case options::list_networks: case options::delete_network: + case options::delete_network_full: case options::kill: { Mode requested; switch (opt) { @@ -376,6 +388,9 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { case options::delete_network: requested = Mode::delete_network; break; + case options::delete_network_full: + requested = Mode::delete_network_full; + break; default: requested = Mode::clean_processes; break; diff --git a/src/cli_args.h b/src/cli_args.h index 5d6a9a6..a2788b7 100644 --- a/src/cli_args.h +++ b/src/cli_args.h @@ -46,7 +46,8 @@ enum class Mode { write_config, network, list_networks, - delete_network + delete_network, + delete_network_full }; // Everything parse_args() extracts from argv, ready to hand to diff --git a/src/commands.cpp b/src/commands.cpp index fba13b7..c16ccde 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -510,7 +510,8 @@ int list_networks_command(const AppConfig& config) { return 0; } -int delete_network_command(const std::string& name, const std::filesystem::path& config_path, AppConfig& config) { +int delete_network_command(const std::string& name, const std::filesystem::path& config_path, AppConfig& config, + bool delete_full) { auto it = std::find_if(config.networks.begin(), config.networks.end(), [&](const NetworkEntry& network) { return network.name == name; }); if (it == config.networks.end()) { @@ -518,6 +519,14 @@ int delete_network_command(const std::string& name, const std::filesystem::path& return 1; } + // Best-effort, not gating the config removal below on success -- see + // teardown_network_state()'s own doc comment (network_bridge.h) for why + // a step "failing" (e.g. a rule already removed by hand) is expected, + // not a reason to leave the config entry in place. + if (delete_full) { + teardown_network_state(*it); + } + config.networks.erase(it); if (!write_config_file(config_path, config)) { return 1; @@ -844,7 +853,9 @@ int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config case Mode::list_networks: return list_networks_command(config); case Mode::delete_network: - return delete_network_command(args.mode_arg, config_path, config); + return delete_network_command(args.mode_arg, config_path, config, false); + case Mode::delete_network_full: + return delete_network_command(args.mode_arg, config_path, config, true); case Mode::run: { // As root, containers-storage mount doesn't need to reexec into a private // user namespace to gain privilege, so the mount is already directly diff --git a/src/network_bridge.cpp b/src/network_bridge.cpp index 733fa50..e0d3bed 100644 --- a/src/network_bridge.cpp +++ b/src/network_bridge.cpp @@ -71,6 +71,19 @@ bool run_admin_command(const NetworkEntry& network, std::vector arg return true; } +// Same shape as run_admin_command() above, but a failure is logged as a +// warning, not an error, and never treated as fatal by its caller -- unlike +// provisioning (where a failure is a real problem), a teardown step failing +// because that piece was already removed by hand, or never existed, is the +// expected common case (see ensure_network_provisioned()'s own +// existence-check caveat, network_bridge.h), not something worth alarming +// about. +void teardown_step(const NetworkEntry& network, std::vector argv, std::string_view what) { + if (run_process(wrap_for_network(network, std::move(argv))).exit_code != 0) { + spdlog::warn("failed to {} for network '{}' (already gone, or never existed)", what, network.name); + } +} + bool bridge_exists(const NetworkEntry& network, const std::string& bridge) { return run_process(wrap_for_network(network, {"ip", "link", "show", bridge})).exit_code == 0; } @@ -203,3 +216,31 @@ bool ensure_network_provisioned(const NetworkEntry& network) { return provision_bridge(network, bridge); } + +void teardown_network_state(const NetworkEntry& network) { + std::string bridge = bridge_name(network.name); + + if (network.kind == NetworkKind::extern_) { + // Remove the MASQUERADE rule(s) before the bridge itself -- purely + // for tidiness, iptables doesn't require the referenced interface + // to still exist for the rule to be removable. + if (network.ipv6) { + teardown_step(network, + {"ip6tables", "-t", "nat", "-D", "POSTROUTING", "-s", network.subnet6, "!", "-o", bridge, + "-j", "MASQUERADE"}, + "remove IPv6 MASQUERADE rule"); + } + teardown_step(network, + {"iptables", "-t", "nat", "-D", "POSTROUTING", "-s", network.subnet, "!", "-o", bridge, "-j", + "MASQUERADE"}, + "remove IPv4 MASQUERADE rule"); + teardown_step(network, {"ip", "link", "del", bridge}, "delete bridge"); + } else { + // Removing the whole persistent namespace destroys everything + // inside it -- the bridge included -- in one step; no separate + // `ip link del` needed. + if (persistent_netns_exists(network.name) && !remove_persistent_netns(network.name)) { + spdlog::warn("failed to remove persistent namespace for network '{}'", network.name); + } + } +} diff --git a/src/network_bridge.h b/src/network_bridge.h index 277a209..7b2c6b9 100644 --- a/src/network_bridge.h +++ b/src/network_bridge.h @@ -87,3 +87,22 @@ bool should_use_veth(const NetworkEntry& network); // entry) -- there's no separate "reconcile" path; calling this again just // recreates whatever's missing. bool ensure_network_provisioned(const NetworkEntry& network); + +// Tears down `network`'s live host-side state -- the reverse of +// ensure_network_provisioned(). For `extern`: removes the MASQUERADE +// rule(s) (`ip6tables` first, if `ipv6`, then `iptables` -- purely for +// tidiness, since iptables doesn't require the interface a rule references +// to still exist), then deletes the bridge itself. For `intern`: removes +// the whole persistent namespace (persistent_netns.h) in one step, which +// destroys everything inside it -- the bridge included -- with no separate +// `ip link del` needed. Deliberately does **not** touch the IPv4/IPv6 +// forwarding sysctls `provision_bridge()` enables for `extern` -- those are +// global host state shared across every `extern` network, not per-network, +// so disabling them here could break others still relying on them. +// Best-effort, like every other host-global teardown in this project +// (`remove_session_cgroup()`, `remove_port_forward()`): logs a warning and +// keeps going past any individual step that fails, rather than stopping -- +// a step failing because that piece was already removed by hand (see +// ensure_network_provisioned()'s own existence-check caveat above) is the +// common case this exists to handle gracefully, not an error to abort on. +void teardown_network_state(const NetworkEntry& network);