-
Notifications
You must be signed in to change notification settings - Fork 5.5k
http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 13 commits
5d98848
7896b64
29ba376
2c264bf
c21e507
4c6b212
0903339
ae9c406
52f9baf
42327a3
04f23a6
a47b2a1
6bd512a
26aa335
dea2787
06b22a7
e125f63
2860297
74724fd
2b57fc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,6 +190,17 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| */ | ||
| virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; | ||
|
|
||
| /** | ||
| * Create a locally generated response using the provided lambdas. | ||
| * @param response_code supplies the HTTP response code. | ||
| * @param body_text supplies the optional body text which is sent using the text/plain content | ||
| * type. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "... or encoded in the grpc-message header."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
| * @param modify_headers supplies an optional callback function that can modify the | ||
| * response headers. | ||
| */ | ||
| virtual void sendLocalReply(Code response_code, const std::string& body_text, | ||
| std::function<void(HeaderMap& headers)> modify_headers) PURE; | ||
|
|
||
| /** | ||
| * Called with 100-Continue headers to be encoded. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,19 @@ class AsyncStreamImpl : public AsyncClient::Stream, | |
| void continueDecoding() override { NOT_IMPLEMENTED; } | ||
| void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED; } | ||
| const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } | ||
| void sendLocalReply(Code code, const std::string& body, | ||
| std::function<void(HeaderMap& headers)> modify_headers) override { | ||
| Utility::sendLocalReply( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is correct for an HTTP client; do we ever hit this in the configured client filter stack? I.e. could it be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially left this empty, and when tests failed put NOT_REACHED and it was reached. With this it works, and I did not dig deeper.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested it again, these fail if this is just
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we can definitely hit this for AsyncClient, since it goes through router and can return 503 for no healthy upstream and a bunch of other reasons.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that makes sense. |
||
| is_grpc_request_, | ||
| [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { | ||
| if (headers != nullptr && modify_headers != nullptr) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can headers ever be nullptr here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, removed test for non-null. |
||
| modify_headers(*headers); | ||
| } | ||
| encodeHeaders(std::move(headers), end_stream); | ||
| }, | ||
| [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, | ||
| remote_closed_, code, body); | ||
| } | ||
| // The async client won't pause if sending an Expect: 100-Continue so simply | ||
| // swallows any incoming encode100Continue. | ||
| void encode100ContinueHeaders(HeaderMapPtr&&) override {} | ||
|
|
@@ -284,6 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, | |
| bool local_closed_{}; | ||
| bool remote_closed_{}; | ||
| Buffer::InstancePtr buffered_body_; | ||
| bool is_grpc_request_{}; | ||
| friend class AsyncClientImpl; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,10 +473,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| // The protocol may have shifted in the HTTP/1.0 case so reset it. | ||
| request_info_.protocol(protocol); | ||
| if (!connection_manager_.config_.http1Settings().accept_http_10_) { | ||
| // Send "Upgrade Required" if HTTP/1.0 support is not expliictly configured on. | ||
| HeaderMapImpl headers{ | ||
| {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}}; | ||
| encodeHeaders(nullptr, headers, true); | ||
| // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on. | ||
| sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), | ||
| Code::UpgradeRequired, "", nullptr); | ||
| return; | ||
| } else { | ||
| // HTTP/1.0 defaults to single-use connections. Make sure the connection | ||
|
|
@@ -499,8 +498,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| connection_manager_.config_.http1Settings().default_host_for_http_10_); | ||
| } else { | ||
| // Require host header. For HTTP/1.1 Host has already been translated to :authority. | ||
| HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::BadRequest))}}; | ||
| encodeHeaders(nullptr, headers, true); | ||
| sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, | ||
| "", nullptr); | ||
| return; | ||
| } | ||
| } | ||
|
|
@@ -513,9 +512,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| // header size http_parser and nghttp2 will allow, down to 16k or 8k for | ||
| // envoy users who do not wish to proxy large headers. | ||
| if (request_headers_->byteSize() > (60 * 1024)) { | ||
| HeaderMapImpl headers{ | ||
| {Headers::get().Status, std::to_string(enumToInt(Code::RequestHeaderFieldsTooLarge))}}; | ||
| encodeHeaders(nullptr, headers, true); | ||
| sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), | ||
| Code::RequestHeaderFieldsTooLarge, "", nullptr); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -526,8 +524,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| // don't support that currently. | ||
| if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { | ||
| connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); | ||
| HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::NotFound))}}; | ||
| encodeHeaders(nullptr, headers, true); | ||
| sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", | ||
| nullptr); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -570,8 +568,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| } else if (websocket_requested) { | ||
| // Do not allow WebSocket upgrades if the route does not support it. | ||
| connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); | ||
| HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::Forbidden))}}; | ||
| encodeHeaders(nullptr, headers, true); | ||
| sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, | ||
| "", nullptr); | ||
| return; | ||
| } | ||
| // Allow non websocket requests to go through websocket enabled routes. | ||
|
|
@@ -653,7 +651,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte | |
| for (; entry != decoder_filters_.end(); entry++) { | ||
| ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders)); | ||
| state_.filter_call_state_ |= FilterCallState::DecodeHeaders; | ||
| FilterHeadersStatus status = (*entry)->handle_->decodeHeaders( | ||
| FilterHeadersStatus status = (*entry)->decodeHeaders( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is not intuitive. Can you add some comments. Is the idea that you want to recompute gRPC status before each filter? I think that makes sense, but worth some comments here and in the wrapper to forwards to parent.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, in case a filter bridges to/from gRPC, for example.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add comments. |
||
| headers, end_stream && continue_data_entry == decoder_filters_.end()); | ||
| state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; | ||
| ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, | ||
|
|
@@ -821,6 +819,24 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { | |
| cached_route_ = std::move(route); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::sendLocalReply( | ||
| ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, const std::string& body, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I really like where this is going! My one question for APIs is id we think sendLocalReply should instead take a reference to the request headers so is_grpc_request can be calculated locally in just one place, and in case we eventually end up with other transformations which might be based on request headers. We can just push this as-is and iterate, but I'd be interested in @mattklein123's take now since I think this is likely to be used in many places here and in downstream filters so it'd be nice to get it right on first pass.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alyssawilk I think this is internal code and not the main API, right? (So I think the code does what you are asking?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is what happens when I do reviews before caffeine :-/ I think we could avoid latching is_grpc_request_ in the two places we do, but that's a smaller request.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean having |
||
| std::function<void(HeaderMap& headers)> modify_headers) { | ||
| Utility::sendLocalReply( | ||
| is_grpc_request, | ||
| [this, filter, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { | ||
| if (headers != nullptr && modify_headers != nullptr) { | ||
| modify_headers(*headers); | ||
| } | ||
| response_headers_ = std::move(headers); | ||
| encodeHeaders(filter, *response_headers_, end_stream); | ||
| }, | ||
| [this, filter](Buffer::Instance& data, bool end_stream) -> void { | ||
| encodeData(filter, data, end_stream); | ||
| }, | ||
| state_.destroyed_, code, body); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( | ||
| ActiveStreamEncoderFilter* filter, HeaderMap& headers) { | ||
| ASSERT(connection_manager_.config_.proxy100Continue()); | ||
|
|
@@ -1316,8 +1332,7 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { | |
| onDecoderFilterAboveWriteBufferHighWatermark(); | ||
| } else { | ||
| parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); | ||
| Http::Utility::sendLocalReply(*this, parent_.state_.destroyed_, Http::Code::PayloadTooLarge, | ||
| CodeUtility::toString(Http::Code::PayloadTooLarge)); | ||
| sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1389,18 +1404,20 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { | |
| parent_.state_.encoder_filters_streaming_ = true; | ||
| stopped_ = false; | ||
|
|
||
| Http::Utility::sendLocalReply( | ||
| [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { | ||
| parent_.response_headers_ = std::move(response_headers); | ||
| parent_.response_encoder_->encodeHeaders(*parent_.response_headers_, end_stream); | ||
| }, | ||
| [&](Buffer::Instance& data, bool end_stream) -> void { | ||
| parent_.response_encoder_->encodeData(data, end_stream); | ||
| parent_.state_.local_complete_ = end_stream; | ||
| parent_.maybeEndEncode(end_stream); | ||
| }, | ||
| parent_.state_.destroyed_, Http::Code::InternalServerError, | ||
| CodeUtility::toString(Http::Code::InternalServerError)); | ||
| Http::Utility::sendLocalReply(Grpc::Common::hasGrpcContentType(*parent_.request_headers_), | ||
| [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { | ||
| parent_.response_headers_ = std::move(response_headers); | ||
| parent_.response_encoder_->encodeHeaders( | ||
| *parent_.response_headers_, end_stream); | ||
| parent_.state_.local_complete_ = end_stream; | ||
| }, | ||
| [&](Buffer::Instance& data, bool end_stream) -> void { | ||
| parent_.response_encoder_->encodeData(data, end_stream); | ||
| parent_.state_.local_complete_ = end_stream; | ||
| }, | ||
| parent_.state_.destroyed_, Http::Code::InternalServerError, | ||
| CodeUtility::toString(Http::Code::InternalServerError)); | ||
| parent_.maybeEndEncode(parent_.state_.local_complete_); | ||
| } else { | ||
| resetStream(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,8 +25,10 @@ | |
|
|
||
| #include "common/buffer/watermark_buffer.h" | ||
| #include "common/common/linked_object.h" | ||
| #include "common/grpc/common.h" | ||
| #include "common/http/conn_manager_config.h" | ||
| #include "common/http/user_agent.h" | ||
| #include "common/http/utility.h" | ||
| #include "common/request_info/request_info_impl.h" | ||
| #include "common/tracing/http_tracer_impl.h" | ||
|
|
||
|
|
@@ -164,6 +166,10 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| const Buffer::Instance* decodingBuffer() override { | ||
| return parent_.buffered_request_data_.get(); | ||
| } | ||
| void sendLocalReply(Code code, const std::string& body, | ||
| std::function<void(HeaderMap& headers)> modify_headers) override { | ||
| parent_.sendLocalReply(nullptr, is_grpc_request_, code, body, modify_headers); | ||
| } | ||
| void encode100ContinueHeaders(HeaderMapPtr&& headers) override; | ||
| void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; | ||
| void encodeData(Buffer::Instance& data, bool end_stream) override; | ||
|
|
@@ -177,10 +183,16 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| void setDecoderBufferLimit(uint32_t limit) override { parent_.setBufferLimit(limit); } | ||
| uint32_t decoderBufferLimit() override { return parent_.buffer_limit_; } | ||
|
|
||
| FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { | ||
| is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); | ||
| return handle_->decodeHeaders(headers, end_stream); | ||
| } | ||
|
|
||
| void requestDataTooLarge(); | ||
| void requestDataDrained(); | ||
|
|
||
| StreamDecoderFilterSharedPtr handle_; | ||
| bool is_grpc_request_{}; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<ActiveStreamDecoderFilter> ActiveStreamDecoderFilterPtr; | ||
|
|
@@ -257,6 +269,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); | ||
| void maybeEndDecode(bool end_stream); | ||
| void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); | ||
| void sendLocalReply(ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the first param here is always set to nullptr? What is the intention to eventually allow proper restart for filters downstream? For now, I would just remove the parameter and add TODOs around allowing proper restart if the filter is also an encoding filter?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but as it is still a TODO, I'll remove it and add the comments to the |
||
| const std::string& body, | ||
| std::function<void(HeaderMap& headers)> modify_headers); | ||
| void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); | ||
| void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream); | ||
| void encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instance& data, bool end_stream); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some docs/indication that this may be transparently converted to a gRPC response in certain cases? Some of the param text might also need to be altered so that it describes gRPC also.