Bladeren bron

Let a virtual printer be told which address to advertise

BambuStudio reads its FTP upload destination out of net.info[].ip in the
MQTT status, and the bridge fills that field from the VP's bind address.
On Docker bridge networking the two are different machines' worth of
address: slicers reach Bambuddy on the host's LAN IP, the container binds
something like 172.24.0.2, and that private address is what the slicer was
handed -- so it opened an FTP connection to an address that does not exist
on its network and the send stalled around 10%.

VIRTUAL_PRINTER_ADVERTISE_ADDRESS supplies the address slicers actually
use, taking precedence over the bind address and over the same-subnet host
interface the bridge falls back to. The armed log line names its source, so
(VIRTUAL_PRINTER_ADVERTISE_ADDRESS) against (bind_address) tells an
operator whether the variable reached the container at all, and the
not-armed diagnostic now names it as the remedy -- a bridge-network install
that has not set it is exactly the one that cannot auto-resolve either, and
until now saw only that nothing worked.

An environment variable rather than a change to how the advertised address
is resolved, which is the decision worth recording. The VP already has a
"Network Interface Override" field, and reading it here is the smaller
patch, but it feeds SSDP and the certificate SANs only: honouring it would
silently move the upload destination on every install that has one set --
the multi-NIC, VLAN and Tailscale setups, which are the ones most likely to
have been arrived at by hand and least likely to survive being
second-guessed. Unset, this changes nothing, and a test pins that.

A value that is not a dotted-quad IPv4 is refused with one warning naming
it and the previous address is used instead. That direction is deliberate:
declining to rewrite would put the real printer's IP back in front of the
slicer, which is the leak the rewrite exists to close, so a typo must not
be able to reopen it. 0.0.0.0 counts as unset and whitespace is stripped,
for values pasted into a compose file.

Host and macvlan networking need none of this and stay what Virtual Printer
is developed against. The variable removes one blocker; it does not make
bridge mode equivalent. The wiki said in three places that the host address
could not be discovered at all, which is no longer true, so those now
describe the variable and keep the recommendation.
maziggy 1 week geleden
bovenliggende
commit
c094102614
4 gewijzigde bestanden met toevoegingen van 241 en 14 verwijderingen
  1. 0 0
      CHANGELOG.md
  2. 73 14
      backend/app/services/virtual_printer/mqtt_bridge.py
  3. 160 0
      backend/tests/unit/test_vp_mqtt_bridge.py
  4. 8 0
      docker-compose.yml

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 73 - 14
backend/app/services/virtual_printer/mqtt_bridge.py

@@ -23,9 +23,11 @@ Identity rewriting at cache time:
 
   - `upgrade_state.sn` (and any other nested dict's `sn` matching the real
     serial) → VP serial
-  - `net.info[*].ip` little-endian uint32 → VP bind IP. BambuStudio reads
-    this as the FTP destination IP. Without this the slicer FTPs straight
-    to the real printer and bypasses Bambuddy.
+  - `net.info[*].ip` little-endian uint32 → the address a slicer can reach
+    Bambuddy on. BambuStudio reads this as the FTP destination IP. Without
+    this the slicer FTPs straight to the real printer and bypasses Bambuddy.
+    Normally that address is the VP bind IP; `VIRTUAL_PRINTER_ADVERTISE_ADDRESS`
+    overrides it for NAT'd deployments (see `ADVERTISE_ADDRESS_ENV`).
   - `ipcam.rtsp_url` is left unchanged: BambuStudio overrides the URL host
     with the device IP it bound to (the VP), so the slicer hits the VP's
     own RTSPS proxy on port 322.
@@ -38,6 +40,7 @@ import copy
 import ipaddress
 import json
 import logging
+import os
 import socket
 from typing import TYPE_CHECKING
 
@@ -53,6 +56,24 @@ logger = logging.getLogger(__name__)
 
 REFRESH_INTERVAL_SECONDS = 30.0
 
