Merge branch 'jt-move-gitaly-hooks-check' into 'master'

cli: Relocated check subcommand to Gitaly binary

Closes #4320

See merge request gitlab-org/gitaly!4763

8 files changed, 206 insertions, 160 deletions

diff --git a/_support/terraform/roles/gitlab/handlers/main.yml b/_support/terraform/roles/gitlab/handlers/main.yml
index 08d1c835c..c6fc7e57a 100644
--- a/_support/terraform/roles/gitlab/handlers/main.yml
+++ b/_support/terraform/roles/gitlab/handlers/main.yml
@@ -13,6 +13,6 @@
 
 - name: verify gitaly-hooks configuration
   command:
-    cmd: /opt/gitlab/embedded/bin/gitaly-hooks check /var/opt/gitlab/gitaly/config.toml
+    cmd: /opt/gitlab/embedded/bin/gitaly check /var/opt/gitlab/gitaly/config.toml
   delegate_to: '{{ item }}'
   with_items: '{{ groups.gitalies }}'
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 95bca501e..39d0e4ba4 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -9,14 +9,10 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/sirupsen/logrus"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/v15/auth"
 	"gitlab.com/gitlab-org/gitaly/v15/client"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/env"
 	gitalylog "gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
@@ -88,31 +84,6 @@ func run(args []string) error {
 		subCmd := args[1]
 
 		switch subCmd {
-		case "check":
-			logrus.SetLevel(logrus.ErrorLevel)
-			if len(args) != 3 {
-				fmt.Fprint(os.Stderr, "no configuration file path provided invoke with: gitaly-hooks check <config_path>")
-				os.Exit(1)
-			}
-
-			configPath := args[2]
-			fmt.Print("Checking GitLab API access: ")
-
-			info, err := check(configPath)
-			if err != nil {
-				fmt.Print("FAIL\n")
-				fmt.Fprint(os.Stderr, err)
-				os.Exit(1)
-			}
-
-			fmt.Print("OK\n")
-			fmt.Printf("GitLab version: %s\n", info.Version)
-			fmt.Printf("GitLab revision: %s\n", info.Revision)
-			fmt.Printf("GitLab Api version: %s\n", info.APIVersion)
-			fmt.Printf("Redis reachable for GitLab: %t\n", info.RedisReachable)
-			fmt.Println("OK")
-
-			return nil
 		case "git":
 			return executeHook(hookCommand{
 				exec:     packObjectsHook,
@@ -215,32 +186,6 @@ func sendFunc(reqWriter io.Writer, stream grpc.ClientStream, stdin io.Reader) fu
 	}
 }
 
-func check(configPath string) (*gitlab.CheckInfo, error) {
-	cfgFile, err := os.Open(configPath)
-	if err != nil {
-		return nil, fmt.Errorf("failed to open config file: %w", err)
-	}
-	defer cfgFile.Close()
-
-	cfg, err := config.Load(cfgFile)
-	if err != nil {
-		return nil, err
-	}
-
-	gitlabAPI, err := gitlab.NewHTTPClient(logrus.New(), cfg.Gitlab, cfg.TLS, prometheus.Config{})
-	if err != nil {
-		return nil, err
-	}
-
-	gitCmdFactory, cleanup, err := git.NewExecCommandFactory(cfg)
-	if err != nil {
-		return nil, err
-	}
-	defer cleanup()
-
-	return hook.NewManager(cfg, config.NewLocator(cfg), gitCmdFactory, nil, gitlabAPI).Check(context.TODO())
-}
-
 func updateHook(ctx context.Context, payload git.HooksPayload, hookClient gitalypb.HookServiceClient, args []string) error {
 	if len(args) != 3 {
 		return fmt.Errorf("update hook expects exactly three arguments, got %q", args)
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index 54e4fd026..798772597 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -14,7 +14,6 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/pelletier/go-toml"
 	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -492,93 +491,6 @@ func TestHooksNotAllowed(t *testing.T) {
 	require.NoFileExists(t, customHookOutputPath)
 }
 
-func TestCheckOK(t *testing.T) {
-	user, password := "user123", "password321"
-
-	c := gitlab.TestServerOptions{
-		User:                        user,
-		Password:                    password,
-		SecretToken:                 "",
-		GLRepository:                "",
-		Changes:                     "",
-		PostReceiveCounterDecreased: false,
-		Protocol:                    "ssh",
-	}
-	serverURL, cleanup := gitlab.NewTestServer(t, c)
-	defer cleanup()
-
-	cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{
-		Gitlab: config.Gitlab{
-			URL: serverURL,
-			HTTPSettings: config.HTTPSettings{
-				User:     user,
-				Password: password,
-			},
-			SecretFile: gitlab.WriteShellSecretFile(t, testhelper.TempDir(t), "the secret"),
-		},
-	}))
-	testcfg.BuildGitalyHooks(t, cfg)
-	testcfg.BuildGitalySSH(t, cfg)
-
-	configPath := writeTemporaryGitalyConfigFile(t, cfg)
-
-	cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"), "check", configPath)
-
-	var stderr, stdout bytes.Buffer
-	cmd.Stderr = &stderr
-	cmd.Stdout = &stdout
-
-	err := cmd.Run()
-	require.NoError(t, err)
-	require.Empty(t, stderr.String())
-
-	output := stdout.String()
-	require.Contains(t, output, "Checking GitLab API access: OK")
-	require.Contains(t, output, "Redis reachable for GitLab: true")
-}
-
-func TestCheckBadCreds(t *testing.T) {
-	user, password := "user123", "password321"
-
-	c := gitlab.TestServerOptions{
-		User:                        user,
-		Password:                    password,
-		SecretToken:                 "",
-		GLRepository:                "",
-		Changes:                     "",
-		PostReceiveCounterDecreased: false,
-		Protocol:                    "ssh",
-		GitPushOptions:              nil,
-	}
-	serverURL, cleanup := gitlab.NewTestServer(t, c)
-	defer cleanup()
-
-	cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{
-		Gitlab: config.Gitlab{
-			URL: serverURL,
-			HTTPSettings: config.HTTPSettings{
-				User:     "wrong",
-				Password: password,
-			},
-			SecretFile: gitlab.WriteShellSecretFile(t, testhelper.TempDir(t), "the secret"),
-		},
-	}))
-	testcfg.BuildGitalyHooks(t, cfg)
-	testcfg.BuildGitalySSH(t, cfg)
-
-	configPath := writeTemporaryGitalyConfigFile(t, cfg)
-
-	cmd := exec.Command(cfg.BinaryPath("gitaly-hooks"), "check", configPath)
-
-	var stderr, stdout bytes.Buffer
-	cmd.Stderr = &stderr
-	cmd.Stdout = &stdout
-
-	require.Error(t, cmd.Run())
-	require.Contains(t, stderr.String(), "HTTP GET to GitLab endpoint /check failed: authorization failed")
-	require.Regexp(t, `Checking GitLab API access: .* level=error msg="Internal API error" .* error="authorization failed" method=GET status=401 url="http://127.0.0.1:[0-9]+/api/v4/internal/check"\nFAIL`, stdout.String())
-}
-
 func runHookServiceServer(t *testing.T, cfg config.Cfg, serverOpts ...testserver.GitalyServerOpt) {
 	runHookServiceWithGitlabClient(t, cfg, gitlab.NewMockClient(
 		t, gitlab.MockAllowed, gitlab.MockPreReceive, gitlab.MockPostReceive,
@@ -729,17 +641,3 @@ func TestRequestedHooks(t *testing.T) {
 		})
 	}
 }
