Merge branch 'pks-remove-user-create-tag-structured-errors-ff' into 'master'

operations: Always use structured errors in UserCreateTag RPC

Closes #4372

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4882

Merged-by: John Cai <jcai@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

4 files changed, 134 insertions, 310 deletions

diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go
index a997282cb..4b7b86d47 100644
--- a/internal/gitaly/server/auth_test.go
+++ b/internal/gitaly/server/auth_test.go
@@ -28,7 +28,6 @@ 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"
@@ -346,10 +345,6 @@ 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 71909dfb1..4fb03d53a 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -14,7 +14,6 @@ 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"
@@ -107,166 +106,102 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 		return nil, err
 	}
 
-	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.
+	// 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 {
+		var notAllowedError hook.NotAllowedError
+		var customHookErr updateref.CustomHookError
+		var updateRefError updateref.Error
+
+		if errors.As(err, &notAllowedError) {
 			detailedErr, err := helper.ErrWithDetails(
-				helper.ErrAlreadyExistsf("tag reference exists already"),
+				helper.ErrPermissionDeniedf("reference update denied by access checks: %w", err),
 				&gitalypb.UserCreateTagError{
-					Error: &gitalypb.UserCreateTagError_ReferenceExists{
-						ReferenceExists: &gitalypb.ReferenceExistsError{
-							ReferenceName: []byte(referenceName.String()),
-							Oid:           oid.String(),
+					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("creating detailed error: %w", err)
+				return nil, helper.ErrInternalf("error details: %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, &notAllowedError) {
-				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(),
-							},
-						},
+		} 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
-			}
-
-			return nil, helper.ErrInternalf("updating reference: %w", err)
-		}
-
-		var customHookErrr updateref.CustomHookError
-		if errors.As(err, &customHookErrr) {
-			// TODO: this case should return a CustomHookError.
-			return &gitalypb.UserCreateTagResponse{
-				PreReceiveError: customHookErrr.Error(),
-			}, nil
-		}
-
-		var updateRefError updateref.Error
-		if errors.As(err, &updateRefError) {
-			refNameOK, err := git.CheckRefFormat(ctx, s.gitCmdFactory, referenceName.String())
+				},
+			)
 			if err != nil {
-				// Should only be reachable if "git
-				// check-ref-format"'s invocation is
-				// incorrect, or if it segfaults on startup
-				// etc.
-				return nil, status.Error(codes.Internal, err.Error())
+				return nil, helper.ErrInternalf("error details: %w", err)
 			}
 
-			if refNameOK {
-				// The tag might not actually exist,
-				// perhaps update-ref died for some
-				// other reason. But saying that it
-				// does is what the Ruby code used to
-				// do, so let's follow it off that
-				// cliff.
-				//
-				// TODO: this case should return a ReferenceExistsError.
-				return &gitalypb.UserCreateTagResponse{
-					Tag:    nil,
-					Exists: true,
-				}, nil
+			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)
 			}
 
-			// It doesn't make sense either to
-			// tell the user to retry with an
-			// invalid ref name, but ditto on the
-			// Ruby bug-for-bug emulation.
-			//
-			// TODO: this case should return a ReferenceUpdateError.
-			return &gitalypb.UserCreateTagResponse{
-				Tag:    nil,
-				Exists: true,
-			}, status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", req.TagName)
+			return nil, detailedErr
 		}
 
-		// The Ruby code did not return this, but always an
-		// "Exists: true" on update-ref failure without any
-		// meaningful error. This is our "PANIC" response, if
-		// we've got an unknown error (it should all be
-		// updateRefError above) let's relay it to the
-		// caller. This should not happen.
-		//
-		// TODO: this case should either return an AccessCheckError in case Rails refused
-		// the update, or alternatively an Internal error.
-		return &gitalypb.UserCreateTagResponse{
-			Tag:    nil,
-			Exists: true,
-		}, status.Error(codes.Internal, err.Error())
+		return nil, helper.ErrInternalf("updating reference: %w", err)
 	}
 
 	return &gitalypb.UserCreateTagResponse{
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 37fa8af95..cb971c65c 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -3,7 +3,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"testing"
@@ -17,7 +16,6 @@ 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"
@@ -132,12 +130,8 @@ func writeAssertObjectTypeUpdateHook(t *testing.T, repoPath, expectedObjectType
 
 func TestUserCreateTag_successful(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagSuccessful)
-}
-
-func testUserCreateTagSuccessful(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(ctx, t, cfg)
@@ -212,12 +206,8 @@ func testUserCreateTagSuccessful(t *testing.T, ctx context.Context) {
 
 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()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	cfg.SocketPath = runOperationServiceServer(t, cfg, testserver.WithDisablePraefect())
 
@@ -332,12 +322,8 @@ func testUserCreateTagTransactional(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_quarantine(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagQuarantine)
-}
-
-func testUserCreateTagQuarantine(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(ctx, t, cfg)
@@ -377,27 +363,18 @@ tagger Jane Doe <janedoe@gitlab.com> 1600000000 +0800
 
 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),
-					},
+	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)
-	}
+		},
+	), err)
+	require.Nil(t, response)
 
 	tagID := text.ChompBytes(testhelper.MustReadFile(t, tagIDOutputPath))
 
