From 93261459e17ba9a8d242a3a935a4e68fa747534d Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Mon, 18 Mar 2024 13:51:51 -0700 Subject: [PATCH 1/2] Only send a serialized Status in the gRPC protocol if it has details --- connect_ext_test.go | 18 +++++++++++++++--- protocol_grpc.go | 39 +++++++++++++++++++++++---------------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/connect_ext_test.go b/connect_ext_test.go index 86bdeeff..05701eb4 100644 --- a/connect_ext_test.go +++ b/connect_ext_test.go @@ -690,13 +690,16 @@ func TestGRPCMarshalStatusError(t *testing.T) { mux := http.NewServeMux() mux.Handle(pingv1connect.NewPingServiceHandler( - pingServer{}, + pingServer{ + // Include error details in the response, so that the Status protobuf will be marshaled. + includeErrorDetails: true, + }, + // We're using a codec that will fail to marshal the Status protobuf, which means the returned error will be ignored connect.WithCodec(failCodec{}), )) server := memhttptest.NewServer(t, mux) assertInternalError := func(tb testing.TB, opts ...connect.ClientOption) { - tb.Helper() client := pingv1connect.NewPingServiceClient(server.Client(), server.URL(), opts...) request := connect.NewRequest(&pingv1.FailRequest{Code: int32(connect.CodeResourceExhausted)}) _, err := client.Fail(context.Background(), request) @@ -705,6 +708,7 @@ func TestGRPCMarshalStatusError(t *testing.T) { var connectErr *connect.Error ok := errors.As(err, &connectErr) assert.True(t, ok) + // This should be Internal, not ResourceExhausted, because we're testing when the Status object itself fails to marshal assert.Equal(t, connectErr.Code(), connect.CodeInternal) assert.True( t, @@ -2609,7 +2613,8 @@ func expectMetadata(meta http.Header, metaType, key, value string) error { type pingServer struct { pingv1connect.UnimplementedPingServiceHandler - checkMetadata bool + checkMetadata bool + includeErrorDetails bool } func (p pingServer) Ping(ctx context.Context, request *connect.Request[pingv1.PingRequest]) (*connect.Response[pingv1.PingResponse], error) { @@ -2646,6 +2651,13 @@ func (p pingServer) Fail(ctx context.Context, request *connect.Request[pingv1.Fa err := connect.NewError(connect.Code(request.Msg.GetCode()), errors.New(errorMessage)) err.Meta().Set(handlerHeader, headerValue) err.Meta().Set(handlerTrailer, trailerValue) + if p.includeErrorDetails { + detail, derr := connect.NewErrorDetail(&pingv1.FailRequest{Code: request.Msg.GetCode()}) + if derr != nil { + return nil, derr + } + err.AddDetail(detail) + } return nil, err } diff --git a/protocol_grpc.go b/protocol_grpc.go index 5a58cf42..82bd02ec 100644 --- a/protocol_grpc.go +++ b/protocol_grpc.go @@ -843,28 +843,35 @@ func grpcErrorToTrailer(trailer http.Header, protobuf Codec, err error) { } status := grpcStatusFromError(err) code := strconv.Itoa(int(status.GetCode())) - bin, binErr := protobuf.Marshal(status) - if binErr != nil { - setHeaderCanonical( - trailer, - grpcHeaderStatus, - strconv.FormatInt(int64(CodeInternal), 10 /* base */), - ) - setHeaderCanonical( - trailer, - grpcHeaderMessage, - grpcPercentEncode( - fmt.Sprintf("marshal protobuf status: %v", binErr), - ), - ) - return + var bin []byte + if len(status.Details) > 0 { + // attempt to serialize the Status first, so we can return an error before setting other headers + var binErr error + bin, binErr = protobuf.Marshal(status) + if binErr != nil { + setHeaderCanonical( + trailer, + grpcHeaderStatus, + strconv.FormatInt(int64(CodeInternal), 10 /* base */), + ) + setHeaderCanonical( + trailer, + grpcHeaderMessage, + grpcPercentEncode( + fmt.Sprintf("marshal protobuf status: %v", binErr), + ), + ) + return + } } if connectErr, ok := asError(err); ok { mergeHeaders(trailer, connectErr.meta) } setHeaderCanonical(trailer, grpcHeaderStatus, code) setHeaderCanonical(trailer, grpcHeaderMessage, grpcPercentEncode(status.GetMessage())) - setHeaderCanonical(trailer, grpcHeaderDetails, EncodeBinaryHeader(bin)) + if len(bin) > 0 { + setHeaderCanonical(trailer, grpcHeaderDetails, EncodeBinaryHeader(bin)) + } } func grpcStatusFromError(err error) *statusv1.Status { From 5253ba69cc0d8a3abe0225cbca6db9610756a899 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Tue, 19 Mar 2024 10:32:59 -0700 Subject: [PATCH 2/2] Emit error metadata headers even when failing to marshal error details --- connect_ext_test.go | 7 ++++--- protocol_grpc.go | 36 +++++++++++++----------------------- 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/connect_ext_test.go b/connect_ext_test.go index 05701eb4..169b417f 100644 --- a/connect_ext_test.go +++ b/connect_ext_test.go @@ -700,16 +700,17 @@ func TestGRPCMarshalStatusError(t *testing.T) { server := memhttptest.NewServer(t, mux) assertInternalError := func(tb testing.TB, opts ...connect.ClientOption) { + tb.Helper() client := pingv1connect.NewPingServiceClient(server.Client(), server.URL(), opts...) request := connect.NewRequest(&pingv1.FailRequest{Code: int32(connect.CodeResourceExhausted)}) _, err := client.Fail(context.Background(), request) tb.Log(err) - assert.NotNil(t, err) + assert.NotNil(t, err, assert.Sprintf("expected an error")) var connectErr *connect.Error ok := errors.As(err, &connectErr) - assert.True(t, ok) + assert.True(t, ok, assert.Sprintf("expected the error to be a connect.Error")) // This should be Internal, not ResourceExhausted, because we're testing when the Status object itself fails to marshal - assert.Equal(t, connectErr.Code(), connect.CodeInternal) + assert.Equal(t, connectErr.Code(), connect.CodeInternal, assert.Sprintf("expected the error code to be Internal, was %s", connectErr.Code())) assert.True( t, strings.HasSuffix(connectErr.Message(), ": boom"), diff --git a/protocol_grpc.go b/protocol_grpc.go index 82bd02ec..d498db59 100644 --- a/protocol_grpc.go +++ b/protocol_grpc.go @@ -838,37 +838,27 @@ func grpcContentTypeFromCodecName(web bool, name string) string { func grpcErrorToTrailer(trailer http.Header, protobuf Codec, err error) { if err == nil { setHeaderCanonical(trailer, grpcHeaderStatus, "0") // zero is the gRPC OK status - setHeaderCanonical(trailer, grpcHeaderMessage, "") return } - status := grpcStatusFromError(err) - code := strconv.Itoa(int(status.GetCode())) - var bin []byte + if connectErr, ok := asError(err); ok { + mergeHeaders(trailer, connectErr.meta) + } + var ( + status = grpcStatusFromError(err) + code = status.GetCode() + message = status.GetMessage() + bin []byte + ) if len(status.Details) > 0 { - // attempt to serialize the Status first, so we can return an error before setting other headers var binErr error bin, binErr = protobuf.Marshal(status) if binErr != nil { - setHeaderCanonical( - trailer, - grpcHeaderStatus, - strconv.FormatInt(int64(CodeInternal), 10 /* base */), - ) - setHeaderCanonical( - trailer, - grpcHeaderMessage, - grpcPercentEncode( - fmt.Sprintf("marshal protobuf status: %v", binErr), - ), - ) - return + code = int32(CodeInternal) + message = fmt.Sprintf("marshal protobuf status: %v", binErr) } } - if connectErr, ok := asError(err); ok { - mergeHeaders(trailer, connectErr.meta) - } - setHeaderCanonical(trailer, grpcHeaderStatus, code) - setHeaderCanonical(trailer, grpcHeaderMessage, grpcPercentEncode(status.GetMessage())) + setHeaderCanonical(trailer, grpcHeaderStatus, strconv.Itoa(int(code))) + setHeaderCanonical(trailer, grpcHeaderMessage, grpcPercentEncode(message)) if len(bin) > 0 { setHeaderCanonical(trailer, grpcHeaderDetails, EncodeBinaryHeader(bin)) }