+# Opt-in override for the address written into `net.info[].ip`. Needed only
+# where the address a slicer has to use to reach Bambuddy is not one of the
+# container's own interfaces — Docker bridge networking being the case that
+# prompted it (#2930), where the bind address is a container-private IP like
+# `172.24.0.2` and a slicer that follows it opens an FTP connection to
+# nothing. Host and macvlan networking stay the supported modes and need
+# nothing set here.
+#
+# Deliberately an environment variable rather than a change to how the VP IP
+# is resolved: the alternative was to prefer the VP's "Network Interface
+# Override" (`remote_interface_ip`), which today feeds SSDP and the cert SANs
+# only. Reading it here would silently move the FTP destination for every
+# install that has it set — the multi-NIC, VLAN and Tailscale setups, i.e.
+# exactly the ones most likely to have been tuned by hand. Unset, this
+# variable changes nothing. Mirrors `VIRTUAL_PRINTER_PASV_ADDRESS`, which
+# exists for the same reason on the FTP side.
+ADVERTISE_ADDRESS_ENV = "VIRTUAL_PRINTER_ADVERTISE_ADDRESS"
+
 # Bambuddy's internal printer state in bambu_mqtt.py (around line 2686+) is
 # updated per-field — each `if "X" in data: self.state.X = ...` block leaves
 # every other field untouched, so the state accumulates everything the
@@ -134,6 +155,32 @@ def _resolve_host_interface_for_target(target_ip: str) -> str | None:
     return ip if isinstance(ip, str) and ip else None
 
 
+def _resolve_advertise_override(vp_name: str) -> str:
+    """Return the validated `net.info[].ip` override from the environment, or "".
+
+    Validated here rather than on each refresh tick for two reasons: a typo
+    produces one warning instead of one every 30s, and an unusable value
+    falls back to the bind address instead of leaving the rewrite unarmed.
+    That second part matters — an unarmed rewrite puts the *real printer IP*
+    back in front of the slicer (#1429), so a mistyped override must not be
+    able to reopen the leak this whole path exists to close.
+    """
+    raw = os.environ.get(ADVERTISE_ADDRESS_ENV, "").strip()
+    if not raw:
+        return ""
+    try:
+        _ip_to_uint32_le(raw)
+    except ValueError:
+        logger.warning(
+            "[%s] %s=%r is not a dotted-quad IPv4 — ignoring it, using the VP bind address instead",
+            vp_name,
+            ADVERTISE_ADDRESS_ENV,
+            raw,
+        )
+        return ""
+    return raw
+
+
 def _merge_ams_dict(prev_ams: dict, new_ams: dict) -> dict:
     """Merge a new ``ams`` blob from an incremental push onto the previous one.
 
@@ -269,6 +316,10 @@ class MQTTBridge:
         # follow-up: makes silent early-returns visible without grepping the
         # source.
         self._not_armed_reason: str | None = None
+        # NAT escape hatch for `net.info[].ip`, resolved once — the process
+        # environment cannot change without a restart. "" means "use the VP
+        # bind address", which is every install that has not set it.
+        self._advertise_address = _resolve_advertise_override(vp_name)
         self._loop: asyncio.AbstractEventLoop | None = None
         self._refresh_task: asyncio.Task | None = None
         self._stopping = False
@@ -427,12 +478,15 @@ class MQTTBridge:
         sees the rewritten value (#1429). Without this sweep the sticky-key
         preservation keeps the poisoned `net.info[].ip` alive forever.
 
-        VP bind IP resolution: when `mqtt_server.bind_address` is empty or
-        `0.0.0.0` (the default for VPs that were never assigned a dedicated
-        bind IP), fall back to auto-resolving the host interface in the same
-        subnet as the printer's IP. Without this fallback, the rewrite never
-        arms on a default-config flat-LAN install and `net.info[].ip` leaks
-        the real printer IP — slicer follows it on Send (#1429 residual).
+        VP IP resolution, in order: the `VIRTUAL_PRINTER_ADVERTISE_ADDRESS`
+        override if one is set (NAT'd deployments where no local interface
+        carries the address slicers use — see `ADVERTISE_ADDRESS_ENV`), then
+        `mqtt_server.bind_address`, then — when that is empty or `0.0.0.0`,
+        the default for VPs never assigned a dedicated bind IP — the host
+        interface sharing a subnet with the printer's IP. Without that last
+        fallback the rewrite never arms on a default-config flat-LAN install
+        and `net.info[].ip` leaks the real printer IP — the slicer follows it
+        on Send (#1429 residual).
         """
 
         def _log_not_armed(reason: str) -> None:
