hooks: Start using hooks payload for transactions

Now that we have transaction-related information as part of the
HooksPayload, this commit converts code to use it instead of the
previous environment variables. This also requires us to inject the
variable into the hook requests, which wasn't yet the case.

While no code site yet injects transaction information via the payload,
this in fact serves as a nice demonstration that the fallback in
`HooksPayloadFromEnv()` works as expected.

6 files changed, 53 insertions, 61 deletions

diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 5de0a0f8b..99598b2b4 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/hook"
 	gitalylog "gitlab.com/gitlab-org/gitaly/internal/log"
-	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/internal/stream"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -73,6 +72,11 @@ func main() {
 		logger.Fatalf("error when getting hooks payload: %v", err)
 	}
 
+	hookEnvironment, err := hookEnvironment(payload)
+	if err != nil {
+		logger.Fatalf("error when computing hook environment: %v", err)
+	}
+
 	conn, err := dialGitaly(payload)
 	if err != nil {
 		logger.Fatalf("error when connecting to gitaly: %v", err)
@@ -92,7 +96,7 @@ func main() {
 
 		req := &gitalypb.UpdateHookRequest{
 			Repository:           payload.Repo,
-			EnvironmentVariables: hookEnvironment(),
+			EnvironmentVariables: hookEnvironment,
 			Ref:                  []byte(ref),
 			OldValue:             oldValue,
 			NewValue:             newValue,
@@ -116,7 +120,7 @@ func main() {
 
 		if err := preReceiveHookStream.Send(&gitalypb.PreReceiveHookRequest{
 			Repository:           payload.Repo,
-			EnvironmentVariables: append(hookEnvironment(), gitObjectDirs()...),
+			EnvironmentVariables: append(hookEnvironment, gitObjectDirs()...),
 			GitPushOptions:       gitPushOptions(),
 		}); err != nil {
 			logger.Fatalf("error when sending request for %q: %v", subCmd, err)
@@ -139,7 +143,7 @@ func main() {
 
 		if err := postReceiveHookStream.Send(&gitalypb.PostReceiveHookRequest{
 			Repository:           payload.Repo,
-			EnvironmentVariables: hookEnvironment(),
+			EnvironmentVariables: hookEnvironment,
 			GitPushOptions:       gitPushOptions(),
 		}); err != nil {
 			logger.Fatalf("error when sending request for %q: %v", subCmd, err)
@@ -178,7 +182,7 @@ func main() {
 
 		if err := referenceTransactionHookStream.Send(&gitalypb.ReferenceTransactionHookRequest{
 			Repository:           payload.Repo,
-			EnvironmentVariables: hookEnvironment(),
+			EnvironmentVariables: hookEnvironment,
 			State:                state,
 		}); err != nil {
 			logger.Fatalf("error when sending request for %q: %v", subCmd, err)
@@ -216,7 +220,7 @@ func dialGitaly(payload git.HooksPayload) (*grpc.ClientConn, error) {
 	return conn, nil
 }
 
-func hookEnvironment() []string {
+func hookEnvironment(payload git.HooksPayload) ([]string, error) {
 	environment := command.AllowedEnvironment()
 
 	for _, kv := range os.Environ() {
@@ -225,14 +229,12 @@ func hookEnvironment() []string {
 		}
 	}
 
-	for _, key := range []string{metadata.PraefectEnvKey, metadata.TransactionEnvKey} {
-		if value, ok := os.LookupEnv(key); ok {
-			env := fmt.Sprintf("%s=%s", key, value)
-			environment = append(environment, env)
-		}
+	payloadEnv, err := payload.Env()
+	if err != nil {
+		return nil, err
 	}
 
-	return environment
+	return append(environment, payloadEnv), nil
 }
 
 func gitObjectDirs() []string {
diff --git a/internal/gitaly/hook/postreceive.go b/internal/gitaly/hook/postreceive.go
index b1d955f2c..d6b1a1da9 100644
--- a/internal/gitaly/hook/postreceive.go
+++ b/internal/gitaly/hook/postreceive.go
@@ -10,6 +10,7 @@ import (
 	"math"
 	"strings"
 
+	"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/proto/go/gitalypb"
@@ -116,17 +117,17 @@ func printAlert(m PostReceiveMessage, w io.Writer) error {
 }
 
 func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
-	changes, err := ioutil.ReadAll(stdin)
+	payload, err := git.HooksPayloadFromEnv(env)
 	if err != nil {
-		return helper.ErrInternalf("reading stdin from request: %w", err)
+		return helper.ErrInternalf("extracting hooks payload: %w", err)
 	}
 
-	primary, err := isPrimary(env)
+	changes, err := ioutil.ReadAll(stdin)
 	if err != nil {
-		return helper.ErrInternalf("could not check role: %w", err)
+		return helper.ErrInternalf("reading stdin from request: %w", err)
 	}
 
-	if !primary {
+	if !isPrimary(payload) {
 		return nil
 	}
 
diff --git a/internal/gitaly/hook/prereceive.go b/internal/gitaly/hook/prereceive.go
index 5750e345e..a67fb59f8 100644
--- a/internal/gitaly/hook/prereceive.go
+++ b/internal/gitaly/hook/prereceive.go
@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
@@ -40,22 +41,22 @@ func getRelativeObjectDirs(repoPath, gitObjectDir, gitAlternateObjectDirs string
 }
 
 func (m *GitLabHookManager) PreReceiveHook(ctx context.Context, repo *gitalypb.Repository, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
-	changes, err := ioutil.ReadAll(stdin)
+	payload, err := git.HooksPayloadFromEnv(env)
 	if err != nil {
-		return helper.ErrInternalf("reading stdin from request: %w", err)
+		return helper.ErrInternalf("extracting hooks payload: %w", err)
 	}
 
-	primary, err := isPrimary(env)
+	changes, err := ioutil.ReadAll(stdin)
 	if err != nil {
-		return helper.ErrInternalf("could not check role: %w", err)
+		return helper.ErrInternalf("reading stdin from request: %w", err)
 	}
 
 	// Only the primary should execute hooks and increment reference counters.
-	if primary {
+	if isPrimary(payload) {
 		if err := m.preReceiveHook(ctx, repo, env, changes, stdout, stderr); err != nil {
 			// If the pre-receive hook declines the push, then we need to stop any
 			// secondaries voting on the transaction.
-			m.stopTransaction(ctx, env)
+			m.stopTransaction(ctx, payload)
 			return err
 		}
 	}
diff --git a/internal/gitaly/hook/referencetransaction.go b/internal/gitaly/hook/referencetransaction.go
index 20c7a7143..c2514a029 100644
--- a/internal/gitaly/hook/referencetransaction.go
+++ b/internal/gitaly/hook/referencetransaction.go
@@ -6,10 +6,16 @@ import (
 	"io"
 	"io/ioutil"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state ReferenceTransactionState, env []string, stdin io.Reader) error {
+	payload, err := git.HooksPayloadFromEnv(env)
+	if err != nil {
+		return helper.ErrInternalf("extracting hooks payload: %w", err)
+	}
+
 	changes, err := ioutil.ReadAll(stdin)
 	if err != nil {
 		return helper.ErrInternalf("reading stdin from request: %w", err)
@@ -24,7 +30,7 @@ func (m *GitLabHookManager) ReferenceTransactionHook(ctx context.Context, state
 
 	hash := sha1.Sum(changes)
 
-	if err := m.voteOnTransaction(ctx, hash[:], env); err != nil {
+	if err := m.voteOnTransaction(ctx, hash[:], payload); err != nil {
 		return helper.ErrInternalf("error voting on transaction: %v", err)
 	}
 
diff --git a/internal/gitaly/hook/transactions.go b/internal/gitaly/hook/transactions.go
index 6d52ebf9d..e12111b0d 100644
--- a/internal/gitaly/hook/transactions.go
+++ b/internal/gitaly/hook/transactions.go
@@ -3,27 +3,20 @@ package hook
 import (
 	"context"
 	"errors"
-	"fmt"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc"
 )
 
-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
+func isPrimary(payload git.HooksPayload) bool {
+	if payload.Transaction == nil {
+		return true
 	}
-
-	return tx.Primary, nil
+	return payload.Transaction.Primary
 }
 
 func (m *GitLabHookManager) getPraefectConn(ctx context.Context, server *metadata.PraefectServer) (*grpc.ClientConn, error) {
@@ -37,45 +30,33 @@ func (m *GitLabHookManager) getPraefectConn(ctx context.Context, server *metadat
 // transactionHandler is a callback invoked on a transaction if it exists.
 type transactionHandler func(ctx context.Context, tx metadata.Transaction, client gitalypb.RefTransactionClient) error
 
-// runWithTransaction runs the given function if the environment identifies a transaction. No error
+// runWithTransaction runs the given function if the payload identifies a transaction. No error
 // is returned if no transaction exists. If a transaction exists and the function is executed on it,
 // then its error will ber returned directly.
-func (m *GitLabHookManager) runWithTransaction(ctx context.Context, env []string, handler transactionHandler) error {
-	tx, err := metadata.TransactionFromEnv(env)
-	if err != nil {
-		if errors.Is(err, metadata.ErrTransactionNotFound) {
-			// No transaction being present is valid, e.g. in case
-			// there is no Praefect server or the transactions
-			// feature flag is not set.
-			return nil
-		}
-		return fmt.Errorf("could not extract transaction: %w", err)
-	}
-
-	praefectServer, err := metadata.PraefectFromEnv(env)
-	if err != nil {
-		return fmt.Errorf("could not extract Praefect server: %w", err)
+func (m *GitLabHookManager) runWithTransaction(ctx context.Context, payload git.HooksPayload, handler transactionHandler) error {
+	if payload.Transaction == nil {
+		return nil
 	}
 
 	ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
 	defer cancel()
 
-	praefectConn, err := m.getPraefectConn(ctx, praefectServer)
+	praefectConn, err := m.getPraefectConn(ctx, payload.Praefect)
 	if err != nil {
 		return err
 	}
 
 	praefectClient := gitalypb.NewRefTransactionClient(praefectConn)
 
-	if err := handler(ctx, tx, praefectClient); err != nil {
+	if err := handler(ctx, *payload.Transaction, praefectClient); err != nil {
 		return err
 	}
 
 	return nil
 }
 
-func (m *GitLabHookManager) voteOnTransaction(ctx context.Context, hash []byte, env []string) error {
-	return m.runWithTransaction(ctx, env, func(ctx context.Context, tx metadata.Transaction, client gitalypb.RefTransactionClient) error {
+func (m *GitLabHookManager) voteOnTransaction(ctx context.Context, hash []byte, payload git.HooksPayload) error {
+	return m.runWithTransaction(ctx, payload, func(ctx context.Context, tx metadata.Transaction, client gitalypb.RefTransactionClient) error {
 		defer prometheus.NewTimer(m.votingDelayMetric).ObserveDuration()
 
 		response, err := client.VoteTransaction(ctx, &gitalypb.VoteTransactionRequest{
@@ -100,8 +81,8 @@ func (m *GitLabHookManager) voteOnTransaction(ctx context.Context, hash []byte,
 	})
 }
 
-func (m *GitLabHookManager) stopTransaction(ctx context.Context, env []string) error {
-	return m.runWithTransaction(ctx, env, func(ctx context.Context, tx metadata.Transaction, client gitalypb.RefTransactionClient) error {
+func (m *GitLabHookManager) stopTransaction(ctx context.Context, payload git.HooksPayload) error {
+	return m.runWithTransaction(ctx, payload, func(ctx context.Context, tx metadata.Transaction, client gitalypb.RefTransactionClient) error {
 		_, err := client.StopTransaction(ctx, &gitalypb.StopTransactionRequest{
 			TransactionId: tx.ID,
 		})
diff --git a/internal/gitaly/hook/update.go b/internal/gitaly/hook/update.go
index d902378a4..d887f2eca 100644
--- a/internal/gitaly/hook/update.go
+++ b/internal/gitaly/hook/update.go
@@ -10,11 +10,12 @@ import (
 )
 
 func (m *GitLabHookManager) UpdateHook(ctx context.Context, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error {
-	primary, err := isPrimary(env)
+	payload, err := git.HooksPayloadFromEnv(env)
 	if err != nil {
-		return helper.ErrInternalf("could not check role: %w", err)
+		return helper.ErrInternalf("extracting hooks payload: %w", err)
 	}
-	if !primary {
+
+	if !isPrimary(payload) {
 		return nil
 	}