Skip to content
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

Modify /etc/resolv.conf when connecting/disconnecting #13191

Merged
merged 1 commit into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 113 additions & 21 deletions libpod/container_internal_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -2045,19 +2045,8 @@ func (c *Container) generateResolvConf() (string, error) {
}
}

ipv6 := false
// If network status is set check for ipv6 and dns namesevers
netStatus := c.getNetworkStatus()
for _, status := range netStatus {
for _, netInt := range status.Interfaces {
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
}
}
}

if status.DNSServerIPs != nil {
for _, nsIP := range status.DNSServerIPs {
networkNameServers = append(networkNameServers, nsIP.String())
Expand All @@ -2070,16 +2059,9 @@ func (c *Container) generateResolvConf() (string, error) {
}
}

if c.config.NetMode.IsSlirp4netns() {
ctrNetworkSlipOpts := []string{}
if c.config.NetworkOptions != nil {
ctrNetworkSlipOpts = append(ctrNetworkSlipOpts, c.config.NetworkOptions["slirp4netns"]...)
}
slirpOpts, err := parseSlirp4netnsNetworkOptions(c.runtime, ctrNetworkSlipOpts)
if err != nil {
return "", err
}
ipv6 = slirpOpts.enableIPv6
ipv6, err := c.checkForIPv6(netStatus)
if err != nil {
return "", err
}

// Ensure that the container's /etc/resolv.conf is compatible with its
Expand Down Expand Up @@ -2160,6 +2142,116 @@ func (c *Container) generateResolvConf() (string, error) {
return destPath, nil
}

// Check if a container uses IPv6.
func (c *Container) checkForIPv6(netStatus map[string]types.StatusBlock) (bool, error) {
for _, status := range netStatus {
for _, netInt := range status.Interfaces {
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 {
return true, nil
}
}
}
}

if c.config.NetMode.IsSlirp4netns() {
mheon marked this conversation as resolved.
Show resolved Hide resolved
ctrNetworkSlipOpts := []string{}
if c.config.NetworkOptions != nil {
ctrNetworkSlipOpts = append(ctrNetworkSlipOpts, c.config.NetworkOptions["slirp4netns"]...)
}
slirpOpts, err := parseSlirp4netnsNetworkOptions(c.runtime, ctrNetworkSlipOpts)
if err != nil {
return false, err
}
return slirpOpts.enableIPv6, nil
}

return false, nil
}

// Add a new nameserver to the container's resolv.conf, ensuring that it is the
// first nameserver present.
// Usable only with running containers.
func (c *Container) addNameserver(ips []string) error {
// Take no action if container is not running.
if !c.ensureState(define.ContainerStateRunning, define.ContainerStateCreated) {
return nil
}

// Do we have a resolv.conf at all?
path, ok := c.state.BindMounts["/etc/resolv.conf"]
if !ok {
return nil
}

// Read in full contents, parse out existing nameservers
contents, err := ioutil.ReadFile(path)
if err != nil {
return err
}
ns := resolvconf.GetNameservers(contents)
options := resolvconf.GetOptions(contents)
search := resolvconf.GetSearchDomains(contents)

// We could verify that it doesn't already exist
// but extra nameservers shouldn't harm anything.
// Ensure we are the first entry in resolv.conf though, otherwise we
// might be after user-added servers.
ns = append(ips, ns...)

// 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.
Comment on lines +2203 to +2205
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we use rename?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would break the bind mount

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mheon, please create a jira card for that future work

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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consideration

Suggested change
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())

}

return nil
}

// Remove an entry from the existing resolv.conf of the container.
// Usable only with running containers.
func (c *Container) removeNameserver(ips []string) error {
// Take no action if container is not running.
if !c.ensureState(define.ContainerStateRunning, define.ContainerStateCreated) {
return nil
}

// Do we have a resolv.conf at all?
path, ok := c.state.BindMounts["/etc/resolv.conf"]
if !ok {
return nil
}

// Read in full contents, parse out existing nameservers
contents, err := ioutil.ReadFile(path)
if err != nil {
return err
}
ns := resolvconf.GetNameservers(contents)
options := resolvconf.GetOptions(contents)
search := resolvconf.GetSearchDomains(contents)

toRemove := make(map[string]bool)
for _, ip := range ips {
toRemove[ip] = true
}

newNS := make([]string, 0, len(ns))
for _, server := range ns {
if !toRemove[server] {
newNS = append(newNS, server)
}
}

if _, err := resolvconf.Build(path, newNS, search, options); err != nil {
return errors.Wrapf(err, "error removing nameservers from container %s resolv.conf", c.ID())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you change the one above, do a similar thing here.

}

return nil
}

// updateHosts updates the container's hosts file
func (c *Container) updateHosts(path string) error {
var hosts string
Expand Down
48 changes: 46 additions & 2 deletions libpod/networking_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -1170,6 +1170,7 @@ func (c *Container) NetworkDisconnect(nameOrID, netName string, force bool) erro
}

// update network status if container is running
oldStatus, statusExist := networkStatus[netName]
delete(networkStatus, netName)
c.state.NetworkStatus = networkStatus
err = c.save()
Expand All @@ -1180,8 +1181,26 @@ func (c *Container) NetworkDisconnect(nameOrID, netName string, force bool) erro
// Reload ports when there are still connected networks, maybe we removed the network interface with the child ip.
// Reloading without connected networks does not make sense, so we can skip this step.
if rootless.IsRootless() && len(networkStatus) > 0 {
return c.reloadRootlessRLKPortMapping()
if err := c.reloadRootlessRLKPortMapping(); err != nil {
return err
}
}

