Add ExposedPorts to Inspect's ContainerConfig#24110
Add ExposedPorts to Inspect's ContainerConfig#24110openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Conversation
Luap99
left a comment
There was a problem hiding this comment.
WE do seems to set exposed ports in the compat API (pkg/api/handlers/compat/containers.go) but there it seems to read of the ports setting so this seems wrong and should be updated now.
Given the code there I also did another test on an image with no exposed ports
[root@fedora ~]# docker run --name test3 -d -p 8081:1234 alpine sleep 1000
323c1996cf21b1148f12cac52be99bcf187210953a4360a77c274a6f04dbf874
[root@fedora ~]# docker inspect --format '{{json .Config.ExposedPorts}}' test3
{"1234/tcp":{}}
[root@fedora ~]# docker inspect --format '{{json .NetworkSettings.Ports}}' test3
{"1234/tcp":[{"HostIp":"0.0.0.0","HostPort":"8081"},{"HostIp":"::","HostPort":"8081"}]}
So it seems even if there is no port exposed and only published it will still show under exposed ports. I do not think your code behaves the same way in that regard
|
Yep, I'll sneak that fix in. |
1cad35f to
006c953
Compare
ba52a12 to
6026b95
Compare
|
@Luap99 Can you check what happens with |
This is where test3 has a exposed port: So there do not include thee network info for the dependency container so this seems different from our podman logic in getContainerNetworkInfo() where we lookup the netns container first and then use that. But I guess as exposed ports are handled outside of that we can fix it at least for them |
400080c to
0fec8d1
Compare
libpod/container_inspect.go
Outdated
There was a problem hiding this comment.
the type documentation describes the protocol ass comma separated value which other callers looping over it respect, i.e. makeInspectPortBindings()
That should be impossible in practise to get here as our port validation on container creation should split them out per protocol but I think we shoould try to keep this looping consistent. I wonder if we need a helper function to correctly iterate over them. I think go 1.23 even added support for actual iterators so we could define a custom one.
Looking at nv it seems we ignore that possibility there so maybe it is to late anyway and we should simplify our looping in the other places instead and ensure that our port parsing will never add ports with comma separated protocols.
There was a problem hiding this comment.
Instead of changing iteration, would you mind if I just add validation to the libpod container create code to make sure no port includes a comma-separated protocol list? I think, for sanity's sake, we should guarantee protocol is a single protocol per PortMapping struct within libpod.
There was a problem hiding this comment.
yeah that would work for me, though looking at the code I think it is guaranteed today already
podman/pkg/specgen/generate/ports_test.go
Line 120 in 639f3c6
I am not sure where you would add the validation? In the With...() libpod functions where we pass the ports? With the current code the risk is that we missing parsing the ports somewhere I guess
There was a problem hiding this comment.
I'd probably do it in container_validate.go to ensure it happens after we have a complete config, just in case.
0fec8d1 to
d0b0864
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
d0b0864 to
5a20dd5
Compare
A field we missed versus Docker. Matches the format of our
existing Ports list in the NetworkConfig, but only includes
exposed ports (and maps these to struct{}, as they never go to
real ports on the host).
Fixes https://issues.redhat.com/browse/RHEL-60382
Signed-off-by: Matt Heon <mheon@redhat.com>
5a20dd5 to
edc3dc5
Compare
|
Tests look to be going green, @containers/podman-maintainers PTAL |
|
[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 |
|
/lgtm |
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>
A field we missed versus Docker. Matches the format of our existing Ports list in the NetworkConfig, but only includes exposed ports (and maps these to struct{}, as they never go to real ports on the host).
Fixes https://issues.redhat.com/browse/RHEL-60382
Does this PR introduce a user-facing change?