diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2020-09-09 15:58:27 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2020-09-30 15:07:01 +0300 |
commit | 8f5eecb4bb6f7b0d9bedcf3bded2529a0c01cf09 (patch) | |
tree | 87de9014ffec660f9d958727cfd6b4c8c3e45e97 | |
parent | 538e5d075acdbb46307d293251ae68fe0dd41b42 (diff) |
hooks: Remove Ruby execution path for postreceive
Currently the only hook in production running their Ruby implementation
is the postreceive hook. For GitLab.com the feature flag was toggled,
and performs very well. As such the feature will be expose to all GitLab
installs, as the feature flag is removed in this change.
-rw-r--r-- | .gitignore | 1 | ||||
-rw-r--r-- | cmd/gitaly-hooks/hooks.go | 4 | ||||
-rw-r--r-- | cmd/gitaly-hooks/hooks_test.go | 141 | ||||
-rw-r--r-- | internal/git/receivepack.go | 1 | ||||
-rw-r--r-- | internal/gitaly/service/hook/post_receive.go | 72 | ||||
-rw-r--r-- | internal/gitaly/service/hook/post_receive_test.go | 377 | ||||
-rw-r--r-- | internal/gitaly/service/hook/pre_receive.go | 33 | ||||
-rw-r--r-- | internal/gitaly/service/hook/stream_command.go | 34 | ||||
-rwxr-xr-x | internal/gitaly/service/hook/testdata/gitlab-shell/hooks/post-receive | 10 | ||||
-rw-r--r-- | internal/gitaly/service/operations/rebase_test.go | 18 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags_test.go | 18 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack_test.go | 24 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 4 | ||||
-rw-r--r-- | ruby/lib/gitlab/git/hook.rb | 1 |
14 files changed, 217 insertions, 521 deletions
diff --git a/.gitignore b/.gitignore index 4f6c22b79..9d277070e 100644 --- a/.gitignore +++ b/.gitignore @@ -13,7 +13,6 @@ cmd/gitaly-wrapper/gitaly-wrapper /_support/bin /internal/service/smarthttp/testdata /internal/testhelper/testdata/data -/internal/testhelper/testdata/gitaly-libexec /*.toml /.ruby-bundle /ruby/vendor/bundle diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go index 009590eee..dda50710c 100644 --- a/cmd/gitaly-hooks/hooks.go +++ b/cmd/gitaly-hooks/hooks.go @@ -172,10 +172,6 @@ func main() { } } - if os.Getenv(featureflag.GoPostReceiveHookEnvVar) == "true" { - environment = append(environment, fmt.Sprintf("%s=true", featureflag.GoPostReceiveHookEnvVar)) - } - if err := postReceiveHookStream.Send(&gitalypb.PostReceiveHookRequest{ Repository: repository, EnvironmentVariables: environment, diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 846ab3504..5e219e983 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -18,7 +18,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gitalyhook "gitlab.com/gitlab-org/gitaly/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/hook" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -228,82 +227,80 @@ func testHooksPrePostReceive(t *testing.T) { hookNames := []string{"pre-receive", "post-receive"} for _, hookName := range hookNames { - for _, useGoPostReceive := range []bool{true, false} { - t.Run(fmt.Sprintf("hookName: %s, using Go PostReceive: %v", hookName, useGoPostReceive), func(t *testing.T) { - customHookOutputPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName) - defer cleanup() + t.Run(fmt.Sprintf("hookName: %s", hookName), func(t *testing.T) { + customHookOutputPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName) + defer cleanup() - config.Config.Gitlab.URL = serverURL - config.Config.Gitlab.SecretFile = filepath.Join(tempGitlabShellDir, ".gitlab_shell_secret") - config.Config.Gitlab.HTTPSettings.User = gitlabUser - config.Config.Gitlab.HTTPSettings.Password = gitlabPassword + config.Config.Gitlab.URL = serverURL + config.Config.Gitlab.SecretFile = filepath.Join(tempGitlabShellDir, ".gitlab_shell_secret") + config.Config.Gitlab.HTTPSettings.User = gitlabUser + config.Config.Gitlab.HTTPSettings.Password = gitlabPassword - gitlabAPI, err := gitalyhook.NewGitlabAPI(config.Config.Gitlab) - require.NoError(t, err) + gitlabAPI, err := gitalyhook.NewGitlabAPI(config.Config.Gitlab) + require.NoError(t, err) - socket, stop := runHookServiceServerWithAPI(t, token, gitlabAPI) - defer stop() + socket, stop := runHookServiceServerWithAPI(t, token, gitlabAPI) + defer stop() - var stderr, stdout bytes.Buffer - stdin := bytes.NewBuffer([]byte(changes)) - hookPath, err := filepath.Abs(fmt.Sprintf("../../ruby/git-hooks/%s", hookName)) - require.NoError(t, err) - cmd := exec.Command(hookPath) - cmd.Stderr = &stderr - cmd.Stdout = &stdout - cmd.Stdin = stdin - cmd.Env = append(testhelper.EnvForHooks( - t, - tempGitlabShellDir, - socket, - token, - testRepo, - testhelper.GlHookValues{ - GLID: glID, - GLUsername: glUsername, - GLRepo: glRepository, - GLProtocol: glProtocol, - GitObjectDir: c.GitObjectDir, - GitAlternateObjectDirs: c.GitAlternateObjectDirs, - }, - testhelper.ProxyValues{ - HTTPProxy: httpProxy, - HTTPSProxy: httpsProxy, - NoProxy: noProxy, - }, - gitPushOptions..., - ), fmt.Sprintf("%s=%v", featureflag.GoPostReceiveHookEnvVar, useGoPostReceive)) - - cmd.Dir = testRepoPath - - require.NoError(t, cmd.Run()) - require.Empty(t, stderr.String()) - require.Empty(t, stdout.String()) + var stderr, stdout bytes.Buffer + stdin := bytes.NewBuffer([]byte(changes)) + hookPath, err := filepath.Abs(fmt.Sprintf("../../ruby/git-hooks/%s", hookName)) + require.NoError(t, err) + cmd := exec.Command(hookPath) + cmd.Stderr = &stderr + cmd.Stdout = &stdout + cmd.Stdin = stdin + cmd.Env = testhelper.EnvForHooks( + t, + tempGitlabShellDir, + socket, + token, + testRepo, + testhelper.GlHookValues{ + GLID: glID, + GLUsername: glUsername, + GLRepo: glRepository, + GLProtocol: glProtocol, + GitObjectDir: c.GitObjectDir, + GitAlternateObjectDirs: c.GitAlternateObjectDirs, + }, + testhelper.ProxyValues{ + HTTPProxy: httpProxy, + HTTPSProxy: httpsProxy, + NoProxy: noProxy, + }, + gitPushOptions..., + ) - output := string(testhelper.MustReadFile(t, customHookOutputPath)) - requireContainsOnce(t, output, "GL_USERNAME="+glUsername) - requireContainsOnce(t, output, "GL_ID="+glID) - requireContainsOnce(t, output, "GL_REPOSITORY="+glRepository) - requireContainsOnce(t, output, "HTTP_PROXY="+httpProxy) - requireContainsOnce(t, output, "http_proxy="+httpProxy) - requireContainsOnce(t, output, "HTTPS_PROXY="+httpsProxy) - requireContainsOnce(t, output, "https_proxy="+httpsProxy) - requireContainsOnce(t, output, "no_proxy="+noProxy) - requireContainsOnce(t, output, "NO_PROXY="+noProxy) - - if hookName == "pre-receive" { - gitObjectDirMatches := gitObjectDirRegex.FindStringSubmatch(output) - require.Len(t, gitObjectDirMatches, 2) - require.Equal(t, gitObjectDir, gitObjectDirMatches[1]) - - gitAlternateObjectDirMatches := gitAlternateObjectDirRegex.FindStringSubmatch(output) - require.Len(t, gitAlternateObjectDirMatches, 2) - require.Equal(t, strings.Join(gitAlternateObjectDirs, ":"), gitAlternateObjectDirMatches[1]) - } else { - require.Contains(t, output, "GL_PROTOCOL="+glProtocol) - } - }) - } + cmd.Dir = testRepoPath + + require.NoError(t, cmd.Run()) + require.Empty(t, stderr.String()) + require.Empty(t, stdout.String()) + + output := string(testhelper.MustReadFile(t, customHookOutputPath)) + requireContainsOnce(t, output, "GL_USERNAME="+glUsername) + requireContainsOnce(t, output, "GL_ID="+glID) + requireContainsOnce(t, output, "GL_REPOSITORY="+glRepository) + requireContainsOnce(t, output, "HTTP_PROXY="+httpProxy) + requireContainsOnce(t, output, "http_proxy="+httpProxy) + requireContainsOnce(t, output, "HTTPS_PROXY="+httpsProxy) + requireContainsOnce(t, output, "https_proxy="+httpsProxy) + requireContainsOnce(t, output, "no_proxy="+noProxy) + requireContainsOnce(t, output, "NO_PROXY="+noProxy) + + if hookName == "pre-receive" { + gitObjectDirMatches := gitObjectDirRegex.FindStringSubmatch(output) + require.Len(t, gitObjectDirMatches, 2) + require.Equal(t, gitObjectDir, gitObjectDirMatches[1]) + + gitAlternateObjectDirMatches := gitAlternateObjectDirRegex.FindStringSubmatch(output) + require.Len(t, gitAlternateObjectDirMatches, 2) + require.Equal(t, strings.Join(gitAlternateObjectDirs, ":"), gitAlternateObjectDirMatches[1]) + } else { + require.Contains(t, output, "GL_PROTOCOL="+glProtocol) + } + }) } } diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go index 9158a82eb..f75afd55a 100644 --- a/internal/git/receivepack.go +++ b/internal/git/receivepack.go @@ -47,7 +47,6 @@ func ReceivePackHookEnv(ctx context.Context, req ReceivePackRequest) ([]string, fmt.Sprintf("GITALY_SOCKET=" + config.GitalyInternalSocketPath()), fmt.Sprintf("GITALY_REPO=%s", repo), fmt.Sprintf("GITALY_TOKEN=%s", config.Config.Auth.Token), - fmt.Sprintf("%s=%s", featureflag.GoPostReceiveHookEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.GoPostReceiveHook))), fmt.Sprintf("%s=%s", featureflag.ReferenceTransactionHookEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.ReferenceTransactionHook))), }, gitlabshellEnv...) diff --git a/internal/gitaly/service/hook/post_receive.go b/internal/gitaly/service/hook/post_receive.go index 84fb27058..2fd3ecad4 100644 --- a/internal/gitaly/service/hook/post_receive.go +++ b/internal/gitaly/service/hook/post_receive.go @@ -3,21 +3,13 @@ package hook import ( "errors" "fmt" - "io" - "io/ioutil" "os/exec" - "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" ) -func isGoPostReceiveHookUsed(env []string) bool { - return getEnvVar(featureflag.GoPostReceiveHookEnvVar, env) == "true" -} - func postReceiveHookResponse(stream gitalypb.HookService_PostReceiveHookServer, code int32, stderr string) error { if err := stream.Send(&gitalypb.PostReceiveHookResponse{ ExitStatus: &gitalypb.ExitStatus{Value: code}, @@ -39,10 +31,6 @@ func (s *server) PostReceiveHook(stream gitalypb.HookService_PostReceiveHookServ return helper.ErrInvalidArgument(err) } - if !isGoPostReceiveHookUsed(firstRequest.GetEnvironmentVariables()) { - return postReceiveHookRuby(firstRequest, stream) - } - stdin := streamio.NewReader(func() ([]byte, error) { req, err := stream.Recv() return req.GetStdin(), err @@ -52,7 +40,7 @@ func (s *server) PostReceiveHook(stream gitalypb.HookService_PostReceiveHookServ env, err := hookRequestEnv(firstRequest) if err != nil { - return err + return helper.ErrInternal(err) } if err := s.manager.PostReceiveHook( @@ -75,64 +63,6 @@ func (s *server) PostReceiveHook(stream gitalypb.HookService_PostReceiveHookServ return postReceiveHookResponse(stream, 0, "") } -func postReceiveHookRuby(firstRequest *gitalypb.PostReceiveHookRequest, stream gitalypb.HookService_PostReceiveHookServer) error { - hookEnv, err := hookRequestEnv(firstRequest) - if err != nil { - return helper.ErrInternal(err) - } - - hookEnv = append(hookEnv, hooks.GitPushOptions(firstRequest.GetGitPushOptions())...) - - primary, err := isPrimary(hookEnv) - if err != nil { - return helper.ErrInternalf("could not check role: %w", err) - } - - stdin := streamio.NewReader(func() ([]byte, error) { - req, err := stream.Recv() - return req.GetStdin(), err - }) - - var status int32 - if primary { - stdout := streamio.NewWriter(func(p []byte) error { return stream.Send(&gitalypb.PostReceiveHookResponse{Stdout: p}) }) - stderr := streamio.NewWriter(func(p []byte) error { return stream.Send(&gitalypb.PostReceiveHookResponse{Stderr: p}) }) - - repoPath, err := helper.GetRepoPath(firstRequest.GetRepository()) - if err != nil { - return helper.ErrInternal(err) - } - - c := exec.Command(gitlabShellHook("post-receive")) - c.Dir = repoPath - - status, err = streamCommandResponse( - stream.Context(), - stdin, - stdout, stderr, - c, - hookEnv, - ) - - if err != nil { - return helper.ErrInternal(err) - } - } else { - _, err := io.Copy(ioutil.Discard, stdin) - if err != nil { - return helper.ErrInternal(err) - } - } - - if err := stream.SendMsg(&gitalypb.PostReceiveHookResponse{ - ExitStatus: &gitalypb.ExitStatus{Value: status}, - }); err != nil { - return helper.ErrInternal(err) - } - - return nil -} - func validatePostReceiveHookRequest(in *gitalypb.PostReceiveHookRequest) error { if in.GetRepository() == nil { return errors.New("repository is empty") diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go index 52b108b40..bd09c2d00 100644 --- a/internal/gitaly/service/hook/post_receive_test.go +++ b/internal/gitaly/service/hook/post_receive_test.go @@ -2,9 +2,7 @@ package hook import ( "bytes" - "fmt" "io" - "os" "path/filepath" "testing" @@ -13,7 +11,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gitalyhook "gitlab.com/gitlab-org/gitaly/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/internal/helper/text" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -54,207 +51,110 @@ func transactionEnv(t *testing.T, primary bool) string { return env } -func TestPostReceive(t *testing.T) { - rubyDir := config.Config.Ruby.Dir - defer func(rubyDir string) { - config.Config.Ruby.Dir = rubyDir - }(rubyDir) +func TestHooksMissingStdin(t *testing.T) { + user, password, secretToken := "user", "password", "secret token" + tempDir, cleanup := testhelper.CreateTemporaryGitlabShellDir(t) + defer cleanup() + testhelper.WriteShellSecretFile(t, tempDir, secretToken) - cwd, err := os.Getwd() - require.NoError(t, err) - config.Config.Ruby.Dir = filepath.Join(cwd, "testdata") + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() - serverSocketPath, stop := runHooksServer(t, config.Config) - defer stop() + c := testhelper.GitlabTestServerOptions{ + User: user, + Password: password, + SecretToken: secretToken, + GLID: "key_id", + GLRepository: "repository", + Changes: "changes", + PostReceiveCounterDecreased: true, + Protocol: "protocol", + RepoPath: testRepoPath, + } - testRepo, _, cleanupFn := testhelper.NewTestRepo(t) - defer cleanupFn() + serverURL, cleanup := testhelper.NewGitlabTestServer(t, c) + defer cleanup() - client, conn := newHooksClient(t, serverSocketPath) - defer conn.Close() + gitlabConfig := config.Gitlab{ + SecretFile: filepath.Join(tempDir, ".gitlab_shell_secret"), + URL: serverURL, + HTTPSettings: config.HTTPSettings{ + User: user, + Password: password, + }, + } + + defer func(cfg config.Cfg) { + config.Config = cfg + }(config.Config) + + config.Config.Gitlab = gitlabConfig + + api, err := gitalyhook.NewGitlabAPI(gitlabConfig) + require.NoError(t, err) testCases := []struct { - desc string - stdin io.Reader - req gitalypb.PostReceiveHookRequest - status int32 - stdout string - stderr string + desc string + env string + fail bool }{ { - desc: "everything OK", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key_id", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"}, - GitPushOptions: []string{"option0", "option1"}}, - status: 0, - stdout: "OK", - stderr: "", - }, - { - desc: "missing stdin", - stdin: bytes.NewBuffer(nil), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key_id", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"}, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "missing gl_id", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"}, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "missing gl_username", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key-123", "GL_USERNAME=", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"}, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "missing gl_protocol", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key-123", "GL_USERNAME=username", "GL_PROTOCOL=", "GL_REPOSITORY=repository"}, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "missing gl_repository value", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key-123", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY="}, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "missing git push option", - stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{"GL_ID=key-123", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", - }, - { - desc: "primary fails with missing stdin because hook gets executed", - stdin: bytes.NewBuffer(nil), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{ - "GL_ID=key_id", - "GL_USERNAME=username", - "GL_PROTOCOL=protocol", - "GL_REPOSITORY=repository", - transactionEnv(t, true), - }, - GitPushOptions: []string{"option0"}, - }, - status: 1, - stdout: "", - stderr: "FAIL", + desc: "empty stdin fails if primary", + env: transactionEnv(t, true), + fail: true, }, { - desc: "secondary succeeds with missing stdin because hook does not get executed", - stdin: bytes.NewBuffer(nil), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{ - "GL_ID=key_id", - "GL_USERNAME=username", - "GL_PROTOCOL=protocol", - "GL_REPOSITORY=repository", - transactionEnv(t, false), - }, - GitPushOptions: []string{"option0"}, - }, - status: 0, - stdout: "", - stderr: "", - }, - { - desc: "Go hook correctly honors the primary flag", - stdin: bytes.NewBuffer(nil), - req: gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: []string{ - "GL_ID=key_id", - "GL_USERNAME=username", - "GL_PROTOCOL=protocol", - "GL_REPOSITORY=repository", - transactionEnv(t, false), - fmt.Sprintf("%s=true", featureflag.GoPostReceiveHookEnvVar), - }, - GitPushOptions: []string{"option0"}, - }, - status: 0, - stdout: "", - stderr: "", + desc: "empty stdin success on secondary", + env: transactionEnv(t, false), }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + serverSocketPath, stop := runHooksServerWithAPI(t, api, config.Config) + defer stop() + + client, conn := newHooksClient(t, serverSocketPath) + defer conn.Close() + ctx, cancel := testhelper.Context() defer cancel() stream, err := client.PostReceiveHook(ctx) require.NoError(t, err) - require.NoError(t, stream.Send(&tc.req)) + require.NoError(t, stream.Send(&gitalypb.PostReceiveHookRequest{ + Repository: testRepo, + EnvironmentVariables: []string{ + "GL_ID=key_id", + "GL_USERNAME=username", + "GL_PROTOCOL=protocol", + "GL_REPOSITORY=repository", tc.env}, + })) go func() { writer := streamio.NewWriter(func(p []byte) error { return stream.Send(&gitalypb.PostReceiveHookRequest{Stdin: p}) }) - _, err := io.Copy(writer, tc.stdin) + _, err := io.Copy(writer, bytes.NewBuffer(nil)) require.NoError(t, err) require.NoError(t, stream.CloseSend(), "close send") }() var status int32 - var stdout, stderr bytes.Buffer for { resp, err := stream.Recv() if err == io.EOF { break } - _, err = stdout.Write(resp.GetStdout()) - require.NoError(t, err) - stderr.Write(resp.GetStderr()) status = resp.GetExitStatus().GetValue() } - assert.Equal(t, tc.status, status) - assert.Equal(t, tc.stderr, text.ChompBytes(stderr.Bytes()), "hook stderr") - assert.Equal(t, tc.stdout, text.ChompBytes(stdout.Bytes()), "hook stdout") + if tc.fail { + require.NotEqual(t, int32(0), status, "exit code should be non-zero") + } else { + require.Equal(t, int32(0), status, "exit code unequal") + } }) } } @@ -297,94 +197,91 @@ To create a merge request for okay, visit: testhelper.WriteShellSecretFile(t, tempDir, secretToken) for _, tc := range testCases { - for _, useGoPostReceive := range []bool{true, false} { - t.Run(fmt.Sprintf("%s:use_go_post_receive:%v", tc.desc, useGoPostReceive), func(t *testing.T) { - c := testhelper.GitlabTestServerOptions{ - User: user, - Password: password, - SecretToken: secretToken, - GLID: "key_id", - GLRepository: "repository", - Changes: "changes", - PostReceiveCounterDecreased: true, - PostReceiveMessages: tc.basicMessages, - PostReceiveAlerts: tc.alertMessages, - Protocol: "protocol", - RepoPath: testRepoPath, - } + t.Run(tc.desc, func(t *testing.T) { + c := testhelper.GitlabTestServerOptions{ + User: user, + Password: password, + SecretToken: secretToken, + GLID: "key_id", + GLRepository: "repository", + Changes: "changes", + PostReceiveCounterDecreased: true, + PostReceiveMessages: tc.basicMessages, + PostReceiveAlerts: tc.alertMessages, + Protocol: "protocol", + RepoPath: testRepoPath, + } - serverURL, cleanup := testhelper.NewGitlabTestServer(t, c) - defer cleanup() + serverURL, cleanup := testhelper.NewGitlabTestServer(t, c) + defer cleanup() - gitlabConfig := config.Gitlab{ - SecretFile: filepath.Join(tempDir, ".gitlab_shell_secret"), - URL: serverURL, - HTTPSettings: config.HTTPSettings{ - User: user, - Password: password, - }, - } + gitlabConfig := config.Gitlab{ + SecretFile: filepath.Join(tempDir, ".gitlab_shell_secret"), + URL: serverURL, + HTTPSettings: config.HTTPSettings{ + User: user, + Password: password, + }, + } - defer func(cfg config.Cfg) { - config.Config = cfg - }(config.Config) + defer func(cfg config.Cfg) { + config.Config = cfg + }(config.Config) - config.Config.Gitlab = gitlabConfig + config.Config.Gitlab = gitlabConfig - api, err := gitalyhook.NewGitlabAPI(gitlabConfig) - require.NoError(t, err) + api, err := gitalyhook.NewGitlabAPI(gitlabConfig) + require.NoError(t, err) - serverSocketPath, stop := runHooksServerWithAPI(t, api, config.Config) - defer stop() + serverSocketPath, stop := runHooksServerWithAPI(t, api, config.Config) + defer stop() - client, conn := newHooksClient(t, serverSocketPath) - defer conn.Close() + client, conn := newHooksClient(t, serverSocketPath) + defer conn.Close() - ctx, cancel := testhelper.Context() - defer cancel() + ctx, cancel := testhelper.Context() + defer cancel() - stream, err := client.PostReceiveHook(ctx) + stream, err := client.PostReceiveHook(ctx) + require.NoError(t, err) + + envVars := []string{ + "GL_ID=key_id", + "GL_USERNAME=username", + "GL_PROTOCOL=protocol", + "GL_REPOSITORY=repository", + } + + require.NoError(t, stream.Send(&gitalypb.PostReceiveHookRequest{ + Repository: testRepo, + EnvironmentVariables: envVars})) + + go func() { + writer := streamio.NewWriter(func(p []byte) error { + return stream.Send(&gitalypb.PostReceiveHookRequest{Stdin: p}) + }) + _, err := writer.Write([]byte("changes")) require.NoError(t, err) + require.NoError(t, stream.CloseSend(), "close send") + }() - envVars := []string{ - "GL_ID=key_id", - "GL_USERNAME=username", - "GL_PROTOCOL=protocol", - "GL_REPOSITORY=repository", - fmt.Sprintf("%s=%v", featureflag.GoPostReceiveHookEnvVar, useGoPostReceive), + var status int32 + var stdout, stderr bytes.Buffer + for { + resp, err := stream.Recv() + if err == io.EOF { + break } - require.NoError(t, stream.Send(&gitalypb.PostReceiveHookRequest{ - Repository: testRepo, - EnvironmentVariables: envVars})) - - go func() { - writer := streamio.NewWriter(func(p []byte) error { - return stream.Send(&gitalypb.PostReceiveHookRequest{Stdin: p}) - }) - _, err := writer.Write([]byte("changes")) - require.NoError(t, err) - require.NoError(t, stream.CloseSend(), "close send") - }() - - var status int32 - var stdout, stderr bytes.Buffer - for { - resp, err := stream.Recv() - if err == io.EOF { - break - } - - _, err = stdout.Write(resp.GetStdout()) - require.NoError(t, err) - stderr.Write(resp.GetStderr()) - status = resp.GetExitStatus().GetValue() - } + _, err = stdout.Write(resp.GetStdout()) + require.NoError(t, err) + stderr.Write(resp.GetStderr()) + status = resp.GetExitStatus().GetValue() + } - assert.Equal(t, int32(0), status) - assert.Equal(t, "", text.ChompBytes(stderr.Bytes()), "hook stderr") - assert.Equal(t, tc.expectedStdout, text.ChompBytes(stdout.Bytes()), "hook stdout") - }) - } + assert.Equal(t, int32(0), status) + assert.Equal(t, "", text.ChompBytes(stderr.Bytes()), "hook stderr") + assert.Equal(t, tc.expectedStdout, text.ChompBytes(stdout.Bytes()), "hook stdout") + }) } } diff --git a/internal/gitaly/service/hook/pre_receive.go b/internal/gitaly/service/hook/pre_receive.go index c9a05e778..112ea7d53 100644 --- a/internal/gitaly/service/hook/pre_receive.go +++ b/internal/gitaly/service/hook/pre_receive.go @@ -4,14 +4,10 @@ import ( "errors" "fmt" "os/exec" - "path/filepath" - "strings" "gitlab.com/gitlab-org/gitaly/internal/git/hooks" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" ) @@ -45,24 +41,6 @@ func preReceiveEnv(req prePostRequest) ([]string, error) { return env, nil } -func gitlabShellHook(hookName string) string { - return filepath.Join(config.Config.Ruby.Dir, "gitlab-shell", "hooks", hookName) -} - -func isPrimary(env []string) (bool, error) { - tx, err := metadata.TransactionFromEnv(env) - if err != nil { - if errors.Is(err, metadata.ErrTransactionNotFound) { - // If there is no transaction, then we only ever write - // to the primary. Thus, we return true. - return true, nil - } - return false, err - } - - return tx.Primary, nil -} - func (s *server) PreReceiveHook(stream gitalypb.HookService_PreReceiveHookServer) error { firstRequest, err := stream.Recv() if err != nil { @@ -124,14 +102,3 @@ func preReceiveHookResponse(stream gitalypb.HookService_PreReceiveHookServer, co return nil } - -func getEnvVar(key string, vars []string) string { - for _, varPair := range vars { - kv := strings.SplitN(varPair, "=", 2) - if kv[0] == key { - return kv[1] - } - } - - return "" -} diff --git a/internal/gitaly/service/hook/stream_command.go b/internal/gitaly/service/hook/stream_command.go deleted file mode 100644 index 2893dd5a8..000000000 --- a/internal/gitaly/service/hook/stream_command.go +++ /dev/null @@ -1,34 +0,0 @@ -package hook - -import ( - "context" - "io" - "os/exec" - - "gitlab.com/gitlab-org/gitaly/internal/command" - "gitlab.com/gitlab-org/gitaly/internal/helper" -) - -func streamCommandResponse( - ctx context.Context, - stdin io.Reader, - stdout, stderr io.Writer, - c *exec.Cmd, - env []string, -) (int32, error) { - cmd, err := command.New(ctx, c, stdin, stdout, stderr, env...) - if err != nil { - return 1, helper.ErrInternal(err) - } - - err = cmd.Wait() - if err == nil { - return 0, nil - } - - if code, ok := command.ExitStatus(err); ok { - return int32(code), nil - } - - return 1, err -} diff --git a/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/post-receive b/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/post-receive deleted file mode 100755 index 09af4c677..000000000 --- a/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/post-receive +++ /dev/null @@ -1,10 +0,0 @@ -#!/usr/bin/env ruby - -# Tests inputs to post-receive - -abort("FAIL") if $stdin.read.empty? -abort("FAIL") if %w[GL_ID GL_REPOSITORY GL_PROTOCOL GL_USERNAME].any? { |k| ENV[k].nil? || ENV[k].empty? } -# git push options are not required. This is only for the sake of testing the values get through -abort("FAIL") if %w[GIT_PUSH_OPTION_COUNT GIT_PUSH_OPTION_0].any? { |k| ENV[k].nil? || ENV[k].empty? } - -puts "OK" diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go index 2149632e0..1741968e6 100644 --- a/internal/gitaly/service/operations/rebase_test.go +++ b/internal/gitaly/service/operations/rebase_test.go @@ -3,7 +3,6 @@ package operations //lint:file-ignore SA1019 due to planned removal in issue https://gitlab.com/gitlab-org/gitaly/issues/1628 import ( - "context" "fmt" "strings" "testing" @@ -13,7 +12,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/catfile" gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -25,9 +23,6 @@ var ( ) func TestSuccessfulUserRebaseConfirmableRequest(t *testing.T) { - featureSets, err := testhelper.NewFeatureSets(nil, featureflag.GoPostReceiveHook) - require.NoError(t, err) - var ruby rubyserver.Server pushOptions := []string{"ci.skip", "test=value"} @@ -40,18 +35,9 @@ func TestSuccessfulUserRebaseConfirmableRequest(t *testing.T) { serverSocketPath, stop := runOperationServiceServerWithRubyServer(t, &ruby) defer stop() - for _, featureSet := range featureSets { - t.Run(featureSet.String(), func(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - ctx = featureSet.Disable(ctx) - testSuccessfulUserRebaseConfirmableRequest(t, ctx, serverSocketPath, pushOptions) - }) - } -} + ctxOuter, cancel := testhelper.Context() + defer cancel() -func testSuccessfulUserRebaseConfirmableRequest(t *testing.T, ctxOuter context.Context, serverSocketPath string, pushOptions []string) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index cf1b4584c..518105134 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -61,21 +61,6 @@ func TestSuccessfulUserDeleteTagRequest(t *testing.T) { } func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { - featureSets, err := testhelper.NewFeatureSets(nil, featureflag.GoPostReceiveHook) - require.NoError(t, err) - - for _, featureSet := range featureSets { - t.Run("disabled "+featureSet.String(), func(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - ctx = featureSet.Disable(ctx) - testSuccessfulGitHooksForUserDeleteTagRequest(t, ctx) - }) - } -} - -func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -88,6 +73,9 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con tagNameInput := "to-be-déleted-soon-tag" defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run() + ctx, cancel := testhelper.Context() + defer cancel() + request := &gitalypb.UserDeleteTagRequest{ Repository: testRepo, TagName: []byte(tagNameInput), diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 183a73444..40bbc6b8c 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -355,26 +355,9 @@ func drainPostReceivePackResponse(stream gitalypb.SmartHTTPService_PostReceivePa } func TestPostReceivePackToHooks(t *testing.T) { - defer func(cfg config.Cfg) { - config.Config = cfg - }(config.Config) - - features, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{featureflag.GoPostReceiveHook}) - require.NoError(t, err) - - for _, feature := range features { - t.Run("disabled "+feature.String(), func(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - ctx = feature.Disable(ctx) - - testPostReceivePackToHooks(t, ctx) - }) - } -} + ctx, cancel := testhelper.Context() + defer cancel() -func testPostReceivePackToHooks(t *testing.T, ctx context.Context) { secretToken := "secret token" glRepository := "some_repo" glID := "key-123" @@ -390,6 +373,9 @@ func testPostReceivePackToHooks(t *testing.T, ctx context.Context) { tempGitlabShellDir, cleanup := testhelper.CreateTemporaryGitlabShellDir(t) defer cleanup() + defer func(cfg config.Cfg) { + config.Config = cfg + }(config.Config) config.Config.GitlabShell.Dir = tempGitlabShellDir defer func(override string) { diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 6f48f055d..812556d55 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -13,8 +13,6 @@ var ( GoFetchSourceBranch = FeatureFlag{Name: "go_fetch_source_branch", OnByDefault: false} // DistributedReads allows praefect to redirect accessor operations to up-to-date secondaries DistributedReads = FeatureFlag{Name: "distributed_reads", OnByDefault: true} - // GoPostReceiveHook will bypass the ruby post-receive hook and use the go implementation - GoPostReceiveHook = FeatureFlag{Name: "go_postreceive_hook", OnByDefault: true} // ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true} // ReferenceTranasctiionsPrimaryWins will change transaction registration such that @@ -40,7 +38,6 @@ var ( var All = []FeatureFlag{ GoFetchSourceBranch, DistributedReads, - GoPostReceiveHook, ReferenceTransactions, ReferenceTransactionsPrimaryWins, ReferenceTransactionHook, @@ -51,6 +48,5 @@ var All = []FeatureFlag{ } const ( - GoPostReceiveHookEnvVar = "GITALY_GO_POSTRECEIVE" ReferenceTransactionHookEnvVar = "GITALY_REFERENCE_TRANSACTION_HOOK" ) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 4a6ee936f..070c93ab3 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -133,7 +133,6 @@ module Gitlab 'GIT_DIR' => repo_path, 'GITALY_REPO' => repository.gitaly_repository.to_json, 'GITALY_SOCKET' => Gitlab.config.gitaly.internal_socket, - 'GITALY_GO_POSTRECEIVE' => repository.feature_enabled?('go-postreceive-hook').to_s, 'GITALY_REFERENCE_TRANSACTION_HOOK' => repository.feature_enabled?('reference-transaction-hook').to_s } end |