operations: Always use structured errors in UserCreateTag RPCpks-remove-user-create-tag-structured-errors-ff

With 0fedeb41f (operations: Introduce structured errors for
UserCreateTag, 2022-07-22) we have adapted the UserCreateTag RPC to
return structured errors instead of embedding errors in a successful
response. This both improves visibility into this RPC and fixes cases
where we needlessly create replication jobs due to missing votes.

This code has been rolled out to production on August 3rd without any
observed negative fallout. Remove the feature flag guarding it.

Changelog: changed

4 files changed, 134 insertions, 310 deletions

diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go
index 7cd8d9efa..ff0b31d59 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,
-)