Centralize binary finding logic

Gitaly needs to locate auxiliary binaries in various places to pass
them as arguments and call them. Currently, the logic to do so is
spread around the code base where the cfg.BinDir is joined with the
binary name. Soon we'll have to make a distinction between binaries
that are in the BinDir and binaries that are unpacked into a runtime
directory on start up. To support this, this commit centralizes the
binary finding logic to BinaryPath helper added to Gitaly's config.
For now, it simply join BinDir with the binary name as each of the
call sites were doing. In a later commit, it will also help with
locating the unpacked binaries correctly.

17 files changed, 40 insertions, 35 deletions

diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index 9db2ea23f..5004c8af6 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -161,8 +161,6 @@ func testHooksPrePostReceive(t *testing.T, cfg config.Cfg, repo *gitalypb.Reposi
 	cfg.Gitlab.HTTPSettings.User = gitlabUser
 	cfg.Gitlab.HTTPSettings.Password = gitlabPassword
 
-	gitalyHooksPath := filepath.Join(cfg.BinDir, "gitaly-hooks")
-
 	gitlabClient, err := gitlab.NewHTTPClient(logger, cfg.Gitlab, cfg.TLS, prometheus.Config{})
 	require.NoError(t, err)
 
@@ -180,7 +178,7 @@ func testHooksPrePostReceive(t *testing.T, cfg config.Cfg, repo *gitalypb.Reposi
 			var stderr, stdout bytes.Buffer
 			stdin := bytes.NewBuffer([]byte(changes))
 			require.NoError(t, err)
-			cmd := exec.Command(gitalyHooksPath)
+			cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"))
 			cmd.Args = []string{hookName}
 			cmd.Stderr = &stderr
 			cmd.Stdout = &stdout
@@ -278,7 +276,7 @@ func testHooksUpdate(t *testing.T, ctx context.Context, cfg config.Cfg, glValues
 
 	refval, oldval, newval := "refval", strings.Repeat("a", 40), strings.Repeat("b", 40)
 
-	cmd := exec.Command(filepath.Join(cfg.BinDir, "gitaly-hooks"))
+	cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"))
 	cmd.Args = []string{"update", refval, oldval, newval}
 	cmd.Env = envForHooks(t, ctx, cfg, repo, glValues, proxyValues{})
 	cmd.Dir = repoPath
@@ -522,7 +520,7 @@ func TestCheckOK(t *testing.T) {
 
 	configPath := writeTemporaryGitalyConfigFile(t, cfg)
 
-	cmd := exec.Command(filepath.Join(cfg.BinDir, "gitaly-hooks"), "check", configPath)
+	cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"), "check", configPath)
 
 	var stderr, stdout bytes.Buffer
 	cmd.Stderr = &stderr
@@ -568,7 +566,7 @@ func TestCheckBadCreds(t *testing.T) {
 
 	configPath := writeTemporaryGitalyConfigFile(t, cfg)
 
-	cmd := exec.Command(filepath.Join(cfg.BinDir, "gitaly-hooks"), "check", configPath)
+	cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"), "check", configPath)
 
 	var stderr, stdout bytes.Buffer
 	cmd.Stderr = &stderr
@@ -652,7 +650,7 @@ func TestGitalyHooksPackObjects(t *testing.T) {
 	baseArgs := []string{
 		"clone",
 		"-u",
-		"git -c uploadpack.allowFilter -c uploadpack.packObjectsHook=" + cfg.BinDir + "/gitaly-hooks upload-pack",
+		"git -c uploadpack.allowFilter -c uploadpack.packObjectsHook=" + cfg.BinaryPath("gitaly-hooks") + " upload-pack",
 		"--quiet",
 		"--no-local",
 		"--bare",
@@ -702,7 +700,7 @@ func TestRequestedHooks(t *testing.T) {
 				payload, err := git.NewHooksPayload(cfg, &gitalypb.Repository{}, nil, nil, git.AllHooks&^hook, nil).Env()
 				require.NoError(t, err)
 
-				cmd := exec.Command(filepath.Join(cfg.BinDir, "gitaly-hooks"))
+				cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"))
 				cmd.Args = hookArgs
 				cmd.Env = []string{payload}
 				require.NoError(t, cmd.Run())
@@ -716,7 +714,7 @@ func TestRequestedHooks(t *testing.T) {
 				payload, err := git.NewHooksPayload(cfg, &gitalypb.Repository{}, nil, nil, hook, nil).Env()
 				require.NoError(t, err)
 
-				cmd := exec.Command(filepath.Join(cfg.BinDir, "gitaly-hooks"))
+				cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"))
 				cmd.Args = hookArgs
 				cmd.Env = []string{payload}
 
diff --git a/cmd/gitaly-ssh/auth_test.go b/cmd/gitaly-ssh/auth_test.go
index a8cf5a6a3..af5f260b3 100644
--- a/cmd/gitaly-ssh/auth_test.go
+++ b/cmd/gitaly-ssh/auth_test.go
@@ -123,7 +123,7 @@ func TestConnectivity(t *testing.T) {
 				fmt.Sprintf("GITALY_ADDRESS=%s", addr),
 				fmt.Sprintf("GITALY_WD=%s", cwd),
 				fmt.Sprintf("PATH=.:%s", os.Getenv("PATH")),
-				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")),
+				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", cfg.BinaryPath("gitaly-ssh")),
 				fmt.Sprintf("SSL_CERT_FILE=%s", certFile),
 			}
 			if testcase.proxy {
diff --git a/cmd/gitaly-ssh/upload_pack_test.go b/cmd/gitaly-ssh/upload_pack_test.go
index fe85df5f1..fc6a99ce3 100644
--- a/cmd/gitaly-ssh/upload_pack_test.go
+++ b/cmd/gitaly-ssh/upload_pack_test.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -85,7 +84,7 @@ func TestVisibilityOfHiddenRefs(t *testing.T) {
 				fmt.Sprintf("GITALY_ADDRESS=unix:%s", socketPath),
 				fmt.Sprintf("GITALY_WD=%s", wd),
 				fmt.Sprintf("PATH=.:%s", os.Getenv("PATH")),
-				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")),
+				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", cfg.BinaryPath("gitaly-ssh")),
 			}
 
 			stdout := &bytes.Buffer{}
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 86918985f..7b9c39c90 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -275,11 +275,9 @@ func setupHookDirectories(cfg config.Cfg, factoryCfg execCommandFactoryConfig) (
 		return hookDirectories{}, nil, fmt.Errorf("creating temporary hooks directory: %w", err)
 	}
 
-	gitalyHooksPath := filepath.Join(cfg.BinDir, "gitaly-hooks")
-
 	// And now we symlink all required hooks to the wrapper script.
 	for _, hook := range []string{"pre-receive", "post-receive", "update", "reference-transaction"} {
-		if err := os.Symlink(gitalyHooksPath, filepath.Join(tempHooksPath, hook)); err != nil {
+		if err := os.Symlink(cfg.BinaryPath("gitaly-hooks"), filepath.Join(tempHooksPath, hook)); err != nil {
 			return hookDirectories{}, nil, fmt.Errorf("creating symlink for %s hook: %w", hook, err)
 		}
 	}
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index d709dd130..e62ce16be 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -348,7 +348,7 @@ func TestExecCommandFactoryHooksPath(t *testing.T) {
 			for _, hook := range []string{"update", "pre-receive", "post-receive", "reference-transaction"} {
 				target, err := os.Readlink(filepath.Join(hooksPath, hook))
 				require.NoError(t, err)
-				require.Equal(t, filepath.Join(cfg.BinDir, "gitaly-hooks"), target)
+				require.Equal(t, cfg.BinaryPath("gitaly-hooks"), target)
 			}
 		})
 
diff --git a/internal/git/command_options.go b/internal/git/command_options.go
index 044ae754d..5b5ed0680 100644
--- a/internal/git/command_options.go
+++ b/internal/git/command_options.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path/filepath"
 	"regexp"
 	"strings"
 
@@ -289,7 +288,7 @@ func withInternalFetch(req repoScopedRequest, withSidechannel bool) func(ctx con
 
 		c.env = append(c.env,
 			fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
-			fmt.Sprintf("GIT_SSH_COMMAND=%s %s", filepath.Join(cfg.BinDir, "gitaly-ssh"), "upload-pack"),
+			fmt.Sprintf("GIT_SSH_COMMAND=%s %s", cfg.BinaryPath("gitaly-ssh"), "upload-pack"),
 			fmt.Sprintf("GITALY_ADDRESS=%s", storageInfo.Address),
 			fmt.Sprintf("GITALY_TOKEN=%s", storageInfo.Token),
 			fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureFlagPairs, ",")),
diff --git a/internal/git/command_options_test.go b/internal/git/command_options_test.go
index 88f41d2d0..e5616d476 100644
--- a/internal/git/command_options_test.go
+++ b/internal/git/command_options_test.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/base64"
 	"fmt"
-	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -466,7 +465,7 @@ func TestWithInternalFetch(t *testing.T) {
 			require.NoError(t, option(ctx, cfg, gitCmdFactory, &commandCfg))
 
 			require.Subset(t, commandCfg.env, []string{
-				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")),
+				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", cfg.BinaryPath("gitaly-ssh")),
 				fmt.Sprintf("GITALY_PAYLOAD=%s", tc.expectedPayload),
 				"CORRELATION_ID=correlation-id-1",
 				"GIT_SSH_VARIANT=simple",
diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index 5a24f8a9d..27fef5510 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
@@ -80,7 +79,7 @@ func WithPackObjectsHookEnv(repo *gitalypb.Repository, protocol string) CmdOpt {
 
 		cc.globals = append(cc.globals, ConfigPair{
 			Key:   "uploadpack.packObjectsHook",
-			Value: filepath.Join(cfg.BinDir, "gitaly-hooks"),
+			Value: cfg.BinaryPath("gitaly-hooks"),
 		})
 
 		return nil
diff --git a/internal/git/smudge/config.go b/internal/git/smudge/config.go
index c64c4f643..ad126e447 100644
--- a/internal/git/smudge/config.go
+++ b/internal/git/smudge/config.go
@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
@@ -100,12 +99,12 @@ func (c Config) GitConfiguration(cfg config.Cfg) (git.ConfigPair, error) {
 	case DriverTypeFilter:
 		return git.ConfigPair{
 			Key:   "filter.lfs.smudge",
-			Value: filepath.Join(cfg.BinDir, "gitaly-lfs-smudge"),
+			Value: cfg.BinaryPath("gitaly-lfs-smudge"),
 		}, nil
 	case DriverTypeProcess:
 		return git.ConfigPair{
 			Key:   "filter.lfs.process",
-			Value: filepath.Join(cfg.BinDir, "gitaly-lfs-smudge"),
+			Value: cfg.BinaryPath("gitaly-lfs-smudge"),
 		}, nil
 	default:
 		return git.ConfigPair{}, fmt.Errorf("unknown driver type: %v", c.DriverType)
diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go
index f6fa27d78..515dca5f3 100644
--- a/internal/git2go/executor.go
+++ b/internal/git2go/executor.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"os/exec"
-	"path/filepath"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
@@ -42,7 +41,7 @@ type Executor struct {
 // configuration.
 func NewExecutor(cfg config.Cfg, gitCmdFactory git.CommandFactory, locator storage.Locator) *Executor {
 	return &Executor{
-		binaryPath:    filepath.Join(cfg.BinDir, BinaryName),
+		binaryPath:    cfg.BinaryPath(BinaryName),
 		gitCmdFactory: gitCmdFactory,
 		locator:       locator,
 		logFormat:     cfg.Logging.Format,
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 29718876f..342b39de0 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -329,6 +329,13 @@ func validateIsDirectory(path, name string) error {
 	return nil
 }
 
+// BinaryPath returns the path to a given binary. BinaryPath does not do any validation, it simply joins the binaryName
+// with the correct base directory.
+func (cfg *Cfg) BinaryPath(binaryName string) string {
+	baseDirectory := cfg.BinDir
+	return filepath.Join(baseDirectory, binaryName)
+}
+
 func (cfg *Cfg) validateStorages() error {
 	if len(cfg.Storages) == 0 {
 		return fmt.Errorf("no storage configurations found. Are you using the right format? https://gitlab.com/gitlab-org/gitaly/issues/397")
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 2967fd398..b6c2bc01f 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -18,6 +18,15 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
+func TestBinaryPath(t *testing.T) {
+	cfg := Cfg{
+		BinDir: "bindir",
+	}
+
+	require.Equal(t, "bindir/gitaly-hooks", cfg.BinaryPath("gitaly-hooks"))
+	require.Equal(t, "bindir", cfg.BinaryPath(""))
+}
+
 func TestLoadBrokenConfig(t *testing.T) {
 	tmpFile := strings.NewReader(`path = "/tmp"\nname="foo"`)
 	_, err := Load(tmpFile)
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index f4f4a7030..9151b64c3 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -429,7 +429,7 @@ func listenGitalySSHCalls(t *testing.T, conf config.Cfg) func() gitalySSHParams
 	t.Helper()
 
 	require.NotEmpty(t, conf.BinDir)
-	initialPath := filepath.Join(conf.BinDir, "gitaly-ssh")
+	initialPath := conf.BinaryPath("gitaly-ssh")
 	updatedPath := initialPath + "-actual"
 	require.NoError(t, os.Rename(initialPath, updatedPath))
 
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 5678a1866..a9dfad29c 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -287,7 +287,7 @@ func testServerPostUploadPackUsesPackObjectsHook(t *testing.T, ctx context.Conte
 	// out. In the best case we'd have just printed the error to stderr and
 	// check the return error message. But it's unfortunately not
 	// transferred back.
-	testhelper.WriteExecutable(t, filepath.Join(cfg.BinDir, "gitaly-hooks"), []byte(hookScript))
+	testhelper.WriteExecutable(t, cfg.BinaryPath("gitaly-hooks"), []byte(hookScript))
 
 	cfg.SocketPath = runSmartHTTPServer(t, cfg)
 
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 66fc1e596..2e86eae75 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -673,7 +673,7 @@ func sshPushCommand(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDeta
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_ADDRESS=%s", serverSocketPath),
 		fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureflag.AllFlags(ctx), ",")),
-		fmt.Sprintf("GIT_SSH_COMMAND=%s receive-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")),
+		fmt.Sprintf("GIT_SSH_COMMAND=%s receive-pack", cfg.BinaryPath("gitaly-ssh")),
 	}
 
 	return cmd
diff --git a/internal/gitaly/service/ssh/upload_archive_test.go b/internal/gitaly/service/ssh/upload_archive_test.go
index b9be5035c..1e1a95b83 100644
--- a/internal/gitaly/service/ssh/upload_archive_test.go
+++ b/internal/gitaly/service/ssh/upload_archive_test.go
@@ -3,7 +3,6 @@ package ssh
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"testing"
 	"time"
 
@@ -136,7 +135,7 @@ func TestUploadArchiveSuccess(t *testing.T) {
 			fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath),
 			fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 			fmt.Sprintf("PATH=%s", ".:"+os.Getenv("PATH")),
-			fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-archive`, filepath.Join(cfg.BinDir, "gitaly-ssh")),
+			fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-archive`, cfg.BinaryPath("gitaly-ssh")),
 		},
 	}, "archive", "master", "--remote=git@localhost:test/test.git")
 }
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 113cd816a..e8c93e066 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -60,7 +60,7 @@ func runClone(
 		fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath),
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureflag.AllFlags(ctx), ",")),
-		fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-pack`, filepath.Join(cfg.BinDir, "gitaly-ssh")),
+		fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-pack`, cfg.BinaryPath("gitaly-ssh")),
 	}
 	if withSidechannel {
 		env = append(env, "GITALY_USE_SIDECHANNEL=1")
@@ -617,7 +617,7 @@ func TestUploadPack_packObjectsHook(t *testing.T) {
 	// custom script which replaces the hook binary. It doesn't do anything
 	// special, but writes an error message and errors out and should thus
 	// cause the clone to fail with this error message.
-	testhelper.WriteExecutable(t, filepath.Join(filterDir, "gitaly-hooks"), []byte(fmt.Sprintf(
+	testhelper.WriteExecutable(t, cfg.BinaryPath("gitaly-hooks"), []byte(fmt.Sprintf(
 		`#!/bin/bash
 		set -eo pipefail
 		echo 'I was invoked' >'%s'
@@ -680,7 +680,7 @@ func testUploadPackWithoutSideband(t *testing.T, opts ...testcfg.Option) {
 	// Those simultaneous writes to both stdout and stderr created a race as we could've invoked
 	// two concurrent `SendMsg`s on the gRPC stream. And given that `SendMsg` is not thread-safe
 	// a deadlock would result.
-	uploadPack := exec.Command(filepath.Join(cfg.BinDir, "gitaly-ssh"), "upload-pack", "dontcare", "dontcare")
+	uploadPack := exec.Command(cfg.BinaryPath("gitaly-ssh"), "upload-pack", "dontcare", "dontcare")
 	uploadPack.Env = []string{
 		fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath),
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),