client: ignore http status header for gRPC streams #8548
Conversation
Fixes grpc#8486 When a gRPC response is received with content type application/grpc, we then do not expect any information in the http status and the status information needs to be conveyed by gRPC status only. In case of missing gRPC status, we will throw an Internal error instead of Unknown in accordance with https://grpc.io/docs/guides/status-codes/ Changes : - Ignore http status in case of content type application/grpc - Change the default rawStatusCode to return Internal for missing grpc status RELEASE NOTES: * client : Ignore http status for gRPC mode and return Internal error for missing grpc status
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8548 +/- ##
==========================================
+ Coverage 81.64% 82.02% +0.37%
==========================================
Files 413 415 +2
Lines 40621 40684 +63
==========================================
+ Hits 33167 33370 +203
+ Misses 5991 5923 -68
+ Partials 1463 1391 -72
🚀 New features to boost your workflow:
|
|
In the grpc-go repo, we use the assignee to indicate who the review is pending on (author or reviewer). We ignore the reviewer status (the yellow pending re-review dot). Assigning to myself. |
|
For the PR title, we add the name of the Go package (e.g. |
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
2 times, most recently
from
e1f6dbc to
f123124
Compare
…rpc-content-type-8486
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
from
f123124 to
b4cce55
Compare
arjan-bal
changed the title
mbissa
changed the title
mbissa
changed the title
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
There was a problem hiding this comment.
nit: We can omit the received prefix int the variable name, shortening it to httpStatus since it doesn't effect readability.
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
Is this explicit check required? Since 200 isn't present in the HTTPStatusConvTab map, I think it may be omitted.
There was a problem hiding this comment.
The idea is when no error information can be derived from http status since it is 200, we can simply return Unknown code.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+1; please remove special cases when possible most of the time. Less code = easier to understand code.
arjan-bal
changed the title
arjan-bal
changed the title
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
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.
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
+1; please remove special cases when possible most of the time. Less code = easier to understand code.
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
from
9c920c8 to
734cd4d
Compare
| // 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 a non gRPC response is received, then evaluate entire http status and | ||
| // process close stream / response. |
| switch httpStatus { | ||
| case "": | ||
| httpStatusErr = "malformed header: missing HTTP status" | ||
| default: |
There was a problem hiding this comment.
Nit: I'm not sure I like the use of a switch instead of an if/else. It seems this would be simpler / easier to read as if httpStatus == "" { ... } else { ... }
| case "": | ||
| httpStatusErr = "malformed header: missing HTTP status" | ||
| default: | ||
| // Any other status code (e.g., "404", "503"). We must parse it. |
There was a problem hiding this comment.
Please use complete sentences. It can still be short:
| // Any other status code (e.g., "404", "503"). We must parse it. | |
| // Parse the status code (e.g. "200", 404"). |
| "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 |
There was a problem hiding this comment.
Lots of options, here, e.g.:
| // In case of informational headers return | |
| // In case of informational headers, return. |
| se := status.New(grpcErrorCode, fmt.Sprintf("transport: malformed http-status: %v", err)) | ||
| t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) | ||
| return | ||
| } |
There was a problem hiding this comment.
Why do we cast to an int here? It seems unnecessary.
| metaHeaderFrame *http2.MetaHeadersFrame | ||
| // output | ||
| wantStatus *status.Status | ||
| // end stream output |
There was a problem hiding this comment.
I'm not sure what this means.
Let's make sure all our comments are useful and easy to understand, or just remove the comment if it's mostly just redundant.
| wantStatus *status.Status | ||
| // end stream output | ||
| wantStatusEndStream *status.Status | ||
| // head channel closed |
There was a problem hiding this comment.
Similar. What's a "head channel"? The variable name is telling me more than this comment.
| // for end stream, when we are already talking gRPC, we expect | ||
| // grpc - status and fail for it, we will ignore http status. |
|
|
||
| got := ts.status | ||
| want := test.wantStatus | ||
| want := test.wantStatusEndStream |
There was a problem hiding this comment.
This test is feeling pretty complicated to me now. Would it be better to just split this test into two separate test functions since it's running two different tests and has multiple different assertions?
Fixes #8486
When a gRPC response is received with content type application/grpc, we then do not expect any information in the http status and the status information needs to be conveyed by gRPC status only.
In case of missing gRPC status, we will throw an Internal error instead of Unknown in accordance with https://grpc.io/docs/guides/status-codes/
RELEASE NOTES: