gitaly/config: Allow configuration without `gitaly-hooks` directory

Our configuration validation logic enforces that the `gitaly-hooks`
directory must be set. This directory is used for two different things,
both of which already have proper replacements:

    - It identifies the location of the `.gitlab_shell_secret` file,
      which is required in order to connect to GitLab. This can also be
      configured via `[gitlab] secret` and `[gitlab] secret_file`.

    - It identifies the location of custom hooks. This can also be
      configured via `[hooks] custom_hooks_directory`.

The `gitaly-hooks` section can thus be considered legacy and should
ideally not be used anymore.

Rewrite the configuration validation to allow the `gitaly-hooks`
directory to remain unset. Instead, we will verify that the GitLab
secret is properly configured.

2 files changed, 104 insertions, 37 deletions

diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index e70fa272e..477baebb5 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -592,7 +592,7 @@ func (cfg *Cfg) Validate() error {
 		cfg.validateListeners,
 		cfg.validateStorages,
 		cfg.validateGit,
-		cfg.validateShell,
+		cfg.validateGitlabSecret,
 		cfg.validateCustomHooks,
 		cfg.validateBinDir,
 		cfg.validateRuntimeDir,
@@ -712,12 +712,20 @@ func (cfg *Cfg) validateListeners() error {
 	return nil
 }
 
-func (cfg *Cfg) validateShell() error {
-	if len(cfg.GitlabShell.Dir) == 0 {
-		return fmt.Errorf("gitlab-shell.dir: is not set")
+func (cfg *Cfg) validateGitlabSecret() error {
+	switch {
+	case len(cfg.Gitlab.Secret) > 0:
+		return nil
+	case len(cfg.Gitlab.SecretFile) > 0:
+		return validateIsFile(cfg.Gitlab.SecretFile, "gitlab.secret_file")
+	case len(cfg.GitlabShell.Dir) > 0:
+		// Note that we do not verify that the secret actually exists, but only verify that the directory
+		// exists. This is not as thorough as we could be, but is done in order to retain our legacy behaviour
+		// in case the secret file wasn't explicitly configured.
+		return validateIsDirectory(cfg.GitlabShell.Dir, "gitlab-shell.dir")
+	default:
+		return fmt.Errorf("GitLab secret not configured")
 	}
-
-	return validateIsDirectory(cfg.GitlabShell.Dir, "gitlab-shell.dir")
 }
 
 func (cfg *Cfg) validateCustomHooks() error {
@@ -747,6 +755,21 @@ func validateIsDirectory(path, name string) error {
 	return nil
 }
 
+func validateIsFile(path, name string) error {
+	s, err := os.Stat(path)
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("%s: path doesn't exist: %q", name, path)
+		}
+		return fmt.Errorf("%s: %w", name, err)
+	}
+	if !s.Mode().IsRegular() {
+		return fmt.Errorf("%s: not a file: %q", name, path)
+	}
+
+	return nil
+}
+
 // packedBinaries are the binaries that are packed in the main Gitaly binary. This should always match
 // the actual list in <root>/packed_binaries.go so the binaries are correctly located.
 //
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 80ca1641b..97d1a1618 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -776,49 +776,93 @@ func TestGit_Validate(t *testing.T) {
 	}
 }
 
-func TestValidateShellPath(t *testing.T) {
-	tmpDir := testhelper.TempDir(t)
-
-	require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "bin"), perm.SharedDir))
-	tmpFile := filepath.Join(tmpDir, "my-file")
-	require.NoError(t, os.WriteFile(tmpFile, []byte{}, perm.PublicFile))
+func TestCfg_ValidateGitlabSecret(t *testing.T) {
+	t.Parallel()
 
-	testCases := []struct {
-		desc      string
-		path      string
-		expErrMsg string
+	for _, tc := range []struct {
+		desc  string
+		setup func(t *testing.T) (Cfg, error)
 	}{
 		{
-			desc:      "When no Shell Path set",
-			path:      "",
-			expErrMsg: "gitlab-shell.dir: is not set",
+			desc: "empty configuration",
+			setup: func(t *testing.T) (Cfg, error) {
+				return Cfg{}, fmt.Errorf("GitLab secret not configured")
+			},
 		},
 		{
-			desc:      "When Shell Path set to non-existing path",
-			path:      "/non/existing/path",
-			expErrMsg: `gitlab-shell.dir: path doesn't exist: "/non/existing/path"`,
+			desc: "with GitLab secret",
+			setup: func(t *testing.T) (Cfg, error) {
+				return Cfg{
+					Gitlab: Gitlab{
+						Secret: "secret token",
+					},
+				}, nil
+			},
 		},
 		{
-			desc:      "When Shell Path set to non-dir path",
-			path:      tmpFile,
-			expErrMsg: fmt.Sprintf(`gitlab-shell.dir: not a directory: %q`, tmpFile),
+			desc: "with non-existent GitLab secret file",
+			setup: func(t *testing.T) (Cfg, error) {
+				return Cfg{
+					Gitlab: Gitlab{
+						SecretFile: "/does/not/exist",
+					},
+				}, fmt.Errorf("%s: path doesn't exist: %q", "gitlab.secret_file", "/does/not/exist")
+			},
 		},
 		{
-			desc:      "When Shell Path set to a valid directory",
-			path:      tmpDir,
-			expErrMsg: "",
+			desc: "with existing GitLab secret file",
+			setup: func(t *testing.T) (Cfg, error) {
+				path := filepath.Join(testhelper.TempDir(t), "secret")
+				require.NoError(t, os.WriteFile(path, []byte("something"), 0o644))
+
+				return Cfg{
+					Gitlab: Gitlab{
+						SecretFile: path,
+					},
+				}, nil
+			},
 		},
-	}
+		{
+			desc: "with non-existent gitlab-shell directory",
+			setup: func(t *testing.T) (Cfg, error) {
+				return Cfg{
+					GitlabShell: GitlabShell{
+						Dir: "/does/not/exist",
+					},
+				}, fmt.Errorf("%s: path doesn't exist: %q", "gitlab-shell.dir", "/does/not/exist")
+			},
+		},
+		{
+			desc: "with non-existent gitlab-shell secret file",
+			setup: func(t *testing.T) (Cfg, error) {
+				// We do not expect an error in case the secret file doesn't exist. This is done in
+				// order to retain our legacy behaviour with a GitLab secret file whose location is
+				// derived from the gitlab-shell directory.
+				return Cfg{
+					GitlabShell: GitlabShell{
+						Dir: testhelper.TempDir(t),
+					},
+				}, nil
+			},
+		},
+		{
+			desc: "with existing gitlab-shell secret file",
+			setup: func(t *testing.T) (Cfg, error) {
+				shellDir := testhelper.TempDir(t)
+				secretPath := filepath.Join(shellDir, ".gitlab_secret_file")
+				require.NoError(t, os.WriteFile(secretPath, []byte("something"), 0o644))
 
-	for _, tc := range testCases {
+				return Cfg{
+					GitlabShell: GitlabShell{
+						Dir: shellDir,
+					},
+				}, nil
+			},
+		},
+	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			cfg := Cfg{GitlabShell: GitlabShell{Dir: tc.path}}
-			err := cfg.validateShell()
-			if tc.expErrMsg != "" {
-				assert.EqualError(t, err, tc.expErrMsg)
-			} else {
-				assert.NoError(t, err)
-			}
+			cfg, expectedErr := tc.setup(t)
+			require.Equal(t, expectedErr, cfg.validateGitlabSecret())
 		})
 	}
 }