Skip to content

client: ignore http status header for gRPC streams #8548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+213 −122
Open

client: ignore http status header for gRPC streams #8548

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
50b5337
ignoring http status when content type is application/grpc grpc#8486
mbissa Aug 28, 2025
b1a7265
addressing review comments for grpc#8486
mbissa Sep 9, 2025
b4cce55
Merge remote-tracking branch 'origin/master' into ignore-status-for-g…
mbissa Sep 9, 2025
7087664
addressing review comments for grpc#8486
mbissa Sep 12, 2025
aceb8be
removing special case for 200 http status code
mbissa Sep 15, 2025
734cd4d
addressing review comments
mbissa Sep 17, 2025
bebc8d3
splitting the test for headers and trailers
mbissa Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 40 additions & 39 deletions internal/transport/http2_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,17 +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.Unknown
rawStatusCode = codes.Internal
// headerError is set if an error is encountered while parsing the headers
headerError string
headerError string
receivedHTTPStatus string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We can omit the received prefix int the variable name, shortening it to httpStatus since it doesn't effect readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

)

if initialHeader {
httpStatusErr = "malformed header: missing HTTP status"
}

for _, hf := range frame.Fields {
switch hf.Name {
case "content-type":
Expand All @@ -1499,32 +1495,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
case "grpc-message":
grpcMessage = decodeGrpcMessage(hf.Value)
case ":status":
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
}
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)
}
return
if !isGRPC {
receivedHTTPStatus = hf.Value
}
httpStatusCode = &statusCode
if statusCode == 200 {
httpStatusErr = ""
break
}

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
Expand All @@ -1539,25 +1512,53 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
}
}

if !isGRPC || httpStatusErr != "" {
var code = codes.Internal // when header does not include HTTP status, return INTERNAL
//if not a gRPC response - evaluate entire http status and process close stream / response
//for 200 -> Unknown, else decode the error
if !isGRPC {
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this explicit check required? Since 200 isn't present in the HTTPStatusConvTab map, I think it may be omitted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is when no error information can be derived from http status since it is 200, we can simply return Unknown code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default case also adds a error message which seems useful. I think we should remove the special case unless it requires different handling in code. We don't need to worry too much about optimizing the error case, since they're expected to be infrequent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1; please remove special cases when possible most of the time. Less code = easier to understand code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

default:
// Any other status code (e.g., "404", "503"). We must parse it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use complete sentences. It can still be short:

Suggested change
// Any other status code (e.g., "404", "503"). We must parse it.
// Parse the status code (e.g. "200", 404").

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we cast to an int here? It seems unnecessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

statusCode := int(c)
if statusCode >= 100 && statusCode < 200 {
//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(
"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
}
Expand Down
91 changes: 73 additions & 18 deletions internal/transport/transport_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2618,6 +2618,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
metaHeaderFrame *http2.MetaHeadersFrame
// output
wantStatus *status.Status
// end stream output
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this means.

Let's make sure all our comments are useful and easy to understand, or just remove the comment if it's mostly just redundant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed unnecessary comments.

wantStatusEndStream *status.Status
}{
{
name: "valid header",
Expand All @@ -2629,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",
Expand All @@ -2643,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",
Expand All @@ -2657,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",
Expand All @@ -2669,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",
Expand All @@ -2682,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",
Expand All @@ -2693,32 +2712,69 @@ 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: "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"},
{Name: "grpc-status", Value: "0"},
{Name: ":status", Value: "504"},
},
},
wantStatus: status.New(
codes.Unavailable,
"unexpected HTTP status code received from server: 504 (Gateway Timeout)",
),
wantStatus: status.New(codes.OK, ""),
wantStatusEndStream: 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: "ignore valid 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: "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"),
},
} {

Expand All @@ -2733,7 +2789,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() {
Expand All @@ -2755,6 +2810,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)
}
Expand Down Expand Up @@ -3159,17 +3217,14 @@ func (s) TestClientTransport_Handle1xxHeaders(t *testing.T) {
wantStatus *status.Status
}{
{
name: "1xx with END_STREAM is error",
name: "1xx with END_STREAM will be ignored",
metaHeaderFrame: &http2.MetaHeadersFrame{
Fields: []hpack.HeaderField{
{Name: ":status", Value: "100"},
},
},
httpFlags: http2.FlagHeadersEndStream,
wantStatus: status.New(
codes.Internal,
"protocol error: informational header with status code 100 must not have END_STREAM set",
),
httpFlags: http2.FlagHeadersEndStream,
wantStatus: nil,
},
{
name: "1xx without END_STREAM is ignored",
Expand Down
26 changes: 12 additions & 14 deletions test/http_header_end2end_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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) {
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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 {
Expand Down