httpgrpc: correct handling of non-loggable errors

CHANGELOG.md

@@ -154,6 +154,7 @@
 * [ENHANCEMENT] Ring: add support for aborting early if a terminal error is returned by a request initiated by `DoUntilQuorum`. #404 #413
 * [ENHANCEMENT] Memcached: allow to configure write and read buffer size (in bytes). #414
 * [ENHANCEMENT] Server: Add `-server.http-read-header-timeout` option to specify timeout for reading HTTP request header. It defaults to 0, in which case reading of headers can take up to `-server.http-read-timeout`, leaving no time for reading body, if there's any. #423
+* [ENHANCEMENT] Make httpgrpc.Server produce non-loggable errors when a header with key `httpgrpc.DoNotLogErrorHeaderKey` and any value is present in the HTTP response. #421
 * [BUGFIX] spanlogger: Support multiple tenant IDs. #59
 * [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
 * [BUGFIX] Ring: `ring_member_ownership_percent` and `ring_tokens_owned` metrics are not updated on scale down. #109

httpgrpc/httpgrpc.go

@@ -6,10 +6,12 @@ package httpgrpc
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/go-kit/log/level"
 	"google.golang.org/grpc/metadata"
+	grpcstatus "google.golang.org/grpc/status"
 
 	spb "github.com/gogo/googleapis/google/rpc"
 	"github.com/gogo/protobuf/types"
@@ -44,7 +46,7 @@ func ErrorFromHTTPResponse(resp *HTTPResponse) error {
 
 // HTTPResponseFromError converts a grpc error into an HTTP response
 func HTTPResponseFromError(err error) (*HTTPResponse, bool) {
-	s, ok := status.FromError(err)
+	s, ok := statusFromError(err)
 	if !ok {
 		return nil, false
 	}
@@ -63,6 +65,29 @@ func HTTPResponseFromError(err error) (*HTTPResponse, bool) {
 	return &resp, true
 }
 
+// statusFromError tries to cast the given error into status.Status.
+// If the given error, or any error from its tree are a status.Status,
+// that status.Status and the outcome true are returned.
+// Otherwise, nil and the outcome false are returned.
+// This implementation differs from status.FromError() because the
+// latter checks only if the given error can be cast to status.Status,
+// and doesn't check other errors in the given error's tree.
+func statusFromError(err error) (*status.Status, bool) {
+	if err == nil {
+		return nil, false
+	}
+	type grpcStatus interface{ GRPCStatus() *grpcstatus.Status }
+	var gs grpcStatus
+	if errors.As(err, &gs) {
+		st := gs.GRPCStatus()
+		if st == nil {
+			return nil, false
+		}
+		return status.FromGRPCStatus(st), true
+	}
+	return nil, false
+}
+
 const (
 	MetadataMethod = "httpgrpc-method"
 	MetadataURL    = "httpgrpc-url"

httpgrpc/httpgrpc_test.go

@@ -2,10 +2,14 @@ package httpgrpc
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
+	"github.com/gogo/status"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
+	grpcstatus "google.golang.org/grpc/status"
 )
 
 func TestAppendMessageSizeToOutgoingContext(t *testing.T) {
@@ -24,3 +28,146 @@ func TestAppendMessageSizeToOutgoingContext(t *testing.T) {
 	require.Equal(t, []string{"GET"}, md.Get(MetadataMethod))
 	require.Equal(t, []string{"/test"}, md.Get(MetadataURL))
 }
+
+func TestErrorf(t *testing.T) {
+	code := 400
+	errMsg := "this is an error"
+	expectedHTTPResponse := &HTTPResponse{
+		Code: int32(code),
+		Body: []byte(errMsg),
+	}
+	err := Errorf(code, errMsg)
+	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 TestErrorFromHTTPResponse(t *testing.T) {
+	var 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 := ErrorFromHTTPResponse(resp)
+	require.Error(t, err)
+	stat, ok := status.FromError(err)
+	require.True(t, ok)
+	require.Equal(t, code, int32(stat.Code()))
+	require.Equal(t, errMsg, stat.Message())
+	checkDetailAsHTTPResponse(t, resp, stat)
+}
+
+func TestHTTPResponseFromError(t *testing.T) {
+	msgErr := "this is an error"
+	testCases := map[string]struct {
+		err                  error
+		isGRPCError          bool
+		isHTTPGRCPError      bool
+		expectedHTTPResponse *HTTPResponse
+	}{
+		"no error cannot be parsed to an HTTPResponse": {
+			err: nil,
+		},
+		"a random error cannot be parsed to an HTTPResponse": {
+			err: fmt.Errorf(msgErr),
+		},
+		"a gRPC error built by gogo/status cannot be parsed to an HTTPResponse": {
+			err: status.Error(codes.Internal, msgErr),
+		},
+		"a gRPC error built by grpc/status cannot be parsed to an HTTPResponse": {
+			err: grpcstatus.Error(codes.Internal, msgErr),
+		},
+		"a gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
+			err:                  Errorf(400, msgErr),
+			expectedHTTPResponse: &HTTPResponse{Code: 400, Body: []byte(msgErr)},
+		},
+		"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)},
+		},
+	}
+	for testName, testData := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			resp, ok := HTTPResponseFromError(testData.err)
+			if testData.expectedHTTPResponse == nil {
+				require.False(t, ok)
+				require.Nil(t, resp)
+			} else {
+				require.True(t, ok)
+
+			}
+		})
+	}
+}
+
+func TestStatusFromError(t *testing.T) {
+	msgErr := "this is an error"
+	testCases := map[string]struct {
+		err            error
+		expectedStatus *status.Status
+	}{
+		"no error cannot be cast to status.Status": {
+			err: nil,
+		},
+		"a random error cannot be cast to status.Status": {
+			err: fmt.Errorf(msgErr),
+		},
+		"a wrapped error of a random error cannot be cast to status.Status": {
+			err: fmt.Errorf("wrapped: %w", fmt.Errorf(msgErr)),
+		},
+		"a gRPC error built by gogo/status can be cast to status.Status": {
+			err:            status.Error(codes.Internal, msgErr),
+			expectedStatus: status.New(codes.Internal, msgErr),
+		},
+		"a wrapped error of a gRPC error built by gogo/status can be cast to status.Status": {
+			err:            fmt.Errorf("wrapped: %w", status.Error(codes.Internal, msgErr)),
+			expectedStatus: status.New(codes.Internal, msgErr),
+		},
+		"a gRPC error built by grpc/status can be cast to status.Status": {
+			err:            grpcstatus.Error(codes.Internal, msgErr),
+			expectedStatus: status.New(codes.Internal, msgErr),
+		},
+		"a wrapped error of a gRPC error built by grpc/status can be cast to status.Status": {
+			err:            fmt.Errorf("wrapped: %w", grpcstatus.Error(codes.Internal, msgErr)),
+			expectedStatus: status.New(codes.Internal, msgErr),
+		},
+		"a gRPC error built by httpgrpc can be cast to status.Status": {
+			err:            Errorf(400, msgErr),
+			expectedStatus: status.New(400, msgErr),
+		},
+		"a wrapped gRPC error built by httpgrpc can be cast to status.Status": {
+			err:            fmt.Errorf("wrapped: %w", Errorf(400, msgErr)),
+			expectedStatus: status.New(400, msgErr),
+		},
+	}
+	for testName, testData := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			stat, ok := statusFromError(testData.err)
+			if testData.expectedStatus == nil {
+				require.False(t, ok)
+				require.Nil(t, stat)
+			} else {
+				require.True(t, ok)
+				require.NotNil(t, stat)
+				require.Equal(t, testData.expectedStatus.Code(), stat.Code())
+				require.Equal(t, testData.expectedStatus.Message(), stat.Message())
+			}
+		})
+	}
+}
+
+func checkDetailAsHTTPResponse(t *testing.T, httpResponse *HTTPResponse, stat *status.Status) {
+	details := stat.Details()
+	require.Len(t, details, 1)
+	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)
+}

