Merge branch 'pks-gitaly-runtime-dir' into 'master'

gitaly/config: Introduce runtime directory configuration

Closes #4113

See merge request gitlab-org/gitaly!4415

8 files changed, 156 insertions, 7 deletions

diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 7d58bdd88..a0e231cb1 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -130,6 +130,12 @@ func run(cfg config.Cfg) error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
+	defer func() {
+		if err := os.RemoveAll(cfg.RuntimeDir); err != nil {
+			log.Warn("could not clean up runtime dir")
+		}
+	}()
+
 	b, err := bootstrap.New(promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "gitaly_connections_total",
diff --git a/config.toml.example b/config.toml.example
index eaa23fd8e..81df9f246 100644
--- a/config.toml.example
+++ b/config.toml.example
@@ -7,6 +7,11 @@ socket_path = "/home/git/gitlab/tmp/sockets/private/gitaly.socket"
 # The directory where Gitaly's executables are stored
 bin_dir = "/home/git/gitaly/_build/bin"
 
+# # Optional: The directory where Gitaly can create all files required to
+# # properly operate at runtime. If not set, Gitaly will create a directory in
+# # the global temporary directory. This directory must exist.
+# runtime_dir = "/home/git/gitaly/run"
+
 # # Optional: listen on a TCP socket. This is insecure (no authentication)
 # listen_addr = "localhost:9999"
 # tls_listen_addr = "localhost:8888"
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index a8a9bdbf9..056b8b170 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -266,7 +266,7 @@ func setupHookDirectories(cfg config.Cfg, factoryCfg execCommandFactoryConfig) (
 
 	// This sets up the new hook location. Hooks now live in a temporary directory, where all
 	// hooks are symlinks to the `gitaly-hooks` binary.
-	tempHooksPath, err := os.MkdirTemp("", "gitaly-hooks-")
+	tempHooksPath, err := os.MkdirTemp(cfg.RuntimeDir, "hooks-*.d")
 	if err != nil {
 		return hookDirectories{}, nil, fmt.Errorf("creating temporary hooks directory: %w", err)
 	}
diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go
index a13f1dfad..f4d981a51 100644
--- a/internal/git/execution_environment.go
+++ b/internal/git/execution_environment.go
@@ -177,7 +177,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 	// In order to support having a single Git binary only as compared to a complete Git
 	// installation, we create our own GIT_EXEC_PATH which contains symlinks to the Git
 	// binary for executables which Git expects to be present.
-	gitExecPath, err := os.MkdirTemp("", "gitaly-git-exec-path-*")
+	gitExecPath, err := os.MkdirTemp(cfg.RuntimeDir, "git-exec-*.d")
 	if err != nil {
 		return ExecutionEnvironment{}, fmt.Errorf("creating Git exec path: %w", err)
 	}
diff --git a/internal/git/gittest/testhelper_test.go b/internal/git/gittest/testhelper_test.go
index 2085d22a0..4565301d2 100644
--- a/internal/git/gittest/testhelper_test.go
+++ b/internal/git/gittest/testhelper_test.go
@@ -45,6 +45,9 @@ func setup(t testing.TB) (config.Cfg, *gitalypb.Repository, string) {
 	cfg.BinDir = filepath.Join(rootDir, "bin.d")
 	require.NoError(t, os.Mkdir(cfg.BinDir, 0o755))
 
+	cfg.RuntimeDir = filepath.Join(rootDir, "run.d")
+	require.NoError(t, os.Mkdir(cfg.RuntimeDir, 0o700))
+
 	require.NoError(t, cfg.Validate())
 
 	repo, repoPath := CloneRepo(t, cfg, cfg.Storages[0])
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 9bd46f2df..3f6757db3 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -48,6 +48,7 @@ type Cfg struct {
 	TLSListenAddr          string            `toml:"tls_listen_addr" split_words:"true"`
 	PrometheusListenAddr   string            `toml:"prometheus_listen_addr" split_words:"true"`
 	BinDir                 string            `toml:"bin_dir"`
+	RuntimeDir             string            `toml:"runtime_dir"`
 	Git                    Git               `toml:"git" envconfig:"git"`
 	Storages               []Storage         `toml:"storage" envconfig:"storage"`
 	Logging                Logging           `toml:"logging" envconfig:"logging"`
@@ -187,6 +188,7 @@ func (cfg *Cfg) Validate() error {
 		cfg.validateShell,
 		cfg.ConfigureRuby,
 		cfg.validateBinDir,
+		cfg.validateRuntimeDir,
 		cfg.validateInternalSocketDir,
 		cfg.validateMaintenance,
 		cfg.validateCgroups,
@@ -213,17 +215,57 @@ func (cfg *Cfg) setDefaults() error {
 		cfg.Hooks.CustomHooksDir = filepath.Join(cfg.GitlabShell.Dir, "hooks")
 	}
 
+	if cfg.RuntimeDir == "" {
+		// If there is no runtime directory configured we just use a temporary runtime
+		// directory. This may not always be an ideal choice given that it's typically
+		// created at `/tmp`, which may get periodically pruned if `noatime` is set.
+		runtimeDir, err := os.MkdirTemp("", "gitaly-")
+		if err != nil {
+			return fmt.Errorf("creating temporary runtime directory: %w", err)
+		}
+
+		cfg.RuntimeDir = runtimeDir
+	} else {
+		// Otherwise, we use the configured runtime directory. Note that we don't use the
+		// runtime directory directly, but instead create a subdirectory within it which is
+		// based on the process's PID. While we could use `MkdirTemp()` instead and don't
+		// bother with preexisting directories, the benefit of using the PID here is that we
+		// can determine whether the directory may still be in use by checking whether the
+		// PID exists. Furthermore, it allows easier debugging in case one wants to inspect
+		// the runtime directory of a running Gitaly node.
+
+		runtimeDir := filepath.Join(cfg.RuntimeDir, fmt.Sprintf("gitaly-%d", os.Getpid()))
+
+		if _, err := os.Stat(runtimeDir); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("statting runtime directory: %w", err)
+		} else if err != nil {
+			// If the directory exists already then it must be from an old invocation of
+			// Gitaly. Because we use the PID as path component we know that the old
+			// instance cannot exist anymore though, so it's safe to remove this
+			// directory now.
+			if err := os.RemoveAll(runtimeDir); err != nil {
+				return fmt.Errorf("removing old runtime directory: %w", err)
+			}
+		}
+
+		if err := os.Mkdir(runtimeDir, 0o700); err != nil {
+			return fmt.Errorf("creating runtime directory: %w", err)
+		}
+
+		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
-
-		tmpDir, err := os.MkdirTemp("", "gitaly-internal")
-		if err != nil {
+		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 = tmpDir
+
+		cfg.InternalSocketDir = socketDir
 	}
 
 	if reflect.DeepEqual(cfg.DailyMaintenance, DailyJob{}) {
@@ -350,6 +392,20 @@ func (cfg *Cfg) validateBinDir() error {
 	return err
 }
 
+func (cfg *Cfg) validateRuntimeDir() error {
+	if len(cfg.RuntimeDir) == 0 {
+		return fmt.Errorf("runtime_dir: is not set")
+	}
+
+	if err := validateIsDirectory(cfg.RuntimeDir, "runtime_dir"); err != nil {
+		return err
+	}
+
+	var err error
+	cfg.RuntimeDir, err = filepath.Abs(cfg.RuntimeDir)
+	return err
+}
+
 // validateGitConfigKey does a best-effort check whether or not a given git config key is valid. It
 // does not allow for assignments in keys, which is overly strict and does not allow some valid
 // keys. It does avoid misinterpretation of keys though and should catch many cases of
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 3546deae9..82c345c71 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -34,6 +34,12 @@ func TestLoadEmptyConfig(t *testing.T) {
 		InternalSocketDir: cfg.InternalSocketDir,
 	}
 	require.NoError(t, expectedCfg.setDefaults())
+
+	// The runtime directory is a temporary path, so we need to take the value from the loaded
+	// config. Furthermore, because `setDefaults()` would append the PID, we can't do so before
+	// calling that function.
+	expectedCfg.RuntimeDir = cfg.RuntimeDir
+
 	require.Equal(t, expectedCfg, cfg)
 }
 
@@ -1045,3 +1051,71 @@ func TestValidateBinDir(t *testing.T) {
 		})
 	}
 }
+
+func TestCfg_RuntimeDir(t *testing.T) {
+	t.Run("defaults", func(t *testing.T) {
+		t.Run("empty runtime directory", func(t *testing.T) {
+			cfg := Cfg{}
+			require.NoError(t, cfg.setDefaults())
+
+			require.Equal(t, os.TempDir(), filepath.Dir(cfg.RuntimeDir))
+			require.True(t, strings.HasPrefix(filepath.Base(cfg.RuntimeDir), "gitaly-"))
+			require.DirExists(t, cfg.RuntimeDir)
+		})
+
+		t.Run("non-existent runtime directory", func(t *testing.T) {
+			cfg := Cfg{
+				RuntimeDir: "/does/not/exist",
+			}
+
+			require.EqualError(t, cfg.setDefaults(), fmt.Sprintf("creating runtime directory: mkdir /does/not/exist/gitaly-%d: no such file or directory", os.Getpid()))
+		})
+
+		t.Run("existent runtime directory", func(t *testing.T) {
+			dir := testhelper.TempDir(t)
+			cfg := Cfg{
+				RuntimeDir: dir,
+			}
+			require.NoError(t, cfg.setDefaults())
+			require.Equal(t, filepath.Join(dir, fmt.Sprintf("gitaly-%d", os.Getpid())), cfg.RuntimeDir)
+			require.DirExists(t, cfg.RuntimeDir)
+		})
+	})
+
+	t.Run("validation", func(t *testing.T) {
+		dirPath := testhelper.TempDir(t)
+		filePath := filepath.Join(dirPath, "file")
+		require.NoError(t, os.WriteFile(filePath, nil, 0o644))
+
+		for _, tc := range []struct {
+			desc        string
+			runtimeDir  string
+			expectedErr error
+		}{
+			{
+				desc:       "valid runtime directory",
+				runtimeDir: dirPath,
+			},
+			{
+				desc:        "unset",
+				runtimeDir:  "",
+				expectedErr: fmt.Errorf("runtime_dir: is not set"),
+			},
+			{
+				desc:        "path doesn't exist",
+				runtimeDir:  "/does/not/exist",
+				expectedErr: fmt.Errorf("runtime_dir: path doesn't exist: %q", "/does/not/exist"),
+			},
+			{
+				desc:        "path is not a directory",
+				runtimeDir:  filePath,
+				expectedErr: fmt.Errorf(`runtime_dir: not a directory: %q`, filePath),
+			},
+		} {
+			t.Run(tc.desc, func(t *testing.T) {
+				err := (&Cfg{RuntimeDir: tc.runtimeDir}).validateRuntimeDir()
+				require.Equal(t, tc.expectedErr, err)
+			})
+		}
+	})
+}
diff --git a/internal/testhelper/testcfg/gitaly_builder.go b/internal/testhelper/testcfg/gitaly_builder.go
index 3f670ebcb..855bbe549 100644
--- a/internal/testhelper/testcfg/gitaly_builder.go
+++ b/internal/testhelper/testcfg/gitaly_builder.go
@@ -106,8 +106,13 @@ func (gc *GitalyCfgBuilder) Build(t testing.TB) config.Cfg {
 		require.NoError(t, os.Mkdir(cfg.GitlabShell.Dir, 0o755))
 	}
 
+	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(root, "internal_socks.d")
+		cfg.InternalSocketDir = filepath.Join(cfg.RuntimeDir, "sock.d")
 		require.NoError(t, os.Mkdir(cfg.InternalSocketDir, 0o755))
 	}