Merge branch 'jc-remove-dependency-on-gitlab-shell-config' into 'master'

Pass down log config through env vars

Closes #1621

See merge request gitlab-org/gitaly!1293

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