diff options
| author | John Cai <jcai@gitlab.com> | 2022-05-06 22:00:57 +0300 |
|---|---|---|
| committer | John Cai <jcai@gitlab.com> | 2022-05-09 15:38:58 +0300 |
| commit | e01b8fa0334367eccba423a4f8815b9d6e733dc7 (patch) | |
| tree | 034a00e3af739f0ced166872e59f1c5182f99830 | |
| parent | 2e30abfa61112d353f2474ab41837882b78e5d1a (diff) | |
limithandler: Do not wrap errors from limithandler
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
| -rw-r--r-- | internal/middleware/limithandler/middleware.go | 3 | ||||
| -rw-r--r-- | internal/middleware/limithandler/middleware_test.go | 16 |
2 files changed, 11 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..729b76626 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,15 @@ 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) + + testhelper.ProtoEqual(t, []interface{}{&gitalypb.LimitError{ + ErrorMessage: "maximum queue size reached", + RetryAfter: &durationpb.Duration{}, + }}, st.Details()) // allow the first request to finish close(s.blockCh) |