@@ -463,14 +517,19 @@ class MQTTBridge:
             )
             return
 
-        vp_ip = getattr(self._mqtt_server, "bind_address", None)
-        vp_ip_source = "bind_address"
+        if self._advertise_address:
+            vp_ip = self._advertise_address
+            vp_ip_source = ADVERTISE_ADDRESS_ENV
+        else:
+            vp_ip = getattr(self._mqtt_server, "bind_address", None)
+            vp_ip_source = "bind_address"
         if not vp_ip or vp_ip in ("0.0.0.0", ""):  # nosec B104
             resolved = _resolve_host_interface_for_target(target_ip)
             if not resolved:
                 _log_not_armed(
                     f"no host interface shares a subnet with printer IP {target_ip} "
-                    "(and VP bind_address is 0.0.0.0/empty)"
+                    f"(and VP bind_address is 0.0.0.0/empty) — set {ADVERTISE_ADDRESS_ENV} "
+                    "to the address slicers reach Bambuddy on if this host is NAT'd"
                 )
                 return
             vp_ip = resolved
@@ -514,7 +573,7 @@ class MQTTBridge:
                 )
 
     def _rewrite_net_info_ips(self, print_state: dict) -> int:
-        """Rewrite every non-zero `net.info[].ip` in `print_state` to the VP bind IP.
+        """Rewrite every non-zero `net.info[].ip` in `print_state` to the VP's IP.
 
         Returns the number of entries rewritten. Mutates `print_state` in place.
 
@@ -595,7 +654,7 @@ class MQTTBridge:
             # stream directly from the printer. On the same LAN this works as
             # long as the slicer's stored access code matches the printer's
             # (i.e. configure the VP with the same access code as its target).
-            # Rewrite real printer IP → VP bind IP in `net.info[*].ip` so the
+            # Rewrite real printer IP → the VP's IP in `net.info[*].ip` so the
             # slicer's FTP destination resolves to the VP, not the real printer.
             self._rewrite_net_info_ips(print_data)
             # Defensive deep copy on store so the cache is fully decoupled from

+ 160 - 0
backend/tests/unit/test_vp_mqtt_bridge.py

@@ -1863,6 +1863,166 @@ class TestBindAddressAutoResolve:
             assert _resolve_host_interface_for_target("203.0.113.1") is None
 
 
