From f89515a19ce98420d655aaa70ed0980dc1cba80a Mon Sep 17 00:00:00 2001 From: Johan Brandhorst-Satzkorn Date: Thu, 7 Sep 2023 11:52:00 -0700 Subject: [PATCH] all: avoid mutating global state in parallel tests (#3687) The SetupSuiteTargetFilters globally mutates the function used for logic inside the target handler service. This means that we could get inconsistent logical behavior in certain tests that were run in parallel, since the logic can change at runtime. Ensure tests that make use of this function are never run in parallel with other tests. --- internal/cmd/commands/server/worker_shutdown_reload_test.go | 1 + .../controller/handlers/targets/target_service_test.go | 2 +- .../controller/handlers/targets/tcp/target_service_test.go | 6 +++--- internal/daemon/controller/handlers/targets/testing.go | 1 + internal/tests/api/credentials/credentials_test.go | 1 + internal/tests/cluster/multi_controller_worker_test.go | 1 - internal/tests/cluster/session_cleanup_test.go | 6 ++---- internal/tests/cluster/worker_bytesupdown_test.go | 2 +- internal/tests/cluster/worker_proxy_test.go | 2 +- internal/tests/cluster/x509_verification_test.go | 3 ++- 10 files changed, 13 insertions(+), 12 deletions(-) diff --git a/internal/cmd/commands/server/worker_shutdown_reload_test.go b/internal/cmd/commands/server/worker_shutdown_reload_test.go index 00f8e7c8c5..3f2089e00e 100644 --- a/internal/cmd/commands/server/worker_shutdown_reload_test.go +++ b/internal/cmd/commands/server/worker_shutdown_reload_test.go @@ -95,6 +95,7 @@ func TestServer_ShutdownWorker(t *testing.T) { require.NotNil(tgtR) // Authorize and connect + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) sess := helper.NewTestSession(ctx, t, tcl, tgt.Id) sConn := sess.Connect(ctx, t) diff --git a/internal/daemon/controller/handlers/targets/target_service_test.go b/internal/daemon/controller/handlers/targets/target_service_test.go index 03b793e79e..61a4ad4257 100644 --- a/internal/daemon/controller/handlers/targets/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/target_service_test.go @@ -41,8 +41,8 @@ func TestWorkerList_Addresses(t *testing.T) { } func TestWorkerList_EgressFilter(t *testing.T) { - t.Parallel() ctx := context.Background() + // This prevents us from running tests in parallel. SetupSuiteTargetFilters(t) conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index 4eec65d544..41f91d4d96 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -2415,8 +2415,8 @@ func TestRemoveTargetCredentialSources(t *testing.T) { } func TestAuthorizeSession(t *testing.T) { - t.Parallel() ctx := context.Background() + // This prevents us from running tests in parallel. targets.SetupSuiteTargetFilters(t) conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) @@ -2712,13 +2712,13 @@ func TestAuthorizeSession(t *testing.T) { } func TestAuthorizeSessionTypedCredentials(t *testing.T) { - t.Parallel() ctx := context.Background() conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + // This prevents us from running tests in parallel. targets.SetupSuiteTargetFilters(t) sche := scheduler.TestScheduler(t, conn, wrapper) @@ -3303,8 +3303,8 @@ func TestAuthorizeSessionTypedCredentials(t *testing.T) { } func TestAuthorizeSession_Errors(t *testing.T) { - t.Parallel() ctx := context.Background() + // This prevents us from running tests in parallel. targets.SetupSuiteTargetFilters(t) conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) diff --git a/internal/daemon/controller/handlers/targets/testing.go b/internal/daemon/controller/handlers/targets/testing.go index 13cd11ed0b..d98d48450d 100644 --- a/internal/daemon/controller/handlers/targets/testing.go +++ b/internal/daemon/controller/handlers/targets/testing.go @@ -8,6 +8,7 @@ import ( ) // SetupSuiteTargetFilters is used to ensure that OSS tests run from the ENT repo use the OSS level of target filtering +// WARNING: Do NOT run tests in parallel when using this. func SetupSuiteTargetFilters(t *testing.T) { oldFn := AuthorizeSessionWorkerFilterFn AuthorizeSessionWorkerFilterFn = AuthorizeSessionWithWorkerFilter diff --git a/internal/tests/api/credentials/credentials_test.go b/internal/tests/api/credentials/credentials_test.go index cfb62c48c8..609388e821 100644 --- a/internal/tests/api/credentials/credentials_test.go +++ b/internal/tests/api/credentials/credentials_test.go @@ -412,6 +412,7 @@ func TestUpdateAfterKeyRotation(t *testing.T) { Level: hclog.Trace, }) + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) tc := controller.NewTestController(t, &controller.TestControllerOpts{SchedulerRunJobInterval: 100 * time.Millisecond}) diff --git a/internal/tests/cluster/multi_controller_worker_test.go b/internal/tests/cluster/multi_controller_worker_test.go index 09c0433ecb..c2ad13d1ee 100644 --- a/internal/tests/cluster/multi_controller_worker_test.go +++ b/internal/tests/cluster/multi_controller_worker_test.go @@ -123,7 +123,6 @@ func TestWorkerAppendInitialUpstreams(t *testing.T) { for { select { case <-time.After(500 * time.Millisecond): - break case <-cancelCtx.Done(): require.FailNow("No worker found after 10 seconds") } diff --git a/internal/tests/cluster/session_cleanup_test.go b/internal/tests/cluster/session_cleanup_test.go index c1dc438015..4dbee048e9 100644 --- a/internal/tests/cluster/session_cleanup_test.go +++ b/internal/tests/cluster/session_cleanup_test.go @@ -63,11 +63,9 @@ func workerGracePeriod(ty timeoutBurdenType) time.Duration { // TestSessionCleanup is the main test for session cleanup, and // dispatches to the individual subtests. func TestSessionCleanup(t *testing.T) { - t.Parallel() for _, burdenCase := range timeoutBurdenCases { burdenCase := burdenCase t.Run(string(burdenCase), func(t *testing.T) { - t.Parallel() t.Run("single_controller", testWorkerSessionCleanupSingle(burdenCase)) t.Run("multi_controller", testWorkerSessionCleanupMulti(burdenCase)) }) @@ -77,8 +75,8 @@ func TestSessionCleanup(t *testing.T) { func testWorkerSessionCleanupSingle(burdenCase timeoutBurdenType) func(t *testing.T) { const op = "cluster.testWorkerSessionCleanupSingle" return func(t *testing.T) { - t.Parallel() require := require.New(t) + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) logger := hclog.New(&hclog.LoggerOptions{ Name: t.Name(), @@ -203,7 +201,7 @@ func testWorkerSessionCleanupSingle(burdenCase timeoutBurdenType) func(t *testin func testWorkerSessionCleanupMulti(burdenCase timeoutBurdenType) func(t *testing.T) { const op = "cluster.testWorkerSessionCleanupMulti" return func(t *testing.T) { - t.Parallel() + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) require := require.New(t) logger := hclog.New(&hclog.LoggerOptions{ diff --git a/internal/tests/cluster/worker_bytesupdown_test.go b/internal/tests/cluster/worker_bytesupdown_test.go index 16753dd0e4..3c6e5e0fb9 100644 --- a/internal/tests/cluster/worker_bytesupdown_test.go +++ b/internal/tests/cluster/worker_bytesupdown_test.go @@ -21,9 +21,9 @@ import ( ) func TestWorkerBytesUpDown(t *testing.T) { - t.Parallel() require := require.New(t) + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) logger := hclog.New(&hclog.LoggerOptions{ diff --git a/internal/tests/cluster/worker_proxy_test.go b/internal/tests/cluster/worker_proxy_test.go index 12dbd81d41..1e2fbd0e0d 100644 --- a/internal/tests/cluster/worker_proxy_test.go +++ b/internal/tests/cluster/worker_proxy_test.go @@ -22,8 +22,8 @@ import ( func TestWorkerSessionProxyMultipleConnections(t *testing.T) { const op = "cluster.TestWorkerSessionMultipleConnections" - t.Parallel() + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) require := require.New(t) diff --git a/internal/tests/cluster/x509_verification_test.go b/internal/tests/cluster/x509_verification_test.go index b8c59bd7ff..597012c209 100644 --- a/internal/tests/cluster/x509_verification_test.go +++ b/internal/tests/cluster/x509_verification_test.go @@ -33,6 +33,7 @@ func TestCustomX509Verification_Client(t *testing.T) { ctx := context.Background() ec := event.TestEventerConfig(t, "TestWorkerReplay", event.TestWithObservationSink(t), event.TestWithSysSink(t)) testLock := &sync.Mutex{} + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) logger := hclog.New(&hclog.LoggerOptions{ Mutex: testLock, @@ -174,9 +175,9 @@ func TestCustomX509Verification_Server(t *testing.T) { func testCustomX509Verification_Server(ec event.TestConfig, certPool *x509.CertPool, dnsName, wantErrContains string) func(t *testing.T) { return func(t *testing.T) { - t.Parallel() req := require.New(t) ctx := context.Background() + // This prevents us from running tests in parallel. tg.SetupSuiteTargetFilters(t) conf, err := config.DevController()