Simplify error handling in ssh package

4 files changed, 67 insertions, 42 deletions

diff --git a/changelogs/unreleased/error-handling-2.yml b/changelogs/unreleased/error-handling-2.yml
new file mode 100644
index 000000000..bd30baaa4
--- /dev/null
+++ b/changelogs/unreleased/error-handling-2.yml
@@ -0,0 +1,5 @@
+---
+title: Simplify error handling in ssh package
+merge_request: 1029
+author:
+type: other
diff --git a/internal/service/ssh/receive_pack.go b/internal/service/ssh/receive_pack.go
index 51df1774a..e952e7777 100644
--- a/internal/service/ssh/receive_pack.go
+++ b/internal/service/ssh/receive_pack.go
@@ -12,18 +12,15 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer) error {
-	ctx := stream.Context()
 	req, err := stream.Recv() // First request contains only Repository, GlId, and GlUsername
 	if err != nil {
-		return err
+		return helper.ErrInternal(err)
 	}
 
-	grpc_logrus.Extract(ctx).WithFields(log.Fields{
+	grpc_logrus.Extract(stream.Context()).WithFields(log.Fields{
 		"GlID":             req.GlId,
 		"GlRepository":     req.GlRepository,
 		"GlUsername":       req.GlUsername,
@@ -31,9 +28,19 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer)
 	}).Debug("SSHReceivePack")
 
 	if err = validateFirstReceivePackRequest(req); err != nil {
-		return err
+		return helper.ErrInvalidArgument(err)
 	}
 
+	if err := sshReceivePack(stream, req); err != nil {
+		return helper.ErrInternal(err)
+	}
+
+	return nil
+}
+
+func sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, req *gitalypb.SSHReceivePackRequest) error {
+	ctx := stream.Context()
+
 	stdin := streamio.NewReader(func() ([]byte, error) {
 		request, err := stream.Recv()
 		return request.GetStdin(), err
@@ -67,17 +74,17 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer)
 	cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, gitOptions...)
 
 	if err != nil {
-		return status.Errorf(codes.Unavailable, "SSHReceivePack: cmd: %v", err)
+		return fmt.Errorf("start cmd: %v", err)
 	}
 
 	if err := cmd.Wait(); err != nil {
 		if status, ok := command.ExitStatus(err); ok {
-			return helper.DecorateError(
-				codes.Internal,
-				stream.Send(&gitalypb.SSHReceivePackResponse{ExitStatus: &gitalypb.ExitStatus{Value: int32(status)}}),
-			)
+			return stream.Send(&gitalypb.SSHReceivePackResponse{
+				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
+			})
 		}
-		return status.Errorf(codes.Unavailable, "SSHReceivePack: %v", err)
+
+		return fmt.Errorf("cmd wait: %v", err)
 	}
 
 	return nil
@@ -85,10 +92,10 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer)
 
 func validateFirstReceivePackRequest(req *gitalypb.SSHReceivePackRequest) error {
 	if req.GlId == "" {
-		return status.Errorf(codes.InvalidArgument, "SSHReceivePack: empty GlId")
+		return fmt.Errorf("empty GlId")
 	}
 	if req.Stdin != nil {
-		return status.Errorf(codes.InvalidArgument, "SSHReceivePack: non-empty data")
+		return fmt.Errorf("non-empty data in first request")
 	}
 
 	return nil
diff --git a/internal/service/ssh/upload_archive.go b/internal/service/ssh/upload_archive.go
index c29ebf07b..683ebe6bb 100644
--- a/internal/service/ssh/upload_archive.go
+++ b/internal/service/ssh/upload_archive.go
@@ -1,24 +1,32 @@
 package ssh
 
 import (
+	"fmt"
+
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func (s *server) SSHUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveServer) error {
 	req, err := stream.Recv() // First request contains Repository only
 	if err != nil {
-		return err
+		return helper.ErrInternal(err)
 	}
 	if err = validateFirstUploadArchiveRequest(req); err != nil {
-		return err
+		return helper.ErrInvalidArgument(err)
+	}
+
+	if err = sshUploadArchive(stream, req); err != nil {
+		return helper.ErrInternal(err)
 	}
 
+	return nil
+}
+
+func sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveServer, req *gitalypb.SSHUploadArchiveRequest) error {
 	repoPath, err := helper.GetRepoPath(req.Repository)
 	if err != nil {
 		return err
@@ -37,28 +45,26 @@ func (s *server) SSHUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer
 	cmd, err := git.BareCommand(stream.Context(), stdin, stdout, stderr, nil, "upload-archive", repoPath)
 
 	if err != nil {
-		return status.Errorf(codes.Unavailable, "SSHUploadArchive: cmd: %v", err)
+		return fmt.Errorf("start cmd: %v", err)
 	}
 
 	if err := cmd.Wait(); err != nil {
 		if status, ok := command.ExitStatus(err); ok {
-			return helper.DecorateError(
-				codes.Internal,
-				stream.Send(&gitalypb.SSHUploadArchiveResponse{ExitStatus: &gitalypb.ExitStatus{Value: int32(status)}}),
-			)
+			return stream.Send(&gitalypb.SSHUploadArchiveResponse{
+				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
+			})
 		}
-		return status.Errorf(codes.Unavailable, "SSHUploadArchive: %v", err)
+		return fmt.Errorf("wait cmd: %v", err)
 	}
 
-	return helper.DecorateError(
-		codes.Internal,
-		stream.Send(&gitalypb.SSHUploadArchiveResponse{ExitStatus: &gitalypb.ExitStatus{Value: 0}}),
-	)
+	return stream.Send(&gitalypb.SSHUploadArchiveResponse{
+		ExitStatus: &gitalypb.ExitStatus{Value: 0},
+	})
 }
 
 func validateFirstUploadArchiveRequest(req *gitalypb.SSHUploadArchiveRequest) error {
 	if req.Stdin != nil {
-		return status.Errorf(codes.InvalidArgument, "SSHUploadArchive: non-empty stdin")
+		return fmt.Errorf("non-empty stdin in first request")
 	}
 
 	return nil
diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go
index add35b0b3..324cd4275 100644
--- a/internal/service/ssh/upload_pack.go
+++ b/internal/service/ssh/upload_pack.go
@@ -1,27 +1,35 @@
 package ssh
 
 import (
+	"fmt"
+
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func (s *server) SSHUploadPack(stream gitalypb.SSHService_SSHUploadPackServer) error {
-	ctx := stream.Context()
-
 	req, err := stream.Recv() // First request contains Repository only
 	if err != nil {
-		return err
+		return helper.ErrInternal(err)
 	}
 
 	if err = validateFirstUploadPackRequest(req); err != nil {
-		return err
+		return helper.ErrInvalidArgument(err)
+	}
+
+	if err = sshUploadPack(stream, req); err != nil {
+		return helper.ErrInternal(err)
 	}
 
+	return nil
+}
+
+func sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, req *gitalypb.SSHUploadPackRequest) error {
+	ctx := stream.Context()
+
 	stdin := streamio.NewReader(func() ([]byte, error) {
 		request, err := stream.Recv()
 		return request.GetStdin(), err
@@ -51,17 +59,16 @@ func (s *server) SSHUploadPack(stream gitalypb.SSHService_SSHUploadPackServer) e
 	cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, args...)
 
 	if err != nil {
-		return status.Errorf(codes.Unavailable, "SSHUploadPack: cmd: %v", err)
+		return fmt.Errorf("start cmd: %v", err)
 	}
 
 	if err := cmd.Wait(); err != nil {
 		if status, ok := command.ExitStatus(err); ok {
-			return helper.DecorateError(
-				codes.Internal,
-				stream.Send(&gitalypb.SSHUploadPackResponse{ExitStatus: &gitalypb.ExitStatus{Value: int32(status)}}),
-			)
+			return stream.Send(&gitalypb.SSHUploadPackResponse{
+				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
+			})
 		}
-		return status.Errorf(codes.Unavailable, "SSHUploadPack: %v", err)
+		return fmt.Errorf("cmd wait: %v", err)
 	}
 
 	return nil
@@ -69,7 +76,7 @@ func (s *server) SSHUploadPack(stream gitalypb.SSHService_SSHUploadPackServer) e
 
 func validateFirstUploadPackRequest(req *gitalypb.SSHUploadPackRequest) error {
 	if req.Stdin != nil {
-		return status.Errorf(codes.InvalidArgument, "SSHUploadPack: non-empty stdin")
+		return fmt.Errorf("non-empty stdin in first request")
 	}
 
 	return nil