From 459e7d1dfa36cb443c41144f0b0535ba01556d23 Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 21 Apr 2022 11:52:43 -0400 Subject: smarthttp: Add finalizing vote in PostReceivePack When all updates are rejected, there will be no votes cast. This leads to an unnecessary replication job. In this case, we want to cast a last but empty vote. This is the same as how SSHReceivePack works. Changelog: changed --- internal/gitaly/service/smarthttp/receive_pack.go | 25 +++++++++ .../gitaly/service/smarthttp/receive_pack_test.go | 60 +++++++++++++++++++++- 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go index 7e9f5776f..1653594bc 100644 --- a/internal/gitaly/service/smarthttp/receive_pack.go +++ b/internal/gitaly/service/smarthttp/receive_pack.go @@ -1,10 +1,14 @@ package smarthttp import ( + "errors" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/v14/internal/git" + "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" + "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" "google.golang.org/grpc/codes" @@ -67,6 +71,27 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err) } + // In cases where all reference updates are rejected by git-receive-pack(1), we would end up + // with no transactional votes at all. This would lead to scheduling + // replication jobs, which wouldn't accomplish anything since no refs + // were updated. + // To prevent replication jobs from being unnecessarily created, do a + // final vote which concludes this RPC to ensure there's always at least + // one vote. In case there was diverging behaviour in git-receive-pack(1) + // which led to a different outcome across voters, then this final vote + // would fail because the sequence of votes would be different. + if err := transaction.VoteOnContext(ctx, s.txManager, voting.Vote{}, voting.Committed); err != nil { + // When the pre-receive hook failed, git-receive-pack(1) exits with code 0. + // It's arguable whether this is the expected behavior, but anyhow it means + // cmd.Wait() did not error out. On the other hand, the gitaly-hooks command did + // stop the transaction upon failure. So this final vote fails. + // To avoid this error being presented to the end user, ignore it when the + // transaction was stopped. + if !errors.Is(err, transaction.ErrTransactionStopped) { + return status.Errorf(codes.Aborted, "final transactional vote: %v", err) + } + } + return nil } diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 608cfd57f..e7349ac80 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -3,6 +3,7 @@ package smarthttp import ( "bytes" "context" + "errors" "fmt" "io" "os" @@ -17,6 +18,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" + gitalyhook "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook" "gitlab.com/gitlab-org/gitaly/v14/internal/gitlab" @@ -789,7 +791,7 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) { expectedResponse := "0049\x01000eunpack ok\n0019ok refs/heads/master\n0019ok refs/heads/branch\n00000000" require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) - require.Equal(t, 4, refTransactionServer.called) + require.Equal(t, 5, refTransactionServer.called) }) t.Run("delete", func(t *testing.T) { @@ -817,6 +819,60 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) { expectedResponse := "0033\x01000eunpack ok\n001cok refs/heads/delete-me\n00000000" require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) - require.Equal(t, 2, refTransactionServer.called) + require.Equal(t, 3, refTransactionServer.called) }) } + +func TestPostReceive_allRejected(t *testing.T) { + t.Parallel() + + cfg := testcfg.Build(t) + + testcfg.BuildGitalyHooks(t, cfg) + + refTransactionServer := &testTransactionServer{} + + hookManager := gitalyhook.NewMockManager( + t, + func( + t *testing.T, + ctx context.Context, + repo *gitalypb.Repository, + pushOptions, env []string, + stdin io.Reader, stdout, stderr io.Writer) error { + return errors.New("not allowed") + }, + gitalyhook.NopPostReceive, + gitalyhook.NopUpdate, + gitalyhook.NopReferenceTransaction, + ) + addr := testserver.RunGitalyServer(t, cfg, nil, func(srv *grpc.Server, deps *service.Dependencies) { + gitalypb.RegisterSmartHTTPServiceServer(srv, NewServer( + deps.GetLocator(), + deps.GetGitCmdFactory(), + deps.GetTxManager(), + deps.GetDiskCache(), + )) + gitalypb.RegisterHookServiceServer(srv, hook.NewServer(deps.GetHookManager(), deps.GetGitCmdFactory(), deps.GetPackObjectsCache())) + }, testserver.WithDisablePraefect(), testserver.WithHookManager(hookManager)) + + ctx, err := txinfo.InjectTransaction(testhelper.Context(t), 1234, "primary", true) + require.NoError(t, err) + ctx = metadata.IncomingToOutgoing(ctx) + + client := newMuxedSmartHTTPClient(t, ctx, addr, cfg.Auth.Token, func() backchannel.Server { + srv := grpc.NewServer() + gitalypb.RegisterRefTransactionServer(srv, refTransactionServer) + return srv + }) + + stream, err := client.PostReceivePack(ctx) + require.NoError(t, err) + + repo, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0]) + + request := &gitalypb.PostReceivePackRequest{Repository: repo, GlId: "key-1234", GlRepository: "some_repo"} + doPush(t, stream, request, newTestPush(t, cfg, nil).body) + + require.Equal(t, 1, refTransactionServer.called) +} -- cgit v1.2.3