Merge branch 'zj-cleanup-exec' into 'master'

Cleanup `git-upload-*` and `git-receive-*` related code

Closes gitaly#1300

See merge request gitlab-org/gitlab-shell!232

4 files changed, 52 insertions, 223 deletions

diff --git a/config.yml.example b/config.yml.example
index c616254..23743fd 100644
--- a/config.yml.example
+++ b/config.yml.example
@@ -49,10 +49,3 @@ log_level: INFO
 # Set to true to see real usernames in the logs instead of key ids, which is easier to follow, but
 # incurs an extra API call on every gitlab-shell command.
 audit_usernames: false
-
-# Git trace log file.
-# If set, git commands receive GIT_TRACE* environment variables
-# See https://git-scm.com/book/es/v2/Git-Internals-Environment-Variables#Debugging for documentation
-# An absolute path starting with / – the trace output will be appended to that file.
-# It needs to exist so we can check permissions and avoid to throwing warnings to the users.
-git_trace_log_file:
diff --git a/lib/gitlab_config.rb b/lib/gitlab_config.rb
index 7645989..85aa889 100644
--- a/lib/gitlab_config.rb
+++ b/lib/gitlab_config.rb
@@ -50,10 +50,6 @@ class GitlabConfig
     @config['audit_usernames'] ||= false
   end
 
-  def git_trace_log_file
-    @config['git_trace_log_file']
-  end
-
   def metrics_log_file
     @config['metrics_log_file'] ||= File.join(ROOT_PATH, 'gitlab-shell-metrics.log')
   end
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index 4cabd37..4d0b26f 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'shellwords'
 require 'pathname'
 
@@ -9,14 +11,15 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   class DisallowedCommandError < StandardError; end
   class InvalidRepositoryPathError < StandardError; end
 
