tstest/integration: address review feedback on Windows service tests

Updates #20381

Signed-off-by: Yaruk Asghar <yaruk@tailscale.com>

tsconst/wintun/wintun.go

@@ -1,10 +1,8 @@
 // Copyright (c) Tailscale Inc & contributors
 // SPDX-License-Identifier: BSD-3-Clause
 
-// Package wintun holds the pinned wintun.dll release the Tailscale Windows
-// client is built and tested against. It is the single source of truth for the
-// version, download URL, hash, and zip layout, so nothing has to copy these
-// strings (see tailscale/tailscale and tailscale/corp build + test code).
+// Package wintun is the single source of truth for the pinned wintun.dll release
+// the Tailscale Windows client is built and tested against.
 package wintun
 
 const (
@@ -16,9 +14,7 @@
 	SHA256 = "07c256185d6ee3652e09fa55c0b673e2624b565e02c4b9091c79ca7d2f24ef51"
 )
 
-// DLLZipPath returns the path within the release zip of the wintun.dll for the
-// given GOARCH (e.g. "amd64", "arm64", "386"). The zip lays DLLs out by wintun's
-// own arch names, which match GOARCH except that 386 is named "x86".
+// DLLZipPath returns the path of the wintun.dll for goArch within the release zip.
 func DLLZipPath(goArch string) string {
 	arch := goArch
 	if goArch == "386" {

tstest/integration/integration.go

@@ -60,9 +60,8 @@
 	verboseTailscaled = flag.Bool("verbose-tailscaled", false, "verbose tailscaled logging")
 	verboseTailscale  = flag.Bool("verbose-tailscale", false, "verbose tailscale CLI logging")
 
-	// runWindowsServiceTests enables the Windows service-mode integration tests
-	// (tailscaled installed as a service). On by default in CI. Tests opt in via
-	// NewTestEnv(t, WindowsServiceMode()) and run serially in this mode.
+	// runWindowsServiceTests enables the Windows service-mode integration tests.
+	// On by default in CI; tests opt in via NewTestEnv(t, UnskipOnWindows()).
 	runWindowsServiceTests = flag.Bool("run-windows-service-tests", cibuild.On(), "run Windows service-mode integration tests")
 )
 
@@ -542,32 +541,34 @@ func (f ConfigureControl) ModifyTestEnv(te *TestEnv) {
 	f(te.Control)
 }
 
-// windowsServiceOpt is the TestEnvOpt returned by WindowsServiceMode.
-type windowsServiceOpt struct{}
+// unskipOnWindowsOpt is the TestEnvOpt returned by UnskipOnWindows.
+type unskipOnWindowsOpt struct{}
 
-func (windowsServiceOpt) ModifyTestEnv(te *TestEnv) { te.windowsService = true }
+func (unskipOnWindowsOpt) ModifyTestEnv(te *TestEnv) {
+	// Only run as a real service on Windows; on other platforms the test runs
+	// the normal userspace daemon with a faked Windows GOOS, as it always has.
+	if runtime.GOOS == "windows" {
+		te.windowsService = true
+	}
+}
 
-// WindowsServiceMode makes NewTestEnv run tailscaled as a Windows service
-// (install-system-daemon) rather than a userspace child process. It's a no-op
-// off Windows. Only tests that pass it run on Windows; all others skip there.
-func WindowsServiceMode() TestEnvOpt { return windowsServiceOpt{} }
+// UnskipOnWindows also runs this test on Windows, as a real service (the only mode so far).
+func UnskipOnWindows() TestEnvOpt { return unskipOnWindowsOpt{} }
 
 // NewTestEnv starts a bunch of services and returns a new test environment.
 // NewTestEnv arranges for the environment's resources to be cleaned up on exit.
 func NewTestEnv(t testing.TB, opts ...TestEnvOpt) *TestEnv {
-	// Integration tests skip on Windows unless a test opts into service mode
-	// via WindowsServiceMode(); a Windows service is a singleton, so the generic
-	// (often multi-node) tests can't run against it. Pre-scan the opts for the
-	// service marker before starting any servers so a skip leaks nothing.
-	serviceMode := false
+	// Integration tests skip on Windows unless a test opts in via UnskipOnWindows.
+	// Pre-scan the opts before starting any servers so a skip leaks nothing.
+	unskipWindows := false
 	for _, o := range opts {
-		if _, ok := o.(windowsServiceOpt); ok {
-			serviceMode = true
+		if _, ok := o.(unskipOnWindowsOpt); ok {
+			unskipWindows = true
 		}
 	}
 	if runtime.GOOS == "windows" {
-		if !serviceMode {
-			t.Skip("integration tests skip on Windows unless run in service mode")
+		if !unskipWindows {
+			t.Skip("integration tests skip on Windows unless the test calls UnskipOnWindows")
 		}
 		if !*runWindowsServiceTests {
 			t.Skip("Windows service tests disabled (--run-windows-service-tests=false)")
@@ -857,12 +858,8 @@ func (n *TestNode) awaitTailscaledRunnable() error {
 	return nil
 }
 
-// daemonEnv returns the environment tailscaled is started with, shared by the
-// userspace child-process path (StartDaemonAsIPNGOOS) and the Windows service
-// path (which writes it to tailscaled-env.txt, since a service doesn't inherit
-// the test process's environment). ipnGOOS is the OS tailscaled should believe
-// it's running as. It does not include TS_PARENT_DEATH_FD, which is specific to
-// the child-process path.
+// daemonEnv returns the extra environment variables to use when starting tailscaled.
+// The ipnGOOS argument overrides [envknob.GOOS].
 func (n *TestNode) daemonEnv(ipnGOOS string) []string {
 	env := []string{
 		"TS_DEBUG_PERMIT_HTTP_C2N=1",
@@ -909,8 +906,7 @@ func (n *TestNode) StartDaemonAsIPNGOOS(ipnGOOS string) *Daemon {
 	}
 
 	if n.env.windowsService {
-		// Service mode has no stderr pipe; keep a parser so other helpers
-		// that reference it don't dereference nil.
+		// TODO(#20443): plumb service logs here so races/panics/DEBUG-ADDR are seen in service mode.
 		n.tailscaledParser = &nodeOutputParser{n: n}
 		return n.startWindowsServiceDaemon()
 	}

tstest/integration/integration_test.go

@@ -58,10 +58,9 @@ func TestMain(m *testing.M) {
 	// Have to disable UPnP which hits the network, otherwise it fails due to HTTP proxy.
 	os.Setenv("TS_DISABLE_UPNP", "true")
 	flag.Parse()
-	if *runWindowsServiceTests {
-		// The Windows service is a singleton (one service, one pipe, one state
-		// dir), so tests against it must run serially. envknob.Setenv refreshes
-		// the already-registered TS_SERIAL_TESTS that tstest.Parallel reads.
+	if *runWindowsServiceTests && runtime.GOOS == "windows" {
+		// On Windows the service is a singleton, so its tests run serially.
+		// envknob.Setenv refreshes the TS_SERIAL_TESTS that tstest.Parallel reads.
 		envknob.Setenv("TS_SERIAL_TESTS", "true")
 	}
 	v := m.Run()
@@ -1181,6 +1180,24 @@ func TestNoControlConnWhenDown(t *testing.T) {
 	d2.MustCleanShutdown(t)
 }
 
+// Issue 2137: make sure Windows tailscaled works with the CLI alone,
+// without the GUI to kick off a Start.
+func TestOneNodeUpWindowsStyle(t *testing.T) {
+	tstest.Parallel(t)
+	env := NewTestEnv(t, UnskipOnWindows())
+	n1 := NewTestNode(t, env)
+	n1.upFlagGOOS = "windows"
+
+	d1 := n1.StartDaemonAsIPNGOOS("windows")
+	n1.AwaitResponding()
+	n1.MustUp("--unattended")
+
+	t.Logf("Got IP: %v", n1.AwaitIP4())
+	n1.AwaitRunning()
+
+	d1.MustCleanShutdown(t)
+}
+
 // TestClientSideJailing tests that when one node is jailed for another, the
 // jailed node cannot initiate connections to the other node however the other
 // node can initiate connections to the jailed node.

tstest/integration/integration_windows_test.go

@@ -1,31 +0,0 @@
-// Copyright (c) Tailscale Inc & contributors
-// SPDX-License-Identifier: BSD-3-Clause
-
-//go:build windows
-
-package integration
-
-import (
-	"testing"
-
-	"tailscale.com/tstest"
-)
-
-// Issue 2137: make sure Windows tailscaled works with the CLI alone, without
-// the GUI to kick off a Start. Runs tailscaled as a real Windows service via
-// WindowsServiceMode; see NewTestEnv's Windows gating.
-func TestOneNodeUpWindowsStyle(t *testing.T) {
-	tstest.Parallel(t)
-	env := NewTestEnv(t, WindowsServiceMode())
-	n1 := NewTestNode(t, env)
-	n1.upFlagGOOS = "windows"
-
-	d1 := n1.StartDaemonAsIPNGOOS("windows")
-	n1.AwaitResponding()
-	n1.MustUp("--unattended")
-
-	t.Logf("Got IP: %v", n1.AwaitIP4())
-	n1.AwaitRunning()
-
-	d1.MustCleanShutdown(t)
-}

tstest/integration/service_notwindows.go

@@ -5,9 +5,8 @@
 
 package integration
 
-// The Windows service backend only compiles and runs on Windows. These stubs
-// let the package build on other platforms; they're never called, because
-// windowsService is only set when runtime.GOOS == "windows".
+// Non-Windows stubs for the Windows service backend; never called, since
+// windowsService is only set on Windows.
 
 func (n *TestNode) startWindowsServiceDaemon() *Daemon {
 	n.env.t.Fatal("Windows service daemon is only supported on Windows")

tstest/integration/service_windows.go

@@ -16,6 +16,7 @@
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -25,31 +26,28 @@
 	"tailscale.com/tsconst/wintun"
 )
 
-// serviceName is the Windows service tailscaled installs itself as
-// (see cmd/tailscaled/install_windows.go).
+// serviceName is the Windows service tailscaled installs itself as.
 const serviceName = "Tailscale"
 
-// startWindowsServiceDaemon installs and starts tailscaled as a LocalSystem
-// Windows service (the mode a customer runs), stages wintun.dll next to the
-// binary, delivers the harness environment via tailscaled-env.txt, and waits
-// for the LocalAPI. It fails the test on error.
+// startWindowsServiceDaemon installs and starts tailscaled as a Windows service.
 func (n *TestNode) startWindowsServiceDaemon() *Daemon {
 	t := n.env.t
 	t.Helper()
 
-	// install-system-daemon fails if the service exists; clear any leftover
-	// service and stale global state from a prior crashed run first.
-	n.uninstallService()
-	n.cleanupServiceState()
+	// A pre-existing Tailscale service means this isn't a disposable/CI machine;
+	// fail rather than uninstall it. Stale state is wiped below, not fatal.
+	if serviceExists(t) {
+		t.Fatal("existing Tailscale service found; run only on a disposable/CI machine")
+	}
 
+	n.cleanupServiceState()
 	stageWintun(t, filepath.Dir(n.env.daemon))
 	n.writeServiceEnvFile()
 
 	if out, err := exec.CommandContext(t.Context(), n.env.daemon, "install-system-daemon").CombinedOutput(); err != nil {
 		t.Fatalf("install-system-daemon: %v\n%s", err, out)
 	}
-	// Teardown (LIFO): stop, uninstall, then wipe global state so the next
-	// serialized test starts clean.
+	// Teardown (LIFO): stop, uninstall, then wipe state for the next test.
 	t.Cleanup(func() {
 		n.stopService()
 		n.uninstallService()
@@ -61,12 +59,11 @@ func (n *TestNode) startWindowsServiceDaemon() *Daemon {
 	return &Daemon{svc: n}
 }
 
-// startService starts the installed Tailscale service and waits for the SCM to
-// report it Running.
+// startService starts the service and waits for the SCM to report it Running.
 func (n *TestNode) startService() {
 	t := n.env.t
 	t.Helper()
-	m := n.connectSCM()
+	m := connectSCM(t)
 	defer m.Disconnect()
 	s, err := m.OpenService(serviceName)
 	if err != nil {
@@ -79,13 +76,12 @@ func (n *TestNode) startService() {
 	n.waitServiceState(s, svc.Running, 60*time.Second)
 }
 
-// stopService requests a stop and waits until the service is actually Stopped,
-// failing the test if it doesn't stop in time (a stuck stop is a real bug, not
-// something to race past).
+// stopService requests a stop and waits until the service is Stopped, failing
+// the test if it doesn't stop in time.
 func (n *TestNode) stopService() {
 	t := n.env.t
 	t.Helper()
-	m := n.connectSCM()
+	m := connectSCM(t)
 	defer m.Disconnect()
 	s, err := m.OpenService(serviceName)
 	if err != nil {
@@ -105,19 +101,15 @@ func (n *TestNode) stopService() {
 	n.waitServiceState(s, svc.Stopped, 60*time.Second)
 }
 
-// uninstallService removes the Tailscale service via tailscaled's
-// uninstall-system-daemon subcommand and waits until it's gone. A service that
-// isn't installed is fine (the prelude calls this to clear leftovers); any
-// other failure fails the test, since a lingering service breaks the next
-// install.
+// uninstallService removes the service via tailscaled's uninstall-system-daemon
+// and waits until it's gone; a missing service is fine.
 func (n *TestNode) uninstallService() {
 	t := n.env.t
 	t.Helper()
-	if !n.serviceExists() {
+	if !serviceExists(t) {
 		return
 	}
-	// Not t.Context(): this also runs from t.Cleanup, where the test's context
-	// is already canceled.
+	// Not t.Context(): this also runs from t.Cleanup, where it's canceled.
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
 	if out, err := exec.CommandContext(ctx, n.env.daemon, "uninstall-system-daemon").CombinedOutput(); err != nil {
@@ -126,13 +118,12 @@ func (n *TestNode) uninstallService() {
 	n.waitServiceGone(30 * time.Second)
 }
 
-// writeServiceEnvFile writes the harness environment a service can't inherit
-// (the SCM starts it without the test process's environment) to the file
-// tailscaled reads at startup: %ProgramData%\Tailscale\tailscaled-env.txt.
+// writeServiceEnvFile writes the harness env to the file tailscaled reads at
+// startup, since a service doesn't inherit the test process's environment.
 func (n *TestNode) writeServiceEnvFile() {
 	t := n.env.t
 	t.Helper()
-	dir := filepath.Join(os.Getenv("ProgramData"), "Tailscale")
+	dir := serviceStateDir()
 	if err := os.MkdirAll(dir, 0o755); err != nil {
 		t.Fatalf("creating %s: %v", dir, err)
 	}
@@ -141,23 +132,26 @@ func (n *TestNode) writeServiceEnvFile() {
 	if err := os.WriteFile(dst, []byte(body), 0o644); err != nil {
 		t.Fatalf("writing %s: %v", dst, err)
 	}
-	// Removed with the rest of the state dir in cleanupServiceState.
 }
 
-// cleanupServiceState removes the global state directory the service writes
-// (%ProgramData%\Tailscale), so a subsequent serialized test doesn't inherit a
-// prior node's identity or state. Called only after the service is stopped.
+// cleanupServiceState removes the service's global state dir so the next test
+// doesn't inherit a prior node's identity; call only after the service stops.
 func (n *TestNode) cleanupServiceState() {
 	t := n.env.t
 	t.Helper()
-	dir := filepath.Join(os.Getenv("ProgramData"), "Tailscale")
+	dir := serviceStateDir()
 	if err := os.RemoveAll(dir); err != nil {
 		t.Logf("removing %s: %v", dir, err)
 	}
 }
 
-// waitServiceReady polls the LocalAPI until BackendState is neither "" nor
-// NoState, guarding the post-start race (tailscale/tailscale#8695).
+// serviceStateDir is the global state dir a Tailscale service uses.
+func serviceStateDir() string {
+	return filepath.Join(os.Getenv("ProgramData"), "Tailscale")
+}
+
+// waitServiceReady polls the LocalAPI until BackendState leaves NoState,
+// guarding the post-start race (tailscale/tailscale#8695).
 func (n *TestNode) waitServiceReady(timeout time.Duration) {
 	t := n.env.t
 	t.Helper()
@@ -192,14 +186,13 @@ func (n *TestNode) waitServiceState(s *mgr.Service, want svc.State, timeout time
 	t.Fatalf("service %q did not reach state %d within %v", serviceName, want, timeout)
 }
 
-// waitServiceGone polls until the service no longer exists, so a fresh
-// install-system-daemon won't collide with a leftover.
+// waitServiceGone polls until the service no longer exists.
 func (n *TestNode) waitServiceGone(timeout time.Duration) {
 	t := n.env.t
 	t.Helper()
 	deadline := time.Now().Add(timeout)
 	for time.Now().Before(deadline) {
-		if !n.serviceExists() {
+		if !serviceExists(t) {
 			return
 		}
 		time.Sleep(time.Second)
@@ -208,10 +201,9 @@ func (n *TestNode) waitServiceGone(timeout time.Duration) {
 }
 
 // serviceExists reports whether the Tailscale service is currently installed.
-func (n *TestNode) serviceExists() bool {
-	t := n.env.t
+func serviceExists(t testing.TB) bool {
 	t.Helper()
-	m := n.connectSCM()
+	m := connectSCM(t)
 	defer m.Disconnect()
 	s, err := m.OpenService(serviceName)
 	if err != nil {
@@ -222,8 +214,7 @@ func (n *TestNode) serviceExists() bool {
 }
 
 // connectSCM connects to the Windows service manager, failing the test on error.
-func (n *TestNode) connectSCM() *mgr.Mgr {
-	t := n.env.t
+func connectSCM(t testing.TB) *mgr.Mgr {
 	t.Helper()
 	m, err := mgr.Connect()
 	if err != nil {
@@ -232,10 +223,8 @@ func (n *TestNode) connectSCM() *mgr.Mgr {
 	return m
 }
 
-// stageWintun makes a verified wintun.dll a sibling of tailscaled.exe in dir;
-// tailscaled loads it from the directory of its own executable (see
-// fullyQualifiedWintunPath in cmd/tailscaled/tailscaled_windows.go), so the
-// adapter only comes up if the DLL is next to the binary.
+// stageWintun downloads and verifies wintun.dll into dir; tailscaled loads it
+// from its executable's dir (cmd/tailscaled.fullyQualifiedWintunPath).
 func stageWintun(t testing.TB, dir string) {
 	t.Helper()
 	req, err := http.NewRequestWithContext(t.Context(), "GET", wintun.URL, nil)
@@ -261,7 +250,7 @@ func stageWintun(t testing.TB, dir string) {
 	if err != nil {
 		t.Fatalf("opening wintun zip: %v", err)
 	}
-	member := wintun.DLLZipPath("amd64")
+	member := wintun.DLLZipPath(runtime.GOARCH)
 	f, err := zr.Open(member)
 	if err != nil {
 		t.Fatalf("wintun zip missing %q: %v", member, err)