From dc63c840d454fe2ba834c6a1ef6c260c8a115e5b Mon Sep 17 00:00:00 2001 From: Sebastiano Romi Date: Sat, 29 Aug 2026 16:43:31 +0200 Subject: [PATCH] Fix Windows PACE network routing --- cross_platform/src/pace_controller/network.py | 168 ++++++++++++++---- cross_platform/src/pace_controller/service.py | 8 +- .../src/pace_controller/transports.py | 35 +++- cross_platform/tests/test_core.py | 96 ++++++++++ 4 files changed, 262 insertions(+), 45 deletions(-) diff --git a/cross_platform/src/pace_controller/network.py b/cross_platform/src/pace_controller/network.py index f716e86..e192f2d 100644 --- a/cross_platform/src/pace_controller/network.py +++ b/cross_platform/src/pace_controller/network.py @@ -2,6 +2,7 @@ from __future__ import annotations +import ipaddress import json import os import platform @@ -19,6 +20,8 @@ class NetworkLease: interface: str address_added: bool platform_name: str + route_added: bool = False + source_address: str = "192.168.10.1" def configure_dedicated_adapter( @@ -35,20 +38,37 @@ def configure_dedicated_adapter( def restore_dedicated_adapter(lease: NetworkLease | None, address: str = "192.168.10.1") -> None: - if lease is None or not lease.address_added: + if lease is None: return + address = lease.source_address or address if lease.platform_name == "Windows": - script = ( - f"Remove-NetIPAddress -InterfaceIndex {int(lease.interface)} " - f"-IPAddress '{address}' -Confirm:$false -ErrorAction SilentlyContinue" - ) + commands: list[str] = [] + if lease.route_added: + commands.append( + f"Remove-NetRoute -InterfaceIndex {int(lease.interface)} " + "-AddressFamily IPv4 -DestinationPrefix '192.168.10.0/24' " + "-Confirm:$false -ErrorAction SilentlyContinue" + ) + if lease.address_added: + commands.append( + f"Remove-NetIPAddress -InterfaceIndex {int(lease.interface)} " + f"-IPAddress '{address}' -Confirm:$false -ErrorAction SilentlyContinue" + ) + if not commands: + return subprocess.run( - ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], + [ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + "; ".join(commands), + ], check=False, capture_output=True, text=True, ) - elif lease.platform_name == "Linux": + elif lease.platform_name == "Linux" and lease.address_added: command = ["ip", "address", "delete", f"{address}/24", "dev", lease.interface] _run_linux_privileged(command, check=False) @@ -78,38 +98,115 @@ def _configure_windows(address: str, prefix_length: int) -> NetworkLease: except json.JSONDecodeError as exc: raise NetworkConfigurationError("Cannot parse Windows adapter information") from exc adapters = raw if isinstance(raw, list) else [raw] - for item in adapters: - ips = item.get("IPs", []) - if isinstance(ips, str): - ips = [ips] - if address in ips: - return NetworkLease(str(item["Index"]), False, "Windows") - candidates = [ - item - for item in adapters - if not item.get("HasGateway", False) - and all(str(ip).startswith("169.254.") for ip in (item.get("IPs", []) if isinstance(item.get("IPs", []), list) else [item.get("IPs")])) - ] - if len(candidates) != 1: + network = ipaddress.ip_network(f"{address}/{prefix_length}", strict=False) + owners = [item for item in adapters if address in _windows_ips(item)] + if len(owners) > 1: raise NetworkConfigurationError( - f"Expected exactly one safe dedicated Ethernet adapter; found {len(candidates)}. No adapter was modified." + f"Address {address} is present on multiple adapters. No adapter was modified." ) - index = int(candidates[0]["Index"]) - command = ( - f"New-NetIPAddress -InterfaceIndex {index} -IPAddress '{address}' " - f"-PrefixLength {prefix_length} -ErrorAction Stop | Out-Null" - ) - result = subprocess.run( - ["powershell", "-NoProfile", "-NonInteractive", "-Command", command], + + address_added = False + if owners: + selected = owners[0] + else: + candidates = [ + item + for item in adapters + if not item.get("HasGateway", False) + and all(value.is_link_local for value in _windows_ipv4_addresses(item)) + ] + if len(candidates) != 1: + raise NetworkConfigurationError( + "Expected exactly one safe dedicated Ethernet adapter; " + f"found {len(candidates)}. No adapter was modified." + ) + selected = candidates[0] + + conflicts = [ + item + for item in adapters + if item is not selected + and any(value in network for value in _windows_ipv4_addresses(item)) + ] + if conflicts: + raise NetworkConfigurationError( + f"Network {network} is already used by another adapter. No adapter was modified." + ) + + index = int(selected["Index"]) + command = ( + f"New-NetIPAddress -InterfaceIndex {index} -AddressFamily IPv4 " + f"-IPAddress '{address}' -PrefixLength {prefix_length} " + "-PolicyStore ActiveStore -ErrorAction Stop | Out-Null" + ) + result = _run_windows_powershell(command) + if result.returncode != 0: + raise NetworkConfigurationError( + result.stderr.strip() + or "Administrator privileges are required to configure Ethernet." + ) + address_added = True + + index = int(selected["Index"]) + destination = str(network) + prepare = f""" +$temporaryAddress = $null +for ($attempt = 1; $attempt -le 20; $attempt++) {{ + $temporaryAddress = @(Get-NetIPAddress -InterfaceIndex {index} -AddressFamily IPv4 -IPAddress '{address}' -ErrorAction SilentlyContinue | + Where-Object AddressState -eq 'Preferred') + if ($temporaryAddress.Count -gt 0) {{ break }} + Start-Sleep -Milliseconds 500 +}} +if ($temporaryAddress.Count -eq 0) {{ + throw 'Windows did not make {address}/{prefix_length} operational on adapter {index}.' +}} +$routes = @(Get-NetRoute -InterfaceIndex {index} -AddressFamily IPv4 -DestinationPrefix '{destination}' -ErrorAction SilentlyContinue) +if ($routes.Count -eq 0) {{ + New-NetRoute -InterfaceIndex {index} -AddressFamily IPv4 -DestinationPrefix '{destination}' -NextHop '0.0.0.0' -RouteMetric 1 -PolicyStore ActiveStore -ErrorAction Stop | Out-Null + Write-Output 'created' +}} else {{ + Write-Output 'existing' +}} +""" + result = _run_windows_powershell(prepare) + if result.returncode != 0: + if address_added: + restore_dedicated_adapter( + NetworkLease(str(index), True, "Windows", source_address=address) + ) + raise NetworkConfigurationError( + result.stderr.strip() or "Cannot prepare the Windows route to the PACE." + ) + route_added = result.stdout.strip().splitlines()[-1:] == ["created"] + return NetworkLease(str(index), address_added, "Windows", route_added, address) + + +def _windows_ips(item: dict[str, object]) -> list[str]: + values = item.get("IPs", []) + if isinstance(values, str): + return [values] + if not isinstance(values, list): + return [] + return [str(value) for value in values if value] + + +def _windows_ipv4_addresses(item: dict[str, object]) -> list[ipaddress.IPv4Address]: + addresses: list[ipaddress.IPv4Address] = [] + for value in _windows_ips(item): + try: + addresses.append(ipaddress.IPv4Address(value)) + except ipaddress.AddressValueError: + continue + return addresses + + +def _run_windows_powershell(script: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], check=False, capture_output=True, text=True, ) - if result.returncode != 0: - raise NetworkConfigurationError( - result.stderr.strip() or "Administrator privileges are required to configure Ethernet." - ) - return NetworkLease(str(index), True, "Windows") def _configure_linux(address: str, prefix_length: int) -> NetworkLease: @@ -147,7 +244,7 @@ def _configure_linux(address: str, prefix_length: int) -> NetworkLease: if entry.get("family") == "inet" ] if address in addresses: - return NetworkLease(name, False, "Linux") + return NetworkLease(name, False, "Linux", source_address=address) if all(str(value).startswith("169.254.") for value in addresses): candidates.append(name) if len(candidates) != 1: @@ -159,7 +256,7 @@ def _configure_linux(address: str, prefix_length: int) -> NetworkLease: ["ip", "address", "add", f"{address}/{prefix_length}", "dev", name], check=True, ) - return NetworkLease(name, True, "Linux") + return NetworkLease(name, True, "Linux", source_address=address) def _run_linux_privileged(command: list[str], check: bool) -> None: @@ -173,4 +270,3 @@ def _run_linux_privileged(command: list[str], check: bool) -> None: raise NetworkConfigurationError( result.stderr.strip() or "Failed to configure the dedicated Ethernet adapter." ) - diff --git a/cross_platform/src/pace_controller/service.py b/cross_platform/src/pace_controller/service.py index c98749a..68e1448 100644 --- a/cross_platform/src/pace_controller/service.py +++ b/cross_platform/src/pace_controller/service.py @@ -208,9 +208,12 @@ def _connect(self, config: ConnectionConfig, module: int) -> None: raise self._network_lease = configure_dedicated_adapter() self._write_log( - f"Temporarily configured dedicated adapter {self._network_lease.interface} for 192.168.10.1/24." + f"Prepared dedicated adapter {self._network_lease.interface} " + "and route for 192.168.10.1/24." + ) + self._transport = create_transport( + config, source_address=self._network_lease.source_address ) - self._transport = create_transport(config) self._transport.connect() identity = self._query("*IDN?") @@ -590,4 +593,3 @@ def _write_log(self, message: str) -> None: def _emit_alarm(self, key: str, **values: object) -> None: self.alarm.emit({"key": key, **values}) - diff --git a/cross_platform/src/pace_controller/transports.py b/cross_platform/src/pace_controller/transports.py index ba50771..3c19edb 100644 --- a/cross_platform/src/pace_controller/transports.py +++ b/cross_platform/src/pace_controller/transports.py @@ -35,21 +35,37 @@ def query(self, command: str) -> str: ... class TcpTransport(ScpiTransport): - def __init__(self, host: str, port: int = 5025, timeout: float = 2.0) -> None: + def __init__( + self, + host: str, + port: int = 5025, + timeout: float = 2.0, + source_address: str | None = None, + ) -> None: self.host = host self.port = port self.timeout = timeout + self.source_address = source_address self._socket: socket.socket | None = None self._buffer = bytearray() self._lock = threading.Lock() def connect(self) -> None: self.close() + connection: socket.socket | None = None try: - self._socket = socket.create_connection((self.host, self.port), self.timeout) - self._socket.settimeout(self.timeout) + source = (self.source_address, 0) if self.source_address else None + connection = socket.create_connection( + (self.host, self.port), self.timeout, source_address=source + ) + connection.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) + connection.settimeout(self.timeout) + self._socket = connection except OSError as exc: - raise TransportError(f"TCP {self.host}:{self.port}: {exc}") from exc + if connection is not None: + connection.close() + via = f" via {self.source_address}" if self.source_address else "" + raise TransportError(f"TCP {self.host}:{self.port}{via}: {exc}") from exc def close(self) -> None: if self._socket is not None: @@ -337,9 +353,16 @@ def _normalize(command: str) -> str: return " ".join(command.strip().upper().split()) -def create_transport(config: ConnectionConfig) -> ScpiTransport: +def create_transport( + config: ConnectionConfig, *, source_address: str | None = None +) -> ScpiTransport: if config.kind == ConnectionKind.ETHERNET: - return TcpTransport(config.host, config.port, config.timeout) + return TcpTransport( + config.host, + config.port, + config.timeout, + source_address=source_address, + ) if config.kind == ConnectionKind.SERIAL: if not config.serial_port: raise TransportError("No serial port selected") diff --git a/cross_platform/tests/test_core.py b/cross_platform/tests/test_core.py index 8e39d18..7c34f39 100644 --- a/cross_platform/tests/test_core.py +++ b/cross_platform/tests/test_core.py @@ -2,7 +2,9 @@ import hashlib import importlib.util +import json import socket +import subprocess import threading import time from pathlib import Path @@ -10,6 +12,7 @@ import pytest from pace_controller import __version__ +from pace_controller import network from pace_controller.external import host_environment from pace_controller.i18n import STRINGS from pace_controller.leak import LeakMonitor @@ -185,6 +188,99 @@ def serve() -> None: assert received == [b"*IDN?\r\n", b":UNIT1:PRES?\r\n"] +def test_tcp_transport_binds_the_dedicated_adapter(monkeypatch: pytest.MonkeyPatch) -> None: + calls: list[object] = [] + + class FakeSocket: + def setsockopt(self, level: int, option: int, value: int) -> None: + calls.append(("setsockopt", level, option, value)) + + def settimeout(self, timeout: float) -> None: + calls.append(("settimeout", timeout)) + + def shutdown(self, how: int) -> None: + calls.append(("shutdown", how)) + + def close(self) -> None: + calls.append(("close",)) + + def fake_create_connection( + destination: tuple[str, int], + timeout: float, + source_address: tuple[str, int] | None = None, + ) -> FakeSocket: + calls.append(("connect", destination, timeout, source_address)) + return FakeSocket() + + monkeypatch.setattr(socket, "create_connection", fake_create_connection) + transport = TcpTransport( + "192.168.10.2", 5025, timeout=4.0, source_address="192.168.10.1" + ) + transport.connect() + transport.close() + + assert calls[0] == ( + "connect", + ("192.168.10.2", 5025), + 4.0, + ("192.168.10.1", 0), + ) + assert ("setsockopt", socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) in calls + + +def test_windows_auto_network_waits_adds_route_and_restores( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[list[str]] = [] + responses = iter( + [ + subprocess.CompletedProcess( + [], + 0, + stdout=json.dumps( + [ + { + "Index": 3, + "Name": "PACE Ethernet", + "IPs": ["169.254.233.163"], + "HasGateway": False, + }, + { + "Index": 16, + "Name": "Corporate", + "IPs": ["10.0.0.20"], + "HasGateway": True, + }, + ] + ), + stderr="", + ), + subprocess.CompletedProcess([], 0, stdout="", stderr=""), + subprocess.CompletedProcess([], 0, stdout="created\n", stderr=""), + subprocess.CompletedProcess([], 0, stdout="", stderr=""), + ] + ) + + def fake_run(command: list[str], **_: object) -> subprocess.CompletedProcess[str]: + calls.append(command) + return next(responses) + + monkeypatch.setattr(network.subprocess, "run", fake_run) + lease = network._configure_windows("192.168.10.1", 24) + + assert lease == network.NetworkLease( + "3", True, "Windows", True, "192.168.10.1" + ) + assert "-PolicyStore ActiveStore" in calls[1][-1] + assert "AddressState -eq 'Preferred'" in calls[2][-1] + assert "New-NetRoute" in calls[2][-1] + + network.restore_dedicated_adapter(lease) + cleanup = calls[3][-1] + assert "Remove-NetRoute" in cleanup + assert "Remove-NetIPAddress" in cleanup + + @pytest.mark.parametrize( ("elapsed", "drop", "expected"), [