limithandler: Do not wrap errors from limithandlerjc-do-not-wrap-detailed-error

In the limithandlers, we are returning structured errors so upstream
clients can interpret that a request limit took place. However, we are
currently swallowing these details by wrapping it in another gRPC error.
Instead, just return this error without wrapping it, since the
structured error already provides enough context as to why the error
happened.

Changelog: changed

2 files changed, 15 insertions, 8 deletions

diff --git a/internal/middleware/limithandler/middleware.go b/internal/middleware/limithandler/middleware.go
index 7ee2aa37f..1b469031d 100644
--- a/internal/middleware/limithandler/middleware.go
+++ b/internal/middleware/limithandler/middleware.go
@@ -6,7 +6,6 @@ import (
 	grpcmwtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"google.golang.org/grpc"
 )
 
@@ -164,6 +163,6 @@ func (w *wrappedStream) RecvMsg(m interface{}) error {
 		// It's our turn!
 		return nil
 	case err := <-errs:
-		return helper.ErrInternalf("rate limiting stream request: %v", err)
+		return err
 	}
 }
diff --git a/internal/middleware/limithandler/middleware_test.go b/internal/middleware/limithandler/middleware_test.go
index c2ff5bb8d..b098324ef 100644
--- a/internal/middleware/limithandler/middleware_test.go
+++ b/internal/middleware/limithandler/middleware_test.go
@@ -13,13 +13,13 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/middleware/limithandler"
 	pb "gitlab.com/gitlab-org/gitaly/v14/internal/middleware/limithandler/testdata"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
 )
@@ -285,11 +285,19 @@ func TestStreamLimitHandler_error(t *testing.T) {
 	}
 
 	err := <-errChan
-	testhelper.RequireGrpcError(
-		t,
-		helper.ErrInternalf("rate limiting stream request: %w",
-			limithandler.ErrMaxQueueSize),
-		err)
+	testhelper.RequireGrpcCode(t, err, codes.Unavailable)
+	// ensure it is a structured error
+	st, ok := status.FromError(err)
+	require.True(t, ok)
+
+	var errFound bool
+	for _, detail := range st.Details() {
+		switch detail.(type) {
+		case *gitalypb.LimitError:
+			errFound = true
+		}
+	}
+	assert.True(t, errFound)
 
 	// allow the first request to finish
 	close(s.blockCh)