From 76aa78b7f78f8a38242aba7598d3cb53149d970c Mon Sep 17 00:00:00 2001 From: John Cai Date: Mon, 7 Mar 2022 09:48:11 -0500 Subject: 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 --- internal/gitaly/service/operations/rebase.go | 47 ++++++++++ internal/gitaly/service/operations/rebase_test.go | 102 +++++++++++++++++---- ...r_rebase_confirmable_improved_error_handling.go | 7 ++ 3 files changed, 138 insertions(+), 18 deletions(-) create mode 100644 internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go 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) -- cgit v1.2.3