Allow server.Server to return internal gRPC errors with HTTPResponses

httpgrpc/httpgrpc.go

@@ -15,6 +15,7 @@ import (
 	spb "github.com/gogo/googleapis/google/rpc"
 	"github.com/gogo/protobuf/types"
 	"github.com/gogo/status"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 
 	"github.com/grafana/dskit/grpcutil"
@@ -104,8 +105,17 @@ func FromHeader(hs http.Header) []*Header {
 	return result
 }
 
-// Errorf returns a HTTP gRPC error than is correctly forwarded over
-// gRPC, and can eventually be converted back to a HTTP response with
+// InternalErrorf returns a gRPC error with code codes.Internal, that is correctly forwarded over
+// gRPC, and can eventually be converted back to an HTTP response with HTTPResponseFromError.
+func InternalErrorf(code int, tmpl string, args ...interface{}) error {
+	return InternalErrorFromHTTPResponse(&HTTPResponse{
+		Code: int32(code),
+		Body: []byte(fmt.Sprintf(tmpl, args...)),
+	})
+}
+
+// Errorf returns an HTTP gRPC error than is correctly forwarded over
+// gRPC, and can eventually be converted back to an HTTP response with
 // HTTPResponseFromError.
 func Errorf(code int, tmpl string, args ...interface{}) error {
 	return ErrorFromHTTPResponse(&HTTPResponse{
@@ -114,15 +124,24 @@ func Errorf(code int, tmpl string, args ...interface{}) error {
 	})
 }
 
-// ErrorFromHTTPResponse converts an HTTP response into a grpc error
+// InternalErrorFromHTTPResponse converts an HTTP response into an internal gRPC error.
+func InternalErrorFromHTTPResponse(resp *HTTPResponse) error {
+	return errorFromHTTPResponse(int32(codes.Internal), resp)
+}
+
+// ErrorFromHTTPResponse converts an HTTP response into a grpc error.
 func ErrorFromHTTPResponse(resp *HTTPResponse) error {
+	return errorFromHTTPResponse(resp.Code, resp)
+}
+
+func errorFromHTTPResponse(code int32, resp *HTTPResponse) error {
 	a, err := types.MarshalAny(resp)
 	if err != nil {
 		return err
 	}
 
 	return status.ErrorProto(&spb.Status{
-		Code:    resp.Code,
+		Code:    code,
 		Message: string(resp.Body),
 		Details: []*types.Any{a},
 	})

httpgrpc/httpgrpc_test.go

@@ -30,23 +30,49 @@ func TestAppendMessageSizeToOutgoingContext(t *testing.T) {
 }
 
 func TestErrorf(t *testing.T) {
-	code := 400
-	errMsg := "this is an error"
+	const (
+		code   = 400
+		errMsg = "this is an error"
+	)
 	expectedHTTPResponse := &HTTPResponse{
 		Code: int32(code),
 		Body: []byte(errMsg),
 	}
 	err := Errorf(code, errMsg)
+	require.Error(t, err)
+
 	stat, ok := status.FromError(err)
 	require.True(t, ok)
 	require.Equal(t, code, int(stat.Code()))
 	require.Equal(t, errMsg, stat.Message())
 	checkDetailAsHTTPResponse(t, expectedHTTPResponse, stat)
 }
 
+func TestInternalErrorf(t *testing.T) {
+	const (
+		code   = 400
+		errMsg = "this is an error"
+	)
+	expectedHTTPResponse := &HTTPResponse{
+		Code: int32(code),
+		Body: []byte(errMsg),
+	}
+
+	err := InternalErrorf(code, errMsg)
+	require.Error(t, err)
+
+	stat, ok := status.FromError(err)
+	require.True(t, ok)
+	require.Equal(t, codes.Internal, stat.Code())
+	require.Equal(t, errMsg, stat.Message())
+	checkDetailAsHTTPResponse(t, expectedHTTPResponse, stat)
+}
+
 func TestErrorFromHTTPResponse(t *testing.T) {
-	var code int32 = 400
-	errMsg := "this is an error"
+	const (
+		code   int32 = 400
+		errMsg       = "this is an error"
+	)
 	headers := []*Header{{Key: "X-Header", Values: []string{"a", "b", "c"}}}
 	resp := &HTTPResponse{
 		Code:    code,
@@ -62,8 +88,29 @@ func TestErrorFromHTTPResponse(t *testing.T) {
 	checkDetailAsHTTPResponse(t, resp, stat)
 }
 
+func TestInternalErrorFromHTTPResponse(t *testing.T) {
+	const (
+		code   int32 = 400
+		errMsg       = "this is an error"
+	)
+	headers := []*Header{{Key: "X-Header", Values: []string{"a", "b", "c"}}}
+	resp := &HTTPResponse{
+		Code:    code,
+		Headers: headers,
+		Body:    []byte(errMsg),
+	}
+	err := InternalErrorFromHTTPResponse(resp)
+	require.Error(t, err)
+	stat, ok := status.FromError(err)
+	require.True(t, ok)
+	require.Equal(t, codes.Internal, stat.Code())
+	require.Equal(t, errMsg, stat.Message())
+	checkDetailAsHTTPResponse(t, resp, stat)
+}
+
 func TestHTTPResponseFromError(t *testing.T) {
-	msgErr := "this is an error"
+	const msgErr = "this is an error"
+	var resp = &HTTPResponse{Code: 400, Body: []byte(msgErr)}
 	testCases := map[string]struct {
 		err                  error
 		isGRPCError          bool
@@ -84,11 +131,19 @@ func TestHTTPResponseFromError(t *testing.T) {
 		},
 		"a gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
 			err:                  Errorf(400, msgErr),
-			expectedHTTPResponse: &HTTPResponse{Code: 400, Body: []byte(msgErr)},
+			expectedHTTPResponse: resp,
 		},
 		"a wrapped gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
 			err:                  fmt.Errorf("wrapped: %w", Errorf(400, msgErr)),
-			expectedHTTPResponse: &HTTPResponse{Code: 400, Body: []byte(msgErr)},
+			expectedHTTPResponse: resp,
+		},
+		"an internal gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
+			err:                  InternalErrorf(400, msgErr),
+			expectedHTTPResponse: resp,
+		},
+		"a wrapped internal gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
+			err:                  fmt.Errorf("wrapped: %w", InternalErrorf(400, msgErr)),
+			expectedHTTPResponse: resp,
 		},
 	}
 	for testName, testData := range testCases {
@@ -99,7 +154,7 @@ func TestHTTPResponseFromError(t *testing.T) {
 				require.Nil(t, resp)
 			} else {
 				require.True(t, ok)
-
+				checkEqualHTTPResponses(t, testData.expectedHTTPResponse, resp)
 			}
 		})
 	}
