-
Notifications
You must be signed in to change notification settings - Fork 45
common: add support for default_host_ips in containers.conf
#422
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
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6458 |
|
LGTM |
|
Just out of curiosity: was DefaultHostIP already a thing and it just needed to be exposed as default_host_ip in containers.conf? |
Luap99
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.
This also needs a basic unit test to confirm the option is getting parsed properly.
2429193 to
6be2cf2
Compare
default_host_ip in containers.confdefault_host_ips in containers.conf
6be2cf2 to
d3a47c2
Compare
This adds support for configuring default host IPs via containers.conf to bind published container ports to when no host IP is explicitly specified (e.g. -p 8000:8000). If multiple IPs are specified, separate port mapping for each of the specified IP would be created. For instance, setting this to `["127.0.0.1", "::1"]` and port specified as `-p 8080:80` will result into two port mappings in podman-- `127.0.0.1:8080:80` and `[::1]:8080:80`. Note that explicit host IP still overrides the default option set in containers.conf. Refers containers/podman#27186 Signed-off-by: Danish Prakash <[email protected]>
d3a47c2 to
8c23523
Compare
Luap99
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.
Please also add a basic test to ensure the field is parsed properly.
| // If empty, the default behavior is to bind to all interfaces (0.0.0.0). | ||
| // If multiple IPs are specified, separate port mapping for each of the specified | ||
| // IP would be created. | ||
| DefaultHostIPs []string `toml:"default_host_ips,omitempty"` |
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.
this should use attributedstring.Slice like other array to support appending accordingly
Also not trying to bikeshed but I wonder if default_host_ips is the best name here? Should it be default_publish_ips or default_port_bind_ips? HostIP IMO could be assumed to be something else unrelated to port mappings, i.e. we already have host_containers_internal_ip for example.
| behavior is to bind to all network interfaces (`0.0.0.0`). If multiple IPs are specified, | ||
| separate port mapping for each of the specified IP would be created. For instance, setting | ||
| this to `["127.0.0.1", "::1"]` and port specified as `-p 8080:80` will result into two | ||
| port mappings in podman--`127.0.0.1:8080:80` and `[::1]:8080:80`. |
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.
typo here with --
This adds support for configuring a default host IP via containers.conf to bind published container ports to when no host IP is explicitly specified (e.g. -p 8000:8000). Note that explicit host IP still overrides the default option set in containers.conf.
Refers containers/podman#27186