Skip to content

Commit b1a7265

Browse files
mbissambissa
committed
addressing review comments for #8486 
1 parent 50b5337 commit b1a7265

File tree

2 files changed

+72
-38
lines changed

2 files changed

+72
-38
lines changed

‎internal/transport/http2_client.go‎

Lines changed: 33 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,16 +1465,13 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
14651465
contentTypeErr = "malformed header: missing HTTP content-type"
14661466
grpcMessage string
14671467
recvCompress string
1468-
httpStatusCode *int
14691468
httpStatusErr string
14701469
rawStatusCode = codes.Internal
14711470
// headerError is set if an error is encountered while parsing the headers
1472-
headerError string
1471+
headerError string
1472+
receivedHttpStatus string
14731473
)
14741474

1475-
if initialHeader {
1476-
httpStatusErr = "malformed header: missing HTTP status"
1477-
}
14781475
for _, hf := range frame.Fields {
14791476
switch hf.Name {
14801477
case "content-type":
@@ -1498,27 +1495,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
14981495
case "grpc-message":
14991496
grpcMessage = decodeGrpcMessage(hf.Value)
15001497
case ":status":
1501-
if hf.Value == "200" {
1502-
httpStatusErr = ""
1503-
statusCode := 200
1504-
httpStatusCode = &statusCode
1505-
break
1506-
}
1507-
1508-
c, err := strconv.ParseInt(hf.Value, 10, 32)
1509-
if err != nil {
1510-
se := status.New(codes.Internal, fmt.Sprintf("transport: malformed http-status: %v", err))
1511-
t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream)
1512-
return
1498+
if !isGRPC {
1499+
receivedHttpStatus = hf.Value
15131500
}
1514-
statusCode := int(c)
1515-
httpStatusCode = &statusCode
1516-
1517-
httpStatusErr = fmt.Sprintf(
1518-
"unexpected HTTP status code received from server: %d (%s)",
1519-
statusCode,
1520-
http.StatusText(statusCode),
1521-
)
15221501
default:
15231502
if isReservedHeader(hf.Name) && !isWhitelistedHeader(hf.Name) {
15241503
break
@@ -1533,26 +1512,48 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
15331512
}
15341513
}
15351514

1536-
//if response is not grpc or endstream doesn't receive a grpc status, fall back to http error code
1515+
//if not a gRPC response - evaluate entire http status and process close stream / response
1516+
//for 200 -> Unknown, else decode the error
15371517
if !isGRPC {
1538-
var code = codes.Internal // when header does not include HTTP status, return INTERNAL
1518+
var grpcErrorCode = codes.Internal // when header does not include HTTP status, return INTERNAL
1519+
var errs []string
15391520

1540-
if httpStatusCode != nil {
1521+
switch receivedHttpStatus {
1522+
case "":
1523+
httpStatusErr = "malformed header: missing HTTP status"
1524+
case "200":
1525+
grpcErrorCode = codes.Unknown
1526+
default:
1527+
// Any other status code (e.g., "404", "503"). We must parse it.
1528+
c, err := strconv.ParseInt(receivedHttpStatus, 10, 32)
1529+
if err != nil {
1530+
se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err))
1531+
t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream)
1532+
return
1533+
}
1534+
statusCode := int(c)
1535+
httpStatusErr = fmt.Sprintf(
1536+
"unexpected HTTP status code received from server: %d (%s)",
1537+
statusCode,
1538+
http.StatusText(statusCode),
1539+
)
15411540
var ok bool
1542-
code, ok = HTTPStatusConvTab[*httpStatusCode]
1541+
grpcErrorCode, ok = HTTPStatusConvTab[statusCode]
15431542
if !ok {
1544-
code = codes.Unknown
1543+
grpcErrorCode = codes.Unknown
15451544
}
1545+
15461546
}
1547-
var errs []string
1547+
15481548
if httpStatusErr != "" {
15491549
errs = append(errs, httpStatusErr)
15501550
}
1551+
15511552
if contentTypeErr != "" {
15521553
errs = append(errs, contentTypeErr)
15531554
}
15541555
// Verify the HTTP response is a 200.
1555-
se := status.New(code, strings.Join(errs, "; "))
1556+
se := status.New(grpcErrorCode, strings.Join(errs, "; "))
15561557
t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream)
15571558
return
15581559
}

