Move bidiPipe to vpn package and make public

Also removes --log-file functionality for now.
coder · deansheather · Nov 18, 2024 · Nov 15, 2024 · Nov 15, 2024 · Nov 15, 2024
commit 76b1a2f394adaa856277ea571810e973b6ebe30c
diff --git a/cli/vpndaemon_windows.go b/cli/vpndaemon_windows.go
@@ -3,12 +3,9 @@
 package cli
 
 import (
-	"io"
-	"os"
-
-	"cdr.dev/slog"
 	"golang.org/x/xerrors"
 
+	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/sloghuman"
 	"github.com/coder/coder/v2/vpn"
 	"github.com/coder/serpent"
@@ -18,7 +15,6 @@ func (r *RootCmd) vpnDaemonRun() *serpent.Command {
 	var (
 		rpcReadHandleInt  int64
 		rpcWriteHandleInt int64
-		logPath           string
 	)
 
 	cmd := &serpent.Command{
@@ -42,16 +38,10 @@ func (r *RootCmd) vpnDaemonRun() *serpent.Command {
 				Value:       serpent.Int64Of(&rpcWriteHandleInt),
 				Required:    true,
 			},
-			{
-				Flag:        "log-path",
-				Env:         "CODER_VPN_DAEMON_LOG_PATH",
-				Description: "The path to the log file to write to.",
-				Value:       serpent.StringOf(&logPath),
-				Required:    false, // logs will also be written to stderr
-			},
 		},
 		Handler: func(inv *serpent.Invocation) error {
 			ctx := inv.Context()
+			logger := inv.Logger.AppendSinks(sloghuman.Sink(inv.Stderr)).Leveled(slog.LevelDebug)
 
 			if rpcReadHandleInt < 0 || rpcWriteHandleInt < 0 {
 				return xerrors.Errorf("rpc-read-handle (%v) and rpc-write-handle (%v) must be positive", rpcReadHandleInt, rpcWriteHandleInt)
@@ -60,18 +50,10 @@ func (r *RootCmd) vpnDaemonRun() *serpent.Command {
 				return xerrors.Errorf("rpc-read-handle (%v) and rpc-write-handle (%v) must be different", rpcReadHandleInt, rpcWriteHandleInt)
 			}
 
-			logger := inv.Logger.AppendSinks(sloghuman.Sink(inv.Stderr)).Leveled(slog.LevelDebug)
-			if logPath != "" {
-				f, err := os.Create(logPath)
-				if err != nil {
-					return xerrors.Errorf("create log file: %w", err)
-				}
-				defer f.Close()
-				logger = logger.AppendSinks(sloghuman.Sink(f))
-			}
-
+			// We don't need to worry about duplicating the handles on Windows,
+			// which is different from Unix.
 			logger.Info(ctx, "opening bidirectional RPC pipe", slog.F("rpc_read_handle", rpcReadHandleInt), slog.F("rpc_write_handle", rpcWriteHandleInt))
-			pipe, err := newBidiPipe(uintptr(rpcReadHandleInt), uintptr(rpcWriteHandleInt))
+			pipe, err := vpn.NewBidirectionalPipe(uintptr(rpcReadHandleInt), uintptr(rpcWriteHandleInt))
 			if err != nil {
 				return xerrors.Errorf("create bidirectional RPC pipe: %w", err)
 			}
@@ -91,58 +73,3 @@ func (r *RootCmd) vpnDaemonRun() *serpent.Command {
 
 	return cmd
 }
-
-type bidiPipe struct {
-	read  *os.File
-	write *os.File
-}
-
-var _ io.ReadWriteCloser = bidiPipe{}
-
-func newBidiPipe(readHandle, writeHandle uintptr) (bidiPipe, error) {
-	read := os.NewFile(readHandle, "rpc_read")
-	_, err := read.Stat()
-	if err != nil {
-		return bidiPipe{}, xerrors.Errorf("stat rpc_read pipe (handle=%v): %w", readHandle, err)
-	}
-	write := os.NewFile(writeHandle, "rpc_write")
-	_, err = write.Stat()
-	if err != nil {
-		return bidiPipe{}, xerrors.Errorf("stat rpc_write pipe (handle=%v): %w", writeHandle, err)
-	}
-	return bidiPipe{
-		read:  read,
-		write: write,
-	}, nil
-}
-
-// Read implements io.Reader. Data is read from the read pipe.
-func (b bidiPipe) Read(p []byte) (int, error) {
-	n, err := b.read.Read(p)
-	if err != nil {
-		return n, xerrors.Errorf("read from rpc_read pipe (handle=%v): %w", b.read.Fd(), err)
-	}
-	return n, nil
-}
-
-// Write implements io.Writer. Data is written to the write pipe.
-func (b bidiPipe) Write(p []byte) (n int, err error) {
-	n, err = b.write.Write(p)
-	if err != nil {
-		return n, xerrors.Errorf("write to rpc_write pipe (handle=%v): %w", b.write.Fd(), err)
-	}
-	return n, nil
-}
-
-// Close implements io.Closer. Both the read and write pipes are closed.
-func (b bidiPipe) Close() error {
-	err := b.read.Close()
-	if err != nil {
-		return xerrors.Errorf("close rpc_read pipe (handle=%v): %w", b.read.Fd(), err)
-	}
-	err = b.write.Close()
-	if err != nil {
-		return xerrors.Errorf("close rpc_write pipe (handle=%v): %w", b.write.Fd(), err)
-	}
-	return nil
-}
diff --git a/cli/vpndaemon_windows_test.go b/cli/vpndaemon_windows_test.go
@@ -5,7 +5,6 @@ package cli_test
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -64,7 +63,7 @@ func TestVPNDaemonRun(t *testing.T) {
 		}
 	})
 
-	t.Run("LogFile", func(t *testing.T) {
+	t.Run("StartsTunnel", func(t *testing.T) {
 		t.Parallel()
 
 		r1, w1, err := os.Pipe()
@@ -76,11 +75,8 @@ func TestVPNDaemonRun(t *testing.T) {
 		defer r2.Close()
 		defer w2.Close()
 
-		logDir := t.TempDir()
-		logPath := filepath.Join(logDir, "coder-daemon.log")
-
 		ctx := testutil.Context(t, testutil.WaitLong)
-		inv, _ := clitest.New(t, "vpn-daemon", "run", "--rpc-read-handle", fmt.Sprint(r1.Fd()), "--rpc-write-handle", fmt.Sprint(w2.Fd()), "--log-path", logPath)
+		inv, _ := clitest.New(t, "vpn-daemon", "run", "--rpc-read-handle", fmt.Sprint(r1.Fd()), "--rpc-write-handle", fmt.Sprint(w2.Fd()))
 		waiter := clitest.StartWithWaiter(t, inv.WithContext(ctx))
 
 		// Send garbage which should cause the handshake to fail and the daemon
@@ -90,11 +86,6 @@ func TestVPNDaemonRun(t *testing.T) {
 		waiter.Cancel()
 		err = waiter.Wait()
 		require.ErrorContains(t, err, "handshake failed")
-
-		// Check that the log file was created and is not empty.
-		stat, err := os.Stat(logPath)
-		require.NoError(t, err)
-		require.Greater(t, stat.Size(), int64(0))
 	})
 
 	// TODO: once the VPN tunnel functionality is implemented, add tests that

diff --git a/vpn/pipe.go b/vpn/pipe.go
@@ -0,0 +1,69 @@
+package vpn
+
+import (
+	"io"
+	"os"
+
+	"github.com/hashicorp/go-multierror"
+	"golang.org/x/xerrors"
+)
+
+// BidirectionalPipe combines a pair of files that can be used for bidirectional
+// communication.
+type BidirectionalPipe struct {
+	read  *os.File
+	write *os.File
+}
+
+var _ io.ReadWriteCloser = BidirectionalPipe{}
+
+// NewBidirectionalPipe creates a new BidirectionalPipe from the given file
+// descriptors.
+func NewBidirectionalPipe(readFd, writeFd uintptr) (BidirectionalPipe, error) {
+	read := os.NewFile(readFd, "pipe_read")
+	_, err := read.Stat()
+	if err != nil {
+		return BidirectionalPipe{}, xerrors.Errorf("stat pipe_read (fd=%v): %w", readFd, err)
+	}
+	write := os.NewFile(writeFd, "pipe_write")
+	_, err = write.Stat()
+	if err != nil {
+		return BidirectionalPipe{}, xerrors.Errorf("stat pipe_write (fd=%v): %w", writeFd, err)
+	}
+	return BidirectionalPipe{
+		read:  read,
+		write: write,
+	}, nil
+}
+
+// Read implements io.Reader. Data is read from the read pipe.
+func (b BidirectionalPipe) Read(p []byte) (int, error) {
+	n, err := b.read.Read(p)
+	if err != nil {
+		return n, xerrors.Errorf("read from pipe_read (fd=%v): %w", b.read.Fd(), err)
+	}
+	return n, nil
+}
+
+// Write implements io.Writer. Data is written to the write pipe.
+func (b BidirectionalPipe) Write(p []byte) (n int, err error) {
+	n, err = b.write.Write(p)
+	if err != nil {
+		return n, xerrors.Errorf("write to pipe_write (fd=%v): %w", b.write.Fd(), err)
+	}
+	return n, nil
+}
+
+// Close implements io.Closer. Both the read and write pipes are closed.
+func (b BidirectionalPipe) Close() error {
+	var err error
+	rErr := b.read.Close()
+	if rErr != nil {
+		err = multierror.Append(err, xerrors.Errorf("close pipe_read (fd=%v): %w", b.read.Fd(), rErr))
+	}
+	wErr := b.write.Close()
+	if err != nil {
+		err = multierror.Append(err, xerrors.Errorf("close pipe_write (fd=%v): %w", b.write.Fd(), wErr))
+	}
+	return err
+}