limithandler: Return structured errorsjc-limithandler-return-structured-limits

In order to alert upstream clients that Gitaly has reached capacity,
return a structured error of gitalypb.LimitError that clients like
workhorse, gitlab-shell, and rails can parse and take action on.

These errors all have the gRPC status Unavailable.

Changelog: changed

4 files changed, 115 insertions, 6 deletions

diff --git a/internal/middleware/limithandler/concurrency_limiter.go b/internal/middleware/limithandler/concurrency_limiter.go
index f6587554a..bec438b25 100644
--- a/internal/middleware/limithandler/concurrency_limiter.go
+++ b/internal/middleware/limithandler/concurrency_limiter.go
@@ -7,10 +7,13 @@ import (
 	"sync"
 	"time"
 
+	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/prometheus/client_golang/prometheus"
 	"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/proto/go/gitalypb"
+	"google.golang.org/protobuf/types/known/durationpb"
 )
 
 // ErrMaxQueueTime indicates a request has reached the maximum time allowed to wait in the
@@ -146,7 +149,27 @@ func (c *ConcurrencyLimiter) Limit(ctx context.Context, lockKey string, f Limite
 
 	var decremented bool
 
+	log := ctxlogrus.Extract(ctx)
 	if err := c.queueInc(ctx); err != nil {
+		if errors.Is(err, ErrMaxQueueSize) {
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrUnavailable(ErrMaxQueueSize),
+				&gitalypb.LimitError{
+					ErrorMessage: err.Error(),
+					RetryAfter:   durationpb.New(0),
+				},
+			)
+			if errGeneratingDetailedErr != nil {
+				log.WithField("max_queue_size_error", err).
+					WithError(errGeneratingDetailedErr).
+					Error("failed to generate detailed error")
+
+				return nil, helper.ErrUnavailable(ErrMaxQueueSize)
+			}
+
+			return nil, detailedErr
+		}
+
 		return nil, err
 	}
 	defer c.queueDec(&decremented)
@@ -164,7 +187,25 @@ func (c *ConcurrencyLimiter) Limit(ctx context.Context, lockKey string, f Limite
 	if err != nil {
 		if errors.Is(err, ErrMaxQueueTime) {
 			c.monitor.Dropped(ctx, "max_time")
+
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrUnavailable(ErrMaxQueueTime),
+				&gitalypb.LimitError{
+					ErrorMessage: err.Error(),
+					RetryAfter:   durationpb.New(0),
+				},
+			)
+			if errGeneratingDetailedErr != nil {
+				log.WithField("max_queue_wait_error", err).
+					WithError(errGeneratingDetailedErr).
+					Error("failed to generate detailed error")
+
+				return nil, helper.ErrUnavailable(ErrMaxQueueTime)
+			}
+
+			return nil, detailedErr
 		}
+
 		return nil, err
 	}
 	defer sem.release()
