Merge branch 'jc-gitaly-internal-socket' into 'master'

Use internal socket dir for internal gitaly socket

Closes #2184

See merge request gitlab-org/gitaly!1642

7 files changed, 80 insertions, 32 deletions

diff --git a/changelogs/unreleased/jc-gitaly-internal-socket.yml b/changelogs/unreleased/jc-gitaly-internal-socket.yml
new file mode 100644
index 000000000..267cab425
--- /dev/null
+++ b/changelogs/unreleased/jc-gitaly-internal-socket.yml
@@ -0,0 +1,5 @@
+---
+title: Use internal socket dir for internal gitaly socket
+merge_request: 1642
+author:
+type: other
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 624c08747..f8860df2c 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -92,6 +92,7 @@ func run(b *bootstrap.Bootstrap) error {
 
 	for _, c := range []starter.Config{
 		{starter.Unix, config.Config.SocketPath},
+		{starter.Unix, config.GitalyInternalSocketPath()},
 		{starter.TCP, config.Config.ListenAddr},
 		{starter.TLS, config.Config.TLSListenAddr},
 	} {
diff --git a/internal/bootstrap/server_factory.go b/internal/bootstrap/server_factory.go
index ca6d870d1..c754fbf53 100644
--- a/internal/bootstrap/server_factory.go
+++ b/internal/bootstrap/server_factory.go
@@ -39,6 +39,7 @@ func (s *GitalyServerFactory) Stop() {
 	}
 
 	s.ruby.Stop()
+	server.CleanupInternalSocketDir()
 }
 
 // GracefulStop stops both the secure and insecure servers gracefully
diff --git a/internal/config/config.go b/internal/config/config.go
index d38616586..a814e315c 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -3,11 +3,13 @@ package config
 import (
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/BurntSushi/toml"
@@ -357,6 +359,52 @@ func validateToken() error {
 	return nil
 }
 
+var (
+	lazyInit                   sync.Once
+	generatedInternalSocketDir string
+)
+
+func generateSocketPath() {
+	// 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
+	// will later want to set its permissions to 0700
+
+	var err error
+	generatedInternalSocketDir, err = ioutil.TempDir("", "gitaly-internal")
+	if err != nil {
+		log.Fatalf("create ruby server socket directory: %v", err)
+	}
+}
+
+// InternalSocketDir will generate a temp dir for internal sockets if one is not provided in the config
+func InternalSocketDir() string {
+	if Config.InternalSocketDir != "" {
+		return Config.InternalSocketDir
+	}
+
+	if generatedInternalSocketDir == "" {
+		lazyInit.Do(generateSocketPath)
+	}
+
+	return generatedInternalSocketDir
+}
+
+// GitalyInternalSocketPath is the path to the internal gitaly socket
+func GitalyInternalSocketPath() string {
+	socketDir := InternalSocketDir()
+	if socketDir == "" {
+		panic("internal socket directory is missing")
+	}
+
+	return filepath.Join(socketDir, "internal.sock")
+}
+
+// GeneratedInternalSocketDir returns the path to the generated internal socket directory
+func GeneratedInternalSocketDir() string {
+	return generatedInternalSocketDir
+}
+
 func validateInternalSocketDir() error {
 	if Config.InternalSocketDir == "" {
 		return nil
@@ -372,6 +420,10 @@ func validateInternalSocketDir() error {
 		return fmt.Errorf("InternalSocketDir %s is not a directory", dir)
 	}
 
+	return trySocketCreation(dir)
+}
+
+func trySocketCreation(dir string) error {
 	// 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
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index fac19e48a..abe48934c 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -714,3 +714,14 @@ func TestValidateInternalSocketDir(t *testing.T) {
 		})
 	}
 }
+
+func TestInternalSocketDir(t *testing.T) {
+	defer func(internalSocketDir string) {
+		Config.InternalSocketDir = internalSocketDir
+	}(Config.InternalSocketDir)
+
+	Config.InternalSocketDir = ""
+	socketDir := InternalSocketDir()
+
+	require.NoError(t, trySocketCreation(socketDir))
+}
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 5f55e3ff4..c95b6934d 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -3,7 +3,6 @@ package rubyserver
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
 	"net"
 	"os"
 	"path"
@@ -14,7 +13,6 @@ import (
 
 	grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
 	grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
-	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
@@ -31,10 +29,6 @@ import (
 )
 
 var (
-	socketDir string
-
-	lazyInit sync.Once
-
 	// ConnectTimeout is the timeout for establishing a connection to the gitaly-ruby process.
 	ConnectTimeout = 40 * time.Second
 )
@@ -46,27 +40,10 @@ 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
-	// will later want to set its permissions to 0700. The permission change
-	// is done in the Ruby child process.
-	var err error
-	socketDir, err = ioutil.TempDir("", "gitaly-ruby")
-	if err != nil {
-		log.Fatalf("create ruby server socket directory: %v", err)
-	}
-}
-
 func socketPath(id int) string {
+	socketDir := config.InternalSocketDir()
 	if socketDir == "" {
-		panic("socketDir is not set")
+		panic("internal socket directory is missing")
 	}
 
 	return filepath.Join(socketDir, fmt.Sprintf("ruby.%d", id))
@@ -95,11 +72,6 @@ func (s *Server) Stop() {
 			w.Process.Stop()
 		}
 	}
-
-	// If we use the old gitaly-ruby temp dir, we should clean up
-	if config.Config.InternalSocketDir == "" && socketDir != "" {
-		os.RemoveAll(socketDir)
-	}
 }
 
 // Start spawns the Ruby server.
@@ -109,8 +81,6 @@ func (s *Server) Start() error {
 }
 
 func (s *Server) start() error {
-	lazyInit.Do(prepareSocketPath)
-
 	wd, err := os.Getwd()
 	if err != nil {
 		return err
diff --git a/internal/server/server.go b/internal/server/server.go
index 97013d48e..250749248 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -3,6 +3,7 @@ package server
 import (
 	"context"
 	"crypto/tls"
+	"os"
 
 	grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
@@ -139,3 +140,10 @@ func NewInsecure(rubyServer *rubyserver.Server) *grpc.Server {
 func NewSecure(rubyServer *rubyserver.Server) *grpc.Server {
 	return createNewServer(rubyServer, true)
 }
+
+// CleanupInternalSocketDir will clean up the directory for internal sockets if it is a generated temp dir
+func CleanupInternalSocketDir() {
+	if tmpDir := config.GeneratedInternalSocketDir(); tmpDir != "" {
+		os.RemoveAll(tmpDir)
+	}
+}