Merge branch 'ps-locator' into 'master'

Locator uses helper for errors creation

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4901

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Pavlo Strokov <pstrokov@gitlab.com>

5 files changed, 268 insertions, 16 deletions

diff --git a/internal/gitaly/config/locator.go b/internal/gitaly/config/locator.go
index 8476bfa64..786eb2d45 100644
--- a/internal/gitaly/config/locator.go
+++ b/internal/gitaly/config/locator.go
@@ -6,8 +6,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 )
 
 const (
@@ -45,15 +44,11 @@ func (l *configLocator) GetRepoPath(repo repository.GitRepo) (string, error) {
 		return "", err
 	}
 
-	if repoPath == "" {
-		return "", status.Errorf(codes.InvalidArgument, "GetRepoPath: empty repo path")
-	}
-
 	if storage.IsGitDirectory(repoPath) {
 		return repoPath, nil
 	}
 
-	return "", status.Errorf(codes.NotFound, "GetRepoPath: not a git repository: %q", repoPath)
+	return "", helper.ErrNotFoundf("GetRepoPath: not a git repository: %q", repoPath)
 }
 
 // GetPath returns the path of the repo passed as first argument. An error is
@@ -67,19 +62,19 @@ func (l *configLocator) GetPath(repo repository.GitRepo) (string, error) {
 
 	if _, err := os.Stat(storagePath); err != nil {
 		if os.IsNotExist(err) {
-			return "", status.Errorf(codes.NotFound, "GetPath: does not exist: %v", err)
+			return "", helper.ErrNotFoundf("GetPath: does not exist: %v", err)
 		}
-		return "", status.Errorf(codes.Internal, "GetPath: storage path: %v", err)
+		return "", helper.ErrInternalf("GetPath: storage path: %v", err)
 	}
 
 	relativePath := repo.GetRelativePath()
 	if len(relativePath) == 0 {
-		err := status.Errorf(codes.InvalidArgument, "GetPath: relative path missing from %+v", repo)
+		err := helper.ErrInvalidArgumentf("GetPath: relative path missing")
 		return "", err
 	}
 
 	if _, err := storage.ValidateRelativePath(storagePath, relativePath); err != nil {
-		return "", status.Errorf(codes.InvalidArgument, "GetRepoPath: %s", err)
+		return "", helper.ErrInvalidArgumentf("GetRepoPath: %s", err)
 	}
 
 	return filepath.Join(storagePath, relativePath), nil
@@ -90,7 +85,7 @@ func (l *configLocator) GetPath(repo repository.GitRepo) (string, error) {
 func (l *configLocator) GetStorageByName(storageName string) (string, error) {
 	storagePath, ok := l.conf.StoragePath(storageName)
 	if !ok {
-		return "", status.Errorf(codes.InvalidArgument, "GetStorageByName: no such storage: %q", storageName)
+		return "", helper.ErrInvalidArgumentf("GetStorageByName: no such storage: %q", storageName)
 	}
 
 	return storagePath, nil
@@ -114,7 +109,7 @@ func (l *configLocator) TempDir(storageName string) (string, error) {
 func (l *configLocator) getPath(storageName, prefix string) (string, error) {
 	storagePath, ok := l.conf.StoragePath(storageName)
 	if !ok {
-		return "", status.Errorf(codes.InvalidArgument, "%s dir: no such storage: %q",
+		return "", helper.ErrInvalidArgumentf("%s dir: no such storage: %q",
 			filepath.Base(prefix), storageName)
 	}
 
diff --git a/internal/gitaly/config/locator_test.go b/internal/gitaly/config/locator_test.go
new file mode 100644
index 000000000..4260cda54
--- /dev/null
+++ b/internal/gitaly/config/locator_test.go
@@ -0,0 +1,257 @@
+//go:build !gitaly_test_sha256
+
+package config_test
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
+)
+
+func TestConfigLocator_GetRepoPath(t *testing.T) {
+	t.Parallel()
+	const storageName = "exists"
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t, testcfg.WithStorages(storageName, "removed"))
+	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, nil, setup.RegisterAll)
+	repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
+	locator := config.NewLocator(cfg)
+
+	t.Run("proper repository path", func(t *testing.T) {
+		if testhelper.IsPraefectEnabled() {
+			repo.RelativePath = strings.TrimPrefix(repoPath, cfg.Storages[0].Path)
+		}
+		path, err := locator.GetRepoPath(repo)
+		require.NoError(t, err)
+		require.Equal(t, filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath()), path)
+	})
+
+	// The storage name still present in the storages list, but not on the disk.
+	require.NoError(t, os.RemoveAll(cfg.Storages[1].Path))
+
+	// The repository path exists on the disk, but it is not a git repository.
+	const notRepositoryFolder = "not-a-git-repo"
+	require.NoError(t, os.MkdirAll(filepath.Join(cfg.Storages[0].Path, notRepositoryFolder), 0o755))
+
+	for _, tc := range []struct {
+		desc   string
+		repo   *gitalypb.Repository
+		expErr error
+	}{
+		{
+			desc:   "storage is empty",
+			repo:   &gitalypb.Repository{RelativePath: repo.RelativePath},
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`),
+		},
+		{
+			desc:   "unknown storage",
+			repo:   &gitalypb.Repository{StorageName: "invalid", RelativePath: repo.RelativePath},
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "invalid"`),
+		},
+		{
+			desc:   "storage doesn't exist on disk",
+			repo:   &gitalypb.Repository{StorageName: cfg.Storages[1].Name, RelativePath: repo.RelativePath},
+			expErr: helper.ErrNotFoundf(`GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
+		},
+		{
+			desc:   "relative path is empty",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: ""},
+			expErr: helper.ErrInvalidArgumentf("GetPath: relative path missing"),
+		},
+		{
+			desc:   "unknown relative path",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "invalid"},
+			expErr: helper.ErrNotFoundf(`GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, "invalid")),
+		},
+		{
+			desc:   "path exists but not a git repository",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: notRepositoryFolder},
+			expErr: helper.ErrNotFoundf(`GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, notRepositoryFolder)),
+		},
+		{
+			desc:   "relative path escapes parent folder",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "../.."},
+			expErr: helper.ErrInvalidArgumentf(`GetRepoPath: relative path escapes root directory`),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			locator := config.NewLocator(cfg)
+			_, err := locator.GetRepoPath(tc.repo)
+			require.Equal(t, tc.expErr, err)
+		})
+	}
+}
+
+func TestConfigLocator_GetPath(t *testing.T) {
+	t.Parallel()
+	const storageName = "exists"
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t, testcfg.WithStorages(storageName, "removed"))
+	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, nil, setup.RegisterAll)
+	repo, _ := gittest.CreateRepository(t, ctx, cfg)
+
+	// The storage name still present in the storages list, but not on the disk.
+	require.NoError(t, os.RemoveAll(cfg.Storages[1].Path))
+
+	// The repository path exists on the disk, but it is not a git repository.
+	const notRepositoryFolder = "not-a-git-repo"
+	require.NoError(t, os.MkdirAll(filepath.Join(cfg.Storages[0].Path, notRepositoryFolder), 0o755))
+
+	for _, tc := range []struct {
+		desc string
+		repo *gitalypb.Repository
+		path string
+	}{
+		{
+			desc: "proper repository path",
+			repo: repo,
+			path: filepath.Join(cfg.Storages[0].Path, repo.RelativePath),
+		},
+		{
+			desc: "path doesn't exist",
+			repo: &gitalypb.Repository{StorageName: storageName, RelativePath: "doesnt/exist"},
+			path: filepath.Join(cfg.Storages[0].Path, "doesnt/exist"),
+		},
+		{
+			desc: "path exists but not a git repository",
+			repo: &gitalypb.Repository{StorageName: storageName, RelativePath: notRepositoryFolder},
+			path: filepath.Join(cfg.Storages[0].Path, notRepositoryFolder),
+		},
+		{
+			desc: "relative path includes travels",
+			repo: &gitalypb.Repository{StorageName: storageName, RelativePath: "some/../other"},
+			path: filepath.Join(cfg.Storages[0].Path, "other"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			locator := config.NewLocator(cfg)
+			path, err := locator.GetPath(tc.repo)
+			require.NoError(t, err)
+			require.Equal(t, tc.path, path)
+		})
+	}
+
+	for _, tc := range []struct {
+		desc   string
+		repo   *gitalypb.Repository
+		expErr error
+	}{
+		{
+			desc:   "storage is empty",
+			repo:   &gitalypb.Repository{RelativePath: repo.RelativePath},
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`),
+		},
+		{
+			desc:   "unknown storage",
+			repo:   &gitalypb.Repository{StorageName: "invalid", RelativePath: repo.RelativePath},
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "invalid"`),
+		},
+		{
+			desc:   "storage doesn't exist on disk",
+			repo:   &gitalypb.Repository{StorageName: cfg.Storages[1].Name, RelativePath: repo.RelativePath},
+			expErr: helper.ErrNotFoundf(`GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
+		},
+		{
+			desc:   "relative path is empty",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: ""},
+			expErr: helper.ErrInvalidArgumentf("GetPath: relative path missing"),
+		},
+		{
+			desc:   "relative path escapes parent folder",
+			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "../.."},
+			expErr: helper.ErrInvalidArgumentf(`GetRepoPath: relative path escapes root directory`),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			locator := config.NewLocator(cfg)
+			_, err := locator.GetPath(tc.repo)
+			require.Equal(t, tc.expErr, err)
+		})
+	}
+}
+
+func TestConfigLocator_CacheDir(t *testing.T) {
+	t.Parallel()
+	const storageName = "exists"
+	cfg := testcfg.Build(t, testcfg.WithStorages(storageName, "removed"))
+	locator := config.NewLocator(cfg)
+
+	t.Run("storage exists", func(t *testing.T) {
+		path, err := locator.CacheDir(storageName)
+		require.NoError(t, err)
+		require.Equal(t, path, filepath.Join(cfg.Storages[0].Path, "+gitaly/cache"))
+	})
+
+	t.Run("storage doesn't exist on disk", func(t *testing.T) {
+		require.NoError(t, os.RemoveAll(cfg.Storages[1].Path))
+		path, err := locator.CacheDir(cfg.Storages[1].Name)
+		require.NoError(t, err)
+		require.Equal(t, filepath.Join(cfg.Storages[1].Path, "+gitaly/cache"), path)
+	})
+
+	t.Run("unknown storage", func(t *testing.T) {
+		_, err := locator.CacheDir("unknown")
+		require.Equal(t, helper.ErrInvalidArgumentf(`cache dir: no such storage: "unknown"`), err)
+	})
+}
+
+func TestConfigLocator_StateDir(t *testing.T) {
+	t.Parallel()
+	const storageName = "exists"
+	cfg := testcfg.Build(t, testcfg.WithStorages(storageName, "removed"))
+	locator := config.NewLocator(cfg)
+
+	t.Run("storage exists", func(t *testing.T) {
+		path, err := locator.StateDir(storageName)
+		require.NoError(t, err)
+		require.Equal(t, path, filepath.Join(cfg.Storages[0].Path, "+gitaly/state"))
+	})
+
+	t.Run("storage doesn't exist on disk", func(t *testing.T) {
+		require.NoError(t, os.RemoveAll(cfg.Storages[1].Path))
+		path, err := locator.StateDir(cfg.Storages[1].Name)
+		require.NoError(t, err)
+		require.Equal(t, filepath.Join(cfg.Storages[1].Path, "+gitaly/state"), path)
+	})
+
+	t.Run("unknown storage", func(t *testing.T) {
+		_, err := locator.StateDir("unknown")
+		require.Equal(t, helper.ErrInvalidArgumentf(`state dir: no such storage: "unknown"`), err)
+	})
+}
+
+func TestConfigLocator_TempDir(t *testing.T) {
+	t.Parallel()
+	const storageName = "exists"
+	cfg := testcfg.Build(t, testcfg.WithStorages(storageName, "removed"))
+	locator := config.NewLocator(cfg)
+
+	t.Run("storage exists", func(t *testing.T) {
+		path, err := locator.TempDir(storageName)
+		require.NoError(t, err)
+		require.Equal(t, path, filepath.Join(cfg.Storages[0].Path, "+gitaly/tmp"))
+	})
+
+	t.Run("storage doesn't exist on disk", func(t *testing.T) {
+		require.NoError(t, os.RemoveAll(cfg.Storages[1].Path))
+		path, err := locator.TempDir(cfg.Storages[1].Name)
+		require.NoError(t, err)
+		require.Equal(t, filepath.Join(cfg.Storages[1].Path, "+gitaly/tmp"), path)
+	})
+
+	t.Run("unknown storage", func(t *testing.T) {
+		_, err := locator.TempDir("unknown")
+		require.Equal(t, helper.ErrInvalidArgumentf(`tmp dir: no such storage: "unknown"`), err)
+	})
+}
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index ef67dbbcc..87c5098fe 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -67,7 +67,7 @@ func TestReceivePack_validation(t *testing.T) {
 					return helper.ErrInvalidArgumentf("repo scoped: invalid Repository")
 				}
 
