branches: Use 'UserCreateBranchError' behind a flag4227-convert-usercreatebranch-to-use-structured-errors

This commit utilizes the newly added 'UserCreateBranchError' in the
'UserCreateBranch' function. Since this replaces the old behavior and is
a breaking change because the old behavior would return StatusOk with
the error ingrained, we add this functionality behind a feature flag
introduced in the previous commit (1117a3665).

2 files changed, 102 insertions, 19 deletions

diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index 2e99a6547..adaa0442d 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -8,6 +8,7 @@ import (
 	"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"
@@ -59,10 +60,32 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, startPointOID, git.ObjectHashSHA1.ZeroOID); err != nil {
 		var customHookErr updateref.CustomHookError
+
 		if errors.As(err, &customHookErr) {
-			return &gitalypb.UserCreateBranchResponse{
-				PreReceiveError: customHookErr.Error(),
-			}, nil
+			if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) {
+				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("creation denied by custom hooks"),
+					&gitalypb.UserCreateBranchError{
+						Error: &gitalypb.UserCreateBranchError_CustomHook{
+							CustomHook: customHookErr.Proto(),
+						},
+					},
+				)
+				if err != nil {
+					return nil, helper.ErrInternalf("error details: %w", err)
+				}
+
+				return nil, detailedErr
+			} else {
+				return &gitalypb.UserCreateBranchResponse{
+					PreReceiveError: customHookErr.Error(),
+				}, nil
+			}
 		}
 
 		var updateRefError updateref.Error
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index b07fb5060..bea777110 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -18,6 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"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"
@@ -42,7 +43,11 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp
 
 func TestUserCreateBranch_successful(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchSuccessful)
+}
+
+func testUserCreateBranchSuccessful(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -117,9 +122,13 @@ func TestUserCreateBranch_successful(t *testing.T) {
 	}
 }
 
-func TestUserCreateBranch_transactions(t *testing.T) {
+func TestUserCreateBranch_Transactions(t *testing.T) {
+	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchTransactions)
+}
+
+func testUserCreateBranchTransactions(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
@@ -193,7 +202,11 @@ func TestUserCreateBranch_transactions(t *testing.T) {
 
 func TestUserCreateBranch_hook(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchHook)
+}
+
+func testUserCreateBranchHook(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -223,7 +236,11 @@ func TestUserCreateBranch_hook(t *testing.T) {
 
 func TestUserCreateBranch_startPoint(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchStartPoint)
+}
+
+func testUserCreateBranchStartPoint(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -296,7 +313,11 @@ func TestUserCreateBranch_startPoint(t *testing.T) {
 
 func TestUserCreateBranch_hookFailure(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchHookFailure)
+}
+
+func testUserCreateBranchHookFailure(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, _, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -306,22 +327,42 @@ func TestUserCreateBranch_hookFailure(t *testing.T) {
 		StartPoint: []byte("c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd"),
 		User:       gittest.TestUser,
 	}
-	// Write a hook that will fail with the environment as the error message
-	// so we can check that string for our env variables.
-	hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1")
+
+	hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1")
+
+	expectedObject := "GL_ID=" + gittest.TestUser.GlId
 
 	for _, hookName := range gitlabPreHooks {
 		gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
 
 		response, err := client.UserCreateBranch(ctx, request)
-		require.Nil(t, err)
-		require.Contains(t, response.PreReceiveError, "GL_USERNAME="+gittest.TestUser.GlUsername)
+
+		if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) {
+			testhelper.RequireGrpcError(t, errWithDetails(t,
+				helper.ErrPermissionDeniedf("creation denied by custom hooks"),
+				&gitalypb.UserCreateBranchError{
+					Error: &gitalypb.UserCreateBranchError_CustomHook{
+						CustomHook: &gitalypb.CustomHookError{
+							HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
+							Stdout:   []byte(expectedObject + "\n"),
+						},
+					},
+				},
+			), err)
+		} else {
+			require.Nil(t, err)
+			require.Contains(t, response.PreReceiveError, expectedObject)
+		}
 	}
 }
 
-func TestUserCreateBranch_failure(t *testing.T) {
+func TestUserCreateBranch_Failure(t *testing.T) {
+	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchFailure)
+}
+
+func testUserCreateBranchFailure(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
@@ -747,8 +788,11 @@ func TestUserDeleteBranch_hookFailure(t *testing.T) {
 
 func TestBranchHookOutput(t *testing.T) {
 	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testBranchHookOutput)
+}
 
-	ctx := testhelper.Context(t)
+func testBranchHookOutput(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -833,8 +877,24 @@ func TestBranchHookOutput(t *testing.T) {
 				expectedError := testCase.output(hookFilename)
 
 				createResponse, err := client.UserCreateBranch(ctx, createRequest)
-				require.NoError(t, err)
-				require.Equal(t, expectedError, createResponse.PreReceiveError)
+
+				if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) {
+					testhelper.RequireGrpcError(t, errWithDetails(t,
+						helper.ErrPermissionDeniedf("creation denied by custom hooks"),
+						&gitalypb.UserCreateBranchError{
+							Error: &gitalypb.UserCreateBranchError_CustomHook{
+								CustomHook: &gitalypb.CustomHookError{
+									HookType: hookTestCase.hookType,
+									Stdout:   []byte(testCase.expectedStdout),
+									Stderr:   []byte(testCase.expectedStderr),
+								},
+							},
+						},
+					), err)
+				} else {
+					require.NoError(t, err)
+					require.Equal(t, expectedError, createResponse.PreReceiveError)
+				}
 
 				gittest.Exec(t, cfg, "-C", repoPath, "branch", branchNameInput)
 				defer gittest.Exec(t, cfg, "-C", repoPath, "branch", "-d", branchNameInput)