diff --git a/internal/middleware/limithandler/concurrency_limiter_test.go b/internal/middleware/limithandler/concurrency_limiter_test.go
index 4a13a3cd4..6c19fec2b 100644
--- a/internal/middleware/limithandler/concurrency_limiter_test.go
+++ b/internal/middleware/limithandler/concurrency_limiter_test.go
@@ -12,6 +12,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/types/known/durationpb"
 )
 
 type counter struct {
@@ -313,7 +316,17 @@ func TestConcurrencyLimiter_queueLimit(t *testing.T) {
 			if tc.featureFlagOn {
 				err := <-errChan
 				assert.Error(t, err)
-				assert.Equal(t, "maximum queue size reached", err.Error())
+
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				details := s.Details()
+				require.Len(t, details, 1)
+
+				limitErr, ok := details[0].(*gitalypb.LimitError)
+				require.True(t, ok)
+
+				assert.Equal(t, ErrMaxQueueSize.Error(), limitErr.ErrorMessage)
+				assert.Equal(t, durationpb.New(0), limitErr.RetryAfter)
 				assert.Equal(t, monitor.droppedSize, 1)
 			} else {
 				<-monitorCh
@@ -391,7 +404,17 @@ func TestLimitConcurrency_queueWaitTime(t *testing.T) {
 		<-dequeuedCh
 		err := <-errChan
 
-		assert.Equal(t, ErrMaxQueueTime, err)
+		s, ok := status.FromError(err)
+		require.True(t, ok)
+		details := s.Details()
+		require.Len(t, details, 1)
+
+		limitErr, ok := details[0].(*gitalypb.LimitError)
+		require.True(t, ok)
+
+		assert.Equal(t, ErrMaxQueueTime.Error(), limitErr.ErrorMessage)
+		assert.Equal(t, durationpb.New(0), limitErr.RetryAfter)
+
 		assert.Equal(t, monitor.droppedTime, 1)
 		close(ch)
 		wg.Wait()
diff --git a/internal/middleware/limithandler/middleware_test.go b/internal/middleware/limithandler/middleware_test.go
index 53f0f53f2..9bba3d049 100644
--- a/internal/middleware/limithandler/middleware_test.go
+++ b/internal/middleware/limithandler/middleware_test.go
@@ -16,9 +16,10 @@ import (
 	"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"
 )
 
 func TestMain(m *testing.M) {
@@ -285,7 +286,17 @@ func TestConcurrencyLimitHandlerMetrics(t *testing.T) {
 
 	var errs int
 	for err := range errChan {
-		testhelper.RequireGrpcError(t, limithandler.ErrMaxQueueSize, err)
+		s, ok := status.FromError(err)
+		require.True(t, ok)
+		details := s.Details()
+		require.Len(t, details, 1)
+
+		limitErr, ok := details[0].(*gitalypb.LimitError)
+		require.True(t, ok)
+
+		assert.Equal(t, limithandler.ErrMaxQueueSize.Error(), limitErr.ErrorMessage)
+		assert.Equal(t, durationpb.New(0), limitErr.RetryAfter)
+
 		errs++
 		if errs == 9 {
 			break
@@ -357,7 +368,17 @@ func testRateLimitHandler(t *testing.T, ctx context.Context) {
 			_, err := client.Unary(ctx, &pb.UnaryRequest{})
 
 			if featureflag.RateLimit.IsEnabled(ctx) {
-				testhelper.RequireGrpcError(t, status.Error(codes.Unavailable, "too many requests"), err)
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				details := s.Details()
+				require.Len(t, details, 1)
+
+				limitErr, ok := details[0].(*gitalypb.LimitError)
+				require.True(t, ok)
+
+				assert.Equal(t, limithandler.ErrRateLimit.Error(), limitErr.ErrorMessage)
+				assert.Equal(t, durationpb.New(0), limitErr.RetryAfter)
+
 			} else {
 				require.NoError(t, err)
 			}
diff --git a/internal/middleware/limithandler/rate_limiter.go b/internal/middleware/limithandler/rate_limiter.go
index f47bb5409..b93727725 100644
--- a/internal/middleware/limithandler/rate_limiter.go
+++ b/internal/middleware/limithandler/rate_limiter.go
@@ -10,7 +10,10 @@ import (
 	"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/proto/go/gitalypb"
+	"gitlab.com/gitlab-org/labkit/log"
 	"golang.org/x/time/rate"
+	"google.golang.org/protobuf/types/known/durationpb"
 )
 
 // RateLimiter is an implementation of Limiter that puts a hard limit on the
@@ -23,6 +26,10 @@ type RateLimiter struct {
 	ticker                           helper.Ticker
 }
 
+// ErrRateLimit is returned when RateLimiter determined a request has breached
+// the rate request limit.
+var ErrRateLimit = errors.New("rate limit reached")
+
 // Limit rejects an incoming reequest if the maximum number of requests per
 // second has been reached
 func (r *RateLimiter) Limit(ctx context.Context, lockKey string, f LimitedFunc) (interface{}, error) {
@@ -37,7 +44,24 @@ func (r *RateLimiter) Limit(ctx context.Context, lockKey string, f LimitedFunc)
 		// of traffic.
 		r.requestsDroppedMetric.Inc()
 		if featureflag.RateLimit.IsEnabled(ctx) {
-			return nil, helper.ErrUnavailable(errors.New("too many requests"))
+			err := helper.ErrUnavailable(ErrRateLimit)
+
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				err,
+				&gitalypb.LimitError{
+					ErrorMessage: ErrRateLimit.Error(),
+					RetryAfter:   durationpb.New(0),
+				},
+			)
+			if errGeneratingDetailedErr != nil {
+				log.WithField("rate_limit_error", err).
+					WithError(errGeneratingDetailedErr).
+					Error("failed to generate detailed error")
+
+				return nil, err
+			}
+
+			return nil, detailedErr
 		}
 	}