diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-02 09:41:54 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-02 09:41:54 +0300 |
commit | 0a03f045e065b9e7a157322fbe486e1f02fd8617 (patch) | |
tree | 46bdc92e9cc2035043cfc2b7c44e95ff6588ec35 | |
parent | 867b74cdceabbcbf65eb75025b4f3a10de9a4c55 (diff) | |
parent | 0fedeb41fda77a8024f923060b646648dbb753bb (diff) |
Merge branch 'pks-user-create-tag-introduce-structured-errors' into 'master'
operations: Introduce structured errors for UserCreateTag
Closes #4137
See merge request gitlab-org/gitaly!4743
-rw-r--r-- | internal/gitaly/server/auth_test.go | 5 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags.go | 104 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags_test.go | 310 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_user_create_tag_structured_errors.go | 9 |
4 files changed, 353 insertions, 75 deletions
diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go index 5a128a8f9..464feecc5 100644 --- a/internal/gitaly/server/auth_test.go +++ b/internal/gitaly/server/auth_test.go @@ -28,6 +28,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v15/internal/gitlab" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/middleware/limithandler" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" @@ -345,6 +346,10 @@ sleep %vs for i := 0; i < 2; i++ { i := i go func() { + // We don't care about the feature flag, but it's required to satisfy our + // sanity checks in testing. So let's just explicitly enable it here. + ctx := featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UserCreateTagStructuredErrors, true) + _, err := client.UserCreateTag(ctx, &gitalypb.UserCreateTagRequest{ Repository: repo, TagName: []byte(fmt.Sprintf("tag-name-%d", i)), diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 6b50cc0c2..f46528a9d 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -12,7 +12,9 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref" + "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -88,6 +90,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR } targetRevision := git.Revision(req.TargetRevision) + referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName)) committerTime, err := dateFromProto(req) if err != nil { @@ -104,8 +107,107 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return nil, err } - referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName)) + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + // We check whether the reference exists up-front so that we can return a proper + // detailed error to the caller in case the tag cannot be created. While this is + // racy given that the tag can be created afterwards by a concurrent caller, we'd + // detect that raciness in `updateReferenceWithHooks()` anyway. + if oid, err := quarantineRepo.ResolveRevision(ctx, referenceName.Revision()); err != nil { + // If the reference wasn't found we're on the happy path, otherwise + // something has gone wrong. + if !errors.Is(err, git.ErrReferenceNotFound) { + return nil, helper.ErrInternalf("resolving target reference: %w", err) + } + } else { + // When resolving the revision succeeds the tag reference exists already. + // Because we don't want to overwrite preexisting references in this RPC we + // thus return an error and alert the caller of this condition. + detailedErr, err := helper.ErrWithDetails( + helper.ErrAlreadyExistsf("tag reference exists already"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_ReferenceExists{ + ReferenceExists: &gitalypb.ReferenceExistsError{ + ReferenceName: []byte(referenceName.String()), + Oid: oid.String(), + }, + }, + }, + ) + if err != nil { + return nil, helper.ErrInternalf("creating detailed error: %w", err) + } + + return nil, detailedErr + } + } + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, quarantineDir, referenceName, tagID, git.ObjectHashSHA1.ZeroOID); err != nil { + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + var notAllowedError hook.NotAllowedError + var customHookErr updateref.CustomHookError + var updateRefError updateref.Error + + if errors.As(err, ¬AllowedError) { + detailedErr, err := helper.ErrWithDetails( + helper.ErrPermissionDeniedf("reference update denied by access checks: %w", err), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_AccessCheck{ + AccessCheck: &gitalypb.AccessCheckError{ + ErrorMessage: notAllowedError.Message, + UserId: notAllowedError.UserID, + Protocol: notAllowedError.Protocol, + Changes: notAllowedError.Changes, + }, + }, + }, + ) + if err != nil { + return nil, helper.ErrInternalf("error details: %w", err) + } + + return nil, detailedErr + } else if errors.As(err, &customHookErr) { + detailedErr, err := helper.ErrWithDetails( + // We explicitly don't include the custom hook error itself + // in the returned error because that would also contain the + // standard output or standard error in the error message. + // It's thus needlessly verbose and duplicates information + // we have available in the structured error anyway. + helper.ErrPermissionDeniedf("reference update denied by custom hooks"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_CustomHook{ + CustomHook: customHookErr.Proto(), + }, + }, + ) + if err != nil { + return nil, helper.ErrInternalf("error details: %w", err) + } + + return nil, detailedErr + } else if errors.As(err, &updateRefError) { + detailedErr, err := helper.ErrWithDetails( + helper.ErrFailedPreconditionf("reference update failed: %w", err), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_ReferenceUpdate{ + ReferenceUpdate: &gitalypb.ReferenceUpdateError{ + ReferenceName: []byte(updateRefError.Reference.String()), + OldOid: updateRefError.OldOID.String(), + NewOid: updateRefError.NewOID.String(), + }, + }, + }, + ) + if err != nil { + return nil, helper.ErrInternalf("error details: %w", err) + } + + return nil, detailedErr + } + + return nil, helper.ErrInternalf("updating reference: %w", err) + } + var customHookErrr updateref.CustomHookError if errors.As(err, &customHookErrr) { // TODO: this case should return a CustomHookError. diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index c1c53a315..c7b1f6ea7 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -3,6 +3,7 @@ package operations import ( + "context" "fmt" "path/filepath" "testing" @@ -16,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" @@ -142,7 +144,11 @@ func writeAssertObjectTypeUpdateHook(t *testing.T, repoPath, expectedObjectType func TestUserCreateTag_successful(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagSuccessful) +} + +func testUserCreateTagSuccessful(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -218,12 +224,15 @@ func TestUserCreateTag_successful(t *testing.T) { func TestUserCreateTag_transactional(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagTransactional) +} + +func testUserCreateTagTransactional(t *testing.T, ctx context.Context) { + t.Parallel() cfg := testcfg.Build(t) cfg.SocketPath = runOperationServiceServer(t, cfg, testserver.WithDisablePraefect()) - ctx := testhelper.Context(t) - transactionServer := &testTransactionServer{} // We're using internal gitaly socket to connect to the server. @@ -335,7 +344,11 @@ func TestUserCreateTag_transactional(t *testing.T) { func TestUserCreateTag_quarantine(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagQuarantine) +} + +func testUserCreateTagQuarantine(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -368,18 +381,35 @@ func TestUserCreateTag_quarantine(t *testing.T) { Timestamp: timestamppb.New(time.Unix(1600000000, 0)), Message: []byte("message"), }) - require.NoError(t, err) - // Conveniently, the pre-receive error will now contain output from our custom hook and thus - // the tag's contents. - testhelper.ProtoEqual(t, &gitalypb.UserCreateTagResponse{ - PreReceiveError: fmt.Sprintf(`object %s + expectedObject := fmt.Sprintf(`object %s type commit tag quarantined-tag tagger Jane Doe <janedoe@gitlab.com> 1600000000 +0800 -message`, commitID), - }, response) +message`, commitID) + + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("reference update denied by custom hooks"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + Stdout: []byte(expectedObject), + }, + }, + }, + ), err) + require.Nil(t, response) + } else { + require.NoError(t, err) + // Conveniently, the pre-receive error will now contain output from our custom hook and thus + // the tag's contents. + testhelper.ProtoEqual(t, &gitalypb.UserCreateTagResponse{ + PreReceiveError: expectedObject, + }, response) + } tagID := text.ChompBytes(testhelper.MustReadFile(t, tagIDOutputPath)) @@ -392,7 +422,11 @@ message`, commitID), func TestUserCreateTag_message(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagMessage) +} + +func testUserCreateTagMessage(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -488,7 +522,11 @@ func TestUserCreateTag_message(t *testing.T) { func TestUserCreateTag_targetRevision(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagTargetRevision) +} + +func testUserCreateTagTargetRevision(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -588,7 +626,11 @@ func TestUserCreateTag_targetRevision(t *testing.T) { func TestUserCreateTag_nonCommitTarget(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagNonCommitTarget) +} + +func testUserCreateTagNonCommitTarget(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -681,7 +723,11 @@ func TestUserCreateTag_nonCommitTarget(t *testing.T) { func TestUserCreateTag_nestedTags(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagNestedTags) +} + +func testUserCreateTagNestedTags(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) @@ -796,7 +842,11 @@ func TestUserCreateTag_nestedTags(t *testing.T) { func TestUserCreateTag_stableTagIDs(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagStableTagIDs) +} + +func testUserCreateTagStableTagIDs(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -872,7 +922,11 @@ func TestUserDeleteTag_prefixedTag(t *testing.T) { func TestUserCreateTag_prefixedTag(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagPrefixedTag) +} + +func testUserCreateTagPrefixedTag(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -908,7 +962,11 @@ func TestUserCreateTag_prefixedTag(t *testing.T) { func TestUserCreateTag_gitHooks(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagGitHooks) +} + +func testUserCreateTagGitHooks(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -1050,32 +1108,71 @@ func TestUserDeleteTag_hookFailure(t *testing.T) { func TestUserCreateTag_hookFailure(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagHookFailure) +} + +func testUserCreateTagHookFailure(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) - for _, hookName := range gitlabPreHooks { - repo, repoPath := gittest.CreateRepository(ctx, t, cfg) - commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents()) - - gittest.WriteCustomHook(t, repoPath, hookName, []byte( - "#!/bin/sh\necho GL_ID=$GL_ID\nexit 1"), - ) - - response, err := client.UserCreateTag(ctx, &gitalypb.UserCreateTagRequest{ - Repository: repo, - TagName: []byte("new-tag"), - TargetRevision: []byte(commitID), - User: gittest.TestUser, + for _, tc := range []struct { + hook string + hookType gitalypb.CustomHookError_HookType + }{ + { + hook: "pre-receive", + hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + }, + { + hook: "update", + hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, + }, + } { + t.Run(tc.hook, func(t *testing.T) { + repo, repoPath := gittest.CreateRepository(ctx, t, cfg) + commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents()) + + gittest.WriteCustomHook(t, repoPath, tc.hook, []byte( + "#!/bin/sh\necho GL_ID=$GL_ID\nexit 1"), + ) + + response, err := client.UserCreateTag(ctx, &gitalypb.UserCreateTagRequest{ + Repository: repo, + TagName: []byte("new-tag"), + TargetRevision: []byte(commitID), + User: gittest.TestUser, + }) + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("reference update denied by custom hooks"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: tc.hookType, + Stdout: []byte( + "GL_ID=" + gittest.TestUser.GlId + "\n", + ), + }, + }, + }, + ), err) + require.Nil(t, response) + } else { + require.NoError(t, err) + require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) + } }) - require.NoError(t, err) - require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId) } } func TestUserCreateTag_preexisting(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagPreexisting) +} + +func testUserCreateTagPreexisting(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) @@ -1096,9 +1193,32 @@ func TestUserCreateTag_preexisting(t *testing.T) { tagName: "v1.1.0", targetRevision: commitID.String(), user: gittest.TestUser, - expectedResponse: &gitalypb.UserCreateTagResponse{ - Exists: true, - }, + expectedErr: func() error { + if featureflag.UserCreateTagStructuredErrors.IsDisabled(ctx) { + return nil + } + + return errWithDetails(t, + helper.ErrAlreadyExistsf("tag reference exists already"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_ReferenceExists{ + ReferenceExists: &gitalypb.ReferenceExistsError{ + ReferenceName: []byte("refs/tags/v1.1.0"), + Oid: commitID.String(), + }, + }, + }, + ) + }(), + expectedResponse: func() *gitalypb.UserCreateTagResponse { + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + return nil + } + + return &gitalypb.UserCreateTagResponse{ + Exists: true, + } + }(), }, { desc: "existing tag nonexisting target revision", @@ -1123,8 +1243,12 @@ func TestUserCreateTag_preexisting(t *testing.T) { func TestUserCreateTag_invalidArgument(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagInvalidArgument) +} + +func testUserCreateTagInvalidArgument(t *testing.T, ctx context.Context) { + t.Parallel() - ctx := testhelper.Context(t) ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx) repo, repoPath := gittest.CreateRepository(ctx, t, cfg) @@ -1232,52 +1356,76 @@ func TestUserCreateTag_invalidArgument(t *testing.T) { func TestTagHookOutput(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testTagHookOutput) +} + +func testTagHookOutput(t *testing.T, ctx context.Context) { + t.Parallel() ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) - testCases := []struct { - desc string - hookContent string - output func(hookPath string) string + for _, tc := range []struct { + desc string + hookContent string + expectedStdout string + expectedStderr string + expectedErr func(hookPath string) string }{ { desc: "empty stdout and empty stderr", hookContent: "#!/bin/sh\nexit 1", - output: func(hookPath string) string { + expectedErr: func(hookPath string) string { return fmt.Sprintf("executing custom hooks: error executing %q: exit status 1", hookPath) }, }, { - desc: "empty stdout and some stderr", - hookContent: "#!/bin/sh\necho stderr >&2\nexit 1", - output: func(string) string { return "stderr\n" }, + desc: "empty stdout and some stderr", + hookContent: "#!/bin/sh\necho stderr >&2\nexit 1", + expectedStderr: "stderr\n", + expectedErr: func(string) string { return "stderr\n" }, }, { - desc: "some stdout and empty stderr", - hookContent: "#!/bin/sh\necho stdout\nexit 1", - output: func(string) string { return "stdout\n" }, + desc: "some stdout and empty stderr", + hookContent: "#!/bin/sh\necho stdout\nexit 1", + expectedStdout: "stdout\n", + expectedErr: func(string) string { return "stdout\n" }, }, { - desc: "some stdout and some stderr", - hookContent: "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1", - output: func(string) string { return "stderr\n" }, + desc: "some stdout and some stderr", + hookContent: "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1", + expectedStdout: "stdout\n", + expectedStderr: "stderr\n", + expectedErr: func(string) string { return "stderr\n" }, }, { - desc: "whitespace stdout and some stderr", - hookContent: "#!/bin/sh\necho ' '\necho stderr >&2\nexit 1", - output: func(string) string { return "stderr\n" }, + desc: "whitespace stdout and some stderr", + hookContent: "#!/bin/sh\necho ' '\necho stderr >&2\nexit 1", + expectedStdout: " \n", + expectedStderr: "stderr\n", + expectedErr: func(string) string { return "stderr\n" }, }, { - desc: "some stdout and whitespace stderr", - hookContent: "#!/bin/sh\necho stdout\necho ' ' >&2\nexit 1", - output: func(string) string { return "stdout\n" }, + desc: "some stdout and whitespace stderr", + hookContent: "#!/bin/sh\necho stdout\necho ' ' >&2\nexit 1", + expectedStdout: "stdout\n", + expectedStderr: " \n", + expectedErr: func(string) string { return "stdout\n" }, }, - } - - for _, hookName := range gitlabPreHooks { - for _, testCase := range testCases { - t.Run(hookName+"/"+testCase.desc, func(t *testing.T) { + } { + for _, hookTC := range []struct { + hook string + hookType gitalypb.CustomHookError_HookType + }{ + { + hook: "pre-receive", + hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + }, + { + hook: "update", + hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, + }, + } { + t.Run(hookTC.hook+"/"+tc.desc, func(t *testing.T) { tagNameInput := "some-tag" createRequest := &gitalypb.UserCreateTagRequest{ Repository: repo, @@ -1291,18 +1439,32 @@ func TestTagHookOutput(t *testing.T) { User: gittest.TestUser, } - hookFilename := gittest.WriteCustomHook(t, repoPath, hookName, []byte(testCase.hookContent)) - expectedError := testCase.output(hookFilename) + hookFilename := gittest.WriteCustomHook(t, repoPath, hookTC.hook, []byte(tc.hookContent)) createResponse, err := client.UserCreateTag(ctx, createRequest) - require.NoError(t, err) + if featureflag.UserCreateTagStructuredErrors.IsEnabled(ctx) { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("reference update denied by custom hooks"), + &gitalypb.UserCreateTagError{ + Error: &gitalypb.UserCreateTagError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: hookTC.hookType, + Stdout: []byte(tc.expectedStdout), + Stderr: []byte(tc.expectedStderr), + }, + }, + }, + ), err) + require.Nil(t, createResponse) + } else { + require.NoError(t, err) - createResponseOk := &gitalypb.UserCreateTagResponse{ - Tag: createResponse.Tag, - Exists: false, - PreReceiveError: expectedError, + testhelper.ProtoEqual(t, &gitalypb.UserCreateTagResponse{ + Tag: createResponse.Tag, + Exists: false, + PreReceiveError: tc.expectedErr(hookFilename), + }, createResponse) } - testhelper.ProtoEqual(t, createResponseOk, createResponse) defer gittest.Exec(t, cfg, "-C", repoPath, "tag", "-d", tagNameInput) gittest.Exec(t, cfg, "-C", repoPath, "tag", tagNameInput) @@ -1310,7 +1472,7 @@ func TestTagHookOutput(t *testing.T) { deleteResponse, err := client.UserDeleteTag(ctx, deleteRequest) require.NoError(t, err) deleteResponseOk := &gitalypb.UserDeleteTagResponse{ - PreReceiveError: expectedError, + PreReceiveError: tc.expectedErr(hookFilename), } testhelper.ProtoEqual(t, deleteResponseOk, deleteResponse) }) diff --git a/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go b/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go new file mode 100644 index 000000000..69d1342b5 --- /dev/null +++ b/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go @@ -0,0 +1,9 @@ +package featureflag + +// UserCreateTagStructuredErrors will enable the use of structured errors in the UserCreateTag RPC. +var UserCreateTagStructuredErrors = NewFeatureFlag( + "user_create_tag_structured_errors", + "v15.3.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4372", + false, +) |