diff --git a/changelogs/unreleased/pks-hooks-stop-transactions.yml b/changelogs/unreleased/pks-hooks-stop-transactions.yml new file mode 100644 index 0000000000000000000000000000000000000000..6cbf5acd6cb4d52e71d4af5b662339d0cac98fa1 --- /dev/null +++ b/changelogs/unreleased/pks-hooks-stop-transactions.yml @@ -0,0 +1,5 @@ +--- +title: 'hook: Stop transactions when post-receive and update hooks fail' +merge_request: 3094 +author: +type: fixed diff --git a/internal/gitaly/hook/postreceive.go b/internal/gitaly/hook/postreceive.go index 2d7c1cf0c3542318f2732bad6bd2c7ac312bed42..6ab89ecddd3e2da65bf4453ab70bbd6b69ff66ba 100644 --- a/internal/gitaly/hook/postreceive.go +++ b/internal/gitaly/hook/postreceive.go @@ -10,6 +10,7 @@ import ( "math" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -126,11 +127,25 @@ func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb. return helper.ErrInternalf("reading stdin from request: %w", err) } - if !isPrimary(payload) { - return nil + if isPrimary(payload) { + if err := m.postReceiveHook(ctx, payload, repo, pushOptions, env, changes, stdout, stderr); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Warn("stopping transaction because post-receive hook failed") + + // If the post-receive hook declines the push, then we need to stop any + // secondaries voting on the transaction. + if err := m.stopTransaction(ctx, payload); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed stopping transaction in post-receive hook") + } + + return err + } } - if len(changes) == 0 { + return nil +} + +func (m *GitLabHookManager) postReceiveHook(ctx context.Context, payload git.HooksPayload, repo *gitalypb.Repository, pushOptions, env []string, stdin []byte, stdout, stderr io.Writer) error { + if len(stdin) == 0 { return helper.ErrInternalf("hook got no reference updates") } @@ -147,7 +162,7 @@ func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb. ok, messages, err := m.gitlabAPI.PostReceive( ctx, repo.GetGlRepository(), payload.ReceiveHooksPayload.UserID, - string(changes), + string(stdin), pushOptions..., ) if err != nil { @@ -176,11 +191,11 @@ func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb. ctx, nil, customEnv, - bytes.NewReader(changes), + bytes.NewReader(stdin), stdout, stderr, ); err != nil { - return err + return fmt.Errorf("executing custom hooks: %w", err) } return nil diff --git a/internal/gitaly/hook/prereceive.go b/internal/gitaly/hook/prereceive.go index 8e5ce6f9c57a5fd22ed879a7e42b56d4847c0225..41dc8aca75061d2686381f85a3729e223a95338f 100644 --- a/internal/gitaly/hook/prereceive.go +++ b/internal/gitaly/hook/prereceive.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -67,9 +68,14 @@ func (m *GitLabHookManager) PreReceiveHook(ctx context.Context, repo *gitalypb.R // Only the primary should execute hooks and increment reference counters. if isPrimary(payload) { if err := m.preReceiveHook(ctx, payload, repo, pushOptions, env, changes, stdout, stderr); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Warn("stopping transaction because pre-receive hook failed") + // If the pre-receive hook declines the push, then we need to stop any // secondaries voting on the transaction. - m.stopTransaction(ctx, payload) + if err := m.stopTransaction(ctx, payload); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed stopping transaction in pre-receive hook") + } + return err } } diff --git a/internal/gitaly/hook/transactions_test.go b/internal/gitaly/hook/transactions_test.go new file mode 100644 index 0000000000000000000000000000000000000000..256e70a7e88fcddf411a2e6a5272884ea9a8c316 --- /dev/null +++ b/internal/gitaly/hook/transactions_test.go @@ -0,0 +1,123 @@ +package hook + +import ( + "context" + "errors" + "io/ioutil" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +type mockTransactionManager struct { + stop func(context.Context, metadata.Transaction, metadata.PraefectServer) error +} + +func (m *mockTransactionManager) Vote(context.Context, metadata.Transaction, metadata.PraefectServer, []byte) error { + return nil +} + +func (m *mockTransactionManager) Stop(ctx context.Context, tx metadata.Transaction, praefect metadata.PraefectServer) error { + return m.stop(ctx, tx, praefect) +} + +func TestHookManager_stopCalled(t *testing.T) { + expectedTx := metadata.Transaction{ + ID: 1234, Node: "primary", Primary: true, + } + + expectedPraefect := metadata.PraefectServer{ + SocketPath: "socket", + Token: "foo", + } + + repo, repoPath, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + var mockTxMgr mockTransactionManager + hookManager := NewManager(config.NewLocator(config.Config), &mockTxMgr, GitlabAPIStub, config.Config) + + hooksPayload, err := git.NewHooksPayload( + config.Config, + repo, + &expectedTx, + &expectedPraefect, + &git.ReceiveHooksPayload{ + UserID: "1234", + Username: "user", + Protocol: "web", + }, + ).Env() + require.NoError(t, err) + + ctx, cleanup := testhelper.Context() + defer cleanup() + + for _, hook := range []string{"pre-receive", "update", "post-receive"} { + cleanup = testhelper.WriteCustomHook(t, repoPath, hook, []byte("#!/bin/sh\nexit 1\n")) + defer cleanup() + } + + preReceiveFunc := func(t *testing.T) error { + return hookManager.PreReceiveHook(ctx, repo, nil, []string{hooksPayload}, strings.NewReader("changes"), ioutil.Discard, ioutil.Discard) + } + updateFunc := func(t *testing.T) error { + return hookManager.UpdateHook(ctx, repo, "ref", git.ZeroOID.String(), git.ZeroOID.String(), []string{hooksPayload}, ioutil.Discard, ioutil.Discard) + } + postReceiveFunc := func(t *testing.T) error { + return hookManager.PostReceiveHook(ctx, repo, nil, []string{hooksPayload}, strings.NewReader("changes"), ioutil.Discard, ioutil.Discard) + } + + for _, tc := range []struct { + desc string + hookFunc func(*testing.T) error + stopErr error + }{ + { + desc: "pre-receive gets successfully stopped", + hookFunc: preReceiveFunc, + }, + { + desc: "pre-receive with stop error does not clobber real error", + hookFunc: preReceiveFunc, + stopErr: errors.New("stop error"), + }, + { + desc: "post-receive gets successfully stopped", + hookFunc: postReceiveFunc, + }, + { + desc: "post-receive with stop error does not clobber real error", + hookFunc: postReceiveFunc, + stopErr: errors.New("stop error"), + }, + { + desc: "update gets successfully stopped", + hookFunc: updateFunc, + }, + { + desc: "update with stop error does not clobber real error", + hookFunc: updateFunc, + stopErr: errors.New("stop error"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + wasInvoked := false + mockTxMgr.stop = func(ctx context.Context, tx metadata.Transaction, praefect metadata.PraefectServer) error { + require.Equal(t, expectedTx, tx) + require.Equal(t, expectedPraefect, praefect) + wasInvoked = true + return tc.stopErr + } + + err := tc.hookFunc(t) + require.Equal(t, "executing custom hooks: exit status 1", err.Error()) + require.True(t, wasInvoked, "expected stop to have been invoked") + }) + } +} diff --git a/internal/gitaly/hook/update.go b/internal/gitaly/hook/update.go index ce96c55b5b0acf82c9a44350418f4f07336ff79a..d8910b0552fb8ce595cfbaf5d9edf6d4012dd849 100644 --- a/internal/gitaly/hook/update.go +++ b/internal/gitaly/hook/update.go @@ -2,8 +2,10 @@ package hook import ( "context" + "fmt" "io" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -15,10 +17,24 @@ func (m *GitLabHookManager) UpdateHook(ctx context.Context, repo *gitalypb.Repos return helper.ErrInternalf("extracting hooks payload: %w", err) } - if !isPrimary(payload) { - return nil + if isPrimary(payload) { + if err := m.updateHook(ctx, payload, repo, ref, oldValue, newValue, env, stdout, stderr); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Warn("stopping transaction because update hook failed") + + // If the update hook declines the push, then we need + // to stop any secondaries voting on the transaction. + if err := m.stopTransaction(ctx, payload); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed stopping transaction in update hook") + } + + return err + } } + return nil +} + +func (m *GitLabHookManager) updateHook(ctx context.Context, payload git.HooksPayload, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error { if ref == "" { return helper.ErrInternalf("hook got no reference") } @@ -50,7 +66,7 @@ func (m *GitLabHookManager) UpdateHook(ctx context.Context, repo *gitalypb.Repos stdout, stderr, ); err != nil { - return err + return fmt.Errorf("executing custom hooks: %w", err) } return nil diff --git a/internal/log/hook.go b/internal/log/hook.go index ec4987b541bcebf2d73ce8fced1d1a9b2b41ecea..278049a74e8a09453cf08cf28709bd22a61eaa67 100644 --- a/internal/log/hook.go +++ b/internal/log/hook.go @@ -41,7 +41,7 @@ func (h *HookLogger) Fatal(err error) { // Fatalf logs a formatted error at the Fatal level func (h *HookLogger) Fatalf(format string, a ...interface{}) { - fmt.Fprintf(os.Stderr, "error executing git hook") + fmt.Fprintln(os.Stderr, "error executing git hook") h.logger.Fatalf(format, a...) }