Exposed ports are only included when not --net=host#24164
Merged
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom Oct 4, 2024
Merged
Exposed ports are only included when not --net=host#24164openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Member
|
code lgtm |
Luap99
reviewed
Oct 4, 2024
libpod/container_inspect.go
Outdated
Member
There was a problem hiding this comment.
typo
but personally I would have a slight preference for if c.config.NetNsCtr == "" && c.NetworkMode() != "host" as I think this is clearer but it should be logically the same
bb2c24c to
5fed0ae
Compare
|
Cockpit tests failed for commit bb2c24c212a53fabd841c8e06838a22842f6b329. @martinpitt, @jelly, @mvollmer please check. |
Undoing some of my own work here from containers#24090 now that we have the ExposedPorts field implemented in inspect. I considered a revert of that patch, but it's still needed as without it we'd be including exposed ports when --net=container which is not correct. Basically, exposed ports for a container should always go in the new ExposedPorts field we added. They sometimes go in the Ports field in NetworkSettings, but only when the container is not net=host and not net=container. We were always including exposed ports, which was not correct, but is an easy logical fix. Also required is a test change to correct the expected behavior as we were testing for incorrect behavior. Fixes https://issues.redhat.com/browse/RHEL-60382 Signed-off-by: Matt Heon <mheon@redhat.com>
5fed0ae to
8061553
Compare
Member
Author
|
Tests green |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
TomSweeneyRedHat
added a commit
to TomSweeneyRedHat/podman
that referenced
this pull request
Oct 28, 2024
This fixes an exposed ports issue in RHEL 4.9-rhel for RHEL 9.5. This includes the fixes from the following PRs: First PR: containers#24090 Second PR: containers#24110 Third PR: containers#24164 With an additional tweak from @Luap99 in containers#24333 regarding the looping in libpod/container_inspect.go. This changes is needed in the 5.2-rhel branch to assure successful upgrades as the same patches have been used for the following issues in the Podman v4.9-rhel branch Fixes: https://issues.redhat.com/browse/ACCELFIX-299 Fixes: https://issues.redhat.com/browse/ACCELFIX-300 Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Undoing some of my own work here from #24090 now that we have the ExposedPorts field implemented in inspect. I considered a revert of that patch, but it's still needed as without it we'd be including exposed ports when --net=container which is not correct.
Basically, exposed ports for a container should always go in the new ExposedPorts field we added. They sometimes go in the Ports field in NetworkSettings, but only when the container is not net=host and not net=container. We were always including exposed ports, which was not correct, but is an easy logical fix.
Also required is a test change to correct the expected behavior as we were testing for incorrect behavior.
Fixes https://issues.redhat.com/browse/RHEL-60382
Does this PR introduce a user-facing change?