diff --git a/changelogs/unreleased/zj-self-healing-hooks.yml b/changelogs/unreleased/zj-self-healing-hooks.yml new file mode 100644 index 0000000000000000000000000000000000000000..1b64a06446c14a4a0811869bb31ba1b3cc11c7f1 --- /dev/null +++ b/changelogs/unreleased/zj-self-healing-hooks.yml @@ -0,0 +1,5 @@ +--- +title: Make git hooks self healing +merge_request: 886 +author: +type: added diff --git a/internal/git/helper.go b/internal/git/helper.go index 85f607c2c45dfc6662b87f4daf74534118559886..87a7222be850df0f72a363405fa1cb2c5497125d 100644 --- a/internal/git/helper.go +++ b/internal/git/helper.go @@ -63,8 +63,8 @@ func Version() (string, error) { func BuildGitOptions(gitOpts []string, otherOpts ...string) []string { args := []string{} - if len(gitOpts) > 0 { - args = append([]string{"-c"}, gitOpts...) + for _, opt := range gitOpts { + args = append(args, "-c", opt) } return append(args, otherOpts...) diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go new file mode 100644 index 0000000000000000000000000000000000000000..63e1053302e72d1297c061c918461c815c03349d --- /dev/null +++ b/internal/git/hooks/hooks.go @@ -0,0 +1,12 @@ +package hooks + +import ( + "path" + + "gitlab.com/gitlab-org/gitaly/internal/config" +) + +// Path returns the path where the global git hooks are located +func Path() string { + return path.Join(config.Config.GitlabShell.Dir, "hooks") +} diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go new file mode 100644 index 0000000000000000000000000000000000000000..9a8088de9477d075597465fff7bf574d43fb640c --- /dev/null +++ b/internal/git/hooks/hooks_test.go @@ -0,0 +1,12 @@ +package hooks + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPath(t *testing.T) { + assert.True(t, strings.HasSuffix(Path(), "hooks")) +} diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go index 46e935ca9ca6078c9b43fe660ed405357a031eb1..3fdfc31f753188e88fed419235e1a2db2a8efc4a 100644 --- a/internal/rubyserver/rubyserver.go +++ b/internal/rubyserver/rubyserver.go @@ -16,6 +16,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer" "gitlab.com/gitlab-org/gitaly/internal/supervisor" @@ -107,6 +108,7 @@ func Start() (*Server, error) { "GITALY_RUBY_GITLAB_SHELL_PATH="+cfg.GitlabShell.Dir, "GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir, "GITALY_VERSION="+version.GetVersion(), + "GITALY_GIT_HOOKS_DIR="+hooks.Path(), ) env = append(env, command.GitEnv...) diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go index ab6d8e5d59693aa1b55ea9c5bd958b5fc4cc8cd4..9c41aac213730999f321ac76bb526203fcbec222 100644 --- a/internal/service/operations/branches_test.go +++ b/internal/service/operations/branches_test.go @@ -1,10 +1,8 @@ package operations import ( - "io/ioutil" "os" "os/exec" - "path" "testing" "github.com/stretchr/testify/require" @@ -99,9 +97,7 @@ func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { defer exec.Command("git", "-C", testRepoPath, "branch", "-D", branchName).Run() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) - defer os.Remove(hookOutputTempPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) ctx, cancel := testhelper.Context() defer cancel() @@ -138,9 +134,9 @@ func TestFailedUserCreateBranchDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -289,8 +285,7 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput) - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -395,10 +390,9 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - require.NoError(t, os.MkdirAll(path.Join(testRepoPath, "hooks"), 0755)) - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/cherry_pick_test.go b/internal/service/operations/cherry_pick_test.go index 6326419ca78cceb1f4a56c00985db964b6adb312..d9440d2cb0cf0930a8813162b144cfe0d0cc4004 100644 --- a/internal/service/operations/cherry_pick_test.go +++ b/internal/service/operations/cherry_pick_test.go @@ -1,10 +1,8 @@ package operations_test import ( - "io/ioutil" "net" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -160,8 +158,7 @@ func TestSuccessfulGitHooksForUserCherryPickRequest(t *testing.T) { for _, hookName := range operations.GitlabHooks { t.Run(hookName, func(t *testing.T) { - hookPath, hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) md := testhelper.GitalyServersMetadata(t, serverSocketPath) @@ -301,9 +298,9 @@ func TestFailedUserCherryPickRequestDueToPreReceiveError(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go index ec76bad83d9ad59f5308f1dd97bf53ab6cce5750..ab8d501538d559a88f3d3ea61860b3de50af0798 100644 --- a/internal/service/operations/commit_files_test.go +++ b/internal/service/operations/commit_files_test.go @@ -2,9 +2,6 @@ package operations_test import ( "fmt" - "io/ioutil" - "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -139,7 +136,7 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) { client, conn := operations.NewOperationClient(t, serverSocketPath) defer conn.Close() - testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() ctxOuter, cancel := testhelper.Context() @@ -154,9 +151,9 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go index 1fdea95dbad79d0b88633aeb32d321d84c50e694..397633f429112e6eef2dab3181249732ad0c2d59 100644 --- a/internal/service/operations/merge_test.go +++ b/internal/service/operations/merge_test.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" "os/exec" - "path" "strings" "testing" "time" @@ -51,9 +50,7 @@ func TestSuccessfulMerge(t *testing.T) { hooks := GitlabHooks hookTempfiles := make([]string, len(hooks)) for i, h := range hooks { - var hookPath string - hookPath, hookTempfiles[i] = WriteEnvToHook(t, testRepoPath, h) - defer os.Remove(hookPath) + hookTempfiles[i] = WriteEnvToHook(t, testRepoPath, h) defer os.Remove(hookTempfiles[i]) } @@ -238,10 +235,9 @@ func TestFailedMergeDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - require.NoError(t, os.MkdirAll(path.Join(testRepoPath, "hooks"), 0755)) - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() @@ -443,9 +439,9 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/rebase_test.go b/internal/service/operations/rebase_test.go index 18fca0c688fd01a3a73436b0baac560a1c2074fb..79c8c2e5caf5d9cbdcac87f56f496862188f46e2 100644 --- a/internal/service/operations/rebase_test.go +++ b/internal/service/operations/rebase_test.go @@ -1,9 +1,6 @@ package operations_test import ( - "io/ioutil" - "os" - "path" "strings" "testing" @@ -101,12 +98,11 @@ func TestFailedUserRebaseRequestDueToPreReceiveError(t *testing.T) { } hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1") - for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/revert_test.go b/internal/service/operations/revert_test.go index 340fb6416a3168d487d1a8d6c08fea77bc84d49e..621b858dec1fec8817455cc12734454fa70d3096 100644 --- a/internal/service/operations/revert_test.go +++ b/internal/service/operations/revert_test.go @@ -1,9 +1,7 @@ package operations_test import ( - "io/ioutil" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -157,8 +155,7 @@ func TestSuccessfulGitHooksForUserRevertRequest(t *testing.T) { for _, hookName := range operations.GitlabHooks { t.Run(hookName, func(t *testing.T) { - hookPath, hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) md := testhelper.GitalyServersMetadata(t, serverSocketPath) @@ -298,9 +295,9 @@ func TestFailedUserRevertRequestDueToPreReceiveError(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go index 385711a1107beaf683fd13f10c247b6677b707b0..268d3571da985c286e064ef83e555c9727ad3c1b 100644 --- a/internal/service/operations/tags_test.go +++ b/internal/service/operations/tags_test.go @@ -1,10 +1,8 @@ package operations import ( - "io/ioutil" "os" "os/exec" - "path" "strings" "testing" @@ -83,8 +81,7 @@ func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -211,8 +208,7 @@ func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagName).Run() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -318,9 +314,9 @@ func TestFailedUserDeleteTagDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() @@ -342,7 +338,7 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() - testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() user := &gitalypb.User{ @@ -361,9 +357,9 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go index a000798689b55c391236250813785229ed9d7cbf..dc182a93ebb486bb7417a940c6325fcc9892ce51 100644 --- a/internal/service/operations/testhelper_test.go +++ b/internal/service/operations/testhelper_test.go @@ -12,6 +12,8 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc" @@ -44,7 +46,20 @@ func TestMain(m *testing.M) { func testMain(m *testing.M) int { defer testhelper.MustHaveNoChildProcess() - var err error + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + if err != nil { + log.Fatal(err) + } + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = oldDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + if err := os.MkdirAll(hooks.Path(), 0755); err != nil { + log.Fatal(err) + } + defer os.RemoveAll(hookDir) testhelper.ConfigureGitalySSH() @@ -92,17 +107,25 @@ func newOperationClient(t *testing.T, serverSocketPath string) (gitalypb.Operati var NewOperationClient = newOperationClient -func WriteEnvToHook(t *testing.T, repoPath, hookName string) (string, string) { +// The callee is responsible for clean up of the specific hook, testMain removes +// the hook dir +func WriteEnvToHook(t *testing.T, repoPath, hookName string) string { hookOutputTemp, err := ioutil.TempFile("", "") require.NoError(t, err) require.NoError(t, hookOutputTemp.Close()) - defer os.Remove(hookOutputTemp.Name()) - hookContent := fmt.Sprintf("#!/bin/sh\n/usr/bin/env > %s\n", hookOutputTemp.Name()) - hookPath := path.Join(repoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, []byte(hookContent), 0755) + _, err = OverrideHooks(hookName, []byte(hookContent)) + require.NoError(t, err) + + return hookOutputTemp.Name() +} + +// When testing hooks, the content for one specific hook can be defined, to simulate +// behaviours on different hook content +func OverrideHooks(name string, content []byte) (func(), error) { + fullPath := path.Join(hooks.Path(), name) - return hookPath, hookOutputTemp.Name() + return func() { os.Remove(fullPath) }, ioutil.WriteFile(fullPath, content, 0755) } diff --git a/internal/service/operations/update_branches_test.go b/internal/service/operations/update_branches_test.go index 6cc0aac1ff616b6275a47a620aee33be19328dac..56be3faf248d2a7c19bcf773d404745ad22421de 100644 --- a/internal/service/operations/update_branches_test.go +++ b/internal/service/operations/update_branches_test.go @@ -1,9 +1,7 @@ package operations import ( - "io/ioutil" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -64,8 +62,7 @@ func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -112,11 +109,11 @@ func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() response, err := client.UserUpdateBranch(ctx, request) diff --git a/internal/service/smarthttp/receive_pack.go b/internal/service/smarthttp/receive_pack.go index 040193aaf984938356f042b5b5b919c5e55294c5..a98988e0923ba7d16855c3d3bf22603f30725cec 100644 --- a/internal/service/smarthttp/receive_pack.go +++ b/internal/service/smarthttp/receive_pack.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -50,15 +51,16 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac env = append(env, fmt.Sprintf("GL_USERNAME=%s", req.GlUsername)) } - env = git.AddGitProtocolEnv(ctx, req, env) - env = append(env, command.GitEnv...) - repoPath, err := helper.GetRepoPath(req.Repository) if err != nil { return err } - gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", "--stateless-rpc", repoPath) + env = git.AddGitProtocolEnv(ctx, req, env) + env = append(env, command.GitEnv...) + + opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + gitOptions := git.BuildGitOptions(opts, "receive-pack", "--stateless-rpc", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, nil, env, gitOptions...) if err != nil { diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go index dc6ffbbf5ef2cd2a4ed72b9bbf8dd7a8b8ea3b21..679270f7ac17fee420dfe4ca7b7e134f9630f4b1 100644 --- a/internal/service/smarthttp/receive_pack_test.go +++ b/internal/service/smarthttp/receive_pack_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "io/ioutil" + "os" "path" "strings" "testing" @@ -14,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/streamio" "golang.org/x/net/context" @@ -30,7 +32,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) { client, conn := newSmartHTTPClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() stream, err := client.PostReceivePack(ctx) @@ -57,7 +59,7 @@ func TestSuccessfulReceivePackRequestWithGitOpts(t *testing.T) { client, conn := newSmartHTTPClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() stream, err := client.PostReceivePack(ctx) @@ -137,6 +139,44 @@ func TestFailedReceivePackRequestWithGitOpts(t *testing.T) { require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) } +func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) { + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + require.NoError(t, err) + defer os.RemoveAll(hookDir) + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = hookDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + require.NoError(t, os.MkdirAll(hooks.Path(), 0755)) + + hookContent := []byte("#!/bin/sh\nexit 1") + ioutil.WriteFile(path.Join(hooks.Path(), "pre-receive"), hookContent, 0755) + + server, serverSocketPath := runSmartHTTPServer(t) + defer server.Stop() + + repo, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + client, conn := newSmartHTTPClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.PostReceivePack(ctx) + require.NoError(t, err) + + push := newTestPush(t, nil) + firstRequest := &gitalypb.PostReceivePackRequest{Repository: repo, GlId: "user-123", GlRepository: "project-123"} + response := doPush(t, stream, firstRequest, push.body) + + expectedResponse := "004a\x01000eunpack ok\n0033ng refs/heads/master pre-receive hook declined\n00000000" + require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) +} + func doPush(t *testing.T, stream gitalypb.SmartHTTPService_PostReceivePackClient, firstRequest *gitalypb.PostReceivePackRequest, body io.Reader) []byte { require.NoError(t, stream.Send(firstRequest)) diff --git a/internal/service/ssh/receive_pack.go b/internal/service/ssh/receive_pack.go index 84decfe9c8ca1c45fa3013780c580e98769a8acc..c74e66ad266ac68b91a6195413ed06c69968c61b 100644 --- a/internal/service/ssh/receive_pack.go +++ b/internal/service/ssh/receive_pack.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -50,15 +51,17 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer) if req.GlRepository != "" { env = append(env, fmt.Sprintf("GL_REPOSITORY=%s", req.GlRepository)) } - env = git.AddGitProtocolEnv(ctx, req, env) - env = append(env, command.GitEnv...) repoPath, err := helper.GetRepoPath(req.Repository) if err != nil { return err } - gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", repoPath) + env = git.AddGitProtocolEnv(ctx, req, env) + env = append(env, command.GitEnv...) + + opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + gitOptions := git.BuildGitOptions(opts, "receive-pack", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, gitOptions...) if err != nil { diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go index 37531c853ade57acf67fb38923c51043a03c6f13..55f1e4740795005466910c0b6267851455819611 100644 --- a/internal/service/ssh/receive_pack_test.go +++ b/internal/service/ssh/receive_pack_test.go @@ -15,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "golang.org/x/net/context" "google.golang.org/grpc/codes" @@ -138,6 +139,30 @@ func TestReceivePackPushFailure(t *testing.T) { t.Errorf("local and remote head equal. push did not fail") } } + +} + +func TestReceivePackPushHookFailure(t *testing.T) { + server, serverSocketPath := runSSHServer(t) + defer server.Stop() + + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + require.NoError(t, err) + defer os.RemoveAll(hookDir) + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = oldDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + require.NoError(t, os.MkdirAll(hooks.Path(), 0755)) + + hookContent := []byte("#!/bin/sh\nexit 1") + ioutil.WriteFile(path.Join(hooks.Path(), "pre-receive"), hookContent, 0755) + + _, _, err = testCloneAndPush(t, serverSocketPath, pushParams{storageName: testRepo.GetStorageName(), glID: "1"}) + require.Error(t, err) + require.Contains(t, err.Error(), "(pre-receive hook declined)") } func testCloneAndPush(t *testing.T, serverSocketPath string, params pushParams) (string, string, error) { diff --git a/ruby/lib/gitlab/config.rb b/ruby/lib/gitlab/config.rb index d6fae8db6483924f239a9f032672f8009883398f..379d28f4ceacec114680c4848b29a62eb326ddb6 100644 --- a/ruby/lib/gitlab/config.rb +++ b/ruby/lib/gitlab/config.rb @@ -6,6 +6,10 @@ module Gitlab ENV['GITALY_RUBY_GIT_BIN_PATH'] end + def hooks_directory + ENV['GITALY_GIT_HOOKS_DIR'] + end + def write_buffer_size @write_buffer_size ||= ENV['GITALY_RUBY_WRITE_BUFFER_SIZE'].to_i end @@ -20,10 +24,6 @@ module Gitlab ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] end - def hooks_path - File.join(path, 'hooks') - end - def git_timeout 10800 # TODO make this configurable or eliminate otherwise https://gitlab.com/gitlab-org/gitaly/issues/885 end diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index 5cbe85574ff839316ec7c3bf56ec346bf483cdb7..6ad7e147872a677513092166af28b96b3d3aadba 100644 --- a/ruby/lib/gitlab/git/gitlab_projects.rb +++ b/ruby/lib/gitlab/git/gitlab_projects.rb @@ -23,7 +23,7 @@ module Gitlab Gitlab::Git::GitlabProjects.new( storage_path, gitaly_repository.relative_path, - global_hooks_path: Gitlab.config.gitlab_shell.hooks_path, + global_hooks_path: Gitlab::Git::Hook.directory, logger: Rails.logger ) end diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 94ff5b4980a4506bb6e656691975c3e25bc3fe2d..43b35b39ad533c5f9c255787cbf0f4fad330d1ff 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -1,17 +1,19 @@ -# Gitaly note: JV: looks like this is only used by Gitlab::Git::HooksService in -# app/services. We shouldn't bother migrating this until we know how -# Gitlab::Git::HooksService will be migrated. +# frozen_string_literal: true module Gitlab module Git class Hook - GL_PROTOCOL = 'web'.freeze + def self.directory + Gitlab.config.git.hooks_directory + end + + GL_PROTOCOL = 'web' attr_reader :name, :path, :repository def initialize(name, repository) @name = name @repository = repository - @path = File.join(repo_path, 'hooks', name) + @path = File.join(self.class.directory, name) end def repo_path diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb index f3e6505c96c6fd310cfba02bdf0bbb6e7d6126e1..b782fbc06e61247567e7039aed452d51ab820e39 100644 --- a/ruby/lib/gitlab/git/repository.rb +++ b/ruby/lib/gitlab/git/repository.rb @@ -64,7 +64,9 @@ module Gitlab repo = Rugged::Repository.init_at(repo_path, true) repo.close - symlink_hooks_to = Gitlab.config.gitlab_shell.hooks_path + # TODO: remove this when self healing hooks has been merged at least + # one release ago: https://gitlab.com/gitlab-org/gitaly/merge_requests/886 + symlink_hooks_to = Gitlab::Git::Hook.directory create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present? end diff --git a/ruby/spec/lib/gitlab/config_spec.rb b/ruby/spec/lib/gitlab/config_spec.rb index bd2aaaede2dbbb7780dbbe3f140f5367cf7f31e5..2a503d00521412f355bfd3331adab79351c55d15 100644 --- a/ruby/spec/lib/gitlab/config_spec.rb +++ b/ruby/spec/lib/gitlab/config_spec.rb @@ -15,6 +15,5 @@ describe Gitlab::Config do end it { expect(subject.path).to eq(gitlab_shell_path) } - it { expect(subject.hooks_path).to eq(File.join(gitlab_shell_path, 'hooks')) } end end diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b43e6a15322a15f964f9be6c9ff052b298207298 --- /dev/null +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Gitlab::Git::Hook do + include TestRepo + + describe '.directory' do + it 'does not raise an KeyError' do + expect { described_class.directory }.not_to raise_error + end + end + + describe '#trigger' do + let!(:old_env) { ENV['GITALY_GIT_HOOKS_DIR'] } + let(:tmp_dir) { Dir.mktmpdir } + let(:hook_names) { %w[pre-receive post-receive update] } + let(:repo) { gitlab_git_from_gitaly(test_repo_read_only) } + + before do + hook_names.each do |f| + path = File.join(tmp_dir, f) + File.write(path, script) + FileUtils.chmod("u+x", path) + end + + ENV['GITALY_GIT_HOOKS_DIR'] = tmp_dir + end + + after do + FileUtils.remove_entry(tmp_dir) + ENV['GITALY_GIT_HOOKS_DIR'] = old_env + end + + context 'when the hooks are successful' do + let(:script) { "#!/usr/bin/env true" } + + it 'returns true' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.first).to be(true) + end + end + end + + context 'when the hooks fail' do + let(:script) { "#!/usr/bin/env false" } + + it 'returns false' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.first).to be(false) + end + end + end + end +end diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb index 818c3c44c489f2a4c79e89e70941119c73093d5a..5a7a6e48307937ee50aaca520372ad68977e907e 100644 --- a/ruby/spec/spec_helper.rb +++ b/ruby/spec/spec_helper.rb @@ -8,6 +8,7 @@ require 'factory_bot' Dir[File.join(__dir__, 'support/helpers/*.rb')].each { |f| require f } ENV['GITALY_RUBY_GIT_BIN_PATH'] ||= 'git' +ENV['GITALY_GIT_HOOKS_DIR'] ||= File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks") RSpec.configure do |config| config.include FactoryBot::Syntax::Methods