Repository navigation
--ipv6-prefix is also read from FCVM_IPV6_PREFIX, and only a routed run's snapshot key includes it - #1029
Conversation
Routed networking needs a /64 it can give VMs addresses from. On a host whose
own addresses are not one (a /112 and a /128 on the hosts this was written
for), every routed run needs --ipv6-prefix with the host's delegated subnet.
The routed tests pass no prefix, so on such a host each one stopped before it
started a VM:
Error: routed mode preflight check failed: routed networking requires a host with a global IPv6 address.
The host needs a non-deprecated /64 (or a /128 with a /64 on-link route).
Use --ipv6-prefix to specify a routable /64 prefix explicitly.
podman run and podman prepare now take the prefix from FCVM_IPV6_PREFIX when
the flag is absent, so a test run sets it once.
Red, with the binding removed and the test kept:
make _test-unit FILTER="-p fcvm --lib -E 'test(/^cli::args::tests::|^test_env::/)'"
Summary [ 5.019s] 9 tests run: 8 passed, 1 failed, 701 skipped
cli::args::tests::ipv6_prefix_is_bound_to_an_environment_variable
assertion `left == right` failed: podman run
left: None
right: Some("FCVM_IPV6_PREFIX")
Green on this commit:
the same command
Summary [ 0.016s] 9 tests run: 9 passed, 701 skipped
make test-unit
Summary [ 71.289s] 1349 tests run: 1349 passed (1 slow), 0 skipped
make lint rc 0
The routed VM tests as root on such a host, on this commit:
without the variable
make _test-root FILTER="-E 'test(/test_forward_localhost_routed/)'"
Summary [ 5.194s] 1 test run: 0 passed, 1 failed, 1690 skipped
with FCVM_IPV6_PREFIX set to the host's delegated subnet
make test-root FILTER="-E 'test(/test_port_forward_routed|test_clone_port_forward_routed|test_forward_localhost_routed/)'"
Summary [ 49.215s] 3 tests run: 3 passed, 1688 skipped
PASS [ 8.493s] test_forward_localhost_routed
PASS [ 11.150s] test_port_forward_routed
PASS [ 29.564s] test_clone_port_forward_routed
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. |
|
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 (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CLI accepts ChangesIPv6 prefix handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Merge Risk: 🔵 Low · up to A launch using an older non-routed snapshot keyed with an explicit IPv6 prefix may cold-boot and leave a second cache entry. The miss falls back safely, so the impact is bounded. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Existing permission and prefix-validation checks remain intact. No introduced vulnerability was identified, but how privileged launchers control inherited environment settings is not established. 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 |
![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: 58f16d9478
ℹ️ 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".
--ipv6-prefix is copied into the launch configuration, and the snapshot key
is computed from that configuration. Reading the flag from FCVM_IPV6_PREFIX
put a prefix on every run made with the variable exported, so a rootless or
bridged run, which never reads the prefix, got a second snapshot key and
missed the snapshot it already had.
The launch configuration now carries the prefix for routed networking only
(launch_ipv6_prefix in src/commands/podman/mod.rs, used by
build_firecracker_config and build_launch_config). A routed run's key still
changes with the prefix, because its guest's addresses come from it.
Red, with the test added and both builders still copying the flag:
make _test-unit FILTER="-p fcvm --lib -E 'test(/^commands::podman::tests::/)'"
Summary [ 5.037s] 41 tests run: 40 passed, 1 failed, 670 skipped
commands::podman::tests::ipv6_prefix_changes_the_snapshot_key_of_a_routed_run_only
assertion `left == right` failed: Rootless never reads the prefix
left: "a65206085631"
right: "be8f64dbf37d"
Green on this commit:
the same command
Summary [ 0.136s] 41 tests run: 41 passed, 670 skipped
make test-unit
Summary [ 71.256s] 1350 tests run: 1350 passed (1 slow), 0 skipped
make lint rc 0
ejc3
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: commands::podman::tests::ipv6_prefix_changes_the_snapshot_key_of_a_routed_run_only
The one finding on this PR, the second snapshot key a rootless or bridged run got with FCVM_IPV6_PREFIX exported, is fixed in f255208. With the test added and both configuration builders still copying the flag it fails (41 tests run: 40 passed, 1 failed, "Rootless never reads the prefix"), and it passes on the head.
NOT-A-DEFECT for the rest of the review summaries. CodeRabbit's summary of f255208 has no actionable comment. Its note that a rootless or bridged run which passed --ipv6-prefix misses its old snapshot once is the behaviour change the description states. Its proposal about a privileged wrapper that takes environment from a less-trusted caller describes no code in this repository: the variable is read by the same process that takes the flag, from its caller's own environment.
1 participant
Routed networking needs a routable IPv6 prefix. Without
--ipv6-prefixfcvm looks for a non-deprecated /64 on the host, or a /128 with an on-link /64 route. On a host whose addresses are neither, every routed run needs the flag with the host's delegated subnet, and the routed tests pass no prefix: there all of them stop at "routed mode preflight check failed" before a VM boots.What changes
--ipv6-prefixonpodman runandpodman prepareis also read fromFCVM_IPV6_PREFIX, as theFCVM_UFFD_*flags are from theirs. A clone takes its prefix from its snapshot, as before.The variable puts a prefix on every run, and only routed networking reads it. The launch configuration, which the snapshot key is computed from, now carries the prefix for a routed run only (second commit), so a rootless or bridged run has the same key with the variable exported as without it.
Contract and impact
One clap attribute on a production flag, and the prefix left out of the launch configuration of the two network modes that never read it. With the variable unset a routed run is unchanged. A rootless or bridged run that passed
--ipv6-prefixused to get a key of its own, and now shares the key of the same run without the flag.Minimum evidence
A unit test with a red for each commit, the unit suite and lint, and the routed VM tests on a host that could not run them before, with and without the variable.
The tests and the documentation lines sit away from the lines the open published-port branches edit, so this merges with them in either order (
git merge-treeboth ways against the top of that stack, no conflict).Evidence
--ipv6-prefix is also read from FCVM_IPV6_PREFIX
FCVM_IPV6_PREFIX changes the snapshot key of a routed run only