transactions: Remove voting via pre-receive hook

When we started out to implement the transaction service, Git didn't yet
provide an ability to hook into its reference transactions. In order to
iterate fast, we thus implemented voting via the pre-receive hook as a
first approximation and a test whether the overall idea of transactional
voting actually works. Starting with Git v2.28.0, we now got a mechanism
to hook into Git's reference transactions via the reference-transaction
hook.

This new hook allows us to perform a vote on each change of a reference
and also to abort any ongoing transaction, which thus allows much wider
coverage of mutating calls to Git compared to the previous pre-receive
implementation. The reference-transaction hook has been battle-tested in
production now, which allows us to get rid of voting via the pre-receive
hook.

The removal of pre-receive voting  should also address some issues we've
been seeing around hooks. As only the primary will verify authorization
of pushes and execute custom hooks, it would abort before performing a
vote while any secondary would nicely chug along and wait for the
primary to arrive, as they didn't abort early due to not executing that
logic. As a result, secondaries would hang until they hit the RPC's
timeout in case the hook logic refused a push. This issue should now go
away with all voting logic moving into the reference-transaction hook.

6 files changed, 21 insertions, 111 deletions

diff --git a/changelogs/unreleased/pks-transaction-remove-prereceive-voting.yml b/changelogs/unreleased/pks-transaction-remove-prereceive-voting.yml
new file mode 100644
index 000000000..912a8cc09
--- /dev/null
+++ b/changelogs/unreleased/pks-transaction-remove-prereceive-voting.yml
@@ -0,0 +1,5 @@
+---
+title: 'transactions: Remove voting via pre-receive hook'
+merge_request: 2578
+author:
+type: changed
diff --git a/internal/gitaly/hook/prereceive.go b/internal/gitaly/hook/prereceive.go
index 441318aa7..9ceed76bc 100644
--- a/internal/gitaly/hook/prereceive.go
+++ b/internal/gitaly/hook/prereceive.go
@@ -3,7 +3,6 @@ package hook
 import (
 	"bytes"
 	"context"
-	"crypto/sha1"
 	"errors"
 	"fmt"
 	"io"
@@ -106,10 +105,5 @@ func (m *GitLabHookManager) PreReceiveHook(ctx context.Context, repo *gitalypb.R
 		}
 	}
 
-	hash := sha1.Sum(changes)
-	if err := m.VoteOnTransaction(ctx, hash[:], env); err != nil {
-		return helper.ErrInternalf("error voting on transaction: %v", err)
-	}
-
 	return nil
 }
diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go
index f9990a6e9..2e706a55a 100644
--- a/internal/gitaly/service/hook/pre_receive_test.go
+++ b/internal/gitaly/service/hook/pre_receive_test.go
@@ -2,11 +2,8 @@ package hook
 
 import (
 	"bytes"
-	"context"
-	"crypto/sha1"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -22,7 +19,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
-	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 )
 
@@ -331,17 +327,6 @@ exit %d
 	assert.Equal(t, customHookReturnMsg, text.ChompBytes(stderr), "hook stderr")
 }
 
