diff --git a/_support/makegen.go b/_support/makegen.go index 3227f1240b214a6f1d5d5e1bcdd9988809cb143d..2ef9cfa42a7446516ccd24af73927896f9d38bc9 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 0000000000000000000000000000000000000000..d4ae0e2cc95512daae52fad3d422af50812a3a78 --- /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 801914937e9045ded579cd66291523b00c7070de..a05ce079c9354f895f72b0f1afa036a24b77cb5d 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 0000000000000000000000000000000000000000..139a7ed0bbbe470018da7b28b8b4e314cd4571a7 --- /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 156da2bbde0b270132458aa587f87e9453b0027d..fb711adda2c5c0b4570841a1edc146e0c7780313 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 8e1f2e54d4cedb6778d41f572a6e519d570f8c8b..c9670612f64bb266a053adba847d1303627fcb87 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 0000000000000000000000000000000000000000..a0b8873ed7eed07a508bb20d78fad0b3712e4b30 --- /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 0000000000000000000000000000000000000000..809a436c295642899d9c201935f36f6c896f2716 --- /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 3b75b665ee739b2114beeb86b771cbd5e1f386f9..475c6a087c9c0c12057520c816e781aaf4e641e6 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 8223b0ae80c14a4ad39504e656d4e656dad0b9e0..abb012bc16b444b1f9fc200c7de9fd50387dde30 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 a823b56cdcda2d8445a3a8893bf4281da2458d51..735464ac8ddf874eacb303f28d88f8c3cd003226 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 0000000000000000000000000000000000000000..2531be71f0552c28276c8e53b9a014c816572cd2 --- /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 8779ec18afb88c701ae38476c80e57a67531e3aa..086ef2fad01be98a519bdfe22abb136d9dbafebe 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 0e1458a7b2869887b75a56766bad2d7da9c29d29..8dc50567a7eb86384cc3be954dbd4e62c73f8b35 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 67f6030dd609302716d593f862f265c86311febc..37efaabed0f8c82a911e7a7ee04442aaf4491944 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 a9cd3fb0c8efb132e3935efa75085f66ec183f5b..0da647350dcc969d11a4a2408c6ba71959289757 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 a0e1742a64af60bb36d7e3f76cb24923f1ef1086..8d32924afe0f5c211bd214914fed1b2b7e672e50 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 222c93d3bb34f43ab19ea0bf697f152302c24748..a60f30001b656f89aebdcd514f5925edf5f9ff84 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 e419408a2289d8a459a2d56ef9a73cacfcc8d75c..c7de3caa13d868476d9078e8e43ede1019cc4f80 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 2e1551ba106285571ff4f72dc67d81c5e71d5f15..dd0fab005e995e840ddb037051601b2f98c46912 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