snapshot run --publish lets a clone choose where its published ports listen - #1031
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSnapshot clones now accept optional publish mappings. The selected mappings are validated and carried through clone setup, state, and reboot paths. Bridged setup also rejects mappings that share a host port and protocol, regardless of host IP. ChangesSnapshot clone port mappings
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CLI
participant SnapshotClone
participant BridgedNetwork
participant VMState
CLI->>SnapshotClone: Pass --publish mappings
SnapshotClone->>SnapshotClone: Validate guest ports and protocols
SnapshotClone->>BridgedNetwork: Set up selected mappings
BridgedNetwork->>BridgedNetwork: Check host port and protocol conflicts
SnapshotClone->>VMState: Record selected mappings
Merge Risk: ⚪ Minimal · up to The change adds clone publish overrides while preserving inherited mappings by default. No actionable merge-blocking issue is established; normal checks remain appropriate, including bridged VM validation. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Clones can deliberately expose previously published services on different host addresses, including addresses reachable off-host. Guest-port restrictions and consistent restore propagation constrain that capability. No introduced security defect was established, but deployment-specific exposure and failure recovery were not demonstrated at runtime. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93388d5afc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…listen
A clone took its port mappings from the snapshot and nothing else, so one
snapshot could not be restored both as the VM that owns the host's ports
(--publish [::]:80:80 with routed networking) and as an ordinary clone on its
per-VM address.
fcvm snapshot run takes --publish in the grammar of podman run. When given,
the clone uses exactly those mappings; when absent it inherits the snapshot's,
host address included. Each mapping's guest port and protocol must be one the
snapshot published, because a guest restored from memory set its published
ports up when it booted (fc-agent's DNAT to loopback) and the restore does not
run that again. The rule is the same for a disk-only clone and for UDP, which
would not need it. Anything else is refused before a VM id, state file, or
network exists, with the snapshot's guest ports in the error, and so are two
mappings that claim one host address and port
(PortMapping::require_distinct_host_sockets). The choice is clone_port_mappings
in src/commands/snapshot.rs.
The clone's state records the mappings it uses, and a disk-only clone sets its
network up from the same list, written back as --publish specs with their host
address (PortMapping's Display). No snapshot key includes the flag. A bridged
clone listens on its veth address whatever HOSTIP says, as podman run does. A
snapshot taken of a clone records that clone's mappings.
Bridged setup replaces every mapping's host address with the VM's veth address,
so two mappings that differ only in HOSTIP became two DNAT rules for one
destination: the first took all the traffic and the second guest port was
unreachable, with no error. BridgedNetwork::setup now refuses two mappings
with the same host port and protocol before it creates anything, next to its
refusal of an IPv6 HOSTIP, and names both mappings as they were given. podman
run and snapshot run --publish both reach it. The search for the first two
mappings on one socket is PortMapping::first_on_one_socket, which both checks
use, and README.md states the rule beside the other bridged HOSTIP rules.
podman run restoring from its snapshot cache builds its SnapshotRunArgs in one
place (snapshot_restore_args) and passes its own --publish, so the clone
listens where that run asked.
Red. Each test failed with the behaviour it pins put back and the test kept
(no flag on snapshot run, the clone's mappings always the snapshot's, RunArgs
written from the snapshot's mappings and not the clone's, no publish on the
cache restore, no check for one host socket claimed twice), and passes on this
commit:
cli::args::tests::snapshot_run_publish_takes_the_podman_run_grammar
CLI should parse: UnknownArgument "--publish"
commands::snapshot::tests::a_clone_inherits_the_snapshots_mappings_or_uses_exactly_the_requested_ones
left: the snapshot's three mappings, where the three requested ones were expected
commands::snapshot::tests::a_clone_cannot_publish_a_guest_port_the_snapshot_did_not
must be refused: the snapshot's mappings came back instead of an error
must be refused: [8080 to guest 80, 8080 to guest 443] (one host socket twice)
commands::snapshot::tests::run_args_from_metadata_carries_boot_plan_fields
left: [] right: ["[::]:80:80/tcp", "127.0.0.1:5300:53/udp"]
commands::podman::tests::a_cache_restore_publishes_where_this_run_asked
left: [] where the run's two specs were expected
network::types::tests::two_mappings_cannot_claim_one_host_socket
one host socket is claimed twice: ()
Red for the bridged refusal, with only its two tests added (run with
FCVM_DATA_DIR on a scratch directory, because the unfixed setup() takes
bridged-subnet.lock before it fails for a user who is not root):
make _test-unit FILTER="-p fcvm --lib -E 'test(/^network::bridged::tests::/)'"
Summary [ 5.023s] 10 tests run: 8 passed, 2 failed, 726 skipped
network::bridged::tests::two_host_addresses_on_one_host_port_are_refused_before_any_setup
network::bridged::tests::a_host_address_and_none_on_one_host_port_are_refused_before_any_setup
both: reserving per-VM network names: creating network namespace fcvm-vm-one-po: failed to create namespace
The third test is a control (another host port, or another protocol on the
same port, is not refused). Its red is the check with the host port dropped
from its comparison:
network::bridged::tests::mappings_on_different_host_ports_or_protocols_are_not_refused
["8080:80", "8081:80"]: port mappings 8080:80/tcp and 8081:80/tcp claim the same host port
Red at the VM level, with clone_port_mappings returning the snapshot's
mappings whatever was requested (as root, set up as in the block below):
make _test-root FILTER="-E 'test(/test_clone_port_forward_routed/)'"
Summary [ 176.378s] 1 test run: 0 passed, 1 failed, 1716 skipped
test_clone_port_forward_routed
Access via 127.221.87.200:25520: FAIL (curl: (7) Failed to connect to 127.221.87.200 port 25520: Connection refused)
Error: snapshot run with an unpublished guest port did not exit within 60 s
Green on this commit:
make test-unit
Summary [ 71.296s] 1371 tests run: 1371 passed (1 slow), 0 skipped
make lint rc 0
make _test-root FILTER=--no-run rc 0 (compiles the root-only tests, runs none)
As root on the build host, at the top of this stack with the commit that reads
--ipv6-prefix from FCVM_IPV6_PREFIX picked on top (the host's own addresses are
not a routable /64, so the routed tests take its delegated subnet from that
variable):
make test-root FILTER="-E 'test(/test_port_forward_routed|test_clone_port_forward_routed|test_forward_localhost_routed/)'"
Summary [ 58.010s] 3 tests run: 3 passed, 1719 skipped
PASS [ 9.0s] fcvm::test_forward_localhost test_forward_localhost_routed
PASS [ 11.5s] fcvm::test_port_forward test_port_forward_routed
PASS [ 37.5s] fcvm::test_snapshot_clone test_clone_port_forward_routed
test_clone_port_forward_routed restores a second clone with --publish on a
127/8 address of its own, checks that it answers there and that its state
records that mapping and no loopback address, and checks that --publish
18081:81 is refused. The refused run is bounded at 60 s and dies with the
test, so a clone that started instead would fail the test, not hang it.
4472878 to
482c48c
Compare
93388d5 to
9ad4b7e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ad4b7ee6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ejc3
left a comment
ejc3
left a comment
There was a problem hiding this comment.
RED-VERIFIED: network::bridged::tests::two_host_addresses_on_one_host_port_are_refused_before_any_setup
This answers the finding of the Codex review of 93388d5, that two host addresses on one host port pass the duplicate check and collapse onto one veth address under bridged networking. The reply on its thread has the output without the fix: with that test and a_host_address_and_none_on_one_host_port_are_refused_before_any_setup added and nothing else, 10 tests run: 8 passed, 2 failed. In 9ad4b7e BridgedNetwork::setup refuses such a pair before it creates anything.
ejc3
left a comment
ejc3
left a comment
There was a problem hiding this comment.
DISAGREE: the finding of the Codex review of 9ad4b7e, a wildcard address beside a specific one on the same port, does not block this PR. The refusal this PR promises is for two mappings on one host address and port, and it holds. Which pairs of different addresses collide depends on the network mode, and a run with such a pair fails at the second bind with the address in the error and no listener left behind (a_bind_failure_names_the_address_and_leaves_no_listener_behind). One mode-aware rule for podman run, snapshot run, and bridged setup, with its test matrix, is tracked in #1039. The reply on the thread says the same.
1 participant
Stacked on:
routed-publish-host-address(PR #1030).A clone took its port mappings from the snapshot and nothing else. With routed networking honouring a mapping's host address (#1030), one snapshot could not be restored both as the VM that owns the host's ports (
--publish '[::]:80:80') and as an ordinary clone on its per-VM address.What changes
fcvm snapshot run --publishtakes the grammar ofpodman run --publish. Given, the clone uses exactly those mappings. Absent, it inherits the snapshot's, host address included.podman runandsnapshot run --publishboth reach the refusal.podman runrestoring from its snapshot cache passes its own--publish, so the clone listens where that run asked.Contract and impact
Production code on the restore path (
snapshot run, andpodman runon a cache hit). Without--publisha clone behaves as before. No snapshot key includes the flag. A bridged VM given two mappings on one host port now fails at network setup with an error that names both, where it used to start with one of them unreachable.Minimum evidence
Unit tests with a red each, the unit suite and lint, and
test_clone_port_forward_routedon a VM with a red.Not in this PR
Evidence
snapshot run --publish lets a clone choose where its published ports listen
Summary by CodeRabbit
New Features
--publishto replace inherited port mappings, provided each guest port and protocol was published by the snapshot. Without an override, mappings are inherited.HOSTIP.Bug Fixes