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
mbissa 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 1 commit
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
6 changes: 3 additions & 3 deletions internal/transport/http2_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1467,15 +1467,14 @@ 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
)

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

for _, hf := range frame.Fields {
switch hf.Name {
case "content-type":
Expand Down Expand Up @@ -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 {
Expand Down
47 changes: 36 additions & 11 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 Down Expand Up @@ -2695,34 +2697,55 @@ 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"},
{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, ""),
},
{
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)

Expand All @@ -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() {
Expand All @@ -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)
}
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

@dfawley dfawley Oct 8, 2025

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

@mbissa mbissa Oct 8, 2025

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

@dfawley dfawley Oct 8, 2025

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

@mbissa mbissa Oct 8, 2025

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