Merge branch 'exception-bridge' into 'master'

Gitaly-Ruby exception bridge

Closes #588

See merge request gitlab-org/gitaly!358

10 files changed, 217 insertions, 159 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7108ff2a8..819d122c7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 UNRELEASED
 
+- Pass details of Gitaly-Ruby's Ruby exceptions back to
+  callers in the request trailers
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/358
 - Allow individual endpoints to be rate-limited per-repository
   https://gitlab.com/gitlab-org/gitaly/merge_requests/376
 - Implement OperationService.UserDeleteBranch RPC
diff --git a/internal/service/commit/find_commits.go b/internal/service/commit/find_commits.go
index dd489d904..effcdd822 100644
--- a/internal/service/commit/find_commits.go
+++ b/internal/service/commit/find_commits.go
@@ -47,6 +47,8 @@ func (s *server) FindCommits(req *pb.FindCommitsRequest, stream pb.CommitService
 	return rubyserver.Proxy(func() error {
 		resp, err := rubyStream.Recv()
 		if err != nil {
+			md := rubyStream.Trailer()
+			stream.SetTrailer(md)
 			return err
 		}
 		return stream.Send(resp)
diff --git a/internal/service/diff/patch.go b/internal/service/diff/patch.go
index d47e06828..690c67c2d 100644
--- a/internal/service/diff/patch.go
+++ b/internal/service/diff/patch.go
@@ -27,6 +27,8 @@ func (s *server) CommitPatch(in *pb.CommitPatchRequest, stream pb.DiffService_Co
 	return rubyserver.Proxy(func() error {
 		resp, err := rubyStream.Recv()
 		if err != nil {
+			md := rubyStream.Trailer()
+			stream.SetTrailer(md)
 			return err
 		}
 		return stream.Send(resp)
diff --git a/internal/service/ref/refs.go b/internal/service/ref/refs.go
index ab1de1538..d7b6ba8e4 100644
--- a/internal/service/ref/refs.go
+++ b/internal/service/ref/refs.go
@@ -91,6 +91,8 @@ func (s *server) FindAllTags(in *pb.FindAllTagsRequest, stream pb.RefService_Fin
 	return rubyserver.Proxy(func() error {
 		resp, err := rubyStream.Recv()
 		if err != nil {
+			md := rubyStream.Trailer()
+			stream.SetTrailer(md)
 			return err
 		}
 		return stream.Send(resp)
diff --git a/ruby/lib/gitaly_server/commit_service.rb b/ruby/lib/gitaly_server/commit_service.rb
index 2f9857e66..69a76276e 100644
--- a/ruby/lib/gitaly_server/commit_service.rb
+++ b/ruby/lib/gitaly_server/commit_service.rb
@@ -3,43 +3,47 @@ module GitalyServer
     include Utils
 
     def commit_stats(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
-      revision = request.revision unless request.revision.empty?
+      bridge_exceptions do
+        repo = Gitlab::Git::Repository.from_call(call)
+        revision = request.revision unless request.revision.empty?
 
-      commit = Gitlab::Git::Commit.find(repo, revision)
+        commit = Gitlab::Git::Commit.find(repo, revision)
 
-      # In the odd case that the revision given doesn't exist we need to raise
-      # an exception. Since GitLab (currently) already does this for us we don't
-      # expect this to actually happen, just guarding against future code change
-      raise GRPC::Internal.new("commit not found for revision '#{revision}'") unless commit
+        # In the odd case that the revision given doesn't exist we need to raise
+        # an exception. Since GitLab (currently) already does this for us we don't
+        # expect this to actually happen, just guarding against future code change
+        raise GRPC::Internal.new("commit not found for revision '#{revision}'") unless commit
 
-      stats = Gitlab::Git::CommitStats.new(repo, commit)
+        stats = Gitlab::Git::CommitStats.new(repo, commit)
 
-      Gitaly::CommitStatsResponse.new(oid: stats.id, additions: stats.additions, deletions: stats.deletions)
+        Gitaly::CommitStatsResponse.new(oid: stats.id, additions: stats.additions, deletions: stats.deletions)
+      end
     end
 
     def find_commits(request, call)
-      repository = Gitlab::Git::Repository.from_call(call)
-      options = {
-        ref: request.revision,
-        limit: request.limit,
-        follow: request.follow,
-        skip_merges: request.skip_merges,
-        disable_walk: request.disable_walk,
-        offset: request.offset
-      }
-      options[:path] = request.paths unless request.paths.empty?
-
-      options[:before] = Time.at(request.before.seconds).to_datetime if request.before
-      options[:after] = Time.at(request.after.seconds).to_datetime if request.after
-
-      Enumerator.new do |y|
-        # Send back 'pages' with 20 commits each
-        repository.raw_log(options).each_slice(20) do |rugged_commits|
-          commits = rugged_commits.map do |rugged_commit|
-            gitaly_commit_from_rugged(rugged_commit)
+      bridge_exceptions do
+        repository = Gitlab::Git::Repository.from_call(call)
+        options = {
+          ref: request.revision,
+          limit: request.limit,
+          follow: request.follow,
+          skip_merges: request.skip_merges,
+          disable_walk: request.disable_walk,
+          offset: request.offset
+        }
+        options[:path] = request.paths unless request.paths.empty?
+
+        options[:before] = Time.at(request.before.seconds).to_datetime if request.before
+        options[:after] = Time.at(request.after.seconds).to_datetime if request.after
+
+        Enumerator.new do |y|
+          # Send back 'pages' with 20 commits each
+          repository.raw_log(options).each_slice(20) do |rugged_commits|
+            commits = rugged_commits.map do |rugged_commit|
+              gitaly_commit_from_rugged(rugged_commit)
+            end
+            y.yield Gitaly::FindCommitsResponse.new(commits: commits)
           end
-          y.yield Gitaly::FindCommitsResponse.new(commits: commits)
         end
       end
     end
diff --git a/ruby/lib/gitaly_server/diff_service.rb b/ruby/lib/gitaly_server/diff_service.rb
index 0632043e2..6f0260c92 100644
--- a/ruby/lib/gitaly_server/diff_service.rb
+++ b/ruby/lib/gitaly_server/diff_service.rb
@@ -1,13 +1,17 @@
 module GitalyServer
   class DiffService < Gitaly::DiffService::Service
+    include Utils
+
     def commit_patch(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
-      commit = Gitlab::Git::Commit.find(repo, request.revision)
+      bridge_exceptions do
+        repo = Gitlab::Git::Repository.from_call(call)
+        commit = Gitlab::Git::Commit.find(repo, request.revision)
 
-      Enumerator.new do |y|
-        io = StringIO.new(commit.to_diff)
-        while chunk = io.read(Gitlab.config.git.write_buffer_size)
-          y.yield Gitaly::CommitPatchResponse.new(data: chunk)
+        Enumerator.new do |y|
+          io = StringIO.new(commit.to_diff)
+          while chunk = io.read(Gitlab.config.git.write_buffer_size)
+            y.yield Gitaly::CommitPatchResponse.new(data: chunk)
+          end
         end
       end
     end
diff --git a/ruby/lib/gitaly_server/operations_service.rb b/ruby/lib/gitaly_server/operations_service.rb
index c1a0d6693..836d38486 100644
--- a/ruby/lib/gitaly_server/operations_service.rb
+++ b/ruby/lib/gitaly_server/operations_service.rb
@@ -3,87 +3,103 @@ module GitalyServer
     include Utils
 
     def user_create_tag(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
-
-      gitaly_user = request.user
-      raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
-      user = Gitlab::Git::User.from_gitaly(gitaly_user)
-
-      tag_name = request.tag_name
-      raise GRPC::InvalidArgument.new('empty tag name') unless tag_name.present?
-
-      target_revision = request.target_revision
-      raise GRPC::InvalidArgument.new('empty target revision') unless target_revision.present?
-
-      created_tag = repo.add_tag(tag_name, user: user, target: target_revision, message: request.message.presence)
-      return Gitaly::UserCreateTagResponse.new unless created_tag
-
-      rugged_commit = created_tag.dereferenced_target.rugged_commit
-      commit = gitaly_commit_from_rugged(rugged_commit)
-      tag = Gitaly::Tag.new(
-        name: tag_name.b,
-        id: created_tag.target,
-        target_commit: commit,
-        message: created_tag.message.to_s.b
-      )
-
-      Gitaly::UserCreateTagResponse.new(tag: tag)
-    rescue Gitlab::Git::Repository::InvalidRef => e
-      raise GRPC::FailedPrecondition.new(e.message)
-    rescue Rugged::TagError
-      return Gitaly::UserCreateTagResponse.new(exists: true)
-    rescue Gitlab::Git::HooksService::PreReceiveError => e
-      return Gitaly::UserCreateTagResponse.new(pre_receive_error: e.message)
+      bridge_exceptions do
+        begin
+          repo = Gitlab::Git::Repository.from_call(call)
+
+          gitaly_user = request.user
+          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+          user = Gitlab::Git::User.from_gitaly(gitaly_user)
+
+          tag_name = request.tag_name
+          raise GRPC::InvalidArgument.new('empty tag name') unless tag_name.present?
+
+          target_revision = request.target_revision
+          raise GRPC::InvalidArgument.new('empty target revision') unless target_revision.present?
+
+          created_tag = repo.add_tag(tag_name, user: user, target: target_revision, message: request.message.presence)
+          return Gitaly::UserCreateTagResponse.new unless created_tag
+
+          rugged_commit = created_tag.dereferenced_target.rugged_commit
+          commit = gitaly_commit_from_rugged(rugged_commit)
+          tag = Gitaly::Tag.new(
+            name: tag_name.b,
+            id: created_tag.target,
+            target_commit: commit,
+            message: created_tag.message.to_s.b
+          )
+
+          Gitaly::UserCreateTagResponse.new(tag: tag)
+        rescue Gitlab::Git::Repository::InvalidRef => e
+          raise GRPC::FailedPrecondition.new(e.message)
+        rescue Rugged::TagError
+          return Gitaly::UserCreateTagResponse.new(exists: true)
+        rescue Gitlab::Git::HooksService::PreReceiveError => e
+          return Gitaly::UserCreateTagResponse.new(pre_receive_error: e.message)
+        end
+      end
     end
 
     def user_delete_tag(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
+      bridge_exceptions do
+        begin
+          repo = Gitlab::Git::Repository.from_call(call)
 
-      gitaly_user = request.user
-      raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
-      user = Gitlab::Git::User.from_gitaly(gitaly_user)
+          gitaly_user = request.user
+          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+          user = Gitlab::Git::User.from_gitaly(gitaly_user)
 
-      tag_name = request.tag_name
-      raise GRPC::InvalidArgument.new('empty tag name') if tag_name.blank?
+          tag_name = request.tag_name
+          raise GRPC::InvalidArgument.new('empty tag name') if tag_name.blank?
 
-      repo.rm_tag(tag_name, user: user)
+          repo.rm_tag(tag_name, user: user)
 
-      Gitaly::UserDeleteTagResponse.new
-    rescue Gitlab::Git::HooksService::PreReceiveError => e
-      Gitaly::UserDeleteTagResponse.new(pre_receive_error: e.message)
+          Gitaly::UserDeleteTagResponse.new
+        rescue Gitlab::Git::HooksService::PreReceiveError => e
+          Gitaly::UserDeleteTagResponse.new(pre_receive_error: e.message)
+        end
+      end
     end
 
     def user_create_branch(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
-      target = request.start_point
-      raise GRPC::InvalidArgument.new('empty start_point') if target.empty?
-      gitaly_user = request.user
-      raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
-
-      branch_name = request.branch_name
-      user = Gitlab::Git::User.from_gitaly(gitaly_user)
-      created_branch = repo.add_branch(branch_name, user: user, target: target)
-      return Gitaly::UserCreateBranchResponse.new unless created_branch
-
-      rugged_commit = created_branch.dereferenced_target.rugged_commit
-      commit = gitaly_commit_from_rugged(rugged_commit)
-      branch = Gitaly::Branch.new(name: branch_name, target_commit: commit)
-      Gitaly::UserCreateBranchResponse.new(branch: branch)
-    rescue Gitlab::Git::Repository::InvalidRef, Gitlab::Git::CommitError => ex
-      raise GRPC::FailedPrecondition.new(ex.message)
-    rescue Gitlab::Git::HooksService::PreReceiveError => ex
-      return Gitaly::UserCreateBranchResponse.new(pre_receive_error: ex.message)
+      bridge_exceptions do
+        begin
+          repo = Gitlab::Git::Repository.from_call(call)
+          target = request.start_point
+          raise GRPC::InvalidArgument.new('empty start_point') if target.empty?
+          gitaly_user = request.user
+          raise GRPC::InvalidArgument.new('empty user') unless gitaly_user
+
+          branch_name = request.branch_name
+          user = Gitlab::Git::User.from_gitaly(gitaly_user)
+          created_branch = repo.add_branch(branch_name, user: user, target: target)
+          return Gitaly::UserCreateBranchResponse.new unless created_branch
+
+          rugged_commit = created_branch.dereferenced_target.rugged_commit
+          commit = gitaly_commit_from_rugged(rugged_commit)
+          branch = Gitaly::Branch.new(name: branch_name, target_commit: commit)
+          Gitaly::UserCreateBranchResponse.new(branch: branch)
+        rescue Gitlab::Git::Repository::InvalidRef, Gitlab::Git::CommitError => ex
+          raise GRPC::FailedPrecondition.new(ex.message)
+        rescue Gitlab::Git::HooksService::PreReceiveError => ex
+          return Gitaly::UserCreateBranchResponse.new(pre_receive_error: ex.message)
+        end
+      end
     end
 
     def user_delete_branch(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
-      user = Gitlab::Git::User.from_gitaly(request.user)
-
-      repo.rm_branch(request.branch_name, user: user)
-
-      Gitaly::UserDeleteBranchResponse.new
-    rescue Gitlab::Git::HooksService::PreReceiveError => e
-      Gitaly::UserDeleteTagResponse.new(pre_receive_error: e.message)
+      bridge_exceptions do
+        begin
+          repo = Gitlab::Git::Repository.from_call(call)
+          user = Gitlab::Git::User.from_gitaly(request.user)
+
+          repo.rm_branch(request.branch_name, user: user)
+
+          Gitaly::UserDeleteBranchResponse.new
+        rescue Gitlab::Git::HooksService::PreReceiveError => e
+          Gitaly::UserDeleteTagResponse.new(pre_receive_error: e.message)
+        end
+      end
     end
   end
 end
diff --git a/ruby/lib/gitaly_server/ref_service.rb b/ruby/lib/gitaly_server/ref_service.rb
index cb9bc963a..dd41ff26f 100644
--- a/ruby/lib/gitaly_server/ref_service.rb
+++ b/ruby/lib/gitaly_server/ref_service.rb
@@ -5,77 +5,89 @@ module GitalyServer
     TAGS_PER_MESSAGE = 100
 
     def create_branch(request, call)
-      start_point = request.start_point
-      start_point = 'HEAD' if start_point.empty?
-      branch_name = request.name
+      bridge_exceptions do
+        begin
+          start_point = request.start_point
+          start_point = 'HEAD' if start_point.empty?
+          branch_name = request.name
 
-      repo = Gitlab::Git::Repository.from_call(call)
-      rugged_ref = repo.rugged.branches.create(branch_name, start_point)
+          repo = Gitlab::Git::Repository.from_call(call)
+          rugged_ref = repo.rugged.branches.create(branch_name, start_point)
 
-      Gitaly::CreateBranchResponse.new(
-        status: :OK,
-        branch: Gitaly::Branch.new(
-          name: rugged_ref.name.b,
-          target_commit: gitaly_commit_from_rugged(rugged_ref.target)
-        )
-      )
-    rescue Rugged::ReferenceError => e
-      status = case e.to_s
-               when /'refs\/heads\/#{branch_name}' is not valid/
-                 :ERR_INVALID
-               when /a reference with that name already exists/
-                 :ERR_EXISTS
-               else
-                 :ERR_INVALID_START_POINT
-               end
+          Gitaly::CreateBranchResponse.new(
+            status: :OK,
+            branch: Gitaly::Branch.new(
+              name: rugged_ref.name.b,
+              target_commit: gitaly_commit_from_rugged(rugged_ref.target)
+            )
+          )
+        rescue Rugged::ReferenceError => e
+          status = case e.to_s
+                   when /'refs\/heads\/#{branch_name}' is not valid/
+                     :ERR_INVALID
+                   when /a reference with that name already exists/
+                     :ERR_EXISTS
+                   else
+                     :ERR_INVALID_START_POINT
+                   end
 
-      Gitaly::CreateBranchResponse.new(status: status)
+          Gitaly::CreateBranchResponse.new(status: status)
+        end
+      end
     end
 
     def delete_branch(request, call)
-      branch_name = request.name
-      raise GRPC::InvalidArgument.new("empty Name") if branch_name.empty?
+      bridge_exceptions do
+        begin
+          branch_name = request.name
+          raise GRPC::InvalidArgument.new("empty Name") if branch_name.empty?
 
-      repo = Gitlab::Git::Repository.from_call(call)
-      repo.delete_branch(branch_name)
+          repo = Gitlab::Git::Repository.from_call(call)
+          repo.delete_branch(branch_name)
 
-      Gitaly::DeleteBranchResponse.new
-    rescue Gitlab::Git::Repository::DeleteBranchError => e
-      raise GRPC::Internal.new(e.to_s)
+          Gitaly::DeleteBranchResponse.new
+        rescue Gitlab::Git::Repository::DeleteBranchError => e
+          raise GRPC::Internal.new(e.to_s)
+        end
+      end
     end
 
     def find_branch(request, call)
-      branch_name = request.name
-      raise GRPC::InvalidArgument.new("empty Name") if branch_name.empty?
+      bridge_exceptions do
+        branch_name = request.name
+        raise GRPC::InvalidArgument.new("empty Name") if branch_name.empty?
 
-      repo = Gitlab::Git::Repository.from_call(call)
-      rugged_branch = repo.find_branch(branch_name)
-      gitaly_branch = Gitaly::Branch.new(
-        name: rugged_branch.name.b,
-        target_commit: gitaly_commit_from_rugged(rugged_branch.dereferenced_target.raw_commit)
-      ) unless rugged_branch.nil?
+        repo = Gitlab::Git::Repository.from_call(call)
+        rugged_branch = repo.find_branch(branch_name)
+        gitaly_branch = Gitaly::Branch.new(
+          name: rugged_branch.name.b,
+          target_commit: gitaly_commit_from_rugged(rugged_branch.dereferenced_target.raw_commit)
+        ) unless rugged_branch.nil?
 
-      Gitaly::FindBranchResponse.new(branch: gitaly_branch)
+        Gitaly::FindBranchResponse.new(branch: gitaly_branch)
+      end
     end
 
     def find_all_tags(request, call)
-      repo = Gitlab::Git::Repository.from_call(call)
+      bridge_exceptions do
+        repo = Gitlab::Git::Repository.from_call(call)
 
-      Enumerator.new do |y|
-        repo.tags.each_slice(TAGS_PER_MESSAGE) do |gitlab_tags|
-          tags = gitlab_tags.map do |gitlab_tag|
-            rugged_commit = gitlab_tag.dereferenced_target&.raw_commit
-            gitaly_commit = gitaly_commit_from_rugged(rugged_commit) if rugged_commit
+        Enumerator.new do |y|
+          repo.tags.each_slice(TAGS_PER_MESSAGE) do |gitlab_tags|
+            tags = gitlab_tags.map do |gitlab_tag|
+              rugged_commit = gitlab_tag.dereferenced_target&.raw_commit
+              gitaly_commit = gitaly_commit_from_rugged(rugged_commit) if rugged_commit
 
-            Gitaly::Tag.new(
-              name: gitlab_tag.name.b,
-              id: gitlab_tag.target,
-              message: gitlab_tag.message.to_s.b,
-              target_commit: gitaly_commit
-            )
-          end
+              Gitaly::Tag.new(
+                name: gitlab_tag.name.b,
+                id: gitlab_tag.target,
+                message: gitlab_tag.message.to_s.b,
+                target_commit: gitaly_commit
+              )
+            end
 
-          y.yield Gitaly::FindAllTagsResponse.new(tags: tags)
+            y.yield Gitaly::FindAllTagsResponse.new(tags: tags)
+          end
         end
       end
     end
diff --git a/ruby/lib/gitaly_server/repository_service.rb b/ruby/lib/gitaly_server/repository_service.rb
index 8006ddd6a..d27d6356e 100644
--- a/ruby/lib/gitaly_server/repository_service.rb
+++ b/ruby/lib/gitaly_server/repository_service.rb
@@ -1,12 +1,16 @@
 module GitalyServer
   class RepositoryService < Gitaly::RepositoryService::Service
+    include Utils
+
     def create_repository(request, _call)
-      repo_path = GitalyServer.repo_path(_call)
+      bridge_exceptions do
+        repo_path = GitalyServer.repo_path(_call)
 
-      # TODO refactor Repository.create to eliminate bogus '/' argument
-      Gitlab::Git::Repository.create('/', repo_path, bare: true, symlink_hooks_to: Gitlab.config.gitlab_shell.hooks_path)
+        # TODO refactor Repository.create to eliminate bogus '/' argument
+        Gitlab::Git::Repository.create('/', repo_path, bare: true, symlink_hooks_to: Gitlab.config.gitlab_shell.hooks_path)
 
-      Gitaly::CreateRepositoryResponse.new
+        Gitaly::CreateRepositoryResponse.new
+      end
     end
   end
 end
diff --git a/ruby/lib/gitaly_server/utils.rb b/ruby/lib/gitaly_server/utils.rb
index 2356794c4..08047cced 100644
--- a/ruby/lib/gitaly_server/utils.rb
+++ b/ruby/lib/gitaly_server/utils.rb
@@ -18,5 +18,14 @@ module GitalyServer
         date: Google::Protobuf::Timestamp.new(seconds: rugged_author[:time].to_i)
       )
     end
+
+    def bridge_exceptions
+      yield
+    rescue GRPC::BadStatus => e
+      # Pass GRPC back without wrapping
+      raise e
+    rescue StandardError => e
+      raise GRPC::Unknown.new(e.message, { "gitaly-ruby.exception.class": e.class.name })
+    end
   end
 end