repository: Structured errors for UserRebaseConfirmablejc-rebase-cofirmable-structured-errors

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, 138 insertions, 18 deletions

diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index 30c4438cb..cdb408f16 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,35 @@ 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 {
+					fmt.Printf("conflicting file: %v\n", conflictingFile)
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrInternalf("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 +130,23 @@ 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.ErrInternalf("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..f2ae232b9 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"
@@ -506,8 +510,12 @@ func TestFailedUserRebaseConfirmableDueToApplyBeingFalse(t *testing.T) {
 }
 
 func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
+		Run(t, testFailedUserRebaseConfirmableRequestDueToPreReceiveError)
+}
+
+func testFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -527,43 +535,79 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
 			rebaseStream, err := client.UserRebaseConfirmable(ctx)
 			require.NoError(t, err)
 
-			headerRequest := buildHeaderRequest(repoProto, gittest.TestUser, fmt.Sprintf("%v", i), rebaseBranchName, branchSha, repoCopyProto, "master")
+			headerRequest := buildHeaderRequest(
+				repoProto,
+				gittest.TestUser,
+				fmt.Sprintf("%v", i),
+				rebaseBranchName,
+				branchSha,
+				repoCopyProto,
+				"master")
+
 			require.NoError(t, rebaseStream.Send(headerRequest), "send header")
 
 			firstResponse, err := rebaseStream.Recv()
 			require.NoError(t, err, "receive first response")
 
 			_, err = repo.ReadCommit(ctx, git.Revision(firstResponse.GetRebaseSha()))
-			require.Equal(t, localrepo.ErrObjectNotFound, err, "commit should not exist in the normal repo given that it is quarantined")
+			require.Equal(t,
+				localrepo.ErrObjectNotFound,
+				err,
+				"commit should not exist in the normal repo given that it is quarantined")
 
 			applyRequest := buildApplyRequest(true)
 			require.NoError(t, rebaseStream.Send(applyRequest), "apply rebase")
 
 			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.ErrInternalf(`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" {
-				require.Equal(t, localrepo.ErrObjectNotFound, err, "commit should have been discarded")
+				require.Equal(t,
+					localrepo.ErrObjectNotFound,
+					err,
+					"commit should have been discarded")
 			} else {
 				require.NoError(t, err)
 			}
 
 			newBranchSha := getBranchSha(t, cfg, repoPath, rebaseBranchName)
-			require.Equal(t, branchSha, newBranchSha, "branch should not change when the rebase fails due to PreReceiveError")
-			require.NotEqual(t, newBranchSha, firstResponse.GetRebaseSha(), "branch should not be the sha returned when the rebase fails due to PreReceiveError")
+			require.Equal(t,
+				branchSha,
+				newBranchSha,
+				"branch should not change when the rebase fails due to PreReceiveError")
+			require.NotEqual(t,
+				newBranchSha,
+				firstResponse.GetRebaseSha(),
+				"branch should not be the sha returned when the rebase fails due to PreReceiveError")
 		})
 	}
 }
 
-func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) {
+func TestUserRebaseConfirmable_gitError(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
+		Run(t, testFailedUserRebaseConfirmableDueToGitError)
+}
+
+func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -577,15 +621,37 @@ func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) {
 	rebaseStream, err := client.UserRebaseConfirmable(ctx)
 	require.NoError(t, err)
 
-	headerRequest := buildHeaderRequest(repoProto, gittest.TestUser, "1", failedBranchName, branchSha, repoCopyProto, "master")
+	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.ErrInternal(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)