diff options
author | James Liu <jliu@gitlab.com> | 2024-01-08 03:33:26 +0300 |
---|---|---|
committer | GitLab <noreply@gitlab.com> | 2024-01-08 03:33:26 +0300 |
commit | 0ffc495be9bb5a8f4ff60764b545443d54210e56 (patch) | |
tree | bf305db5438046531766588e0fffc5875a0eba59 | |
parent | 7977654d7112e2a28ecc1ead38f182f3a0e9e510 (diff) | |
parent | e95f3408ddd61a77beb276a0ea90a116dfba9e22 (diff) |
Merge branch 'smh-fix-incorrect-error-report' into 'master'
Report stat failures correctly when creating a repository
Closes #5751
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6597
Merged-by: James Liu <jliu@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: James Liu <jliu@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
-rw-r--r-- | internal/gitaly/repoutil/create.go | 17 | ||||
-rw-r--r-- | internal/gitaly/repoutil/create_test.go | 66 |
2 files changed, 70 insertions, 13 deletions
diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go index 83eb2d847..1bc49ab7c 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -3,6 +3,7 @@ package repoutil import ( "bytes" "context" + "errors" "fmt" "io" "io/fs" @@ -81,8 +82,12 @@ func Create( // The repository must not exist on disk already, or otherwise we won't be able to // create it with atomic semantics. - if _, err := os.Stat(targetPath); !os.IsNotExist(err) { - return structerr.NewAlreadyExists("repository exists already") + if _, err := os.Stat(targetPath); !errors.Is(err, fs.ErrNotExist) { + if err == nil { + return structerr.NewAlreadyExists("repository exists already") + } + + return fmt.Errorf("pre-lock stat: %w", err) } newRepo, newRepoDir, err := tempdir.NewRepository(ctx, repository.GetStorageName(), logger, locator) @@ -216,8 +221,12 @@ func Create( // and seeded our temporary repository. While we would notice this at the point of moving // the repository into place, we want to be as sure as possible that the action will succeed // previous to the first transactional vote. - if _, err := os.Stat(targetPath); !os.IsNotExist(err) { - return structerr.NewAlreadyExists("repository exists already") + if _, err := os.Stat(targetPath); !errors.Is(err, fs.ErrNotExist) { + if err == nil { + return structerr.NewAlreadyExists("repository exists already") + } + + return fmt.Errorf("post-lock stat: %w", err) } if err := transaction.VoteOnContext(ctx, txManager, vote, voting.Prepared); err != nil { diff --git a/internal/gitaly/repoutil/create_test.go b/internal/gitaly/repoutil/create_test.go index e449ff448..bed75c705 100644 --- a/internal/gitaly/repoutil/create_test.go +++ b/internal/gitaly/repoutil/create_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" + "syscall" "testing" "github.com/stretchr/testify/require" @@ -54,6 +56,14 @@ func TestCreate(t *testing.T) { } } + type requireErrorFunc func(*testing.T, config.Cfg, string, error) + + equalError := func(expected error) requireErrorFunc { + return func(t *testing.T, _ config.Cfg, _ string, actual error) { + require.Equal(t, actual, expected) + } + } + for _, tc := range []struct { desc string opts []CreateOption @@ -67,7 +77,7 @@ func TestCreate(t *testing.T) { realRepoPath string, ) transactional bool - expectedErr error + requireError requireErrorFunc }{ { desc: "no seeding", @@ -111,7 +121,7 @@ func TestCreate(t *testing.T) { require.NoDirExists(t, realRepoPath) require.NoDirExists(t, tempRepoPath) }, - expectedErr: errors.New("some error"), + requireError: equalError(errors.New("some error")), }, { desc: "preexisting directory", @@ -128,7 +138,38 @@ func TestCreate(t *testing.T) { requireFullRepackTimestampExists(t, realRepoPath, false) }, - expectedErr: structerr.NewAlreadyExists("repository exists already"), + requireError: equalError(structerr.NewAlreadyExists("repository exists already")), + }, + { + desc: "pre-lock stat fails", + setup: func(t *testing.T, repo *gitalypb.Repository, repoPath string) { + require.NoError(t, os.MkdirAll(repoPath, perm.PublicDir)) + parentDir := filepath.Dir(repoPath) + // Drop permissions to trigger a stat failure. + require.NoError(t, os.Chmod(parentDir, 0)) + // Restore the permissions so the directory can be cleaned up. + t.Cleanup(func() { require.NoError(t, os.Chmod(parentDir, perm.PublicDir)) }) + }, + verify: func(t *testing.T, tempRepo *gitalypb.Repository, tempRepoPath string, realRepo *gitalypb.Repository, realRepoPath string) { + // Restore the permissions so the below checks work. + require.NoError(t, os.Chmod(filepath.Dir(realRepoPath), perm.PublicDir)) + + require.NoDirExists(t, tempRepoPath) + + require.DirExists(t, realRepoPath) + dirEntries, err := os.ReadDir(realRepoPath) + require.NoError(t, err) + require.Empty(t, dirEntries, "directory should not have been modified") + + requireFullRepackTimestampExists(t, realRepoPath, false) + }, + requireError: func(t *testing.T, cfg config.Cfg, relativePath string, actual error) { + require.Equal(t, fmt.Errorf("pre-lock stat: %w", &fs.PathError{ + Op: "stat", + Path: filepath.Join(cfg.Storages[0].Path, relativePath), + Err: syscall.EACCES, + }), actual) + }, }, { desc: "locked", @@ -147,7 +188,7 @@ func TestCreate(t *testing.T) { requireFullRepackTimestampExists(t, realRepoPath, false) }, - expectedErr: fmt.Errorf("locking repository: %w", safe.ErrFileAlreadyLocked), + requireError: equalError(fmt.Errorf("locking repository: %w", safe.ErrFileAlreadyLocked)), }, { desc: "successful transaction", @@ -180,7 +221,7 @@ func TestCreate(t *testing.T) { require.NoDirExists(t, tempRepoPath) require.NoDirExists(t, realRepoPath) }, - expectedErr: structerr.NewFailedPrecondition("preparatory vote: %w", errors.New("vote failed")), + requireError: equalError(structerr.NewFailedPrecondition("preparatory vote: %w", errors.New("vote failed"))), }, { desc: "failing post-commit vote", @@ -203,7 +244,7 @@ func TestCreate(t *testing.T) { requireFullRepackTimestampExists(t, realRepoPath, true) }, - expectedErr: structerr.NewFailedPrecondition("committing vote: %w", errors.New("vote failed")), + requireError: equalError(structerr.NewFailedPrecondition("committing vote: %w", errors.New("vote failed"))), }, { desc: "voting happens after lock", @@ -227,7 +268,7 @@ func TestCreate(t *testing.T) { require.NoDirExists(t, tempRepoPath) require.NoDirExists(t, realRepoPath) }, - expectedErr: fmt.Errorf("locking repository: %w", errors.New("file already locked")), + requireError: equalError(fmt.Errorf("locking repository: %w", errors.New("file already locked"))), }, { desc: "vote is deterministic", @@ -345,7 +386,8 @@ func TestCreate(t *testing.T) { } var tempRepo *gitalypb.Repository - require.Equal(t, tc.expectedErr, Create(ctx, logger, locator, gitCmdFactory, txManager, repoCounter, repo, func(tr *gitalypb.Repository) error { + + err = Create(ctx, logger, locator, gitCmdFactory, txManager, repoCounter, repo, func(tr *gitalypb.Repository) error { tempRepo = tr // The temporary repository must have been created in Gitaly's @@ -365,7 +407,13 @@ func TestCreate(t *testing.T) { require.Equal(t, "true", text.ChompBytes(isBareRepo)) return nil - }, tc.opts...)) + }, tc.opts...) + + if tc.requireError != nil { + tc.requireError(t, cfg, repo.RelativePath, err) + } else { + require.NoError(t, err) + } var tempRepoPath string if tempRepo != nil { |