tstest/integration: don't require TestMake, stop leaking binaries in /tmp

Previously all tests shared their tailscale+tailscaled binaries in
system /tmp directories, which often leaked, and required TestMain to
clean up (which feature/taildrop didn't use).

This makes it use testing.T.TempDir for the binaries, but still only
builds them once and efficiently as possible depending on the OS
copies them around between each test's temp dir.

Updates #15812

Change-Id: I0e2585613f272c3d798a423b8ad1737f8916f527
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

tstest/integration/integration.go

@@ -65,61 +65,151 @@ var (
 // as a last ditch place to report errors.
 var MainError syncs.AtomicValue[error]
 
-// CleanupBinaries cleans up any resources created by calls to BinaryDir, TailscaleBinary, or TailscaledBinary.
-// It should be called from TestMain after all tests have completed.
-func CleanupBinaries() {
-	buildOnce.Do(func() {})
-	if binDir != "" {
-		os.RemoveAll(binDir)
+// Binaries contains the paths to the tailscale and tailscaled binaries.
+type Binaries struct {
+	Dir        string
+	Tailscale  BinaryInfo
+	Tailscaled BinaryInfo
+}
+
+// BinaryInfo describes a tailscale or tailscaled binary.
+type BinaryInfo struct {
+	Path string // abs path to tailscale or tailscaled binary
+	Size int64
+
+	// FD and FDmu are set on Unix to efficiently copy the binary to a new
+	// test's automatically-cleaned-up temp directory.
+	FD   *os.File // for Unix (macOS, Linux, ...)
+	FDMu sync.Locker
+
+	// Contents is used on Windows instead of FD to copy the binary between
+	// test directories. (On Windows you can't keep an FD open while an earlier
+	// test's temp directories are deleted.)
+	// This burns some memory and costs more in I/O, but oh well.
+	Contents []byte
+}
+
+func (b BinaryInfo) CopyTo(dir string) (BinaryInfo, error) {
+	ret := b
+	ret.Path = filepath.Join(dir, path.Base(b.Path))
+
+	switch runtime.GOOS {
+	case "linux":
+		// TODO(bradfitz): be fancy and use linkat with AT_EMPTY_PATH to avoid
+		// copying? I couldn't get it to work, though.
+		// For now, just do the same thing as every other Unix and copy
+		// the binary.
+		fallthrough
+	case "darwin", "freebsd", "openbsd", "netbsd":
+		f, err := os.OpenFile(ret.Path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o755)
+		if err != nil {
+			return BinaryInfo{}, err
+		}
+		b.FDMu.Lock()
+		b.FD.Seek(0, 0)
+		size, err := io.Copy(f, b.FD)
+		b.FDMu.Unlock()
+		if err != nil {
+			f.Close()
+			return BinaryInfo{}, fmt.Errorf("copying %q: %w", b.Path, err)
+		}
+		if size != b.Size {
+			f.Close()
+			return BinaryInfo{}, fmt.Errorf("copy %q: size mismatch: %d != %d", b.Path, size, b.Size)
+		}
+		if err := f.Close(); err != nil {
+			return BinaryInfo{}, err
+		}
+		return ret, nil
+	case "windows":
+		return ret, os.WriteFile(ret.Path, b.Contents, 0o755)
+	default:
+		return BinaryInfo{}, fmt.Errorf("unsupported OS %q", runtime.GOOS)
 	}
 }
 
-// BinaryDir returns a directory containing test tailscale and tailscaled binaries.
-// If any test calls BinaryDir, there must be a TestMain function that calls
-// CleanupBinaries after all tests are complete.
-func BinaryDir(tb testing.TB) string {
+// GetBinaries create a temp directory using tb and builds (or copies previously
+// built) cmd/tailscale and cmd/tailscaled binaries into that directory.
+//
+// It fails tb if the build or binary copies fail.
+func GetBinaries(tb testing.TB) *Binaries {
+	dir := tb.TempDir()
 	buildOnce.Do(func() {
-		binDir, buildErr = buildTestBinaries()
+		buildErr = buildTestBinaries(dir)
 	})
 	if buildErr != nil {
 		tb.Fatal(buildErr)
 	}
-	return binDir
-}
-
-// TailscaleBinary returns the path to the test tailscale binary.
-// If any test calls TailscaleBinary, there must be a TestMain function that calls
-// CleanupBinaries after all tests are complete.
-func TailscaleBinary(tb testing.TB) string {
-	return filepath.Join(BinaryDir(tb), "tailscale"+exe())
-}
-
-// TailscaledBinary returns the path to the test tailscaled binary.
-// If any test calls TailscaleBinary, there must be a TestMain function that calls
-// CleanupBinaries after all tests are complete.
-func TailscaledBinary(tb testing.TB) string {
-	return filepath.Join(BinaryDir(tb), "tailscaled"+exe())
+	if binariesCache.Dir == dir {
+		return binariesCache
+	}
+	ts, err := binariesCache.Tailscale.CopyTo(dir)
+	if err != nil {
+		tb.Fatalf("copying tailscale binary: %v", err)
+	}
+	tsd, err := binariesCache.Tailscaled.CopyTo(dir)
+	if err != nil {
+		tb.Fatalf("copying tailscaled binary: %v", err)
+	}
+	return &Binaries{
+		Dir:        dir,
+		Tailscale:  ts,
+		Tailscaled: tsd,
+	}
 }
 
 var (
-	buildOnce sync.Once
-	buildErr  error
-	binDir    string
+	buildOnce     sync.Once
+	buildErr      error
+	binariesCache *Binaries
 )
 
 // buildTestBinaries builds tailscale and tailscaled.
-// It returns the dir containing the binaries.
-func buildTestBinaries() (string, error) {
-	bindir, err := os.MkdirTemp("", "")
-	if err != nil {
-		return "", err
+// On success, it initializes [binariesCache].
+func buildTestBinaries(dir string) error {
+	getBinaryInfo := func(name string) (BinaryInfo, error) {
+		bi := BinaryInfo{Path: filepath.Join(dir, name+exe())}
+		fi, err := os.Stat(bi.Path)
+		if err != nil {
+			return BinaryInfo{}, fmt.Errorf("stat %q: %v", bi.Path, err)
+		}
+		bi.Size = fi.Size()
+
+		switch runtime.GOOS {
+		case "windows":
+			bi.Contents, err = os.ReadFile(bi.Path)
+			if err != nil {
+				return BinaryInfo{}, fmt.Errorf("read %q: %v", bi.Path, err)
+			}
+		default:
+			bi.FD, err = os.OpenFile(bi.Path, os.O_RDONLY, 0)
+			if err != nil {
+				return BinaryInfo{}, fmt.Errorf("open %q: %v", bi.Path, err)
+			}
+			bi.FDMu = new(sync.Mutex)
+			// Note: bi.FD is copied around between tests but never closed, by
+			// design. It will be closed when the process exits, and that will
+			// close the inode that we're copying the bytes from for each test.
+		}
+		return bi, nil
 	}
-	err = build(bindir, "tailscale.com/cmd/tailscaled", "tailscale.com/cmd/tailscale")
+	err := build(dir, "tailscale.com/cmd/tailscaled", "tailscale.com/cmd/tailscale")
 	if err != nil {
-		os.RemoveAll(bindir)
-		return "", err
+		return err
 	}
-	return bindir, nil
+	b := &Binaries{
+		Dir: dir,
+	}
+	b.Tailscale, err = getBinaryInfo("tailscale")
+	if err != nil {
+		return err
+	}
+	b.Tailscaled, err = getBinaryInfo("tailscaled")
+	if err != nil {
+		return err
+	}
+	binariesCache = b
+	return nil
 }
 
 func build(outDir string, targets ...string) error {
@@ -442,10 +532,11 @@ func NewTestEnv(t testing.TB, opts ...TestEnvOpt) *TestEnv {
 	}
 	control.HTTPTestServer = httptest.NewUnstartedServer(control)
 	trafficTrap := new(trafficTrap)
+	binaries := GetBinaries(t)
 	e := &TestEnv{
 		t:                 t,
-		cli:               TailscaleBinary(t),
-		daemon:            TailscaledBinary(t),
+		cli:               binaries.Tailscale.Path,
+		daemon:            binaries.Tailscaled.Path,
 		LogCatcher:        logc,
 		LogCatcherServer:  httptest.NewServer(logc),
 		Control:           control,

tstest/integration/integration_test.go

@@ -49,7 +49,6 @@ func TestMain(m *testing.M) {
 	os.Setenv("TS_DISABLE_UPNP", "true")
 	flag.Parse()
 	v := m.Run()
-	CleanupBinaries()
 	if v != 0 {
 		os.Exit(v)
 	}

tstest/integration/vms/harness_test.go

@@ -134,11 +134,12 @@ func newHarness(t *testing.T) *Harness {
 	loginServer := fmt.Sprintf("http://%s", ln.Addr())
 	t.Logf("loginServer: %s", loginServer)
 
+	binaries := integration.GetBinaries(t)
 	h := &Harness{
 		pubKey:         string(pubkey),
-		binaryDir:      integration.BinaryDir(t),
-		cli:            integration.TailscaleBinary(t),
-		daemon:         integration.TailscaledBinary(t),
+		binaryDir:      binaries.Dir,
+		cli:            binaries.Tailscale.Path,
+		daemon:         binaries.Tailscaled.Path,
 		signer:         signer,
 		loginServerURL: loginServer,
 		cs:             cs,

tstest/integration/vms/vms_test.go

@@ -28,7 +28,6 @@ import (
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/sync/semaphore"
 	"tailscale.com/tstest"
-	"tailscale.com/tstest/integration"
 	"tailscale.com/types/logger"
 )
 
@@ -51,13 +50,6 @@ var (
 	}()
 )
 
-func TestMain(m *testing.M) {
-	flag.Parse()
-	v := m.Run()
-	integration.CleanupBinaries()
-	os.Exit(v)
-}
-
 func TestDownloadImages(t *testing.T) {
 	if !*runVMTests {
 		t.Skip("not running integration tests (need --run-vm-tests)")