Pass env vars correctly into custom hooks

6 files changed, 112 insertions, 25 deletions

diff --git a/changelogs/unreleased/jc-fix-go-prereceive-update.yml b/changelogs/unreleased/jc-fix-go-prereceive-update.yml
new file mode 100644
index 000000000..7f3feea0d
--- /dev/null
+++ b/changelogs/unreleased/jc-fix-go-prereceive-update.yml
@@ -0,0 +1,5 @@
+---
+title: Pass env vars correctly into custom hooks
+merge_request: 2331
+author:
+type: fixed
diff --git a/internal/service/hook/pre_receive.go b/internal/service/hook/pre_receive.go
index 9adfa0d8c..c5053661c 100644
--- a/internal/service/hook/pre_receive.go
+++ b/internal/service/hook/pre_receive.go
@@ -137,7 +137,7 @@ func (s *server) voteOnTransaction(stream gitalypb.HookService_PreReceiveHookSer
 	return nil
 }
 
-func (s *server) executeCustomHooks(stream gitalypb.HookService_PreReceiveHookServer, changes []byte, repository *gitalypb.Repository, reqEnvVars []string) error {
+func (s *server) executeCustomHooks(firstReq prePostRequest, stream gitalypb.HookService_PreReceiveHookServer, changes []byte, repository *gitalypb.Repository, reqEnvVars []string) error {
 	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}) })
 
@@ -151,12 +151,11 @@ func (s *server) executeCustomHooks(stream gitalypb.HookService_PreReceiveHookSe
 		return fmt.Errorf("creating custom hooks executor: %w", err)
 	}
 
-	_, gitObjectDirEnv, err := alternates.PathAndEnv(repository)
+	env, err := preReceiveEnv(firstReq)
 	if err != nil {
-		return fmt.Errorf("getting git object dir from request %w", err)
+		return fmt.Errorf("getting env vars from request: %v", err)
 	}
