From cdf9dcd210a001a700c9bc2a93a70ccccc1ff163 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sat, 5 Sep 2026 08:36:12 +0000 Subject: [PATCH] Add global.with-veth/with-ipv6 config defaults for -n/--network creation Replaces --no-ipv6/--no-veth (plain flags) with --with-ipv6/--with-veth, each taking an explicit true/false value (e.g. --with-veth=false), parsed via the same parse_bool_flag() the config file itself already uses (now exported from config_file.h so cli_args.cpp can reuse it). create_network_command() now resolves ipv6/veth as CLI flag -> config's own global.with-ipv6/global.with-veth -> true, so a host that always wants the tap+relay fallback (or no IPv6) can set it once in the config instead of passing the flag on every network creation. -w/--write-config fills in both new keys like the existing six unshare-* bools. --- CLAUDE.md | 92 +++++++++----- README.md | 12 +- src/cli_args.cpp | 67 ++++++---- src/cli_args.h | 15 ++- src/commands.cpp | 16 ++- src/config_file.cpp | 120 +++++++++++------- src/config_file.h | 17 +++ tests/integration/test_config_bwrap_chain.cpp | 23 ++++ tests/unit/test_cli_args.cpp | 35 +++++ tests/unit/test_network_subnet.cpp | 2 +- 10 files changed, 287 insertions(+), 112 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3b125bf..21b7070 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,20 +75,34 @@ Source layout (all under `src/`): consumption needed, `'n'`'s own `case` just does `network_specs.push_back(optarg)`. The same post-loop split as `-v` decides `Mode::network` (standalone, exactly one occurrence) vs. join-with-`-r` - (repeatable, no limit). `--extern`/`--intern`/`--subnet `/`--no-ipv6`/ - `--subnet6 `/`--no-veth` (`ParsedArgs::network_extern_flag`/`network_intern_flag`/ - `network_subnet_flag`/`network_no_ipv6_flag`/`network_subnet6_flag`/ - `network_no_veth_flag`) only + (repeatable, no limit). `--extern`/`--intern`/`--subnet `/`--with-ipv6`/ + `--subnet6 `/`--with-veth` (`ParsedArgs::network_extern_flag`/`network_intern_flag`/ + `network_subnet_flag`/`network_with_ipv6_flag`/`network_subnet6_flag`/ + `network_with_veth_flag`) only apply to the standalone (create) case and are rejected with a clear error if given any other way (e.g. alongside `-r`) — `Mode::network` additionally - requires exactly one of `--extern`/`--intern`. `--no-veth` forces - `NetworkEntry::veth` (`config_file.h`) to `false` at creation time, - overriding the default `true` — see `network_bridge.h`'s - `probe_veth_support()`/`should_use_veth()` for what this controls: lets - the tap+relay fallback (the real target device's kernel lacks `CONFIG_VETH` - — see `docs/networking-design.md`'s addendum) be exercised on a - veth-capable machine like this dev box, without needing the actual - veth-less hardware. **`-n` used to belong to + requires exactly one of `--extern`/`--intern`. Unlike the plain boolean + flags elsewhere in this file, `--with-ipv6`/`--with-veth` are + `required_argument` (e.g. `--with-veth=false`), parsed via + `parse_bool_flag()` (`config_file.h` — exported specifically so this file + can reuse the exact same accepted forms, `"1"/"on"/"yes"/"true"` and + `"0"/"off"/"no"/"false"`, as the config file itself, rather than a second, + drifting copy) into `ParsedArgs::network_with_ipv6_flag`/`network_with_veth_flag` + (`std::optional` — `nullopt` means "not given on the CLI, use the + config file's own default", not "false"). `--with-veth=false` forces + `NetworkEntry::veth` (`config_file.h`) to `false` at creation time; when + neither is given, `create_network_command()` (`commands.cpp`) falls back to + `AppConfig::with_veth`/`with_ipv6` (`config_file.h`'s own two new + `global.with-veth`/`global.with-ipv6` keys, same "unset means enabled" + convention as the six `unshare-*` keys), only defaulting to `true` if that, + too, is unset — see `network_bridge.h`'s + `probe_veth_support()`/`should_use_veth()` for what a resolved `false` + controls: lets the tap+relay fallback (the real target device's kernel + lacks `CONFIG_VETH` — see `docs/networking-design.md`'s addendum) be + exercised on a veth-capable machine like this dev box, without needing the + actual veth-less hardware, or be made this dev box's own default via the + config file instead of passing `--with-veth=false` on every `-n --extern`/ + `--intern` invocation. **`-n` used to belong to `--no-nsenter`**: reassigned here since `--network` will be far more heavily used; `--no-nsenter` moved to long-option-only (`options::no_nsenter`) rather than hunting for a new letter, matching `--kill`'s own "rare/niche @@ -200,7 +214,8 @@ Source layout (all under `src/`): `create_volume_command()`/`delete_volume_command()`'s use of `write_config_file()` (which only ever persists `AppConfig` fields that are already set), this fills in *every* field before writing — the six - `unshare-*` bools via `.value_or(true)`, and `log_level` from the actually + `unshare-*` bools plus `with-veth`/`with-ipv6`, all via `.value_or(true)`, + and `log_level` from the actually active `spdlog::get_level()` (not merely a default for when unset — this also captures an explicit `--log-level` passed alongside `-w` on the same command line, overriding whatever an existing config file's own @@ -1666,7 +1681,7 @@ Source layout (all under `src/`): `host.containers.internal` file (`dns-internal-hosts/-`, likewise just deleted) — called from `clean_processes_command()` (`commands.cpp`) alongside the three existing sweeps. `--no-dns` - (`cli_args.{h,cpp}`, long-option only, same precedent as `--no-veth`) opts + (`cli_args.{h,cpp}`, long-option only, plain boolean flag, no value) opts out even when `dnsmasq` is available. `build_bwrap_args()`/`run_bwrap()` (`bwrap.{h,cpp}`) gained a `inject_dns_resolv_conf` bool, resolved once in `run_container()` from `!network_specs.empty() && is_dnsmasq_available() && @@ -2054,20 +2069,32 @@ Source layout (all under `src/`): the local YAML config file located by `config_file_path()` (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`, falling back to `$HOME/.config/slocker-lite/config.yaml`). Supported `global` keys: - `log-level`, and six `unshare-` keys (`unshare-user`/`unshare-ipc`/ + `log-level`; six `unshare-` keys (`unshare-user`/`unshare-ipc`/ `unshare-pid`/`unshare-net`/`unshare-uts`/`unshare-cgroup`, one per `bwrap.cpp`'s own `namespace_probes` entry) controlling whether `-r/--run` - requests each of bwrap's `--unshare-xxx` flags — every other long option is a - one-shot flag, not a setting, so it doesn't belong in a persistent config file. - Each `unshare-*` key accepts `"1"`/`"on"`/`"yes"`/`"true"` (enabled) or - `"0"`/`"off"`/`"no"`/`"false"` (disabled), case-insensitively (`.cpp`-local - `parse_bool_flag()`); an unset key defaults to enabled, and an unrecognized - value logs a `spdlog::warn` and is treated as unset (default enabled) rather - than failing the whole config load — consistent with this file's existing - forward-compatible/ignore-malformed-entries policy (only malformed *YAML - syntax* is a hard error). A missing file returns a default-constructed - (empty) `AppConfig`, not an error; unknown sections/keys (and malformed - individual volume entries) are likewise ignored for forward-compatibility. + requests each of bwrap's `--unshare-xxx` flags; and two `with-` keys + (`with-veth`/`with-ipv6`, `AppConfig::with_veth`/`with_ipv6`) giving + `-n/--network`'s own creation-time `veth`/`ipv6` policy (`NetworkEntry`, + above) a persistent default, used whenever the corresponding + `--with-veth`/`--with-ipv6` CLI flag (`cli_args.{h,cpp}`, see above) isn't + given — every other long option is a one-shot flag, not a setting, so it + doesn't belong in a persistent config file. All eight of these boolean keys + share one small `.cpp`-local `BoolGlobalKey {key, field}` pairing table (two + arrays, `unshare_keys` and `network_default_keys`, both consumed by shared + `load_bool_keys()`/`write_bool_keys()` helpers rather than repeating the same + find-parse-or-warn / emit-if-set loop body per group) and accept + `"1"`/`"on"`/`"yes"`/`"true"` (enabled) or `"0"`/`"off"`/`"no"`/`"false"` + (disabled), case-insensitively — `parse_bool_flag()`, exported (not just this + file's own internal helper) specifically so `cli_args.cpp`'s own + `--with-ipv6`/`--with-veth` value parsing accepts exactly the same forms as + the config file itself, rather than a second, drifting copy. An unset key + defaults to enabled, and an unrecognized value logs a `spdlog::warn` and is + treated as unset (default enabled) rather than failing the whole config load + — consistent with this file's existing forward-compatible/ignore-malformed- + entries policy (only malformed *YAML syntax* is a hard error). A missing file + returns a default-constructed (empty) `AppConfig`, not an error; unknown + sections/keys (and malformed individual volume entries) are likewise ignored + for forward-compatibility. `main()` applies `config->log_level` (via the existing `apply_log_level()`) right after `spdlog::cfg::load_env_levels()` and before parsing CLI options, so an explicit `--log-level` on the command line always overwrites it @@ -2076,8 +2103,8 @@ Source layout (all under `src/`): document-building/emitter API, symmetric to the read side) — used by `-v/--volume` (`create_volume_command()`, `commands.cpp`) to persist a new `VolumeEntry {name, directory}` into the `volumes` section, preserving - `global` (including any set `unshare-*` keys, re-serialized as canonical - `"true"`/`"false"`) untouched. **`VolumeEntry`/the `volumes` section is a + `global` (including any set `unshare-*`/`with-veth`/`with-ipv6` keys, + re-serialized as canonical `"true"`/`"false"`) untouched. **`VolumeEntry`/the `volumes` section is a distinct concept from `OciImageConfig::volumes`**: this is a user-defined `name -> host directory` mapping created via `-v/--volume`, not an image's own declared mount points (still unconsumed, see `oci_image.{h,cpp}` above). @@ -2106,8 +2133,9 @@ Source layout (all under `src/`): `ipv6` (`parse_bool_flag()`, written as canonical `"true"`/`"false"`, always written regardless of value — unlike `subnet6`, there's no companion field whose presence depends on it) — see `network_bridge.h`'s - `probe_veth_support()`/`should_use_veth()` above and `--no-veth` - (`cli_args.{h,cpp}`) below for what it controls. + `probe_veth_support()`/`should_use_veth()` above and `--with-veth`/ + `global.with-veth` (`cli_args.{h,cpp}`/`config_file.{h,cpp}`) below for + what it controls. - `network_subnet.{h,cpp}` — pure CIDR arithmetic backing `-n/--network`'s subnet allocation and `network_bridge.{h,cpp}`'s (see below) gateway-address computation; no kernel/`ip`/`iptables` calls of its own. `is_valid_network_name()` @@ -2253,8 +2281,8 @@ Build directory is `buildDir/` (already configured). full flag list: `-m/--mount`, `-r/--run`, `-u/--umount`, `-c/--cleanup`, `-l/--list-images`, `-i/--inspect`, `-x/--exec`, `--kill`, `--no-nsenter`, `-D/--daemonize`, `--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`, - `--delete-volume-full`, `-n/--network`, `--extern`, `--intern`, `--subnet`, `--no-ipv6`, `--subnet6`, - `--no-veth`, `--list-networks`, `--delete-network`, `-p/--port-forward`, `--no-dns`, `--list-processes`, `--clean-processes`, + `--delete-volume-full`, `-n/--network`, `--extern`, `--intern`, `--subnet`, `--with-ipv6`, `--subnet6`, + `--with-veth`, `--list-networks`, `--delete-network`, `-p/--port-forward`, `--no-dns`, `--list-processes`, `--clean-processes`, `-w/--write-config`, `-t/--test [-- ]`, `--log-level`, `-h/--help`, `-V/--version`) - Run tests: `meson test -C buildDir` (the `[unit]` + safe `[integration]` categories diff --git a/README.md b/README.md index 091b537..d23b550 100644 --- a/README.md +++ b/README.md @@ -125,7 +125,7 @@ slocker-lite -V|--version | `--list-volumes` | List all named volumes (see `-v/--volume`) with their host directory. | | `--delete-volume ` | Remove a named volume from the config. The host directory is left untouched. | | `--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 bridge in its own dedicated namespace, with an uplink out to the host's real network so containers reach it via IPv4 NAT/forwarding — IPv6, if enabled, is same-bridge reachability only, no NAT: its ULA addresses are non-globally-routable by design, and this kernel's IPv6 stack has no NAT support at all regardless) 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. See [`docs/networking-design.md`](docs/networking-design.md) for the full design, including the tap+relay veth fallback. | +| `-n, --network ` | Create/manage a persistent named network: requires exactly one of `--extern` (a bridge in its own dedicated namespace, with an uplink out to the host's real network so containers reach it via IPv4 NAT/forwarding — IPv6, if enabled, is same-bridge reachability only, no NAT: its ULA addresses are non-globally-routable by design, and this kernel's IPv6 stack has no NAT support at all regardless) 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); `--with-ipv6=false` disables (and `--subnet6 ` overrides) the auto-allocated IPv6 range. `--with-veth=false` 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). Both default to the config file's own `global.with-ipv6`/`global.with-veth` (enabled if unset either way — see `-w/--write-config` below) when not given explicitly. 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. See [`docs/networking-design.md`](docs/networking-design.md) for the full design, including the tap+relay veth fallback. | | `--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. Its live bridge/namespace/iptables state is left untouched. | | `--delete-network-full ` | Like `--delete-network`, but also tears down its live state (the IPv4 MASQUERADE rule and the bridge itself for `extern`, or the whole persistent namespace for `intern`), so a network with the same name can be created fresh afterward. | @@ -212,6 +212,8 @@ global: unshare-net: on unshare-uts: on unshare-cgroup: on + with-veth: on + with-ipv6: on volumes: mydata: /home/user/slocker-volumes/mydata networks: @@ -233,7 +235,13 @@ above is also the default with nothing configured. **Note on `unshare-net`**: with no `slirp4netns`-style setup yet implemented, leaving it enabled (the default) means a sandboxed container currently has no network access at all — set `unshare-net: off` if you need the sandbox to see the host's network in -the meantime. No other long options belong in a config file (one-shot +the meantime. `global.with-veth`/`global.with-ipv6` (same accepted values, +same "on unless set otherwise" default) give `-n/--network`'s own creation-time +`--with-veth`/`--with-ipv6` a persistent default, so e.g. a dev machine that's +always used to exercise the tap+relay fallback can set `with-veth: off` once +instead of passing `--with-veth=false` on every network creation — an explicit +`--with-veth`/`--with-ipv6` on the command line still overrides it for that one +call. No other long options belong in a config file (one-shot commands like `--mount`/`--run`/`--user` don't). The `volumes` section is managed by `-v/--volume` (see above) rather than hand-edited — it's what `-r/--run`'s own `-v` usage looks named volumes up in. The `networks` section diff --git a/src/cli_args.cpp b/src/cli_args.cpp index f4476ad..2b63cf4 100644 --- a/src/cli_args.cpp +++ b/src/cli_args.cpp @@ -24,6 +24,7 @@ #include #include "config.h" +#include "config_file.h" namespace { @@ -37,10 +38,13 @@ namespace { // typo-prone brevity isn't worth it. --no-nsenter used to be -n/--no-nsenter; // 'n' was reassigned to the much more heavily-used -n/--network (see below), // so --no-nsenter moved here too -- it's a rare debugging override, long-only -// is no real loss. --extern/--intern/--no-ipv6/--no-veth (booleans) and -// --subnet/--subnet6 (values) only apply to -n/--network's standalone -// (create) use, not the --run-joining use, and have no natural short letter -// of their own worth spending. +// is no real loss. --extern/--intern/--with-ipv6/--with-veth (booleans, +// each taking an explicit true/false value -- e.g. --with-veth=false -- so +// they double as each one's own config-file-default override, see +// config_file.h's global.with-ipv6/global.with-veth) and --subnet/--subnet6 +// (values) only apply to -n/--network's standalone (create) use, not the +// --run-joining use, and have no natural short letter of their own worth +// spending. namespace options { constexpr int log_level = 256; constexpr int user = 257; @@ -58,11 +62,11 @@ constexpr int no_nsenter = 268; constexpr int network_extern = 269; constexpr int network_intern = 270; constexpr int network_subnet = 271; -constexpr int network_no_ipv6 = 272; +constexpr int network_with_ipv6 = 272; constexpr int network_subnet6 = 273; constexpr int list_networks = 274; constexpr int delete_network = 275; -constexpr int network_no_veth = 276; +constexpr int network_with_veth = 276; constexpr int delete_network_full = 277; constexpr int no_dns = 278; } // namespace options @@ -98,9 +102,9 @@ constexpr std::array long_options = {{ {"extern", no_argument, nullptr, options::network_extern}, {"intern", no_argument, nullptr, options::network_intern}, {"subnet", required_argument, nullptr, options::network_subnet}, - {"no-ipv6", no_argument, nullptr, options::network_no_ipv6}, + {"with-ipv6", required_argument, nullptr, options::network_with_ipv6}, {"subnet6", required_argument, nullptr, options::network_subnet6}, - {"no-veth", no_argument, nullptr, options::network_no_veth}, + {"with-veth", required_argument, nullptr, options::network_with_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}, @@ -126,7 +130,7 @@ void print_usage(const char* prog) { " {0} --delete-volume \n" " {0} --delete-volume-full \n" " {0} -n|--network --extern|--intern [--subnet ]\n" - " [--no-ipv6] [--subnet6 ] [--no-veth]\n" + " [--with-ipv6=false] [--subnet6 ] [--with-veth=false]\n" " {0} --list-networks\n" " {0} --delete-network \n" " {0} --delete-network-full \n" @@ -230,11 +234,14 @@ void print_usage(const char* prog) { " to the host's real network) or --intern (only\n" " reachable by other containers on the same\n" " network); --subnet overrides the\n" - " auto-allocated IPv4 range, --no-ipv6 disables\n" - " (and --subnet6 overrides) the\n" - " auto-allocated IPv6 range, on by default; --no-veth\n" - " forces the tap-relay fallback even if the kernel\n" - " supports veth (useful for testing that path). With\n" + " auto-allocated IPv4 range, --with-ipv6=false\n" + " disables (and --subnet6 overrides) the\n" + " auto-allocated IPv6 range -- defaults to the\n" + " config file's own global.with-ipv6 (enabled if\n" + " unset); --with-veth=false forces the tap-relay\n" + " fallback even if the kernel supports veth\n" + " (useful for testing that path) -- defaults to\n" + " global.with-veth (enabled if unset). With\n" " --run, instead join to the container;\n" " may be repeated, no membership limit\n" " --list-networks list all named networks (see -n/--network)\n" @@ -482,15 +489,29 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { case options::network_subnet: out.network_subnet_flag = optarg; break; - case options::network_no_ipv6: - out.network_no_ipv6_flag = true; + case options::network_with_ipv6: { + auto parsed = parse_bool_flag(optarg); + if (!parsed) { + spdlog::error("--with-ipv6: invalid value '{}' (expected true/false)", optarg); + print_usage(argv[0]); + return 1; + } + out.network_with_ipv6_flag = *parsed; break; + } case options::network_subnet6: out.network_subnet6_flag = optarg; break; - case options::network_no_veth: - out.network_no_veth_flag = true; + case options::network_with_veth: { + auto parsed = parse_bool_flag(optarg); + if (!parsed) { + spdlog::error("--with-veth: invalid value '{}' (expected true/false)", optarg); + print_usage(argv[0]); + return 1; + } + out.network_with_veth_flag = *parsed; break; + } case 'p': // Repeatable, only meaningful with -r -- same accumulate-now, // resolve-after-the-loop shape as -n above, except -p has no @@ -571,11 +592,11 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { } bool network_create_flags_given = out.network_extern_flag || out.network_intern_flag || - out.network_subnet_flag.has_value() || out.network_no_ipv6_flag || - out.network_subnet6_flag.has_value() || out.network_no_veth_flag; + out.network_subnet_flag.has_value() || out.network_with_ipv6_flag.has_value() || + out.network_subnet6_flag.has_value() || out.network_with_veth_flag.has_value(); if (network_create_flags_given && out.mode != Mode::network) { spdlog::error( - "--extern/--intern/--subnet/--no-ipv6/--subnet6/--no-veth require standalone --network (not --run)"); + "--extern/--intern/--subnet/--with-ipv6/--subnet6/--with-veth require standalone --network (not --run)"); print_usage(argv[0]); return 1; } @@ -585,8 +606,8 @@ std::optional parse_args(int argc, char* argv[], ParsedArgs& out) { print_usage(argv[0]); return 1; } - if (out.network_subnet6_flag && out.network_no_ipv6_flag) { - spdlog::error("--subnet6 and --no-ipv6 can't be used together"); + if (out.network_subnet6_flag && out.network_with_ipv6_flag == std::optional(false)) { + spdlog::error("--subnet6 and --with-ipv6=false can't be used together"); print_usage(argv[0]); return 1; } diff --git a/src/cli_args.h b/src/cli_args.h index 7382d54..8b654f5 100644 --- a/src/cli_args.h +++ b/src/cli_args.h @@ -69,12 +69,17 @@ struct ParsedArgs { bool network_extern_flag = false; bool network_intern_flag = false; std::optional network_subnet_flag; - bool network_no_ipv6_flag = false; + // --with-ipv6/--with-veth at network-creation time (e.g. --with-veth=false) + // override the config file's own global.with-ipv6/global.with-veth + // (config_file.h), which itself defaults to enabled -- nullopt here means + // "no CLI override, use whatever create_network_command() resolves from + // the config". + std::optional network_with_ipv6_flag; std::optional network_subnet6_flag; - // Force the tap+relay fallback (network_tap_relay.h) even on a - // veth-capable kernel -- lets that path be exercised on a dev machine - // that isn't the real veth-less target device. - bool network_no_veth_flag = false; + // --with-veth=false forces the tap+relay fallback (network_tap_relay.h) + // even on a veth-capable kernel -- lets that path be exercised on a dev + // machine that isn't the real veth-less target device. + std::optional network_with_veth_flag; // -p/--port-forward occurrences, raw // "[:]:" strings (repeatable, only // meaningful with -r) -- parsed into diff --git a/src/commands.cpp b/src/commands.cpp index 9123a38..758f379 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -567,8 +567,11 @@ int write_config_command(const std::filesystem::path& config_path, const AppConf full.unshare_net = full.unshare_net.value_or(true); full.unshare_uts = full.unshare_uts.value_or(true); full.unshare_cgroup = full.unshare_cgroup.value_or(true); + full.with_veth = full.with_veth.value_or(true); + full.with_ipv6 = full.with_ipv6.value_or(true); // `volumes` is left exactly as loaded -- an open-ended list with no - // "default" entry to materialize, unlike the six fixed unshare-* flags. + // "default" entry to materialize, unlike the eight fixed boolean flags + // above. if (!write_config_file(config_path, full)) { return 1; @@ -900,9 +903,14 @@ int dispatch_command(const ParsedArgs& args, const std::filesystem::path& config return write_config_command(config_path, config); case Mode::network: { NetworkKind kind = args.network_extern_flag ? NetworkKind::extern_ : NetworkKind::intern; - return create_network_command(args.network_specs.front(), kind, args.network_subnet_flag, - !args.network_no_ipv6_flag, args.network_subnet6_flag, - !args.network_no_veth_flag, config_path, config); + // An explicit --with-ipv6/--with-veth on the command line wins; + // otherwise fall back to the config file's own global.with-ipv6/ + // global.with-veth (config_file.h), which itself defaults to + // enabled when unset. + bool ipv6 = args.network_with_ipv6_flag.value_or(config.with_ipv6.value_or(true)); + bool veth = args.network_with_veth_flag.value_or(config.with_veth.value_or(true)); + return create_network_command(args.network_specs.front(), kind, args.network_subnet_flag, ipv6, + args.network_subnet6_flag, veth, config_path, config); } case Mode::list_networks: return list_networks_command(config); diff --git a/src/config_file.cpp b/src/config_file.cpp index ad3f268..0727c64 100644 --- a/src/config_file.cpp +++ b/src/config_file.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -35,32 +36,15 @@ std::string_view scalar_value(const yaml_node_t& node) { return {reinterpret_cast(node.data.scalar.value), node.data.scalar.length}; } -// Accepts the usual truthy/falsy string forms, case-insensitively. Returns -// nullopt for anything else -- the caller logs and leaves the field unset -// (equivalent to "enabled", the default for every global.unshare-* key) -// rather than treating a typo as a hard config-file error. -std::optional parse_bool_flag(std::string_view value) { - std::string lower(value); - std::transform(lower.begin(), lower.end(), lower.begin(), - [](unsigned char c) { return static_cast(std::tolower(c)); }); - if (lower == "1" || lower == "on" || lower == "yes" || lower == "true") { - return true; - } - if (lower == "0" || lower == "off" || lower == "no" || lower == "false") { - return false; - } - return std::nullopt; -} - -// Pairs a global.unshare-* YAML key with the AppConfig field it fills, so -// load_config_file()/write_config_file() can share one list instead of -// repeating all six keys twice. -struct UnshareKey { +// Pairs a boolean global.* YAML key with the AppConfig field it fills, so +// load_config_file()/write_config_file() can share one list (per group) +// instead of repeating each group's keys twice. +struct BoolGlobalKey { const char* key; std::optional AppConfig::*field; }; -constexpr std::array unshare_keys = {{ +constexpr std::array unshare_keys = {{ {"unshare-user", &AppConfig::unshare_user}, {"unshare-ipc", &AppConfig::unshare_ipc}, {"unshare-pid", &AppConfig::unshare_pid}, @@ -69,6 +53,13 @@ constexpr std::array unshare_keys = {{ {"unshare-cgroup", &AppConfig::unshare_cgroup}, }}; +// Defaults applied to -n/--network creation's own ipv6/veth policy -- see +// AppConfig::with_veth/with_ipv6's own doc comment (config_file.h). +constexpr std::array network_default_keys = {{ + {"with-veth", &AppConfig::with_veth}, + {"with-ipv6", &AppConfig::with_ipv6}, +}}; + // Finds `key` in a YAML_MAPPING_NODE and returns its value node, or nullptr if // `node` isn't a mapping or has no such (scalar) key. const yaml_node_t* find_in_mapping(yaml_document_t& document, const yaml_node_t& node, @@ -96,8 +87,61 @@ int add_mapping(yaml_document_t& document) { YAML_BLOCK_MAPPING_STYLE); } +// Reads every key in `keys` out of `global` into `config`, one group's worth +// at a time -- shared by load_config_file() for both unshare_keys and +// network_default_keys rather than writing the same loop body twice. Leaves +// a field unset (equivalent to "enabled", the default for every key handled +// here) rather than treating an unrecognized value as a hard config-file +// error. +void load_bool_keys(yaml_document_t& document, const yaml_node_t& global, AppConfig& config, + std::span keys) { + for (const auto& key : keys) { + const yaml_node_t* value = find_in_mapping(document, global, key.key); + if (!value || value->type != YAML_SCALAR_NODE) { + continue; + } + std::string_view raw = scalar_value(*value); + if (auto parsed = parse_bool_flag(raw)) { + config.*key.field = *parsed; + } else { + spdlog::warn("config file: unrecognized value '{}' for global.{}, ignoring (leaving enabled)", raw, + key.key); + } + } +} + +// The write-side counterpart of load_bool_keys() -- appends one mapping pair +// per already-set field in `keys` to `global_mapping`. +void write_bool_keys(yaml_document_t& document, int global_mapping, const AppConfig& config, + std::span keys) { + for (const auto& key : keys) { + if (auto value = config.*key.field) { + yaml_document_append_mapping_pair(&document, global_mapping, add_scalar(document, key.key), + add_scalar(document, *value ? "true" : "false")); + } + } +} + } // namespace +// Accepts the usual truthy/falsy string forms, case-insensitively. Returns +// nullopt for anything else -- callers here log and leave the field unset +// (equivalent to "enabled", the default for every affected key); cli_args.cpp's +// own --with-ipv6/--with-veth parsing (see config_file.h) treats nullopt as a +// hard parse error instead. +std::optional parse_bool_flag(std::string_view value) { + std::string lower(value); + std::transform(lower.begin(), lower.end(), lower.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + if (lower == "1" || lower == "on" || lower == "yes" || lower == "true") { + return true; + } + if (lower == "0" || lower == "off" || lower == "no" || lower == "false") { + return false; + } + return std::nullopt; +} + std::filesystem::path config_file_path() { const char* xdg_config_home = std::getenv("XDG_CONFIG_HOME"); std::filesystem::path config_home; @@ -143,19 +187,8 @@ std::optional load_config_file(const std::filesystem::path& path) { config.log_level = std::string(scalar_value(*log_level)); } } - for (const auto& unshare_key : unshare_keys) { - const yaml_node_t* value = find_in_mapping(document, *global, unshare_key.key); - if (!value || value->type != YAML_SCALAR_NODE) { - continue; - } - std::string_view raw = scalar_value(*value); - if (auto parsed = parse_bool_flag(raw)) { - config.*unshare_key.field = *parsed; - } else { - spdlog::warn("config file: unrecognized value '{}' for global.{}, ignoring (leaving enabled)", - raw, unshare_key.key); - } - } + load_bool_keys(document, *global, config, unshare_keys); + load_bool_keys(document, *global, config, network_default_keys); } if (const yaml_node_t* volumes = find_in_mapping(document, *root, "volumes")) { if (volumes->type == YAML_MAPPING_NODE) { @@ -251,21 +284,18 @@ bool write_config_file(const std::filesystem::path& path, const AppConfig& confi int root = add_mapping(document); - bool has_unshare_override = - std::any_of(unshare_keys.begin(), unshare_keys.end(), - [&](const auto& unshare_key) { return (config.*unshare_key.field).has_value(); }); - if (config.log_level || has_unshare_override) { + auto has_bool_override = [&](const auto& keys) { + return std::any_of(keys.begin(), keys.end(), + [&](const auto& key) { return (config.*key.field).has_value(); }); + }; + if (config.log_level || has_bool_override(unshare_keys) || has_bool_override(network_default_keys)) { int global = add_mapping(document); if (config.log_level) { yaml_document_append_mapping_pair(&document, global, add_scalar(document, "log-level"), add_scalar(document, *config.log_level)); } - for (const auto& unshare_key : unshare_keys) { - if (auto value = config.*unshare_key.field) { - yaml_document_append_mapping_pair(&document, global, add_scalar(document, unshare_key.key), - add_scalar(document, *value ? "true" : "false")); - } - } + write_bool_keys(document, global, config, unshare_keys); + write_bool_keys(document, global, config, network_default_keys); yaml_document_append_mapping_pair(&document, root, add_scalar(document, "global"), global); } diff --git a/src/config_file.h b/src/config_file.h index 14d12eb..b6c84d7 100644 --- a/src/config_file.h +++ b/src/config_file.h @@ -19,8 +19,17 @@ #include #include #include +#include #include +// Parses the usual truthy/falsy string forms accepted by every boolean +// global.* config key ("1"/"on"/"yes"/"true", "0"/"off"/"no"/"false", +// case-insensitively) -- nullopt for anything else. Exported (not just +// config_file.cpp-local) so cli_args.cpp's own --with-ipv6/--with-veth value +// parsing accepts exactly the same forms as the config file itself, rather +// than a second, drifting copy of this logic. +std::optional parse_bool_flag(std::string_view value); + // A user-defined named volume (-v/--volume): maps `name` to a host `directory`. // Unrelated to OciImageConfig::volumes, which are mount points an *image* declares // it wants -- this is a separate, user-driven concept, tracked here so it can be @@ -77,6 +86,14 @@ struct AppConfig { std::optional unshare_uts; // global.unshare-uts (default: enabled) std::optional unshare_cgroup; // global.unshare-cgroup (default: enabled) + // Defaults for -n/--network's own `ipv6`/`veth` policy at creation time + // (NetworkEntry's own fields, above), used whenever the corresponding + // --with-ipv6/--with-veth CLI flag (cli_args.h) isn't given. Same + // accepted values/default-enabled convention as the six unshare-* keys + // above. + std::optional with_veth; // global.with-veth (default: enabled) + std::optional with_ipv6; // global.with-ipv6 (default: enabled) + std::vector volumes; // volumes section: name -> directory std::vector networks; // networks section: name -> {kind, subnet, ipv6, subnet6} diff --git a/tests/integration/test_config_bwrap_chain.cpp b/tests/integration/test_config_bwrap_chain.cpp index b4d6a3a..94b00da 100644 --- a/tests/integration/test_config_bwrap_chain.cpp +++ b/tests/integration/test_config_bwrap_chain.cpp @@ -102,3 +102,26 @@ TEST_CASE("config file -> NamespaceConfig -> bwrap argv: default (unset) config CHECK(contains(argv, flag)); } } + +TEST_CASE("config file -> AppConfig: global.with-veth/with-ipv6 round-trip", "[integration]") { + ScratchXdgDirs scratch; + auto config_path = scratch.path() / "config.yaml"; + + AppConfig written; + written.with_veth = false; + written.with_ipv6 = false; + REQUIRE(write_config_file(config_path, written)); + + auto loaded = load_config_file(config_path); + REQUIRE(loaded.has_value()); + CHECK(loaded->with_veth == std::optional(false)); + CHECK(loaded->with_ipv6 == std::optional(false)); + + // Same "unset means enabled" convention as the six unshare-* keys -- + // create_network_command()'s own resolution (commands.cpp) is + // `args.network_with_*_flag.value_or(config.with_*.value_or(true))`. + auto loaded_empty = load_config_file(scratch.path() / "nonexistent.yaml"); + REQUIRE(loaded_empty.has_value()); + CHECK_FALSE(loaded_empty->with_veth.has_value()); + CHECK_FALSE(loaded_empty->with_ipv6.has_value()); +} diff --git a/tests/unit/test_cli_args.cpp b/tests/unit/test_cli_args.cpp index dfbc6f0..865a84c 100644 --- a/tests/unit/test_cli_args.cpp +++ b/tests/unit/test_cli_args.cpp @@ -171,6 +171,41 @@ TEST_CASE("parse_args: -v with -r accumulates volume_specs as (spec, container-p CHECK(result.args.volume_specs[0].second == "/data"); } +TEST_CASE("parse_args: --with-ipv6/--with-veth accept true/false and default to unset", "[unit]") { + auto plain = run_parse({"-n", "net1", "--extern"}); + REQUIRE_FALSE(plain.exit_code.has_value()); + CHECK_FALSE(plain.args.network_with_ipv6_flag.has_value()); + CHECK_FALSE(plain.args.network_with_veth_flag.has_value()); + + auto disabled = run_parse({"-n", "net1", "--extern", "--with-ipv6=false", "--with-veth=false"}); + REQUIRE_FALSE(disabled.exit_code.has_value()); + REQUIRE(disabled.args.network_with_ipv6_flag.has_value()); + CHECK(*disabled.args.network_with_ipv6_flag == false); + REQUIRE(disabled.args.network_with_veth_flag.has_value()); + CHECK(*disabled.args.network_with_veth_flag == false); + + // Same accepted forms as the config file itself (parse_bool_flag(), + // config_file.h) -- not just a literal "true"/"false". + auto on = run_parse({"-n", "net1", "--intern", "--with-ipv6=on", "--with-veth=1"}); + REQUIRE_FALSE(on.exit_code.has_value()); + REQUIRE(on.args.network_with_ipv6_flag.has_value()); + CHECK(*on.args.network_with_ipv6_flag == true); + REQUIRE(on.args.network_with_veth_flag.has_value()); + CHECK(*on.args.network_with_veth_flag == true); +} + +TEST_CASE("parse_args: --with-ipv6 with an unrecognized value is a parse error", "[unit]") { + auto result = run_parse({"-n", "net1", "--extern", "--with-ipv6=maybe"}); + REQUIRE(result.exit_code.has_value()); + CHECK(*result.exit_code == 1); +} + +TEST_CASE("parse_args: --subnet6 and --with-ipv6=false can't be used together", "[unit]") { + auto result = run_parse({"-n", "net1", "--extern", "--subnet6", "fdf0::/64", "--with-ipv6=false"}); + REQUIRE(result.exit_code.has_value()); + CHECK(*result.exit_code == 1); +} + TEST_CASE("parse_args: --kill requires a numeric pid", "[unit]") { auto ok = run_parse({"--kill", "5678"}); REQUIRE_FALSE(ok.exit_code.has_value()); diff --git a/tests/unit/test_network_subnet.cpp b/tests/unit/test_network_subnet.cpp index 687b285..74525cb 100644 --- a/tests/unit/test_network_subnet.cpp +++ b/tests/unit/test_network_subnet.cpp @@ -105,7 +105,7 @@ TEST_CASE("allocate_ipv6_subnet: skips a subnet already in use", "[unit]") { } TEST_CASE("allocate_ipv6_subnet: an ipv6-disabled existing entry doesn't block reuse of its subnet6", "[unit]") { - // A network created with --no-ipv6 has ipv6=false and an empty subnet6 + // A network created with --with-ipv6=false has ipv6=false and an empty subnet6 // -- nothing to collide with, so this is really just confirming // allocate_ipv6_subnet() doesn't crash/misbehave on such an entry. std::vector existing = {{"v4only", NetworkKind::extern_, "10.168.0.0/24", false, "", true}};