Prevent returning HTTP status 499 instead of 5xx (#8865)

* Testing grpc: the client connection is closing Signed-off-by: Yuri Nikolic <[email protected]> * Testing grpc: the client connection is closing Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint Signed-off-by: Yuri Nikolic <[email protected]> * Get rid of mockClient struct Signed-off-by: Yuri Nikolic <[email protected]> * Fix WrapGRPCErrorWithContextError() Signed-off-by: Yuri Nikolic <[email protected]> * Update CHANGELOG.md Signed-off-by: Yuri Nikolic <[email protected]> * Refactoring Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint issues Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findigs Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findigs Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findigs Signed-off-by: Yuri Nikolic <[email protected]> * Update pkg/util/globalerror/grpc_test.go Co-authored-by: Oleg Zaytsev <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> Co-authored-by: Oleg Zaytsev <[email protected]>
grafana · Aug 2, 2024 · cbab4cb · cbab4cb
1 parent 25c938c
commit cbab4cb
Show file tree

Hide file tree

Showing 4 changed files with 113 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -75,6 +75,7 @@
 * [BUGFIX] Query-frontend: Ensure that internal errors result in an HTTP 500 response code instead of 422. #8595 #8666
 * [BUGFIX] Configuration: Multi line envs variables are flatten during injection to be compatible with YAML syntax
 * [BUGFIX] Querier: fix issue where queries can return incorrect results if a single store-gateway returns overlapping chunks for a series. #8827
+* [BUGFIX] Querier: do not return `grpc: the client connection is closing` errors as HTTP `499`. #8865
 
 ### Mixin
 

diff --git a/go.mod b/go.mod
@@ -16,7 +16,7 @@ require (
 	github.com/go-openapi/swag v0.23.0
 	github.com/gogo/protobuf v1.3.2
 	github.com/gogo/status v1.1.1
-	github.com/golang/protobuf v1.5.4 // indirect
+	github.com/golang/protobuf v1.5.4
 	github.com/golang/snappy v0.0.4
 	github.com/google/gopacket v1.1.19
 	github.com/gorilla/mux v1.8.1

diff --git a/pkg/util/globalerror/grpc.go b/pkg/util/globalerror/grpc.go
@@ -7,12 +7,23 @@ import (
 
 	"github.com/gogo/status"
 	"github.com/grafana/dskit/grpcutil"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	grpcstatus "google.golang.org/grpc/status"
 
 	"github.com/grafana/mimir/pkg/mimirpb"
 )
 
+var grpcClientConnectionIsClosingErr string
+
+func init() {
+	// Ignore deprecation warning for now
+	//nolint:staticcheck
+	if stat, ok := grpcutil.ErrorToStatus(grpc.ErrClientConnClosing); ok && stat.Code() == codes.Canceled {
+		grpcClientConnectionIsClosingErr = stat.Message()
+	}
+}
+
 // WrapGRPCErrorWithContextError checks if the given error is a gRPC error corresponding
 // to a standard golang context error, and if it is, wraps the former with the latter.
 // If the given error isn't a gRPC error, or it doesn't correspond to a standard golang
@@ -24,10 +35,12 @@ func WrapGRPCErrorWithContextError(err error) error {
 	if stat, ok := grpcutil.ErrorToStatus(err); ok {
 		switch stat.Code() {
 		case codes.Canceled:
-			return &ErrorWithStatus{
-				UnderlyingErr: err,
-				Status:        stat,
-				ctxErr:        context.Canceled,
+			if stat.Message() != grpcClientConnectionIsClosingErr {
+				return &ErrorWithStatus{
+					UnderlyingErr: err,
+					Status:        stat,
+					ctxErr:        context.Canceled,
+				}
 			}
 		case codes.DeadlineExceeded:
 			return &ErrorWithStatus{

diff --git a/pkg/util/globalerror/grpc_test.go b/pkg/util/globalerror/grpc_test.go
@@ -5,15 +5,21 @@ package globalerror
 import (
 	"context"
 	"io"
+	"net"
 	"testing"
 
 	"github.com/gogo/status"
+	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/grafana/dskit/flagext"
+	"github.com/grafana/dskit/grpcclient"
 	"github.com/grafana/dskit/grpcutil"
 	"github.com/grafana/dskit/httpgrpc"
 	"github.com/grafana/dskit/middleware"
+	dskitserver "github.com/grafana/dskit/server"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	grpcstatus "google.golang.org/grpc/status"
 
@@ -77,16 +83,24 @@ func TestWrapContextError(t *testing.T) {
 				expectedGrpcCode:   codes.DeadlineExceeded,
 				expectedContextErr: context.DeadlineExceeded,
 			},
+			"grpc.ErrClientConnClosing": {
+				origErr:            status.Error(codes.Canceled, "grpc: the client connection is closing"),
+				expectedGrpcCode:   codes.Canceled,
+				expectedContextErr: nil,
+			},
 		}
 
 		for testName, testData := range tests {
 			t.Run(testName, func(t *testing.T) {
 				wrapped := WrapGRPCErrorWithContextError(testData.origErr)
-
-				assert.NotEqual(t, testData.origErr, wrapped)
 				assert.ErrorIs(t, wrapped, testData.origErr)
 
-				assert.True(t, errors.Is(wrapped, testData.expectedContextErr))
+				if testData.expectedContextErr != nil {
+					assert.ErrorIs(t, wrapped, testData.expectedContextErr)
+					assert.NotEqual(t, testData.origErr, wrapped)
+				} else {
+					assert.Equal(t, testData.origErr, wrapped)
+				}
 				assert.Equal(t, testData.expectedGrpcCode, grpcutil.ErrorToStatusCode(wrapped))
 
 				//lint:ignore faillint We want to explicitly assert on status.FromError()
@@ -284,3 +298,80 @@ func checkErrorWithStatusDetails(t *testing.T, details []any, expected *mimirpb.
 		require.Equal(t, expected, errDetails)
 	}
 }
+
+func TestGRPCClientClosingConnectionError_IsNotContextCanceled(t *testing.T) {
+	ctx := context.Background()
+
+	_, client, cc := prepareTest(t)
+
+	// Calls to Succeed() should be successful when cc is open.
+	_, err := client.Succeed(ctx, nil)
+	require.NoError(t, err)
+
+	// We close cc.
+	err = cc.Close()
+	require.NoError(t, err)
+
+	// Calls to Succeed() should fail with "grpc: the client connection is closing" when cc is closed.
+	_, err = client.Succeed(ctx, nil)
+	require.Error(t, err)
+	require.NotErrorIs(t, err, context.Canceled)
+
+	wrapErr := WrapGRPCErrorWithContextError(err)
+	require.Error(t, wrapErr)
+	require.NotErrorIs(t, wrapErr, context.Canceled)
+	checkGRPCConnectionIsClosingError(t, err)
+}
+
+func prepareTest(t *testing.T) (dskitserver.FakeServerServer, dskitserver.FakeServerClient, *grpc.ClientConn) {
+	grpcServer := grpc.NewServer()
+	t.Cleanup(grpcServer.GracefulStop)
+
+	server := &mockServer{}
+	dskitserver.RegisterFakeServerServer(grpcServer, server)
+
+	listener, err := net.Listen("tcp", "localhost:0")
+	require.NoError(t, err)
+
+	go func() {
+		require.NoError(t, grpcServer.Serve(listener))
+	}()
+
+	// Create a real gRPC client connecting to the gRPC server we control in this test.
+	clientCfg := grpcclient.Config{}
+	flagext.DefaultValues(&clientCfg)
+
+	opts, err := clientCfg.DialOption(nil, nil)
+	require.NoError(t, err)
+
+	cc, err := grpc.NewClient(listener.Addr().String(), opts...)
+	require.NoError(t, err)
+
+	client := dskitserver.NewFakeServerClient(cc)
+
+	// This is another source of "grpc: the client connection is closing",
+	// because at this point the connection is already closed.
+	t.Cleanup(func() {
+		err := cc.Close()
+		require.Error(t, err)
+		require.NotErrorIs(t, err, context.Canceled)
+		checkGRPCConnectionIsClosingError(t, err)
+	})
+
+	return server, client, cc
+}
+
+func checkGRPCConnectionIsClosingError(t *testing.T, err error) {
+	stat, ok := grpcutil.ErrorToStatus(err)
+	require.True(t, ok)
+	require.Equal(t, codes.Canceled, stat.Code())
+	require.Equal(t, "grpc: the client connection is closing", stat.Message())
+}
+
+type mockServer struct {
+	dskitserver.UnimplementedFakeServerServer
+}
+
+func (s *mockServer) Succeed(_ context.Context, _ *empty.Empty) (*empty.Empty, error) {
+	return nil, nil
+}