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.

Sign up for GitHub

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

Let pasta configure interface, fix IPv6 outbound connectivity #458

Merged
merged 2 commits into from
Aug 17, 2024
Merged

Let pasta configure interface, fix IPv6 outbound connectivity #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ade4c86
pasta: Let it run in background, and wait until it forks
sbrivio-rh Aug 16, 2024
36ceb0e
child, pasta: Allow drivers to configure their own interface, let pas…
sbrivio-rh Aug 16, 2024
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
13 changes: 10 additions & 3 deletions pkg/child/child.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/network/lxcusernic/lxcusernic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down
6 changes: 6 additions & 0 deletions pkg/network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,17 @@ 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.
//
// netmsg MAY be modified.
// devName is like "tap" or "eth0"
ConfigureNetworkChild(netmsg *messages.ParentInitNetworkDriverCompleted, detachedNetNSPath string) (devName string, err error)

ChildDriverInfo() (*ChildDriverInfo, error)
}
35 changes: 16 additions & 19 deletions pkg/network/pasta/pasta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -111,12 +107,10 @@ func (d *parentDriver) ConfigureNetwork(childPID int, stateDir, detachedNetNSPat
}

opts := []string{
"--foreground",
"--stderr",
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See commit message:

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).

That was deprecated in passt's commit bca0fefa32e0 ("conf, passt: Make --stderr do nothing, and deprecate it"), included in upstream version 2024_06_24.1ee2eca. We can leave it for backwards compatibility, but in general we were already printing most stuff on stderr when running with --foreground anyway.

"--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(),
Expand Down Expand Up @@ -147,21 +141,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
Copy link
Member

Choose a reason for hiding this comment

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

Where is the stderr now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's all in out -- that's what out, err := cmd.CombinedOutput() does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pkg.go.dev/os/exec#Cmd.CombinedOutput

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)
}

Copy link
Member

Choose a reason for hiding this comment

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

What will happen if the background pasta process dies after several minutes/hours/days?
Can RootlessKit detect that death?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't, but if it happens (even though we haven't received this kind of "stability" bug reports for quite a long time now), we would like users to report an issue, because anyway context will be lost (TCP connections, at least), rather than hide this by restarting the process or similar.

Copy link
Member

Choose a reason for hiding this comment

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

The pasta process can be killed by the kernel mostly due to OOM, and in that case RootlessKit should probably abort, or at least print an error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pretty much impossible because it doesn't allocate memory dynamically, so I don't think it can actually be picked as a candidate by the OOM killer.

Anyway, the network interface will go away in that case, because we close the tap file descriptor, so something like that could actually be added.

I don't think it's actually needed, because the network interface going away is very obvious, but I guess I can implement it if needed (that will take a bit longer though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda I'm trying to implement this with the LinkSubscribe() method from github.com/vishvananda/netlink.

That part looks trivial, but I don't know where to add this kind of monitor in rootlesskit. I thought there was something like that for slirp4netns' --exit-fd, but I can't find it. How would you detect that slirp4netns terminates?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we lack the death detection for slirp4netns too 😅

Maybe we can use pidfd for kernel >= 5.3.
https://man7.org/linux/man-pages/man2/pidfd_open.2.html

A PID file descriptor can be monitored using poll(2), select(2), and epoll(7).

If we want to implement this too for old kernel, probably just watch SIGCHLD and see if the PID of (pasta|slirp4netns) is still valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we lack the death detection for slirp4netns too 😅

I feel better now. 😄

I think we should leave out PID tracking, because it keeps the interface to pasta very simple (also in terms of AppArmor and SELinux), and we have a valid, solid alternative (netlink monitor on the network interface). Anyway, this is the simple part.

The complicated part for me, whatever we choose as monitor, is where and how to add this to rootlesskit. I really don't know how it works with... threads? Should we spawn a thread somewhere? Is there something like that already in rootlesskit?

Copy link
Member

Choose a reason for hiding this comment

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

We don't seem to have any monitoring stuff in the current code base, but it can be probably spawned as a goroutine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, I'll give it a try, but with very low priority, so it might take a while.

netmsg := messages.ParentInitNetworkDriverCompleted{
Dev: tap,
MTU: d.mtu,
Expand Down Expand Up @@ -191,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
Expand Down
6 changes: 6 additions & 0 deletions pkg/network/slirp4netns/slirp4netns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down
6 changes: 6 additions & 0 deletions pkg/network/vpnkit/vpnkit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down