-type testTransactionServer struct {
-	handler func(in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error)
-}
-
-func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-	if s.handler != nil {
-		return s.handler(in)
-	}
-	return nil, nil
-}
-
 func TestPreReceiveHook_Primary(t *testing.T) {
 	rubyDir := config.Config.Ruby.Dir
 	defer func(rubyDir string) {
@@ -357,11 +342,9 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 		primary            bool
 		allowedHandler     http.HandlerFunc
 		preReceiveHandler  http.HandlerFunc
-		stdin              []byte
 		hookExitCode       int32
 		expectedExitStatus int32
 		expectedStderr     string
-		expectedReftxHash  []byte
 	}{
 		{
 			desc:               "primary checks for permissions",
@@ -375,7 +358,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 			primary:            false,
 			allowedHandler:     allowedHandler(false),
 			expectedExitStatus: 0,
-			expectedReftxHash:  []byte{},
 		},
 		{
 			desc:               "primary tries to increase reference counter",
@@ -391,7 +373,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 			allowedHandler:     allowedHandler(true),
 			preReceiveHandler:  preReceiveHandler(false),
 			expectedExitStatus: 0,
-			expectedReftxHash:  []byte{},
 		},
 		{
 			desc:               "primary executes hook",
@@ -408,76 +389,11 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 			preReceiveHandler:  preReceiveHandler(true),
 			hookExitCode:       123,
 			expectedExitStatus: 0,
-			expectedReftxHash:  []byte{},
-		},
-		{
-			desc:               "primary hook triggers transaction",
-			primary:            true,
-			stdin:              []byte("foobar"),
-			allowedHandler:     allowedHandler(true),
-			preReceiveHandler:  preReceiveHandler(true),
-			hookExitCode:       0,
-			expectedExitStatus: 0,
-			expectedReftxHash:  []byte("foobar"),
-		},
-		{
-			desc:               "secondary hook triggers transaction",
-			primary:            false,
-			stdin:              []byte("foobar"),
-			allowedHandler:     allowedHandler(true),
-			preReceiveHandler:  preReceiveHandler(true),
-			hookExitCode:       0,
-			expectedExitStatus: 0,
-			expectedReftxHash:  []byte("foobar"),
-		},
-		{
-			desc:               "primary Ruby hook triggers transaction",
-			primary:            true,
-			stdin:              []byte("foobar"),
-			allowedHandler:     allowedHandler(true),
-			preReceiveHandler:  preReceiveHandler(true),
-			hookExitCode:       0,
-			expectedExitStatus: 0,
-			expectedReftxHash:  []byte("foobar"),
-		},
-		{
-			desc:               "secondary Ruby hook triggers transaction",
-			primary:            false,
-			stdin:              []byte("foobar"),
-			allowedHandler:     allowedHandler(true),
-			preReceiveHandler:  preReceiveHandler(true),
-			hookExitCode:       0,
-			expectedExitStatus: 0,
-			expectedReftxHash:  []byte("foobar"),
 		},
 	}
 
-	transactionServer := &testTransactionServer{}
-	grpcServer := grpc.NewServer()
-	gitalypb.RegisterRefTransactionServer(grpcServer, transactionServer)
-
-	listener, err := net.Listen("tcp", "127.0.0.1:0")
-	require.NoError(t, err)
-
-	errQ := make(chan error)
-	go func() {
-		errQ <- grpcServer.Serve(listener)
-	}()
-	defer func() {
-		grpcServer.Stop()
-		require.NoError(t, <-errQ)
-	}()
-
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			var reftxHash []byte
-			transactionServer.handler = func(in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-				reftxHash = in.ReferenceUpdatesHash
-				return &gitalypb.VoteTransactionResponse{
-					State: gitalypb.VoteTransactionResponse_COMMIT,
-				}, nil
-			}
-
 			testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 			defer cleanupFn()
 
@@ -509,12 +425,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 			ctx, cancel := testhelper.Context()
 			defer cancel()
 
-			transactionServer := metadata.PraefectServer{
-				ListenAddr: "tcp://" + listener.Addr().String(),
-			}
-			transactionServerEnv, err := transactionServer.Env()
-			require.NoError(t, err)
-
 			transaction := metadata.Transaction{
 				ID:      1234,
 				Node:    "node-1",
@@ -529,7 +439,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 				"GL_USERNAME=username",
 				"GL_REPOSITORY=repository",
 				transactionEnv,
-				transactionServerEnv,
 			}
 
 			stream, err := client.PreReceiveHook(ctx)
@@ -538,22 +447,12 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 				Repository:           testRepo,
 				EnvironmentVariables: environment,
 			}))
-			require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{
-				Stdin: tc.stdin,
-			}))
 			require.NoError(t, stream.CloseSend())
 
 			_, stderr, status, _ := sendPreReceiveHookRequest(t, stream, &bytes.Buffer{})
 
-			var expectedReftxHash []byte
-			if tc.expectedReftxHash != nil {
-				hash := sha1.Sum(tc.expectedReftxHash)
-				expectedReftxHash = hash[:]
-			}
-
 			require.Equal(t, tc.expectedExitStatus, status)
 			require.Equal(t, tc.expectedStderr, text.ChompBytes(stderr))
-			require.Equal(t, expectedReftxHash[:], reftxHash)
 		})
 	}
 }
diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go
index fd1ad0ffa..4afef9783 100644
--- a/internal/gitaly/service/hook/reference_transaction_test.go
+++ b/internal/gitaly/service/hook/reference_transaction_test.go
@@ -1,6 +1,7 @@
 package hook
 
 import (
+	"context"
 	"crypto/sha1"
 	"net"
 	"testing"
@@ -15,6 +16,17 @@ import (
 	"google.golang.org/grpc/codes"
 )
 
+type testTransactionServer struct {
+	handler func(in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error)
+}
+
+func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
+	if s.handler != nil {
+		return s.handler(in)
+	}
+	return nil, nil
+}
+
 func TestReferenceTransactionHookInvalidArgument(t *testing.T) {
 	serverSocketPath, stop := runHooksServer(t, config.Config)
 	defer stop()
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 8a93f4abf..a3be918b6 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -206,9 +206,9 @@ func testUserCreateBranchWithTransaction(t *testing.T, withRefTxHook bool) {
 			require.Empty(t, response.PreReceiveError)
 
 			if withRefTxHook {
-				require.Equal(t, 2, transactionServer.called)
-			} else {
 				require.Equal(t, 1, transactionServer.called)
+			} else {
+				require.Equal(t, 0, transactionServer.called)
 			}
 		})
 	}
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index abb27c363..183a73444 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -659,9 +659,9 @@ func TestPostReceiveWithReferenceTransactionHook(t *testing.T) {
 			// If the reference-transaction hook is not supported or the feature flag is
 			// not enabled, voting only happens via the pre-receive hook.
 			if features.IsDisabled(featureflag.ReferenceTransactionHook) || !supported {
-				require.Equal(t, 1, refTransactionServer.called)
+				require.Equal(t, 0, refTransactionServer.called)
 			} else {
-				require.Equal(t, 5, refTransactionServer.called)
+				require.Equal(t, 4, refTransactionServer.called)
 			}
 		})
 	}