grpc-web: Fix 503 handling for gRPC Web text#14516
grpc-web: Fix 503 handling for gRPC Web text#14516dio merged 59 commits intoenvoyproxy:mainfrom dio:grpc-web-text
Conversation
This fixes the 503 handling for gRPC Web requests with text content-type. Previously, when the upstream cannot be contacted (local reset or connection reset), gRPC Web filter always expect gRPC response from local reply handler. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks, some comments to get this started
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for tackling this! A few thoughts
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
The error message from CI: ##[error]System.IO.IOException: No space left on device |
| return Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling") && | ||
| !Grpc::Common::isGrpcResponseHeaders(headers, end_stream) && | ||
| !(Http::Utility::getResponseStatus(headers) == enumToInt(Http::Code::OK) && |
There was a problem hiding this comment.
mind commenting why we do not convert if the response code is OK?
There was a problem hiding this comment.
Sure. Commented. Hope it clarifies, but if you see it is wrong, please let me know. Thanks!
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
@snowp please let me know if you need me to introduce more changes to this PR. Thank you! |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks for working on this! Just some questions/suggestions
|
|
||
| void GrpcWebFilter::mergeAndLimitNonProtoEncodedResponseData(Buffer::OwnedImpl& output, | ||
| Buffer::Instance* last_data) { | ||
| const auto* encoding_buffer = encoder_callbacks_->encodingBuffer(); |
There was a problem hiding this comment.
Instead of doing all this gymnastics to create a new buffer, can you just call addEncodedData and then read from the buffer?
There was a problem hiding this comment.
Do you mean addEncodedData for the last_data?
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
/retest |
|
Retrying Azure Pipelines: |
5 participants
Commit Message: This fixes the 503 handling for gRPC Web requests with
application/grpc-web-textcontent-type. Previously, when the upstream cannot be contacted (since local reset or connection reset), the gRPC Web filter always expects gRPC response from the local reply handler. This fix is guarded byenvoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handlingfeature flag.Additional Description: #13831 (comment).
Risk Level: Low
Testing: Integration test, and manual test
Docs Changes: N/A
Release Notes: Added, since adding
envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handlingfeature flag.Platform-Specific Features: N/A
Fixes #13831
Signed-off-by: Dhi Aurrahman dio@rockybars.com