repository: Structured errors for UserRebaseConfirmable

Currently, UserRebaseConfirmable will return gRPC OK even when there is
an error during rebase or if there is an error during PreReceive when it
calls the GitLab API for an access check.

This change modifies the error handling to return a detailed gRPC
structured error in either of these failure modes.

Changelog: changed

3 files changed, 100 insertions, 12 deletions

diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index 30c4438cb..e23a18fe9 100644
--- a/internal/gitaly/service/operations/rebase.go
+++ b/internal/gitaly/service/operations/rebase.go
@@ -9,6 +9,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -67,6 +68,34 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		SkipEmptyCommits: true,
 	})
 	if err != nil {
+		if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+			var conflictErr git2go.ConflictingFilesError
+			if errors.As(err, &conflictErr) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+				for _, conflictingFile := range conflictErr.ConflictingFiles {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrFailedPreconditionf("rebasing commits: %w", err),
+					&gitalypb.UserRebaseConfirmableError{
+						Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
+							RebaseConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
+						},
+					},
+				)
+				if err != nil {
+					return helper.ErrInternalf("error details: %w", err)
+				}
+
+				return detailedErr
+			}
+
+			return helper.ErrInternalf("rebasing commits: %w", err)
+		}
+
 		return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
 			GitError: err.Error(),
 		})
@@ -100,6 +129,22 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		header.GitPushOptions...); err != nil {
 		switch {
 		case errors.As(err, &updateref.HookError{}):
+			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrPermissionDeniedf("access check: %q", err),
+					&gitalypb.UserRebaseConfirmableError{
+						Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
+							AccessCheck: &gitalypb.AccessCheckError{
+								ErrorMessage: err.Error(),
+							},
+						},
+					},
+				)
+				if err != nil {
+					return helper.ErrInternalf("error details: %w", err)
+				}
+				return detailedErr
+			}
 			return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
 				PreReceiveError: err.Error(),
 			})
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 1d388ded7..a43f084f3 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -1,6 +1,8 @@
 package operations
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -14,7 +16,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -507,8 +511,11 @@ func TestFailedUserRebaseConfirmableDueToApplyBeingFalse(t *testing.T) {
 
 func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
+		Run(t, testFailedUserRebaseConfirmableRequestDueToPreReceiveError)
+}
 
+func testFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -541,11 +548,23 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
 
 			secondResponse, err := rebaseStream.Recv()
 
-			require.NoError(t, err, "receive second response")
-			require.Contains(t, secondResponse.PreReceiveError, "failure")
-
-			_, err = rebaseStream.Recv()
-			require.Equal(t, io.EOF, err)
+			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+				testhelper.RequireGrpcError(t, errWithDetails(t,
+					helper.ErrPermissionDeniedf(`access check: "failure\n"`),
+					&gitalypb.UserRebaseConfirmableError{
+						Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
+							AccessCheck: &gitalypb.AccessCheckError{
+								ErrorMessage: "failure\n",
+							},
+						},
+					},
+				), err)
+			} else {
+				require.NoError(t, err, "receive second response")
+				require.Contains(t, secondResponse.PreReceiveError, "failure")
+				_, err = rebaseStream.Recv()
+				require.Equal(t, io.EOF, err)
+			}
 
 			_, err = repo.ReadCommit(ctx, git.Revision(firstResponse.GetRebaseSha()))
 			if hookName == "pre-receive" {
@@ -561,10 +580,13 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
 	}
 }
 
-func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) {
+func TestUserRebaseConfirmable_gitError(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
+		Run(t, testFailedUserRebaseConfirmableDueToGitError)
+}
 
+func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repoCopyProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
@@ -578,14 +600,28 @@ func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) {
 	require.NoError(t, err)
 
 	headerRequest := buildHeaderRequest(repoProto, gittest.TestUser, "1", failedBranchName, branchSha, repoCopyProto, "master")
+
 	require.NoError(t, rebaseStream.Send(headerRequest), "send header")
 
 	firstResponse, err := rebaseStream.Recv()
-	require.NoError(t, err, "receive first response")
-	require.Contains(t, firstResponse.GitError, "conflict")
+	if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+		testhelper.RequireGrpcError(t, errWithDetails(t,
+			helper.ErrFailedPrecondition(errors.New(`rebasing commits: rebase: commit "eb8f5fb9523b868cef583e09d4bf70b99d2dd404": there are conflicting files`)),
+			&gitalypb.UserRebaseConfirmableError{
+				Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
+					RebaseConflict: &gitalypb.MergeConflictError{
+						ConflictingFiles: [][]byte{[]byte("README.md")},
+					},
+				},
+			},
+		), err)
+	} else {
+		require.NoError(t, err, "receive first response")
+		require.Contains(t, firstResponse.GitError, "conflict")
 
-	_, err = rebaseStream.Recv()
-	require.Equal(t, io.EOF, err)
+		_, err = rebaseStream.Recv()
+		require.Equal(t, io.EOF, err)
+	}
 
 	newBranchSha := getBranchSha(t, cfg, repoPath, failedBranchName)
 	require.Equal(t, branchSha, newBranchSha, "branch should not change when the rebase fails due to GitError")
diff --git a/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go
new file mode 100644
index 000000000..9f49b4559
--- /dev/null
+++ b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go
@@ -0,0 +1,7 @@
+package featureflag
+
+// UserRebaseConfirmableImprovedErrorHandling enables proper error handling in the UserRebaseConfirmable
+// RPC. When this flag is disabled many error cases were returning successfully with an error message
+// embedded in the response. With this flag enabled, this is converted to return real gRPC errors with
+// structured errors.
+var UserRebaseConfirmableImprovedErrorHandling = NewFeatureFlag("user_rebase_confirmable_improved_error_handling", false)