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

Add host.containers.internal entry into container's etc/hosts #9972

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
2 changes: 2 additions & 0 deletions libpod/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ type Container struct {

// This is true if a container is restored from a checkpoint.
restoreFromCheckpoint bool

slirp4netnsSubnet *net.IPNet
}

// ContainerState contains the current state of the container
Expand Down
93 changes: 73 additions & 20 deletions libpod/container_internal_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,34 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti
return c.save()
}

// Retrieves a container's "root" net namespace container dependency.
Copy link
Member

Choose a reason for hiding this comment

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

Can you note that this returns nil if there is no dependency?

Copy link
Author

@bblenard bblenard Apr 9, 2021

Choose a reason for hiding this comment

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

I don't think this can be null unless there is an error, although I could be wrong. So would you like a note about it returning an nil on error? That being said I am adding some extra error handling in that function based on your comment, but it wont make sense if State.Container interface function can return nil, nil.

func (c *Container) getRootNetNsDepCtr() (depCtr *Container, err error) {
containersVisited := map[string]int{c.config.ID: 1}
nextCtr := c.config.NetNsCtr
for nextCtr != "" {
// Make sure we aren't in a loop
if _, visited := containersVisited[nextCtr]; visited {
return nil, errors.New("loop encountered while determining net namespace container")
}
containersVisited[nextCtr] = 1

depCtr, err = c.runtime.state.Container(nextCtr)
if err != nil {
return nil, errors.Wrapf(err, "error fetching dependency %s of container %s", c.config.NetNsCtr, c.ID())
}
// This should never happen without an error
if depCtr == nil {
break
}
nextCtr = depCtr.config.NetNsCtr
}

if depCtr == nil {
return nil, errors.New("unexpected error depCtr is nil without reported error from runtime state")
}
return depCtr, nil
}