‎internal/transport/transport_test.go‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2631,7 +2631,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26312631
},
26322632
},
26332633
// no error
2634-
wantStatus: status.New(codes.OK, ""),
2634+
wantStatus: status.New(codes.OK, ""),
2635+
wantStatusEndStream: status.New(codes.OK, ""),
26352636
},
26362637
{
26372638
name: "missing content-type header",
@@ -2645,6 +2646,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26452646
codes.Unknown,
26462647
"malformed header: missing HTTP content-type",
26472648
),
2649+
wantStatusEndStream: status.New(
2650+
codes.Unknown,
2651+
"malformed header: missing HTTP content-type",
2652+
),
26482653
},
26492654
{
26502655
name: "invalid grpc status header field",
@@ -2659,6 +2664,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26592664
codes.Internal,
26602665
"transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax",
26612666
),
2667+
wantStatusEndStream: status.New(
2668+
codes.Internal,
2669+
"transport: malformed grpc-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax",
2670+
),
26622671
},
26632672
{
26642673
name: "invalid http content type",
@@ -2671,6 +2680,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26712680
codes.Internal,
26722681
"malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"",
26732682
),
2683+
wantStatusEndStream: status.New(
2684+
codes.Internal,
2685+
"malformed header: missing HTTP status; transport: received unexpected content-type \"application/json\"",
2686+
),
26742687
},
26752688
{
26762689
name: "http fallback and invalid http status",
@@ -2684,6 +2697,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26842697
codes.Internal,
26852698
"transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax",
26862699
),
2700+
wantStatusEndStream: status.New(
2701+
codes.Internal,
2702+
"transport: malformed http-status: strconv.ParseInt: parsing \"xxxx\": invalid syntax",
2703+
),
26872704
},
26882705
{
26892706
name: "http2 frame size exceeds",
@@ -2695,6 +2712,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
26952712
codes.Internal,
26962713
"peer header list size exceeded limit",
26972714
),
2715+
wantStatusEndStream: status.New(
2716+
codes.Internal,
2717+
"peer header list size exceeded limit",
2718+
),
26982719
},
26992720
{
27002721
name: "ignoring bad http status in grpc mode",
@@ -2705,7 +2726,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
27052726
{Name: ":status", Value: "504"},
27062727
},
27072728
},
2708-
wantStatus: status.New(codes.OK, ""),
2729+
wantStatus: status.New(codes.OK, ""),
2730+
wantStatusEndStream: status.New(codes.OK, ""),
27092731
},
27102732
{
27112733
name: "missing http status and grpc status",
@@ -2729,7 +2751,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
27292751
wantStatusEndStream: status.New(codes.Internal, ""),
27302752
},
27312753
{
2732-
name: "trailer only grpc timeout ignores http status",
2754+
name: "ignore valid http status for grpc",
27332755
metaHeaderFrame: &http2.MetaHeadersFrame{
27342756
Fields: []hpack.HeaderField{
27352757
{Name: "content-type", Value: "application/grpc"},
@@ -2738,14 +2760,25 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
27382760
{Name: ":status", Value: "200"},
27392761
},
27402762
},
2763+
wantStatus: status.New(codes.OK, ""),
2764+
wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"),
2765+
},
2766+
{
2767+
name: "ignore illegal http status for grpc",
2768+
metaHeaderFrame: &http2.MetaHeadersFrame{
2769+
Fields: []hpack.HeaderField{
2770+
{Name: "content-type", Value: "application/grpc"},
2771+
{Name: "grpc-status", Value: "4"},
2772+
{Name: "grpc-message", Value: "Request timed out: Internal error"},
2773+
{Name: ":status", Value: "thisIsIllegal"},
2774+
},
2775+
},
2776+
wantStatus: status.New(codes.OK, ""),
27412777
wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"),
27422778
},
27432779
} {
27442780

27452781
t.Run(test.name, func(t *testing.T) {
2746-
if test.wantStatus == nil {
2747-
t.Skip()
2748-
}
27492782
ts := testStream()
27502783
s := testClient(ts)
27512784

0 commit comments

Comments
 (0)