httpgrpc/server/server.go

@@ -27,6 +27,13 @@ import (
 	"github.com/grafana/dskit/middleware"
 )
 
+var (
+	// DoNotLogErrorHeaderKey is a header key used for marking non-loggable errors. More precisely, if an HTTP response
+	// has a status code 5xx, and contains a header with key DoNotLogErrorHeaderKey and any values, the generated error
+	// will be marked as non-loggable.
+	DoNotLogErrorHeaderKey = http.CanonicalHeaderKey("X-DoNotLogError")
+)
+
 // Server implements HTTPServer.  HTTPServer is a generated interface that gRPC
 // servers must implement.
 type Server struct {
@@ -62,13 +69,18 @@ func (s Server) Handle(ctx context.Context, r *httpgrpc.HTTPRequest) (*httpgrpc.
 
 	recorder := httptest.NewRecorder()
 	s.handler.ServeHTTP(recorder, req)
+	header := recorder.Header()
 	resp := &httpgrpc.HTTPResponse{
 		Code:    int32(recorder.Code),
-		Headers: fromHeader(recorder.Header()),
+		Headers: fromHeader(header),
 		Body:    recorder.Body.Bytes(),
 	}
 	if recorder.Code/100 == 5 {
-		return nil, httpgrpc.ErrorFromHTTPResponse(resp)
+		err := httpgrpc.ErrorFromHTTPResponse(resp)
+		if _, ok := header[DoNotLogErrorHeaderKey]; ok {
+			err = middleware.DoNotLogError{Err: err}
+		}
+		return nil, err
 	}
 	return resp, nil
 }
@@ -227,6 +239,9 @@ func toHeader(hs []*httpgrpc.Header, header http.Header) {
 func fromHeader(hs http.Header) []*httpgrpc.Header {
 	result := make([]*httpgrpc.Header, 0, len(hs))
 	for k, vs := range hs {
+		if k == DoNotLogErrorHeaderKey {
+			continue
+		}
 		result = append(result, &httpgrpc.Header{
 			Key:    k,
 			Values: vs,