// Make standard bind mounts to include in the container
func (c *Container) makeBindMounts() error {
if err := os.Chown(c.state.RunDir, c.RootUID(), c.RootGID()); err != nil {
Expand Down Expand Up @@ -1396,24 +1424,9 @@ func (c *Container) makeBindMounts() error {
// We want /etc/resolv.conf and /etc/hosts from the
// other container. Unless we're not creating both of
// them.
var (
depCtr *Container
nextCtr string
)

// I don't like infinite loops, but I don't think there's
// a serious risk of looping dependencies - too many
// protections against that elsewhere.
nextCtr = c.config.NetNsCtr
for {
depCtr, err = c.runtime.state.Container(nextCtr)
if err != nil {
return errors.Wrapf(err, "error fetching dependency %s of container %s", c.config.NetNsCtr, c.ID())
}
nextCtr = depCtr.config.NetNsCtr
if nextCtr == "" {
break
}
depCtr, err := c.getRootNetNsDepCtr()
if err != nil {
return errors.Wrapf(err, "error fetching network namespace dependency container for container %s", c.ID())
}

// We need that container's bind mounts
Expand Down Expand Up @@ -1698,7 +1711,12 @@ func (c *Container) generateResolvConf() (string, error) {
nameservers = resolvconf.GetNameservers(resolv.Content)
// slirp4netns has a built in DNS server.
if c.config.NetMode.IsSlirp4netns() {
nameservers = append([]string{slirp4netnsDNS}, nameservers...)
slirp4netnsDNS, err := GetSlirp4netnsDNS(c.slirp4netnsSubnet)
if err != nil {
logrus.Warn("failed to determine Slirp4netns DNS: ", err.Error())
} else {
nameservers = append([]string{slirp4netnsDNS.String()}, nameservers...)
}
}
}

Expand Down Expand Up @@ -1779,7 +1797,12 @@ func (c *Container) getHosts() string {
if c.Hostname() != "" {
if c.config.NetMode.IsSlirp4netns() {
// When using slirp4netns, the interface gets a static IP
hosts += fmt.Sprintf("# used by slirp4netns\n%s\t%s %s\n", slirp4netnsIP, c.Hostname(), c.config.Name)
slirp4netnsIP, err := GetSlirp4netnsGateway(c.slirp4netnsSubnet)
if err != nil {
logrus.Warn("failed to determine slirp4netnsIP: ", err.Error())
} else {
hosts += fmt.Sprintf("# used by slirp4netns\n%s\t%s %s\n", slirp4netnsIP.String(), c.Hostname(), c.config.Name)
}
} else {
hasNetNS := false
netNone := false
Expand All @@ -1802,6 +1825,36 @@ func (c *Container) getHosts() string {
}
}
}

// Add gateway entry
var depCtr *Container
if c.config.NetNsCtr != "" {
// ignoring the error because there isn't anything to do
depCtr, _ = c.getRootNetNsDepCtr()
} else if len(c.state.NetworkStatus) != 0 {
depCtr = c
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This last block is not necessary. depCtr will be nil by default.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure syntactically how to get rid of that block 😬

I added it because I wanted to ensure mutual exclusion between shared container network namespace configurations and containers with network namespaces created for them. Basically I didn't want the situation where

        var depCtr *Container
	if c.config.NetNsCtr != "" { // **TRUE**
		// ignoring the error because there isn't anything to do
		depCtr, _ = c.getRootNetNsDepCtr()
	} 
	if len(c.state.NetworkStatus) != 0 { // **TRUE**
		depCtr = c
	}

But If that is not possible / something to worry about I can make them both if's and remove the unnecessary block

Copy link
Author

Choose a reason for hiding this comment

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

@rhatdan just in case you didn't see my response.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is fine.

depCtr = nil
}

if depCtr != nil {
for _, pluginResultsRaw := range depCtr.state.NetworkStatus {
pluginResult, _ := cnitypes.GetResult(pluginResultsRaw)
for _, ip := range pluginResult.IPs {
hosts += fmt.Sprintf("%s host.containers.internal\n", ip.Gateway)
}
}
} else if c.config.NetMode.IsSlirp4netns() {
gatewayIP, err := GetSlirp4netnsGateway(c.slirp4netnsSubnet)
if err != nil {
logrus.Warn("failed to determine gatewayIP: ", err.Error())
} else {
hosts += fmt.Sprintf("%s host.containers.internal\n", gatewayIP.String())
}
} else {
logrus.Debug("network configuration does not support host.containers.internal address")
}

return hosts
}

Expand Down
27 changes: 14 additions & 13 deletions libpod/networking_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,12 @@ import (
)

const (
// slirp4netnsIP is the IP used by slirp4netns to configure the tap device
// inside the network namespace.
slirp4netnsIP = "10.0.2.100"

// slirp4netnsDNS is the IP for the built-in DNS server in the slirp network
slirp4netnsDNS = "10.0.2.3"

// slirp4netnsMTU the default MTU override
slirp4netnsMTU = 65520

// default slirp4ns subnet
defaultSlirp4netnsSubnet = "10.0.2.0/24"

// rootlessCNINSName is the file name for the rootless network namespace bind mount
rootlessCNINSName = "rootless-cni-ns"
)
Expand Down Expand Up @@ -360,15 +356,20 @@ func (r *Runtime) GetRootlessCNINetNs(new bool) (*RootlessCNI, error) {
}

// build a new resolv.conf file which uses the slirp4netns dns server address
resolveIP := slirp4netnsDNS
resolveIP, err := GetSlirp4netnsDNS(nil)
if err != nil {
return nil, errors.Wrap(err, "failed to determine default slirp4netns DNS address")
}

Copy link
Member

Choose a reason for hiding this comment

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

The block below here should also use GetSlirp4netnsDNS for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed: ef6027b2db9d7ebd4a294af002c08c0c93d0e5db

if netOptions.cidr != "" {
_, cidr, err := net.ParseCIDR(netOptions.cidr)
if err != nil {
return nil, errors.Wrap(err, "failed to parse slirp4netns cidr")
}
// the slirp dns ip is always the third ip in the subnet
cidr.IP[len(cidr.IP)-1] = cidr.IP[len(cidr.IP)-1] + 3
resolveIP = cidr.IP.String()
resolveIP, err = GetSlirp4netnsDNS(cidr)
if err != nil {
return nil, errors.Wrapf(err, "failed to determine slirp4netns DNS address from cidr: %s", cidr.String())
}
}
conf, err := resolvconf.Get()
if err != nil {
Expand All @@ -377,7 +378,7 @@ func (r *Runtime) GetRootlessCNINetNs(new bool) (*RootlessCNI, error) {
searchDomains := resolvconf.GetSearchDomains(conf.Content)
dnsOptions := resolvconf.GetOptions(conf.Content)

_, err = resolvconf.Build(filepath.Join(cniDir, "resolv.conf"), []string{resolveIP}, searchDomains, dnsOptions)
_, err = resolvconf.Build(filepath.Join(cniDir, "resolv.conf"), []string{resolveIP.String()}, searchDomains, dnsOptions)
if err != nil {
return nil, errors.Wrap(err, "failed to create rootless cni resolv.conf")
}
Expand Down Expand Up @@ -577,7 +578,7 @@ func (r *Runtime) setupRootlessNetNS(ctr *Container) error {
// set up port forwarder for CNI-in-slirp4netns
netnsPath := ctr.state.NetNS.Path()
// TODO: support slirp4netns port forwarder as well
return r.setupRootlessPortMappingViaRLK(ctr, netnsPath, "")
return r.setupRootlessPortMappingViaRLK(ctr, netnsPath)
}
return nil
}
Expand Down
92 changes: 80 additions & 12 deletions libpod/networking_slirp4netns.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,89 @@ func (r *Runtime) setupSlirp4netns(ctr *Container) error {
return err
}

// Set a default slirp subnet. Parsing a string with the net helper is easier than building the struct myself
_, ctr.slirp4netnsSubnet, _ = net.ParseCIDR(defaultSlirp4netnsSubnet)

// Set slirp4netnsSubnet addresses now that we are pretty sure the command executed
if netOptions.cidr != "" {
ipv4, ipv4network, err := net.ParseCIDR(netOptions.cidr)
if err != nil || ipv4.To4() == nil {
return errors.Errorf("invalid cidr %q", netOptions.cidr)
}
ctr.slirp4netnsSubnet = ipv4network
}

if havePortMapping {
if netOptions.isSlirpHostForward {
return r.setupRootlessPortMappingViaSlirp(ctr, cmd, apiSocket)
}
return r.setupRootlessPortMappingViaRLK(ctr, netnsPath, netOptions.cidr)
return r.setupRootlessPortMappingViaRLK(ctr, netnsPath)
}

return nil
}

// Get expected slirp ipv4 address based on subnet. If subnet is null use default subnet
// Reference: https://github.com/rootless-containers/slirp4netns/blob/master/slirp4netns.1.md#description
func GetSlirp4netnsIP(subnet *net.IPNet) (*net.IP, error) {
_, slirpSubnet, _ := net.ParseCIDR(defaultSlirp4netnsSubnet)
if subnet != nil {
slirpSubnet = subnet
}
expectedIP, err := addToIP(slirpSubnet, uint32(100))
if err != nil {
return nil, errors.Wrapf(err, "error calculating expected ip for slirp4netns")
}
return expectedIP, nil
}

// Get expected slirp Gateway ipv4 address based on subnet
// Reference: https://github.com/rootless-containers/slirp4netns/blob/master/slirp4netns.1.md#description
func GetSlirp4netnsGateway(subnet *net.IPNet) (*net.IP, error) {
_, slirpSubnet, _ := net.ParseCIDR(defaultSlirp4netnsSubnet)
if subnet != nil {
slirpSubnet = subnet
}
expectedGatewayIP, err := addToIP(slirpSubnet, uint32(2))
if err != nil {
return nil, errors.Wrapf(err, "error calculating expected gateway ip for slirp4netns")
}
return expectedGatewayIP, nil
}

// Get expected slirp DNS ipv4 address based on subnet
// Reference: https://github.com/rootless-containers/slirp4netns/blob/master/slirp4netns.1.md#description
func GetSlirp4netnsDNS(subnet *net.IPNet) (*net.IP, error) {
_, slirpSubnet, _ := net.ParseCIDR(defaultSlirp4netnsSubnet)
if subnet != nil {
slirpSubnet = subnet
}
expectedDNSIP, err := addToIP(slirpSubnet, uint32(3))
if err != nil {
return nil, errors.Wrapf(err, "error calculating expected dns ip for slirp4netns")
}
return expectedDNSIP, nil
}

// Helper function to calculate slirp ip address offsets
// Adapted from: https://github.com/signalsciences/ipv4/blob/master/int.go#L12-L24
func addToIP(subnet *net.IPNet, offset uint32) (*net.IP, error) {
// I have no idea why I have to do this, but if I don't ip is 0
ipFixed := subnet.IP.To4()

ipInteger := uint32(ipFixed[3]) | uint32(ipFixed[2])<<8 | uint32(ipFixed[1])<<16 | uint32(ipFixed[0])<<24
ipNewRaw := ipInteger + offset
// Avoid overflows
if ipNewRaw < ipInteger {
return nil, errors.Errorf("integer overflow while calculating ip address offset, %s + %d", ipFixed, offset)
}
ipNew := net.IPv4(byte(ipNewRaw>>24), byte(ipNewRaw>>16&0xFF), byte(ipNewRaw>>8)&0xFF, byte(ipNewRaw&0xFF))
if !subnet.Contains(ipNew) {
return nil, errors.Errorf("calculated ip address %s is not within given subnet %s", ipNew.String(), subnet.String())
}
return &ipNew, nil
}

func waitForSync(syncR *os.File, cmd *exec.Cmd, logFile io.ReadSeeker, timeout time.Duration) error {
prog := filepath.Base(cmd.Path)
if len(cmd.Args) > 0 {
Expand Down Expand Up @@ -363,7 +437,7 @@ func waitForSync(syncR *os.File, cmd *exec.Cmd, logFile io.ReadSeeker, timeout t
return nil
}

func (r *Runtime) setupRootlessPortMappingViaRLK(ctr *Container, netnsPath, slirp4CIDR string) error {
func (r *Runtime) setupRootlessPortMappingViaRLK(ctr *Container, netnsPath string) error {
syncR, syncW, err := os.Pipe()
if err != nil {
return errors.Wrapf(err, "failed to open pipe")
Expand All @@ -390,17 +464,11 @@ func (r *Runtime) setupRootlessPortMappingViaRLK(ctr *Container, netnsPath, slir
}
}

childIP := slirp4netnsIP
// set the correct childIP when a custom cidr is set
if slirp4CIDR != "" {
_, cidr, err := net.ParseCIDR(slirp4CIDR)
if err != nil {
return errors.Wrap(err, "failed to parse slirp4netns cidr")
}
// the slirp container ip is always the hundredth ip in the subnet
cidr.IP[len(cidr.IP)-1] = cidr.IP[len(cidr.IP)-1] + 100
childIP = cidr.IP.String()
slirp4netnsIP, err := GetSlirp4netnsIP(ctr.slirp4netnsSubnet)
if err != nil {
return errors.Wrapf(err, "failed to get slirp4ns ip")
}
childIP := slirp4netnsIP.String()
outer:
for _, r := range ctr.state.NetworkStatus {
for _, i := range r.IPs {
Expand Down
21 changes: 21 additions & 0 deletions test/system/500-networking.bats
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,27 @@ load helpers
done
}

@test "podman run with slirp4ns assigns correct gateway address to host.containers.internal" {
CIDR="$(random_rfc1918_subnet)"
Copy link
Member

Choose a reason for hiding this comment

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

Much better, thank you, but I'm kind of uncomfortable now with the variable name: a CIDR is defined as a full subnet, slash, and mask. A future maintainer may find this a stumbling point, and waste time (e.g. on line 169) wondering how a CIDR can have .2 appended. This is about a 3-out-of-10 gripe, I'm not going to insist on you changing it, but I'd like to bring it to the attention of others in case someone else on the team has a strong opinion either way.

run_podman run --network slirp4netns:cidr="${CIDR}.0/24" \
$IMAGE grep 'host.containers.internal' /etc/hosts
is "$output" "${CIDR}.2 host.containers.internal" "host.containers.internal should be the cidr+2 address"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not feeling this behavior is correct.
The CIDR+2 address isn't useful when slirp4netns is launched with --disable-host-loopback.
And --disable-host-loopback should not be removed, as exposing bare 127.0.0.1 to containers might result in container-breakout.

I suggest exposing the "real" host IP regardless to rootful/rootless.

Copy link
Author

Choose a reason for hiding this comment

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

So what would be the alternative? Are you suggesting to derive all interface ip addresses not including lo?

The issue (#5651) was to add behavior that reflects docker's host.docker.internal.

I had a look through some of the docker code and as far as I can tell that is a configuration option given to the daemon so if a user asks for the host gateway address to be added to their container it is added based on that configuration docker-ce reference

So should podman have a similar configuration option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you suggesting to derive all interface ip addresses not including lo?

The first interface (eth0) would be fine

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about this solution.

There can be so many different interfaces in so many different configurations that trying to derive an ip address from one of the interfaces seems unstable. I don't necessarily disagree that exposing the loopback interface to the container could result in a container breakout but this change doesn't give that access it simply puts a /etc/hosts entry for that interface.

Like I said above. I think the only viable alternative is to make the interface a configurable option similar to dockerd's --host-gateway-ip launch option.

Copy link
Author

Choose a reason for hiding this comment

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

@AkihiroSuda bump in case you didn't see my last response

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having an option SGTM


@test "podman run with slirp4ns adds correct dns address to resolv.conf" {
CIDR="$(random_rfc1918_subnet)"
run_podman run --network slirp4netns:cidr="${CIDR}.0/24" \
$IMAGE grep "${CIDR}" /etc/resolv.conf
is "$output" "nameserver ${CIDR}.3" "resolv.conf should have slirp4netns cidr+3 as a nameserver"
}

@test "podman run with slirp4ns assigns correct ip address container" {
CIDR="$(random_rfc1918_subnet)"
run_podman run --network slirp4netns:cidr="${CIDR}.0/24" \
$IMAGE sh -c "ip address | grep ${CIDR}"
is "$output" ".*inet ${CIDR}.100/24 \+" "container should have slirp4netns cidr+100 assigned to interface"
}

# "network create" now works rootless, with the help of a special container
@test "podman network create" {
myport=54322
Expand Down