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 109b15502..ef9e0f786 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 007522314..3dd9eb1b2 100644
--- a/cmd/gitaly-ssh/auth_test.go
+++ b/cmd/gitaly-ssh/auth_test.go
@@ -122,7 +122,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 9a4c8b881..de4ee857e 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 9a60a78c4..de98c6f97 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -347,7 +347,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 89e24eabc..d72c8f120 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"
 
@@ -292,7 +291,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(flagsWithValue, ",")),
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 9a77a2ce8..c99f87f92 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 6256a9e74..264f34a56 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 6e06ab8a3..38f2cfd04 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -281,6 +281,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 313331ae6..cfd28b8f2 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -20,6 +20,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 69c049ea9..1b6d17954 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -420,7 +420,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 a65c17ef5..342285ccc 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -684,7 +684,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(flagsWithValues, ",")),
-		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 a9d010699..811517e84 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -65,7 +65,7 @@ func runClone(
 		fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath),
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(flagsWithValues, ",")),
-		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")
@@ -622,7 +622,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'
@@ -685,7 +685,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),