+# ---------------------------------------------------------------------------
+# VIRTUAL_PRINTER_ADVERTISE_ADDRESS override (#2930)
+# ---------------------------------------------------------------------------
+
+
+class TestAdvertiseAddressOverride:
+    """#2930: behind NAT — Docker bridge networking being the case that
+    prompted it — no local interface carries the address a slicer uses to
+    reach Bambuddy, so both the bind address and the auto-resolved host
+    interface put a container-private IP into `net.info[].ip` and the
+    slicer's FTP upload goes nowhere. The env override supplies that address
+    directly. It is opt-in precisely so that every install which does not
+    set it keeps the behaviour it has today.
+    """
+
+    ENV = "VIRTUAL_PRINTER_ADVERTISE_ADDRESS"
+    HOST_IP = "192.168.1.50"
+    CONTAINER_IP = "172.24.0.2"
+
+    @staticmethod
+    def _bound_bridge(bind_address: str) -> MQTTBridge:
+        """A bridge with its target client already attached, so
+        `_refresh_ip_encoding` reaches IP resolution instead of returning
+        early on an unbound client."""
+        bridge = _make_bridge(_make_server(bind_address=bind_address))
+        bridge._target_client = _make_paho_client()
+        return bridge
+
+    @pytest.mark.asyncio
+    async def test_override_wins_over_an_explicit_bind_address(self, monkeypatch):
+        """The bind address is the container IP; the slicer has to be told the
+        host IP or its FTP connection has nowhere to land."""
+        monkeypatch.setenv(self.ENV, self.HOST_IP)
+        bridge = _make_bridge(_make_server(bind_address=self.CONTAINER_IP))
+        await bridge.start()
+        try:
+            payload = json.dumps(
+                {
+                    "print": {
+                        "command": "push_status",
+                        "net": {"info": [{"ip": _ip_to_uint32_le(H2D_IP), "mask": 0xFFFFFF}]},
+                    }
+                }
+            ).encode()
+            bridge._on_printer_raw(f"device/{H2D_SERIAL}/report", payload)
+            await asyncio.sleep(0.01)
+
+            assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(self.HOST_IP)
+            encoded = bridge.get_latest_print_state()["net"]["info"][0]["ip"]
+            assert encoded == _ip_to_uint32_le(self.HOST_IP)
+            # Decoded independently of the helper that produced it: asserting
+            # both sides with `_ip_to_uint32_le` would agree even if the byte
+            # order were wrong, and the slicer reads this field as LE.
+            assert socket.inet_ntoa(encoded.to_bytes(4, "little")) == self.HOST_IP
+        finally:
+            await bridge.stop()
+
+    @pytest.mark.asyncio
+    async def test_override_wins_over_auto_resolve(self, monkeypatch):
+        """A VP left on the default 0.0.0.0 bind would otherwise auto-resolve a
+        host interface — inside a bridge-network container that resolves to the
+        container's own address, or to nothing at all."""
+        monkeypatch.setenv(self.ENV, self.HOST_IP)
+        bridge = self._bound_bridge("0.0.0.0")  # nosec B104
+        with patch(
+            "backend.app.services.virtual_printer.mqtt_bridge._resolve_host_interface_for_target",
+            return_value=self.CONTAINER_IP,
+        ):
+            bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(self.HOST_IP)
+
+    def test_unset_leaves_the_bind_address_untouched(self, monkeypatch):
+        """The whole point of making this opt-in: with nothing set, resolution
+        is byte-for-byte what it was before the override existed."""
+        monkeypatch.delenv(self.ENV, raising=False)
+        bridge = self._bound_bridge(VP_IP)
+        bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
+
+    def test_invalid_override_falls_back_instead_of_disarming(self, caplog, monkeypatch):
+        """A typo must not un-arm the rewrite. Unarmed means the *real printer
+        IP* reaches the slicer (#1429) — strictly worse than the wrong VP IP,
+        so an unusable override degrades to the bind address."""
+        monkeypatch.setenv(self.ENV, "192.168.1")
+        with caplog.at_level(logging.WARNING, logger="backend.app.services.virtual_printer.mqtt_bridge"):
+            bridge = self._bound_bridge(VP_IP)
+        bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
+        warnings = [r for r in caplog.records if self.ENV in r.getMessage()]
+        assert len(warnings) == 1, "operator gets exactly one line naming the bad value"
+        assert "192.168.1" in warnings[0].getMessage()
+
+    def test_hostname_override_falls_back(self, monkeypatch):
+        """`net.info[].ip` is a uint32 — a hostname cannot round-trip through
+        it, and resolving one here would pick an address the operator did not
+        choose. Fall back rather than guess."""
+        monkeypatch.setenv(self.ENV, "bambuddy.local")
+        bridge = self._bound_bridge(VP_IP)
+        bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
+
+    def test_whitespace_only_override_is_ignored(self, monkeypatch):
+        """`VIRTUAL_PRINTER_ADVERTISE_ADDRESS=` in a compose file is how people
+        leave a variable listed but unused — not a configuration error."""
+        monkeypatch.setenv(self.ENV, "   ")
+        bridge = self._bound_bridge(VP_IP)
+        bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
+
+    def test_surrounding_whitespace_is_tolerated(self, monkeypatch):
+        """Copy-pasted compose values carry stray spaces; that should not cost
+        the user a warning and a silent fall back to the wrong address."""
+        monkeypatch.setenv(self.ENV, f"  {self.HOST_IP}  ")
+        bridge = self._bound_bridge(self.CONTAINER_IP)
+        bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(self.HOST_IP)
+
+    def test_wildcard_override_falls_through_to_auto_resolve(self, monkeypatch):
+        """0.0.0.0 is a bind address, never a destination — treat it as unset
+        rather than encoding it and sending the slicer to 0.0.0.0."""
+        monkeypatch.setenv(self.ENV, "0.0.0.0")
+        bridge = self._bound_bridge("0.0.0.0")  # nosec B104
+        with patch(
+            "backend.app.services.virtual_printer.mqtt_bridge._resolve_host_interface_for_target",
+            return_value=VP_IP,
+        ):
+            bridge._refresh_ip_encoding()
+        assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
+
+    def test_armed_log_names_the_override_as_the_source(self, caplog, monkeypatch):
+        """An operator reading the log has to be able to tell where the
+        advertised IP came from, otherwise a stale variable is invisible."""
+        monkeypatch.setenv(self.ENV, self.HOST_IP)
+        bridge = self._bound_bridge(self.CONTAINER_IP)
+        with caplog.at_level(logging.INFO, logger="backend.app.services.virtual_printer.mqtt_bridge"):
+            bridge._refresh_ip_encoding()
+        armed = [r for r in caplog.records if "IP encoding armed" in r.getMessage()]
+        assert len(armed) == 1
+        assert self.ENV in armed[0].getMessage()
+        assert self.HOST_IP in armed[0].getMessage()
+
+    def test_not_armed_message_points_at_the_override(self, caplog, monkeypatch):
+        """The bridge-network install that has NOT set the variable is exactly
+        the one that cannot auto-resolve — the diagnostic has to name the
+        remedy, or the operator sees only that nothing works."""
+        monkeypatch.delenv(self.ENV, raising=False)
+        bridge = self._bound_bridge("0.0.0.0")  # nosec B104
+        with (
+            patch(
+                "backend.app.services.virtual_printer.mqtt_bridge._resolve_host_interface_for_target",
+                return_value=None,
+            ),
+            caplog.at_level(logging.INFO, logger="backend.app.services.virtual_printer.mqtt_bridge"),
+        ):
+            bridge._refresh_ip_encoding()
+        not_armed = [r for r in caplog.records if "NOT armed" in r.getMessage()]
+        assert len(not_armed) == 1
+        assert self.ENV in not_armed[0].getMessage()
+
+
 class TestNotArmedDiagnosticLogging:
     """#1429 follow-up: every silent early-return in `_refresh_ip_encoding`
     now emits one INFO line explaining WHY the rewrite couldn't arm. Throttled

+ 8 - 0
docker-compose.yml

@@ -123,6 +123,14 @@ services:
       # Required for FTP passive mode to work behind NAT.
       # Example: VIRTUAL_PRINTER_PASV_ADDRESS=192.168.1.100
       #- VIRTUAL_PRINTER_PASV_ADDRESS=
+      # Virtual printer: the address slicers use to reach Bambuddy, written into
+      # the MQTT status the slicer reads its upload destination from. Only needed
+      # in bridge mode, where that address belongs to the host and the container
+      # cannot see it — without this the slicer is told the container IP (e.g.
+      # 172.17.0.2) and the upload has nowhere to go. Leave unset on host or
+      # macvlan networking, which remain the recommended modes.
+      # Example: VIRTUAL_PRINTER_ADVERTISE_ADDRESS=192.168.1.100
+      #- VIRTUAL_PRINTER_ADVERTISE_ADDRESS=
       #
       # External PostgreSQL (optional — uses SQLite by default)
       # Example: DATABASE_URL=postgresql+asyncpg://bambuddy:password@db-host:5432/bambuddy

Some files were not shown because too many files changed in this diff