Merge branch 'jc-remove-ff-for-hook-rpc' into 'master'

Remove Feature Flag for Hook RPCs

Closes #2876 and #2830

See merge request gitlab-org/gitaly!2283

17 files changed, 42 insertions, 130 deletions

diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 633c31827..1e144c67a 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -51,11 +51,6 @@ func main() {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	if os.Getenv(featureflag.HooksRPCEnvVar) != "true" {
-		executeScript(ctx, subCmd, logger)
-		return
-	}
-
 	repository, err := repositoryFromEnv()
 	if err != nil {
 		logger.Fatalf("error when getting repository: %v", err)
@@ -300,46 +295,3 @@ func check(configPath string) (int, error) {
 
 	return 0, nil
 }
-
-func executeScript(ctx context.Context, subCmd string, logger *gitalylog.HookLogger) {
-	gitalyRubyDir := os.Getenv("GITALY_RUBY_DIR")
-	if gitalyRubyDir == "" {
-		logger.Fatalf("GITALY_RUBY_DIR not set")
-	}
-
-	rubyHookPath := filepath.Join(gitalyRubyDir, "gitlab-shell", "hooks", subCmd)
-
-	var hookCmd *exec.Cmd
-
-	switch subCmd {
-	case "update":
-		args := os.Args[2:]
-		requireArgs := 3
-		if len(args) != requireArgs {
-			logger.Fatalf("update hook requires %d arguments, got: %d", requireArgs, len(args))
-		}
-
-		hookCmd = exec.Command(rubyHookPath, args...)
-	case "pre-receive", "post-receive":
-		hookCmd = exec.Command(rubyHookPath)
-	default:
-		logger.Fatalf("subcommand name invalid: %v", subCmd)
-	}
-
-	cmd, err := command.New(ctx, hookCmd, os.Stdin, os.Stdout, os.Stderr, os.Environ()...)
-	if err != nil {
-		logger.Fatalf("error when starting command %q: %v", hookCmd.String(), err)
-	}
-
-	if err := cmd.Wait(); err != nil {
-		logger.Errorf("error when executing ruby hook: %v", err)
-		var exitError *exec.ExitError
-		if errors.As(err, &exitError) {
-			os.Exit(exitError.ExitCode())
-		} else {
-			os.Exit(1)
-		}
-	}
-
-	os.Exit(0)
-}
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index 54f3690b1..f4f1ed5a6 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -116,7 +116,7 @@ func TestHooksPrePostReceive(t *testing.T) {
 
 	hookNames := []string{"pre-receive", "post-receive"}
 
-	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.HooksRPC, featureflag.GoPreReceiveHook})
+	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.GoPreReceiveHook})
 	require.NoError(t, err)
 
 	for _, hookName := range hookNames {
@@ -161,10 +161,6 @@ func TestHooksPrePostReceive(t *testing.T) {
 					gitPushOptions...,
 				)
 
-				if featureSet.IsEnabled(featureflag.HooksRPC) {
-					cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.HooksRPCEnvVar))
-				}
-
 				if featureSet.IsEnabled(featureflag.GoPreReceiveHook) {
 					cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.GoPreReceiveHookEnvVar))
 				}
@@ -224,7 +220,7 @@ func TestHooksUpdate(t *testing.T) {
 	socket, stop := runHookServiceServer(t, token)
 	defer stop()
 
-	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.HooksRPC, featureflag.GoUpdateHook})
+	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.GoUpdateHook})
 	require.NoError(t, err)
 
 	for _, featureSet := range featureSets {
@@ -277,9 +273,6 @@ open('%s', 'w') { |f| f.puts(JSON.dump(ARGV)) }
 
 	var stdout, stderr bytes.Buffer
 
-	if features.IsEnabled(featureflag.HooksRPC) {
-		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.HooksRPCEnvVar))
-	}
 	if features.IsEnabled(featureflag.GoUpdateHook) {
 		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.GoUpdateHookEnvVar))
 	}
@@ -347,9 +340,6 @@ func TestHooksPostReceiveFailed(t *testing.T) {
 	config.Config.Gitlab.URL = ts.URL
 	config.Config.Gitlab.SecretFile = filepath.Join(tempGitlabShellDir, ".gitlab_shell_secret")
 
-	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.HooksRPC})
-	require.NoError(t, err)
-
 	token := "abc123"
 
 	gitlabAPI, err := hook.NewGitlabAPI(config.Config.Gitlab)
