Skip to content
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

Jump to bottom

transport: return http-status when grpc-status is missing #4606

Closed
wants to merge 6 commits into from
Closed

transport: return http-status when grpc-status is missing #4606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e948ab
Only return http status when grpc status is missing
JNProtzman Jul 21, 2021
008beeb
ParseUint, not ParseInt and avoid alloc
JNProtzman Jul 22, 2021
fbb727b
grpc-status over http-status if everything else is good
JNProtzman Aug 23, 2021
64e41a7
Merge branch 'master' of https://github.com/grpc/grpc-go into grpc_st…
JNProtzman Aug 23, 2021
ae2fd9c
Trigger workflow run
JNProtzman Aug 23, 2021
e9a576e
Merge branch 'master' of https://github.com/grpc/grpc-go into grpc_st…
JNProtzman Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 13 additions & 5 deletions internal/transport/http2_client.go
View file
Open in desktop
Expand Up @@ -1304,7 +1304,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
recvCompress string
httpStatusCode *int
httpStatusErr string
rawStatusCode = codes.Unknown
rawStatusCode *codes.Code
// headerError is set if an error is encountered while parsing the headers
headerError string
)
Expand All @@ -1326,13 +1326,14 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
case "grpc-encoding":
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)
return
}
rawStatusCode = codes.Code(uint32(code))
rsc := codes.Code(uint32(code))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't ParseInt above be ParseUint?

And then we need a bounds check on this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted this to ParseUint. What kind of bounds check should I do?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, nevermind the bounds checks. If we were to add a new status code, I believe we'd want to allow it to be propagated by a gRPC proxy without requiring recompilation of the proxy. So any unsigned value is fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK great. I think this is ready for re-review then!

rawStatusCode = &rsc
case "grpc-message":
grpcMessage = decodeGrpcMessage(hf.Value)
case "grpc-status-details-bin":
Expand Down Expand Up @@ -1380,7 +1381,10 @@ 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 {
// grpc-status only takes precedence over http-status if everything else is ok.
if rawStatusCode != nil && contentTypeErr == "" && httpStatusErr == "" {
code = *rawStatusCode
Copy link
Member

Choose a reason for hiding this comment

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

So now here is the big question: what if rawStatusCode is OK?

If you read the spec literally, we should ignore the HTTP status in this case. This appears to be what the other languages do as well, so I suppose we should do that, even though to me it appears to be covering up errors in the system. (@ejona86 @markdroth FYI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you mentioned - this results in potentially covering up an error. Here's an example in tests in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this part of the change is right. A missing content-type error should continue to take priority over a grpc-status of OK (though that's admittedly a gray area and also unlikely to happen). We should still use that precedence, but only change it so we take grpc-status over http-status if everything higher priority is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a stab at this. I'm not 100% sure it's what you meant but I think it is. If we receive a grpc-status but have encountered another error, we'll take the http-status over the grpc-status. Otherwise, we'll default to the http-status.

} else if httpStatusCode != nil {
var ok bool
code, ok = HTTPStatusConvTab[*httpStatusCode]
if !ok {
Expand Down Expand Up @@ -1453,7 +1457,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
}

if statusGen == nil {
statusGen = status.New(rawStatusCode, grpcMessage)
rsc := codes.Unknown
if rawStatusCode != nil {
rsc = *rawStatusCode
}
statusGen = status.New(rsc, grpcMessage)
}

// if client received END_STREAM from server while stream was still active, send RST_STREAM
Expand Down
4 changes: 2 additions & 2 deletions internal/transport/transport_test.go
View file
Open in desktop
Expand Up @@ -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",
),
},
{
Expand Down Expand Up @@ -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"},
Copy link
Member

Choose a reason for hiding this comment

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

If there is a grpc-status, then there should be a grpc-message to go along with it, or we should use the empty string like we do normally -- we should not be using the generated "unexpected HTTP status code" error message. At least, that's what it seems like it should be doing, though it's not mentioned in our docs. Does that seem right to you, @ejona86 / @markdroth?

Maybe set this error code to a different number and change the wantStatus code, so it is clear that's the source of the error, not the 504->Unavailable conversion?

Copy link
Member

Choose a reason for hiding this comment

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

I would say a grpc-message should go along with it from a "we don't hate ourselves" perspective. If there isn't a message, I wouldn't use an empty string; I'd leave the header missing. I wouldn't suggest a useless message like "UNAVAILABLE"; again, I'd prefer just not having it.

But that's a bit neither here nor there. In this case, there is a grpc-status, so you should use that and the grpc-message if it isn't present. I don't think this test is correct: the status shouldn't have a status description.

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave the header missing

This is client-side. If the header is missing, the client would use "" as the message string presented to the user. (We have to give them some string, since it's returned as a string value; we can't use nil.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then yeah, empty string seems appropriate for your API. Java's API lets the description (which we call grpc-message) be null. Makes sense that you wouldn't have such a distinction in Go.

Copy link
Member

Choose a reason for hiding this comment

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

Ping @JNProtzman, I think this is the last issue: to not use the generated http error message along with the grpc-status header value in the status returned to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfawley sorry for the delay here. I'm currently in Iceland and have plans to travel through Europe until Nov. 1st. I won't be able to pick this up until then... sorry!

{Name: ":status", Value: "504"},
},
},
Expand Down
11 changes: 6 additions & 5 deletions test/end2end_test.go
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ import (
"os"
"reflect"
"runtime"
"strconv"
"strings"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -7217,7 +7218,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
})
}
Expand All @@ -7227,7 +7228,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
})
}
Expand All @@ -7236,7 +7237,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
})
}
Expand Down Expand Up @@ -7269,7 +7270,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,
Expand All @@ -7279,7 +7280,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) {
header: []string{
":status", "502",
"content-type", "application/grpc",
"grpc-status", "3",
"grpc-status", "14",
},
errCode: codes.Unavailable,
},
Expand Down