mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-07 06:31:22 +02:00
fix(vp): resolve hostname/FQDN targets in MQTT bridge IP encoding (#1429 follow-up)
Printers added to Bambuddy by hostname/FQDN (e.g. p1s.fritz.box) hit 'invalid IPv4' in _ip_to_uint32_le, so the net.info[*].ip rewrite never armed and BambuStudio Send went straight to the real printer instead of the Bambuddy archive whenever the printer was powered on. Add _resolve_target_to_ipv4(target): IPv4 pass-through, else socket.getaddrinfo(target, family=AF_INET). AF_INET filter is load-bearing because net.info[*].ip is uint32 LE and IPv6 can't round-trip. OSError returns None so a transient DNS failure recovers on the next 30s refresh tick via the existing not-armed throttle. Apply the resolver to both the encode call and the host-interface picker (which also assumes dotted-quad). Armed log line now carries configured->resolved when they differ, so bad-DNS regressions stay legible in 'docker logs'. The unresolvable not-armed reason now names the configured value rather than parroting 'invalid IPv4', distinguishing 'DNS gave a v6 result' from 'user typed garbage'. Root-caused by @Mape6; @TrickShotMLG02 confirmed the FQDN workaround on the same release. Pre-0.2.4 these setups worked by accident because there was no net.info[].ip rewrite at all.
This commit is contained in:
maziggy
@@ -20,6 +20,8 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **`docker-compose.yml`: bridge-mode warning about the 1001-port FTP passive range + docker-proxy RAM footprint (#1646, reported by @TheFou)** — Reporter on bridge mode (Docker default `userland-proxy: true`) saw ~2000 `docker-proxy` host processes spawn from the commented `"50000-51000:50000-51000"` line, pinning ~3.5 GB of host RAM before they had even logged in for the first time. Linux's host-mode default in the same compose file sidesteps this entirely (zero docker-proxy cost) — the issue only fires when a user forces bridge mode (typically macOS/Windows / Docker Desktop). The 1001-port range itself is load-bearing on the VP server side (`virtual_printer/ftp_server.py:567-574` documents the widening from 100 ports as multi-VP collision-avoidance headroom; reverting would regress that), so the fix is documentation, not code. Added a warning block above the commented FTP-passive line pointing bridge-mode users at `{ "userland-proxy": false }` in `/etc/docker/daemon.json` — the reporter confirmed this clears the issue on their setup. Kernel does NAT directly via iptables/nftables in that mode, no per-port host process needed; only side-effect is that connections originating from 127.0.0.1 on the host itself can't reach the container, which doesn't matter for nearly every Bambuddy install.
|
||||
|
||||
### Fixed
|
||||
- **VP MQTT bridge `net.info[].ip` rewrite never armed when the printer was added by hostname/FQDN (#1429, root-caused by @Mape6, also hit @TrickShotMLG02)** — Reporter on a flat 192.168.3.0/24 LAN had added a P1S to Bambuddy by its router-provided DNS name `p1s.fritz.box` instead of its IPv4. On 0.2.4+ that one detail kept Bambu Studio Send going to the real printer instead of the Bambuddy archive whenever the printer was powered on — exact same surface symptom #1429 was originally about, but a separate root cause from the bind-IP encoding work shipped on 2026-06-02. The defensive `NOT armed` logging ([[issue1429_vp_ip_leak]]) added in this release pinpointed it on the reporter's bundle: `MQTT bridge IP encoding NOT armed: invalid IPv4 (target='p1s.fritz.box', vp='192.168.3.27'): invalid literal for int() with base 10: 'p1s'`. The encoder `_ip_to_uint32_le` (and the host-interface picker `find_interface_for_ip`) both assume dotted-quad IPv4 and bail on anything else, so `BambuMQTTClient.ip_address` being the configured FQDN string short-circuited the rewrite path and `net.info[*].ip` kept leaking the real printer's IPv4. Switching the printer record to an IPv4 cleared the issue immediately for the reporter — that workaround confirms the diagnosis exactly. **Why this didn't bite pre-0.2.4**: the bridge didn't do `net.info[].ip` rewriting at all before #1429 shipped, so FQDN-configured printers worked by accident — nothing was trying to parse the host as IPv4. **Fix** adds `_resolve_target_to_ipv4(target)` in `mqtt_bridge.py`: pass-through when `target` already parses as `ipaddress.IPv4Address`, otherwise `socket.getaddrinfo(target, None, family=socket.AF_INET)` to filter to IPv4-only (the `net.info[*].ip` field is uint32 LE — there's no IPv6 representation that fits, so an AF_INET6 result must not slip through). Returns `None` on empty input *and* on `OSError` from getaddrinfo so transient DNS hiccups don't break the encoding permanently; `_refresh_ip_encoding` falls back to the existing `NOT armed` throttle which re-resolves on every 30s refresh tick (DHCP / DNS churn picks itself up). Both the `_ip_to_uint32_le(target_ip)` call AND the `_resolve_host_interface_for_target(target_ip)` call now receive the resolved IPv4, so the same fix covers the bind-address auto-resolve path used on default-config (0.0.0.0 bind) installs that don't have a dedicated VP bind IP. The configured FQDN is preserved into the armed log line as `configured→resolved` (`target=p1s.fritz.box→192.168.3.153`) so a bad-DNS regression stays legible in `docker logs` without grepping back to the not-armed lines. The unresolvable-input not-armed reason is now `could not resolve printer host '<input>' to IPv4 (invalid address and DNS lookup failed)` — names the actual configured value, not just `invalid IPv4 (target=...)`, so future bundles distinguish "DNS gave us a v6 address" from "user typed garbage" without a guess. **Tests**: 5 new in `TestHostnameResolution` (pass-through for IPv4, empty/None → None, FQDN → resolved IPv4 with AF_INET filter asserted, `OSError` → None, end-to-end FQDN-targeted bridge arms with the resolved IPv4 in `_target_ip_uint32_le` and the `configured→resolved` shape in the armed log). The existing `test_invalid_ipv4_logs_value_error` renamed to `test_unresolvable_target_logs_reason` and now patches `getaddrinfo` to `OSError` so the test is hermetic; asserts the new `could not resolve printer host 'not.an.ip'` message. 49 bridge tests pass; ruff clean.
|
||||
|
||||
- **MakerWorld URL imports into a writable external folder wrote bytes to internal storage, not the NAS (#1645, reported and root-caused by @needo37)** — Reporter linked a writable external SMB folder, selected it as the destination in the MakerWorld import dialog, the import succeeded, the file card appeared in the File Manager under the external folder's view — but `ls` on the NAS turned up nothing, and a `find` across the whole NAS and the container for the original filename matched nothing either. The bytes had landed in Bambuddy's internal `<DATA_DIR>/archive/library/files/<uuid>.3mf` instead of `<external_path>/<filename>` on the mount. Root cause was the byte-import save helper `save_3mf_bytes_to_library` at `backend/app/api/routes/library.py:422`: it accepts `folder_id` but never loaded the folder or inspected `is_external` / `external_path`, hardcoded the destination to `get_library_files_dir() / <uuid><ext>`, and left the `LibraryFile` row with `is_external=False`. So the row's `folder_id` pointed at the external folder while its bytes + `is_external` flag both said "managed/internal" — exact same class of bug as #1112 (which got fixed for the multipart-upload and move paths but never applied to the byte-import path). Compounded by the UUID-renamed on-disk copy: searching for the human-readable basename anywhere — NAS or container — never matches. **Fix** is a direct mirror of the multipart-upload path that's done this correctly since #1112: load the target `LibraryFolder` (when `folder_id` is non-None), feed it to the existing `_resolve_upload_destination(target_folder, filename)` helper which already produces `(dest, is_external)` and enforces the 403-read-only / 400-unwritable-or-missing / 409-collision rejections, write the bytes to that destination (real filename for external, UUID for managed), and persist the row via `_stored_file_path(dest, is_external)` + `is_external=is_external`. The route-layer read-only guard at `makerworld.py:256-260` is preserved — it returns the friendlier error before the upstream download burns bandwidth — and `_resolve_upload_destination`'s identical check stays as defence-in-depth for any future caller that skips the route gate. Thumbnails continue to live under the managed `get_library_thumbnails_dir()` regardless of the 3MF's location, matching the upload path. **Tests**: 4 new in `TestImport` (writable external → bytes on mount + `is_external=True` + absolute file_path persisted; read-only external → 403 at route, no download; missing external_path → 400; filename collision → 409 with the pre-existing file's bytes untouched). 21 existing makerworld tests + 72 library-route tests stay green. Ruff clean.
|
||||
|
||||
- **X2D archives lose 3MF metadata because FTPS handshake fails on firmware 01.01.00.00 (#1638, reported by @vasmarfas)** — Reporter's first archive entries from a brand-new X2D landed almost empty (only print time visible, no filament weight / layers / MakerWorld link / thumbnail), and Spoolman filament-usage tracking also went silent. The support bundle traces the symptom end-to-end: at print start `backend/app/main.py::on_print_start` tries the usual FTP-download dance for the 3MF, every connect attempt to the printer fails with `[SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1032)`, and ~2 minutes later `Could not find 3MF file for print: /data/Metadata/plate_1.gcode` → `Created fallback archive N for <name> (no 3MF available)`. The fallback path writes the row with `file_path=""`, `file_size=0`, `content_hash=NULL`, and no layers / filament / model-link fields — exactly the "almost empty card" in the reporter's screenshot. Spoolman tracking and reprint-grouping also degrade from the same root cause: both depend on metadata pulled out of the 3MF by `ThreeMFParser`. The proximate cause is the FTPS handshake: Python 3.13's default `ssl.create_default_context()` negotiates TLS 1.3, and the X2D's implicit-FTPS server on port 990 rejects the ClientHello with `WRONG_VERSION_NUMBER`. This is the same shape of symptom as the P2S 01.02.00.00 FTPS bug from #1401 — handshake / data-channel breakage triggered by the move to Python 3.13's TLS-1.3 default — but the wire-level failure mode is different (P2S completes the handshake and truncates mid-stream with 426; X2D fails the handshake outright). Both are addressed via the per-model registry that #1401 established: `backend/app/services/ftp_profiles.py` gains an `X2D` entry with `cap_tls_v1_2=True` plus a `N6 → X2D` SSDP alias, so the X2D's `ImplicitFTP_TLS` connection caps the SSL context's `maximum_version` to TLS 1.2 and the ClientHello looks like the one the firmware accepted before the Python upgrade. Deliberately conservative — every other model stays on negotiated TLS 1.3, only X2D-tagged sessions flip. **Honest caveat**: this ships as a hypothesis-driven trial rather than a confirmed root-cause fix. The TLS-1.2 cap is the most likely cure given the symptom's family resemblance to #1401, but `WRONG_VERSION_NUMBER` could equally describe the X2D switching to explicit FTPS (AUTH TLS on a plaintext greeting) or moving the FTPS service to a different port — both would need a different code path. The reporter has been asked to test this build; if the cap doesn't clear the error, the registry slot stays useful as a tuning anchor and the next round of diagnostics (`openssl s_client -connect <ip>:990 -tls1_2` from a network-adjacent host) will tell us which of (2)/(3) applies. **Tests**: 3 new in `test_ftp_profiles.py` mirroring the existing P2S coverage — `X2D` resolves to `cap_tls_v1_2=True`, `N6` SSDP code aliases to the X2D profile, lowercase `x2d` still hits the cap. Existing P2S + default + unknown-model + frozen-dataclass + non-capped-spot-check (X1C / H2D / P1S / A1) tests stay green. **Verified**: ruff clean; the integration test at `test_cap_tls_v1_2_actually_applied_to_ssl_context` already pins the profile→`ImplicitFTP_TLS`→`ssl_context.maximum_version` wiring so this entry can't silently fail to apply.
|
||||
|
||||
@@ -35,8 +35,10 @@ from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import copy
|
||||
import ipaddress
|
||||
import json
|
||||
import logging
|
||||
import socket
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -90,6 +92,38 @@ def _ip_to_uint32_le(ip_str: str) -> int:
|
||||
return parts[0] | (parts[1] << 8) | (parts[2] << 16) | (parts[3] << 24)
|
||||
|
||||
|
||||
def _resolve_target_to_ipv4(target: str) -> str | None:
|
||||
"""Return a dotted-quad IPv4 for `target`, resolving hostnames if needed.
|
||||
|
||||
The printer client may be configured by IPv4 *or* by hostname/FQDN
|
||||
(e.g. `p1s.fritz.box`) — the latter is common on home LANs with a
|
||||
DNS-providing router. The downstream `net.info[].ip` field is a
|
||||
32-bit little-endian integer though, so a hostname can't round-trip
|
||||
through it; we have to pick *one* concrete IPv4 to write in.
|
||||
|
||||
Returns None if `target` is empty, not parseable as IPv4, and DNS
|
||||
resolution fails — caller logs that as the not-armed reason and
|
||||
re-tries on the next refresh tick (DHCP/DNS churn picks itself up).
|
||||
"""
|
||||
if not target:
|
||||
return None
|
||||
try:
|
||||
return str(ipaddress.IPv4Address(target))
|
||||
except (ValueError, ipaddress.AddressValueError):
|
||||
pass
|
||||
try:
|
||||
# AF_INET filters to IPv4 only; the rewrite field is uint32 LE,
|
||||
# there's no IPv6 representation that fits.
|
||||
infos = socket.getaddrinfo(target, None, family=socket.AF_INET)
|
||||
except OSError:
|
||||
return None
|
||||
for info in infos:
|
||||
sockaddr = info[4]
|
||||
if sockaddr and isinstance(sockaddr[0], str):
|
||||
return sockaddr[0]
|
||||
return None
|
||||
|
||||
|
||||
def _resolve_host_interface_for_target(target_ip: str) -> str | None:
|
||||
"""Pick a host-side IPv4 for `net.info[].ip` when the VP has no dedicated bind IP.
|
||||
|
||||
@@ -411,11 +445,21 @@ class MQTTBridge:
|
||||
_log_not_armed("target_client is None (bridge not bound to a printer)")
|
||||
return
|
||||
|
||||
target_ip = getattr(client, "ip_address", None)
|
||||
if not target_ip:
|
||||
configured_target = getattr(client, "ip_address", None)
|
||||
if not configured_target:
|
||||
_log_not_armed("printer client has no ip_address yet")
|
||||
return
|
||||
|
||||
# Printers configured by hostname/FQDN (e.g. `p1s.fritz.box`) need to
|
||||
# be resolved to an IPv4 before encoding: net.info[*].ip is uint32 LE
|
||||
# and can't carry a hostname (#1429 follow-up).
|
||||
target_ip = _resolve_target_to_ipv4(configured_target)
|
||||
if not target_ip:
|
||||
_log_not_armed(
|
||||
f"could not resolve printer host {configured_target!r} to IPv4 (invalid address and DNS lookup failed)"
|
||||
)
|
||||
return
|
||||
|
||||
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
|
||||
@@ -446,11 +490,12 @@ class MQTTBridge:
|
||||
self._vp_ip_uint32_le = new_vp_le
|
||||
# Clear the dedup so a future failure re-emits the diagnostic line.
|
||||
self._not_armed_reason = None
|
||||
target_display = target_ip if target_ip == configured_target else f"{configured_target}→{target_ip}"
|
||||
logger.info(
|
||||
"[%s] MQTT bridge IP encoding %s: target=%s vp=%s (%s)",
|
||||
self.vp_name,
|
||||
"updated" if was_armed else "armed",
|
||||
target_ip,
|
||||
target_display,
|
||||
vp_ip,
|
||||
vp_ip_source,
|
||||
)
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
import socket
|
||||
from pathlib import Path
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
@@ -12,6 +13,7 @@ from backend.app.services.virtual_printer.mqtt_bridge import (
|
||||
MQTTBridge,
|
||||
_ip_to_uint32_le,
|
||||
_resolve_host_interface_for_target,
|
||||
_resolve_target_to_ipv4,
|
||||
)
|
||||
from backend.app.services.virtual_printer.mqtt_server import SimpleMQTTServer
|
||||
|
||||
@@ -1088,6 +1090,62 @@ class TestIpEncoding:
|
||||
_ip_to_uint32_le("not.an.ip.actually")
|
||||
|
||||
|
||||
class TestHostnameResolution:
|
||||
"""#1429 follow-up: users who configured the printer by FQDN (common on
|
||||
LANs with router-provided DNS like `p1s.fritz.box`) hit `invalid IPv4`
|
||||
on the encoder and the rewrite never armed — slicer kept FTPing direct
|
||||
to the real printer. The bridge now resolves hostname→IPv4 first."""
|
||||
|
||||
def test_pass_through_for_valid_ipv4(self):
|
||||
assert _resolve_target_to_ipv4("192.168.1.50") == "192.168.1.50"
|
||||
|
||||
def test_empty_returns_none(self):
|
||||
assert _resolve_target_to_ipv4("") is None
|
||||
assert _resolve_target_to_ipv4(None) is None # type: ignore[arg-type]
|
||||
|
||||
def test_hostname_resolves_via_getaddrinfo(self):
|
||||
with patch(
|
||||
"backend.app.services.virtual_printer.mqtt_bridge.socket.getaddrinfo",
|
||||
return_value=[(2, 1, 6, "", ("192.168.3.153", 0))],
|
||||
) as mock_gai:
|
||||
assert _resolve_target_to_ipv4("p1s.fritz.box") == "192.168.3.153"
|
||||
# AF_INET filter prevents an IPv6-only result from being picked,
|
||||
# since net.info[*].ip is a uint32 LE that can't carry v6.
|
||||
assert mock_gai.call_args.kwargs.get("family") == socket.AF_INET
|
||||
|
||||
def test_dns_failure_returns_none(self):
|
||||
with patch(
|
||||
"backend.app.services.virtual_printer.mqtt_bridge.socket.getaddrinfo",
|
||||
side_effect=OSError("Name or service not known"),
|
||||
):
|
||||
assert _resolve_target_to_ipv4("nope.invalid") is None
|
||||
|
||||
def test_fqdn_target_arms_encoding(self, caplog):
|
||||
"""End-to-end: a client whose `ip_address` is an FQDN should arm
|
||||
the bridge once DNS resolves, and the cached rewrite uses the
|
||||
resolved IPv4 (not the hostname string) for the `net.info[].ip`
|
||||
encoding."""
|
||||
server = _make_server(bind_address=VP_IP)
|
||||
bridge = _make_bridge(server)
|
||||
client = _make_paho_client(ip="p1s.fritz.box")
|
||||
bridge._target_client = client
|
||||
with (
|
||||
patch(
|
||||
"backend.app.services.virtual_printer.mqtt_bridge.socket.getaddrinfo",
|
||||
return_value=[(2, 1, 6, "", (H2D_IP, 0))],
|
||||
),
|
||||
caplog.at_level(logging.INFO, logger="backend.app.services.virtual_printer.mqtt_bridge"),
|
||||
):
|
||||
bridge._refresh_ip_encoding()
|
||||
assert bridge._target_ip_uint32_le == _ip_to_uint32_le(H2D_IP)
|
||||
assert bridge._vp_ip_uint32_le == _ip_to_uint32_le(VP_IP)
|
||||
armed = [r for r in caplog.records if "MQTT bridge IP encoding armed" in r.getMessage()]
|
||||
assert len(armed) == 1
|
||||
# Operator should see configured→resolved in the log line so a
|
||||
# bad-DNS regression is immediately legible.
|
||||
assert "p1s.fritz.box→192.168.255.133" in armed[0].getMessage()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Auto-resolve fallback for default-config (bind_address = "0.0.0.0")
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -1236,17 +1294,26 @@ class TestNotArmedDiagnosticLogging:
|
||||
assert H2D_IP in msg
|
||||
assert "no host interface" in msg
|
||||
|
||||
def test_invalid_ipv4_logs_value_error(self, caplog):
|
||||
def test_unresolvable_target_logs_reason(self, caplog):
|
||||
"""When `ip_address` isn't a valid IPv4 *and* doesn't resolve via DNS,
|
||||
the bridge must report a single concrete not-armed reason naming the
|
||||
configured value — operator can then see exactly what input failed."""
|
||||
server = _make_server(bind_address=VP_IP)
|
||||
bridge = _make_bridge(server)
|
||||
client = _make_paho_client()
|
||||
client.ip_address = "not.an.ip"
|
||||
bridge._target_client = client
|
||||
with caplog.at_level(logging.INFO, logger="backend.app.services.virtual_printer.mqtt_bridge"):
|
||||
with (
|
||||
patch(
|
||||
"backend.app.services.virtual_printer.mqtt_bridge.socket.getaddrinfo",
|
||||
side_effect=OSError("nodename nor servname provided"),
|
||||
),
|
||||
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 "invalid IPv4" in not_armed[0].getMessage()
|
||||
assert "could not resolve printer host 'not.an.ip'" in not_armed[0].getMessage()
|
||||
|
||||
def test_successful_arm_clears_dedup_so_future_failure_relogs(self, caplog):
|
||||
"""After a successful arm, the dedup must reset so a subsequent
|
||||
|
||||
Reference in New Issue
Block a user