From 468241a13f945a7e852607d49edddb1a090c4f7d Mon Sep 17 00:00:00 2001 From: Alex Chan Date: Wed, 19 Aug 2026 08:42:11 +0100 Subject: [PATCH] envknob: add SetenvForTest helper for safe environment variable handling Our tests are inconsistent in how they set environment variables and cleanup. Missing cleanup logic can leak environment variable state across tests and change the behaviour of subsequent tests. This recently caused flakes in feature/acme, where `TestGetCertPEMWithValidity` leaked `TS_CERT_SHARE_MODE` and `TestAsyncRenewalDedup` to fail inconsistently. To fix the immediate flake and prevent future leaks, introduce a `SetenvForTest` helper that handles setting and cleaning up environment variables in tests. Fixes #20902 Change-Id: I9f8ef45ec875d66034cfd0054311bd69b67e2243 Signed-off-by: Alex Chan --- envknob/envknob.go | 21 +++++++++++++++++++++ feature/acme/cert_test.go | 7 ++----- ipn/ipnlocal/state_test.go | 3 +-- ipn/ipnserver/server_test.go | 18 ++++++------------ ipn/store/kubestore/store_kube_test.go | 5 ++--- logtail/logtail_test.go | 2 +- wgengine/magicsock/endpoint_test.go | 4 ++-- wgengine/magicsock/magicsock_test.go | 2 +- wgengine/netstack/netstack_test.go | 8 ++------ wgengine/userspace_test.go | 7 ++----- 10 files changed, 40 insertions(+), 37 deletions(-) diff --git a/envknob/envknob.go b/envknob/envknob.go index 73a0da700..b0e35f5b2 100644 --- a/envknob/envknob.go +++ b/envknob/envknob.go @@ -35,6 +35,7 @@ import ( "tailscale.com/kube/kubetypes" "tailscale.com/syncs" "tailscale.com/types/opt" + "tailscale.com/util/testenv" "tailscale.com/version" "tailscale.com/version/distro" ) @@ -110,6 +111,26 @@ func Setenv(envVar, val string) { } } +// SetenvForTest safely changes an environment variable in a test. +// +// It is designed to avoid leaking state between tests. It registers +// a cleanup function to reset the variable once the test completes, +// and panics if you try to set environment variables in parallel tests. +// +// Use this instead of t.Setenv() if your variable will be read by envknob, +// and you need to update envknob's internal caches with the new value. +func SetenvForTest(t testenv.TB, envVar, val string) { + t.Helper() + + // This is copied from [tstest.AssertNotParallel], which we can't + // call here because it would create an import cycle. + t.Setenv("ASSERT_NOT_PARALLEL_TEST", "1") // panics if t.Parallel was called + + prev := String(envVar) + Setenv(envVar, val) + t.Cleanup(func() { Setenv(envVar, prev) }) +} + // String returns the named environment variable, using os.Getenv. // // If the variable is non-empty, it's also tracked & logged as being diff --git a/feature/acme/cert_test.go b/feature/acme/cert_test.go index 38d763ce1..3bdb6d2ab 100644 --- a/feature/acme/cert_test.go +++ b/feature/acme/cert_test.go @@ -27,7 +27,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "tailscale.com/envknob" "tailscale.com/health" "tailscale.com/ipn" "tailscale.com/ipn/ipnlocal" @@ -744,12 +743,10 @@ func TestGetCertPEMWithValidity(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - - tstest.AssertNotParallel(t) if tt.readOnlyMode { - envknob.Setenv("TS_CERT_SHARE_MODE", "ro") + t.Setenv("TS_CERT_SHARE_MODE", "ro") } else { - envknob.Setenv("TS_CERT_SHARE_MODE", "") + t.Setenv("TS_CERT_SHARE_MODE", "") } os.RemoveAll(certDirPath) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index e8114cc6c..0238aedde 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -407,8 +407,7 @@ func TestStartShutsDownPreviousControlClient(t *testing.T) { // predictable, but maybe a bit less thorough. This is more of an overall // state machine test than a test of the wgengine+magicsock integration. func TestStateMachine(t *testing.T) { - envknob.Setenv("TAILSCALE_USE_WIP_CODE", "1") - defer envknob.Setenv("TAILSCALE_USE_WIP_CODE", "") + envknob.SetenvForTest(t, "TAILSCALE_USE_WIP_CODE", "1") c := qt.New(t) logf := tstest.WhileTestRunningLogger(t) diff --git a/ipn/ipnserver/server_test.go b/ipn/ipnserver/server_test.go index 409faf443..23359482e 100644 --- a/ipn/ipnserver/server_test.go +++ b/ipn/ipnserver/server_test.go @@ -23,7 +23,7 @@ import ( func TestUserConnectDisconnectNonWindows(t *testing.T) { enableLogging := false if runtime.GOOS == "windows" { - setGOOSForTest(t, "linux") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "linux") } ctx := context.Background() @@ -66,7 +66,7 @@ func TestUserConnectDisconnectNonWindows(t *testing.T) { func TestUserConnectDisconnectOnWindows(t *testing.T) { enableLogging := false - setGOOSForTest(t, "windows") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "windows") ctx := context.Background() server := lapitest.NewServer(t, lapitest.WithLogging(enableLogging)) @@ -88,7 +88,7 @@ func TestUserConnectDisconnectOnWindows(t *testing.T) { func TestIPNAlreadyInUseOnWindows(t *testing.T) { enableLogging := false - setGOOSForTest(t, "windows") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "windows") ctx := context.Background() server := lapitest.NewServer(t, lapitest.WithLogging(enableLogging)) @@ -111,7 +111,7 @@ func TestIPNAlreadyInUseOnWindows(t *testing.T) { func TestSequentialOSUserSwitchingOnWindows(t *testing.T) { enableLogging := false - setGOOSForTest(t, "windows") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "windows") ctx := context.Background() server := lapitest.NewServer(t, lapitest.WithLogging(enableLogging)) @@ -140,7 +140,7 @@ func TestSequentialOSUserSwitchingOnWindows(t *testing.T) { func TestConcurrentOSUserSwitchingOnWindows(t *testing.T) { enableLogging := false - setGOOSForTest(t, "windows") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "windows") ctx := context.Background() server := lapitest.NewServer(t, lapitest.WithLogging(enableLogging)) @@ -212,7 +212,7 @@ func TestConcurrentOSUserSwitchingOnWindows(t *testing.T) { func TestBlockWhileIdentityInUse(t *testing.T) { enableLogging := false - setGOOSForTest(t, "windows") + envknob.SetenvForTest(t, "TS_DEBUG_FAKE_GOOS", "windows") ctx := context.Background() server := lapitest.NewServer(t, lapitest.WithLogging(enableLogging)) @@ -312,12 +312,6 @@ func checkError(tb testing.TB, got, want error) { } } -func setGOOSForTest(tb testing.TB, goos string) { - tb.Helper() - envknob.Setenv("TS_DEBUG_FAKE_GOOS", goos) - tb.Cleanup(func() { envknob.Setenv("TS_DEBUG_FAKE_GOOS", "") }) -} - func pumpIPNBus(watcher *local.IPNBusWatcher) { for { _, err := watcher.Next() diff --git a/ipn/store/kubestore/store_kube_test.go b/ipn/store/kubestore/store_kube_test.go index 14b69787e..19c206ca5 100644 --- a/ipn/store/kubestore/store_kube_test.go +++ b/ipn/store/kubestore/store_kube_test.go @@ -375,8 +375,7 @@ func TestWriteTLSCertAndKey(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Set POD_NAME for testing selectors - envknob.Setenv("POD_NAME", "ingress-proxies-1") - defer envknob.Setenv("POD_NAME", "") + envknob.SetenvForTest(t, "POD_NAME", "ingress-proxies-1") secret := tt.initial // track current state client := &kubeclient.FakeClient{ @@ -772,7 +771,7 @@ func TestNewWithClient(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - envknob.Setenv("TS_CERT_SHARE_MODE", tt.certMode) + envknob.SetenvForTest(t, "TS_CERT_SHARE_MODE", tt.certMode) t.Setenv("POD_NAME", "ingress-proxies-1") diff --git a/logtail/logtail_test.go b/logtail/logtail_test.go index 2fe11af48..ef56f37e2 100644 --- a/logtail/logtail_test.go +++ b/logtail/logtail_test.go @@ -70,7 +70,7 @@ type LogtailTestServer struct { // the default FlushDelay and any other fake timers advance automatically. func newTestLogtailServer(t *testing.T) (*LogtailTestServer, *Logger) { // Enable the logtail started message - envknob.Setenv("TS_DEBUG_LOGTAIL", "1") + envknob.SetenvForTest(t, "TS_DEBUG_LOGTAIL", "1") conf, uploaded := newCaptureServer(t, 0) conf.Bus = eventbustest.NewBus(t) diff --git a/wgengine/magicsock/endpoint_test.go b/wgengine/magicsock/endpoint_test.go index 0df59bbc4..1f8bbe784 100644 --- a/wgengine/magicsock/endpoint_test.go +++ b/wgengine/magicsock/endpoint_test.go @@ -570,9 +570,9 @@ func Test_endpoint_sendDiscoPingsLocked_neverDirectUDP(t *testing.T) { t.Run(fmt.Sprintf("neverDirectUDP=%v", neverDirectUDP), func(t *testing.T) { prev := envknob.String("TS_DEBUG_NEVER_DIRECT_UDP") if neverDirectUDP { - envknob.Setenv("TS_DEBUG_NEVER_DIRECT_UDP", "true") + envknob.SetenvForTest(t, "TS_DEBUG_NEVER_DIRECT_UDP", "true") } else { - envknob.Setenv("TS_DEBUG_NEVER_DIRECT_UDP", "") + envknob.SetenvForTest(t, "TS_DEBUG_NEVER_DIRECT_UDP", "") } t.Cleanup(func() { envknob.Setenv("TS_DEBUG_NEVER_DIRECT_UDP", prev) }) now := mono.Now() diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 654df9352..fd2828e7c 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -2371,7 +2371,7 @@ func TestSetNetworkMapWithNoPeers(t *testing.T) { for i := 1; i <= 3; i++ { v := !debugEnableSilentDisco() - envknob.Setenv("TS_DEBUG_ENABLE_SILENT_DISCO", fmt.Sprint(v)) + envknob.SetenvForTest(t, "TS_DEBUG_ENABLE_SILENT_DISCO", fmt.Sprint(v)) c.SetNetworkMap(tailcfg.NodeView{}, nil) if c.lastFlags.heartbeatDisabled != v { t.Fatalf("call %d: didn't store netmap", i) diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index f0e1b04b1..1521eb718 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -739,9 +739,7 @@ func makeHangDialer(tb testing.TB) (netx.DialFunc, chan struct{}) { // TestTCPForwardLimits verifies that the limits on the TCP forwarder work in a // success case (i.e. when we don't hit the limit). func TestTCPForwardLimits(t *testing.T) { - tstest.AssertNotParallel(t) // calls envknob.Setenv - envknob.Setenv("TS_DEBUG_NETSTACK", "true") - t.Cleanup(func() { envknob.Setenv("TS_DEBUG_NETSTACK", "") }) + envknob.SetenvForTest(t, "TS_DEBUG_NETSTACK", "true") impl := makeNetstack(t, func(impl *Impl) { impl.ProcessSubnets = true @@ -818,9 +816,7 @@ func TestTCPForwardLimits(t *testing.T) { // forwarding works. func TestTCPForwardLimits_PerClient(t *testing.T) { clientmetric.ResetForTest(t) - tstest.AssertNotParallel(t) // calls envknob.Setenv - envknob.Setenv("TS_DEBUG_NETSTACK", "true") - t.Cleanup(func() { envknob.Setenv("TS_DEBUG_NETSTACK", "") }) + envknob.SetenvForTest(t, "TS_DEBUG_NETSTACK", "true") // Set our test override limits during this test. maxInFlightConnectionAttemptsForTest.Store(2) diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 9f4af60f0..cce080cf7 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -12,7 +12,6 @@ import ( "net/http" "net/http/httptest" "net/netip" - "os" "runtime" "slices" "sync" @@ -266,11 +265,9 @@ func TestUserspaceEnginePeerMTUReconfig(t *testing.T) { t.Skipf("skipping on %q; peer MTU not supported", runtime.GOOS) } - defer os.Setenv("TS_DEBUG_ENABLE_PMTUD", os.Getenv("TS_DEBUG_ENABLE_PMTUD")) - envknob.Setenv("TS_DEBUG_ENABLE_PMTUD", "") + envknob.SetenvForTest(t, "TS_DEBUG_ENABLE_PMTUD", "") // Turn on debugging to help diagnose problems. - defer os.Setenv("TS_DEBUG_PMTUD", os.Getenv("TS_DEBUG_PMTUD")) - envknob.Setenv("TS_DEBUG_PMTUD", "true") + envknob.SetenvForTest(t, "TS_DEBUG_PMTUD", "true") var knobs controlknobs.Knobs