podman run refuses two --publish mappings that claim one host address and port - #1033
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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: 9a4cc8194a
ℹ️ 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".
… and port
fcvm podman run --publish 8080:80,8080:443 was accepted. Routed and rootless
networking then failed the second bind after the VM's network existed, and
bridged networking installed both DNAT rules and delivered only the first, so
guest port 443 was unreachable with no error. snapshot run --publish already
refuses such a list (PortMapping::require_distinct_host_sockets).
podman run now parses its --publish specs in one place, publish_mappings,
before anything else reads them, and applies the same check there. The check
that already ran at that point, a published guest port that
--forward-localhost also claims, moves into the function and uses the parsed
mappings, where it parsed each spec again and skipped the ones that did not
parse. A malformed spec now fails the run before the kernel, the rootfs and
the image are looked up. It was already refused later on both paths: by the
launch path on a cache miss, and on a cache hit by the restore, which parses
the run's specs strictly.
Red, with the check removed from publish_mappings:
make _test-unit FILTER="-p fcvm --lib -E 'test(/^commands::podman::tests::publish_/)'"
Summary [ 5.017s] 1 test run: 0 passed, 1 failed, 731 skipped
commands::podman::tests::publish_specs_are_parsed_strictly_for_a_run
called `Result::unwrap_err()` on an `Ok` value: [PortMapping { host_ip: None, host_port: 8080, guest_port: 80, proto: Tcp }, PortMapping { host_ip: None, host_port: 8080, guest_port: 443, proto: Tcp }]
Green on this commit:
the same command
Summary [ 0.011s] 1 test run: 1 passed, 731 skipped
make test-unit
Summary [ 71.285s] 1376 tests run: 1376 passed (1 slow), 0 skipped
make lint rc 0
make _test-root FILTER=--no-run rc 0 (compiles the root-only tests, runs none)
dee5355 to
db197db
Compare
9a4cc81 to
cd19033
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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: network::bridged::tests::a_host_address_and_none_on_one_host_port_are_refused_before_any_setup
This answers the finding of the Codex review of 9a4cc81: under bridged networking a mapping with no host address and one with an explicit IPv4 address on the same port pass the duplicate check, and the first rule shadows the second. The reply on its thread has the result without the fix: with that test added and nothing else, setup() goes on to create its namespace. The refusal is in bridged setup, in 9ad4b7e under this PR, so podman run and snapshot run --publish both get it. Codex's review of the head, cd19033, found no major issue.
1 participant
Stacked on:
forward-localhost-any-host-loopback(PR #1032).fcvm podman run --publish 8080:80,8080:443was accepted. Routed and rootless networking then failed the second bind after the VM's network existed, and bridged networking installed both DNAT rules and delivered only the first.snapshot run --publishrefuses such a list since #1031.What changes
podman runparses its--publishspecs in one place, before anything else reads them, and refuses two mappings that claim one host address and port. The check for a published guest port that--forward-localhostalso claims moves into the same function. Two mappings that differ only in HOSTIP are two sockets for routed and rootless networking and stay allowed here. For bridged networking they are one, and its setup refuses them since #1031. A malformed spec now fails the run before the kernel, the rootfs and the image are looked up; it was already refused later on both the cache-hit and the cache-miss path.Contract and impact
Production code at the start of
podman run. A valid--publishlist behaves as before.Minimum evidence
A unit test with a red, the unit suite and lint.
Evidence
podman run refuses two --publish mappings that claim one host address and port