Merge branch 'toon-fix-prerecv-noise' into 'master'

ssh: Clean up output when pre-receive hook fails

See merge request gitlab-org/gitaly!4318

2 files changed, 68 insertions, 2 deletions

diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index b96d7833f..8501525d7 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -101,7 +101,15 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 	// 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 {
-		return status.Errorf(codes.Aborted, "final transactional vote: %v", err)
+		// 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/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index a810d6766..6439446bd 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -240,6 +241,57 @@ func TestReceivePackPushHookFailure(t *testing.T) {
 	require.Contains(t, err.Error(), "(pre-receive hook declined)")
 }
 
+func TestReceivePackPushHookFailureWithCustomHook(t *testing.T) {
+	t.Parallel()
+
+	cfg := testcfg.Build(t)
+	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
+
+	testcfg.BuildGitalySSH(t, cfg)
+	testcfg.BuildGitalyHooks(t, cfg)
+
+	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(gitCmdFactory))
+	ctx := testhelper.Context(t)
+
+	repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
+		Seed: gittest.SeedGitLabTest,
+	})
+
+	cloneDetails, cleanup := setupSSHClone(t, cfg, repo, repoPath)
+	defer cleanup()
+
+	hookContent := []byte("#!/bin/sh\necho 'this is wrong' >&2;exit 1")
+	gittest.WriteCustomHook(t, cloneDetails.RemoteRepoPath, "pre-receive", hookContent)
+
+	cmd := sshPushCommand(ctx, t, cfg, cloneDetails, cfg.SocketPath,
+		pushParams{
+			storageName:  cfg.Storages[0].Name,
+			glID:         "1",
+			glRepository: repo.GlRepository,
+		})
+
+	stdout, err := cmd.StdoutPipe()
+	require.NoError(t, err)
+	stderr, err := cmd.StderrPipe()
+	require.NoError(t, err)
+
+	require.NoError(t, cmd.Start())
+
+	c, err := io.Copy(io.Discard, stdout)
+	require.NoError(t, err)
+	require.Equal(t, c, int64(0))
+
+	slurpErr, err := io.ReadAll(stderr)
+	require.NoError(t, err)
+
+	require.Error(t, cmd.Wait())
+
+	require.Contains(t, string(slurpErr), "remote: this is wrong")
+	require.Contains(t, string(slurpErr), "(pre-receive hook declined)")
+
+	require.NotContains(t, string(slurpErr), "final transactional vote: transaction was stopped")
+}
+
 func TestObjectPoolRefAdvertisementHidingSSH(t *testing.T) {
 	t.Parallel()
 
@@ -599,7 +651,7 @@ func setupSSHClone(t *testing.T, cfg config.Cfg, remoteRepo *gitalypb.Repository
 		}
 }
 
-func sshPush(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDetails SSHCloneDetails, serverSocketPath string, params pushParams) (string, string, error) {
+func sshPushCommand(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDetails SSHCloneDetails, serverSocketPath string, params pushParams) *exec.Cmd {
 	pbTempRepo := &gitalypb.Repository{
 		StorageName:   params.storageName,
 		RelativePath:  cloneDetails.TempRepo,
@@ -624,6 +676,12 @@ func sshPush(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDetails SSH
 		fmt.Sprintf("GIT_SSH_COMMAND=%s receive-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")),
 	}
 
+	return cmd
+}
+
+func sshPush(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDetails SSHCloneDetails, serverSocketPath string, params pushParams) (string, string, error) {
+	cmd := sshPushCommand(ctx, t, cfg, cloneDetails, serverSocketPath, params)
+
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		return "", "", fmt.Errorf("error pushing: %v: %q", err, out)