From 1ed9d219191d19125defe1f6c0e5455e6dacdb8d Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 13 Oct 2020 12:23:35 +0200 Subject: hooks: Check command ported to Go GitLab-Shell has `bin/check` as a small utility to check the API access required for the hooks. These hooks are now managed by Gitaly, and gitaly-hooks has a subcommand `check` too. This subcommend used to invoke `bin/check` from the vendored `gitlab-shell`. This change ports the logic to Go, and removes `bin/check`. It depends on the same config values that the hooks depend on, which wasn't always true, but usually was just because of Omnibus configuration reconfiguration. --- changelogs/unreleased/zj-shell-check-go.yml | 5 +++ cmd/gitaly-hooks/hooks.go | 46 +++++++++++-------------- cmd/gitaly-hooks/hooks_test.go | 40 ++++++++-------------- internal/gitaly/hook/access.go | 14 ++++++-- internal/gitaly/hook/check.go | 53 +++++++++++++++++++++++++++++ internal/testhelper/testserver.go | 12 +++---- ruby/gitlab-shell/bin/check | 31 ----------------- 7 files changed, 110 insertions(+), 91 deletions(-) create mode 100644 changelogs/unreleased/zj-shell-check-go.yml create mode 100644 internal/gitaly/hook/check.go delete mode 100755 ruby/gitlab-shell/bin/check diff --git a/changelogs/unreleased/zj-shell-check-go.yml b/changelogs/unreleased/zj-shell-check-go.yml new file mode 100644 index 000000000..df8787f57 --- /dev/null +++ b/changelogs/unreleased/zj-shell-check-go.yml @@ -0,0 +1,5 @@ +--- +title: 'hooks: Check command ported to Go' +merge_request: 2650 +author: +type: changed diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go index 6b9173a14..25efc1b88 100644 --- a/cmd/gitaly-hooks/hooks.go +++ b/cmd/gitaly-hooks/hooks.go @@ -7,18 +7,15 @@ import ( "io" "log" "os" - "os/exec" - "path/filepath" "strconv" "strings" "github.com/golang/protobuf/jsonpb" - "github.com/pelletier/go-toml" gitalyauth "gitlab.com/gitlab-org/gitaly/auth" "gitlab.com/gitlab-org/gitaly/client" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" - "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/hook" gitalylog "gitlab.com/gitlab-org/gitaly/internal/log" "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata" @@ -69,12 +66,21 @@ func main() { if subCmd == "check" { configPath := fixedArgs[2] - status, err := check(configPath) + fmt.Print("Checking GitLab API access: ") + + info, err := check(configPath) if err != nil { + fmt.Print("FAIL\n") log.Fatal(err) } - os.Exit(status) + 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") + os.Exit(0) } ctx, cancel := context.WithCancel(context.Background()) @@ -337,35 +343,21 @@ func sendFunc(reqWriter io.Writer, stream grpc.ClientStream, stdin io.Reader) fu } } -func check(configPath string) (int, error) { +func check(configPath string) (*hook.CheckInfo, error) { cfgFile, err := os.Open(configPath) if err != nil { - return 1, fmt.Errorf("failed to open config file: %w", err) + return nil, fmt.Errorf("failed to open config file: %w", err) } defer cfgFile.Close() - var c config.Cfg - - if err := toml.NewDecoder(cfgFile).Decode(&c); err != nil { - return 1, fmt.Errorf("failed to decode toml: %w", err) + if err := config.Load(cfgFile); err != nil { + return nil, err } - cmd := exec.Command(filepath.Join(c.GitlabShell.Dir, "bin", "check")) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - gitlabshellEnv, err := gitlabshell.EnvFromConfig(c) + gitlabAPI, err := hook.NewGitlabAPI(config.Config.Gitlab) if err != nil { - return 1, err - } - - cmd.Env = append(os.Environ(), gitlabshellEnv...) - - if err = cmd.Run(); err != nil { - if status, ok := command.ExitStatus(err); ok { - return status, nil - } - return 1, fmt.Errorf("failed to run %q: %w", cmd.String(), err) + return nil, err } - return 0, nil + return hook.NewManager(gitlabAPI, config.Config).Check(context.TODO()) } diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 5e219e983..a418da6e2 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -8,6 +8,7 @@ import ( "log" "os" "os/exec" + "path" "path/filepath" "regexp" "strings" @@ -622,30 +623,25 @@ func TestCheckOK(t *testing.T) { }() 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"))) testhelper.WriteShellSecretFile(t, gitlabShellDir, "the secret") - testhelper.WriteTemporaryGitlabShellConfigFile(t, - gitlabShellDir, - testhelper.GitlabShellConfig{GitlabURL: serverURL, HTTPSettings: testhelper.HTTPSettings{User: user, Password: password}}) - - configPath, cleanup := testhelper.WriteTemporaryGitalyConfigFile(t, tempDir, serverURL, user, password) + configPath, cleanup := testhelper.WriteTemporaryGitalyConfigFile(t, tempDir, serverURL, user, password, path.Join(gitlabShellDir, ".gitlab_shell_secret")) defer cleanup() + cmd := exec.Command(fmt.Sprintf("%s/gitaly-hooks", config.Config.BinDir), "check", configPath) var stderr, stdout bytes.Buffer cmd.Stderr = &stderr cmd.Stdout = &stdout - require.NoError(t, cmd.Run()) - require.Empty(t, stderr.String()) - expectedCheckOutput := "Check GitLab API access: OK\nRedis available via internal API: OK\n" - require.Equal(t, expectedCheckOutput, stdout.String()) + err = cmd.Run() + require.NoError(t, err) + require.Contains(t, stderr.String(), "status=200") + + 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) { @@ -671,28 +667,22 @@ func TestCheckBadCreds(t *testing.T) { }() 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"))) - - testhelper.WriteTemporaryGitlabShellConfigFile(t, gitlabShellDir, testhelper.GitlabShellConfig{GitlabURL: serverURL, HTTPSettings: testhelper.HTTPSettings{User: user + "wrong", Password: password}}) testhelper.WriteShellSecretFile(t, gitlabShellDir, "the secret") - configPath, cleanup := testhelper.WriteTemporaryGitalyConfigFile(t, tempDir, serverURL, "wrong", password) + config.Config.Gitlab.URL = serverURL + configPath, cleanup := testhelper.WriteTemporaryGitalyConfigFile(t, tempDir, serverURL, "wrong", password, path.Join(gitlabShellDir, ".gitlab_shell_secret")) defer cleanup() - cmd := exec.Command(fmt.Sprintf("%s/gitaly-hooks", config.Config.BinDir), "check", configPath) + cmd := exec.Command(filepath.Join(config.Config.BinDir, "gitaly-hooks"), "check", configPath) var stderr, stdout bytes.Buffer cmd.Stderr = &stderr cmd.Stdout = &stdout require.Error(t, cmd.Run()) - require.Equal(t, "Check GitLab API access: ", stdout.String()) - require.Equal(t, "FAILED. code: 401\n", stderr.String()) + require.Contains(t, stderr.String(), "Internal API error (401)") + require.Equal(t, "Checking GitLab API access: FAIL\n", stdout.String()) } func runHookServiceServer(t *testing.T, token string) (string, func()) { diff --git a/internal/gitaly/hook/access.go b/internal/gitaly/hook/access.go index f3bf5f1ad..c47e68261 100644 --- a/internal/gitaly/hook/access.go +++ b/internal/gitaly/hook/access.go @@ -56,6 +56,8 @@ func marshallGitObjectDirs(gitObjectDirRel string, gitAltObjectDirsRel []string) type GitlabAPI interface { // Allowed queries the gitlab internal api /allowed endpoint to determine if a ref change for a given repository and user is allowed Allowed(ctx context.Context, repo *gitalypb.Repository, glRepository, glID, glProtocol, changes string) (bool, string, error) + // Check verifies that GitLab can be reached, and authenticated to + Check(ctx context.Context) (*CheckInfo, error) // PreReceive queries the gitlab internal api /pre_receive to increase the reference counter PreReceive(ctx context.Context, glRepository string) (bool, error) // PostReceive queries the gitlab internal api /post_receive to decrease the reference counter @@ -279,13 +281,21 @@ func (a *AllowedRequest) parseAndSetGLID(glID string) error { } // mockAPI is a noop gitlab API client -type mockAPI struct { -} +type mockAPI struct{} func (m *mockAPI) Allowed(ctx context.Context, repo *gitalypb.Repository, glRepository, glID, glProtocol, changes string) (bool, string, error) { return true, "", nil } +func (m *mockAPI) Check(ctx context.Context) (*CheckInfo, error) { + return &CheckInfo{ + Version: "v13.5.0", + Revision: "deadbeef", + APIVersion: "v4", + RedisReachable: true, + }, nil +} + func (m *mockAPI) PreReceive(ctx context.Context, glRepository string) (bool, error) { return true, nil } diff --git a/internal/gitaly/hook/check.go b/internal/gitaly/hook/check.go new file mode 100644 index 000000000..97b963be4 --- /dev/null +++ b/internal/gitaly/hook/check.go @@ -0,0 +1,53 @@ +package hook + +import ( + "context" + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" +) + +// CheckInfo represents the response of GitLabs `check` API endpoint +type CheckInfo struct { + // GitLab Server version + Version string `json:"gitlab_version"` + // Revision of the Git object of the running GitLab + Revision string `json:"gitlab_revision"` + // The version of the API, expected to be v4 + APIVersion string `json:"api_version"` + // GitLab needs a working Redis, even if the check result is successful + // GitLab might still not be able to handle hook API calls without it + RedisReachable bool `json:"redis"` +} + +// Check performs an HTTP request to the internal/check API endpoint to verify +// the connection and tokens. It returns basic information of the installed +// GitLab +func (a *gitlabAPI) Check(ctx context.Context) (*CheckInfo, error) { + resp, err := a.client.Get(ctx, "/check") + if err != nil { + return nil, fmt.Errorf("HTTP GET to GitLab endpoint /check failed: %w", err) + } + + defer func() { + io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + }() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("Check HTTP request failed with status: %d", resp.StatusCode) + } + + var info CheckInfo + if err := json.NewDecoder(resp.Body).Decode(&info); err != nil { + return nil, fmt.Errorf("failed to decode response from /check endpoint: %w", err) + } + + return &info, nil +} + +func (m *GitLabHookManager) Check(ctx context.Context) (*CheckInfo, error) { + return m.gitlabAPI.Check(ctx) +} diff --git a/internal/testhelper/testserver.go b/internal/testhelper/testserver.go index ce2e85f82..741b69870 100644 --- a/internal/testhelper/testserver.go +++ b/internal/testhelper/testserver.go @@ -874,16 +874,16 @@ func WriteTemporaryGitlabShellConfigFile(t FatalLogger, dir string, config Gitla // WriteTemporaryGitalyConfigFile writes a gitaly toml file into a temporary directory. It returns the path to // the file as well as a cleanup function -func WriteTemporaryGitalyConfigFile(t testing.TB, tempDir, gitlabURL, user, password string) (string, func()) { +func WriteTemporaryGitalyConfigFile(t testing.TB, tempDir, gitlabURL, user, password, secretFile string) (string, func()) { path := filepath.Join(tempDir, "config.toml") contents := fmt.Sprintf(` -[gitlab-shell] - dir = "%s/gitlab-shell" - gitlab_url = %q - [gitlab-shell.http-settings] +[gitlab] + url = "%s" + secret_file = "%s" + [gitlab.http-settings] user = %q password = %q -`, tempDir, gitlabURL, user, password) +`, gitlabURL, secretFile, user, password) require.NoError(t, ioutil.WriteFile(path, []byte(contents), 0644)) return path, func() { diff --git a/ruby/gitlab-shell/bin/check b/ruby/gitlab-shell/bin/check deleted file mode 100755 index c8c0ae982..000000000 --- a/ruby/gitlab-shell/bin/check +++ /dev/null @@ -1,31 +0,0 @@ -#!/usr/bin/env ruby - -require_relative '../lib/gitlab_init' -require_relative '../lib/gitlab_net' - -# -# GitLab shell check task -# - -print "Check GitLab API access: " -begin - resp = GitlabNet.new.check - - if resp.code != "200" - abort "FAILED. code: #{resp.code}" - end - - puts 'OK' - - check_values = JSON.parse(resp.body) - - print 'Redis available via internal API: ' - if check_values['redis'] - puts 'OK' - else - abort 'FAILED' - end -rescue GitlabNet::ApiUnreachableError - abort "FAILED: Failed to connect to internal API" -end - -- cgit v1.2.3