From b65d5df0096e19b870402186596a35636e83fd28 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 10 Jun 2024 10:32:22 -0700 Subject: [PATCH] [vnet] minor fixes (#42663) * [vnet] minor fixes This commit: - ensures the tunfile finalizer won't close the file before the FD is written to the unix socket - explicitly closes the TUN after it's sent to avoid keeping the FD open - removes a forgetting Println * defer tun close, simplify goroutines+channels * fix linux build --- lib/vnet/ipbits.go | 1 - lib/vnet/setup.go | 109 +++++++++++++++++++-------------------- lib/vnet/setup_darwin.go | 16 +++--- lib/vnet/setup_other.go | 3 +- 4 files changed, 64 insertions(+), 65 deletions(-) diff --git a/lib/vnet/ipbits.go b/lib/vnet/ipbits.go index f69ba3dbfed02..d76a636a0df54 100644 --- a/lib/vnet/ipbits.go +++ b/lib/vnet/ipbits.go @@ -67,7 +67,6 @@ func randomFreeIPv4InNet(ipNet *net.IPNet, free func(ipv4) bool) (ipv4, error) { netMask := ipv4(binary.BigEndian.Uint32(ipNet.Mask)) netPrefix := ipv4(binary.BigEndian.Uint32(ipNet.IP)) - fmt.Println(netPrefix.String(), netMask.String()) // Pick a random starting point and increment until finding a free address. randAddrSuffix := ipv4(mathrand.Uint32()) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index d778fdf5c69b3..fe9ee88a78357 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -176,21 +176,16 @@ func AdminSubcommand(ctx context.Context, socketPath, ipv6Prefix, dnsAddr string ctx, cancel := context.WithCancel(ctx) defer cancel() - tunCh, errCh := createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix, dnsAddr) - var tun tun.Device - select { - case tun = <-tunCh: - case err := <-errCh: - return trace.Wrap(err, "performing admin setup") - } - tunName, err := tun.Name() + tunName, err := createAndSendTUNDevice(ctx, socketPath) if err != nil { - return trace.Wrap(err, "getting TUN name") - } - if err := sendTUNNameAndFd(socketPath, tunName, tun.File().Fd()); err != nil { - return trace.Wrap(err, "sending TUN over socket") + return trace.Wrap(err) } + errCh := make(chan error) + go func() { + errCh <- trace.Wrap(osConfigurationLoop(ctx, tunName, ipv6Prefix, dnsAddr)) + }() + // Stay alive until we get an error on errCh, indicating that the osConfig loop exited. // If the socket is deleted, indicating that the parent process exited, cancel the context and then wait // for the osConfig loop to exit and send an err on errCh. @@ -204,74 +199,74 @@ func AdminSubcommand(ctx context.Context, socketPath, ipv6Prefix, dnsAddr string cancel() return trace.Wrap(<-errCh) } - case err = <-errCh: + case err := <-errCh: return trace.Wrap(err) } } } -// createAndSetupTUNDeviceAsRoot creates a virtual network device and configures the host OS to use that device for -// VNet connections. -// -// After the TUN device is created, it will be sent on the result channel. Any error will be sent on the err -// channel. Always select on both the result channel and the err channel when waiting for a result. -// -// This will keep running until [ctx] is canceled or an unrecoverable error is encountered, in order to keep -// the host OS configuration up to date. -func createAndSetupTUNDeviceAsRoot(ctx context.Context, ipv6Prefix, dnsAddr string) (<-chan tun.Device, <-chan error) { - tunCh := make(chan tun.Device, 1) - errCh := make(chan error, 2) - +// createAndSendTUNDevice creates a virtual network TUN device and sends the open file descriptor on +// [socketPath]. It returns the name of the TUN device or an error. +func createAndSendTUNDevice(ctx context.Context, socketPath string) (string, error) { tun, tunName, err := createTUNDevice(ctx) if err != nil { - errCh <- trace.Wrap(err, "creating TUN device") - return tunCh, errCh + return "", trace.Wrap(err, "creating TUN device") } - tunCh <- tun + defer func() { + // We can safely close the TUN device in the admin process after it has been sent on the socket. + if err := tun.Close(); err != nil { + slog.WarnContext(ctx, "Failed to close TUN device.", "error", trace.Wrap(err)) + } + }() + + if err := sendTUNNameAndFd(socketPath, tunName, tun.File()); err != nil { + return "", trace.Wrap(err, "sending TUN over socket") + } + return tunName, nil +} + +// osConfigurationLoop will keep running until [ctx] is canceled or an unrecoverable error is encountered, in +// order to keep the host OS configuration up to date. +func osConfigurationLoop(ctx context.Context, tunName, ipv6Prefix, dnsAddr string) error { osConfigurator, err := newOSConfigurator(tunName, ipv6Prefix, dnsAddr) if err != nil { - errCh <- trace.Wrap(err, "creating OS configurator") - return tunCh, errCh + return trace.Wrap(err, "creating OS configurator") } // Clean up any stale configuration left by a previous VNet instance that may have failed to clean up. // This is necessary in case any stale /etc/resolver/ entries are still present, we need to // be able to reach the proxy in order to fetch the vnet_config. if err := osConfigurator.deconfigureOS(ctx); err != nil { - errCh <- trace.Wrap(err, "cleaning up OS configuration on startup") - return tunCh, errCh + return trace.Wrap(err, "cleaning up OS configuration on startup") } - go func() { - defer func() { - // Shutting down, deconfigure OS. Pass context.Background because [ctx] has likely been canceled - // already but we still need to clean up. - errCh <- trace.Wrap(osConfigurator.deconfigureOS(context.Background())) - }() - - if err := osConfigurator.updateOSConfiguration(ctx); err != nil { - errCh <- trace.Wrap(err, "applying initial OS configuration") - return + defer func() { + // Shutting down, deconfigure OS. Pass context.Background because [ctx] has likely been canceled + // already but we still need to clean up. + if err := osConfigurator.deconfigureOS(context.Background()); err != nil { + slog.ErrorContext(ctx, "Error deconfiguring host OS before shutting down.", "error", err) } + }() + + if err := osConfigurator.updateOSConfiguration(ctx); err != nil { + return trace.Wrap(err, "applying initial OS configuration") + } - // Re-configure the host OS every 10 seconds. This will pick up any newly logged-in clusters by - // reading profiles from TELEPORT_HOME. - ticker := time.NewTicker(10 * time.Second) - defer ticker.Stop() - for { - select { - case <-ticker.C: - if err := osConfigurator.updateOSConfiguration(ctx); err != nil { - errCh <- trace.Wrap(err, "updating OS configuration") - return - } - case <-ctx.Done(): - return + // Re-configure the host OS every 10 seconds. This will pick up any newly logged-in clusters by + // reading profiles from TELEPORT_HOME. + ticker := time.NewTicker(10 * time.Second) + defer ticker.Stop() + for { + select { + case <-ticker.C: + if err := osConfigurator.updateOSConfiguration(ctx); err != nil { + return trace.Wrap(err, "updating OS configuration") } + case <-ctx.Done(): + return ctx.Err() } - }() - return tunCh, errCh + } } func createTUNDevice(ctx context.Context) (tun.Device, string, error) { diff --git a/lib/vnet/setup_darwin.go b/lib/vnet/setup_darwin.go index 3cb7a3ae6346c..9c7bfbe04d567 100644 --- a/lib/vnet/setup_darwin.go +++ b/lib/vnet/setup_darwin.go @@ -27,6 +27,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "time" @@ -141,7 +142,7 @@ func createUnixSocket() (*net.UnixListener, string, error) { // sendTUNNameAndFd sends the name of the TUN device and its open file descriptor over a unix socket, meant // for passing the TUN from the root process which must create it to the user process. -func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { +func sendTUNNameAndFd(socketPath, tunName string, tunFile *os.File) error { socketAddr := &net.UnixAddr{Name: socketPath, Net: "unix"} conn, err := net.DialUnix(socketAddr.Net, nil /*laddr*/, socketAddr) if err != nil { @@ -153,11 +154,14 @@ func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { } // Write the device name as the main message and pass the file desciptor as out-of-band data. - rights := unix.UnixRights(int(fd)) - if _, _, err := conn.WriteMsgUnix([]byte(tunName), rights, socketAddr); err != nil { - return trace.Wrap(err, "writing to unix conn") - } - return nil + rights := unix.UnixRights(int(tunFile.Fd())) + _, _, err = conn.WriteMsgUnix([]byte(tunName), rights, socketAddr) + + // Hint to the garbage collector not to call the finalizer on tunFile, which would close the file and + // invalidate fd, until it has been written to the socket. + runtime.KeepAlive(tunFile) + + return trace.Wrap(err, "writing to unix conn") } // recvTUNNameAndFd receives the name of a TUN device and its open file descriptor over a unix socket, meant diff --git a/lib/vnet/setup_other.go b/lib/vnet/setup_other.go index 4d1976f93c088..60251b7251f87 100644 --- a/lib/vnet/setup_other.go +++ b/lib/vnet/setup_other.go @@ -22,6 +22,7 @@ package vnet import ( "context" "net" + "os" "runtime" "github.com/gravitational/trace" @@ -37,7 +38,7 @@ func createUnixSocket() (*net.UnixListener, string, error) { return nil, "", trace.Wrap(ErrVnetNotImplemented) } -func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { +func sendTUNNameAndFd(socketPath, tunName string, tunFile *os.File) error { return trace.Wrap(ErrVnetNotImplemented) }