-  GIT_COMMANDS = %w(git-upload-pack git-receive-pack git-upload-archive git-lfs-authenticate).freeze
-  GITALY_MIGRATED_COMMANDS = {
+  GITALY_COMMANDS = {
     'git-upload-pack' => File.join(ROOT_PATH, 'bin', 'gitaly-upload-pack'),
     'git-upload-archive' => File.join(ROOT_PATH, 'bin', 'gitaly-upload-archive'),
     'git-receive-pack' => File.join(ROOT_PATH, 'bin', 'gitaly-receive-pack')
   }.freeze
+
+  GIT_COMMANDS = (GITALY_COMMANDS.keys + ['git-lfs-authenticate']).freeze
   API_COMMANDS = %w(2fa_recovery_codes).freeze
-  GL_PROTOCOL = 'ssh'.freeze
+  GL_PROTOCOL = 'ssh'
 
   attr_accessor :gl_id, :gl_repository, :repo_name, :command, :git_access, :git_protocol
 
@@ -136,64 +139,31 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       return
     end
 
-    executable = @command
-    args = []
-
-    if GITALY_MIGRATED_COMMANDS.key?(executable) && @gitaly
-      executable = GITALY_MIGRATED_COMMANDS[executable]
-
-      gitaly_address = @gitaly['address']
-
-      # The entire gitaly_request hash should be built in gitlab-ce and passed
-      # on as-is. For now we build a fake one on the spot.
-      gitaly_request = {
-        'repository' => @gitaly['repository'],
-        'gl_repository' => @gl_repository,
-        'gl_id' => @gl_id,
-        'gl_username' => @username,
-        'git_config_options' => @git_config_options,
-        'git_protocol' => @git_protocol
-      }
-
-      args = [gitaly_address, JSON.dump(gitaly_request)]
-    end
+    # TODO: instead of building from pieces here in gitlab-shell, build the
+    # entire gitaly_request in gitlab-ce and pass on as-is here.
+    args = JSON.dump(
+      'repository' => @gitaly['repository'],
+      'gl_repository' => @gl_repository,
+      'gl_id' => @gl_id,
+      'gl_username' => @username,
+      'git_config_options' => @git_config_options,
+      'git_protocol' => @git_protocol
+    )
 
-    args_string = [File.basename(executable), *args].join(' ')
+    gitaly_address = @gitaly['address']
+    executable = GITALY_COMMANDS.fetch(@command)
+    gitaly_bin = File.basename(executable)
+    args_string = [gitaly_bin, gitaly_address, args].join(' ')
     $logger.info('executing git command', command: args_string, user: log_username)
-    exec_cmd(executable, *args)
+
+    exec_cmd(executable, gitaly_address: gitaly_address, token: @gitaly['token'], json_args: args)
   end
 
   # This method is not covered by Rspec because it ends the current Ruby process.
-  def exec_cmd(*args)
-    # If you want to call a command without arguments, use
-    # exec_cmd(['my_command', 'my_command']) . Otherwise use
-    # exec_cmd('my_command', 'my_argument', ...).
-    if args.count == 1 && !args.first.is_a?(Array)
-      raise DisallowedCommandError
-    end
-
-    env = {
-      'HOME' => ENV['HOME'],
-      'PATH' => ENV['PATH'],
-      'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
-      'LANG' => ENV['LANG'],
-      'GL_ID' => @gl_id,
-      'GL_PROTOCOL' => GL_PROTOCOL,
-      'GL_REPOSITORY' => @gl_repository,
-      'GL_USERNAME' => @username
-    }
-    if @gitaly && @gitaly.include?('token')
-      env['GITALY_TOKEN'] = @gitaly['token']
-    end
-
-    if git_trace_available?
-      env.merge!(
-        'GIT_TRACE' => @config.git_trace_log_file,
-        'GIT_TRACE_PACKET' => @config.git_trace_log_file,
-        'GIT_TRACE_PERFORMANCE' => @config.git_trace_log_file
-      )
-    end
+  def exec_cmd(executable, gitaly_address:, token:, json_args:)
+    env = { 'GITALY_TOKEN' => token }
 
+    args = [executable, gitaly_address, json_args]
     # We use 'chdir: ROOT_PATH' to let the next executable know where config.yml is.
     Kernel.exec(env, *args, unsetenv_others: true, chdir: ROOT_PATH)
   end
@@ -274,21 +244,4 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
            "#{resp['message']}"
     end
   end
-
-  def git_trace_available?
-    return false unless @config.git_trace_log_file
-
-    if Pathname(@config.git_trace_log_file).relative?
-      $logger.warn('git trace log path must be absolute, ignoring', git_trace_log_file: @config.git_trace_log_file)
-      return false
-    end
-
-    begin
-      File.open(@config.git_trace_log_file, 'a') { nil }
-      return true
-    rescue => ex
-      $logger.warn('Failed to open git trace log file', git_trace_log_file: @config.git_trace_log_file, error: ex.to_s)
-      return false
-    end
-  end
 end
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 96c4878..135a474 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -201,19 +201,13 @@ describe GitlabShell do
       end
     end
 
-    context 'git-upload-pack' do
-      it_behaves_like 'upload-pack', 'git-upload-pack'
-    end
-
-    context 'git upload-pack' do
-      it_behaves_like 'upload-pack', 'git upload-pack'
-    end
-
     context 'gitaly-upload-pack' do
       let(:ssh_cmd) { "git-upload-pack gitlab-ci.git" }
+
       before do
         allow(api).to receive(:check_access).and_return(gitaly_check_access)
       end
+
       after { subject.exec(ssh_cmd) }
 
       it "should process the command" do
@@ -221,12 +215,13 @@ describe GitlabShell do
       end
 
       it "should execute the command" do
-        expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-upload-pack"), 'unix:gitaly.socket', gitaly_message)
+        expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-upload-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil)
       end
 
       it "should log the command execution" do
         message = "executing git command"
         user_string = "user with id #{gl_id}"
