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

Merged
merged 8 commits into from
Oct 8, 2025
Merged

client: ignore http status header for gRPC streams #8548

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 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
09cf708
Merge branch 'master' into ignore-status-for-grpc-content-type-8486
mbissa Oct 8, 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
81 changes: 43 additions & 38 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
Contributor 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,9 +1495,39 @@ 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 !isGRPC {
receivedHTTPStatus = hf.Value
}
default:
if isReservedHeader(hf.Name) && !isWhitelistedHeader(hf.Name) {
break
}
v, err := decodeMetadataHeader(hf.Name, hf.Value)
if err != nil {
headerError = fmt.Sprintf("transport: malformed %s: %v", hf.Name, err)
logger.Warningf("Failed to decode metadata header (%q, %q): %v", hf.Name, hf.Value, err)
break
}
mdata[hf.Name] = append(mdata[hf.Name], v)
}
}

// 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 {
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
Contributor 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
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor 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.
c, err := strconv.ParseInt(receivedHTTPStatus, 10, 32)
if err != nil {
se := status.New(codes.Internal, fmt.Sprintf("transport: malformed http-status: %v", err))
se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err))
t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream)
return
}
Expand All @@ -1512,52 +1538,31 @@ 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
return
}
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
}
v, err := decodeMetadataHeader(hf.Name, hf.Value)
if err != nil {
headerError = fmt.Sprintf("transport: malformed %s: %v", hf.Name, err)
logger.Warningf("Failed to decode metadata header (%q, %q): %v", hf.Name, hf.Value, err)
break
}
mdata[hf.Name] = append(mdata[hf.Name], v)
}
}

if !isGRPC || httpStatusErr != "" {
var code = codes.Internal // when header does not include HTTP status, return INTERNAL

if httpStatusCode != nil {
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
82 changes: 68 additions & 14 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
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 @@ -2752,9 +2807,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
}

s.operateHeaders(test.metaHeaderFrame)

got := ts.status
want := test.wantStatus
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
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",
Copy link
Member

Choose a reason for hiding this comment

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

It seems like you have changed this test instead of adding a new one? Shouldn't we still have a test that sets these exact header fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missing grpc status is no longer relevant in the the initial headers - it is tested in the trailers test where grpc status is expected/mandatory.

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",
Copy link
Member

Choose a reason for hiding this comment

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

Similar - don't we still want to have a test with an illegal grpc-status-details-bin and status 200? We can also have this new test case, but it seems like having more test cases is always better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the content type is application/grpc (as was in previous version of the test) the http status is immaterial (200/404 etc won't make a difference) and it is tested in the test case above this with 404. This was more important variation of data where we check without correct 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
Loading