-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: add support for changing default host IPs via containers.conf #27938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@danishprakash , you probably noticed, but this right now does not compile. The code looks OK, I just wanted to test it locally more :-) |
|
Yes, I guess I can mark it as such? This is dependent on containers/container-libs#422. A go.mod replace directive could help you test this locally, but it's best to let that get merged first imo. |
mheon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern, but the code otherwise LGTM
pkg/checkpoint/checkpoint_restore.go
Outdated
| return nil, err | ||
| } | ||
| defaultHostIPs := cfg.Network.DefaultHostIPs.Get() | ||
| pubPorts, err := specgenutil.CreatePortBindings(restoreOptions.PublishPorts, defaultHostIPs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe when importing a checkpoint, given that we may be changing details about the ports versus when the checkpoint was made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is expected here, this code path is when the user asked for new ports
podman container restore --publish so these new ports need to use the config setting.
|
@danishprakash The way we do thing normally is to add the go mod replace here to your fork/branch and then make sure CI passes here with that. Then we merge container-libs and you can rebase this br and remove the raplce and bump to the modules to |
pkg/checkpoint/checkpoint_restore.go
Outdated
| return nil, err | ||
| } | ||
| defaultHostIPs := cfg.Network.DefaultHostIPs.Get() | ||
| pubPorts, err := specgenutil.CreatePortBindings(restoreOptions.PublishPorts, defaultHostIPs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is expected here, this code path is when the user asked for new ports
podman container restore --publish so these new ports need to use the config setting.
pkg/specgenutil/util.go
Outdated
| // CreatePortBindings iterates ports mappings into SpecGen format. | ||
| func CreatePortBindings(ports []string) ([]types.PortMapping, error) { | ||
| // defaultHostIPs is the list of default host IPs to use when no IP is specified in the port mapping. | ||
| func CreatePortBindings(ports []string, defaultHostIPs []string) ([]types.PortMapping, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the wrong place here, I know we are very inconsistent overall but config fields should be read and processed on the server side while this here is cli parsing that is called on the remote client.
I think the easiest thing is to do this inside setupContainer( in libpod where we have other netowrking checks as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks for pointing it out! I was slightly confused earlier with the distinction.
6399ecd to
d8b9829
Compare
| if len(defaultHostIPs) > 0 { | ||
| expanded := make([]types.PortMapping, 0, len(ctr.config.PortMappings)*len(defaultHostIPs)) | ||
| for _, pm := range ctr.config.PortMappings { | ||
| if pm.HostIP == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Podman treats 0.0.0.0 differently; it leaves it unset because of netavark but seems to add it later on for inspect. But since the CLI strips this information, we can't be sure whether the user passed 0.0.0.0 or "', and prevent the server from overriding 0.0.0.0 with defaultHostIPs.
podman/pkg/specgenutil/util.go
Lines 182 to 183 in 98788a6
| // If hostIP is 0.0.0.0, leave it unset - netavark treats | |
| // 0.0.0.0 and empty differently, Docker does not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That blame is from roughly 5 years ago so I'm not sure if it still applies to netavark (I can check) but if it still applies, then perhaps we need an indicator to distinguish between 0.0.0.0 and "" for the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should get rid of that, netavark should be able to handle this just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pasta on f42 seems to be not happy with the changes:
[+0539s] # [22:33:37.823800306] $ /var/tmp/go/src/github.com/containers/podman/bin/podman run -d -p 5249:80 quay.io/libpod/testimage:20241011 sleep 60
[+0539s] # [22:33:37.954873074] Error: pasta failed with exit code 1:
[+0539s] # Altering mapping of already mapped port number: ::1/5249-5249:80-80
[+0539s] # Failed to bind port 5249 ((null)) for option '-t ::1/5249-5249:80-80'
f42 seems to be running a version based on a commit which was not tagged upstream (20250919.g623dbf6); but trying a released version that matches that timestamp (20250919.623dbf6), I got the tests passing locally. Furthermore, I tried latest passt release and the tests pass still.
So I'm guessing this has to do with the environment, perhaps config changes. Currently unsure but I'm trying to figure out what's different.
Add support for `default_host_ips` in containers.conf to set default host IP(s) if no IP is set when forwarding ports. Multiple IPs can be configured, and passing explicit IP with -p will always override the configured defaults. Signed-off-by: Danish Prakash <contact@danishpraka.sh>
Podman strips `0.0.0.0` to `""` because netavark handles both of them differently. With `""` netavark binds dual stack, same as docker. Signed-off-by: Danish Prakash <contact@danishpraka.sh>
3dd3594 to
5a5a4c2
Compare
Add support for
default_host_ipsin containers.conf to set default host IP(s) if no IP is set when forwarding ports. Multiple IPs can be configured, and passing an explicit IP with -p will always override the configured defaults.Fixes #27186