+
         expect($logger).to receive(:info).with(message, command: "gitaly-upload-pack unix:gitaly.socket #{gitaly_message}", user: user_string)
       end
 
@@ -238,6 +233,11 @@ describe GitlabShell do
 
     context 'git-receive-pack' do
       let(:ssh_cmd) { "git-receive-pack gitlab-ci.git" }
+
+      before do
+        allow(api).to receive(:check_access).and_return(gitaly_check_access)
+      end
+
       after { subject.exec(ssh_cmd) }
 
       it "should process the command" do
@@ -245,13 +245,13 @@ describe GitlabShell do
       end
 
       it "should execute the command" do
-        expect(subject).to receive(:exec_cmd).with('git-receive-pack')
+        expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil)
       end
 
       it "should log the command execution" do
         message = "executing git command"
         user_string = "user with id #{gl_id}"
-        expect($logger).to receive(:info).with(message, command: "git-receive-pack", user: user_string)
+        expect($logger).to receive(:info).with(message, command: "gitaly-receive-pack unix:gitaly.socket #{gitaly_message}", user: user_string)
       end
     end
 
@@ -267,7 +267,7 @@ describe GitlabShell do
       end
 
       it "should execute the command" do
-        expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), 'unix:gitaly.socket', gitaly_message)
+        expect(subject).to receive(:exec_cmd).with(File.join(ROOT_PATH, "bin/gitaly-receive-pack"), gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil)
       end
 
       it "should log the command execution" do
@@ -284,7 +284,6 @@ describe GitlabShell do
 
     shared_examples_for 'upload-archive' do |command|
       let(:ssh_cmd) { "#{command} gitlab-ci.git" }
-      let(:exec_cmd_params) { ['git-upload-archive'] }
       let(:exec_cmd_log_params) { exec_cmd_params }
 
       after { subject.exec(ssh_cmd) }
@@ -309,14 +308,6 @@ describe GitlabShell do
       end
     end
 
-    context 'git-upload-archive' do
-      it_behaves_like 'upload-archive', 'git-upload-archive'
-    end
-
-    context 'git upload-archive' do
-      it_behaves_like 'upload-archive', 'git upload-archive'
-    end
-
     context 'gitaly-upload-archive' do
       before do
         allow(api).to receive(:check_access).and_return(gitaly_check_access)
@@ -327,8 +318,7 @@ describe GitlabShell do
         let(:exec_cmd_params) do
           [
             File.join(ROOT_PATH, "bin", gitaly_executable),
-            'unix:gitaly.socket',
-            gitaly_message
+            { gitaly_address: 'unix:gitaly.socket', json_args: gitaly_message, token: nil }
           ]
         end
         let(:exec_cmd_log_params) do
@@ -424,6 +414,19 @@ describe GitlabShell do
     let(:ssh_cmd) { "git-upload-pack gitlab-ci.git" }
 
     describe 'check access with api' do
+      before do
+        allow(api).to receive(:check_access).and_return(
+          GitAccessStatus.new(
+            false,
+            'denied',
+            gl_repository: nil,
+            gl_id: nil,
+            gl_username: nil,
+            git_config_options: nil,
+            gitaly: nil,
+            git_protocol: nil))
+      end
+
       after { subject.exec(ssh_cmd) }
 
       it "should call api.check_access" do
@@ -431,126 +434,10 @@ describe GitlabShell do
       end
 
       it "should disallow access and log the attempt if check_access returns false status" do
-        allow(api).to receive(:check_access).and_return(GitAccessStatus.new(
-                  false,
-                  'denied',
-                  gl_repository: nil,
-                  gl_id: nil,
-                  gl_username: nil,
-                  git_config_options: nil,
-                  gitaly: nil,
-                  git_protocol: nil))
         message = 'Access denied'
         user_string = "user with id #{gl_id}"
