From 3e948ab878a5ec3670149edd891f9ab625ea9d72 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Tue, 20 Jul 2021 21:56:13 -0400 Subject: [PATCH 1/4] Only return http status when grpc status is missing --- internal/transport/http2_client.go | 15 +++++++++++---- internal/transport/transport_test.go | 4 ++-- test/end2end_test.go | 11 ++++++----- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 8b6254b5bdc..d9dcce7df95 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1290,7 +1290,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { statusGen *status.Status httpStatusCode *int httpStatusErr string - rawStatusCode = codes.Unknown + rawStatusCode *codes.Code // headerError is set if an error is encountered while parsing the headers headerError string ) @@ -1318,7 +1318,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } - rawStatusCode = codes.Code(uint32(code)) + rsc := codes.Code(uint32(code)) + rawStatusCode = &rsc case "grpc-message": grpcMessage = decodeGrpcMessage(hf.Value) case "grpc-status-details-bin": @@ -1366,7 +1367,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if !isGRPC || httpStatusErr != "" { var code = codes.Internal // when header does not include HTTP status, return INTERNAL - if httpStatusCode != nil { + if rawStatusCode != nil { + code = *rawStatusCode + } else if httpStatusCode != nil { var ok bool code, ok = HTTPStatusConvTab[*httpStatusCode] if !ok { @@ -1438,7 +1441,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } if statusGen == nil { - statusGen = status.New(rawStatusCode, grpcMessage) + if rawStatusCode == nil { + rsc := codes.Unknown + rawStatusCode = &rsc + } + statusGen = status.New(*rawStatusCode, grpcMessage) } // if client received END_STREAM from server while stream was still active, send RST_STREAM diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 92990eaf7b2..e07137b1b02 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2031,7 +2031,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, }, wantStatus: status.New( - codes.Unknown, + codes.OK, "malformed header: missing HTTP content-type", ), }, @@ -2090,7 +2090,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, - {Name: "grpc-status", Value: "0"}, + {Name: "grpc-status", Value: "14"}, {Name: ":status", Value: "504"}, }, }, diff --git a/test/end2end_test.go b/test/end2end_test.go index 3d941b187bf..6bb1f5b8c91 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -34,6 +34,7 @@ import ( "os" "reflect" "runtime" + "strconv" "strings" "sync" "sync/atomic" @@ -7218,7 +7219,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingHTTPMode(t *testing.T) { doHTTPHeaderTest(t, transport.HTTPStatusConvTab[int(httpCode)], []string{ ":status", fmt.Sprintf("%d", httpCode), "content-type", "text/html", // non-gRPC content type to switch to HTTP mode. - "grpc-status", "1", // Make up a gRPC status error + "grpc-status", strconv.Itoa(int(transport.HTTPStatusConvTab[httpCode])), "grpc-status-details-bin", "???", // Make up a gRPC field parsing error }) } @@ -7228,7 +7229,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingHTTPMode(t *testing.T) { doHTTPHeaderTest(t, transport.HTTPStatusConvTab[int(httpCode)], []string{ ":status", fmt.Sprintf("%d", httpCode), // Omitting content type to switch to HTTP mode. - "grpc-status", "1", // Make up a gRPC status error + "grpc-status", strconv.Itoa(int(transport.HTTPStatusConvTab[httpCode])), "grpc-status-details-bin", "???", // Make up a gRPC field parsing error }) } @@ -7237,7 +7238,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingHTTPMode(t *testing.T) { doHTTPHeaderTest(t, codes.Internal, []string{ ":status", "abc", // Omitting content type to switch to HTTP mode. - "grpc-status", "1", // Make up a gRPC status error + "grpc-status", "13", // Make up a gRPC status error "grpc-status-details-bin", "???", // Make up a gRPC field parsing error }) } @@ -7270,7 +7271,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { header: []string{ ":status", "502", "content-type", "application/grpc", - "grpc-status", "0", + "grpc-status", "14", "grpc-tags-bin", "???", }, errCode: codes.Unavailable, @@ -7280,7 +7281,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { header: []string{ ":status", "502", "content-type", "application/grpc", - "grpc-status", "3", + "grpc-status", "14", }, errCode: codes.Unavailable, }, From 008beeb40332c37074a630bf2075003658bdc567 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Wed, 21 Jul 2021 23:02:00 -0400 Subject: [PATCH 2/4] ParseUint, not ParseInt and avoid alloc --- internal/transport/http2_client.go | 10 +++++----- internal/transport/transport_test.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index d9dcce7df95..09b3a7c397a 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1312,7 +1312,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { case "grpc-encoding": s.recvCompress = hf.Value case "grpc-status": - code, err := strconv.ParseInt(hf.Value, 10, 32) + code, err := strconv.ParseUint(hf.Value, 10, 32) if err != nil { se := status.New(codes.Internal, fmt.Sprintf("transport: malformed grpc-status: %v", err)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) @@ -1441,11 +1441,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } if statusGen == nil { - if rawStatusCode == nil { - rsc := codes.Unknown - rawStatusCode = &rsc + rsc := codes.Unknown + if rawStatusCode != nil { + rsc = *rawStatusCode } - statusGen = status.New(*rawStatusCode, grpcMessage) + statusGen = status.New(rsc, grpcMessage) } // if client received END_STREAM from server while stream was still active, send RST_STREAM diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index e07137b1b02..db028cf5d42 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2046,7 +2046,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Internal, - "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", + "transport: malformed grpc-status: strconv.ParseUint: parsing \"xxxx\": invalid syntax", ), }, { From fbb727b80e19e8d170d5b3c3ca93583a6c435d13 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Sun, 22 Aug 2021 21:23:24 -0400 Subject: [PATCH 3/4] grpc-status over http-status if everything else is good --- internal/transport/http2_client.go | 3 ++- internal/transport/transport_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 09b3a7c397a..58bd7f504e3 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1367,7 +1367,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if !isGRPC || httpStatusErr != "" { var code = codes.Internal // when header does not include HTTP status, return INTERNAL - if rawStatusCode != nil { + // grpc-status only takes precedence over http-status if everything else is ok. + if rawStatusCode != nil && contentTypeErr == "" && httpStatusErr == "" { code = *rawStatusCode } else if httpStatusCode != nil { var ok bool diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index db028cf5d42..4954b3cd676 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2031,7 +2031,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, }, wantStatus: status.New( - codes.OK, + codes.Unknown, "malformed header: missing HTTP content-type", ), }, From ae2fd9ce531697f4d7e30010e48e6a274d2994df Mon Sep 17 00:00:00 2001 From: James Protzman Date: Sun, 22 Aug 2021 21:41:27 -0400 Subject: [PATCH 4/4] Trigger workflow run