@@ -111,7 +166,11 @@ func checkDetailAsHTTPResponse(t *testing.T, httpResponse *HTTPResponse, stat *s
 	respDetails, ok := details[0].(*HTTPResponse)
 	require.True(t, ok)
 	require.NotNil(t, respDetails)
-	require.Equal(t, httpResponse.Code, respDetails.Code)
-	require.Equal(t, httpResponse.Headers, respDetails.Headers)
-	require.Equal(t, httpResponse.Body, respDetails.Body)
+	checkEqualHTTPResponses(t, httpResponse, respDetails)
+}
+
+func checkEqualHTTPResponses(t *testing.T, expectedResp, resp *HTTPResponse) {
+	require.Equal(t, expectedResp.GetCode(), resp.GetCode())
+	require.Equal(t, expectedResp.GetHeaders(), resp.GetHeaders())
+	require.Equal(t, expectedResp.GetBody(), resp.GetBody())
 }

httpgrpc/server/server.go

@@ -38,6 +38,10 @@ func WithReturn4XXErrors(s *Server) {
 	s.return4XXErrors = true
 }
 
+func WithSendOverGRPCAsInternal(s *Server) {
+	s.sendOverGRPCAsInternal = true
+}
+
 func applyServerOptions(s *Server, opts ...Option) *Server {
 	for _, opt := range opts {
 		opt(s)
@@ -48,8 +52,9 @@ func applyServerOptions(s *Server, opts ...Option) *Server {
 // Server implements HTTPServer.  HTTPServer is a generated interface that gRPC
 // servers must implement.
 type Server struct {
-	handler         http.Handler
-	return4XXErrors bool
+	handler                http.Handler
+	return4XXErrors        bool
+	sendOverGRPCAsInternal bool
 }
 
 // NewServer makes a new Server.
@@ -80,7 +85,7 @@ func (s Server) Handle(ctx context.Context, r *httpgrpc.HTTPRequest) (*httpgrpc.
 		Body:    recorder.Body.Bytes(),
 	}
 	if s.shouldReturnError(resp) {
-		err := httpgrpc.ErrorFromHTTPResponse(resp)
+		err := s.errorFromResponse(resp)
 		if doNotLogError {
 			err = middleware.DoNotLogError{Err: err}
 		}
@@ -89,6 +94,13 @@ func (s Server) Handle(ctx context.Context, r *httpgrpc.HTTPRequest) (*httpgrpc.
 	return resp, nil
 }
 
+func (s Server) errorFromResponse(resp *httpgrpc.HTTPResponse) error {
+	if s.sendOverGRPCAsInternal {
+		return httpgrpc.InternalErrorFromHTTPResponse(resp)
+	}
+	return httpgrpc.ErrorFromHTTPResponse(resp)
+}
+
 func (s Server) shouldReturnError(resp *httpgrpc.HTTPResponse) bool {
 	mask := resp.GetCode() / 100
 	return mask == 5 || (s.return4XXErrors && mask == 4)

httpgrpc/server/server_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/grafana/dskit/user"
 )
 
-func TestReturn4XXErrorsOption(t *testing.T) {
+func TestWithReturn4XXErrors(t *testing.T) {
 	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		_, err := fmt.Fprint(w, "test")
 		require.NoError(t, err)
@@ -41,6 +41,22 @@ func TestReturn4XXErrorsOption(t *testing.T) {
 	require.True(t, server.return4XXErrors)
 }
 
+func TestWithHTTPErrorsEnabled(t *testing.T) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, err := fmt.Fprint(w, "test")
+		require.NoError(t, err)
+	})
+	serverOptions := make([]Option, 0, 1)
+	server := NewServer(handler, serverOptions...)
+	require.NotNil(t, server)
+	require.False(t, server.sendOverGRPCAsInternal)
+
+	serverOptions = append(serverOptions, WithSendOverGRPCAsInternal)
+	server = NewServer(handler, serverOptions...)
+	require.NotNil(t, server)
+	require.True(t, server.sendOverGRPCAsInternal)
+}
+
 type testServer struct {
 	*Server
 	URL        string
@@ -252,6 +268,55 @@ func TestServerHandleReturn4XXErrors(t *testing.T) {
 	}
 }
 
+func TestServerHandleWithErrors(t *testing.T) {
+	testCases := map[string]struct {
+		errorCode      int
+		withHTTPErrors bool
+		expectedError  bool
+	}{
+		"HTTPResponse with code 5xx should return the same code when server created with WithSendOverGRPCAsInternal": {
+			errorCode:      http.StatusInternalServerError,
+			withHTTPErrors: true,
+		},
+		"HTTPResponse with code 5xx should return the same code when server created without WithSendOverGRPCAsInternal": {
+			errorCode:      http.StatusInternalServerError,
+			withHTTPErrors: false,
+		},
+		"HTTPResponse with code 4xx should return the same code when server created with WithSendOverGRPCAsInternal": {
+			errorCode:      http.StatusUnprocessableEntity,
+			withHTTPErrors: true,
+		},
+		"HTTPResponse with code 4xx should return the same code when server created without WithSendOverGRPCAsInternal": {
+			errorCode:      http.StatusUnprocessableEntity,
+			withHTTPErrors: false,
+		},
+	}
+	errMsg := "this is an error"
+	for testName, testData := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				http.Error(w, errMsg, testData.errorCode)
+			})
+
+			serverOptions := make([]Option, 0, 2)
+			serverOptions = append(serverOptions, WithReturn4XXErrors)
+			if testData.withHTTPErrors {
+				serverOptions = append(serverOptions, WithSendOverGRPCAsInternal)
+			}
+			s := NewServer(h, serverOptions...)
+
+			req := &httpgrpc.HTTPRequest{
+				Method: "GET",
+				Url:    "/test",
+			}
+			resp, err := s.Handle(context.Background(), req)
+			require.Error(t, err)
+			require.Nil(t, resp)
+			checkError(t, err, testData.errorCode, errMsg)
+		})
+	}
+}
+
 func checkError(t *testing.T, err error, expectedCode int, expectedMessage string) {
 	resp, ok := httpgrpc.HTTPResponseFromError(err)
 	require.True(t, ok)