Merge branch 'extend-shell-paths' into 'master'

Fix gitlab-shell paths by setting bin-path. Also require gitlab-shell path to be set

See merge request gitlab-org/gitaly!365

6 files changed, 30 insertions, 44 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2da2d8dd5..b34220e0c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ UNRELEASED
   https://gitlab.com/gitlab-org/gitaly/merge_requests/366
 - Implement RepositoryService::CreateRepository
   https://gitlab.com/gitlab-org/gitaly/merge_requests/361
+- Fix path bug for gitlab-shell. gitlab-shell path is now required
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/365
 
 v0.40.0
 - Use context cancellation instead of command.Close
diff --git a/_support/test-boot-time b/_support/test-boot-time
index bfd62f9b9..d264d8577 100755
--- a/_support/test-boot-time
+++ b/_support/test-boot-time
@@ -20,6 +20,9 @@ path = "#{dir}"
 
 [gitaly-ruby]
 dir = "#{gitaly_dir}/ruby"
+
+[gitlab-shell]
+dir = "#{gitaly_dir}"
 EOS
 )
 
diff --git a/internal/command/command.go b/internal/command/command.go
index a2d7adabf..742b87fb2 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -83,12 +83,8 @@ func GitPath() string {
 
 // GitlabShell creates a gitlab-shell Command with the given args
 func GitlabShell(ctx context.Context, envs []string, executable string, args ...string) (*Command, error) {
-	shellPath, ok := config.GitlabShellPath()
-	if !ok {
-		return nil, fmt.Errorf("path to gitlab-shell not set")
-	}
 	// Don't allow any git-command to ask (interactively) for credentials
-	return New(ctx, exec.Command(path.Join(shellPath, executable), args...), nil, nil, nil, envs...)
+	return New(ctx, exec.Command(path.Join(config.GitlabShellBinPath(), executable), args...), nil, nil, nil, envs...)
 }
 
 var wg = &sync.WaitGroup{}
diff --git a/internal/config/config.go b/internal/config/config.go
index 8bc707d55..a755b39d4 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"path"
 
 	log "github.com/sirupsen/logrus"
 
@@ -89,24 +90,23 @@ func Validate() error {
 
 func validateShell() error {
 	if len(Config.GitlabShell.Dir) == 0 {
-		log.WithField("dir", Config.GitlabShell.Dir).
-			Warn("gitlab-shell.dir not set")
-		return nil
+		return fmt.Errorf("gitlab-shell.dir is not set")
 	}
 
-	if s, err := os.Stat(Config.GitlabShell.Dir); err != nil {
-		log.WithField("dir", Config.GitlabShell.Dir).
-			WithError(err).
-			Warn("gitlab-shell.dir set but not found")
+	return validateIsDirectory(Config.GitlabShell.Dir, "gitlab-shell.dir")
+}
+
+func validateIsDirectory(path, name string) error {
+	s, err := os.Stat(path)
+	if err != nil {
 		return err
-	} else if !s.IsDir() {
-		log.WithField("dir", Config.GitlabShell.Dir).
-			Warn("gitlab-shell.dir set but not a directory")
-		return fmt.Errorf("not a directory: %q", Config.GitlabShell.Dir)
+	}
+	if !s.IsDir() {
+		return fmt.Errorf("not a directory: %q", path)
 	}
 
-	log.WithField("dir", Config.GitlabShell.Dir).
-		Debug("gitlab-shell.dir set")
+	log.WithField("dir", path).
+		Debugf("%s set", name)
 
 	return nil
 }
@@ -168,11 +168,8 @@ func StoragePath(storageName string) (string, bool) {
 	return "", false
 }
 
-// GitlabShellPath returns the full path to gitlab-shell.
+// GitlabShellBinPath returns the full path to gitlab-shell.
 // The second boolean return value indicates if it's found
-func GitlabShellPath() (string, bool) {
-	if len(Config.GitlabShell.Dir) == 0 {
-		return "", false
-	}
-	return Config.GitlabShell.Dir, true
+func GitlabShellBinPath() string {
+	return path.Join(Config.GitlabShell.Dir, "bin")
 }
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 4329602f4..05cbb48e2 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -297,6 +297,7 @@ func TestValidateShellPath(t *testing.T) {
 
 	tmpDir, err := ioutil.TempDir("", "gitaly-tests-")
 	require.NoError(t, err)
+	require.NoError(t, os.MkdirAll(path.Join(tmpDir, "bin"), 0755))
 	tmpFile := path.Join(tmpDir, "my-file")
 	defer os.RemoveAll(tmpDir)
 	fp, err := os.Create(tmpFile)
@@ -311,7 +312,7 @@ func TestValidateShellPath(t *testing.T) {
 		{
 			desc:      "When no Shell Path set",
 			path:      "",
-			shouldErr: false,
+			shouldErr: true,
 		},
 		{
 			desc:      "When Shell Path set to non-existing path",
diff --git a/internal/service/repository/fetch_remote_test.go b/internal/service/repository/fetch_remote_test.go
index a4776ffe9..e84af57be 100644
--- a/internal/service/repository/fetch_remote_test.go
+++ b/internal/service/repository/fetch_remote_test.go
@@ -112,10 +112,11 @@ func TestFetchRemoteSuccess(t *testing.T) {
 
 	dir, err := ioutil.TempDir("", "gitlab-shell.")
 	require.NoError(t, err)
+	require.NoError(t, os.MkdirAll(path.Join(dir, "bin"), 0755))
 	defer func(dir string) {
 		os.RemoveAll(dir)
 	}(dir)
-	testhelper.MustRunCommand(t, nil, "go", "build", "-o", path.Join(dir, "gitlab-projects"), "gitlab.com/gitlab-org/gitaly/internal/testhelper/gitlab-projects")
+	testhelper.MustRunCommand(t, nil, "go", "build", "-o", path.Join(dir, "bin", "gitlab-projects"), "gitlab.com/gitlab-org/gitaly/internal/testhelper/gitlab-projects")
 
 	// We need to know about gitlab-shell...
 	defer func(oldPath string) {
@@ -152,11 +153,10 @@ func TestFetchRemoteFailure(t *testing.T) {
 	client, _ := newRepositoryClient(t)
 
 	tests := []struct {
-		desc      string
-		req       *pb.FetchRemoteRequest
-		shellPath string
-		code      codes.Code
-		err       string
+		desc string
+		req  *pb.FetchRemoteRequest
+		code codes.Code
+		err  string
 	}{
 		{
 			desc: "invalid storage",
@@ -164,12 +164,6 @@ func TestFetchRemoteFailure(t *testing.T) {
 			code: codes.NotFound,
 			err:  "Storage not found",
 		},
-		{
-			desc: "no shell-path",
-			req:  &pb.FetchRemoteRequest{Repository: testRepo, Remote: "origin"},
-			code: codes.Internal,
-			err:  "gitlab-shell",
-		},
 	}
 
 	for _, tc := range tests {
@@ -177,13 +171,6 @@ func TestFetchRemoteFailure(t *testing.T) {
 			ctx, cancel := testhelper.Context()
 			defer cancel()
 
-			if len(tc.shellPath) == 0 {
-				defer func(oldPath string) {
-					config.Config.GitlabShell.Dir = oldPath
-				}(config.Config.GitlabShell.Dir)
-				config.Config.GitlabShell.Dir = ""
-			}
-
 			resp, err := client.FetchRemote(ctx, tc.req)
 			testhelper.AssertGrpcError(t, err, tc.code, tc.err)
 			assert.Error(t, err)