Modify /etc/resolv.conf when connecting/disconnecting #13191
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
openshift-ci
bot
added
the
approved
mheon
force-pushed
the
resolvconf_fixes
branch
5 times, most recently
from
c266d38 to
8ef6cfa
Compare
|
This should go green, @containers/podman-maintainers PTAL |
libpod/container_internal_linux.go
Outdated
| for _, netAddress := range netInt.Subnets { | ||
| // Note: only using To16() does not work since it also returns a valid ip for ipv4 | ||
| if netAddress.IPNet.IP.To4() == nil && netAddress.IPNet.IP.To16() != nil { | ||
| ipv6 = true |
There was a problem hiding this comment.
Can we break out of these loops on true?
| // hold the container lock, so there should be no risk of parallel | ||
| // modification. | ||
| if _, err := resolvconf.Build(path, ns, search, options); err != nil { | ||
| return errors.Wrapf(err, "error adding new nameserver to container %s resolv.conf", c.ID()) |
There was a problem hiding this comment.
For consideration
| return errors.Wrapf(err, "error adding new nameserver to container %s resolv.conf", c.ID()) | |
| return errors.Wrapf(err, "error adding new nameserver to resolv.conf in container %s", c.ID()) |
| } | ||
|
|
||
| if _, err := resolvconf.Build(path, newNS, search, options); err != nil { | ||
| return errors.Wrapf(err, "error removing nameservers from container %s resolv.conf", c.ID()) |
There was a problem hiding this comment.
if you change the one above, do a similar thing here.
| // We're rewriting the container's resolv.conf as part of this, but we | ||
| // hold the container lock, so there should be no risk of parallel | ||
| // modification. |
There was a problem hiding this comment.
how to prevent the container doesn't read a half-written file?
If we cannot use an atomic rename, do we need to temporarily pause the container and manually open and write the file to not depend on the undocumented ioutil.WriteFile behavior that it writes the file in-place?
There was a problem hiding this comment.
I think adding an optional pause/unpause should be enough initially. This is a temporary solution until we rewrite Aardvark to write firewall rules to allow containers to access it via a single address, even as their network membership changes, so the fix doesn't need to be permanent either. Given this I don't want to make the behavior too strongly integrated.
There was a problem hiding this comment.
mheon, please create a jira card for that future work
The `podman network connect` and `podman network disconnect` commands give containers access to different networks than the ones they were created with; these networks can also have DNS servers associated with them. Until now, however, we did not modify resolv.conf as network membership changed. With this PR, `podman network connect` will add any new nameservers supported by the new network to the container's /etc/resolv.conf, and `podman network disconnect` command will do the opposite, removing the network's nameservers from `/etc/resolv.conf`. Fixes containers#9603 Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
resolvconf_fixes
branch
from
8ef6cfa to
87cca4e
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
The
podman network connectandpodman network disconnectcommands give containers access to different networks than the ones they were created with; these networks can also have DNS servers associated with them. Until now, however, we did not modify resolv.conf as network membership changed.With this PR,
podman network connectwill add any new nameservers supported by the new network to the container's/etc/resolv.conf, and
podman network disconnectcommand will do the opposite, removing the network's nameservers from/etc/resolv.conf.Fixes #9603
Still to-do: do this for
/etc/hostsas well. That's slightly lower priority and will happen after RC5.