Allow socket dir for Gitaly-Ruby to be configured

In case the `/tmp` directory can't be used for security reasons, this
can be configured. The default behaviour as in production right now will
remain. So if nothing is set, `/tmp` will be reused.

10 files changed, 159 insertions, 7 deletions

diff --git a/changelogs/unreleased/zj-config-ruby-socket-dir.yml b/changelogs/unreleased/zj-config-ruby-socket-dir.yml
new file mode 100644
index 000000000..8da63c07f
--- /dev/null
+++ b/changelogs/unreleased/zj-config-ruby-socket-dir.yml
@@ -0,0 +1,5 @@
+---
+title: Allow socket dir for Gitaly-Ruby to be configured
+merge_request: 1184
+author:
+type: added
diff --git a/cmd/gitaly-hooks/gitaly_hooks.log b/cmd/gitaly-hooks/gitaly_hooks.log
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/cmd/gitaly-hooks/gitaly_hooks.log
diff --git a/config.toml.example b/config.toml.example
index fccfc31d9..9c5db6013 100644
--- a/config.toml.example
+++ b/config.toml.example
@@ -14,6 +14,11 @@ bin_dir = "/home/git/gitaly"
 # # Optional: export metrics via Prometheus
 # prometheus_listen_addr = "localhost:9236"
 
+# # Optional: configure where the Gitaly creates the sockets for internal connections. If unset, Gitaly will create a randomly
+# # named temp directory each time it boots.
+# # Non Gitaly clients should never connect to these sockets.
+# internal_socket_dir = "/home/git/gitlab/tmp/sockets/private/internal"
+
 # # Optional: authenticate Gitaly requests using a shared secret
 # [auth]
 # token = 'abc123secret'