@@ -410,12 +387,8 @@ message`, commitID)
 
 func TestUserCreateTag_message(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagMessage)
-}
-
-func testUserCreateTagMessage(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	for _, tc := range []struct {
@@ -510,12 +483,8 @@ func testUserCreateTagMessage(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_targetRevision(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagTargetRevision)
-}
-
-func testUserCreateTagTargetRevision(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	for _, tc := range []struct {
@@ -614,12 +583,8 @@ func testUserCreateTagTargetRevision(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_nonCommitTarget(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagNonCommitTarget)
-}
-
-func testUserCreateTagNonCommitTarget(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)
@@ -711,12 +676,8 @@ func testUserCreateTagNonCommitTarget(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_nestedTags(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagNestedTags)
-}
-
-func testUserCreateTagNestedTags(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)
@@ -830,12 +791,8 @@ func testUserCreateTagNestedTags(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_stableTagIDs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagStableTagIDs)
-}
-
-func testUserCreateTagStableTagIDs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(ctx, t, cfg)
@@ -910,12 +867,8 @@ func TestUserDeleteTag_prefixedTag(t *testing.T) {
 
 func TestUserCreateTag_prefixedTag(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagPrefixedTag)
-}
-
-func testUserCreateTagPrefixedTag(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(ctx, t, cfg)
@@ -950,12 +903,8 @@ func testUserCreateTagPrefixedTag(t *testing.T, ctx context.Context) {
 
 func TestUserCreateTag_gitHooks(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagGitHooks)
-}
-
-func testUserCreateTagGitHooks(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	for _, hookName := range GitlabHooks {
@@ -1096,12 +1045,8 @@ func TestUserDeleteTag_hookFailure(t *testing.T) {
 
 func TestUserCreateTag_hookFailure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagHookFailure)
-}
-
-func testUserCreateTagHookFailure(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	for _, tc := range []struct {
@@ -1131,37 +1076,28 @@ func testUserCreateTagHookFailure(t *testing.T, ctx context.Context) {
 				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",
-								),
-							},
+			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)
-			}
+				},
+			), err)
+			require.Nil(t, response)
 		})
 	}
 }
 
 func TestUserCreateTag_preexisting(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testUserCreateTagPreexisting)
-}
-
-func testUserCreateTagPreexisting(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)
@@ -1181,32 +1117,17 @@ func testUserCreateTagPreexisting(t *testing.T, ctx context.Context) {
 			tagName:        "v1.1.0",
 			targetRevision: commitID.String(),
 			user:           gittest.TestUser,
-			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(),
-							},
+			expectedErr: 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",
@@ -1231,12 +1152,8 @@ func testUserCreateTagPreexisting(t *testing.T, ctx context.Context) {
 
 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)
@@ -1344,12 +1261,8 @@ func testUserCreateTagInvalidArgument(t *testing.T, ctx context.Context) {
 
 func TestTagHookOutput(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserCreateTagStructuredErrors).Run(t, testTagHookOutput)
-}
-
-func testTagHookOutput(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	for _, tc := range []struct {
@@ -1430,29 +1343,19 @@ func testTagHookOutput(t *testing.T, ctx context.Context) {
 				hookFilename := gittest.WriteCustomHook(t, repoPath, hookTC.hook, []byte(tc.hookContent))
 
 				createResponse, err := client.UserCreateTag(ctx, createRequest)
-				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),
-								},
+				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)
-
-					testhelper.ProtoEqual(t, &gitalypb.UserCreateTagResponse{
-						Tag:             createResponse.Tag,
-						Exists:          false,
-						PreReceiveError: tc.expectedErr(hookFilename),
-					}, createResponse)
-				}
+					},
+				), err)
+				require.Nil(t, createResponse)
 
 				defer gittest.Exec(t, cfg, "-C", repoPath, "tag", "-d", tagNameInput)
 				gittest.Exec(t, cfg, "-C", repoPath, "tag", tagNameInput)
diff --git a/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go b/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go
deleted file mode 100644
index 69d1342b5..000000000
--- a/internal/metadata/featureflag/ff_user_create_tag_structured_errors.go
+++ /dev/null
@@ -1,9 +0,0 @@
-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,
-)