Quellcode durchsuchen

fix(vp): offer every host IP as a bind target, not one per adapter (issue #3121)

Each enabled virtual printer needs its own IP address, and the documented
way to get several is to add secondary addresses to the adapter already in
use. Linux reads those back through `ip -j addr show`, which reports every
address. Windows and macOS have no `ip` command and fell through to a psutil
enumeration that stopped at the first IPv4 of each adapter, so a host with
three addresses on one NIC offered exactly one bind target and the second
virtual printer could only fail with "Bind IP ... is already in use".

The psutil path now collects every address, marking the ones after an
adapter's first as aliases the way the iproute2 path does. Callers that want
interfaces rather than addresses -- the discovery scan and the support bundle
-- project the primaries back out, so their view is unchanged.

The ioctl fallback that a Linux host without iproute2 used to get is now
reached only when psutil itself is missing, which gains that host aliases
too. The interface-name exclusions stay Linux-only: they are Linux device
names, and a Windows adapter called "Local Area Connection" matches the "lo"
prefix.
maziggy vor 5 Tagen
Ursprung
Commit
74173eab8d
3 geänderte Dateien mit 239 neuen und 18 gelöschten Zeilen
  1. 1 0
      CHANGELOG.md
  2. 79 18
      backend/app/services/network_utils.py
  3. 159 0
      backend/tests/unit/services/test_network_utils.py

+ 1 - 0
CHANGELOG.md

@@ -29,6 +29,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **Every FTP session Bambuddy opens now records how it closed (#3009, reported by @grengojbo)** — the report traced a print completion that opened two FTP connections to the printer, deleted one file and then, as far as the log showed, did nothing else until the printer was powered off 21 minutes later, and concluded the connections were being left open. They were not: the post-print SD-card cleanup opens one connection per candidate filename and closes each in a `finally`, which a run against a real FTPS server confirms at the server end for both the delete and the 550 not-here case. The trouble is that nothing in the log could have said so. Neither the clean close nor the hard socket drop logged anything at any level, so a session closed properly and a socket genuinely abandoned produced the same output — none — and the only way to tell them apart was to read the source. Both now log one DEBUG line naming the printer, whether QUIT was acknowledged or the socket had to be dropped without it, why, and how long the session was held. Every connect in a debug log is now paired with a close, so the next person suspecting a leaked FTP connection can settle it from a support bundle rather than by inference. Nothing about the connection handling itself changed, and at default log level nothing new is printed. This does not explain the SD-card read/write error in that report or in #645; it only removes one theory from the list by making it checkable.
 
 ### Fixed
+- **On Windows and macOS the Virtual Printer's bind dropdown offered one entry per network adapter, not one per IP address (#3121, reported by @SJB-OLVG)** — each enabled virtual printer needs its own address, and the documented way to get several is to add secondary IPs to the adapter you already have. Linux reads those back through `ip -j addr show`, which reports every address; Windows and macOS have no `ip` command and fell through to an enumeration that stopped at the first IPv4 of each adapter. A Windows host with three addresses on one NIC therefore offered exactly one bind target, and creating a second virtual printer could only fail with "Bind IP ... is already in use". Every address is now listed on those platforms too, with the ones after an adapter's first marked as aliases exactly as on Linux. A Linux host without `iproute2` installed gains the same thing, having previously been limited to one address per interface for the same reason. The wiki gained the Windows instructions for adding those addresses, which it had for every other platform but that one, and the creation steps it describes now match the dialog — bind interface, model and access code are set on the printer's card after **Create**, not in the dialog itself.
 - **A macOS native install could silently lose all access to the printer after a system or Homebrew update (#3114, reported by @TheeBobbyDonuts)** — macOS grants Local Network permission to a code signature, and judges a launchd-spawned process on its own rather than letting it inherit the grant of the Terminal that started it. Homebrew ships Python unsigned on Intel, so there was no identity for that grant to attach to: every connection to the printer was dropped with no error Bambuddy could log and no permission prompt to answer. The web interface worked, the printer showed as disconnected, all three port checks reported unreachable while the subnet check passed, and the log recorded no MQTT attempt at all — not a failure, just silence. Toggling the Python entry in Privacy & Security did nothing, because the entry referred to an identity that no longer resolved. `install.sh` now ad-hoc signs the interpreter during a macOS install, and `install/update_macos.sh` re-checks it on every update, since `brew upgrade python` lays down a fresh unsigned binary under a new versioned path. Both sign only what is currently unsigned: on Apple Silicon the linker already ad-hoc signs every binary, and re-signing one would rotate the hash its identity is built from and revoke a working grant on each update. The connection diagnostic also gained a **macOS Local Network permission** check, which reports the missing signature where there is one, and otherwise points at System Settings — where an Apple Silicon machine, whose identity changes with every Python upgrade, can re-grant it.
 - **The Library bulk add-to-queue answered 200 for a call that queued nothing, and queued items nothing could dispatch (#3112, reported by @toxxicpickles)** — `POST /api/v1/library/files/add-to-queue` reported every per-file rejection in an `errors` array and returned `200` regardless, so a caller that checked the status code saw a successful request, no visible failure, and no queue item. That is now a `400` when nothing at all was added, with the same per-file reasons in the body; a call that created some items still succeeds, because it did. The items it created were also aimed at nothing: the route always wrote them with no printer and no target model, and the scheduler dispatches on one or the other, so those rows could never be picked up by anything and sat in Unassigned until someone opened each one and assigned it by hand. The request now takes an optional `printer_id` or `target_model` for the batch, and with neither it aims each file at the model its own G-code declares — when a printer of that model is active, since aiming a job at hardware nobody owns would only look like progress. Three gates `POST /api/v1/queue/` has applied for a while now apply here too, because an item reaching the scheduler through this route has to be as printable as one reaching it through that one: the cross-model check that stops a file sliced for one printer being dispatched to another, the filename check that would otherwise surface as a failed upload hours later, and the filament requirements the scheduler matches before handing a model-based item to hardware. This endpoint has no caller inside Bambuddy — the Library's own Print action goes through the queue API with a printer already chosen — which is how it came to drift this far from it.
 - **The connection diagnostic told anyone whose LAN was not a /24 that their printer was on a different network (#3092, reported by @cwawak)** — the reporter's LAN is `192.168.96.0/22`, his Bambuddy sits at `192.168.96.9` and his printer at `192.168.98.170`, four hundred addresses inside the same subnet. The *Network subnet* check warned that the two were on different networks and told him to go configure routing between two halves of one LAN. An IPv4 address does not carry its prefix, and the check supplied `/24` for both sides — the most common LAN, and not the only one. It now reads the prefix off the interface that owns the address, so a `/22`, a `/16` and a `/25` are each read as written, and when no local interface claims the address the check is skipped rather than asserting a split it cannot see. The same check also picked which of Bambuddy's own addresses to compare by probing a route toward `10.255.255.255`, which on any host with more than one network card is not the card the printer is on; it now asks for the route toward the printer itself. On a two-NIC host that alone was enough to warn about a printer sitting on the second card's own subnet.

+ 79 - 18
backend/app/services/network_utils.py

@@ -26,8 +26,8 @@ def _is_excluded(name: str) -> bool:
     return any(name.startswith(prefix) for prefix in EXCLUDED_INTERFACE_PREFIXES)
 
 
-def _get_network_interfaces_psutil() -> list[dict]:
-    """Non-Linux path (Windows, macOS, BSD): enumerate interfaces via psutil.
+def _psutil_ipv4_entries(exclude_by_name: bool = False) -> list[dict]:
+    """Every bindable IPv4 address psutil reports, one entry per address.
 
     The ioctl request numbers in the Linux path (SIOCGIFADDR 0x8915,
     SIOCGIFNETMASK 0x891B) and the sockaddr layout they return are
@@ -38,10 +38,22 @@ def _get_network_interfaces_psutil() -> list[dict]:
     (``psutil>=6.0.0``) and gives cross-platform name + IPv4 + netmask in one
     call, so we use it for everything that isn't Linux.
 
+    Secondary addresses are included. psutil returns every unicast address
+    bound to an adapter, so a Windows host with three IPs on one NIC offers
+    three bind targets rather than one (#3121) — the same thing iproute2 gives
+    Linux. ``is_alias`` marks every address after an interface's first, which
+    is the closest Windows equivalent of an iproute2 alias label.
+
     Filters: IPv4 only (matches the Linux path), skip loopback and
     link-local (169.254.0.0/16), skip interfaces psutil reports as down.
-    No name-based exclusion — users may legitimately want to bind a VP to a
-    Hyper-V / WSL / Tailscale / utun virtual adapter.
+
+    Args:
+        exclude_by_name: apply ``EXCLUDED_INTERFACE_PREFIXES``. Only ever true
+            on Linux — those are Linux device names, and a Windows adapter
+            named "Local Area Connection" would match the ``lo`` prefix. The
+            address-class filters above cover the equivalent ground elsewhere,
+            and users may legitimately want to bind a VP to a Hyper-V / WSL /
+            Tailscale / utun adapter.
     """
     try:
         import psutil
@@ -49,7 +61,7 @@ def _get_network_interfaces_psutil() -> list[dict]:
         logger.warning("psutil not available, interface detection unavailable on this platform")
         return []
 
-    interfaces = []
+    entries = []
     try:
         addrs_by_iface = psutil.net_if_addrs()
         stats_by_iface = psutil.net_if_stats()
@@ -58,10 +70,14 @@ def _get_network_interfaces_psutil() -> list[dict]:
         return []
 
     for name, addrs in addrs_by_iface.items():
+        if exclude_by_name and _is_excluded(name):
+            continue
+
         stats = stats_by_iface.get(name)
         if stats is not None and not stats.isup:
             continue
 
+        ipv4_count = 0
         for addr in addrs:
             if addr.family != socket.AF_INET:
                 continue
@@ -82,20 +98,42 @@ def _get_network_interfaces_psutil() -> list[dict]:
             except ValueError:
                 continue
 
-            interfaces.append(
+            entries.append(
                 {
                     "name": name,
                     "ip": ip,
                     "netmask": netmask,
                     "subnet": str(network),
+                    # No label to read on this path, so position is all we
+                    # have: the first address an adapter reports is its
+                    # primary, the rest are secondaries.
+                    "is_alias": ipv4_count > 0,
+                    "label": name,
                 }
             )
-            # First IPv4 per interface is enough; matches Linux ioctl which
-            # returns only the primary IP (aliases land via get_all_interface_ips
-            # on Linux, which has no Windows analogue worth replicating).
-            break
+            ipv4_count += 1
+
+    return entries
 
-    return interfaces
+
+def _get_network_interfaces_psutil() -> list[dict]:
+    """The primary IPv4 of each interface, in ``get_network_interfaces`` shape.
+
+    That function's callers want one subnet per interface — discovery scan
+    targets, the support bundle — not one entry per alias, so the secondary
+    addresses are dropped here rather than never collected.
+    """
+    return [
+        {key: entry[key] for key in ("name", "ip", "netmask", "subnet")}
+        for entry in _psutil_ipv4_entries()
+        if not entry["is_alias"]
+    ]
+
+
+def _sort_interface_entries(entries: list[dict]) -> list[dict]:
+    """Sort in place and return: primary IPs first per interface, then by name."""
+    entries.sort(key=lambda e: (e["name"], e["is_alias"], e["ip"]))
+    return entries
 
 
 def get_network_interfaces(include_excluded: bool = False) -> list[dict]:
@@ -181,7 +219,8 @@ def get_all_interface_ips(include_excluded: bool = False) -> list[dict]:
     """Get all IPs (primary + aliases) for every interface, minus the excluded ones.
 
     Uses `ip -j addr show` to see secondary/alias IPs that ioctl misses.
-    Falls back to ioctl-based get_network_interfaces() if `ip` is unavailable.
+    Falls back to :func:`_fallback_get_all_ips` wherever `ip` isn't there to
+    ask — which is every non-Linux host.
 
     Args:
         include_excluded: see :func:`get_network_interfaces`.
@@ -189,8 +228,11 @@ def get_all_interface_ips(include_excluded: bool = False) -> list[dict]:
     Returns:
         List of dicts with name, ip, netmask, subnet, is_alias, label
     """
-    if not _IP_CMD:
-        logger.debug("ip command not found, using ioctl fallback")
+    # Windows and macOS have no `ip`, so there is nothing to try first. Going
+    # straight to psutil is what lets a Windows NIC carrying three IPs offer
+    # three bind targets instead of one (#3121).
+    if not sys.platform.startswith("linux") or not _IP_CMD:
+        logger.debug("ip command unavailable on this platform, enumerating via psutil")
         return _fallback_get_all_ips(include_excluded)
 
     try:
@@ -245,13 +287,32 @@ def get_all_interface_ips(include_excluded: bool = False) -> list[dict]:
             )
             ipv4_count += 1
 
-    # Sort: primary IPs first per interface, then by interface name
-    entries.sort(key=lambda e: (e["name"], e["is_alias"], e["ip"]))
-    return entries
+    return _sort_interface_entries(entries)
 
 
 def _fallback_get_all_ips(include_excluded: bool = False) -> list[dict]:
-    """Fallback: wrap get_network_interfaces() result with alias fields."""
+    """Enumerate without iproute2: psutil first, ioctl only if it finds nothing.
+
+    psutil is the better answer because it reports secondary addresses, so a
+    host with no `ip` command still gets one bind target per IP instead of per
+    interface. The ioctl wrap below is what such a host used to get (minus the
+    aliases it never saw) and is kept for the one case psutil can't serve: a
+    hand-rolled venv missing the dependency. It only ever runs on Linux, since
+    the ioctl path returns nothing anywhere else.
+    """
+    # EXCLUDED_INTERFACE_PREFIXES are Linux device names; see _psutil_ipv4_entries.
+    exclude_by_name = sys.platform.startswith("linux") and not include_excluded
+    entries = _psutil_ipv4_entries(exclude_by_name=exclude_by_name)
+    if entries:
+        # Deliberately not sorted. psutil's adapter order is what this path has
+        # always returned, and find_interface_for_ip() answers with the first
+        # entry whose subnet holds the target -- which the MQTT bridge uses as
+        # the source IP for the #1429 rewrite and the SSDP proxy as its local
+        # interface. Re-ordering it would quietly re-pick those on a host with
+        # two adapters on one subnet. The iproute2 path sorts because it always
+        # has; only Linux sees that order.
+        return entries
+
     return [
         {
             **iface,

+ 159 - 0
backend/tests/unit/services/test_network_utils.py

@@ -128,3 +128,162 @@ class TestFindLocalIPv4Network:
 
     def test_a_hostname_is_none(self):
         assert network_utils.find_local_ipv4_network("printer.local") is None
+
+
+def _fake_windows_psutil():
+    """#3121's host: one vmxnet3 NIC carrying three IPv4 addresses.
+
+    "Local Area Connection" is here on purpose — it starts with ``lo``, so it
+    is what EXCLUDED_INTERFACE_PREFIXES would eat if the Linux name filter were
+    applied to Windows adapter names.
+    """
+    addrs = {
+        "Ethernet0": [
+            _Addr(socket.AF_INET, "10.10.24.6", "255.255.255.0", None, None),
+            _Addr(socket.AF_INET, "10.10.24.7", "255.255.255.0", None, None),
+            _Addr(socket.AF_INET, "10.10.24.8", "255.255.255.0", None, None),
+        ],
+        "Local Area Connection": [_Addr(socket.AF_INET, "192.168.7.5", "255.255.255.0", None, None)],
+    }
+    stats = {
+        "Ethernet0": _Stats(True, 0, 0, 1500, 0),
+        "Local Area Connection": _Stats(True, 0, 0, 1500, 0),
+    }
+    return addrs, stats
+
+
+def _patch_psutil(addrs, stats):
+    """Both psutil calls the enumerator makes, as one context manager."""
+    return patch.multiple(
+        "psutil",
+        net_if_addrs=lambda: addrs,
+        net_if_stats=lambda: stats,
+    )
+
+
+class TestSecondaryAddresses:
+    """#3121: a NIC with several IPv4 addresses is several VP bind targets.
+
+    The Virtual Printer needs one bind IP per printer. Linux gets one dropdown
+    entry per alias from `ip -j addr show`; Windows and macOS have no `ip`, so
+    everything they offer comes out of psutil.
+    """
+
+    def test_every_ipv4_on_an_interface_is_listed(self):
+        addrs, stats = _fake_windows_psutil()
+        with _patch_psutil(addrs, stats):
+            entries = network_utils._psutil_ipv4_entries()
+
+        eth0 = [e for e in entries if e["name"] == "Ethernet0"]
+        assert [e["ip"] for e in eth0] == ["10.10.24.6", "10.10.24.7", "10.10.24.8"]
+        # Position is the only alias signal on this path: first = primary.
+        assert [e["is_alias"] for e in eth0] == [False, True, True]
+        assert {e["subnet"] for e in eth0} == {"10.10.24.0/24"}
+
+    def test_get_network_interfaces_still_returns_one_per_interface(self):
+        """Discovery subnets and the support bundle want interfaces, not aliases."""
+        addrs, stats = _fake_windows_psutil()
+        with _patch_psutil(addrs, stats):
+            result = network_utils._get_network_interfaces_psutil()
+
+        assert [i["ip"] for i in result if i["name"] == "Ethernet0"] == ["10.10.24.6"]
+        # The narrower shape this function has always returned.
+        assert set(result[0]) == {"name", "ip", "netmask", "subnet"}
+
+    @patch("backend.app.services.network_utils.sys")
+    def test_windows_dropdown_offers_each_secondary_ip(self, mock_sys):
+        """The actual bug: only one entry per NIC reached the bind dropdown."""
+        mock_sys.platform = "win32"
+        addrs, stats = _fake_windows_psutil()
+        with _patch_psutil(addrs, stats):
+            entries = network_utils.get_all_interface_ips()
+
+        assert [e["ip"] for e in entries if e["name"] == "Ethernet0"] == [
+            "10.10.24.6",
+            "10.10.24.7",
+            "10.10.24.8",
+        ]
+
+    @patch("backend.app.services.network_utils.sys")
+    def test_windows_keeps_adapters_matching_a_linux_prefix(self, mock_sys):
+        """EXCLUDED_INTERFACE_PREFIXES must not run against Windows names."""
+        mock_sys.platform = "win32"
+        addrs, stats = _fake_windows_psutil()
+        with _patch_psutil(addrs, stats):
+            entries = network_utils.get_all_interface_ips()
+
+        assert "Local Area Connection" in {e["name"] for e in entries}
+
+    @patch("backend.app.services.network_utils.sys")
+    def test_linux_without_iproute2_gets_aliases_and_keeps_its_exclusions(self, mock_sys):
+        """psutil replaces the ioctl fallback, so no-`ip` hosts see aliases too.
+
+        The name exclusions still apply here — unlike Windows, these really are
+        the local device names, and docker0 has no business in the dropdown.
+        """
+        mock_sys.platform = "linux"
+        addrs = {
+            "eth0": [
+                _Addr(socket.AF_INET, "192.168.1.100", "255.255.255.0", None, None),
+                _Addr(socket.AF_INET, "192.168.1.101", "255.255.255.0", None, None),
+            ],
+            "docker0": [_Addr(socket.AF_INET, "172.17.0.1", "255.255.0.0", None, None)],
+        }
+        stats = {"eth0": _Stats(True, 0, 0, 1500, 0), "docker0": _Stats(True, 0, 0, 1500, 0)}
+
+        with _patch_psutil(addrs, stats), patch.object(network_utils, "_IP_CMD", None):
+            entries = network_utils.get_all_interface_ips()
+            unfiltered = network_utils.get_all_interface_ips(include_excluded=True)
+
+        assert [e["ip"] for e in entries] == ["192.168.1.100", "192.168.1.101"]
+        assert "docker0" not in {e["name"] for e in entries}
+        assert "docker0" in {e["name"] for e in unfiltered}
+
+    @patch("backend.app.services.network_utils.sys")
+    def test_ioctl_remains_the_last_resort(self, mock_sys):
+        """A venv without psutil still enumerates, just without the aliases."""
+        mock_sys.platform = "linux"
+        with (
+            patch.object(network_utils, "_IP_CMD", None),
+            patch.object(network_utils, "_psutil_ipv4_entries", return_value=[]),
+            patch.object(
+                network_utils,
+                "get_network_interfaces",
+                return_value=[
+                    {"name": "eth0", "ip": "192.168.1.100", "netmask": "255.255.255.0", "subnet": "192.168.1.0/24"}
+                ],
+            ),
+        ):
+            entries = network_utils.get_all_interface_ips()
+
+        assert entries == [
+            {
+                "name": "eth0",
+                "ip": "192.168.1.100",
+                "netmask": "255.255.255.0",
+                "subnet": "192.168.1.0/24",
+                "is_alias": False,
+                "label": "eth0",
+            }
+        ]
+
+    @patch("backend.app.services.network_utils.sys")
+    def test_adapter_order_is_preserved_for_source_ip_selection(self, mock_sys):
+        """find_interface_for_ip() answers with the first match, so order matters.
+
+        The MQTT bridge takes that answer as the source IP for the #1429
+        rewrite and the SSDP proxy as its local interface. On a host with two
+        adapters on one subnet, re-ordering the enumeration would silently
+        re-pick both, so this path stays in psutil's adapter order rather than
+        being sorted by name the way the iproute2 path is.
+        """
+        mock_sys.platform = "win32"
+        addrs = {
+            "Zeta": [_Addr(socket.AF_INET, "10.10.24.6", "255.255.255.0", None, None)],
+            "Alpha": [_Addr(socket.AF_INET, "10.10.24.9", "255.255.255.0", None, None)],
+        }
+        stats = {"Zeta": _Stats(True, 0, 0, 1500, 0), "Alpha": _Stats(True, 0, 0, 1500, 0)}
+
+        with _patch_psutil(addrs, stats):
+            assert [e["name"] for e in network_utils.get_all_interface_ips()] == ["Zeta", "Alpha"]
+            assert network_utils.find_interface_for_ip("10.10.24.200")["name"] == "Zeta"