hooks: Remove prereceive hook Ruby implementation

This change is part of a larger effort to move away from using the hooks
being implemented in Ruby, and moves the execution to Go to speed up the
hooks.

The prereceive hook was already enabled by default, meaning that the
change made doesn't influence anyone that hasn't explicity turned it
off. Given there were no bug reports, the old implementation is removed
as fallback.

11 files changed, 8 insertions, 269 deletions

diff --git a/changelogs/unreleased/zj-prereceive-feature-flag.yml b/changelogs/unreleased/zj-prereceive-feature-flag.yml
new file mode 100644
index 000000000..6132705c1
--- /dev/null
+++ b/changelogs/unreleased/zj-prereceive-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: 'hooks: Remove prereceive hook Ruby implementation'
+merge_request: 2519
+author:
+type: changed
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index ba7414107..61ea7979b 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -125,9 +125,7 @@ func main() {
 			logger.Fatalf("error when getting preReceiveHookStream client for %q: %v", subCmd, err)
 		}
 
-		environment := glValues()
-		environment = append(environment, gitObjectDirs()...)
-
+		environment := append(glValues(), gitObjectDirs()...)
 		for _, key := range []string{metadata.PraefectEnvKey, metadata.TransactionEnvKey} {
 			if value, ok := os.LookupEnv(key); ok {
 				env := fmt.Sprintf("%s=%s", key, value)
@@ -135,10 +133,6 @@ func main() {
 			}
 		}
 
-		if os.Getenv(featureflag.GoPreReceiveHookEnvVar) == "true" {
-			environment = append(environment, fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar))
-		}
-
 		if err := preReceiveHookStream.Send(&gitalypb.PreReceiveHookRequest{
 			Repository:           repository,
 			EnvironmentVariables: environment,
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index ceed2fe02..40d282748 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -227,7 +227,7 @@ func testHooksPrePostReceive(t *testing.T) {
 
 	hookNames := []string{"pre-receive", "post-receive"}
 
-	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{featureflag.GoPreReceiveHook, featureflag.GoPostReceiveHook})
+	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{featureflag.GoPostReceiveHook})
 	require.NoError(t, err)
 
 	for _, hookName := range hookNames {
@@ -277,10 +277,6 @@ func testHooksPrePostReceive(t *testing.T) {
 					gitPushOptions...,
 				)
 
-				if !featureSet.IsDisabled(featureflag.GoPreReceiveHook) {
-					cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar))
-				}
-
 				cmd.Dir = testRepoPath
 
 				require.NoError(t, cmd.Run())
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index 5d9384c2e..9158a82eb 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.GoPreReceiveHookEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.GoPreReceiveHook))),
 		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_test.go b/internal/gitaly/service/hook/post_receive_test.go
index cf86e9a22..4d9f953d8 100644
--- a/internal/gitaly/service/hook/post_receive_test.go
+++ b/internal/gitaly/service/hook/post_receive_test.go
@@ -351,9 +351,6 @@ To create a merge request for okay, visit:
 					"GL_USERNAME=username",
 					"GL_PROTOCOL=protocol",
 					"GL_REPOSITORY=repository"}