@@ -358,43 +348,35 @@ func TestHooksPostReceiveFailed(t *testing.T) {
 	socket, stop := runHookServiceServerWithAPI(t, token, gitlabAPI)
 	defer stop()
 
-	for _, featureSet := range featureSets {
-		t.Run(fmt.Sprintf("features: %v", featureSet), func(t *testing.T) {
-			customHookOutputPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, "post-receive")
-			defer cleanup()
-
-			var stdout, stderr bytes.Buffer
+	customHookOutputPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, "post-receive")
+	defer cleanup()
 
-			postReceiveHookPath, err := filepath.Abs("../../ruby/git-hooks/post-receive")
-			require.NoError(t, err)
-			cmd := exec.Command(postReceiveHookPath)
-			cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo, testhelper.GlHookValues{
-				GLID:       glID,
-				GLUsername: glUsername,
-				GLRepo:     glRepository,
-				GLProtocol: glProtocol,
-			})
-			cmd.Stdout = &stdout
-			cmd.Stderr = &stderr
-			cmd.Stdin = bytes.NewBuffer([]byte(changes))
-			cmd.Dir = testRepoPath
+	var stdout, stderr bytes.Buffer
 
-			if featureSet.IsEnabled(featureflag.HooksRPC) {
-				cmd.Env = append(cmd.Env, fmt.Sprintf("%s=true", featureflag.HooksRPCEnvVar))
-			}
+	postReceiveHookPath, err := filepath.Abs("../../ruby/git-hooks/post-receive")
+	require.NoError(t, err)
+	cmd := exec.Command(postReceiveHookPath)
+	cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo, testhelper.GlHookValues{
+		GLID:       glID,
+		GLUsername: glUsername,
+		GLRepo:     glRepository,
+		GLProtocol: glProtocol,
+	})
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	cmd.Stdin = bytes.NewBuffer([]byte(changes))
+	cmd.Dir = testRepoPath
 
-			err = cmd.Run()
-			code, ok := command.ExitStatus(err)
+	err = cmd.Run()
+	code, ok := command.ExitStatus(err)
 
-			require.True(t, ok, "expect exit status in %v", err)
-			require.Equal(t, 1, code, "exit status")
-			require.Empty(t, stdout.String())
-			require.Empty(t, stderr.String())
+	require.True(t, ok, "expect exit status in %v", err)
+	require.Equal(t, 1, code, "exit status")
+	require.Empty(t, stdout.String())
+	require.Empty(t, stderr.String())
 
