From ade4c86a252a996c235a26d7913cf9d38bbe18da Mon Sep 17 00:00:00 2001 From: Stefano Brivio Date: Fri, 16 Aug 2024 12:45:32 +0000 Subject: [PATCH 1/2] pasta: Let it run in background, and wait until it forks pasta(1), like many daemons, will fork to background only once it's ready to operate (handle traffic). On top of that, it doesn't need an explicit clean-up phase (unless --no-netns-quit is given), because it will terminate as soon as the target network namespace goes away. If we make it run in foreground and proceed as soon as we invoked the command (in background), we'll claim we're done too early, and tests such as integration-net.sh will occasionally fail, because pasta is still initialising sockets and interfaces as UDP packets are sent. Drop --foreground, and use CombinedOutput() instead of the Start() method of exec.Command(). Drop the explicit cleanup steps, too. Signed-off-by: Stefano Brivio --- pkg/network/pasta/pasta.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/pkg/network/pasta/pasta.go b/pkg/network/pasta/pasta.go index 0ea13b22..6a2520fa 100644 --- a/pkg/network/pasta/pasta.go +++ b/pkg/network/pasta/pasta.go @@ -111,7 +111,6 @@ func (d *parentDriver) ConfigureNetwork(childPID int, stateDir, detachedNetNSPat } opts := []string{ - "--foreground", "--stderr", "--ns-ifname=" + d.ifname, "--mtu=" + strconv.Itoa(d.mtu), @@ -147,21 +146,18 @@ func (d *parentDriver) ConfigureNetwork(childPID int, stateDir, detachedNetNSPat // `Couldn't open user namespace /proc/51813/ns/user: Permission denied` // Possibly related to AppArmor. cmd := exec.Command(d.binary, opts...) - cmd.Stdout = d.logWriter - cmd.Stderr = d.logWriter - cleanups = append(cleanups, func() error { - logrus.Debugf("killing pasta") - if cmd.Process != nil { - _ = cmd.Process.Kill() - } - wErr := cmd.Wait() - logrus.Debugf("killed pasta: %v", wErr) - return nil - }) logrus.Debugf("Executing %v", cmd.Args) - if err := cmd.Start(); err != nil { + out, err := cmd.CombinedOutput() + if err != nil { + exitErr := &exec.ExitError{} + if errors.As(err, &exitErr) { + return nil, common.Seq(cleanups), + fmt.Errorf("pasta failed with exit code %d:\n%s", + exitErr.ExitCode(), string(out)) + } return nil, common.Seq(cleanups), fmt.Errorf("executing %v: %w", cmd, err) } + netmsg := messages.ParentInitNetworkDriverCompleted{ Dev: tap, MTU: d.mtu, From 36ceb0e329a99ff415a1cece25bb1edc6835eb2c Mon Sep 17 00:00:00 2001 From: Stefano Brivio Date: Fri, 16 Aug 2024 09:58:51 +0000 Subject: [PATCH 2/2] child, pasta: Allow drivers to configure their own interface, let pasta do that As reported in https://github.com/moby/moby/issues/48257, when Docker rootless uses pasta through rootlesskit for user-mode connectivity, IPv6 can't be used for outbound connections because no addresses and no routes are configured in the container. The reason is that rootlesskit won't configure IPv6 addresses on the interface, and at the same time it doesn't ask pasta to do so using the --config-net option. Add a ChildDriverInfo() method to childDriver, returning a single piece of information, that is, whether the driver configures the network interface by itself, which is true only for pasta. If that's the case, there's no reason to call activateDev() from setupNet(). Further, in the pasta driver, skip the call to PrepareTap(), because pasta can take care of that as well. At the same time, ask pasta to do all that: set up the tap device, and configure IPv4 and IPv6, using --config-net. While at it, drop options --no-ra and --no-dhcp, as the container might want to send router solicitations and DHCP requests even if we permanently configure IPv4 and IPv6 addresses and routes, and there's no reason to ignore those requests. Drop --stderr as well: it doesn't do anything anymore, and it has been obsoleted in pasta for a while (it will always print to stderr when starting in foreground anyway). Link: https://github.com/moby/moby/issues/48257 Signed-off-by: Stefano Brivio --- pkg/child/child.go | 13 ++++++++++--- pkg/network/lxcusernic/lxcusernic.go | 6 ++++++ pkg/network/network.go | 6 ++++++ pkg/network/pasta/pasta.go | 13 +++++++------ pkg/network/slirp4netns/slirp4netns.go | 6 ++++++ pkg/network/vpnkit/vpnkit.go | 6 ++++++ 6 files changed, 41 insertions(+), 9 deletions(-) diff --git a/pkg/child/child.go b/pkg/child/child.go index b4d14787..2e86b8b9 100644 --- a/pkg/child/child.go +++ b/pkg/child/child.go @@ -215,8 +215,11 @@ func setupNet(stateDir string, msg *messages.ParentInitNetworkDriverCompleted, e if err := os.WriteFile(stateDirResolvConf, generateResolvConf(msg.DNS), 0644); err != nil { return fmt.Errorf("writing %s: %w", stateDirResolvConf, err) } - if err := activateDev(dev, msg.IP, msg.Netmask, msg.Gateway, msg.MTU); err != nil { - return err + Info, _ := driver.ChildDriverInfo() + if !Info.ConfiguresInterface { + if err := activateDev(dev, msg.IP, msg.Netmask, msg.Gateway, msg.MTU); err != nil { + return err + } } if etcWasCopied { // remove copied-up link @@ -255,7 +258,11 @@ func setupNet(stateDir string, msg *messages.ParentInitNetworkDriverCompleted, e return fmt.Errorf("writing %s: %w", stateDirResolvConf, err) } if err := ns.WithNetNSPath(detachedNetNSPath, func(_ ns.NetNS) error { - return activateDev(dev, msg.IP, msg.Netmask, msg.Gateway, msg.MTU) + Info, _ := driver.ChildDriverInfo() + if !Info.ConfiguresInterface { + return activateDev(dev, msg.IP, msg.Netmask, msg.Gateway, msg.MTU) + } + return nil }); err != nil { return err } diff --git a/pkg/network/lxcusernic/lxcusernic.go b/pkg/network/lxcusernic/lxcusernic.go index 3ec643e1..63d7853f 100644 --- a/pkg/network/lxcusernic/lxcusernic.go +++ b/pkg/network/lxcusernic/lxcusernic.go @@ -148,6 +148,12 @@ func exchangeDHCP(c *client4.Client, dev string, detachedNetNSPath string) (*dhc return ack, nil } +func (d *childDriver) ChildDriverInfo() (*network.ChildDriverInfo, error) { + return &network.ChildDriverInfo { + ConfiguresInterface: false, + }, nil +} + func (d *childDriver) ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (string, error) { dev := netmsg.Dev if dev == "" { diff --git a/pkg/network/network.go b/pkg/network/network.go index 74238787..11713fc9 100644 --- a/pkg/network/network.go +++ b/pkg/network/network.go @@ -17,6 +17,10 @@ type ParentDriver interface { ConfigureNetwork(childPID int, stateDir, detachedNetNSPath string) (netmsg *messages.ParentInitNetworkDriverCompleted, cleanup func() error, err error) } +type ChildDriverInfo struct { + ConfiguresInterface bool // Driver configures own namespace interface +} + // ChildDriver is called from the child namespace type ChildDriver interface { // ConfigureNetworkChild is executed in the child's namespaces, excluding detached-netns. @@ -24,4 +28,6 @@ type ChildDriver interface { // netmsg MAY be modified. // devName is like "tap" or "eth0" ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (devName string, err error) + + ChildDriverInfo() (*ChildDriverInfo, error) } diff --git a/pkg/network/pasta/pasta.go b/pkg/network/pasta/pasta.go index 6a2520fa..064230b6 100644 --- a/pkg/network/pasta/pasta.go +++ b/pkg/network/pasta/pasta.go @@ -17,7 +17,6 @@ import ( "github.com/rootless-containers/rootlesskit/v2/pkg/messages" "github.com/rootless-containers/rootlesskit/v2/pkg/network" "github.com/rootless-containers/rootlesskit/v2/pkg/network/iputils" - "github.com/rootless-containers/rootlesskit/v2/pkg/network/parentutils" ) // NewParentDriver instantiates new parent driver. @@ -92,9 +91,6 @@ func (d *parentDriver) MTU() int { func (d *parentDriver) ConfigureNetwork(childPID int, stateDir, detachedNetNSPath string) (*messages.ParentInitNetworkDriverCompleted, func() error, error) { tap := d.ifname var cleanups []func() error - if err := parentutils.PrepareTap(childPID, detachedNetNSPath, tap); err != nil { - return nil, common.Seq(cleanups), fmt.Errorf("setting up tap %s: %w", tap, err) - } address, err := iputils.AddIPInt(d.ipnet.IP, 100) if err != nil { @@ -114,8 +110,7 @@ func (d *parentDriver) ConfigureNetwork(childPID int, stateDir, detachedNetNSPat "--stderr", "--ns-ifname=" + d.ifname, "--mtu=" + strconv.Itoa(d.mtu), - "--no-dhcp", - "--no-ra", + "--config-net", "--address=" + address.String(), "--netmask=" + strconv.Itoa(netmask), "--gateway=" + gateway.String(), @@ -187,6 +182,12 @@ func NewChildDriver() network.ChildDriver { type childDriver struct { } +func (d *childDriver) ChildDriverInfo() (*network.ChildDriverInfo, error) { + return &network.ChildDriverInfo { + ConfiguresInterface: true, + }, nil +} + func (d *childDriver) ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (string, error) { // NOP return netmsg.Dev, nil diff --git a/pkg/network/slirp4netns/slirp4netns.go b/pkg/network/slirp4netns/slirp4netns.go index d69717e2..4c3d4d13 100644 --- a/pkg/network/slirp4netns/slirp4netns.go +++ b/pkg/network/slirp4netns/slirp4netns.go @@ -337,6 +337,12 @@ func NewChildDriver() network.ChildDriver { type childDriver struct { } +func (d *childDriver) ChildDriverInfo() (*network.ChildDriverInfo, error) { + return &network.ChildDriverInfo { + ConfiguresInterface: false, + }, nil +} + func (d *childDriver) ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (string, error) { tap := netmsg.Dev if tap == "" { diff --git a/pkg/network/vpnkit/vpnkit.go b/pkg/network/vpnkit/vpnkit.go index 6b5db535..2c3c9a88 100644 --- a/pkg/network/vpnkit/vpnkit.go +++ b/pkg/network/vpnkit/vpnkit.go @@ -172,6 +172,12 @@ func NewChildDriver() network.ChildDriver { type childDriver struct { } +func (d *childDriver) ChildDriverInfo() (*network.ChildDriverInfo, error) { + return &network.ChildDriverInfo { + ConfiguresInterface: false, + }, nil +} + func (d *childDriver) ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (tap string, err error) { tapName := netmsg.Dev if tapName == "" {