From 499e61346d1822a7ef75811595787bb76035f041 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Tue, 8 Sep 2026 18:35:25 +0200 Subject: [PATCH] fix(net): detect interfaces without following /sys symlinks /sys/class/net/ is always a symlink into /sys/devices; in nested containers (sysbox runtime) that target is not visible, so the link is dead. iface_exists() used Path.exists(), which follows the link and therefore tested the visibility of the target instead of the existence of the interface. detect_egress_iface() discarded every interface it had correctly read from the routing table, so automtu aborted with "Could not detect egress interface" and rc=2 even though `ip -4 route show default` and `ip link show dev eth0` both worked. The documented escape hatch --egress-if was equally dead, because core.py validates it through the same call. Chosen fix: os.path.lexists() rather than probing netlink for existence. It asks the right question -- "is there an entry named " -- and costs no subprocess on a healthy host. Netlink (`ip link show`) is only the fallback for when /sys/class/net itself is unavailable, so sysfs stays the preferred path everywhere. read_iface_mtu() gains the same cascade: sysfs first, MTU parsed from `ip link show dev ` when the sysfs path is unreadable, RuntimeError naming both sources when neither answers - no silent default. core.py turns that into an error line plus rc=3 instead of a traceback. list_ifaces() had the same defect (is_dir() on a dead symlink) and would have left Docker bridge detection blind in the same environments. Regression tests build a /sys replacement whose class/net/eth0 points at a missing target and mock the ip command: they fail if lexists becomes exists again, if the MTU fallback is removed, or if the symlink filter is dropped. Co-Authored-By: Claude Opus 5 (1M context) --- src/automtu/core.py | 16 +++-- src/automtu/net.py | 55 ++++++++++++----- tests/unit/test_core.py | 82 +++++++++++++++++++++++++ tests/unit/test_net.py | 132 +++++++++++++++++++++++++++++++++++----- 4 files changed, 251 insertions(+), 34 deletions(-) diff --git a/src/automtu/core.py b/src/automtu/core.py index 9008758..eaf570c 100644 --- a/src/automtu/core.py +++ b/src/automtu/core.py @@ -2,8 +2,8 @@ from __future__ import annotations import statistics import sys +from collections.abc import Iterable from dataclasses import dataclass -from typing import Iterable, Optional from .docker import detect_docker_ifaces from .net import ( @@ -28,7 +28,7 @@ class Result: wg_mtu: int -def _split_targets(items: Optional[list[str]]) -> list[str]: +def _split_targets(items: list[str] | None) -> list[str]: raw: list[str] = [] for item in items or []: raw.extend([x.strip() for x in item.split(",") if x.strip()]) @@ -119,7 +119,11 @@ def run_automtu(args) -> int: set_iface_mtu(egress, args.force_egress_mtu, args.dry_run) base_mtu = int(args.force_egress_mtu) else: - base_mtu = int(read_iface_mtu(egress)) + try: + base_mtu = int(read_iface_mtu(egress)) + except RuntimeError as exc: + print(f"[automtu][ERROR] {exc}", file=sys.stderr) + return 3 log(f"[automtu] Egress base MTU: {base_mtu}") # Targets (explicit + optional WG auto targets) @@ -140,8 +144,8 @@ def run_automtu(args) -> int: # PMTU probing effective_mtu = base_mtu - probe_results: dict[str, Optional[int]] = {} - chosen_pmtu: Optional[int] = None + probe_results: dict[str, int | None] = {} + chosen_pmtu: int | None = None if targets: log( @@ -186,7 +190,7 @@ def run_automtu(args) -> int: f"[automtu] Computed {args.wg_if} MTU: {wg_mtu} (overhead={args.wg_overhead}, min={args.wg_min})" ) - wg_mtu_set: Optional[int] = None + wg_mtu_set: int | None = None wg_mtu_clamped = False if args.set_wg_mtu is not None: diff --git a/src/automtu/net.py b/src/automtu/net.py index 672a898..b6ea6da 100644 --- a/src/automtu/net.py +++ b/src/automtu/net.py @@ -5,7 +5,6 @@ import pathlib import re import subprocess import sys -from typing import Optional def _run(cmd: list[str]) -> str: @@ -14,27 +13,55 @@ def _run(cmd: list[str]) -> str: ).stdout.strip() +SYSFS_NET = pathlib.Path("/sys/class/net") + + +def _ip_link_show(iface: str) -> str: + return _run(["ip", "link", "show", "dev", iface]) + + def iface_exists(iface: str) -> bool: - return pathlib.Path(f"/sys/class/net/{iface}").exists() + """ + True if the interface exists. Does not follow the /sys/class/net symlink; + falls back to netlink when sysfs is unavailable. + """ + if os.path.lexists(SYSFS_NET / iface): + return True + return bool(_ip_link_show(iface)) def list_ifaces() -> list[str]: """ - Return a sorted list of all network interfaces visible under /sys/class/net. + Return a sorted list of all network interfaces, netlink as fallback. + Plain files in /sys/class/net (e.g. bonding_masters) are not interfaces. """ - base = pathlib.Path("/sys/class/net") - if not base.exists(): - return [] - names: list[str] = [] - for p in base.iterdir(): - if p.is_dir(): - names.append(p.name) - names.sort() - return names + try: + return sorted( + p.name for p in SYSFS_NET.iterdir() if p.is_symlink() or p.is_dir() + ) + except OSError: + out = _run(["ip", "-o", "link", "show"]) + return sorted(re.findall(r"^\d+:\s+([^:@\s]+)", out, flags=re.MULTILINE)) def read_iface_mtu(iface: str) -> int: - return int(pathlib.Path(f"/sys/class/net/{iface}/mtu").read_text().strip()) + """ + Read the interface MTU from sysfs, falling back to netlink. + Raises RuntimeError if neither source reports an MTU. + """ + try: + return int((SYSFS_NET / iface / "mtu").read_text().strip()) + except (OSError, ValueError): + pass + + m = re.search(r"\bmtu\s+(\d+)\b", _ip_link_show(iface)) + if not m: + raise RuntimeError( + f"Could not read MTU of {iface}: " + f"{SYSFS_NET / iface / 'mtu'} is unreadable and " + f"'ip link show dev {iface}' reported no MTU." + ) + return int(m.group(1)) def set_iface_mtu(iface: str, mtu: int, dry: bool) -> None: @@ -53,7 +80,7 @@ def require_root(*, dry: bool, needs_root: bool) -> None: raise SystemExit(1) -def detect_egress_iface(ignore_vpn: bool = True) -> Optional[str]: +def detect_egress_iface(ignore_vpn: bool = True) -> str | None: devs: list[str] = [] for cmd in ( ["ip", "-4", "route", "show", "default"], diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 5cb9915..f5edd1f 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -1,12 +1,46 @@ import io +import pathlib +import tempfile import unittest from contextlib import redirect_stdout from types import SimpleNamespace from unittest.mock import patch +from automtu import net from automtu.core import run_automtu +def _args(**over) -> SimpleNamespace: + base = { + "dry_run": True, + "egress_if": None, + "prefer_wg_egress": False, + "force_egress_mtu": None, + "pmtu_target": None, + "auto_pmtu_from_wg": False, + "pmtu_min_payload": 1200, + "pmtu_max_payload": 1472, + "pmtu_timeout": 1.0, + "pmtu_policy": "min", + "apply_egress_mtu": False, + "apply_wg_mtu": False, + "apply_docker_mtu": False, + "apply_all": False, + "docker_if": None, + "docker_no_user_bridges": False, + "wg_if": "wg0", + "wg_overhead": 80, + "wg_min": 1280, + "set_wg_mtu": None, + "persist": None, + "uninstall": False, + "print_mtu": None, + "print_json": False, + } + base.update(over) + return SimpleNamespace(**base) + + class TestCore(unittest.TestCase): def test_run_automtu_happy_path_all_mocked(self) -> None: args = SimpleNamespace( @@ -164,5 +198,53 @@ class TestCore(unittest.TestCase): mock_set.assert_not_called() +class TestCoreInSysboxContainer(unittest.TestCase): + """Container with dead /sys/class/net symlinks: routing and ip link work.""" + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + root = pathlib.Path(tmp.name) + self.netdir = root / "class" / "net" + self.netdir.mkdir(parents=True) + for name in ("eth0", "lo"): + (self.netdir / name).symlink_to(root / "devices" / "virtual" / "net" / name) + + @staticmethod + def _fake_run(cmd: list[str]) -> str: + if cmd[:5] == ["ip", "-4", "route", "show", "default"]: + return "default via 172.28.0.1 dev eth0" + if cmd[:3] == ["ip", "link", "show"] and cmd[-1] == "eth0": + return ( + "2: eth0@if5: mtu 1450 qdisc noqueue state UP" + ) + return "" + + def test_print_mtu_effective_yields_number_and_rc0(self) -> None: + with ( + patch.object(net, "SYSFS_NET", self.netdir), + patch.object(net, "_run", side_effect=self._fake_run), + patch("automtu.core.probe_pmtu", return_value=1400), + ): + buf = io.StringIO() + with redirect_stdout(buf): + rc = run_automtu(_args(pmtu_target=["1.1.1.1"], print_mtu="effective")) + + self.assertEqual(rc, 0) + self.assertEqual(int(buf.getvalue().strip()), 1400) + + def test_explicit_egress_if_is_accepted(self) -> None: + with ( + patch.object(net, "SYSFS_NET", self.netdir), + patch.object(net, "_run", side_effect=self._fake_run), + ): + buf = io.StringIO() + with redirect_stdout(buf): + rc = run_automtu(_args(egress_if="eth0", print_mtu="egress")) + + self.assertEqual(rc, 0) + self.assertEqual(int(buf.getvalue().strip()), 1450) + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/tests/unit/test_net.py b/tests/unit/test_net.py index e293f46..c1149ce 100644 --- a/tests/unit/test_net.py +++ b/tests/unit/test_net.py @@ -1,8 +1,120 @@ +import pathlib +import tempfile import unittest -from pathlib import Path from unittest.mock import patch -import automtu.net as net +from automtu import net + +IP_LINK_ETH0 = ( + "2: eth0@if5: mtu 1420 qdisc noqueue state UP " + "mode DEFAULT group default\n link/ether 02:42:ac:1c:00:02 brd ff:ff:ff:ff:ff:ff" +) +IP_LINK_ALL = ( + "1: lo: mtu 65536 qdisc noqueue state UNKNOWN\n" + "2: eth0@if5: mtu 1420 qdisc noqueue state UP" +) + + +def _sysbox_sysfs(root: pathlib.Path, ifaces=("eth0", "lo")) -> pathlib.Path: + """/sys replacement as seen inside a sysbox container: dead symlinks.""" + netdir = root / "class" / "net" + netdir.mkdir(parents=True) + for name in ifaces: + (netdir / name).symlink_to(root / "devices" / "virtual" / "net" / name) + return netdir + + +class SysboxSysfsBase(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.root = pathlib.Path(self._tmp.name) + self.addCleanup(self._tmp.cleanup) + + +class TestDeadSymlinkSysfs(SysboxSysfsBase): + def test_iface_exists_true_for_dead_symlink_without_netlink(self) -> None: + netdir = _sysbox_sysfs(self.root) + self.assertFalse((netdir / "eth0").exists()) + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", return_value=""), + ): + self.assertTrue(net.iface_exists("eth0")) + self.assertFalse(net.iface_exists("eth9")) + + def test_iface_exists_falls_back_to_netlink_without_sysfs(self) -> None: + with ( + patch.object(net, "SYSFS_NET", self.root / "absent" / "net"), + patch.object(net, "_run", return_value=IP_LINK_ETH0), + ): + self.assertTrue(net.iface_exists("eth0")) + + def test_detect_egress_iface_accepts_dead_symlink_iface(self) -> None: + netdir = _sysbox_sysfs(self.root) + + def fake_run(cmd: list[str]) -> str: + if cmd[:5] == ["ip", "-4", "route", "show", "default"]: + return "default via 172.28.0.1 dev eth0" + return "" + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", side_effect=fake_run), + ): + self.assertEqual(net.detect_egress_iface(), "eth0") + + def test_list_ifaces_keeps_dead_symlinks_and_skips_plain_files(self) -> None: + netdir = _sysbox_sysfs(self.root, ("eth0", "lo", "br-abc123")) + (netdir / "bonding_masters").write_text("") + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", return_value=""), + ): + self.assertEqual(net.list_ifaces(), ["br-abc123", "eth0", "lo"]) + + def test_list_ifaces_falls_back_to_netlink_without_sysfs(self) -> None: + with ( + patch.object(net, "SYSFS_NET", self.root / "absent" / "net"), + patch.object(net, "_run", return_value=IP_LINK_ALL), + ): + self.assertEqual(net.list_ifaces(), ["eth0", "lo"]) + + +class TestReadIfaceMtu(SysboxSysfsBase): + def test_prefers_sysfs_over_netlink(self) -> None: + netdir = self.root / "class" / "net" + (netdir / "eth0").mkdir(parents=True) + (netdir / "eth0" / "mtu").write_text("1500\n") + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", return_value=IP_LINK_ETH0) as run, + ): + self.assertEqual(net.read_iface_mtu("eth0"), 1500) + run.assert_not_called() + + def test_netlink_fallback_on_dead_symlink(self) -> None: + netdir = _sysbox_sysfs(self.root) + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", return_value=IP_LINK_ETH0), + ): + self.assertEqual(net.read_iface_mtu("eth0"), 1420) + + def test_raises_when_neither_sysfs_nor_netlink_answers(self) -> None: + netdir = _sysbox_sysfs(self.root) + + with ( + patch.object(net, "SYSFS_NET", netdir), + patch.object(net, "_run", return_value=""), + self.assertRaises(RuntimeError) as ctx, + ): + net.read_iface_mtu("eth0") + + self.assertIn("eth0", str(ctx.exception)) class TestNet(unittest.TestCase): @@ -46,18 +158,10 @@ class TestNet(unittest.TestCase): self.assertFalse(net.default_route_uses_iface("wg0")) def test_list_ifaces_returns_sorted_names(self) -> None: - fake = [ - Path("/sys/class/net/eth0"), - Path("/sys/class/net/lo"), - Path("/sys/class/net/wg0"), - ] - - with ( - patch("automtu.net.pathlib.Path.exists", return_value=True), - patch("automtu.net.pathlib.Path.iterdir", return_value=fake), - patch.object(Path, "is_dir", return_value=True), - ): - self.assertEqual(net.list_ifaces(), ["eth0", "lo", "wg0"]) + with tempfile.TemporaryDirectory() as tmp: + netdir = _sysbox_sysfs(pathlib.Path(tmp), ("wg0", "eth0", "lo")) + with patch.object(net, "SYSFS_NET", netdir): + self.assertEqual(net.list_ifaces(), ["eth0", "lo", "wg0"]) if __name__ == "__main__":