-        expect($logger).to receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string)
-      end
-    end
-  end
-
-  describe :exec_cmd do
-    let(:shell) { GitlabShell.new(gl_id) }
-    let(:env) do
-      {
-        'HOME' => ENV['HOME'],
-        'PATH' => ENV['PATH'],
-        'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
-        'LANG' => ENV['LANG'],
-        'GL_ID' => gl_id,
-        'GL_PROTOCOL' => 'ssh',
-        'GL_REPOSITORY' => gl_repository,
-        'GL_USERNAME' => 'testuser'
-      }
-    end
-    let(:exec_options) { { unsetenv_others: true, chdir: ROOT_PATH } }
-    before do
-      allow(Kernel).to receive(:exec)
-      shell.gl_repository = gl_repository
-      shell.git_protocol = git_protocol
-      shell.instance_variable_set(:@username, gl_username)
-    end
-
-    it "uses Kernel::exec method" do
-      expect(Kernel).to receive(:exec).with(env, 1, 2, exec_options).once
-      shell.send :exec_cmd, 1, 2
-    end
 
-    it "refuses to execute a lone non-array argument" do
-      expect { shell.send :exec_cmd, 1 }.to raise_error(GitlabShell::DisallowedCommandError)
-    end
-
-    it "allows one argument if it is an array" do
-      expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once
-      shell.send :exec_cmd, [1, 2]
-    end
-
-    context "when specifying a git_tracing log file" do
-      let(:git_trace_log_file) { '/tmp/git_trace_performance.log' }
-
-      before do
-        allow_any_instance_of(GitlabConfig).to receive(:git_trace_log_file).and_return(git_trace_log_file)
-        shell
-      end
-
-      it "uses GIT_TRACE_PERFORMANCE" do
-        expected_hash = hash_including(
-          'GIT_TRACE' => git_trace_log_file,
-          'GIT_TRACE_PACKET' => git_trace_log_file,
-          'GIT_TRACE_PERFORMANCE' => git_trace_log_file
-        )
-        expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once
-
-        shell.send :exec_cmd, [1, 2]
-      end
-
-      context "when provides a relative path" do
-        let(:git_trace_log_file) { 'git_trace_performance.log' }
-
-        it "does not uses GIT_TRACE*" do
-          # If we try to use it we'll show a warning to the users
-          expected_hash = hash_excluding(
-            'GIT_TRACE', 'GIT_TRACE_PACKET', 'GIT_TRACE_PERFORMANCE'
-          )
-          expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once
-
-          shell.send :exec_cmd, [1, 2]
-        end
-
-        it "writes an entry on the log" do
-          message = 'git trace log path must be absolute, ignoring'
-
-          expect($logger).to receive(:warn).
-            with(message, git_trace_log_file: git_trace_log_file)
-
-          expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once
-          shell.send :exec_cmd, [1, 2]
-        end
-      end
-
-      context "when provides a file not writable" do
-        before do
-          expect(File).to receive(:open).with(git_trace_log_file, 'a').and_raise(Errno::EACCES)
-        end
-
-        it "does not uses GIT_TRACE*" do
-          # If we try to use it we'll show a warning to the users
-          expected_hash = hash_excluding(
-            'GIT_TRACE', 'GIT_TRACE_PACKET', 'GIT_TRACE_PERFORMANCE'
-          )
-          expect(Kernel).to receive(:exec).with(expected_hash, [1, 2], exec_options).once
-
-          shell.send :exec_cmd, [1, 2]
-        end
-
-        it "writes an entry on the log" do
-          message = 'Failed to open git trace log file'
-          error = 'Permission denied'
-
-          expect($logger).to receive(:warn).
-            with(message, git_trace_log_file: git_trace_log_file, error: error)
-
-          expect(Kernel).to receive(:exec).with(env, [1, 2], exec_options).once
-          shell.send :exec_cmd, [1, 2]
-        end
+        expect($logger).to receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string)
       end
     end
   end