-				if useGoPreReceive {
-					envVars = append(envVars, "GITALY_GO_PRERECEIVE=true")
-				}
 
 				require.NoError(t, stream.Send(&gitalypb.PostReceiveHookRequest{
 					Repository:           testRepo,
diff --git a/internal/gitaly/service/hook/pre_receive.go b/internal/gitaly/service/hook/pre_receive.go
index 833d9af32..c9a05e778 100644
--- a/internal/gitaly/service/hook/pre_receive.go
+++ b/internal/gitaly/service/hook/pre_receive.go
@@ -1,11 +1,8 @@
 package hook
 
 import (
-	"crypto/sha1"
 	"errors"
 	"fmt"
-	"io"
-	"io/ioutil"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -14,7 +11,6 @@ import (
 	"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/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -79,10 +75,6 @@ func (s *server) PreReceiveHook(stream gitalypb.HookService_PreReceiveHookServer
 	reqEnvVars := firstRequest.GetEnvironmentVariables()
 	repository := firstRequest.GetRepository()
 
-	if !useGoPreReceiveHook(reqEnvVars) {
-		return s.preReceiveHookRuby(firstRequest, stream)
-	}
-
 	stdin := streamio.NewReader(func() ([]byte, error) {
 		req, err := stream.Recv()
 		return req.GetStdin(), err
@@ -122,10 +114,6 @@ func validatePreReceiveHookRequest(in *gitalypb.PreReceiveHookRequest) error {
 	return nil
 }
 
-func useGoPreReceiveHook(env []string) bool {
-	return getEnvVar(featureflag.GoPreReceiveHookEnvVar, env) == "true"
-}
-
 func preReceiveHookResponse(stream gitalypb.HookService_PreReceiveHookServer, code int32, stderr string) error {
 	if err := stream.Send(&gitalypb.PreReceiveHookResponse{
 		ExitStatus: &gitalypb.ExitStatus{Value: code},
@@ -137,80 +125,6 @@ func preReceiveHookResponse(stream gitalypb.HookService_PreReceiveHookServer, co
 	return nil
 }
 
-func (s *server) preReceiveHookRuby(firstRequest *gitalypb.PreReceiveHookRequest, stream gitalypb.HookService_PreReceiveHookServer) error {
-	referenceUpdatesHasher := sha1.New()
-
-	stdin := streamio.NewReader(func() ([]byte, error) {
-		req, err := stream.Recv()
-		if err != nil {
-			return nil, err
-		}
-
-		stdin := req.GetStdin()
-		if _, err := referenceUpdatesHasher.Write(stdin); err != nil {
-			return stdin, err
-		}
-
-		return stdin, nil
-	})
-
-	env, err := preReceiveEnv(firstRequest)
-	if err != nil {
-		return helper.ErrInternal(err)
-	}
-
-	primary, err := isPrimary(env)
-	if err != nil {
-		return helper.ErrInternalf("could not check role: %w", err)
-	}
-
-	var status int32
-
-	// Only the primary should execute hooks.
-	if primary {
-		stdout := streamio.NewWriter(func(p []byte) error { return stream.Send(&gitalypb.PreReceiveHookResponse{Stdout: p}) })
-		stderr := streamio.NewWriter(func(p []byte) error { return stream.Send(&gitalypb.PreReceiveHookResponse{Stderr: p}) })
-
-		repoPath, err := helper.GetRepoPath(firstRequest.GetRepository())
-		if err != nil {
-			return helper.ErrInternal(err)
-		}
-
-		c := exec.Command(gitlabShellHook("pre-receive"))
-		c.Dir = repoPath
-
-		status, err = streamCommandResponse(
-			stream.Context(),
-			stdin,
-			stdout, stderr,
-			c,
-			env,
-		)
-		if err != nil {
-			return helper.ErrInternal(err)
-		}
-	} else {
-		// We need to read all of stdin on secondaries so that we
-		// arrive at the same hash as the primary.
-		_, err := io.Copy(ioutil.Discard, stdin)
-		if err != nil {
-			return helper.ErrInternal(err)
-		}
-	}
-
-	if err := s.manager.VoteOnTransaction(stream.Context(), referenceUpdatesHasher.Sum(nil), env); err != nil {
-		return helper.ErrInternalf("error voting on transaction: %w", err)
-	}
-
-	if err := stream.SendMsg(&gitalypb.PreReceiveHookResponse{
-		ExitStatus: &gitalypb.ExitStatus{Value: status},
-	}); err != nil {
-		return helper.ErrInternal(err)
-	}
-
-	return nil
-}
-
 func getEnvVar(key string, vars []string) string {
 	for _, varPair := range vars {
 		kv := strings.SplitN(varPair, "=", 2)
diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go
index 8f1c1c416..e29e7bc94 100644
--- a/internal/gitaly/service/hook/pre_receive_test.go
+++ b/internal/gitaly/service/hook/pre_receive_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/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"
@@ -84,148 +83,6 @@ func receivePreReceive(t *testing.T, stream gitalypb.HookService_PreReceiveHookC
 	return stdout, stderr, status
 }
 
-func TestPreReceive(t *testing.T) {
-	rubyDir := config.Config.Ruby.Dir
-	defer func(rubyDir string) {
-		config.Config.Ruby.Dir = rubyDir
-	}(rubyDir)
-
-	cwd, err := os.Getwd()
-	require.NoError(t, err)
-	config.Config.Ruby.Dir = filepath.Join(cwd, "testdata")
-
-	serverSocketPath, stop := runHooksServer(t, config.Config.Hooks)
-	defer stop()
-
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
-
-	client, conn := newHooksClient(t, serverSocketPath)
-	defer conn.Close()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	testCases := []struct {
-		desc           string
-		stdin          io.Reader
-		req            gitalypb.PreReceiveHookRequest
-		status         int32
-		stdout, stderr string
-	}{
-		{
-			desc:  "everything OK",
-			stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=key-123",
-					"GL_PROTOCOL=protocol",
-					"GL_USERNAME=username",
-					"GL_REPOSITORY=repository",
-				},
-			},
-			status: 0,
-			stdout: "OK",
-			stderr: "",
-		},
-		{
-			desc:  "missing stdin",
-			stdin: bytes.NewBuffer(nil),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=key-123",
-					"GL_PROTOCOL=protocol",
-					"GL_USERNAME=username",
-					"GL_REPOSITORY=repository",
-				},
-			},
-			status: 1,
-			stdout: "",
-			stderr: "FAIL",
-		},
-		{
-			desc:  "missing gl_protocol",
-			stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=key-123",
-					"GL_USERNAME=username",
-					"GL_PROTOCOL=",
-					"GL_REPOSITORY=repository",
-				},
-			},
-			status: 1,
-			stdout: "",
-			stderr: "FAIL",
-		},
-		{
-			desc:  "missing gl_id",
-			stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=",
-					"GL_PROTOCOL=protocol",
-					"GL_USERNAME=username",
-					"GL_REPOSITORY=repository",
-				},
-			},
-			status: 1,
-			stdout: "",
-			stderr: "FAIL",
-		},
-		{
-			desc:  "missing gl_username",
-			stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=key-123",
-					"GL_PROTOCOL=protocol",
-					"GL_USERNAME=",
-					"GL_REPOSITORY=repository",
-				},
-			},
-			status: 1,
-			stdout: "",
-			stderr: "FAIL",
-		},
-		{
-			desc:  "missing gl_repository",
-			stdin: bytes.NewBufferString("a\nb\nc\nd\ne\nf\ng"),
-			req: gitalypb.PreReceiveHookRequest{
-				Repository: testRepo,
-				EnvironmentVariables: []string{
-					"GL_ID=key-123",
-					"GL_PROTOCOL=protocol",
-					"GL_USERNAME=username",
-					"GL_REPOSITORY=",
-				},
-			},
-			status: 1,
-			stdout: "",
-			stderr: "FAIL",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			stream, err := client.PreReceiveHook(ctx)
-			require.NoError(t, err)
-			require.NoError(t, stream.Send(&tc.req))
-
-			stdout, stderr, status := receivePreReceive(t, stream, tc.stdin)
-
-			assert.Equal(t, tc.status, status)
-			assert.Equal(t, tc.stderr, text.ChompBytes(stderr), "hook stderr")
-			assert.Equal(t, tc.stdout, text.ChompBytes(stdout), "hook stdout")
-		})
-	}
-}
-
 func TestPreReceiveHook_GitlabAPIAccess(t *testing.T) {
 	user, password := "user", "password"
 	secretToken := "secret123"
@@ -299,7 +156,6 @@ func TestPreReceiveHook_GitlabAPIAccess(t *testing.T) {
 			"GL_PROTOCOL=" + protocol,
 			"GL_USERNAME=username",
 			"GL_REPOSITORY=" + glRepository,
-			fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar),
 		},
 	}
 
