From 50b53376da7813afb1e1e299974f818839ecc00d Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Thu, 28 Aug 2025 04:18:49 +0000 Subject: [PATCH 1/6] ignoring http status when content type is application/grpc grpc#8486 Fixes https://github.com/grpc/grpc-go/issues/8486 When a gRPC response is received with content type application/grpc, we then do not expect any information in the http status and the status information needs to be conveyed by gRPC status only. In case of missing gRPC status, we will throw an Internal error instead of Unknown in accordance with https://grpc.io/docs/guides/status-codes/ Changes : - Ignore http status in case of content type application/grpc - Change the default rawStatusCode to return Internal for missing grpc status RELEASE NOTES: * client : Ignore http status for gRPC mode and return Internal error for missing grpc status --- internal/transport/http2_client.go | 6 ++-- internal/transport/transport_test.go | 47 +++++++++++++++++++++------- test/http_header_end2end_test.go | 26 +++++++-------- 3 files changed, 51 insertions(+), 28 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 5467fe9715a3..d0cadf6cce73 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1467,7 +1467,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { recvCompress string httpStatusCode *int httpStatusErr string - rawStatusCode = codes.Unknown + rawStatusCode = codes.Internal // headerError is set if an error is encountered while parsing the headers headerError string ) @@ -1475,7 +1475,6 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if initialHeader { httpStatusErr = "malformed header: missing HTTP status" } - for _, hf := range frame.Fields { switch hf.Name { case "content-type": @@ -1534,7 +1533,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - if !isGRPC || httpStatusErr != "" { + //if response is not grpc or endstream doesn't receive a grpc status, fall back to http error code + if !isGRPC { var code = codes.Internal // when header does not include HTTP status, return INTERNAL if httpStatusCode != nil { diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index b8f97c3c9464..b5674503cda5 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2618,6 +2618,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame *http2.MetaHeadersFrame // output wantStatus *status.Status + // end stream output + wantStatusEndStream *status.Status }{ { name: "valid header", @@ -2695,7 +2697,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { ), }, { - name: "bad status in grpc mode", + name: "ignoring bad http status in grpc mode", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, @@ -2703,26 +2705,47 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: ":status", Value: "504"}, }, }, - wantStatus: status.New( - codes.Unavailable, - "unexpected HTTP status code received from server: 504 (Gateway Timeout)", - ), + wantStatus: status.New(codes.OK, ""), }, { - name: "missing http status", + name: "missing http status and grpc status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, }, }, - wantStatus: status.New( - codes.Internal, - "malformed header: missing HTTP status", - ), + wantStatus: status.New(codes.OK, ""), + wantStatusEndStream: status.New(codes.Internal, ""), + }, + { + name: "ignore http status and fail for grpc status missing in trailer", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/grpc"}, + {Name: ":status", Value: "504"}, + }, + }, + wantStatus: status.New(codes.OK, ""), + wantStatusEndStream: status.New(codes.Internal, ""), + }, + { + name: "trailer only grpc timeout ignores http status", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/grpc"}, + {Name: "grpc-status", Value: "4"}, + {Name: "grpc-message", Value: "Request timed out: Internal error"}, + {Name: ":status", Value: "200"}, + }, + }, + wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), }, } { t.Run(test.name, func(t *testing.T) { + if test.wantStatus == nil { + t.Skip() + } ts := testStream() s := testClient(ts) @@ -2733,7 +2756,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { } s.operateHeaders(test.metaHeaderFrame) - got := ts.status want := test.wantStatus if got.Code() != want.Code() || got.Message() != want.Message() { @@ -2755,6 +2777,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { got := ts.status want := test.wantStatus + if test.wantStatusEndStream != nil { + want = test.wantStatusEndStream + } if got.Code() != want.Code() || got.Message() != want.Message() { t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want) } diff --git a/test/http_header_end2end_test.go b/test/http_header_end2end_test.go index 0044a9f7ebf9..9ad33ef3c88a 100644 --- a/test/http_header_end2end_test.go +++ b/test/http_header_end2end_test.go @@ -103,10 +103,9 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { errCode codes.Code }{ { - name: "missing gRPC status", + name: "missing gRPC content type", header: []string{ ":status", "403", - "content-type", "application/grpc", }, errCode: codes.PermissionDenied, }, @@ -120,23 +119,23 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { errCode: codes.Internal, }, { - name: "Malformed grpc-tags-bin field", + name: "Malformed grpc-tags-bin field ignores http status", header: []string{ ":status", "502", "content-type", "application/grpc", "grpc-status", "0", "grpc-tags-bin", "???", }, - errCode: codes.Unavailable, + errCode: codes.Internal, }, { - name: "gRPC status error", + name: "gRPC status error ignoring http status", header: []string{ ":status", "502", "content-type", "application/grpc", "grpc-status", "3", }, - errCode: codes.Unavailable, + errCode: codes.InvalidArgument, }, } { t.Run(test.name, func(t *testing.T) { @@ -161,7 +160,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { errCode codes.Code }{ { - name: "trailer missing grpc-status", + name: "trailer missing grpc-status to ignore http status", responseHeader: []string{ ":status", "200", "content-type", "application/grpc", @@ -170,10 +169,10 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { // trailer missing grpc-status ":status", "502", }, - errCode: codes.Unavailable, + errCode: codes.Internal, }, { - name: "malformed grpc-status-details-bin field with status 404", + name: "malformed grpc-status-details-bin field with status 404 to be ignored due to content type", responseHeader: []string{ ":status", "404", "content-type", "application/grpc", @@ -183,20 +182,19 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { "grpc-status", "0", "grpc-status-details-bin", "????", }, - errCode: codes.Unimplemented, + errCode: codes.Internal, }, { - name: "malformed grpc-status-details-bin field with status 200", + name: "malformed grpc-status-details-bin field with status 404 and no content type", responseHeader: []string{ - ":status", "200", - "content-type", "application/grpc", + ":status", "404", }, trailer: []string{ // malformed grpc-status-details-bin field "grpc-status", "0", "grpc-status-details-bin", "????", }, - errCode: codes.Internal, + errCode: codes.Unimplemented, }, } for _, test := range tests { From b1a72650791c0e93eaf6623c8c5915ca9b68ef2a Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Tue, 9 Sep 2025 03:42:33 +0000 Subject: [PATCH 2/6] addressing review comments for grpc#8486 --- internal/transport/http2_client.go | 65 ++++++++++++++-------------- internal/transport/transport_test.go | 45 ++++++++++++++++--- 2 files changed, 72 insertions(+), 38 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index d0cadf6cce73..df7295665dcd 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1465,16 +1465,13 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { contentTypeErr = "malformed header: missing HTTP content-type" grpcMessage string recvCompress string - httpStatusCode *int httpStatusErr string rawStatusCode = codes.Internal // headerError is set if an error is encountered while parsing the headers - headerError string + headerError string + receivedHttpStatus string ) - if initialHeader { - httpStatusErr = "malformed header: missing HTTP status" - } for _, hf := range frame.Fields { switch hf.Name { case "content-type": @@ -1498,27 +1495,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { case "grpc-message": grpcMessage = decodeGrpcMessage(hf.Value) case ":status": - if hf.Value == "200" { - httpStatusErr = "" - statusCode := 200 - httpStatusCode = &statusCode - break - } - - c, err := strconv.ParseInt(hf.Value, 10, 32) - if err != nil { - se := status.New(codes.Internal, fmt.Sprintf("transport: malformed http-status: %v", err)) - t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) - return + if !isGRPC { + receivedHttpStatus = hf.Value } - statusCode := int(c) - httpStatusCode = &statusCode - - httpStatusErr = fmt.Sprintf( - "unexpected HTTP status code received from server: %d (%s)", - statusCode, - http.StatusText(statusCode), - ) default: if isReservedHeader(hf.Name) && !isWhitelistedHeader(hf.Name) { break @@ -1533,26 +1512,48 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - //if response is not grpc or endstream doesn't receive a grpc status, fall back to http error code + //if not a gRPC response - evaluate entire http status and process close stream / response + //for 200 -> Unknown, else decode the error if !isGRPC { - var code = codes.Internal // when header does not include HTTP status, return INTERNAL + var grpcErrorCode = codes.Internal // when header does not include HTTP status, return INTERNAL + var errs []string - if httpStatusCode != nil { + switch receivedHttpStatus { + case "": + httpStatusErr = "malformed header: missing HTTP status" + case "200": + grpcErrorCode = codes.Unknown + default: + // Any other status code (e.g., "404", "503"). We must parse it. + c, err := strconv.ParseInt(receivedHttpStatus, 10, 32) + if err != nil { + se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err)) + t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) + return + } + statusCode := int(c) + httpStatusErr = fmt.Sprintf( + "unexpected HTTP status code received from server: %d (%s)", + statusCode, + http.StatusText(statusCode), + ) var ok bool - code, ok = HTTPStatusConvTab[*httpStatusCode] + grpcErrorCode, ok = HTTPStatusConvTab[statusCode] if !ok { - code = codes.Unknown + grpcErrorCode = codes.Unknown } + } - var errs []string + if httpStatusErr != "" { errs = append(errs, httpStatusErr) } + if contentTypeErr != "" { errs = append(errs, contentTypeErr) } // Verify the HTTP response is a 200. - se := status.New(code, strings.Join(errs, "; ")) + se := status.New(grpcErrorCode, strings.Join(errs, "; ")) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index b5674503cda5..4801ba3a1ac0 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2631,7 +2631,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, }, // no error - wantStatus: status.New(codes.OK, ""), + wantStatus: status.New(codes.OK, ""), + wantStatusEndStream: status.New(codes.OK, ""), }, { name: "missing content-type header", @@ -2645,6 +2646,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Unknown, "malformed header: missing HTTP content-type", ), + wantStatusEndStream: status.New( + codes.Unknown, + "malformed header: missing HTTP content-type", + ), }, { name: "invalid grpc status header field", @@ -2659,6 +2664,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", ), + wantStatusEndStream: status.New( + codes.Internal, + "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", + ), }, { name: "invalid http content type", @@ -2671,6 +2680,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", ), + wantStatusEndStream: status.New( + codes.Internal, + "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", + ), }, { name: "http fallback and invalid http status", @@ -2684,6 +2697,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", ), + wantStatusEndStream: status.New( + codes.Internal, + "transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", + ), }, { name: "http2 frame size exceeds", @@ -2695,6 +2712,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "peer header list size exceeded limit", ), + wantStatusEndStream: status.New( + codes.Internal, + "peer header list size exceeded limit", + ), }, { name: "ignoring bad http status in grpc mode", @@ -2705,7 +2726,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: ":status", Value: "504"}, }, }, - wantStatus: status.New(codes.OK, ""), + wantStatus: status.New(codes.OK, ""), + wantStatusEndStream: status.New(codes.OK, ""), }, { name: "missing http status and grpc status", @@ -2729,7 +2751,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { wantStatusEndStream: status.New(codes.Internal, ""), }, { - name: "trailer only grpc timeout ignores http status", + name: "ignore valid http status for grpc", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, @@ -2738,14 +2760,25 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: ":status", Value: "200"}, }, }, + wantStatus: status.New(codes.OK, ""), + wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), + }, + { + name: "ignore illegal http status for grpc", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/grpc"}, + {Name: "grpc-status", Value: "4"}, + {Name: "grpc-message", Value: "Request timed out: Internal error"}, + {Name: ":status", Value: "thisIsIllegal"}, + }, + }, + wantStatus: status.New(codes.OK, ""), wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), }, } { t.Run(test.name, func(t *testing.T) { - if test.wantStatus == nil { - t.Skip() - } ts := testStream() s := testClient(ts) From 70876643551d0634aab066c187ec22445cdd836b Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Fri, 12 Sep 2025 08:24:00 +0000 Subject: [PATCH 3/6] addressing review comments for grpc#8486 --- internal/transport/http2_client.go | 12 ++++++++---- internal/transport/transport_test.go | 15 +++++++-------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index df11ba13618d..faaa9d4026a6 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1512,8 +1512,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - //if not a gRPC response - evaluate entire http status and process close stream / response - //for 200 -> Unknown, else decode the error + // If a non gRPC response is received, then evaluate entire http status and process close stream / response. + // In case http status doesn't provide any error information (status : 200), evalute response code to be Unknown. if !isGRPC { var grpcErrorCode = codes.Internal // when header does not include HTTP status, return INTERNAL var errs []string @@ -1533,8 +1533,12 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } statusCode := int(c) if statusCode >= 100 && statusCode < 200 { + if endStream { + se := status.New(codes.Internal, fmt.Sprintf( + "protocol error: informational header with status code %d must not have END_STREAM set", statusCode)) + t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) + } //In case of informational headers return - //For trailers, since we are already in gRPC mode, we will ignore all http statuses and not enter this block return } httpStatusErr = fmt.Sprintf( @@ -1557,7 +1561,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if contentTypeErr != "" { errs = append(errs, contentTypeErr) } - // Verify the HTTP response is a 200. + se := status.New(grpcErrorCode, strings.Join(errs, "; ")) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 075ef17543b1..3fed8cffc6ee 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2807,12 +2807,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { } s.operateHeaders(test.metaHeaderFrame) - got := ts.status - want := test.wantStatus - if test.wantStatusEndStream != nil { - want = test.wantStatusEndStream - } + want := test.wantStatusEndStream if got.Code() != want.Code() || got.Message() != want.Message() { t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want) } @@ -3217,14 +3213,17 @@ func (s) TestClientTransport_Handle1xxHeaders(t *testing.T) { wantStatus *status.Status }{ { - name: "1xx with END_STREAM will be ignored", + name: "1xx with END_STREAM is error", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: ":status", Value: "100"}, }, }, - httpFlags: http2.FlagHeadersEndStream, - wantStatus: nil, + httpFlags: http2.FlagHeadersEndStream, + wantStatus: status.New( + codes.Internal, + "protocol error: informational header with status code 100 must not have END_STREAM set", + ), }, { name: "1xx without END_STREAM is ignored", From aceb8bef40ff21b328cd6bbed9cb94fa7d2c6228 Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Mon, 15 Sep 2025 08:53:54 +0000 Subject: [PATCH 4/6] removing special case for 200 http status code --- internal/transport/http2_client.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index faaa9d4026a6..60d945e7f80e 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1521,8 +1521,6 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { switch receivedHTTPStatus { case "": httpStatusErr = "malformed header: missing HTTP status" - case "200": - grpcErrorCode = codes.Unknown default: // Any other status code (e.g., "404", "503"). We must parse it. c, err := strconv.ParseInt(receivedHTTPStatus, 10, 32) @@ -1551,7 +1549,6 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if !ok { grpcErrorCode = codes.Unknown } - } if httpStatusErr != "" { From 734cd4d178d9883f518051d813d31f644aaec325 Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Wed, 17 Sep 2025 08:57:53 +0000 Subject: [PATCH 5/6] addressing review comments --- internal/transport/http2_client.go | 33 ++++++++++---------- internal/transport/transport_test.go | 45 ++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 60d945e7f80e..a3b92bd3e14d 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1466,10 +1466,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { grpcMessage string recvCompress string httpStatusErr string - rawStatusCode = codes.Internal + // the code from the grpc-status header, if present + grpcStatusCode = codes.Internal // headerError is set if an error is encountered while parsing the headers - headerError string - receivedHTTPStatus string + headerError string + httpStatus string ) for _, hf := range frame.Fields { @@ -1491,13 +1492,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } - rawStatusCode = codes.Code(uint32(code)) + grpcStatusCode = codes.Code(uint32(code)) case "grpc-message": grpcMessage = decodeGrpcMessage(hf.Value) case ":status": - if !isGRPC { - receivedHTTPStatus = hf.Value - } + httpStatus = hf.Value default: if isReservedHeader(hf.Name) && !isWhitelistedHeader(hf.Name) { break @@ -1512,18 +1511,18 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - // If a non gRPC response is received, then evaluate entire http status and process close stream / response. - // In case http status doesn't provide any error information (status : 200), evalute response code to be Unknown. + // If a non gRPC response is received, then evaluate entire http status and + // process close stream / response. + // In case http status doesn't provide any error information (status : 200), + // evalute response code to be Unknown. if !isGRPC { - var grpcErrorCode = codes.Internal // when header does not include HTTP status, return INTERNAL - var errs []string - - switch receivedHTTPStatus { + var grpcErrorCode = codes.Internal + switch httpStatus { case "": httpStatusErr = "malformed header: missing HTTP status" default: // Any other status code (e.g., "404", "503"). We must parse it. - c, err := strconv.ParseInt(receivedHTTPStatus, 10, 32) + c, err := strconv.ParseInt(httpStatus, 10, 32) if err != nil { se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) @@ -1536,7 +1535,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { "protocol error: informational header with status code %d must not have END_STREAM set", statusCode)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) } - //In case of informational headers return + // In case of informational headers return return } httpStatusErr = fmt.Sprintf( @@ -1550,7 +1549,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { grpcErrorCode = codes.Unknown } } - + var errs []string if httpStatusErr != "" { errs = append(errs, httpStatusErr) } @@ -1613,7 +1612,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { return } - status := istatus.NewWithProto(rawStatusCode, grpcMessage, mdata[grpcStatusDetailsBinHeader]) + status := istatus.NewWithProto(grpcStatusCode, grpcMessage, mdata[grpcStatusDetailsBinHeader]) // 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 3fed8cffc6ee..2f5852a7161e 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2620,6 +2620,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { wantStatus *status.Status // end stream output wantStatusEndStream *status.Status + // head channel closed + headerChanClosedForEndStream bool }{ { name: "valid header", @@ -2644,11 +2646,14 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Unknown, - "malformed header: missing HTTP content-type", + "unexpected HTTP status code received from server: 200 (OK); malformed header: missing HTTP content-type", ), + headerChanClosedForEndStream: true, + // when headerChan is closed, it is already gRPC and we just need + // grpc-status wantStatusEndStream: status.New( - codes.Unknown, - "malformed header: missing HTTP content-type", + codes.OK, + "", ), }, { @@ -2680,11 +2685,32 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", ), + // This content type will only come when end stream is also initial + // header, otherwise we would already be talking gRPC wantStatusEndStream: status.New( codes.Internal, "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", ), }, + { + name: "invalid http content type with http status 504 translation", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/json"}, + {Name: ":status", Value: "504"}, + }, + }, + wantStatus: status.New( + codes.Unavailable, + "unexpected HTTP status code received from server: 504 (Gateway Timeout); transport: received unexpected content-type \"application/json\"", + ), + // This content type will only come when end stream is also initial + // header, otherwise we would already be talking gRPC + wantStatusEndStream: status.New( + codes.Unavailable, + "unexpected HTTP status code received from server: 504 (Gateway Timeout); transport: received unexpected content-type \"application/json\"", + ), + }, { name: "http fallback and invalid http status", metaHeaderFrame: &http2.MetaHeadersFrame{ @@ -2697,9 +2723,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", ), + // for end stream, when we are already talking gRPC, we expect + // grpc - status and fail for it, we will ignore http status. + headerChanClosedForEndStream: true, wantStatusEndStream: status.New( codes.Internal, - "transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", + "", ), }, { @@ -2747,8 +2776,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: ":status", Value: "504"}, }, }, - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.Internal, ""), + wantStatus: status.New(codes.OK, ""), + headerChanClosedForEndStream: true, + wantStatusEndStream: status.New(codes.Internal, ""), }, { name: "ignore valid http status for grpc", @@ -2797,6 +2827,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }) t.Run(fmt.Sprintf("%s-end_stream", test.name), func(t *testing.T) { ts := testStream() + if test.headerChanClosedForEndStream { + ts.headerChanClosed = 1 + } s := testClient(ts) test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ From bebc8d3abd2202a6816f57dfe932eec7f1cefa36 Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Tue, 7 Oct 2025 15:40:45 +0000 Subject: [PATCH 6/6] splitting the test for headers and trailers --- internal/transport/http2_client.go | 18 +- internal/transport/transport_test.go | 263 ++++++++++++++------------- 2 files changed, 143 insertions(+), 138 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index a3b92bd3e14d..148f3368228c 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1511,31 +1511,29 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - // If a non gRPC response is received, then evaluate entire http status and - // process close stream / response. + // If a non-gRPC response is received, then evaluate the HTTP status to + // process the response and close the stream. // In case http status doesn't provide any error information (status : 200), - // evalute response code to be Unknown. + // then evalute response code to be Unknown. if !isGRPC { var grpcErrorCode = codes.Internal - switch httpStatus { - case "": + if httpStatus == "" { httpStatusErr = "malformed header: missing HTTP status" - default: - // Any other status code (e.g., "404", "503"). We must parse it. - c, err := strconv.ParseInt(httpStatus, 10, 32) + } else { + // Parse the status codes (e.g. "200", 404"). + statusCode, err := strconv.Atoi(httpStatus) if err != nil { se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } - statusCode := int(c) if statusCode >= 100 && statusCode < 200 { if endStream { se := status.New(codes.Internal, fmt.Sprintf( "protocol error: informational header with status code %d must not have END_STREAM set", statusCode)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) } - // In case of informational headers return + // In case of informational headers, return. return } httpStatusErr = fmt.Sprintf( diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 2f5852a7161e..15f098174f9f 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2588,56 +2588,50 @@ func (s) TestClientHandshakeInfoDialer(t *testing.T) { } } -func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { - testStream := func() *ClientStream { - return &ClientStream{ - Stream: &Stream{ - buf: &recvBuffer{ - c: make(chan recvMsg), - mu: sync.Mutex{}, - }, +func newTestClientStream() *ClientStream { + return &ClientStream{ + Stream: &Stream{ + buf: &recvBuffer{ + c: make(chan recvMsg), }, - done: make(chan struct{}), - headerChan: make(chan struct{}), - } + }, + done: make(chan struct{}), + headerChan: make(chan struct{}), } +} - testClient := func(ts *ClientStream) *http2Client { - return &http2Client{ - mu: sync.Mutex{}, - activeStreams: map[uint32]*ClientStream{ - 0: ts, - }, - controlBuf: newControlBuffer(make(<-chan struct{})), - } +func newTestHTTP2Client(cs *ClientStream) *http2Client { + return &http2Client{ + activeStreams: map[uint32]*ClientStream{ + // StreamID 0 is reserved for connection-level flow control. + // Client-initiated streams must use odd-numbered stream identifiers. + 1: cs, + }, + controlBuf: newControlBuffer(make(<-chan struct{})), } +} - for _, test := range []struct { - name string - // input +// TestClientDecodeHeader validates the handling of initial header frames that +// do not signal the end of a stream. For all headers that indicate grpc content +// type, http status will be ignored. +func (s) TestClientDecodeHeader(t *testing.T) { + tests := []struct { + name string metaHeaderFrame *http2.MetaHeadersFrame - // output - wantStatus *status.Status - // end stream output - wantStatusEndStream *status.Status - // head channel closed - headerChanClosedForEndStream bool + wantStatus *status.Status }{ { - name: "valid header", + name: "valid_header", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, - {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "200"}, }, }, - // no error - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.OK, ""), + wantStatus: status.New(codes.OK, ""), }, { - name: "missing content-type header", + name: "missing_content_type_header", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "grpc-status", Value: "0"}, @@ -2648,16 +2642,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Unknown, "unexpected HTTP status code received from server: 200 (OK); malformed header: missing HTTP content-type", ), - headerChanClosedForEndStream: true, - // when headerChan is closed, it is already gRPC and we just need - // grpc-status - wantStatusEndStream: status.New( - codes.OK, - "", - ), }, { - name: "invalid grpc status header field", + name: "invalid_grpc_status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, @@ -2669,13 +2656,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", ), - wantStatusEndStream: status.New( - codes.Internal, - "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", - ), }, { - name: "invalid http content type", + name: "invalid_content_type", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/json"}, @@ -2685,15 +2668,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Internal, "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", ), - // This content type will only come when end stream is also initial - // header, otherwise we would already be talking gRPC - wantStatusEndStream: status.New( - codes.Internal, - "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", - ), }, { - name: "invalid http content type with http status 504 translation", + name: "invalid_content_type_with_http_status_504", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/json"}, @@ -2704,146 +2681,176 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { codes.Unavailable, "unexpected HTTP status code received from server: 504 (Gateway Timeout); transport: received unexpected content-type \"application/json\"", ), - // This content type will only come when end stream is also initial - // header, otherwise we would already be talking gRPC - wantStatusEndStream: status.New( - codes.Unavailable, - "unexpected HTTP status code received from server: 504 (Gateway Timeout); transport: received unexpected content-type \"application/json\"", - ), }, { - name: "http fallback and invalid http status", + name: "http_fallback_and_invalid_http_status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ - // No content type provided then fallback into handling http error. {Name: ":status", Value: "xxxx"}, }, }, wantStatus: status.New( codes.Internal, - "transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", - ), - // for end stream, when we are already talking gRPC, we expect - // grpc - status and fail for it, we will ignore http status. - headerChanClosedForEndStream: true, - wantStatusEndStream: status.New( - codes.Internal, - "", + "transport: malformed http-status: strconv.Atoi: parsing \"xxxx\": invalid syntax", ), }, { - name: "http2 frame size exceeds", + name: "http2_frame_size_exceeds", metaHeaderFrame: &http2.MetaHeadersFrame{ - Fields: nil, Truncated: true, }, wantStatus: status.New( codes.Internal, "peer header list size exceeded limit", ), - wantStatusEndStream: status.New( - codes.Internal, - "peer header list size exceeded limit", - ), }, { - name: "ignoring bad http status in grpc mode", + name: "missing_http_status_and_grpc_status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, - {Name: "grpc-status", Value: "0"}, - {Name: ":status", Value: "504"}, }, }, - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.OK, ""), + wantStatus: status.New(codes.OK, ""), }, { - name: "missing http status and grpc status", + name: "ignore_http_status_for_grpc", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, + {Name: ":status", Value: "504"}, }, }, - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.Internal, ""), + wantStatus: status.New(codes.OK, ""), }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cs := newTestClientStream() + s := newTestHTTP2Client(cs) + + tc.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 1, + }, + } + + s.operateHeaders(tc.metaHeaderFrame) + got := cs.status + want := tc.wantStatus + if got.Code() != want.Code() || got.Message() != want.Message() { + t.Errorf("operateHeaders(%v) got status %q, want %q", tc.metaHeaderFrame, got, want) + } + }) + } +} + +// TestClientDecodeTrailer validates the handling of trailer frames, which may +// or may not also be the initial header frame (header-only response). +func (s) TestClientDecodeTrailer(t *testing.T) { + tests := []struct { + name string + metaHeaderFrame *http2.MetaHeadersFrame + wantEndStreamStatus *status.Status + }{ { - name: "ignore http status and fail for grpc status missing in trailer", + name: "valid_trailer", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, - {Name: ":status", Value: "504"}, + {Name: "grpc-status", Value: "0"}, + {Name: ":status", Value: "200"}, + }, + }, + wantEndStreamStatus: status.New(codes.OK, ""), + }, + { + name: "missing_content_type_in_grpc_mode", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "grpc-status", Value: "0"}, + {Name: ":status", Value: "200"}, }, }, - wantStatus: status.New(codes.OK, ""), - headerChanClosedForEndStream: true, - wantStatusEndStream: status.New(codes.Internal, ""), + wantEndStreamStatus: status.New(codes.OK, ""), }, { - name: "ignore valid http status for grpc", + name: "invalid_grpc_status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, - {Name: "grpc-status", Value: "4"}, - {Name: "grpc-message", Value: "Request timed out: Internal error"}, + {Name: "grpc-status", Value: "xxxx"}, {Name: ":status", Value: "200"}, }, }, - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), + wantEndStreamStatus: status.New( + codes.Internal, + "transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax", + ), + }, + { + name: "missing_grpc_status_in_grpc_mode", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: ":status", Value: "xxxx"}, + }, + }, + wantEndStreamStatus: status.New(codes.Internal, ""), + }, + { + name: "http2_frame_size_exceeds", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Truncated: true, + }, + wantEndStreamStatus: status.New( + codes.Internal, + "peer header list size exceeded limit", + ), }, { - name: "ignore illegal http status for grpc", + name: "missing_grpc_status_in_trailer", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/grpc"}, + }, + }, + wantEndStreamStatus: status.New(codes.Internal, ""), + }, + { + name: "deadline_exceeded_status", metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, {Name: "grpc-status", Value: "4"}, {Name: "grpc-message", Value: "Request timed out: Internal error"}, - {Name: ":status", Value: "thisIsIllegal"}, + {Name: ":status", Value: "200"}, }, }, - wantStatus: status.New(codes.OK, ""), - wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), + wantEndStreamStatus: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"), }, - } { - - t.Run(test.name, func(t *testing.T) { - ts := testStream() - s := testClient(ts) - - test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - }, - } + } - s.operateHeaders(test.metaHeaderFrame) - got := ts.status - want := test.wantStatus - if got.Code() != want.Code() || got.Message() != want.Message() { - t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want) - } - }) - t.Run(fmt.Sprintf("%s-end_stream", test.name), func(t *testing.T) { - ts := testStream() - if test.headerChanClosedForEndStream { - ts.headerChanClosed = 1 - } - s := testClient(ts) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cs := newTestClientStream() + // Mark headerChanClosed to indicate trailer frames. + cs.headerChanClosed = 1 + // Simulate the state where the initial headers have already been processed. + s := newTestHTTP2Client(cs) - test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ + tc.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ FrameHeader: http2.FrameHeader{ - StreamID: 0, + StreamID: 1, Flags: http2.FlagHeadersEndStream, }, } - s.operateHeaders(test.metaHeaderFrame) - got := ts.status - want := test.wantStatusEndStream + s.operateHeaders(tc.metaHeaderFrame) + got := cs.status + want := tc.wantEndStreamStatus if got.Code() != want.Code() || got.Message() != want.Message() { - t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want) + t.Errorf("operateHeaders(%v) got status %q, want %q", tc.metaHeaderFrame, got, want) } }) }