-			output := string(testhelper.MustReadFile(t, customHookOutputPath))
-			require.Empty(t, output, "custom hook should not have run")
-		})
-	}
+	output := string(testhelper.MustReadFile(t, customHookOutputPath))
+	require.Empty(t, output, "custom hook should not have run")
 }
 
 func TestHooksNotAllowed(t *testing.T) {
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index b8c649827..6e27a5b58 100644
--- a/internal/git/receivepack.go
+++ b/internal/git/receivepack.go
@@ -46,7 +46,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.HooksRPCEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.HooksRPC))),
 		fmt.Sprintf("%s=%s", featureflag.GoUpdateHookEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.GoUpdateHook))),
 		fmt.Sprintf("%s=%s", featureflag.GoPreReceiveHookEnvVar, strconv.FormatBool(featureflag.IsEnabled(ctx, featureflag.GoPreReceiveHook))),
 	}, gitlabshellEnv...)
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index b302fe076..40440b0f9 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -6,12 +6,6 @@ const (
 	UploadPackFilter = "upload_pack_filter"
 	// LinguistFileCountStats will invoke an additional git-linguist command to get the number of files per language
 	LinguistFileCountStats = "linguist_file_count_stats"
-	// HooksRPC will invoke update, pre receive, and post receive hooks by using RPCs
-	HooksRPC = "hooks_rpc"
-	// GitalyRubyCallHookRPC will invoke update, pre receive, and post receive hooks from the operations service by using RPCs
-	// note: there doesn't need to be a separate gitaly ruby feature flag name. The same featureflag string can be used for both the go code
-	// and being passed into gitaly-ruby
-	GitalyRubyCallHookRPC = "call_hook_rpc"
 	// GoUpdateHook will bypass the ruby update hook and use the go implementation of custom hooks
 	GoUpdateHook = "go_update_hook"
 	// RemoteBranchesLsRemote will use `ls-remote` for remote branches
@@ -25,8 +19,6 @@ const (
 )
 
 const (
-	// HooksRPCEnvVar is the name of the environment variable we use to pass the feature flag down into gitaly-hooks
-	HooksRPCEnvVar         = "GITALY_HOOK_RPCS_ENABLED"
 	GoUpdateHookEnvVar     = "GITALY_GO_UPDATE"
 	GoPreReceiveHookEnvVar = "GITALY_GO_PRERECEIVE"
 )
diff --git a/internal/service/operations/apply_patch_test.go b/internal/service/operations/apply_patch_test.go
index 32e36af1c..309bc2b0d 100644
--- a/internal/service/operations/apply_patch_test.go
+++ b/internal/service/operations/apply_patch_test.go
@@ -20,7 +20,7 @@ import (
 )
 
 func TestSuccessfulUserApplyPatch(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go
index 8c06d3791..0d8b94ccf 100644
--- a/internal/service/operations/branches_test.go
+++ b/internal/service/operations/branches_test.go
@@ -76,7 +76,7 @@ func TestSuccessfulCreateBranchRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	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 0b1431c7a..3a30688b7 100644
--- a/internal/service/operations/cherry_pick_test.go
+++ b/internal/service/operations/cherry_pick_test.go
@@ -117,7 +117,7 @@ func TestSuccessfulUserCherryPickRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCherryPickRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go
index cc292041a..2aceef1ba 100644
--- a/internal/service/operations/commit_files_test.go
+++ b/internal/service/operations/commit_files_test.go
@@ -127,7 +127,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctxWithFeatureFlags cont
 }
 
 func TestSuccessfulUserCommitFilesRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go
index 869a7a441..bb5ef67aa 100644
--- a/internal/service/operations/merge_test.go
+++ b/internal/service/operations/merge_test.go
@@ -25,7 +25,7 @@ var (
 )
 
 func TestSuccessfulMerge(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/rebase_test.go b/internal/service/operations/rebase_test.go
index a967f0f22..7df1cf618 100644
--- a/internal/service/operations/rebase_test.go
+++ b/internal/service/operations/rebase_test.go
@@ -24,7 +24,7 @@ var (
 )
 
 func TestSuccessfulUserRebaseConfirmableRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/revert_test.go b/internal/service/operations/revert_test.go
index bb8845478..a910bc25d 100644
--- a/internal/service/operations/revert_test.go
+++ b/internal/service/operations/revert_test.go
@@ -117,7 +117,7 @@ func TestSuccessfulUserRevertRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserRevertRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/squash_test.go b/internal/service/operations/squash_test.go
index 6787bfe48..6a363ec9a 100644
--- a/internal/service/operations/squash_test.go
+++ b/internal/service/operations/squash_test.go
@@ -29,7 +29,7 @@ var (
 )
 
 func TestSuccessfulUserSquashRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/submodules_test.go b/internal/service/operations/submodules_test.go
index 4553995f2..b59deb76d 100644
--- a/internal/service/operations/submodules_test.go
+++ b/internal/service/operations/submodules_test.go
@@ -16,7 +16,7 @@ import (
 )
 
 func TestSuccessfulUserUpdateSubmoduleRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go
index 6e0c64805..201eda9b3 100644
--- a/internal/service/operations/tags_test.go
+++ b/internal/service/operations/tags_test.go
@@ -50,7 +50,7 @@ func TestSuccessfulUserDeleteTagRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
@@ -195,7 +195,7 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 
 	ctx, cancel := testhelper.Context()
@@ -298,7 +298,7 @@ func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestFailedUserDeleteTagDueToHooks(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 
 	ctx, cancel := testhelper.Context()
diff --git a/internal/service/operations/update_branches_test.go b/internal/service/operations/update_branches_test.go
index 6602b11d9..978195c27 100644
--- a/internal/service/operations/update_branches_test.go
+++ b/internal/service/operations/update_branches_test.go
@@ -53,7 +53,7 @@ func TestSuccessfulUserUpdateBranchRequest(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
@@ -100,7 +100,7 @@ func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, ctx context.
 }
 
 func TestFailedUserUpdateBranchDueToHooks(t *testing.T) {
-	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GitalyRubyCallHookRPC, featureflag.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoUpdateHook)
 	require.NoError(t, err)
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index 0c40b4681..463460060 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -340,15 +340,7 @@ func drainPostReceivePackResponse(stream gitalypb.SmartHTTPService_PostReceivePa
 	return err
 }
 
-func TestPostReceivePackToHooks_WithRPC(t *testing.T) {
-	testPostReceivePackToHooks(t, true)
-}
-
-func TestPostReceivePackToHooks_WithoutRPC(t *testing.T) {
-	testPostReceivePackToHooks(t, false)
-}
-
-func testPostReceivePackToHooks(t *testing.T, callRPC bool) {
+func TestPostReceivePackToHooks(t *testing.T) {
 	defer func(cfg config.Cfg) {
 		config.Config = cfg
 	}(config.Config)
@@ -415,10 +407,6 @@ func testPostReceivePackToHooks(t *testing.T, callRPC bool) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	if callRPC {
-		ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.HooksRPC)
-	}
-
 	stream, err := client.PostReceivePack(ctx)
 	require.NoError(t, err)
 
@@ -474,7 +462,7 @@ func TestPostReceiveWithTransactions(t *testing.T) {
 	gitlabUser := "gitlab_user-1234"
 	gitlabPassword := "gitlabsecret9887"
 
-	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.HooksRPC, featureflag.ReferenceTransactions, featureflag.GoPreReceiveHook})
+	featureSets, err := testhelper.NewFeatureSets([]string{featureflag.ReferenceTransactions, featureflag.GoPreReceiveHook})
 	require.NoError(t, err)
 
 	for _, features := range featureSets {
diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index 4cea56305..f60a638e1 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -118,7 +118,6 @@ module Gitlab
           'GIT_DIR' => repo_path,
           'GITALY_REPO' => repository.gitaly_repository.to_json,
           'GITALY_SOCKET' => Gitlab.config.gitaly.internal_socket,
-          'GITALY_HOOK_RPCS_ENABLED' => repository.feature_enabled?('call-hook-rpc').to_s,
           'GITALY_GO_UPDATE' => repository.feature_enabled?('go-update-hook').to_s,
           'GITALY_GO_PRERECEIVE' => repository.feature_enabled?('go-prereceive-hook').to_s
         }