Merge branch 'jc-fix-gitaly-hooks-check' into 'master'

Fix gitaly-hooks check command which was broken due to an incorrect implemention

Closes #2203

See merge request gitlab-org/gitaly!1761

6 files changed, 87 insertions, 65 deletions

diff --git a/changelogs/unreleased/jc-fix-gitaly-hooks-check.yml b/changelogs/unreleased/jc-fix-gitaly-hooks-check.yml
new file mode 100644
index 000000000..ee0c215d2
--- /dev/null
+++ b/changelogs/unreleased/jc-fix-gitaly-hooks-check.yml
@@ -0,0 +1,5 @@
+---
+title: Fix gitaly-hooks check command which was broken due to an incorrect implemention
+merge_request: 1761
+author:
+type: fixed
diff --git a/cmd/gitaly-hooks/gitaly_hooks.log b/cmd/gitaly-hooks/gitaly_hooks.log
deleted file mode 100644
index e69de29bb..000000000
--- a/cmd/gitaly-hooks/gitaly_hooks.log
+++ /dev/null
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index c526c1033..9f616520b 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -4,19 +4,20 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strings"
 
+	"github.com/BurntSushi/toml"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
-	"gitlab.com/gitlab-org/gitaly/internal/log"
-	"gopkg.in/yaml.v2"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/gitlabshell"
+	gitalylog "gitlab.com/gitlab-org/gitaly/internal/log"
 )
 
 func main() {
-	var logger = log.NewHookLogger()
+	var logger = gitalylog.NewHookLogger()
 
 	if len(os.Args) < 2 {
 		logger.Fatal(errors.New("requires hook name"))
@@ -27,21 +28,20 @@ func main() {
 	if subCmd == "check" {
 		configPath := os.Args[2]
 
-		if err := checkGitlabAccess(configPath); err != nil {
-			os.Stderr.WriteString(err.Error())
-			os.Exit(1)
+		status, err := check(configPath)
+		if err != nil {
+			log.Fatal(err)
 		}
 
-		os.Stdout.WriteString("OK")
-		os.Exit(0)
+		os.Exit(status)
 	}
 
-	gitlabRubyDir := os.Getenv("GITALY_RUBY_DIR")
-	if gitlabRubyDir == "" {
+	gitalyRubyDir := os.Getenv("GITALY_RUBY_DIR")
+	if gitalyRubyDir == "" {
 		logger.Fatal(errors.New("GITALY_RUBY_DIR not set"))
 	}
 
-	rubyHookPath := filepath.Join(gitlabRubyDir, "gitlab-shell", "hooks", subCmd)
+	rubyHookPath := filepath.Join(gitalyRubyDir, "gitlab-shell", "hooks", subCmd)
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -85,36 +85,31 @@ type HTTPSettings struct {
 	Password string `yaml:"password"`
 }
 
-func checkGitlabAccess(configPath string) error {
+func check(configPath string) (int, error) {
 	cfgFile, err := os.Open(configPath)
 	if err != nil {
-		return fmt.Errorf("error when opening config file: %v", err)
+		return 1, fmt.Errorf("error when opening config file: %v", err)
 	}
 	defer cfgFile.Close()
 
-	config := GitlabShellConfig{}
+	var c config.Cfg
 
-	if err := yaml.NewDecoder(cfgFile).Decode(&config); err != nil {
-		return fmt.Errorf("load toml: %v", err)
+	if _, err := toml.DecodeReader(cfgFile, &c); err != nil {
+		fmt.Println(err)
+		return 1, err
 	}
 
-	req, err := http.NewRequest("GET", fmt.Sprintf("%s/api/v4/internal/check", strings.TrimRight(config.GitlabURL, "/")), nil)
-	if err != nil {
-		return fmt.Errorf("could not create request for %s: %v", config.GitlabURL, err)
-	}
-
-	req.SetBasicAuth(config.HTTPSettings.User, config.HTTPSettings.Password)
-
-	client := &http.Client{}
+	cmd := exec.Command(filepath.Join(c.GitlabShell.Dir, "bin", "check"))
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	cmd.Env = append(os.Environ(), gitlabshell.EnvFromConfig(c)...)
 
-	resp, err := client.Do(req)
-	if err != nil {
-		return fmt.Errorf("error with request for %s: %v", config.GitlabURL, err)
-	}
-
-	if resp.StatusCode != 200 {
-		return fmt.Errorf("FAILED. code: %d", resp.StatusCode)
+	if err = cmd.Run(); err != nil {
+		if status, ok := command.ExitStatus(err); ok {
+			return status, nil
+		}
+		return 1, err
 	}
 
-	return nil
+	return 0, nil
 }
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index c7dbae128..c9689580f 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"bytes"
-	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -14,7 +13,6 @@ import (
 	"path"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -190,7 +188,19 @@ func TestCheckOK(t *testing.T) {
 		os.RemoveAll(tempDir)
 	}()
 
-	configPath := writeTemporaryConfigFile(t, tempDir, GitlabShellConfig{GitlabURL: ts.URL, HTTPSettings: HTTPSettings{User: user, Password: password}})
+	gitlabShellDir := filepath.Join(tempDir, "gitlab-shell")
+	binDir := filepath.Join(gitlabShellDir, "bin")
+	require.NoError(t, os.MkdirAll(gitlabShellDir, 0755))
+	require.NoError(t, os.MkdirAll(binDir, 0755))
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Symlink(filepath.Join(cwd, "../../ruby/gitlab-shell/bin/check"), filepath.Join(binDir, "check")))
+
+	writeShellSecretFile(t, gitlabShellDir, "the secret")
+	writeTemporaryConfigFile(t, gitlabShellDir, GitlabShellConfig{GitlabURL: ts.URL, HTTPSettings: HTTPSettings{User: user, Password: password}})
+
+	configPath, cleanup := writeTemporaryGitalyConfigFile(t, tempDir)
+	defer cleanup()
 
 	cmd := exec.Command(fmt.Sprintf("%s/gitaly-hooks", config.Config.BinDir), "check", configPath)
 
@@ -200,7 +210,8 @@ func TestCheckOK(t *testing.T) {
 
 	require.NoError(t, cmd.Run())
 	require.Empty(t, stderr.String())
-	require.Equal(t, "OK", stdout.String())
+	expectedCheckOutput := "Check GitLab API access: OK\nRedis available via internal API: OK\n"
+	require.Equal(t, expectedCheckOutput, stdout.String())
 }
 
 func TestCheckBadCreds(t *testing.T) {
@@ -215,7 +226,19 @@ func TestCheckBadCreds(t *testing.T) {
 		os.RemoveAll(tempDir)
 	}()
 
-	configPath := writeTemporaryConfigFile(t, tempDir, GitlabShellConfig{GitlabURL: ts.URL, HTTPSettings: HTTPSettings{User: user + "wrong", Password: password}})
+	gitlabShellDir := filepath.Join(tempDir, "gitlab-shell")
+	binDir := filepath.Join(gitlabShellDir, "bin")
+	require.NoError(t, os.MkdirAll(gitlabShellDir, 0755))
+	require.NoError(t, os.MkdirAll(binDir, 0755))
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Symlink(filepath.Join(cwd, "../../ruby/gitlab-shell/bin/check"), filepath.Join(binDir, "check")))
+
+	writeTemporaryConfigFile(t, gitlabShellDir, GitlabShellConfig{GitlabURL: ts.URL, HTTPSettings: HTTPSettings{User: user + "wrong", Password: password}})
+	writeShellSecretFile(t, gitlabShellDir, "the secret")
+
+	configPath, cleanup := writeTemporaryGitalyConfigFile(t, tempDir)
+	defer cleanup()
 
 	cmd := exec.Command(fmt.Sprintf("%s/gitaly-hooks", config.Config.BinDir), "check", configPath)
 
@@ -224,8 +247,8 @@ func TestCheckBadCreds(t *testing.T) {
 	cmd.Stdout = &stdout
 
 	require.Error(t, cmd.Run())
-	require.Equal(t, "FAILED. code: 401", stderr.String())
-	require.Empty(t, stdout.String())
+	require.Equal(t, "Check GitLab API access: ", stdout.String())
+	require.Equal(t, "FAILED. code: 401\n", stderr.String())
 }
 
 func handleAllowed(t *testing.T, secretToken string, key int, glRepository, changes string) func(w http.ResponseWriter, r *http.Request) {
@@ -280,21 +303,13 @@ func handlePostReceive(t *testing.T, secretToken string, key int, glRepository,
 
 func handleCheck(t *testing.T, user, password string) func(w http.ResponseWriter, r *http.Request) {
 	return func(w http.ResponseWriter, r *http.Request) {
-		auth := strings.SplitN(r.Header.Get("Authorization"), " ", 2)
-
-		if len(auth) != 2 || auth[0] != "Basic" {
+		u, p, ok := r.BasicAuth()
+		if !ok || u != user || p != password {
 			http.Error(w, "authorization failed", http.StatusUnauthorized)
 			return
 		}
 
-		payload, _ := base64.StdEncoding.DecodeString(auth[1])
-		pair := strings.SplitN(string(payload), ":", 2)
-
-		if pair[0] != user || pair[1] != password {
-			w.WriteHeader(http.StatusUnauthorized)
-			return
-		}
-
+		w.Write([]byte(`{"redis": true}`))
 		w.WriteHeader(http.StatusOK)
 	}
 }
@@ -332,6 +347,19 @@ func writeTemporaryConfigFile(t *testing.T, dir string, config GitlabShellConfig
 	return path
 }
 
+func writeTemporaryGitalyConfigFile(t *testing.T, tempDir string) (string, func()) {
+	path := filepath.Join(tempDir, "config.toml")
+	contents := fmt.Sprintf(`
+[gitlab-shell]
+  dir = "%s/gitlab-shell"
+`, tempDir)
+	require.NoError(t, ioutil.WriteFile(path, []byte(contents), 0644))
+
+	return path, func() {
+		os.RemoveAll(path)
+	}
+}
+
 func env(t *testing.T, glRepo, gitlabShellDir string, key int) []string {
 	rubyDir, err := filepath.Abs("../../ruby")
 	require.NoError(t, err)
diff --git a/internal/gitlabshell/env.go b/internal/gitlabshell/env.go
index 70e391ece..79bae0646 100644
--- a/internal/gitlabshell/env.go
+++ b/internal/gitlabshell/env.go
@@ -6,13 +6,18 @@ import "gitlab.com/gitlab-org/gitaly/internal/config"
 func Env() []string {
 	cfg := config.Config
 
+	return EnvFromConfig(cfg)
+}
+
+// EnvFromConfig returns a set of environment variables from a config struct relevant to gitlab shell
+func EnvFromConfig(cfg config.Cfg) []string {
 	return []string{
 		"GITALY_GITLAB_SHELL_DIR=" + cfg.GitlabShell.Dir,
 		"GITALY_LOG_DIR=" + cfg.Logging.Dir,
 		"GITALY_LOG_FORMAT=" + cfg.Logging.Format,
 		"GITALY_LOG_LEVEL=" + cfg.Logging.Level,
 		"GITLAB_SHELL_DIR=" + cfg.GitlabShell.Dir, //GITLAB_SHELL_DIR has been deprecated
-		"GITALY_BIN_DIR=" + config.Config.BinDir,
-		"GITALY_RUBY_DIR=" + config.Config.Ruby.Dir,
+		"GITALY_BIN_DIR=" + cfg.BinDir,
+		"GITALY_RUBY_DIR=" + cfg.Ruby.Dir,
 	}
 }
diff --git a/ruby/gitlab-shell/bin/check b/ruby/gitlab-shell/bin/check
index d2224a641..c8c0ae982 100755
--- a/ruby/gitlab-shell/bin/check
+++ b/ruby/gitlab-shell/bin/check
@@ -29,14 +29,3 @@ rescue GitlabNet::ApiUnreachableError
   abort "FAILED: Failed to connect to internal API"
 end
 
-config = GitlabConfig.new
-
-abort("ERROR: missing option in config.yml") unless config.auth_file
-
-print "\nAccess to #{config.auth_file}: "
-if system(File.dirname(__FILE__) + '/gitlab-keys', 'check-permissions')
-  print 'OK'
-else
-  abort "FAILED"
-end
-puts "\n"