-
-// writeTemporaryGitalyConfigFile writes the given Gitaly configuration into a temporary file and
-// returns its path.
-func writeTemporaryGitalyConfigFile(t testing.TB, cfg config.Cfg) string {
-	t.Helper()
-
-	path := filepath.Join(testhelper.TempDir(t), "config.toml")
-
-	contents, err := toml.Marshal(cfg)
-	require.NoError(t, err)
-	require.NoError(t, os.WriteFile(path, contents, 0o644))
-
-	return path
-}
diff --git a/cmd/gitaly/check.go b/cmd/gitaly/check.go
new file mode 100644
index 000000000..00dcb7603
--- /dev/null
+++ b/cmd/gitaly/check.go
@@ -0,0 +1,72 @@
+package main
+
+import (
+	"context"
+	"flag"
+	"fmt"
+	"os"
+
+	"github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
+)
+
+func execCheck() {
+	logrus.SetLevel(logrus.ErrorLevel)
+
+	checkCmd := flag.NewFlagSet("check", flag.ExitOnError)
+	checkCmd.Usage = func() {
+		fmt.Fprintf(os.Stderr, "Usage: %v check <configfile>\n", os.Args[0])
+		checkCmd.PrintDefaults()
+	}
+
+	_ = checkCmd.Parse(os.Args[2:])
+
+	if checkCmd.NArg() != 1 || checkCmd.Arg(0) == "" {
+		fmt.Fprintf(os.Stderr, "error: invalid argument(s)")
+		checkCmd.Usage()
+		os.Exit(2)
+	}
+
+	configPath := checkCmd.Arg(0)
+
+	fmt.Print("Checking GitLab API access: ")
+	info, err := checkAPI(configPath)
+	if err != nil {
+		fmt.Println("FAILED")
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	}
+
+	fmt.Println("OK")
+	fmt.Printf("GitLab version: %s\n", info.Version)
+	fmt.Printf("GitLab revision: %s\n", info.Revision)
+	fmt.Printf("GitLab Api version: %s\n", info.APIVersion)
+	fmt.Printf("Redis reachable for GitLab: %t\n", info.RedisReachable)
+	fmt.Println("OK")
+
+	os.Exit(0)
+}
+
+func checkAPI(configPath string) (*gitlab.CheckInfo, error) {
+	cfg, err := loadConfig(configPath)
+	if err != nil {
+		return nil, fmt.Errorf("load config: config_path %q: %w", configPath, err)
+	}
+
+	gitlabAPI, err := gitlab.NewHTTPClient(logrus.New(), cfg.Gitlab, cfg.TLS, prometheus.Config{})
+	if err != nil {
+		return nil, err
+	}
+
+	gitCmdFactory, cleanup, err := git.NewExecCommandFactory(cfg)
+	if err != nil {
+		return nil, err
+	}
+	defer cleanup()
+
+	return hook.NewManager(cfg, config.NewLocator(cfg), gitCmdFactory, nil, gitlabAPI).Check(context.Background())
+}
diff --git a/cmd/gitaly/check_test.go b/cmd/gitaly/check_test.go
new file mode 100644
index 000000000..01df6c00e
--- /dev/null
+++ b/cmd/gitaly/check_test.go
@@ -0,0 +1,119 @@
+package main
+
+import (
+	"bytes"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+
+	"github.com/pelletier/go-toml"
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
+)
+
+func TestMain(m *testing.M) {
+	testhelper.Run(m)
+}
+
+func TestCheckOK(t *testing.T) {
+	user, password := "user123", "password321"
+
+	c := gitlab.TestServerOptions{
+		User:                        user,
+		Password:                    password,
+		SecretToken:                 "",
+		GLRepository:                "",
+		Changes:                     "",
+		PostReceiveCounterDecreased: false,
+		Protocol:                    "ssh",
+	}
+	serverURL, cleanup := gitlab.NewTestServer(t, c)
+	defer cleanup()
+
+	cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{
+		Gitlab: config.Gitlab{
+			URL: serverURL,
+			HTTPSettings: config.HTTPSettings{
+				User:     user,
+				Password: password,
+			},
+			SecretFile: gitlab.WriteShellSecretFile(t, testhelper.TempDir(t), "the secret"),
+		},
+	}))
+	testcfg.BuildGitaly(t, cfg)
+
+	configPath := writeTemporaryGitalyConfigFile(t, cfg)
+
+	cmd := exec.Command(cfg.BinaryPath("gitaly"), "check", configPath)
+
+	var stderr, stdout bytes.Buffer
+	cmd.Stderr = &stderr
+	cmd.Stdout = &stdout
+
+	err := cmd.Run()
+	require.NoError(t, err)
+	require.Empty(t, stderr.String())
+
+	output := stdout.String()
+	require.Contains(t, output, "Checking GitLab API access: OK")
+	require.Contains(t, output, "Redis reachable for GitLab: true")
+}
+
+func TestCheckBadCreds(t *testing.T) {
+	user, password := "user123", "password321"
+
+	c := gitlab.TestServerOptions{
+		User:                        user,
+		Password:                    password,
+		SecretToken:                 "",
+		GLRepository:                "",
+		Changes:                     "",
+		PostReceiveCounterDecreased: false,
+		Protocol:                    "ssh",
+		GitPushOptions:              nil,
+	}
+	serverURL, cleanup := gitlab.NewTestServer(t, c)
+	defer cleanup()
+
+	cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{
+		Gitlab: config.Gitlab{
+			URL: serverURL,
+			HTTPSettings: config.HTTPSettings{
+				User:     "wrong",
+				Password: password,
+			},
+			SecretFile: gitlab.WriteShellSecretFile(t, testhelper.TempDir(t), "the secret"),
+		},
+	}))
+	testcfg.BuildGitaly(t, cfg)
+
+	configPath := writeTemporaryGitalyConfigFile(t, cfg)
+
+	cmd := exec.Command(cfg.BinaryPath("gitaly"), "check", configPath)
+
+	var stderr, stdout bytes.Buffer
+	cmd.Stderr = &stderr
+	cmd.Stdout = &stdout
+
+	require.Error(t, cmd.Run())
+	require.Contains(t, stderr.String(), "HTTP GET to GitLab endpoint /check failed: authorization failed")
+	require.Regexp(t, `Checking GitLab API access: .* level=error msg="Internal API error" .* error="authorization failed" method=GET status=401 url="http://127.0.0.1:[0-9]+/api/v4/internal/check"\nFAIL`, stdout.String())
+}
+
+// writeTemporaryGitalyConfigFile writes the given Gitaly configuration into a temporary file and
+// returns its path.
+func writeTemporaryGitalyConfigFile(t testing.TB, cfg config.Cfg) string {
+	t.Helper()
+
+	path := filepath.Join(testhelper.TempDir(t), "config.toml")
+
+	contents, err := toml.Marshal(cfg)
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(path, contents, 0o644))
+
+	return path
+}
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 793bb015e..0f04a2b5d 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -73,11 +73,17 @@ func loadConfig(configPath string) (config.Cfg, error) {
 
 func flagUsage() {
 	fmt.Println(version.GetVersionString())
-	fmt.Printf("Usage: %v [OPTIONS] configfile\n", os.Args[0])
+	fmt.Printf("Usage: %v [command] [options] <configfile>\n", os.Args[0])
 	flag.PrintDefaults()
+	fmt.Printf("\nThe commands are:\n\n\tcheck\tchecks accessability of internal Rails API\n")
 }
 
 func main() {
+	// If invoked with subcommand check
+	if len(os.Args) > 1 && os.Args[1] == "check" {
+		execCheck()
+	}
+
 	flag.Usage = flagUsage
 	flag.Parse()
 
diff --git a/doc/hooks.md b/doc/hooks.md
index 232273587..821367ca5 100644
--- a/doc/hooks.md
+++ b/doc/hooks.md
@@ -55,7 +55,6 @@ through gitaly.
 
 | subcommand     | purpose  | arguments | stdin  |
 |----------------|----------|-----------|--------|
-| `check`        | checks if the hooks can reach the gitlab server | none | none |
 | `pre-receive`  | used as the git pre-receive hook  none | `<old-value>` SP `<new-value>` SP `<ref-name>` LF |
 | `update`       | used as the git update hook | `<ref-name>` `<old-object>` `<new-object>` | none
 | `post-receive` | used as the git post-receive hook | none | `<old-value>` SP `<new-value>` SP `<ref-name>` LF |
diff --git a/internal/testhelper/testcfg/build.go b/internal/testhelper/testcfg/build.go
index d8a88f0df..ffacd17cb 100644
--- a/internal/testhelper/testcfg/build.go
+++ b/internal/testhelper/testcfg/build.go
@@ -50,6 +50,13 @@ func BuildPraefect(t testing.TB, cfg config.Cfg) string {
 	return buildGitalyCommand(t, cfg, "praefect")
 }
 
+// BuildGitaly builds the gitaly binary and installs it into the binary directory. The gitaly binary
+// embeds other binaries it needs to use when servicing requests. The packed binaries are not built
+// prior to building this gitaly binary and thus cannot be guaranteed to be from the same build.
+func BuildGitaly(t testing.TB, cfg config.Cfg) string {
+	return buildGitalyCommand(t, cfg, "gitaly")
+}
+
 // buildGitalyCommand builds an executable and places it in the correct directory depending
 // whether it is packed in the production build or not.
 func buildGitalyCommand(t testing.TB, cfg config.Cfg, executableName string) string {