@@ -404,7 +260,6 @@ func TestPreReceive_APIErrors(t *testing.T) {
 					"GL_PROTOCOL=web",
 					"GL_USERNAME=username",
 					"GL_REPOSITORY=repository",
-					fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar),
 				},
 			}))
 			require.NoError(t, stream.CloseSend())
@@ -465,7 +320,6 @@ exit %d
 			"GL_PROTOCOL=web",
 			"GL_USERNAME=username",
 			"GL_REPOSITORY=repository",
-			fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar),
 		},
 	}))
 
@@ -501,7 +355,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 	testCases := []struct {
 		desc               string
 		primary            bool
-		useRubyHook        bool
 		allowedHandler     http.HandlerFunc
 		preReceiveHandler  http.HandlerFunc
 		stdin              []byte
@@ -580,7 +433,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 		{
 			desc:               "primary Ruby hook triggers transaction",
 			primary:            true,
-			useRubyHook:        true,
 			stdin:              []byte("foobar"),
 			allowedHandler:     allowedHandler(true),
 			preReceiveHandler:  preReceiveHandler(true),
@@ -591,7 +443,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 		{
 			desc:               "secondary Ruby hook triggers transaction",
 			primary:            false,
-			useRubyHook:        true,
 			stdin:              []byte("foobar"),
 			allowedHandler:     allowedHandler(true),
 			preReceiveHandler:  preReceiveHandler(true),
@@ -680,9 +531,6 @@ func TestPreReceiveHook_Primary(t *testing.T) {
 				transactionEnv,
 				transactionServerEnv,
 			}
-			if !tc.useRubyHook {
-				environment = append(environment, fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar))
-			}
 
 			stream, err := client.PreReceiveHook(ctx)
 			require.NoError(t, err)
diff --git a/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/pre-receive b/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/pre-receive
deleted file mode 100755
index 12574b7b7..000000000
--- a/internal/gitaly/service/hook/testdata/gitlab-shell/hooks/pre-receive
+++ /dev/null
@@ -1,9 +0,0 @@
-#!/usr/bin/env ruby
-
-# Tests inputs to pre-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? }
-
-puts "OK"
-
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index d87009a0e..103df99b2 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}
-	// GoPreReceiveHook will bypass the ruby pre-receive hook and use the go implementation
-	GoPreReceiveHook = FeatureFlag{Name: "go_prereceive_hook", 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
@@ -39,7 +37,6 @@ var (
 var All = []FeatureFlag{
 	GoFetchSourceBranch,
 	DistributedReads,
-	GoPreReceiveHook,
 	GoPostReceiveHook,
 	ReferenceTransactions,
 	ReferenceTransactionsOperationService,
@@ -50,7 +47,6 @@ var All = []FeatureFlag{
 }
 
 const (
-	GoPreReceiveHookEnvVar         = "GITALY_GO_PRERECEIVE"
 	GoPostReceiveHookEnvVar        = "GITALY_GO_POSTRECEIVE"
 	ReferenceTransactionHookEnvVar = "GITALY_REFERENCE_TRANSACTION_HOOK"
 )
diff --git a/internal/testhelper/testserver.go b/internal/testhelper/testserver.go
index df7b2370f..fbf50a664 100644
--- a/internal/testhelper/testserver.go
+++ b/internal/testhelper/testserver.go
@@ -513,7 +513,7 @@ func handleAllowed(options GitlabTestServerOptions) func(w http.ResponseWriter,
 			return
 		}
 		w.WriteHeader(http.StatusUnauthorized)
-		w.Write([]byte(`{"message":"401 Unauthorized"}`))
+		w.Write([]byte(`{"message":"401 Unauthorized\n"}`))
 	}
 }
 
diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index e16895722..c391bbf78 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -120,7 +120,6 @@ module Gitlab
           'GIT_DIR' => repo_path,
           'GITALY_REPO' => repository.gitaly_repository.to_json,
           'GITALY_SOCKET' => Gitlab.config.gitaly.internal_socket,
-          'GITALY_GO_PRERECEIVE' => repository.feature_enabled?('go-prereceive-hook', on_by_default: true).to_s,
           'GITALY_GO_POSTRECEIVE' => repository.feature_enabled?('go-postreceive-hook').to_s
         }
       end