Merge branch '696-replace-os-mkdirtemp-usages-by-t-tempdir-2' into 'main'

Resolve "Replace os.MkdirTemp usages by t.TempDir" (part 2)

Closes #696

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/910



Merged-by: Igor Drozdov <idrozdov@gitlab.com>
Approved-by: Igor Drozdov <idrozdov@gitlab.com>
Co-authored-by: Ash McKenzie <amckenzie@gitlab.com>

client/client_test.go

@@ -25,7 +25,7 @@ var (
 )
 
 func TestClients(t *testing.T) {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
 	testCases := []struct {
 		desc            string
@@ -58,7 +58,7 @@ func TestClients(t *testing.T) {
 		},
 		{
 			desc:   "Https client",
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 			server: func(t *testing.T, handlers []testserver.TestRequestHandler) string {
 				return testserver.StartHttpsServer(t, handlers, "")
 			},
@@ -66,7 +66,7 @@ func TestClients(t *testing.T) {
 		},
 		{
 			desc:   "Secret with newlines",
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 			server: func(t *testing.T, handlers []testserver.TestRequestHandler) string {
 				return testserver.StartHttpsServer(t, handlers, "")
 			},

client/httpsclient_test.go

@@ -15,6 +15,8 @@ import (
 
 //go:generate openssl req -newkey rsa:4096 -new -nodes -x509 -days 3650 -out ../internal/testhelper/testdata/testroot/certs/client/server.crt -keyout ../internal/testhelper/testdata/testroot/certs/client/key.pem -subj "/C=US/ST=California/L=San Francisco/O=GitLab/OU=GitLab-Shell/CN=localhost"
 func TestSuccessfulRequests(t *testing.T) {
+	testRoot := testhelper.PrepareTestRootDir(t)
+
 	testCases := []struct {
 		desc                                        string
 		caFile, caPath                              string
@@ -22,25 +24,25 @@ func TestSuccessfulRequests(t *testing.T) {
 	}{
 		{
 			desc:   "Valid CaFile",
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 		},
 		{
 			desc:   "Valid CaPath",
-			caPath: path.Join(testhelper.TestRoot, "certs/valid"),
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caPath: path.Join(testRoot, "certs/valid"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 		},
 		{
 			desc:   "Invalid cert with self signed cert option enabled",
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 		},
 		{
 			desc:   "Client certs with CA",
-			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
+			caFile: path.Join(testRoot, "certs/valid/server.crt"),
 			// Run the command "go generate httpsclient_test.go" to
 			// regenerate the following test fixtures:
-			clientCAPath:   path.Join(testhelper.TestRoot, "certs/client/server.crt"),
-			clientCertPath: path.Join(testhelper.TestRoot, "certs/client/server.crt"),
-			clientKeyPath:  path.Join(testhelper.TestRoot, "certs/client/key.pem"),
+			clientCAPath:   path.Join(testRoot, "certs/client/server.crt"),
+			clientCertPath: path.Join(testRoot, "certs/client/server.crt"),
+			clientKeyPath:  path.Join(testRoot, "certs/client/key.pem"),
 		},
 	}
 
@@ -63,6 +65,8 @@ func TestSuccessfulRequests(t *testing.T) {
 }
 
 func TestFailedRequests(t *testing.T) {
+	testRoot := testhelper.PrepareTestRootDir(t)
+
 	testCases := []struct {
 		desc                   string
 		caFile                 string
@@ -72,17 +76,17 @@ func TestFailedRequests(t *testing.T) {
 	}{
 		{
 			desc:          "Invalid CaFile",
-			caFile:        path.Join(testhelper.TestRoot, "certs/invalid/server.crt"),
+			caFile:        path.Join(testRoot, "certs/invalid/server.crt"),
 			expectedError: "Internal API unreachable",
 		},
 		{
 			desc:                   "Missing CaFile",
-			caFile:                 path.Join(testhelper.TestRoot, "certs/invalid/missing.crt"),
+			caFile:                 path.Join(testRoot, "certs/invalid/missing.crt"),
 			expectedCaFileNotFound: true,
 		},
 		{
 			desc:          "Invalid CaPath",
-			caPath:        path.Join(testhelper.TestRoot, "certs/invalid"),
+			caPath:        path.Join(testRoot, "certs/invalid"),
 			expectedError: "Internal API unreachable",
 		},
 		{
@@ -108,8 +112,6 @@ func TestFailedRequests(t *testing.T) {
 }
 
 func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPath, clientKeyPath string) (*GitlabNetClient, error) {
-	testhelper.PrepareTestRootDir(t)
-
 	requests := []testserver.TestRequestHandler{
 		{
 			Path: "/api/v4/internal/hello",

client/testserver/testserver.go

@@ -95,8 +95,10 @@ func StartRetryHttpServer(t *testing.T, handlers []TestRequestHandler) string {
 func StartHttpsServer(t *testing.T, handlers []TestRequestHandler, clientCAPath string) string {
 	t.Helper()
 
-	crt := path.Join(testhelper.TestRoot, "certs/valid/server.crt")
-	key := path.Join(testhelper.TestRoot, "certs/valid/server.key")
+	testRoot := testhelper.PrepareTestRootDir(t)
+
+	crt := path.Join(testRoot, "certs/valid/server.crt")
+	key := path.Join(testRoot, "certs/valid/server.key")
 
 	server := httptest.NewUnstartedServer(buildHandler(handlers))
 	cer, err := tls.LoadX509KeyPair(crt, key)

cmd/gitlab-sshd/acceptance_test.go

@@ -141,9 +141,9 @@ func startGitOverHTTPServer(t *testing.T) string {
 }
 
 func buildAllowedResponse(t *testing.T, filename string) string {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
-	body, err := os.ReadFile(filepath.Join(testhelper.TestRoot, filename))
+	body, err := os.ReadFile(filepath.Join(testRoot, filename))
 	require.NoError(t, err)
 
 	response := strings.Replace(string(body), "GITALY_REPOSITORY", testRepo, 1)

internal/config/config_test.go

@@ -63,9 +63,9 @@ func TestCustomPrometheusMetrics(t *testing.T) {
 }
 
 func TestNewFromDir(t *testing.T) {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
-	cfg, err := NewFromDir(testhelper.TestRoot)
+	cfg, err := NewFromDir(testRoot)
 	require.NoError(t, err)
 
 	require.Equal(t, 10*time.Second, time.Duration(cfg.Server.GracePeriod))

internal/gitlabnet/accessverifier/client_test.go

@@ -56,7 +56,8 @@ func buildExpectedResponse(who string) *Response {
 }
 
 func TestSuccessfulResponses(t *testing.T) {
-	okResponse := testResponse{body: responseBody(t, "allowed.json"), status: http.StatusOK}
+	testRoot := testhelper.PrepareTestRootDir(t)
+	okResponse := testResponse{body: responseBody(t, testRoot, "allowed.json"), status: http.StatusOK}
 	client := setup(t,
 		map[string]testResponse{"first": okResponse, "test@TEST.TEST": okResponse},
 		map[string]testResponse{"1": okResponse},
@@ -94,9 +95,10 @@ func TestSuccessfulResponses(t *testing.T) {
 }
 
 func TestGeoPushGetCustomAction(t *testing.T) {
+	testRoot := testhelper.PrepareTestRootDir(t)
 	client := setup(t, map[string]testResponse{
 		"custom": {
-			body:   responseBody(t, "allowed_with_push_payload.json"),
+			body:   responseBody(t, testRoot, "allowed_with_push_payload.json"),
 			status: 300,
 		},
 	}, nil)
@@ -123,9 +125,10 @@ func TestGeoPushGetCustomAction(t *testing.T) {
 }
 
 func TestGeoPullGetCustomAction(t *testing.T) {
+	testRoot := testhelper.PrepareTestRootDir(t)
 	client := setup(t, map[string]testResponse{
 		"custom": {
-			body:   responseBody(t, "allowed_with_pull_payload.json"),
+			body:   responseBody(t, testRoot, "allowed_with_pull_payload.json"),
 			status: 300,
 		},
 	}, nil)
@@ -241,10 +244,9 @@ type testResponse struct {
 	status int
 }
 
-func responseBody(t *testing.T, name string) []byte {
+func responseBody(t *testing.T, testRoot, name string) []byte {
 	t.Helper()
-	testhelper.PrepareTestRootDir(t)
-	body, err := os.ReadFile(path.Join(testhelper.TestRoot, "responses", name))
+	body, err := os.ReadFile(path.Join(testRoot, "responses", name))
 	require.NoError(t, err)
 	return body
 }

internal/sshd/server_config_test.go

@@ -31,20 +31,20 @@ func TestNewServerConfigWithoutHosts(t *testing.T) {
 }
 
 func TestHostKeyAndCerts(t *testing.T) {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
 	srvCfg := config.ServerConfig{
 		Listen:                  "127.0.0.1",
 		ConcurrentSessionsLimit: 1,
 		HostKeyFiles: []string{
-			path.Join(testhelper.TestRoot, "certs/valid/server.key"),
+			path.Join(testRoot, "certs/valid/server.key"),
 		},
 		HostCertFiles: []string{
-			path.Join(testhelper.TestRoot, "certs/valid/server-cert.pub"),
-			path.Join(testhelper.TestRoot, "certs/valid/server2-cert.pub"),
-			path.Join(testhelper.TestRoot, "certs/invalid/server-cert.pub"),
-			path.Join(testhelper.TestRoot, "certs/invalid-path.key"),
-			path.Join(testhelper.TestRoot, "certs/invalid/server.crt"),
+			path.Join(testRoot, "certs/valid/server-cert.pub"),
+			path.Join(testRoot, "certs/valid/server2-cert.pub"),
+			path.Join(testRoot, "certs/invalid/server-cert.pub"),
+			path.Join(testRoot, "certs/invalid-path.key"),
+			path.Join(testRoot, "certs/invalid/server.crt"),
 		},
 	}
 
@@ -57,7 +57,7 @@ func TestHostKeyAndCerts(t *testing.T) {
 	require.Len(t, cfg.hostKeyToCertMap, 1)
 
 	// Check that the entry is pointing to the server's public key
-	data, err := os.ReadFile(path.Join(testhelper.TestRoot, "certs/valid/server.pub"))
+	data, err := os.ReadFile(path.Join(testRoot, "certs/valid/server.pub"))
 	require.NoError(t, err)
 
 	publicKey, _, _, _, err := ssh.ParseAuthorizedKey(data)
@@ -77,7 +77,7 @@ func TestFailedAuthorizedKeysClient(t *testing.T) {
 }
 
 func TestUserKeyHandling(t *testing.T) {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
 	validRSAKey := rsaPublicKey(t)
 
@@ -101,9 +101,9 @@ func TestUserKeyHandling(t *testing.T) {
 		Listen:                  "127.0.0.1",
 		ConcurrentSessionsLimit: 1,
 		HostKeyFiles: []string{
-			path.Join(testhelper.TestRoot, "certs/valid/server.key"),
-			path.Join(testhelper.TestRoot, "certs/invalid-path.key"),
-			path.Join(testhelper.TestRoot, "certs/invalid/server.crt"),
+			path.Join(testRoot, "certs/valid/server.key"),
+			path.Join(testRoot, "certs/invalid-path.key"),
+			path.Join(testRoot, "certs/invalid/server.crt"),
 		},
 	}
 
@@ -154,7 +154,7 @@ func TestUserKeyHandling(t *testing.T) {
 }
 
 func TestUserCertificateHandling(t *testing.T) {
-	testhelper.PrepareTestRootDir(t)
+	testRoot := testhelper.PrepareTestRootDir(t)
 
 	validUserCert := userCert(t, ssh.UserCert, time.Now().Add(time.Hour))
 
@@ -178,9 +178,9 @@ func TestUserCertificateHandling(t *testing.T) {
 		Listen:                  "127.0.0.1",
 		ConcurrentSessionsLimit: 1,
 		HostKeyFiles: []string{
-			path.Join(testhelper.TestRoot, "certs/valid/server.key"),
-			path.Join(testhelper.TestRoot, "certs/invalid-path.key"),
-			path.Join(testhelper.TestRoot, "certs/invalid/server.crt"),
+			path.Join(testRoot, "certs/valid/server.key"),
+			path.Join(testRoot, "certs/invalid-path.key"),
+			path.Join(testRoot, "certs/invalid/server.crt"),
 		},
 	}
 

internal/sshd/sshd_test.go

@@ -32,9 +32,9 @@ var (
 )
 
 func TestListenAndServe(t *testing.T) {
-	s := setupServer(t)
+	s, testRoot := setupServer(t)
 
-	client, err := ssh.Dial("tcp", serverUrl, clientConfig(t))
+	client, err := ssh.Dial("tcp", serverUrl, clientConfig(t, testRoot))
 	require.NoError(t, err)
 	defer client.Close()
 
@@ -43,7 +43,7 @@ func TestListenAndServe(t *testing.T) {
 
 	holdSession(t, client)
 
-	_, err = ssh.Dial("tcp", serverUrl, clientConfig(t))
+	_, err = ssh.Dial("tcp", serverUrl, clientConfig(t, testRoot))
 	require.Equal(t, err.Error(), "dial tcp 127.0.0.1:50000: connect: connection refused")
 
 	client.Close()
@@ -52,6 +52,8 @@ func TestListenAndServe(t *testing.T) {
 }
 
 func TestListenAndServe_proxyProtocolEnabled(t *testing.T) {
+	testRoot := testhelper.PrepareTestRootDir(t)
+
 	target, err := net.ResolveTCPAddr("tcp", serverUrl)
 	require.NoError(t, err)
 
@@ -193,7 +195,7 @@ func TestListenAndServe_proxyProtocolEnabled(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			sshConn, sshChans, sshRequs, err := ssh.NewClientConn(conn, serverUrl, clientConfig(t))
+			sshConn, sshChans, sshRequs, err := ssh.NewClientConn(conn, serverUrl, clientConfig(t, testRoot))
 			if sshConn != nil {
 				defer sshConn.Close()
 			}
@@ -213,9 +215,9 @@ func TestListenAndServe_proxyProtocolEnabled(t *testing.T) {
 }
 
 func TestCorrelationId(t *testing.T) {
-	setupServer(t)
+	_, testRoot := setupServer(t)
 
-	client, err := ssh.Dial("tcp", serverUrl, clientConfig(t))
+	client, err := ssh.Dial("tcp", serverUrl, clientConfig(t, testRoot))
 	require.NoError(t, err)
 	defer client.Close()
 
@@ -223,7 +225,7 @@ func TestCorrelationId(t *testing.T) {
 
 	previousCorrelationId := correlationId
 
-	client, err = ssh.Dial("tcp", serverUrl, clientConfig(t))
+	client, err = ssh.Dial("tcp", serverUrl, clientConfig(t, testRoot))
 	require.NoError(t, err)
 	defer client.Close()
 
@@ -270,9 +272,9 @@ func TestLivenessProbe(t *testing.T) {
 }
 
 func TestInvalidClientConfig(t *testing.T) {
-	setupServer(t)
+	_, testRoot := setupServer(t)
 
-	cfg := clientConfig(t)
+	cfg := clientConfig(t, testRoot)
 	cfg.User = "unknown"
 	_, err := ssh.Dial("tcp", serverUrl, cfg)
 	require.Error(t, err)
@@ -291,12 +293,12 @@ func TestClosingHangedConnections(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	s := setupServerWithContext(t, nil, ctx)
+	s, testRoot := setupServerWithContext(t, nil, ctx)
 
 	unauthenticatedRequestStatus := make(chan string)
 	completed := make(chan bool)
 
-	clientCfg := clientConfig(t)
+	clientCfg := clientConfig(t, testRoot)
 	clientCfg.HostKeyCallback = func(_ string, _ net.Addr, _ ssh.PublicKey) error {
 		unauthenticatedRequestStatus <- "authentication-started"
 		<-completed // Wait infinitely
@@ -322,12 +324,12 @@ func TestLoginGraceTime(t *testing.T) {
 			LoginGraceTime: config.YamlDuration(50 * time.Millisecond),
 		},
 	}
-	s := setupServerWithConfig(t, cfg)
+	s, testRoot := setupServerWithConfig(t, cfg)
 
 	unauthenticatedRequestStatus := make(chan string)
 	completed := make(chan bool)
 
-	clientCfg := clientConfig(t)
+	clientCfg := clientConfig(t, testRoot)
 	clientCfg.HostKeyCallback = func(_ string, _ net.Addr, _ ssh.PublicKey) error {
 		unauthenticatedRequestStatus <- "authentication-started"
 		<-completed // Wait infinitely
@@ -374,21 +376,23 @@ func TestExtractMetaDataFromNilContext(t *testing.T) {
 	require.Equal(t, command.LogData{}, data)
 }
 
-func setupServer(t *testing.T) *Server {
+func setupServer(t *testing.T) (*Server, string) {
 	t.Helper()
 
 	return setupServerWithConfig(t, nil)
 }
 
-func setupServerWithConfig(t *testing.T, cfg *config.Config) *Server {
+func setupServerWithConfig(t *testing.T, cfg *config.Config) (*Server, string) {
 	t.Helper()
 
 	return setupServerWithContext(t, cfg, context.Background())
 }
 
-func setupServerWithContext(t *testing.T, cfg *config.Config, ctx context.Context) *Server {
+func setupServerWithContext(t *testing.T, cfg *config.Config, ctx context.Context) (*Server, string) {
 	t.Helper()
 
+	testRoot := testhelper.PrepareTestRootDir(t)
+
 	requests := []testserver.TestRequestHandler{
 		{
 			Path: "/api/v4/internal/authorized_keys",
@@ -411,8 +415,6 @@ func setupServerWithContext(t *testing.T, cfg *config.Config, ctx context.Contex
 		},
 	}
 
-	testhelper.PrepareTestRootDir(t)
-
 	url := testserver.StartSocketHttpServer(t, requests)
 
 	if cfg == nil {
@@ -425,25 +427,26 @@ func setupServerWithContext(t *testing.T, cfg *config.Config, ctx context.Contex
 	cfg.User = user
 	cfg.Server.Listen = serverUrl
 	cfg.Server.ConcurrentSessionsLimit = 1
-	cfg.Server.HostKeyFiles = []string{path.Join(testhelper.TestRoot, "certs/valid/server.key")}
+	cfg.Server.HostKeyFiles = []string{path.Join(testRoot, "certs/valid/server.key")}
 
 	s, err := NewServer(cfg)
 	require.NoError(t, err)
 
 	go func() { require.NoError(t, s.ListenAndServe(ctx)) }()
+	// NOTE: Changing the below to { require.NoError(t, s.Shutdown()) } results in failed tests
 	t.Cleanup(func() { s.Shutdown() })
 
 	verifyStatus(t, s, StatusReady)
 
-	return s
+	return s, testRoot
 }
 
-func clientConfig(t *testing.T) *ssh.ClientConfig {
-	keyRaw, err := os.ReadFile(path.Join(testhelper.TestRoot, "certs/valid/server_authorized_key"))
+func clientConfig(t *testing.T, testRoot string) *ssh.ClientConfig {
+	keyRaw, err := os.ReadFile(path.Join(testRoot, "certs/valid/server_authorized_key"))
 	pKey, _, _, _, err := ssh.ParseAuthorizedKey(keyRaw)
 	require.NoError(t, err)
 
-	key, err := os.ReadFile(path.Join(testhelper.TestRoot, "certs/client/key.pem"))
+	key, err := os.ReadFile(path.Join(testRoot, "certs/client/key.pem"))
 	require.NoError(t, err)
 	signer, err := ssh.ParsePrivateKey(key)
 	require.NoError(t, err)

internal/testhelper/testhelper.go

@@ -11,10 +11,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var (
-	TestRoot, _ = os.MkdirTemp("", "test-gitlab-shell")
-)
-
 func TempEnv(env map[string]string) func() {
 	var original = make(map[string]string)
 	for key, value := range env {
@@ -29,24 +25,25 @@ func TempEnv(env map[string]string) func() {
 	}
 }
 
-func PrepareTestRootDir(t *testing.T) {
+func PrepareTestRootDir(t *testing.T) string {
 	t.Helper()
 
-	require.NoError(t, os.MkdirAll(TestRoot, 0700))
+	testRoot := t.TempDir()
+	t.Cleanup(func() { require.NoError(t, os.RemoveAll(testRoot)) })
 
-	t.Cleanup(func() { require.NoError(t, os.RemoveAll(TestRoot)) })
-
-	require.NoError(t, copyTestData())
+	require.NoError(t, copyTestData(testRoot))
 
 	oldWd, err := os.Getwd()
 	require.NoError(t, err)
 
 	t.Cleanup(func() { os.Chdir(oldWd) })
 
-	require.NoError(t, os.Chdir(TestRoot))
+	require.NoError(t, os.Chdir(testRoot))
+
+	return testRoot
 }
 
-func copyTestData() error {
+func copyTestData(testRoot string) error {
 	testDataDir, err := getTestDataDir()
 	if err != nil {
 		return err
@@ -54,7 +51,7 @@ func copyTestData() error {
 
 	testdata := path.Join(testDataDir, "testroot")
 
-	return copy.Copy(testdata, TestRoot)
+	return copy.Copy(testdata, testRoot)
 }
 
 func getTestDataDir() (string, error) {