From fe87cc542d648742c182801f03d08a409cd744bf Mon Sep 17 00:00:00 2001 From: James Protzman Date: Fri, 21 May 2021 11:41:50 -0400 Subject: [PATCH 01/15] Validate http 200 for all non-end-of-stream messages --- internal/transport/http2_client.go | 2 +- internal/transport/transport_test.go | 2 ++ test/end2end_test.go | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 119f01e3ebc..6bbf9c09b1f 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1313,7 +1313,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - if !isGRPC { + if !isGRPC || (httpStatus != "200" && !endStream) { var ( code = codes.Internal // when header does not include HTTP status, return INTERNAL httpStatusCode int diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 3aef8627714..001f44f12a3 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -1991,6 +1991,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, {Name: "grpc-status", Value: "0"}, + {Name: ":status", Value: "200"}, }, }, // no error @@ -2002,6 +2003,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, {Name: "grpc-status", Value: "xxxx"}, + {Name: ":status", Value: "200"}, }, }, wantStatus: status.New( diff --git a/test/end2end_test.go b/test/end2end_test.go index 76ff07a27c2..b0ce3054264 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -7245,7 +7245,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { "grpc-status", "0", "grpc-status-details-bin", "????", }, - errCode: codes.Internal, + errCode: codes.Unimplemented, }, } { doHTTPHeaderTest(t, test.errCode, test.responseHeader, test.trailer) From 154e68706b17243724a4aafcc87caab6178f91f5 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Fri, 21 May 2021 11:47:25 -0400 Subject: [PATCH 02/15] Add additional test --- test/end2end_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/end2end_test.go b/test/end2end_test.go index b0ce3054264..07eba09e756 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -7217,7 +7217,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { } } -// Testing non-Trailers-only Trailers (delievered in second HEADERS frame) +// Testing non-Trailers-only Trailers (delivered in second HEADERS frame) func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { for _, test := range []struct { responseHeader []string @@ -7247,6 +7247,18 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { }, errCode: codes.Unimplemented, }, + { + responseHeader: []string{ + ":status", "200", + "content-type", "application/grpc", + }, + trailer: []string{ + // malformed grpc-status-details-bin field + "grpc-status", "0", + "grpc-status-details-bin", "????", + }, + errCode: codes.Internal, + }, } { doHTTPHeaderTest(t, test.errCode, test.responseHeader, test.trailer) } From 9cb2546daf261c297e0440cb032c61d60fc224ed Mon Sep 17 00:00:00 2001 From: James Protzman Date: Tue, 25 May 2021 16:55:10 -0400 Subject: [PATCH 03/15] Add test for bad http status code in gRPC mode --- internal/transport/transport_test.go | 31 +++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 001f44f12a3..fb32dcc0f4c 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2047,6 +2047,25 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { "peer header list size exceeded limit", ), }, + { + name: "bad 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"}, + }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + }, + }, + }, + wantStatus: status.New( + codes.Unavailable, + "Gateway Timeout: HTTP status code 504; transport: missing content-type field", + ), + }, } { t.Run(test.name, func(t *testing.T) { ts := &Stream{ @@ -2068,11 +2087,13 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { list: &itemList{}, }, } - test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, + if test.metaHeaderFrame.HeadersFrame == nil { + test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + } } s.operateHeaders(test.metaHeaderFrame) From 71ad20236182d87ff79a41dd7d1141189ead2522 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Wed, 26 May 2021 20:02:04 -0400 Subject: [PATCH 04/15] fix error messages --- internal/transport/http2_client.go | 9 +++++---- internal/transport/http_util.go | 14 +++++++------- internal/transport/transport_test.go | 4 ++-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 6bbf9c09b1f..9d5bb009967 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1316,7 +1316,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { if !isGRPC || (httpStatus != "200" && !endStream) { var ( code = codes.Internal // when header does not include HTTP status, return INTERNAL - httpStatusCode int + httpStatusCode *int ) if httpStatus != "" { @@ -1326,17 +1326,18 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } - httpStatusCode = int(c) + statusCode := int(c) + httpStatusCode = &statusCode var ok bool - code, ok = HTTPStatusConvTab[httpStatusCode] + code, ok = HTTPStatusConvTab[statusCode] if !ok { code = codes.Unknown } } // Verify the HTTP response is a 200. - se := status.New(code, constructHTTPErrMsg(&httpStatusCode, contentTypeErr)) + se := status.New(code, constructHTTPErrMsg(httpStatusCode, contentTypeErr)) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } diff --git a/internal/transport/http_util.go b/internal/transport/http_util.go index 15d775fca3c..6dc122ce120 100644 --- a/internal/transport/http_util.go +++ b/internal/transport/http_util.go @@ -178,15 +178,15 @@ func decodeGRPCStatusDetails(rawDetails string) (*status.Status, error) { func constructHTTPErrMsg(httpStatus *int, contentTypeErr string) string { var errMsgs []string - if httpStatus == nil { - errMsgs = append(errMsgs, "malformed header: missing HTTP status") - } else { - errMsgs = append(errMsgs, fmt.Sprintf("%s: HTTP status code %d", http.StatusText(*(httpStatus)), *httpStatus)) + if httpStatus != nil { + errMsgs = append(errMsgs, fmt.Sprintf( + "unexpected HTTP status code received from server: %d (%s)", + *httpStatus, + http.StatusText(*(httpStatus)), + )) } - if contentTypeErr == "" { - errMsgs = append(errMsgs, "transport: missing content-type field") - } else { + if contentTypeErr != "" { errMsgs = append(errMsgs, contentTypeErr) } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index fb32dcc0f4c..a123aab6218 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2020,7 +2020,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Internal, - ": HTTP status code 0; transport: received the unexpected content-type \"application/json\"", + "transport: received the unexpected content-type \"application/json\"", ), }, { @@ -2063,7 +2063,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Unavailable, - "Gateway Timeout: HTTP status code 504; transport: missing content-type field", + "unexpected HTTP status code received from server: 504 (Gateway Timeout)", ), }, } { From 0e104a348afd4f8681a4df0e378e29f58c637439 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Thu, 27 May 2021 12:30:10 -0400 Subject: [PATCH 05/15] new test case, fix error messaging --- internal/transport/http2_client.go | 3 ++- internal/transport/http_util.go | 5 +++-- internal/transport/transport_test.go | 16 +++++++++++++++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 9d5bb009967..59b7521c7a6 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1266,7 +1266,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { // that the peer is speaking gRPC and we are in gRPC mode. isGRPC = !initialHeader mdata = make(map[string][]string) - contentTypeErr string + contentTypeErr = "missing content-type" grpcMessage string statusGen *status.Status @@ -1283,6 +1283,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { contentTypeErr = fmt.Sprintf("transport: received the unexpected content-type %q", hf.Value) break } + contentTypeErr = "" mdata[hf.Name] = append(mdata[hf.Name], hf.Value) isGRPC = true case "grpc-encoding": diff --git a/internal/transport/http_util.go b/internal/transport/http_util.go index 6dc122ce120..e8d85e1f2c9 100644 --- a/internal/transport/http_util.go +++ b/internal/transport/http_util.go @@ -177,8 +177,9 @@ func decodeGRPCStatusDetails(rawDetails string) (*status.Status, error) { // Format: HTTP status code and its corresponding message + content-type error message. func constructHTTPErrMsg(httpStatus *int, contentTypeErr string) string { var errMsgs []string - - if httpStatus != nil { + if httpStatus == nil { + errMsgs = append(errMsgs, "malformed header: missing HTTP status") + } else { errMsgs = append(errMsgs, fmt.Sprintf( "unexpected HTTP status code received from server: %d (%s)", *httpStatus, diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index a123aab6218..a79e05b2e31 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -1997,6 +1997,20 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { // no error wantStatus: status.New(codes.OK, ""), }, + { + name: "missing content-type header", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "grpc-status", Value: "0"}, + {Name: ":status", Value: "200"}, + }, + }, + // no error + wantStatus: status.New( + codes.Unknown, + "unexpected HTTP status code received from server: 200 (OK); missing content-type", + ), + }, { name: "invalid grpc status header field", metaHeaderFrame: &http2.MetaHeadersFrame{ @@ -2020,7 +2034,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Internal, - "transport: received the unexpected content-type \"application/json\"", + "malformed header: missing HTTP status; transport: received the unexpected content-type \"application/json\"", ), }, { From 17758ef8f3f2573c9bbce012931db420958a989a Mon Sep 17 00:00:00 2001 From: James Protzman Date: Thu, 27 May 2021 20:23:34 -0400 Subject: [PATCH 06/15] address pr comments --- internal/transport/http2_client.go | 65 ++++++++++++++++++---------- internal/transport/http_util.go | 21 --------- internal/transport/transport_test.go | 8 +++- test/end2end_test.go | 10 ++--- 4 files changed, 53 insertions(+), 51 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 59b7521c7a6..3571006b71b 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -24,6 +24,7 @@ import ( "io" "math" "net" + "net/http" "strconv" "strings" "sync" @@ -1269,18 +1270,22 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { contentTypeErr = "missing content-type" grpcMessage string statusGen *status.Status - - httpStatus string - rawStatus string + httpStatusCode *int + httpStatusErr string + rawStatus string // headerError is set if an error is encountered while parsing the headers headerError string ) + if !endStream { + httpStatusErr = "malformed header: missing HTTP status" + } + for _, hf := range frame.Fields { switch hf.Name { case "content-type": if _, validContentType := grpcutil.ContentSubtype(hf.Value); !validContentType { - contentTypeErr = fmt.Sprintf("transport: received the unexpected content-type %q", hf.Value) + contentTypeErr = fmt.Sprintf("transport: received unexpected content-type %q", hf.Value) break } contentTypeErr = "" @@ -1299,7 +1304,27 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { headerError = fmt.Sprintf("transport: malformed grpc-status-details-bin: %v", err) } case ":status": - httpStatus = hf.Value + 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 + } + 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 @@ -1314,31 +1339,25 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } } - if !isGRPC || (httpStatus != "200" && !endStream) { - var ( - code = codes.Internal // when header does not include HTTP status, return INTERNAL - httpStatusCode *int - ) - - if httpStatus != "" { - c, err := strconv.ParseInt(httpStatus, 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) - httpStatusCode = &statusCode + 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[statusCode] + code, ok = HTTPStatusConvTab[*httpStatusCode] if !ok { code = 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, constructHTTPErrMsg(httpStatusCode, contentTypeErr)) + se := status.New(code, strings.Join(errs, "; ")) t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } diff --git a/internal/transport/http_util.go b/internal/transport/http_util.go index e8d85e1f2c9..d8247bcdf69 100644 --- a/internal/transport/http_util.go +++ b/internal/transport/http_util.go @@ -173,27 +173,6 @@ func decodeGRPCStatusDetails(rawDetails string) (*status.Status, error) { return status.FromProto(st), nil } -// constructErrMsg constructs error message to be returned in HTTP fallback mode. -// Format: HTTP status code and its corresponding message + content-type error message. -func constructHTTPErrMsg(httpStatus *int, contentTypeErr string) string { - var errMsgs []string - if httpStatus == nil { - errMsgs = append(errMsgs, "malformed header: missing HTTP status") - } else { - errMsgs = append(errMsgs, fmt.Sprintf( - "unexpected HTTP status code received from server: %d (%s)", - *httpStatus, - http.StatusText(*(httpStatus)), - )) - } - - if contentTypeErr != "" { - errMsgs = append(errMsgs, contentTypeErr) - } - - return strings.Join(errMsgs, "; ") -} - type timeoutUnit uint8 const ( diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index a79e05b2e31..f916b7862b3 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2008,7 +2008,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { // no error wantStatus: status.New( codes.Unknown, - "unexpected HTTP status code received from server: 200 (OK); missing content-type", + "missing content-type", ), }, { @@ -2034,7 +2034,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { }, wantStatus: status.New( codes.Internal, - "malformed header: missing HTTP status; transport: received the unexpected content-type \"application/json\"", + "transport: received unexpected content-type \"application/json\"", ), }, { @@ -2115,6 +2115,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { got := ts.status want := test.wantStatus if got.Code() != want.Code() || got.Message() != want.Message() { + t.Logf("got: %s", got.Message()) + t.Logf("got: %d", got.Code()) + t.Logf("want: %s", want.Message()) + t.Logf("want: %d", want.Code()) t.Fatalf("operateHeaders(%v); status = %v; want %v", test.metaHeaderFrame, got, want) } }) diff --git a/test/end2end_test.go b/test/end2end_test.go index f7086fdc610..914509092e4 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -7254,7 +7254,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { ":status", "403", "content-type", "application/grpc", }, - errCode: codes.Unknown, + errCode: codes.PermissionDenied, }, { // malformed grpc-status. @@ -7263,7 +7263,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { "content-type", "application/grpc", "grpc-status", "abc", }, - errCode: codes.Internal, + errCode: codes.Unavailable, }, { // Malformed grpc-tags-bin field. @@ -7273,7 +7273,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { "grpc-status", "0", "grpc-tags-bin", "???", }, - errCode: codes.Internal, + errCode: codes.Unavailable, }, { // gRPC status error. @@ -7282,7 +7282,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { "content-type", "application/grpc", "grpc-status", "3", }, - errCode: codes.InvalidArgument, + errCode: codes.Unavailable, }, } { doHTTPHeaderTest(t, test.errCode, test.header) @@ -7305,7 +7305,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) { // trailer missing grpc-status ":status", "502", }, - errCode: codes.Unknown, + errCode: codes.Unavailable, }, { responseHeader: []string{ From 399b76e8e64945b4aeef7c032390648fd86c6558 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Fri, 4 Jun 2021 10:16:12 -0400 Subject: [PATCH 07/15] Fix test log, pr comments --- internal/transport/http2_client.go | 2 +- internal/transport/transport_test.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 3571006b71b..a86b0c19ee3 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1267,7 +1267,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { // that the peer is speaking gRPC and we are in gRPC mode. isGRPC = !initialHeader mdata = make(map[string][]string) - contentTypeErr = "missing content-type" + contentTypeErr = "malformed header: missing HTTP content-type" grpcMessage string statusGen *status.Status httpStatusCode *int diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 5161766bd8b..c9887a394ea 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2005,10 +2005,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: ":status", Value: "200"}, }, }, - // no error wantStatus: status.New( codes.Unknown, - "missing content-type", + "malformed header: missing HTTP content-type", ), }, { @@ -2115,11 +2114,13 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { got := ts.status want := test.wantStatus if got.Code() != want.Code() || got.Message() != want.Message() { - t.Logf("got: %s", got.Message()) - t.Logf("got: %d", got.Code()) - t.Logf("want: %s", want.Message()) - t.Logf("want: %d", want.Code()) - t.Fatalf("operateHeaders(%v); status = %v; want %v", test.metaHeaderFrame, got, want) + t.Fatalf("operateHeaders(%v); status = \ngot - code: %d message: %s\nwant - code: %d message: %s", + test.metaHeaderFrame, + got.Code(), + got.Message(), + want.Code(), + want.Message(), + ) } }) } From 144692003ec9d27681fc45816e19a5cc89dbbb20 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Mon, 7 Jun 2021 18:55:41 -0400 Subject: [PATCH 08/15] Use status in error instead of proto, add String method to Status --- internal/status/status.go | 14 +++++++++----- internal/transport/transport_test.go | 8 +------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/status/status.go b/internal/status/status.go index 710223b8ded..f2d43e56d89 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -97,7 +97,7 @@ func (s *Status) Err() error { if s.Code() == codes.OK { return nil } - return &Error{e: s.Proto()} + return &Error{s: s} } // WithDetails returns a new status with the provided details messages appended to the status. @@ -136,19 +136,23 @@ func (s *Status) Details() []interface{} { return details } +func (s *Status) String() string { + return fmt.Sprintf("rpc error: code = %s desc = %s", s.Code(), s.Message()) +} + // Error wraps a pointer of a status proto. It implements error and Status, // and a nil *Error should never be returned by this package. type Error struct { - e *spb.Status + s *Status } func (e *Error) Error() string { - return fmt.Sprintf("rpc error: code = %s desc = %s", codes.Code(e.e.GetCode()), e.e.GetMessage()) + return e.s.String() } // GRPCStatus returns the Status represented by se. func (e *Error) GRPCStatus() *Status { - return FromProto(e.e) + return FromProto(e.s.s) } // Is implements future error.Is functionality. @@ -158,5 +162,5 @@ func (e *Error) Is(target error) bool { if !ok { return false } - return proto.Equal(e.e, tse.e) + return proto.Equal(e.s.s, tse.s.s) } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index c9887a394ea..d625f3cc124 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2114,13 +2114,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { got := ts.status want := test.wantStatus if got.Code() != want.Code() || got.Message() != want.Message() { - t.Fatalf("operateHeaders(%v); status = \ngot - code: %d message: %s\nwant - code: %d message: %s", - test.metaHeaderFrame, - got.Code(), - got.Message(), - want.Code(), - want.Message(), - ) + t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want) } }) } From efaf7abb5b33288185a97115973cd5c7a1da54c6 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Tue, 8 Jun 2021 20:42:52 -0400 Subject: [PATCH 09/15] return status directly --- internal/status/status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/status/status.go b/internal/status/status.go index f2d43e56d89..e5c6513edd1 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -152,7 +152,7 @@ func (e *Error) Error() string { // GRPCStatus returns the Status represented by se. func (e *Error) GRPCStatus() *Status { - return FromProto(e.s.s) + return e.s } // Is implements future error.Is functionality. From 6230a62ba91df447d8182eb416bf60d64bf7672c Mon Sep 17 00:00:00 2001 From: James Protzman Date: Wed, 16 Jun 2021 21:48:49 -0400 Subject: [PATCH 10/15] pr comment --- internal/transport/http2_client.go | 2 +- internal/transport/transport_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index a86b0c19ee3..ee44def26fc 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1277,7 +1277,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { headerError string ) - if !endStream { + if initialHeader { httpStatusErr = "malformed header: missing HTTP status" } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index d625f3cc124..8f36c65d8bc 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2029,10 +2029,11 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/json"}, + {Name: ":status", Value: "200"}, }, }, wantStatus: status.New( - codes.Internal, + codes.Unknown, "transport: received unexpected content-type \"application/json\"", ), }, From 43990b62bc19f55258124f22c81d3b92e688482c Mon Sep 17 00:00:00 2001 From: James Protzman Date: Sun, 20 Jun 2021 18:53:24 -0400 Subject: [PATCH 11/15] test updates --- internal/transport/transport_test.go | 67 +++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 11 deletions(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 8f36c65d8bc..d19f333b5e3 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -1993,6 +1993,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "200"}, }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, }, // no error wantStatus: status.New(codes.OK, ""), @@ -2004,6 +2010,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "200"}, }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, }, wantStatus: status.New( codes.Unknown, @@ -2018,6 +2030,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "xxxx"}, {Name: ":status", Value: "200"}, }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, }, wantStatus: status.New( codes.Internal, @@ -2029,12 +2047,17 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/json"}, - {Name: ":status", Value: "200"}, + }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, }, }, wantStatus: status.New( - codes.Unknown, - "transport: received unexpected content-type \"application/json\"", + codes.Internal, + "malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"", ), }, { @@ -2044,6 +2067,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { // No content type provided then fallback into handling http error. {Name: ":status", Value: "xxxx"}, }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, }, wantStatus: status.New( codes.Internal, @@ -2055,6 +2084,12 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: nil, Truncated: true, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, }, wantStatus: status.New( codes.Internal, @@ -2080,6 +2115,24 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { "unexpected HTTP status code received from server: 504 (Gateway Timeout)", ), }, + { + name: "missing http status", + metaHeaderFrame: &http2.MetaHeadersFrame{ + Fields: []hpack.HeaderField{ + {Name: "content-type", Value: "application/grpc"}, + }, + HeadersFrame: &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, + }, + }, + }, + wantStatus: status.New( + codes.Internal, + "malformed header: missing HTTP status", + ), + }, } { t.Run(test.name, func(t *testing.T) { ts := &Stream{ @@ -2101,14 +2154,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { list: &itemList{}, }, } - if test.metaHeaderFrame.HeadersFrame == nil { - test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - } - } s.operateHeaders(test.metaHeaderFrame) From 8b0e76145df4b641250f823ce51484d77dbd56a8 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Thu, 24 Jun 2021 23:48:51 -0400 Subject: [PATCH 12/15] fix go vet issue --- internal/transport/transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index d19f333b5e3..7397507d094 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -2131,7 +2131,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { wantStatus: status.New( codes.Internal, "malformed header: missing HTTP status", - ), + ), }, } { t.Run(test.name, func(t *testing.T) { From 472d502a9d3a036fcf0a86405efc0251428cb85f Mon Sep 17 00:00:00 2001 From: James Protzman Date: Thu, 1 Jul 2021 23:16:37 -0400 Subject: [PATCH 13/15] test endstream and not endstream --- internal/transport/http2_client.go | 25 +++--- internal/transport/transport_test.go | 111 ++++++++++++--------------- 2 files changed, 62 insertions(+), 74 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index ee44def26fc..a02a00b27ef 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1272,7 +1272,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { statusGen *status.Status httpStatusCode *int httpStatusErr string - rawStatus string + rawStatusCode *codes.Code // headerError is set if an error is encountered while parsing the headers headerError string ) @@ -1294,7 +1294,14 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { case "grpc-encoding": s.recvCompress = hf.Value case "grpc-status": - rawStatus = hf.Value + code, err := strconv.ParseInt(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 + } + c := codes.Code(uint32(code)) + rawStatusCode = &c case "grpc-message": grpcMessage = decodeGrpcMessage(hf.Value) case "grpc-status-details-bin": @@ -1414,17 +1421,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } if statusGen == nil { - rawStatusCode := codes.Unknown - if rawStatus != "" { - code, err := strconv.ParseInt(rawStatus, 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.Unknown + if rawStatusCode != nil { + rsc = *rawStatusCode } - statusGen = status.New(rawStatusCode, grpcMessage) + statusGen = status.New(rsc, grpcMessage) } // 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 7397507d094..92990eaf7b2 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -1978,6 +1978,31 @@ func (s) TestClientHandshakeInfo(t *testing.T) { } func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { + testStream := func() *Stream { + return &Stream{ + done: make(chan struct{}), + headerChan: make(chan struct{}), + buf: &recvBuffer{ + c: make(chan recvMsg), + mu: sync.Mutex{}, + }, + } + } + + testClient := func(ts *Stream) *http2Client { + return &http2Client{ + mu: sync.Mutex{}, + activeStreams: map[uint32]*Stream{ + 0: ts, + }, + controlBuf: &controlBuffer{ + ch: make(chan struct{}), + done: make(chan struct{}), + list: &itemList{}, + }, + } + } + for _, test := range []struct { name string // input @@ -1993,12 +2018,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "200"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, // no error wantStatus: status.New(codes.OK, ""), @@ -2010,12 +2029,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "200"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Unknown, @@ -2030,12 +2043,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "xxxx"}, {Name: ":status", Value: "200"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Internal, @@ -2048,12 +2055,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/json"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Internal, @@ -2067,12 +2068,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { // No content type provided then fallback into handling http error. {Name: ":status", Value: "xxxx"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Internal, @@ -2084,12 +2079,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { metaHeaderFrame: &http2.MetaHeadersFrame{ Fields: nil, Truncated: true, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Internal, @@ -2104,11 +2093,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { {Name: "grpc-status", Value: "0"}, {Name: ":status", Value: "504"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - }, - }, }, wantStatus: status.New( codes.Unavailable, @@ -2121,12 +2105,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { Fields: []hpack.HeaderField{ {Name: "content-type", Value: "application/grpc"}, }, - HeadersFrame: &http2.HeadersFrame{ - FrameHeader: http2.FrameHeader{ - StreamID: 0, - Flags: http2.FlagHeadersEndStream, - }, - }, }, wantStatus: status.New( codes.Internal, @@ -2134,24 +2112,33 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { ), }, } { + t.Run(test.name, func(t *testing.T) { - ts := &Stream{ - done: make(chan struct{}), - headerChan: make(chan struct{}), - buf: &recvBuffer{ - c: make(chan recvMsg), - mu: sync.Mutex{}, + ts := testStream() + s := testClient(ts) + + test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, }, } - s := &http2Client{ - mu: sync.Mutex{}, - activeStreams: map[uint32]*Stream{ - 0: ts, - }, - controlBuf: &controlBuffer{ - ch: make(chan struct{}), - done: make(chan struct{}), - list: &itemList{}, + + 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() + s := testClient(ts) + + test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ + FrameHeader: http2.FrameHeader{ + StreamID: 0, + Flags: http2.FlagHeadersEndStream, }, } From adfc275a2ea177ef325581d09184d836b93ddbcc Mon Sep 17 00:00:00 2001 From: James Protzman Date: Thu, 1 Jul 2021 23:27:51 -0400 Subject: [PATCH 14/15] test grpc-status, not http status --- test/end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/end2end_test.go b/test/end2end_test.go index fe260caaa67..fc112bf21c3 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -7263,7 +7263,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) { "content-type", "application/grpc", "grpc-status", "abc", }, - errCode: codes.Unavailable, + errCode: codes.Internal, }, { // Malformed grpc-tags-bin field. From 9207e93a8a597d6d5922ccb442a5620f34a1c112 Mon Sep 17 00:00:00 2001 From: James Protzman Date: Tue, 13 Jul 2021 18:51:38 -0400 Subject: [PATCH 15/15] Minor suggestions --- internal/transport/http2_client.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index a5a7e52331d..9592d443d3e 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -1286,7 +1286,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { statusGen *status.Status httpStatusCode *int httpStatusErr string - rawStatusCode *codes.Code + rawStatusCode = codes.Unknown // headerError is set if an error is encountered while parsing the headers headerError string ) @@ -1314,8 +1314,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) return } - c := codes.Code(uint32(code)) - rawStatusCode = &c + rawStatusCode = codes.Code(uint32(code)) case "grpc-message": grpcMessage = decodeGrpcMessage(hf.Value) case "grpc-status-details-bin": @@ -1435,11 +1434,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { } if statusGen == nil { - rsc := codes.Unknown - if rawStatusCode != nil { - rsc = *rawStatusCode - } - statusGen = status.New(rsc, grpcMessage) + statusGen = status.New(rawStatusCode, grpcMessage) } // if client received END_STREAM from server while stream was still active, send RST_STREAM