Document UDP port-forward support
CLAUDE.md's port_forward.{h,cpp} entry, README.md's -p table row, and
docs/networking-design.md's syntax line all updated for the new
[/tcp|udp] suffix. Includes the local dev-machine (root, via the scoped
doas rule) verification detail: TCP unaffected, UDP confirmed end-to-end
(a raw datagram sent to the forwarded host port was read back inside the
container via -x/--exec), same port pair coexisting on both protocols,
invalid-protocol parse errors, clean teardown, and --clean-processes
sweeping both the old 3-field and new 4-field state-file formats. Real
Android iptables/tetherctrl_FORWARD confirmation for UDP is still open.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -1040,38 +1040,54 @@ Source layout (all under `src/`):
|
||||
earlier retry was compensating for a problem this fix removes outright,
|
||||
not one it makes more likely to need retrying.
|
||||
- `port_forward.{h,cpp}` — implements `-p`. `parse_port_forward_spec()`
|
||||
splits `"[<network>:]<host-port>:<container-port>"` on `':'` (2 or 3
|
||||
fields; the network name is deliberately restricted to excluding `':'` --
|
||||
`is_valid_network_name()`, `network_subnet.h` -- specifically so this
|
||||
split stays unambiguous) and validates both ports are `1..65535`
|
||||
splits `"[<network>:]<host-port>:<container-port>[/tcp|udp]"` on `':'` (2
|
||||
or 3 fields; the network name is deliberately restricted to excluding
|
||||
`':'` -- `is_valid_network_name()`, `network_subnet.h` -- specifically so
|
||||
this split stays unambiguous) and validates both ports are `1..65535`
|
||||
(`.cpp`-local `parse_port()`) -- pure syntax/range parsing, no knowledge of
|
||||
which networks exist or joined; that's `add_port_forward()`'s job, called
|
||||
later once `join_networks()` (`network_join.h`) has actually run.
|
||||
later once `join_networks()` (`network_join.h`) has actually run. The
|
||||
optional `/tcp`/`/udp` suffix (`PortForwardProtocol`, `port_forward.h` --
|
||||
plain `enum class`, no prefix, same convention as `NetworkKind`) is
|
||||
stripped off the trailing container-port field *before* `parse_port()`
|
||||
ever sees it (`container_port_str` is an owned copy for exactly this,
|
||||
unlike `host_port_str`, which stays an alias since it never carries a
|
||||
suffix) -- an unrecognized value is a hard parse error (exact lowercase
|
||||
match only, `"tcp"`/`"udp"`, same case-sensitivity precedent as
|
||||
`apply_log_level()`'s `valid_levels` check, not `config_file.cpp`'s
|
||||
case-insensitive YAML parsing, a different context); omitted defaults to
|
||||
`tcp`, so every pre-existing `-p` spec keeps working unchanged. Both
|
||||
`PortForwardSpec` and `ActivePortForward` carry the resolved
|
||||
`protocol` field, and a `.cpp`-local `iptables_proto()` converts it to the
|
||||
exact string `iptables`'s own `-p` flag expects.
|
||||
`add_port_forward()` resolves `spec.network` against the `JoinedNetwork`
|
||||
list -- by name if given (erroring if that network wasn't successfully
|
||||
joined, or isn't `extern`: an `intern` network's bridge has no path from
|
||||
the host at all, so forwarding into one could never work), or, if unset,
|
||||
the container's sole joined `extern` network (erroring if none or more
|
||||
than one, rather than guessing). Then adds one iptables `DNAT` rule to
|
||||
**both** `nat PREROUTING` *and* `nat OUTPUT` -- a real bug caught by
|
||||
than one, rather than guessing). Then adds one iptables `DNAT` rule,
|
||||
matching `spec.protocol` (`-p tcp` or `-p udp`), to **both**
|
||||
`nat PREROUTING` *and* `nat OUTPUT` -- a real bug caught by
|
||||
testing, not assumed: `PREROUTING`-only left `curl <this host's own real
|
||||
IP>:<host-port>`, run *on this same host*, connection-refused, since
|
||||
`PREROUTING` only ever sees packets arriving from an actual network
|
||||
interface, never locally-generated ones (those go through `OUTPUT`
|
||||
instead) -- the same split Docker's own DNAT setup already accounts for.
|
||||
Also adds one `FORWARD ACCEPT` rule for the destination (in case of a
|
||||
default `FORWARD DROP` policy, which would otherwise silently eat the
|
||||
forwarded traffic even though the `DNAT` itself succeeded); if a later
|
||||
rule fails after an earlier one already landed, those are removed again so
|
||||
a failure doesn't leave a half-applied mapping. **Known limitation, not
|
||||
solved here, also found by testing**: `curl localhost:<host-port>` (or any
|
||||
`127.0.0.0/8` destination) specifically still doesn't work even with both
|
||||
`DNAT` chains covered -- confirmed to be a separate problem, NAT
|
||||
hairpinning: once `DNAT` rewrites the destination to the container's IP,
|
||||
the packet still carries its *original* source address (`127.0.0.1`); the
|
||||
container's own kernel sees an inbound packet claiming to be *from*
|
||||
loopback arriving on a non-loopback interface (`eth<N>`) and drops it as a
|
||||
martian source. (A `net.ipv4.conf.{all,lo}.route_localnet=1` sysctl was
|
||||
Also adds one `FORWARD ACCEPT` rule for the destination, matching the same
|
||||
protocol (in case of a default `FORWARD DROP` policy, which would
|
||||
otherwise silently eat the forwarded traffic even though the `DNAT` itself
|
||||
succeeded); if a later rule fails after an earlier one already landed,
|
||||
those are removed again so a failure doesn't leave a half-applied mapping.
|
||||
**Known limitation, not solved here, also found by testing**: `curl
|
||||
localhost:<host-port>` (or any `127.0.0.0/8` destination) specifically
|
||||
still doesn't work even with both `DNAT` chains covered -- confirmed to be
|
||||
a separate problem, NAT hairpinning, and equally true for UDP (the martian-
|
||||
source check is at the IP layer, not the TCP state machine): once `DNAT`
|
||||
rewrites the destination to the container's IP, the packet still carries
|
||||
its *original* source address (`127.0.0.1`); the container's own kernel
|
||||
sees an inbound packet claiming to be *from* loopback arriving on a
|
||||
non-loopback interface (`eth<N>`) and drops it as a martian source. (A
|
||||
`net.ipv4.conf.{all,lo}.route_localnet=1` sysctl was
|
||||
tried and confirmed *not* to fix this on its own, then removed again
|
||||
rather than left in as dead/superstitious code.) A full fix needs source
|
||||
masquerading scoped to exactly this case (matching only host-local
|
||||
@@ -1089,6 +1105,43 @@ Source layout (all under `src/`):
|
||||
reachable via `curl <host's real IP>:8080` from the host; the rule was
|
||||
confirmed gone (connection refused) after the session was killed.
|
||||
|
||||
**UDP support added.** `-p <host-port>:<container-port>/udp` produces the
|
||||
exact same three-rule shape (2 DNAT + 1 FORWARD ACCEPT) with `-p udp`
|
||||
instead of `-p tcp` throughout -- no other argv-shape change needed, since
|
||||
`iptables`'s own `-p`/`--dport`/`-j DNAT --to-destination` and `-d
|
||||
... --dport ... -j ACCEPT` forms are identical between the two protocol
|
||||
modules. No dedup/coexistence tracking exists in `add_port_forward()` (it
|
||||
unconditionally issues `iptables -A` on every call), so the same port
|
||||
pair can be forwarded once per protocol without collision -- e.g.
|
||||
`-p 53:53/tcp -p 53:53/udp` for a DNS-like service produces two textually
|
||||
distinct rule sets that add/remove independently. **Verified end-to-end on
|
||||
this dev machine (root, via the scoped `doas` rule)**: on a freshly
|
||||
created `extern` network, `-p 18080:80/tcp -p 18081:80/udp` against one
|
||||
busybox container running both `httpd` (TCP) and `nc -u -l -p 80 >
|
||||
/tmp/udp-received.txt` (UDP) simultaneously -- `curl <host's real IP>
|
||||
:18080` got the expected TCP response, and a raw UDP datagram sent to
|
||||
`<host's real IP>:18081` (via a small Python `socket.SOCK_DGRAM` script,
|
||||
no `nc` available on this dev host) was confirmed to have actually reached
|
||||
the container by `-x/--exec`ing in and `cat`ing the received-data file
|
||||
afterward. Session teardown removed exactly the four rules that were
|
||||
added (confirmed via the debug log's matching `-D` lines for both
|
||||
protocols). An invalid protocol suffix (`-p 8080:80/xyz`) errored cleanly
|
||||
with the new parse-time message, before touching iptables or even
|
||||
attempting the mount/run, and cleaned up the layer import it had already
|
||||
done. `--clean-processes` against hand-planted stale port-forward records
|
||||
(a rootless test, same shape as the original crash-orphan sweep test
|
||||
below -- root isn't needed for the sweep *logic*, only the underlying
|
||||
`iptables -D` calls) correctly swept both an old-format 3-field record
|
||||
(defaulting to `tcp`, per the parsing note above) and a new-format
|
||||
4-field `udp` record, each attempting the right `-p tcp`/`-p udp` removal
|
||||
and reporting `removed stale port-forward rules for '<name>'`, while
|
||||
leaving a third record matching a still-running session completely
|
||||
untouched. **Still to verify**: real-device confirmation that UDP DNAT
|
||||
behaves the same way through Android's iptables/`tetherctrl_FORWARD`
|
||||
chain as TCP already does (assumed protocol-agnostic by the rule
|
||||
mechanics, not yet proven there) -- see `TODO.md`/this file's own status
|
||||
tracking for whether that's landed yet.
|
||||
|
||||
**Crash-orphan sweep** (commit 6 of `docs/networking-design.md`'s
|
||||
sequence): unlike `join_networks()`'s veths (torn down automatically by
|
||||
the kernel once the session's namespace goes away) or the bridges/
|
||||
@@ -1106,14 +1159,24 @@ Source layout (all under `src/`):
|
||||
directory's filenames directly against `list_sessions()`'s own
|
||||
`SessionInfo::path` to reuse its liveness check, rather than re-deriving
|
||||
pid liveness a second, drifting way. `record_port_forwards()` writes one
|
||||
line per mapping (`"<host_port> <container_ip> <container_port>"`) to that
|
||||
path — a no-op if there's nothing to record. `clean_stale_port_forwards()`
|
||||
line per mapping (`"<host_port> <container_ip> <container_port> <proto>"`,
|
||||
`<proto>` the same `"tcp"`/`"udp"` string `iptables_proto()` produces) to
|
||||
that path — a no-op if there's nothing to record. `clean_stale_port_forwards()`
|
||||
(`commands.cpp`'s `clean_processes_command()`, alongside
|
||||
`clean_stale_sessions()`) scans that directory: a record whose filename
|
||||
doesn't match any currently-*running* session is stale — every line is
|
||||
parsed back into an `ActivePortForward` and removed
|
||||
(`remove_port_forward()`) before the record file itself is deleted; a
|
||||
record whose session is still running is left completely untouched.
|
||||
**Line parsing is deliberately per-line (`std::getline` + `std::istringstream`),
|
||||
not one big `while (in >> a >> b >> c >> proto)`**: chaining a 4th `>>`
|
||||
directly would fail-and-short-circuit the whole `while` condition on an
|
||||
older, pre-UDP-support 3-field line *before* the loop body (the actual
|
||||
removal) ever ran for it, silently leaking that rule forever with no
|
||||
error logged. The per-line parse instead reads the first three fields,
|
||||
skips the line entirely only if *those* fail, and otherwise defaults an
|
||||
absent or unrecognized 4th token to `tcp` — same forward-compatible
|
||||
posture as `config_file.cpp`'s "unknown keys ignored" policy.
|
||||
**Verified via a controlled scratch test** (root wasn't needed for the
|
||||
logic itself — only the underlying `iptables -D` calls, already proven
|
||||
working as root above; killing a root-owned `slocker-lite` process
|
||||
|
||||
Reference in New Issue
Block a user