-				return helper.ErrInvalidArgumentf("GetPath: relative path missing from storage_name:\"default\"")
+				return helper.ErrInvalidArgumentf("GetPath: relative path missing")
 			}(),
 		},
 		{
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 31520b8fc..9e4bd4833 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -445,7 +445,7 @@ func TestUploadPack_validation(t *testing.T) {
 				if testhelper.IsPraefectEnabled() {
 					return helper.ErrInvalidArgumentf("repo scoped: invalid Repository")
 				}
-				return helper.ErrInvalidArgumentf("GetPath: relative path missing from storage_name:%q", "default")
+				return helper.ErrInvalidArgumentf("GetPath: relative path missing")
 			}(),
 		},
 		{
diff --git a/internal/tempdir/clean_test.go b/internal/tempdir/clean_test.go
index c6899aef8..1c33a7dbc 100644
--- a/internal/tempdir/clean_test.go
+++ b/internal/tempdir/clean_test.go
@@ -87,7 +87,7 @@ func TestCleanNoStorageExists(t *testing.T) {
 	locator := config.NewLocator(cfg)
 
 	err := clean(locator, config.Storage{Name: "does-not-exist", Path: "/something"})
-	require.EqualError(t, err, "temporary dir: rpc error: code = InvalidArgument desc = tmp dir: no such storage: \"does-not-exist\"")
+	require.EqualError(t, err, "temporary dir: tmp dir: no such storage: \"does-not-exist\"")
 }
 
 type mockLocator struct {