diff --git a/doc/configuration/README.md b/doc/configuration/README.md
index a1a087973..936477ce5 100644
--- a/doc/configuration/README.md
+++ b/doc/configuration/README.md
@@ -42,6 +42,7 @@ name = "my_shard"
 |----|----|--------|-----|
 |socket_path|string|see notes|A path which gitaly should open a Unix socket. Required unless listen_addr is set|
 |listen_addr|string|see notes|TCP address for Gitaly to listen on (See #GITALY_LISTEN_ADDR). Required unless socket_path is set|
+|internal_socket_dir|string|yes|Path where Gitaly will create sockets for internal Gitaly calls to connect to|
 |bin_dir|string|yes|Directory containing Gitaly's executables|
 |prometheus_listen_addr|string|no|TCP listen address for Prometheus metrics. If not set, no Prometheus listener is started|
 |storage|array|yes|An array of storage shards|
diff --git a/internal/config/config.go b/internal/config/config.go
index 5fa84002f..259ff23b0 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -3,6 +3,7 @@ package config
 import (
 	"fmt"
 	"io"
+	"net"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -15,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	internallog "gitlab.com/gitlab-org/gitaly/internal/config/log"
 	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
 const (
@@ -49,6 +51,7 @@ type Cfg struct {
 	Concurrency                []Concurrency `toml:"concurrency"`
 	GracefulRestartTimeout     time.Duration
 	GracefulRestartTimeoutToml duration `toml:"graceful_restart_timeout"`
+	InternalSocketDir          string   `toml:"internal_socket_dir"`
 }
 
 // TLS configuration
@@ -146,6 +149,7 @@ func Validate() error {
 		validateShell(),
 		ConfigureRuby(),
 		validateBinDir(),
+		validateInternalSocketDir(),
 	} {
 		if err != nil {
 			return err
@@ -300,3 +304,38 @@ func validateToken() error {
 	log.Warn("Authentication is enabled but not enforced because transitioning=true. Gitaly will accept unauthenticated requests.")
 	return nil
 }
+
+func validateInternalSocketDir() error {
+	if Config.InternalSocketDir == "" {
+		return nil
+	}
+
+	dir := Config.InternalSocketDir
+
+	f, err := os.Stat(dir)
+	switch {
+	case err != nil:
+		return fmt.Errorf("InternalSocketDir: %s", err)
+	case !f.IsDir():
+		return fmt.Errorf("InternalSocketDir %s is not a directory", dir)
+	}
+
+	// To validate the socket can actually be created, we open and close a socket.
+	// Any error will be assumed persistent for when the gitaly-ruby sockets are created
+	// and thus fatal at boot time
+	b, err := text.RandomHex(4)
+	if err != nil {
+		return err
+	}
+
+	socketPath := filepath.Join(dir, fmt.Sprintf("test-%s.sock", b))
+	defer os.Remove(socketPath)
+
+	// Attempt to create an actual socket and not just a file to catch socket path length problems
+	l, err := net.Listen("unix", socketPath)
+	if err != nil {
+		return fmt.Errorf("socket could not be created in %s: %s", dir, err)
+	}
+
+	return l.Close()
+}
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 9827a2668..59666890a 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -551,3 +551,71 @@ func TestLoadGracefulRestartTimeout(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateInternalSocketDir(t *testing.T) {
+	defer func(internalSocketDir string) {
+		Config.InternalSocketDir = internalSocketDir
+	}(Config.InternalSocketDir)
+
+	// create a valid socket directory
+	tempDir, err := ioutil.TempDir("testdata", t.Name())
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	// create a symlinked socket directory
+	dirName := "internal_socket_dir"
+	validSocketDirSymlink := filepath.Join(tempDir, dirName)
+	tmpSocketDir, err := ioutil.TempDir(tempDir, "")
+	require.NoError(t, err)
+	tmpSocketDir, err = filepath.Abs(tmpSocketDir)
+	require.NoError(t, err)
+	require.NoError(t, os.Symlink(tmpSocketDir, validSocketDirSymlink))
+
+	// create a broken symlink
+	dirName = "internal_socket_dir_broken"
+	brokenSocketDirSymlink := filepath.Join(tempDir, dirName)
+	require.NoError(t, os.Symlink("/does/not/exist", brokenSocketDirSymlink))
+
+	testCases := []struct {
+		desc              string
+		internalSocketDir string
+		shouldError       bool
+	}{
+		{
+			desc:              "empty socket dir",
+			internalSocketDir: "",
+			shouldError:       false,
+		},
+		{
+			desc:              "non existing directory",
+			internalSocketDir: "/tmp/relative/path/to/nowhere",
+			shouldError:       true,
+		},
+		{
+			desc:              "valid socket directory",
+			internalSocketDir: tempDir,
+			shouldError:       false,
+		},
+		{
+			desc:              "valid symlinked directory",
+			internalSocketDir: validSocketDirSymlink,
+			shouldError:       false,
+		},
+		{
+			desc:              "broken symlinked directory",
+			internalSocketDir: brokenSocketDirSymlink,
+			shouldError:       true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			Config.InternalSocketDir = tc.internalSocketDir
+			if tc.shouldError {
+				assert.Error(t, validateInternalSocketDir())
+				return
+			}
+			assert.NoError(t, validateInternalSocketDir())
+		})
+	}
+}
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 21bfbd744..2f8e57bc9 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -47,6 +47,11 @@ func init() {
 }
 
 func prepareSocketPath() {
+	if config.Config.InternalSocketDir != "" {
+		socketDir = config.Config.InternalSocketDir
+		return
+	}
+
 	// The socket path must be short-ish because listen(2) fails on long
 	// socket paths. We hope/expect that ioutil.TempDir creates a directory
 	// that is not too deep. We need a directory, not a tempfile, because we
@@ -64,7 +69,7 @@ func socketPath(id int) string {
 		panic("socketDir is not set")
 	}
 
-	return path.Join(filepath.Clean(socketDir), fmt.Sprintf("socket.%d", id))
+	return filepath.Join(socketDir, fmt.Sprintf("ruby.%d", id))
 }
 
 // Server represents a gitaly-ruby helper process.
@@ -91,7 +96,8 @@ func (s *Server) Stop() {
 		}
 	}
 
-	if socketDir != "" {
+	// If we use the old gitaly-ruby temp dir, we should clean up
+	if config.Config.InternalSocketDir == "" && socketDir != "" {
 		os.RemoveAll(socketDir)
 	}
 }
@@ -133,7 +139,7 @@ func (s *Server) start() error {
 		env = append(env, "SENTRY_ENVIRONMENT="+sentryEnvironment)
 	}
 
-	gitalyRuby := path.Join(cfg.Ruby.Dir, "bin/gitaly-ruby")
+	gitalyRuby := path.Join(cfg.Ruby.Dir, "bin", "gitaly-ruby")
 
 	numWorkers := cfg.Ruby.NumWorkers
 	balancer.ConfigureBuilder(numWorkers, 0)
diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go
index 0c1af2cfd..f2a05721d 100644
--- a/internal/service/conflicts/testhelper_test.go
+++ b/internal/service/conflicts/testhelper_test.go
@@ -1,11 +1,13 @@
 package conflicts
 
 import (
+	"io/ioutil"
 	"net"
 	"os"
 	"testing"
 
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -23,7 +25,14 @@ var RubyServer = &rubyserver.Server{}
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	hooks.Override = "/does/not/exist"
+	tempDir, err := ioutil.TempDir("", "gitaly")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	hooks.Override = tempDir + "/hooks"
+	config.Config.InternalSocketDir = tempDir + "/sock"
 
 	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
diff --git a/internal/service/wiki/testhelper_test.go b/internal/service/wiki/testhelper_test.go
index 1e0f0e062..5de3ac199 100644
--- a/internal/service/wiki/testhelper_test.go
+++ b/internal/service/wiki/testhelper_test.go
@@ -2,6 +2,7 @@ package wiki
 
 import (
 	"bytes"
+	"io/ioutil"
 	"net"
 	"os"
 	"path"
@@ -10,6 +11,7 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -29,18 +31,24 @@ type createWikiPageOpts struct {
 
 var (
 	mockPageContent = bytes.Repeat([]byte("Mock wiki page content"), 10000)
+	rubyServer      = &rubyserver.Server{}
 )
 
 func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-var rubyServer = &rubyserver.Server{}
-
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	hooks.Override = "/does/not/exist"
+	tempDir, err := ioutil.TempDir("", "gitaly")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	hooks.Override = tempDir + "/hooks"
+	config.Config.InternalSocketDir = tempDir + "/sock"
 
 	if err := rubyServer.Start(); err != nil {
 		log.Fatal(err)
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 178e174dd..54d5447c6 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -58,6 +58,13 @@ func configure() error {
 	config.Config.SocketPath = "/bogus"
 	config.Config.GitlabShell.Dir = "/"
 
+	dir, err := ioutil.TempDir("", "internal_socket")
+	if err != nil {
+		return err
+	}
+
+	config.Config.InternalSocketDir = dir
+
 	for _, f := range []func() error{
 		ConfigureRuby,
 		config.Validate,
@@ -284,6 +291,10 @@ func ConfigureRuby() error {
 		config.Config.Ruby.Dir = path.Join(path.Dir(currentFile), "../../ruby")
 	}
 
+	if err := config.ConfigureRuby(); err != nil {
+		log.Fatalf("validate ruby config: %v", err)
+	}
+
 	return nil
 }