Merge branch 'pks-updateref-ignore-post-receive-custom-hook-error' into 'master'

updateref: Ignore failures of custom post-receive hooks

Closes #3595

See merge request gitlab-org/gitaly!4714

3 files changed, 52 insertions, 26 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index 23155bc40..fe5cf75df 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
@@ -268,7 +269,23 @@ func (u *UpdaterWithHooks) UpdateReference(
 	}
 
 	if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String()))
+		// CustomHook errors are returned in case a custom hook has returned an error code.
+		// The post-receive hook has special semantics though. Quoting githooks(5):
+		//
+		//    This hook does not affect the outcome of git receive-pack, as it is called
+		//    after the real work is done.
+		//
+		// This means that even if the hook returns an error, then that error should not
+		// impact whatever git-receive-pack(1) has been doing. And given that we emulate
+		// behaviour of this command here, we need to behave the same.
+		var customHookErr hook.CustomHookError
+		if errors.As(err, &customHookErr) {
+			// Only log the error when we've got a custom-hook error, but otherwise
+			// ignore it and continue with whatever we have been doing.
+			ctxlogrus.Extract(ctx).WithError(err).Error("custom post-receive hook returned an error")
+		} else {
+			return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String()))
+		}
 	}
 
 	return nil
diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go
index f04dac5a9..2b759f689 100644
--- a/internal/git/updateref/update_with_hooks_test.go
+++ b/internal/git/updateref/update_with_hooks_test.go
@@ -230,7 +230,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			expectedErr: "reference-transaction failure",
 		},
 		{
-			desc: "post-receive error",
+			desc: "post-receive error is ignored",
 			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				return nil
 			},
@@ -245,7 +245,6 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 				require.NoError(t, err)
 				return errors.New("ignored")
 			},
-			expectedErr:         "post-receive failure",
 			expectedRefDeletion: true,
 		},
 	}
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 8eebfbce6..33265a342 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -440,25 +440,26 @@ func TestUserMergeBranch_failingHooks(t *testing.T) {
 	hookContent := []byte("#!/bin/sh\necho 'stdout' && echo 'stderr' >&2\nexit 1")
 
 	for _, tc := range []struct {
-		hookName     string
-		hookType     gitalypb.CustomHookError_HookType
-		shouldUpdate bool
+		hookName   string
+		hookType   gitalypb.CustomHookError_HookType
+		shouldFail bool
 	}{
 		{
-			hookName:     "pre-receive",
-			hookType:     gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
-			shouldUpdate: false,
+			hookName:   "pre-receive",
+			hookType:   gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
+			shouldFail: true,
 		},
 		{
-			hookName:     "update",
-			hookType:     gitalypb.CustomHookError_HOOK_TYPE_UPDATE,
-			shouldUpdate: false,
+			hookName:   "update",
+			hookType:   gitalypb.CustomHookError_HOOK_TYPE_UPDATE,
+			shouldFail: true,
 		},
 		{
 			hookName: "post-receive",
 			hookType: gitalypb.CustomHookError_HOOK_TYPE_POSTRECEIVE,
-			// The post-receive hook runs after references have been updated.
-			shouldUpdate: true,
+			// The post-receive hook runs after references have been updated and any
+			// failures of it are ignored.
+			shouldFail: false,
 		},
 	} {
 		t.Run(tc.hookName, func(t *testing.T) {
@@ -485,22 +486,31 @@ func TestUserMergeBranch_failingHooks(t *testing.T) {
 			require.NoError(t, mergeBidi.CloseSend(), "close send")
 
 			secondResponse, err := mergeBidi.Recv()
-			testhelper.RequireGrpcError(t, errWithDetails(t,
-				helper.ErrPermissionDeniedf("stderr\n"),
-				&gitalypb.UserMergeBranchError{
-					Error: &gitalypb.UserMergeBranchError_CustomHook{
-						CustomHook: &gitalypb.CustomHookError{
-							HookType: tc.hookType,
-							Stdout:   []byte("stdout\n"),
-							Stderr:   []byte("stderr\n"),
+			if tc.shouldFail {
+				testhelper.RequireGrpcError(t, errWithDetails(t,
+					helper.ErrPermissionDeniedf("stderr\n"),
+					&gitalypb.UserMergeBranchError{
+						Error: &gitalypb.UserMergeBranchError_CustomHook{
+							CustomHook: &gitalypb.CustomHookError{
+								HookType: tc.hookType,
+								Stdout:   []byte("stdout\n"),
+								Stderr:   []byte("stderr\n"),
+							},
 						},
 					},
-				},
-			), err)
-			require.Nil(t, secondResponse)
+				), err)
+				require.Nil(t, secondResponse)
+			} else {
+				testhelper.ProtoEqual(t, &gitalypb.UserMergeBranchResponse{
+					BranchUpdate: &gitalypb.OperationBranchUpdate{
+						CommitId: firstResponse.CommitId,
+					},
+				}, secondResponse)
+				require.NoError(t, err)
+			}
 
 			currentBranchHead := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", mergeBranchName)
-			if tc.shouldUpdate {
+			if !tc.shouldFail {
 				require.Equal(t, firstResponse.CommitId, text.ChompBytes(currentBranchHead), "branch head updated")
 			} else {
 				require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated")