config: Remove internal socket path configurationpks-config-remove-internal-socket-directory

With commit 7a8b33aa7 (gitaly/config: Introduce runtime directory
configuration, 2022-03-17), we have introduced a new global runtime
directory that is supposed to hold all files and directories required by
Gitaly to operate correctly. This new directory supersedes the previous
internal runtime directory, which shouldn't be used anymore.

We have since converted the GDK, CNG and Omnibus accordingly and put up
a deprecation notice for v15.0. Let's thus remove the old configuration
option so that we now always use the runtime directory instead.

Changelog: removed

7 files changed, 105 insertions, 82 deletions

diff --git a/internal/git/gittest/testhelper_test.go b/internal/git/gittest/testhelper_test.go
index 4565301d2..47cefc0cd 100644
--- a/internal/git/gittest/testhelper_test.go
+++ b/internal/git/gittest/testhelper_test.go
@@ -47,6 +47,7 @@ func setup(t testing.TB) (config.Cfg, *gitalypb.Repository, string) {
 
 	cfg.RuntimeDir = filepath.Join(rootDir, "run.d")
 	require.NoError(t, os.Mkdir(cfg.RuntimeDir, 0o700))
+	require.NoError(t, os.Mkdir(cfg.InternalSocketDir(), 0o700))
 
 	require.NoError(t, cfg.Validate())
 
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 6dffa0535..8e9328104 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -62,7 +62,6 @@ type Cfg struct {
 	Concurrency            []Concurrency     `toml:"concurrency"`
 	RateLimiting           []RateLimiting    `toml:"rate_limiting"`
 	GracefulRestartTimeout Duration          `toml:"graceful_restart_timeout"`
-	InternalSocketDir      string            `toml:"internal_socket_dir"`
 	DailyMaintenance       DailyJob          `toml:"daily_maintenance"`
 	Cgroups                cgroups.Config    `toml:"cgroups"`
 	PackObjectsCache       StreamCacheConfig `toml:"pack_objects_cache"`
@@ -272,17 +271,12 @@ func (cfg *Cfg) setDefaults() error {
 		cfg.RuntimeDir = runtimeDir
 	}
 
-	if cfg.InternalSocketDir == "" {
-		// The socket path must be short-ish because listen(2) fails on long
-		// socket paths. We hope/expect that os.MkdirTemp 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
-		socketDir := filepath.Join(cfg.RuntimeDir, "sock.d")
-		if err := os.Mkdir(socketDir, 0o700); err != nil {
-			return fmt.Errorf("create internal socket directory: %w", err)
-		}
-
-		cfg.InternalSocketDir = socketDir
+	// The socket path must be short-ish because listen(2) fails on long
+	// socket paths. We hope/expect that os.MkdirTemp 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
+	if err := os.Mkdir(cfg.InternalSocketDir(), 0o700); err != nil {
+		return fmt.Errorf("create internal socket directory: %w", err)
 	}
 
 	if reflect.DeepEqual(cfg.DailyMaintenance, DailyJob{}) {
@@ -390,9 +384,14 @@ func (cfg *Cfg) Storage(storageName string) (Storage, bool) {
 	return Storage{}, false
 }
 
+// InternalSocketDir returns the location of the internal socket directory.
+func (cfg *Cfg) InternalSocketDir() string {
+	return filepath.Join(cfg.RuntimeDir, "sock.d")
+}
+
 // InternalSocketPath is the path to the internal Gitaly socket.
 func (cfg *Cfg) InternalSocketPath() string {
-	return filepath.Join(cfg.InternalSocketDir, fmt.Sprintf("internal_%d.sock", os.Getpid()))
+	return filepath.Join(cfg.InternalSocketDir(), fmt.Sprintf("internal_%d.sock", os.Getpid()))
 }
 
 func (cfg *Cfg) validateBinDir() error {
@@ -466,17 +465,14 @@ func (cfg *Cfg) validateToken() error {
 }
 
 func (cfg *Cfg) validateInternalSocketDir() error {
-	if cfg.InternalSocketDir == "" {
-		return nil
-	}
-
-	if err := validateIsDirectory(cfg.InternalSocketDir, "internal_socket_dir"); err != nil {
+	if err := validateIsDirectory(cfg.InternalSocketDir(), "internal_socket_dir"); err != nil {
 		return err
 	}
 
-	if err := trySocketCreation(cfg.InternalSocketDir); err != nil {
+	if err := trySocketCreation(cfg.InternalSocketDir()); err != nil {
 		return fmt.Errorf("internal_socket_dir: try create socket: %w", err)
 	}
+
 	return nil
 }
 
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 82c345c71..763fe4b1c 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -1,7 +1,6 @@
 package config
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -30,8 +29,7 @@ func TestLoadEmptyConfig(t *testing.T) {
 	require.NoError(t, err)
 
 	expectedCfg := Cfg{
-		Prometheus:        prometheus.DefaultConfig(),
-		InternalSocketDir: cfg.InternalSocketDir,
+		Prometheus: prometheus.DefaultConfig(),
 	}
 	require.NoError(t, expectedCfg.setDefaults())
 
@@ -604,80 +602,112 @@ dir = '%s'`, gitlabShellDir))
 }
 
 func TestValidateInternalSocketDir(t *testing.T) {
-	// create a valid socket directory
-	tmpDir := testhelper.TempDir(t)
-	// create a symlinked socket directory
-	dirName := "internal_socket_dir"
-	validSocketDirSymlink := filepath.Join(tmpDir, dirName)
-	tmpSocketDir, err := os.MkdirTemp(tmpDir, "")
-	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(tmpDir, dirName)
-	require.NoError(t, os.Symlink("/does/not/exist", brokenSocketDirSymlink))
-
-	pathTooLongForSocket := filepath.Join(tmpDir, strings.Repeat("/nested_directory", 10))
-	require.NoError(t, os.MkdirAll(pathTooLongForSocket, os.ModePerm))
+	verifyPathDoesNotExist := func(t *testing.T, runtimeDir string, actualErr error) {
+		require.Equal(t, fmt.Errorf("internal_socket_dir: path doesn't exist: %q", filepath.Join(runtimeDir, "sock.d")), actualErr)
+	}
 
 	testCases := []struct {
-		desc              string
-		internalSocketDir string
-		expErrMsgRegexp   string
+		desc   string
+		setup  func(t *testing.T) string
+		verify func(t *testing.T, runtimeDir string, actualErr error)
 	}{
 		{
-			desc:              "empty socket dir",
-			internalSocketDir: "",
+			desc: "unconfigured runtime directory",
+			setup: func(t *testing.T) string {
+				return ""
+			},
+			verify: verifyPathDoesNotExist,
 		},
 		{
-			desc:              "non existing directory",
-			internalSocketDir: "/tmp/relative/path/to/nowhere",
-			expErrMsgRegexp:   `internal_socket_dir: path doesn't exist: "/tmp/relative/path/to/nowhere"`,
+			desc: "non existing directory",
+			setup: func(t *testing.T) string {
+				return "/path/does/not/exist"
+			},
+			verify: verifyPathDoesNotExist,
 		},
 		{
-			desc:              "valid socket directory",
-			internalSocketDir: tmpDir,
+			desc: "runtime directory missing sock.d",
+			setup: func(t *testing.T) string {
+				runtimeDir := testhelper.TempDir(t)
+				return runtimeDir
+			},
+			verify: verifyPathDoesNotExist,
 		},
 		{
-			desc:              "valid symlinked directory",
-			internalSocketDir: validSocketDirSymlink,
+			desc: "runtime directory with valid sock.d",
+			setup: func(t *testing.T) string {
+				runtimeDir := testhelper.TempDir(t)
+				require.NoError(t, os.Mkdir(filepath.Join(runtimeDir, "sock.d"), os.ModePerm))
+				return runtimeDir
+			},
+		},
+		{
+			desc: "symlinked runtime directory",
+			setup: func(t *testing.T) string {
+				runtimeDir := testhelper.TempDir(t)
+				require.NoError(t, os.Mkdir(filepath.Join(runtimeDir, "sock.d"), os.ModePerm))
+
+				// Create a symlink which points to the real runtime directory.
+				symlinkDir := testhelper.TempDir(t)
+				symlink := filepath.Join(symlinkDir, "symlink-to-runtime-dir")
+				require.NoError(t, os.Symlink(runtimeDir, symlink))
+
+				return symlink
+			},
 		},
 		{
-			desc:              "broken symlinked directory",
-			internalSocketDir: brokenSocketDirSymlink,
-			expErrMsgRegexp:   fmt.Sprintf(`internal_socket_dir: path doesn't exist: %q`, brokenSocketDirSymlink),
+			desc: "broken symlinked runtime directory",
+			setup: func(t *testing.T) string {
+				symlinkDir := testhelper.TempDir(t)
+				symlink := filepath.Join(symlinkDir, "symlink-to-runtime-dir")
+				require.NoError(t, os.Symlink("/path/does/not/exist", symlink))
+				return symlink
+			},
+			verify: verifyPathDoesNotExist,
 		},
 		{
-			desc:              "socket can't be created",
-			internalSocketDir: pathTooLongForSocket,
-			expErrMsgRegexp:   `internal_socket_dir: try create socket: socket could not be created in .*\/test-.{8}\.sock: bind: invalid argument`,
+			desc: "socket can't be created",
+			setup: func(t *testing.T) string {
+				tempDir := testhelper.TempDir(t)
+
+				runtimeDirTooLongForSockets := filepath.Join(tempDir, strings.Repeat("/nested_directory", 10))
+				socketDir := filepath.Join(runtimeDirTooLongForSockets, "sock.d")
+				require.NoError(t, os.MkdirAll(socketDir, os.ModePerm))
+
+				return runtimeDirTooLongForSockets
+			},
+			verify: func(t *testing.T, runtimeDir string, actualErr error) {
+				require.Error(t, actualErr)
+				require.Regexp(t,
+					fmt.Sprintf(
+						"internal_socket_dir: try create socket: socket could not be created in %s: listen unix %s: bind: invalid argument",
+						filepath.Join(runtimeDir, "sock\\.d"),
+						filepath.Join(runtimeDir, "sock\\.d", "test-.{8}\\.sock"),
+					),
+					actualErr.Error(),
+				)
+			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			err := (&Cfg{InternalSocketDir: tc.internalSocketDir}).validateInternalSocketDir()
-			if tc.expErrMsgRegexp != "" {
-				assert.Regexp(t, tc.expErrMsgRegexp, err)
+			runtimeDir := tc.setup(t)
+
+			cfg := Cfg{
+				RuntimeDir: runtimeDir,
+			}
+
+			actualErr := cfg.validateInternalSocketDir()
+			if tc.verify == nil {
+				require.NoError(t, actualErr)
 			} else {
-				assert.NoError(t, err)
+				tc.verify(t, runtimeDir, actualErr)
 			}
 		})
 	}
 }
 
-func TestInternalSocketDir(t *testing.T) {
-	cfg, err := Load(bytes.NewReader(nil))
-	require.NoError(t, err)
-	socketDir := cfg.InternalSocketDir
-
-	require.NoError(t, trySocketCreation(socketDir))
-	require.NoError(t, os.RemoveAll(socketDir))
-}
-
 func TestLoadDailyMaintenance(t *testing.T) {
 	for _, tt := range []struct {
 		name        string
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index f9342446a..794178123 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -142,7 +142,7 @@ func (s *Server) start() error {
 
 	for i := 0; i < numWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
-		socketPath := filepath.Join(cfg.InternalSocketDir, fmt.Sprintf("ruby.%d", i))
+		socketPath := filepath.Join(cfg.InternalSocketDir(), fmt.Sprintf("ruby.%d", i))
 
 		// Use 'ruby-cd' to make sure gitaly-ruby has the same working directory
 		// as the current process. This is a hack to sort-of support relative
diff --git a/internal/gitaly/rubyserver/rubyserver_test.go b/internal/gitaly/rubyserver/rubyserver_test.go
index 4cdc40f8d..d852841a1 100644
--- a/internal/gitaly/rubyserver/rubyserver_test.go
+++ b/internal/gitaly/rubyserver/rubyserver_test.go
@@ -98,8 +98,8 @@ func (mockGitCommandFactory) HooksPath(context.Context) string {
 
 func TestSetupEnv(t *testing.T) {
 	cfg := config.Cfg{
-		BinDir:            "/bin/dit",
-		InternalSocketDir: "/gitaly",
+		BinDir:     "/bin/dit",
+		RuntimeDir: "/gitaly",
 		Logging: config.Logging{
 			Config: log.Config{
 				Dir: "/log/dir",
diff --git a/internal/testhelper/testcfg/gitaly_builder.go b/internal/testhelper/testcfg/gitaly_builder.go
index 855bbe549..86974e4a6 100644
--- a/internal/testhelper/testcfg/gitaly_builder.go
+++ b/internal/testhelper/testcfg/gitaly_builder.go
@@ -109,11 +109,7 @@ func (gc *GitalyCfgBuilder) Build(t testing.TB) config.Cfg {
 	if cfg.RuntimeDir == "" {
 		cfg.RuntimeDir = filepath.Join(root, "runtime.d")
 		require.NoError(t, os.Mkdir(cfg.RuntimeDir, 0o700))
-	}
-
-	if cfg.InternalSocketDir == "" {
-		cfg.InternalSocketDir = filepath.Join(cfg.RuntimeDir, "sock.d")
-		require.NoError(t, os.Mkdir(cfg.InternalSocketDir, 0o755))
+		require.NoError(t, os.Mkdir(cfg.InternalSocketDir(), 0o755))
 	}
 
 	if len(cfg.Storages) != 0 && len(gc.storages) != 0 {
diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go
index 60ffb750e..6033e7e12 100644
--- a/internal/testhelper/testserver/gitaly.go
+++ b/internal/testhelper/testserver/gitaly.go
@@ -161,7 +161,7 @@ func runGitaly(t testing.TB, cfg config.Cfg, rubyServer *rubyserver.Server, regi
 		[]*limithandler.LimiterMiddleware{deps.GetLimitHandler()},
 	)
 
-	if cfg.InternalSocketDir != "" {
+	if cfg.RuntimeDir != "" {
 		internalServer, err := serverFactory.CreateInternal()
 		require.NoError(t, err)
 		t.Cleanup(internalServer.Stop)
@@ -169,8 +169,8 @@ func runGitaly(t testing.TB, cfg config.Cfg, rubyServer *rubyserver.Server, regi
 		registrar(internalServer, deps)
 		registerHealthServerIfNotRegistered(internalServer)
 
-		require.NoError(t, os.MkdirAll(cfg.InternalSocketDir, 0o700))
-		t.Cleanup(func() { require.NoError(t, os.RemoveAll(cfg.InternalSocketDir)) })
+		require.NoError(t, os.MkdirAll(cfg.InternalSocketDir(), 0o700))
+		t.Cleanup(func() { require.NoError(t, os.RemoveAll(cfg.InternalSocketDir())) })
 
 		internalListener, err := net.Listen("unix", cfg.InternalSocketPath())
 		require.NoError(t, err)