-
-	env := append(reqEnvVars, gitObjectDirEnv...)
+	env = append(env, reqEnvVars...)
 
 	if err = executor(
 		stream.Context(),
@@ -230,7 +229,7 @@ func (s *server) PreReceiveHook(stream gitalypb.HookService_PreReceiveHookServer
 			return preReceiveHookResponse(stream, int32(1), message)
 		}
 
-		if err := s.executeCustomHooks(stream, changes, repository, reqEnvVars); err != nil {
+		if err := s.executeCustomHooks(firstRequest, stream, changes, repository, reqEnvVars); err != nil {
 			var exitError *exec.ExitError
 			if errors.As(err, &exitError) {
 				return preReceiveHookResponse(stream, int32(exitError.ExitCode()), "")
diff --git a/internal/service/hook/update.go b/internal/service/hook/update.go
index 5741da00c..50f3f30a7 100644
--- a/internal/service/hook/update.go
+++ b/internal/service/hook/update.go
@@ -58,16 +58,15 @@ func updateHookRuby(in *gitalypb.UpdateHookRequest, stream gitalypb.HookService_
 	)
 
 	if err != nil {
-		return helper.ErrInternal(err)
-	}
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			return updateHookResponse(stream, int32(exitError.ExitCode()))
+		}
 
-	if err := stream.SendMsg(&gitalypb.UpdateHookResponse{
-		ExitStatus: &gitalypb.ExitStatus{Value: status},
-	}); err != nil {
 		return helper.ErrInternal(err)
 	}
 
-	return nil
+	return updateHookResponse(stream, status)
 }
 
 func (s *server) UpdateHook(in *gitalypb.UpdateHookRequest, stream gitalypb.HookService_UpdateHookServer) error {
@@ -99,8 +98,23 @@ func (s *server) UpdateHook(in *gitalypb.UpdateHookRequest, stream gitalypb.Hook
 		stdout,
 		stderr,
 	); err != nil {
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			return updateHookResponse(stream, int32(exitError.ExitCode()))
+		}
+
 		return helper.ErrInternal(err)
 	}
 
+	return updateHookResponse(stream, 0)
+}
+
+func updateHookResponse(stream gitalypb.HookService_UpdateHookServer, code int32) error {
+	if err := stream.Send(&gitalypb.UpdateHookResponse{
+		ExitStatus: &gitalypb.ExitStatus{Value: code},
+	}); err != nil {
+		return helper.ErrInternalf("sending response: %v", err)
+	}
+
 	return nil
 }
diff --git a/internal/service/hook/update_test.go b/internal/service/hook/update_test.go
index 4acf58ee3..5ecefe8ac 100644
--- a/internal/service/hook/update_test.go
+++ b/internal/service/hook/update_test.go
@@ -2,6 +2,7 @@ package hook
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -11,6 +12,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
+	"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"
@@ -201,3 +203,64 @@ func TestUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdate_CustomHooks(t *testing.T) {
+	rubyDir := config.Config.Ruby.Dir
+	defer func() {
+		config.Config.Ruby.Dir = 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, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	client, conn := newHooksClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	req := gitalypb.UpdateHookRequest{
+		Repository:           testRepo,
+		Ref:                  []byte("master"),
+		OldValue:             "a",
+		NewValue:             "b",
+		EnvironmentVariables: []string{"GL_ID=key-123", "GL_USERNAME=username", "GL_PROTOCOL=protocol", "GL_REPOSITORY=repository"},
+	}
+
+	errorMsg := "error123"
+	cleanup, err := testhelper.WriteCustomHook(testRepoPath, "update", []byte(fmt.Sprintf(`#!/bin/bash
+echo %s 1>&2
+exit 1
+`, errorMsg)))
+	require.NoError(t, err)
+	defer cleanup()
+
+	req.EnvironmentVariables = append(req.EnvironmentVariables, featureflag.GoUpdateHookEnvVar+"=true")
+	stream, err := client.UpdateHook(ctx, &req)
+	require.NoError(t, err)
+
+	var status int32
+	var stderr, stdout bytes.Buffer
+	for {
+		resp, err := stream.Recv()
+		if err == io.EOF {
+			break
+		}
+		require.NoError(t, err, "when receiving stream")
+
+		stderr.Write(resp.GetStderr())
+		stdout.Write(resp.GetStdout())
+
+		status = resp.GetExitStatus().GetValue()
+	}
+
+	assert.Equal(t, int32(1), status)
+	assert.Equal(t, errorMsg, text.ChompBytes(stderr.Bytes()), "hook stderr")
+	assert.Equal(t, "", text.ChompBytes(stdout.Bytes()), "hook stdout")
+}
diff --git a/internal/service/operations/rebase_test.go b/internal/service/operations/rebase_test.go
index 7df1cf618..a175c4324 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.GoUpdateHook)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoPreReceiveHook, 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 a910bc25d..4f251ffe7 100644
--- a/internal/service/operations/revert_test.go
+++ b/internal/service/operations/revert_test.go
@@ -2,6 +2,7 @@ package operations
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -283,19 +284,24 @@ func TestFailedUserRevertRequestDueToPreReceiveError(t *testing.T) {
 
 	hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1")
 
-	for _, hookName := range GitlabPreHooks {
-		t.Run(hookName, func(t *testing.T) {
-			remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, hookContent)
-			require.NoError(t, err)
-			defer remove()
-
-			md := testhelper.GitalyServersMetadata(t, serverSocketPath)
-			ctx := metadata.NewOutgoingContext(ctxOuter, md)
+	featureSet, err := testhelper.NewFeatureSets(nil, featureflag.GoPreReceiveHook, featureflag.GoUpdateHook)
+	require.NoError(t, err)
 
-			response, err := client.UserRevert(ctx, request)
-			require.NoError(t, err)
-			require.Contains(t, response.PreReceiveError, "GL_ID="+testhelper.TestUser.GlId)
-		})
+	for _, hookName := range GitlabPreHooks {
+		for _, features := range featureSet {
+			t.Run(fmt.Sprintf("%s, features: %v", hookName, features.String()), func(t *testing.T) {
+				remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, hookContent)
+				require.NoError(t, err)
+				defer remove()
+
+				md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+				ctx := metadata.NewOutgoingContext(ctxOuter, md)
+
+				response, err := client.UserRevert(features.WithParent(ctx), request)
+				require.NoError(t, err)
+				require.Contains(t, response.PreReceiveError, "GL_ID="+testhelper.TestUser.GlId)
+			})
+		}
 	}
 }