diff options
author | John Cai <jcai@gitlab.com> | 2019-06-06 02:10:58 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2019-06-27 22:32:18 +0300 |
commit | 7de57dbbf34a94b565bf8e7481bdb4944b3a344f (patch) | |
tree | 7baa03878b079d3265592671b45bf6e4de2282fd | |
parent | 8d52596e33bf98558399b8d86f870d741769e575 (diff) |
Pass down log config through env vars
-rw-r--r-- | _support/makegen.go | 14 | ||||
-rw-r--r-- | changelogs/unreleased/jc-remove-dependency-on-gitlab-shell-config.yml | 5 | ||||
-rw-r--r-- | config.toml.example | 2 | ||||
-rw-r--r-- | doc/configuration/logging.md | 41 | ||||
-rw-r--r-- | internal/config/config.go | 1 | ||||
-rw-r--r-- | internal/git/receivepack.go | 5 | ||||
-rw-r--r-- | internal/gitlabshell/env.go | 15 | ||||
-rw-r--r-- | internal/gitlabshell/env_test.go | 52 | ||||
-rw-r--r-- | internal/rubyserver/rubyserver.go | 6 | ||||
-rw-r--r-- | internal/service/smarthttp/receive_pack_test.go | 2 | ||||
-rw-r--r-- | internal/service/ssh/receive_pack_test.go | 2 | ||||
-rwxr-xr-x | ruby/gitlab-shell/bin/dump-config | 12 | ||||
-rw-r--r-- | ruby/gitlab-shell/lib/gitlab_config.rb | 54 | ||||
-rw-r--r-- | ruby/gitlab-shell/lib/gitlab_init.rb | 5 | ||||
-rw-r--r-- | ruby/gitlab-shell/lib/gitlab_logger.rb | 16 | ||||
-rw-r--r-- | ruby/gitlab-shell/spec/gitlab_logger_spec.rb | 14 | ||||
-rw-r--r-- | ruby/lib/gitlab/config.rb | 20 | ||||
-rw-r--r-- | ruby/lib/gitlab/git/hook.rb | 5 | ||||
-rw-r--r-- | ruby/spec/lib/gitlab/config_spec.rb | 2 | ||||
-rw-r--r-- | ruby/spec/lib/gitlab/git/hook_spec.rb | 2 |
20 files changed, 234 insertions, 41 deletions
diff --git a/_support/makegen.go b/_support/makegen.go index 3227f1240..2ef9cfa42 100644 --- a/_support/makegen.go +++ b/_support/makegen.go @@ -362,7 +362,7 @@ assemble-go: build .PHONY: assemble-ruby assemble-ruby: mkdir -p $(ASSEMBLY_ROOT) - rm -rf {{ .GitalyRubyDir }}/tmp {{ .GitlabShellDir }}/tmp + rm -rf {{ .GitalyRubyDir }}/tmp {{ .GitlabShellDir }}/tmp mkdir -p $(ASSEMBLY_ROOT)/ruby/ rsync -a --delete {{ .GitalyRubyDir }}/ $(ASSEMBLY_ROOT)/ruby/ rm -rf $(ASSEMBLY_ROOT)/ruby/spec $(ASSEMBLY_ROOT)/{{ .GitlabShellRelDir }}/spec $(ASSEMBLY_ROOT)/{{ .GitlabShellRelDir }}/gitlab-shell.log @@ -388,12 +388,15 @@ binaries: assemble git -C $@ fsck --no-progress .PHONY: prepare-tests -prepare-tests: {{ .TestRepo }} {{ .GitTestRepo }} ../.ruby-bundle +prepare-tests: {{ .GitlabShellDir }}/config.yml {{ .TestRepo }} {{ .GitTestRepo }} ../.ruby-bundle + +{{ .GitlabShellDir }}/config.yml: {{ .GitlabShellDir }}/config.yml.example + cp $< $@ .PHONY: test test: test-go rspec rspec-gitlab-shell -.PHONY: test-go +.PHONY: test-go test-go: prepare-tests @cd {{ .SourceDir }} && go test -tags "$(BUILD_TAGS)" -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching @@ -415,9 +418,6 @@ rspec-gitlab-shell: {{ .GitlabShellDir }}/config.yml assemble-go prepare-tests # rspec in {{ .GitlabShellRelDir }} @cd {{ .GitalyRubyDir }} && bundle exec bin/ruby-cd {{ .GitlabShellDir }} rspec -{{ .GitlabShellDir }}/config.yml: {{ .GitlabShellDir }}/config.yml.example - cp $< $@ - .PHONY: verify verify: check-mod-tidy lint check-formatting staticcheck notice-up-to-date govendor-tagged rubocop @@ -475,7 +475,7 @@ notice-tmp: {{ .GoVendor }} clean-ruby-vendor-go cd {{ .SourceDir }} && go mod vendor cd {{ .GopathSourceDir }} && env GOPATH={{ .BuildDir }} GO111MODULE=off govendor license -template _support/notice.template -o {{ .BuildDir }}/NOTICE -.PHONY: clean-ruby-vendor-go +.PHONY: clean-ruby-vendor-go clean-ruby-vendor-go: cd {{ .SourceDir }} && mkdir -p ruby/vendor && find ruby/vendor -type f -name '*.go' -delete diff --git a/changelogs/unreleased/jc-remove-dependency-on-gitlab-shell-config.yml b/changelogs/unreleased/jc-remove-dependency-on-gitlab-shell-config.yml new file mode 100644 index 000000000..d4ae0e2cc --- /dev/null +++ b/changelogs/unreleased/jc-remove-dependency-on-gitlab-shell-config.yml @@ -0,0 +1,5 @@ +--- +title: Pass down gitlab-shell log config through env vars +merge_request: 1293 +author: +type: other diff --git a/config.toml.example b/config.toml.example index 801914937..a05ce079c 100644 --- a/config.toml.example +++ b/config.toml.example @@ -39,6 +39,8 @@ path = "/home/git/repositories" # # You can optionally configure Gitaly to output JSON-formatted log messages to stdout # [logging] +# # The directory where Gitaly stores extra log files +dir = "/home/git/gitlab/log" # format = "json" # # Optional: Set log level to only log entries with that severity or above # # One of, in order: debug, info, warn, errror, fatal, panic diff --git a/doc/configuration/logging.md b/doc/configuration/logging.md new file mode 100644 index 000000000..139a7ed0b --- /dev/null +++ b/doc/configuration/logging.md @@ -0,0 +1,41 @@ +# Logging + +The following values configure logging in Gitaly under the `[logging]` section + +### dir + +While main gitaly application logs go to stdout, there are extra log files that go to a configured directory. These include: + +1. Gitlab shell logs + +### format + +The format of log messages. The current supported format is `json`. Any other value will be ignored and the default `text` format will be used. + +### level + +The log level. Valid values are the following: + +1. `trace` +1. `debug` +1. `info` +1. `warn` +1. `error` +1. `fatal` +1. `panic` + +The default log level is `info`. + +#### Gitlab Shell Exceptions + +Gitlab Shell does not spport `panic` or `trace`. `panic` will fall back to `error`, while `trace` will fall back to `debug`. Any other invalid log levels +will default to `info`. + +## Format + +```toml +[logging] +dir = "/home/gitaly/logs" +format = "json" +level = "info" +```
\ No newline at end of file diff --git a/internal/config/config.go b/internal/config/config.go index 156da2bbd..fb711adda 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -85,6 +85,7 @@ type Storage struct { // Logging contains the logging configuration for Gitaly type Logging struct { + Dir string `toml:"dir"` Format string `toml:"format"` SentryDSN string `toml:"sentry_dsn"` RubySentryDSN string `toml:"ruby_sentry_dsn"` diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go index 8e1f2e54d..c9670612f 100644 --- a/internal/git/receivepack.go +++ b/internal/git/receivepack.go @@ -5,6 +5,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git/hooks" + "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" ) // ReceivePackRequest abstracts away the different requests that end up @@ -18,12 +19,12 @@ type ReceivePackRequest interface { // HookEnv is information we pass down to the Git hooks during // git-receive-pack. func HookEnv(req ReceivePackRequest) []string { - return []string{ + return append([]string{ fmt.Sprintf("GL_ID=%s", req.GetGlId()), fmt.Sprintf("GL_USERNAME=%s", req.GetGlUsername()), fmt.Sprintf("GITLAB_SHELL_DIR=%s", config.Config.GitlabShell.Dir), fmt.Sprintf("GL_REPOSITORY=%s", req.GetGlRepository()), - } + }, gitlabshell.Env()...) } // ReceivePackConfig contains config options we want to enforce when diff --git a/internal/gitlabshell/env.go b/internal/gitlabshell/env.go new file mode 100644 index 000000000..a0b8873ed --- /dev/null +++ b/internal/gitlabshell/env.go @@ -0,0 +1,15 @@ +package gitlabshell + +import "gitlab.com/gitlab-org/gitaly/internal/config" + +// Env is a helper that returns a slice with environment variables used by gitlab shell +func Env() []string { + cfg := config.Config + + 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, + } +} diff --git a/internal/gitlabshell/env_test.go b/internal/gitlabshell/env_test.go new file mode 100644 index 000000000..809a436c2 --- /dev/null +++ b/internal/gitlabshell/env_test.go @@ -0,0 +1,52 @@ +package gitlabshell_test + +import ( + "bytes" + "encoding/json" + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +func TestGitHooksConfig(t *testing.T) { + testhelper.ConfigureRuby() + + defer func(cfg config.Cfg) { + config.Config = cfg + }(config.Config) + + loggingDir, err := ioutil.TempDir("", t.Name()) + require.NoError(t, err) + defer func() { os.RemoveAll(loggingDir) }() + + config.Config.Logging.Dir = loggingDir + config.Config.Logging.Level = "fatal" + config.Config.Logging.Format = "my-custom-format" + config.Config.GitlabShell.Dir = "../../ruby/gitlab-shell" + + dumpConfigPath := filepath.Join(config.Config.Ruby.Dir, "gitlab-shell", "bin", "dump-config") + + var stdout bytes.Buffer + + cmd := exec.Command(dumpConfigPath) + cmd.Env = append(os.Environ(), gitlabshell.Env()...) + cmd.Stdout = &stdout + + require.NoError(t, cmd.Run()) + + rubyConfigMap := make(map[string]interface{}) + + require.NoError(t, json.NewDecoder(&stdout).Decode(&rubyConfigMap)) + require.Equal(t, config.Config.Logging.Level, rubyConfigMap["log_level"]) + require.Equal(t, config.Config.Logging.Format, rubyConfigMap["log_format"]) + + dir := filepath.Dir(rubyConfigMap["log_file"].(string)) + require.Equal(t, config.Config.Logging.Dir, dir) +} diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go index 3b75b665e..475c6a087 100644 --- a/internal/rubyserver/rubyserver.go +++ b/internal/rubyserver/rubyserver.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git/hooks" + "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer" "gitlab.com/gitlab-org/gitaly/internal/supervisor" @@ -108,11 +109,10 @@ func Start() (*Server, error) { "GITALY_RUBY_GIT_BIN_PATH="+command.GitPath(), fmt.Sprintf("GITALY_RUBY_WRITE_BUFFER_SIZE=%d", streamio.WriteBufferSize), fmt.Sprintf("GITALY_RUBY_MAX_COMMIT_OR_TAG_MESSAGE_SIZE=%d", helper.MaxCommitOrTagMessageSize), - "GITALY_RUBY_GITLAB_SHELL_PATH="+cfg.GitlabShell.Dir, "GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir, "GITALY_VERSION="+version.GetVersion(), - "GITALY_GIT_HOOKS_DIR="+hooks.Path(), - ) + "GITALY_GIT_HOOKS_DIR="+hooks.Path()) + env = append(env, gitlabshell.Env()...) env = append(env, command.GitEnv...) diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go index 8223b0ae8..abb012bc1 100644 --- a/internal/service/smarthttp/receive_pack_test.go +++ b/internal/service/smarthttp/receive_pack_test.go @@ -62,7 +62,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) { "GL_ID=user-123", "GL_REPOSITORY=project-456", "GL_PROTOCOL=http", - "GITLAB_SHELL_DIR=" + "/foo/bar/gitlab-shell", + "GITALY_GITLAB_SHELL_DIR=" + "/foo/bar/gitlab-shell", } { require.Contains(t, strings.Split(string(envData), "\n"), env) } diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go index a823b56cd..735464ac8 100644 --- a/internal/service/ssh/receive_pack_test.go +++ b/internal/service/ssh/receive_pack_test.go @@ -99,7 +99,7 @@ func TestReceivePackPushSuccess(t *testing.T) { "GL_ID=user-123", "GL_REPOSITORY=project-456", "GL_PROTOCOL=ssh", - "GITLAB_SHELL_DIR=" + "/foo/bar/gitlab-shell", + "GITALY_GITLAB_SHELL_DIR=" + "/foo/bar/gitlab-shell", } { require.Contains(t, strings.Split(string(envData), "\n"), env) } diff --git a/ruby/gitlab-shell/bin/dump-config b/ruby/gitlab-shell/bin/dump-config new file mode 100755 index 000000000..2531be71f --- /dev/null +++ b/ruby/gitlab-shell/bin/dump-config @@ -0,0 +1,12 @@ +#!/usr/bin/env ruby + +# only used for tests +require 'json' + +require_relative '../lib/gitlab_init' +require_relative '../lib/gitlab_config' +require_relative '../lib/gitlab_logger' + +gitlab_config = GitlabConfig.new + +puts gitlab_config.to_json diff --git a/ruby/gitlab-shell/lib/gitlab_config.rb b/ruby/gitlab-shell/lib/gitlab_config.rb index 8779ec18a..086ef2fad 100644 --- a/ruby/gitlab-shell/lib/gitlab_config.rb +++ b/ruby/gitlab-shell/lib/gitlab_config.rb @@ -1,44 +1,68 @@ require 'yaml' class GitlabConfig - attr_reader :config - - def initialize - @config = YAML.load_file(File.join(ROOT_PATH, 'config.yml')) - end - def secret_file - @config['secret_file'] ||= File.join(ROOT_PATH, '.gitlab_shell_secret') + fetch_from_legacy_config('secret_file',File.join(ROOT_PATH, '.gitlab_shell_secret')) end # Pass a default value because this is called from a repo's context; in which # case, the repo's hooks directory should be the default. # - def custom_hooks_dir - @config['custom_hooks_dir'] || File.join(ROOT_PATH, 'hooks') + def custom_hooks_dir(default: nil) + fetch_from_legacy_config('custom_hooks_dir', File.join(ROOT_PATH, 'hooks')) end def gitlab_url - (@config['gitlab_url'] ||= "http://localhost:8080").sub(%r{/*$}, '') + fetch_from_legacy_config('gitlab_url',"http://localhost:8080").sub(%r{/*$}, '') end def http_settings - @config['http_settings'] ||= {} + fetch_from_legacy_config('http_settings', {}) end def log_file - @config['log_file'] ||= File.join(ROOT_PATH, 'gitlab-shell.log') + return File.join(LOG_PATH, 'gitlab-shell.log') unless LOG_PATH.empty? + + fetch_from_legacy_config('log_file', File.join(ROOT_PATH, 'gitlab-shell.log')) end def log_level - @config['log_level'] ||= 'INFO' + return LOG_LEVEL unless LOG_LEVEL.empty? + + fetch_from_legacy_config('log_level', 'INFO') end def log_format - @config['log_format'] ||= 'text' + return LOG_FORMAT unless LOG_FORMAT.empty? + + fetch_from_legacy_config('log_format', 'text') end def metrics_log_file - @config['metrics_log_file'] ||= File.join(ROOT_PATH, 'gitlab-shell-metrics.log') + fetch_from_legacy_config('metrics_log_file', File.join(ROOT_PATH, 'gitlab-shell-metrics.log')) + end + + def to_json + { + secret_file: secret_file, + custom_hooks_dir: custom_hooks_dir, + gitlab_url: gitlab_url, + http_settings: http_settings, + log_file: log_file, + log_level: log_level, + log_format: log_format, + metrics_log_file: metrics_log_file + }.to_json + end + + def fetch_from_legacy_config(key, default) + legacy_config.fetch(key, default) + end + + private + + def legacy_config + # TODO: deprecate @legacy_config that is parsing the gitlab-shell config.yml + @legacy_config ||= YAML.load_file(File.join(ROOT_PATH, 'config.yml')) end end diff --git a/ruby/gitlab-shell/lib/gitlab_init.rb b/ruby/gitlab-shell/lib/gitlab_init.rb index 0e1458a7b..8dc50567a 100644 --- a/ruby/gitlab-shell/lib/gitlab_init.rb +++ b/ruby/gitlab-shell/lib/gitlab_init.rb @@ -1,4 +1,7 @@ -ROOT_PATH = ENV.fetch('GITLAB_SHELL_DIR', File.expand_path('..', __dir__)) +ROOT_PATH = ENV.fetch('GITALY_GITLAB_SHELL_DIR', File.expand_path('..', __dir__)) +LOG_PATH = ENV.fetch('GITALY_LOG_DIR', "") +LOG_LEVEL = ENV.fetch('GITALY_LOG_LEVEL', "") +LOG_FORMAT = ENV.fetch('GITALY_LOG_FORMAT', "") # We are transitioning parts of gitlab-shell into the gitaly project. In # gitaly, GITALY_EMBEDDED will be true. diff --git a/ruby/gitlab-shell/lib/gitlab_logger.rb b/ruby/gitlab-shell/lib/gitlab_logger.rb index 67f6030dd..37efaabed 100644 --- a/ruby/gitlab-shell/lib/gitlab_logger.rb +++ b/ruby/gitlab-shell/lib/gitlab_logger.rb @@ -8,8 +8,20 @@ def convert_log_level(log_level) Logger.const_get(log_level.upcase) rescue NameError $stderr.puts "WARNING: Unrecognized log level #{log_level.inspect}." - $stderr.puts "WARNING: Falling back to INFO." - Logger::INFO + + fallback_level = Logger::INFO + + case log_level + when 'trace' + fallback_level = Logger::DEBUG + when 'panic' + fallback_level = Logger::ERROR + end + + fallback_level_name = Logger.constants.detect{ |c| Logger.const_get(c) == fallback_level} + $stderr.puts "WARNING: Falling back to #{fallback_level_name}" + + fallback_level end class GitlabLogger diff --git a/ruby/gitlab-shell/spec/gitlab_logger_spec.rb b/ruby/gitlab-shell/spec/gitlab_logger_spec.rb index a9cd3fb0c..0da647350 100644 --- a/ruby/gitlab-shell/spec/gitlab_logger_spec.rb +++ b/ruby/gitlab-shell/spec/gitlab_logger_spec.rb @@ -3,11 +3,15 @@ require_relative '../lib/gitlab_logger' require 'securerandom' describe :convert_log_level do - subject { convert_log_level :extreme } - - it "converts invalid log level to Logger::INFO" do - expect($stderr).to receive(:puts).at_least(:once) - is_expected.to eq(Logger::INFO) + [ + ['extreme', Logger::INFO], + ['panic', Logger::ERROR], + ['trace', Logger::DEBUG] + ].each do |level, expected| + it "converts invalid log level to Logger::INFO" do + expect($stderr).to receive(:puts).at_least(:once) + expect(convert_log_level(level)).to eq(expected) + end end end diff --git a/ruby/lib/gitlab/config.rb b/ruby/lib/gitlab/config.rb index a0e1742a6..8d32924af 100644 --- a/ruby/lib/gitlab/config.rb +++ b/ruby/lib/gitlab/config.rb @@ -41,7 +41,7 @@ module Gitlab include TestSetup def path - @path ||= ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] + @path ||= ENV['GITALY_GITLAB_SHELL_DIR'] end def git_timeout @@ -69,6 +69,20 @@ module Gitlab end end + class Logging + def dir + @dir ||= ENV['GITALY_LOG_DIR'] + end + + def level + @level ||= ENV['GITALY_LOG_LEVEL'] + end + + def format + @format ||= ENV['GITALY_LOG_FORMAT'] + end + end + def git @git ||= Git.new end @@ -77,6 +91,10 @@ module Gitlab @gitlab_shell ||= GitlabShell.new end + def logging + @logging ||= Logging.new + end + def gitaly @gitaly ||= Gitaly.new end diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 222c93d3b..a60f30001 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -102,7 +102,10 @@ module Gitlab def env_base_vars(gl_id, gl_username) { - 'GITLAB_SHELL_DIR' => Gitlab.config.gitlab_shell.path, + 'GITALY_GITLAB_SHELL_DIR' => Gitlab.config.gitlab_shell.path, + 'GITALY_LOG_DIR' => Gitlab.config.logging.dir, + 'GITALY_LOG_LEVEL' => Gitlab.config.logging.level, + 'GITALY_LOG_FORMAT' => Gitlab.config.logging.format, 'GL_ID' => gl_id, 'GL_USERNAME' => gl_username, 'GL_REPOSITORY' => repository.gl_repository, diff --git a/ruby/spec/lib/gitlab/config_spec.rb b/ruby/spec/lib/gitlab/config_spec.rb index e419408a2..c7de3caa1 100644 --- a/ruby/spec/lib/gitlab/config_spec.rb +++ b/ruby/spec/lib/gitlab/config_spec.rb @@ -7,7 +7,7 @@ describe Gitlab::Config do let(:gitlab_shell_path) { '/foo/bar/gitlab-shell' } before do - allow(ENV).to receive(:[]).with('GITALY_RUBY_GITLAB_SHELL_PATH').and_return(gitlab_shell_path) + allow(ENV).to receive(:[]).with('GITALY_GITLAB_SHELL_DIR').and_return(gitlab_shell_path) end it { expect(subject.path).to eq(gitlab_shell_path) } diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb index 2e1551ba1..dd0fab005 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -38,7 +38,7 @@ describe Gitlab::Git::Hook do 'GL_PROTOCOL' => 'web', 'PWD' => repo.path, 'GIT_DIR' => repo.path, - 'GITLAB_SHELL_DIR' => '/foobar/gitlab-shell' + 'GITALY_GITLAB_SHELL_DIR' => '/foobar/gitlab-shell' } end |