-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
use etchosts package from c/common #13918
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@Luap99 all kinds of test unhappiness |
With my new logic you can no longer use |
I agree we should block the use of --add-hosts on containers in Pods as well as shared network namespace. |
I'm a little iffy on the change as it could be considered breaking. Can we check if Docker allows |
I just tried it and get an error
|
OK, I think we can justify this as docker-compat, then. I don't really see an issue with allowing it (it does add to all containers using the netns, but that isn't necessarily a bad thing) but if we can justify the removal I don't really mind. |
The problem with keeping this is that we also have to remove them when the container stops. The current logic just completely ignores this case so if you restart the container it will duplicate the entries every time as long as the infra container stays up. |
Yes lets remove the complication. Users have a workaround to add the hosts to the original pod or container. |
7907e41
to
474e598
Compare
LGTM |
if err != nil { | ||
// should we return an error here? | ||
logrus.Errorf("unable to lookup uts namespace for container %s: %v", c.ID(), err) | ||
return "" |
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.
Nit: I think it would be better to change the signature to return error here and let caller deal with it.
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.
Likely, however this function is used in many places right now and I didn't want to update all callers. Since this is somewhat unrelated to this change. I don't think we will ever hit this error.
@mheon PTAL
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 definitely change the signature, I can handle that today
Use the new logic from c/common to create the hosts file. This will help to better allign the hosts files between buildah and podman. Also this fixes several bugs: - remove host entries when container is stopped and has a netNsCtr - add entries for containers in a pod - do not duplicate entries in the hosts file - use the correct slirp ip when an userns is used Features: - configure host.containers.internal entry in containers.conf - configure base hosts file in containers.conf Fixes containers#12003 Fixes containers#13224 Signed-off-by: Paul Holzinger <[email protected]>
When we lookup the hostname for a given container we have to check if the container is joined to another utsns and use this hostname then instead. This fixes a problem where the `hostname` command would use the correct name but /etc/hostname would contain a different name. Signed-off-by: Paul Holzinger <[email protected]>
When we connect or disconnect from a network we also have to update /etc/hosts to ensure we only have valid entries in there. This also fixes problems with docker-compose since this makes use of network connect/disconnect. Fixes containers#12533 Signed-off-by: Paul Holzinger <[email protected]>
Because /etc/hosts is shared for all containers with a shared network namespace you should not be able to add hosts from a joined container. Only the primary netns container can set the hosts. Signed-off-by: Paul Holzinger <[email protected]>
Update the documentation for /etc/hosts options --add-host and --no-hosts. Also make sure that all references use the same text for consistency. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
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.
/lgtm
/hold
/hold cancel |
Use the new logic from c/common to create the hosts file. This will help
to better align the hosts files between buildah and podman.
Also this fixes several bugs:
Features:
Fixes #12003
Fixes #13224
libpod: fix c.Hostname() to respect the utsNsCtr
When we lookup the hostname for a given container we have to check if
the container is joined to another utsns and use this hostname then
instead.
This fixes a problem where the
hostname
command would use the correctname but /etc/hostname would contain a different name.
network dis-/connect: update /etc/hosts
When we connect or disconnect from a network we also have to update
/etc/hosts to ensure we only have valid entries in there.
This also fixes problems with docker-compose since this makes use of
network connect/disconnect.
Fixes #12533