// Update resolv.conf if required
if statusExist {
stringIPs := make([]string, 0, len(oldStatus.DNSServerIPs))
for _, ip := range oldStatus.DNSServerIPs {
stringIPs = append(stringIPs, ip.String())
}
if len(stringIPs) == 0 {
return nil
}
logrus.Debugf("Removing DNS Servers %v from resolv.conf", stringIPs)
if err := c.removeNameserver(stringIPs); err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -1263,11 +1282,36 @@ func (c *Container) NetworkConnect(nameOrID, netName string, netOpts types.PerNe
if err != nil {
return err
}

// The first network needs a port reload to set the correct child ip for the rootlessport process.
// Adding a second network does not require a port reload because the child ip is still valid.
if rootless.IsRootless() && len(networks) == 0 {
return c.reloadRootlessRLKPortMapping()
if err := c.reloadRootlessRLKPortMapping(); err != nil {
return err
}
}

ipv6, err := c.checkForIPv6(networkStatus)
if err != nil {
return err
}

// Update resolv.conf if required
stringIPs := make([]string, 0, len(results[netName].DNSServerIPs))
for _, ip := range results[netName].DNSServerIPs {
if (ip.To4() == nil) && !ipv6 {
continue
}
stringIPs = append(stringIPs, ip.String())
}
if len(stringIPs) == 0 {
return nil
}
logrus.Debugf("Adding DNS Servers %v to resolv.conf", stringIPs)
if err := c.addNameserver(stringIPs); err != nil {
return err
}

return nil
}

Expand Down
31 changes: 31 additions & 0 deletions test/e2e/network_connect_disconnect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package integration

import (
"os"
"strings"

. "github.com/containers/podman/v4/test/utils"
"github.com/containers/storage/pkg/stringid"
Expand Down Expand Up @@ -77,6 +78,11 @@ var _ = Describe("Podman network connect and disconnect", func() {
Expect(session).Should(Exit(0))
defer podmanTest.removeNetwork(netName)

gw := podmanTest.Podman([]string{"network", "inspect", netName, "--format", "{{(index .Subnets 0).Gateway}}"})
gw.WaitWithDefaultTimeout()
Expect(gw).Should(Exit(0))
ns := gw.OutputToString()

ctr := podmanTest.Podman([]string{"run", "-dt", "--name", "test", "--network", netName, ALPINE, "top"})
ctr.WaitWithDefaultTimeout()
Expect(ctr).Should(Exit(0))
Expand All @@ -85,6 +91,11 @@ var _ = Describe("Podman network connect and disconnect", func() {
exec.WaitWithDefaultTimeout()
Expect(exec).Should(Exit(0))

exec2 := podmanTest.Podman([]string{"exec", "-it", "test", "cat", "/etc/resolv.conf"})
exec2.WaitWithDefaultTimeout()
Expect(exec2).Should(Exit(0))
Expect(strings.Contains(exec2.OutputToString(), ns)).To(BeTrue())

dis := podmanTest.Podman([]string{"network", "disconnect", netName, "test"})
dis.WaitWithDefaultTimeout()
Expect(dis).Should(Exit(0))
Expand All @@ -98,6 +109,11 @@ var _ = Describe("Podman network connect and disconnect", func() {
exec = podmanTest.Podman([]string{"exec", "-it", "test", "ip", "addr", "show", "eth0"})
exec.WaitWithDefaultTimeout()
Expect(exec).Should(ExitWithError())

exec3 := podmanTest.Podman([]string{"exec", "-it", "test", "cat", "/etc/resolv.conf"})
exec3.WaitWithDefaultTimeout()
Expect(exec3).Should(Exit(0))
Expect(strings.Contains(exec3.OutputToString(), ns)).To(BeFalse())
})

It("bad network name in connect should result in error", func() {
Expand Down Expand Up @@ -182,6 +198,16 @@ var _ = Describe("Podman network connect and disconnect", func() {
Expect(session).Should(Exit(0))
defer podmanTest.removeNetwork(newNetName)

gw := podmanTest.Podman([]string{"network", "inspect", newNetName, "--format", "{{(index .Subnets 0).Gateway}}"})
gw.WaitWithDefaultTimeout()
Expect(gw).Should(Exit(0))
ns := gw.OutputToString()

exec2 := podmanTest.Podman([]string{"exec", "-it", "test", "cat", "/etc/resolv.conf"})
exec2.WaitWithDefaultTimeout()
Expect(exec2).Should(Exit(0))
Expect(strings.Contains(exec2.OutputToString(), ns)).To(BeFalse())

ip := "10.11.100.99"
mac := "44:11:44:11:44:11"
connect := podmanTest.Podman([]string{"network", "connect", "--ip", ip, "--mac-address", mac, newNetName, "test"})
Expand All @@ -206,6 +232,11 @@ var _ = Describe("Podman network connect and disconnect", func() {
Expect(exec.OutputToString()).Should(ContainSubstring(ip))
Expect(exec.OutputToString()).Should(ContainSubstring(mac))

exec3 := podmanTest.Podman([]string{"exec", "-it", "test", "cat", "/etc/resolv.conf"})
exec3.WaitWithDefaultTimeout()
Expect(exec3).Should(Exit(0))
Expect(strings.Contains(exec3.OutputToString(), ns)).To(BeTrue())

// make sure no logrus errors are shown https://github.com/containers/podman/issues/9602
rm := podmanTest.Podman([]string{"rm", "--time=0", "-f", "test"})
rm.WaitWithDefaultTimeout()
Expand Down