[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

@@ -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())

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/<proxy address> 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) {

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

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