From e95f3408ddd61a77beb276a0ea90a116dfba9e22 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Wed, 3 Jan 2024 16:11:29 +0200 Subject: Report stat failures correctly when creating a repository Gitaly currently reports always a 'repository exists already' when statting a repository during creation fails. This is wrong and confusing as we hide the actual cause of the error. This commit fixes the error check to report failures of the stat call correctly. Changelog: fixed --- internal/gitaly/repoutil/create.go | 17 +++++++-- 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 { -- cgit v1.2.3