diff options
| -rw-r--r-- | CHANGELOG.md | 2 | ||||
| -rw-r--r-- | internal/rubyserver/proxy.go | 19 | ||||
| -rw-r--r-- | internal/rubyserver/rubyserver_test.go | 57 | ||||
| -rw-r--r-- | internal/service/repository/create.go | 2 | ||||
| -rw-r--r-- | internal/service/repository/testhelper_test.go | 1 |
5 files changed, 67 insertions, 14 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d2c9fb94..bfde4fd4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ UNRELEASED +- Check repo existence before passing to gitaly-ruby + https://gitlab.com/gitlab-org/gitaly/merge_requests/528 - Implement ExtractCommitSignature RPC https://gitlab.com/gitlab-org/gitaly/merge_requests/521 - Update Gitlab::Git vendoring to b10ea6e386a025759aca5e9ef0d23931e77d1012 diff --git a/internal/rubyserver/proxy.go b/internal/rubyserver/proxy.go index 7364e9e5f..29aa97814 100644 --- a/internal/rubyserver/proxy.go +++ b/internal/rubyserver/proxy.go @@ -23,14 +23,31 @@ const ( repoAltDirsHeader = "gitaly-repo-alt-dirs" ) +// SetHeadersWithoutRepoCheck adds headers that tell gitaly-ruby the full +// path to the repository. It is not an error if the repository does not +// yet exist. This can be used on RPC calls that will create a +// repository. +func SetHeadersWithoutRepoCheck(ctx context.Context, repo *pb.Repository) (context.Context, error) { + return setHeaders(ctx, repo, false) +} + // SetHeaders adds headers that tell gitaly-ruby the full path to the repository. func SetHeaders(ctx context.Context, repo *pb.Repository) (context.Context, error) { + return setHeaders(ctx, repo, true) +} + +func setHeaders(ctx context.Context, repo *pb.Repository, mustExist bool) (context.Context, error) { storagePath, err := helper.GetStorageByName(repo.GetStorageName()) if err != nil { return nil, err } - repoPath, err := helper.GetPath(repo) + var repoPath string + if mustExist { + repoPath, err = helper.GetRepoPath(repo) + } else { + repoPath, err = helper.GetPath(repo) + } if err != nil { return nil, err } diff --git a/internal/rubyserver/rubyserver_test.go b/internal/rubyserver/rubyserver_test.go index 77943731e..5233d720c 100644 --- a/internal/rubyserver/rubyserver_test.go +++ b/internal/rubyserver/rubyserver_test.go @@ -1,6 +1,7 @@ package rubyserver import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -23,32 +24,64 @@ func TestStopSafe(t *testing.T) { } func TestSetHeaders(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + testCases := []struct { + desc string repo *pb.Repository errType codes.Code + setter func(context.Context, *pb.Repository) (context.Context, error) }{ { + desc: "SetHeaders invalid storage", repo: &pb.Repository{StorageName: "foo", RelativePath: "bar.git"}, errType: codes.InvalidArgument, + setter: SetHeaders, + }, + { + desc: "SetHeaders invalid rel path", + repo: &pb.Repository{StorageName: testRepo.StorageName, RelativePath: "bar.git"}, + errType: codes.NotFound, + setter: SetHeaders, }, { + desc: "SetHeaders OK", repo: testRepo, errType: codes.OK, + setter: SetHeaders, + }, + { + desc: "SetHeadersWithoutRepoCheck invalid storage", + repo: &pb.Repository{StorageName: "foo", RelativePath: "bar.git"}, + errType: codes.InvalidArgument, + setter: SetHeadersWithoutRepoCheck, + }, + { + desc: "SetHeadersWithoutRepoCheck invalid relative path", + repo: &pb.Repository{StorageName: testRepo.StorageName, RelativePath: "bar.git"}, + errType: codes.OK, + setter: SetHeadersWithoutRepoCheck, + }, + { + desc: "SetHeadersWithoutRepoCheck OK", + repo: testRepo, + errType: codes.OK, + setter: SetHeadersWithoutRepoCheck, }, } for _, tc := range testCases { - ctx, cancel := testhelper.Context() - defer cancel() - - clientCtx, err := SetHeaders(ctx, tc.repo) - - if tc.errType != codes.OK { - testhelper.AssertGrpcError(t, err, tc.errType, "") - assert.Nil(t, clientCtx) - } else { - assert.NoError(t, err) - assert.NotNil(t, clientCtx) - } + t.Run(tc.desc, func(t *testing.T) { + clientCtx, err := tc.setter(ctx, tc.repo) + + if tc.errType != codes.OK { + testhelper.AssertGrpcError(t, err, tc.errType, "") + assert.Nil(t, clientCtx) + } else { + assert.NoError(t, err) + assert.NotNil(t, clientCtx) + } + }) } } diff --git a/internal/service/repository/create.go b/internal/service/repository/create.go index 65ecd63b2..9116e9c51 100644 --- a/internal/service/repository/create.go +++ b/internal/service/repository/create.go @@ -13,7 +13,7 @@ func (s *server) CreateRepository(ctx context.Context, req *pb.CreateRepositoryR return nil, err } - clientCtx, err := rubyserver.SetHeaders(ctx, req.GetRepository()) + clientCtx, err := rubyserver.SetHeadersWithoutRepoCheck(ctx, req.GetRepository()) if err != nil { return nil, err } diff --git a/internal/service/repository/testhelper_test.go b/internal/service/repository/testhelper_test.go index d41be6fc0..7926c015e 100644 --- a/internal/service/repository/testhelper_test.go +++ b/internal/service/repository/testhelper_test.go @@ -88,6 +88,7 @@ func TestMain(m *testing.M) { func testMain(m *testing.M) int { defer testhelper.MustHaveNoChildProcess() + testhelper.ConfigureTestStorage() testhelper.ConfigureRuby() config.Config.Auth = config.Auth{Token: config.Token(testhelper.RepositoryAuthToken)} |