Merge branch 'jc-finalizing-vote-post-receive-pack' into 'master'

smarthttp: Add finalizing vote in PostReceivePack

Closes #4110

See merge request gitlab-org/gitaly!4488

5 files changed, 95 insertions, 4 deletions

diff --git a/internal/gitaly/service/setup/register.go b/internal/gitaly/service/setup/register.go
index 2cc0bcd9f..32746ce4a 100644
--- a/internal/gitaly/service/setup/register.go
+++ b/internal/gitaly/service/setup/register.go
@@ -111,6 +111,7 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) {
 	gitalypb.RegisterSmartHTTPServiceServer(srv, smarthttp.NewServer(
 		deps.GetLocator(),
 		deps.GetGitCmdFactory(),
+		deps.GetTxManager(),
 		deps.GetDiskCache(),
 		smarthttp.WithPackfileNegotiationMetrics(smarthttpPackfileNegotiationMetrics),
 	))
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 7eb070b4d..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"
@@ -761,6 +763,7 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) {
 		gitalypb.RegisterSmartHTTPServiceServer(srv, NewServer(
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
+			deps.GetTxManager(),
 			deps.GetDiskCache(),
 		))
 		gitalypb.RegisterHookServiceServer(srv, hook.NewServer(deps.GetHookManager(), deps.GetGitCmdFactory(), deps.GetPackObjectsCache()))
@@ -788,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) {
@@ -816,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)
+}
diff --git a/internal/gitaly/service/smarthttp/server.go b/internal/gitaly/service/smarthttp/server.go
index e1a3c60d0..0145d54d9 100644
--- a/internal/gitaly/service/smarthttp/server.go
+++ b/internal/gitaly/service/smarthttp/server.go
@@ -5,6 +5,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/cache"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -14,15 +15,21 @@ type server struct {
 	gitCmdFactory              git.CommandFactory
 	packfileNegotiationMetrics *prometheus.CounterVec
 	infoRefCache               infoRefCache
+	txManager                  transaction.Manager
 }
 
 // NewServer creates a new instance of a grpc SmartHTTPServer
-func NewServer(locator storage.Locator, gitCmdFactory git.CommandFactory,
-	cache cache.Streamer, serverOpts ...ServerOpt,
+func NewServer(
+	locator storage.Locator,
+	gitCmdFactory git.CommandFactory,
+	txManager transaction.Manager,
+	cache cache.Streamer,
+	serverOpts ...ServerOpt,
 ) gitalypb.SmartHTTPServiceServer {
 	s := &server{
 		locator:       locator,
 		gitCmdFactory: gitCmdFactory,
+		txManager:     txManager,
 		packfileNegotiationMetrics: prometheus.NewCounterVec(
 			prometheus.CounterOpts{},
 			[]string{"git_negotiation_feature"},
diff --git a/internal/gitaly/service/smarthttp/testhelper_test.go b/internal/gitaly/service/smarthttp/testhelper_test.go
index 68daaca57..321cfed87 100644
--- a/internal/gitaly/service/smarthttp/testhelper_test.go
+++ b/internal/gitaly/service/smarthttp/testhelper_test.go
@@ -31,6 +31,7 @@ func startSmartHTTPServerWithOptions(t *testing.T, cfg config.Cfg, opts []Server
 		gitalypb.RegisterSmartHTTPServiceServer(srv, NewServer(
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
+			deps.GetTxManager(),
 			deps.GetDiskCache(),
 			opts...,
 		))