reverse bridge: use local reply instead of ContinueAndEndStream#13205
reverse bridge: use local reply instead of ContinueAndEndStream#13205snowp merged 3 commits intoenvoyproxy:masterfrom
Conversation
Instead of returning ContinueAndEndStream we can respond with a local reply. This removes the only use case of ContinueAndEndStream in non-test code. Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
Some context: https://envoyproxy.slack.com/archives/C78HA81DH/p1600707354050600 tl;dr this will let us remove ContinueAndEndStream and reduce some of the complexity of the HCM |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
| } | ||
| decoder_callbacks_->sendLocalReply(Http::Code::OK, badContentTypeMessage(headers), nullptr, | ||
| Grpc::Status::WellKnownGrpcStatus::Unknown, | ||
| RcDetails::get().GrpcBridgeFailedContentType); |
There was a problem hiding this comment.
I think this ends up setting content-type of the response to text unless the http connection manager's local_reply_config is configured with a mapper that matches and sets content-type.
Assuming that's correct, I think that's worth mentioning (or linking to docs) in the release notes.
There was a problem hiding this comment.
I think this would piggy back on the default local reply handling for gRPC and hit this code: https://github.com/envoyproxy/envoy/blob/master/source/common/http/utility.cc#L476-L492
There was a problem hiding this comment.
Ah. Was looking for that but didn't find it.
There was a problem hiding this comment.
A little late to this but do we have a integration test that covers this case? I'm slightly concerned about the sendLocalReply path having very little testing when called in the encoding path. cc @alyssawilk @snowp
There was a problem hiding this comment.
I thought we did but now that I look into it I see that there isn't, and while adding one it seems like we're hitting some ASSERT. I'll see if I can come up with a quick fix, if not I'll revert this PR.
|
ping @zuercher, lmk if you still think there is a need for additional release note information |
…am (envoyproxy#13205)" This reverts commit 07f7c59. Signed-off-by: Snow Pettersen <snowp@lyft.com>
…dEndStream (envoyproxy#13205)"" This reverts commit b1e8cfd. Signed-off-by: Snow Pettersen <snowp@lyft.com>
…yproxy#13205) Instead of returning ContinueAndEndStream we can respond with a local reply. This removes the only use case of ContinueAndEndStream in non-test code. Signed-off-by: Snow Pettersen <snowp@lyft.com>
…) (#13414) Instead of returning ContinueAndEndStream we can respond with a local reply. This removes the only use case of ContinueAndEndStream in non-test code. Signed-off-by: Snow Pettersen <snowp@lyft.com>
Instead of returning ContinueAndEndStream we can respond with a local
reply. This removes the only use case of ContinueAndEndStream in
non-test code.
Signed-off-by: Snow Pettersen snowp@lyft.com
Risk Level: Medium (changes the set of headers returned)
Testing: Existing tests
Docs Changes: n/a
Release Notes: n/a