From 7ecfd1aa0715581b8cd837ce92c18e21b8c2f214 Mon Sep 17 00:00:00 2001 From: Zach <3724288+zedkipp@users.noreply.github.com> Date: Mon, 15 Dec 2025 09:40:59 -0700 Subject: [PATCH] fix: isolate keyring usage by parallel test processes (#21256) This change ensures keyring tests that utilize the real OS keyring use credentials that are isolated by process ID so that parallel test processes do not access the same credentials. https://github.com/coder/internal/issues/1192 --- cli/keyring_test.go | 19 ++------------- cli/sessionstore/sessionstore_test.go | 24 +++++++------------ cli/sessionstore/sessionstore_windows_test.go | 3 ++- cli/sessionstore/testhelpers/testhelpers.go | 15 ++++++++++++ 4 files changed, 27 insertions(+), 34 deletions(-) create mode 100644 cli/sessionstore/testhelpers/testhelpers.go diff --git a/cli/keyring_test.go b/cli/keyring_test.go index 7cb190845a..08f5db7c8d 100644 --- a/cli/keyring_test.go +++ b/cli/keyring_test.go @@ -2,15 +2,11 @@ package cli_test import ( "bytes" - "crypto/rand" - "encoding/binary" - "fmt" "net/url" "os" "path" "runtime" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,23 +15,12 @@ import ( "github.com/coder/coder/v2/cli/clitest" "github.com/coder/coder/v2/cli/config" "github.com/coder/coder/v2/cli/sessionstore" + "github.com/coder/coder/v2/cli/sessionstore/testhelpers" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/pty/ptytest" "github.com/coder/serpent" ) -// keyringTestServiceName generates a unique service name for keyring tests -// using the test name and a nanosecond timestamp to prevent collisions. -func keyringTestServiceName(t *testing.T) string { - t.Helper() - var n uint32 - err := binary.Read(rand.Reader, binary.BigEndian, &n) - if err != nil { - t.Fatal(err) - } - return fmt.Sprintf("%s_%v_%d", t.Name(), time.Now().UnixNano(), n) -} - type keyringTestEnv struct { serviceName string keyring sessionstore.Keyring @@ -52,7 +37,7 @@ func setupKeyringTestEnv(t *testing.T, clientURL string, args ...string) keyring cmd, err := root.Command(root.AGPL()) require.NoError(t, err) - serviceName := keyringTestServiceName(t) + serviceName := testhelpers.KeyringServiceName(t) root.WithKeyringServiceName(serviceName) root.UseKeyringWithGlobalConfig() diff --git a/cli/sessionstore/sessionstore_test.go b/cli/sessionstore/sessionstore_test.go index 1ecb027991..7e8f0cb2fb 100644 --- a/cli/sessionstore/sessionstore_test.go +++ b/cli/sessionstore/sessionstore_test.go @@ -3,18 +3,17 @@ package sessionstore_test import ( "encoding/json" "errors" - "fmt" "net/url" "os" "path" "runtime" "testing" - "time" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/config" "github.com/coder/coder/v2/cli/sessionstore" + "github.com/coder/coder/v2/cli/sessionstore/testhelpers" ) type storedCredentials map[string]struct { @@ -22,13 +21,6 @@ type storedCredentials map[string]struct { APIToken string `json:"api_token"` } -// Generate a test service name for use with the OS keyring. It uses a combination -// of the test name and a nanosecond timestamp to prevent collisions. -func keyringTestServiceName(t *testing.T) string { - t.Helper() - return t.Name() + "_" + fmt.Sprintf("%v", time.Now().UnixNano()) -} - func TestKeyring(t *testing.T) { t.Parallel() @@ -47,7 +39,7 @@ func TestKeyring(t *testing.T) { t.Run("ReadNonExistent", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) t.Cleanup(func() { _ = backend.Delete(srvURL) }) @@ -60,7 +52,7 @@ func TestKeyring(t *testing.T) { t.Run("DeleteNonExistent", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) t.Cleanup(func() { _ = backend.Delete(srvURL) }) @@ -73,7 +65,7 @@ func TestKeyring(t *testing.T) { t.Run("WriteAndRead", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) t.Cleanup(func() { _ = backend.Delete(srvURL) }) @@ -101,7 +93,7 @@ func TestKeyring(t *testing.T) { t.Run("WriteAndDelete", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) t.Cleanup(func() { _ = backend.Delete(srvURL) }) @@ -125,7 +117,7 @@ func TestKeyring(t *testing.T) { t.Run("OverwriteToken", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) t.Cleanup(func() { _ = backend.Delete(srvURL) }) @@ -156,7 +148,7 @@ func TestKeyring(t *testing.T) { t.Run("MultipleServers", func(t *testing.T) { t.Parallel() - backend := sessionstore.NewKeyringWithService(keyringTestServiceName(t)) + backend := sessionstore.NewKeyringWithService(testhelpers.KeyringServiceName(t)) srvURL, err := url.Parse(testURL) require.NoError(t, err) srvURL2, err := url.Parse(testURL2) @@ -220,7 +212,7 @@ func TestKeyring(t *testing.T) { srv2URL, err := url.Parse(testURL2) require.NoError(t, err) - serviceName := keyringTestServiceName(t) + serviceName := testhelpers.KeyringServiceName(t) backend := sessionstore.NewKeyringWithService(serviceName) t.Cleanup(func() { _ = backend.Delete(srv1URL) diff --git a/cli/sessionstore/sessionstore_windows_test.go b/cli/sessionstore/sessionstore_windows_test.go index ef643d3033..e677d0988f 100644 --- a/cli/sessionstore/sessionstore_windows_test.go +++ b/cli/sessionstore/sessionstore_windows_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/sessionstore" + "github.com/coder/coder/v2/cli/sessionstore/testhelpers" ) func readRawKeychainCredential(t *testing.T, serviceName string) []byte { @@ -31,7 +32,7 @@ func TestWindowsKeyring_WriteReadDelete(t *testing.T) { srvURL, err := url.Parse(testURL) require.NoError(t, err) - serviceName := keyringTestServiceName(t) + serviceName := testhelpers.KeyringServiceName(t) backend := sessionstore.NewKeyringWithService(serviceName) t.Cleanup(func() { _ = backend.Delete(srvURL) }) diff --git a/cli/sessionstore/testhelpers/testhelpers.go b/cli/sessionstore/testhelpers/testhelpers.go new file mode 100644 index 0000000000..d07bdb8097 --- /dev/null +++ b/cli/sessionstore/testhelpers/testhelpers.go @@ -0,0 +1,15 @@ +package testhelpers + +import ( + "fmt" + "os" + "testing" +) + +// KeyringServiceName generates a test service name for use with the OS keyring. +// It intends to prevent keyring usage collisions between parallel tests within a +// process and parallel test processes (which may occur on CI). +func KeyringServiceName(t *testing.T) string { + t.Helper() + return t.Name() + "_" + fmt.Sprintf("%v", os.Getpid()) +}