-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Common: Consume, proxy and insert metadata from downstream to upstream #5656
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 30 commits
6884899
c986662
676b677
510fbd4
69e47c0
f8150ae
33ee213
6be013a
d762053
5de2d09
78f1c8b
082e4ff
fbbf45e
d957782
eca73af
1a8f4d0
7828283
8b173b9
bbe67be
6d573a3
72fc846
ff0b6ef
c779408
eb1a88f
7d29acb
3f507e4
2effaac
cfb962a
feac3fd
55f36b7
6ca0d45
9f6e92d
51dd633
22d3fe0
2b7e86b
a11b330
ec2b5eb
4ce2c3e
91fe461
ea90bc5
c72ee7e
df82d46
eaa9c9d
e74e302
fbfb66b
bf92ee8
554d0c8
d2ddf13
f1a5650
66ebbc0
2abf956
02f18b7
d8b4185
7e9838c
327e5c3
1ac4073
393f31e
fd2e952
bd1adbc
444518f
7afbb2f
b0f8471
18ea24d
3e2d319
3d20b82
dd9c623
563b449
be630e3
f345fcc
ac1ee95
d281e12
e43c751
be94ae1
24f41ee
fe0e9a0
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 |
|---|---|---|
|
|
@@ -227,6 +227,15 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| */ | ||
| virtual HeaderMap& addDecodedTrailers() PURE; | ||
|
|
||
| /** | ||
| * Adds decoded metadata. This function can only be called in | ||
| * StreamDecoderFilter::decodeHeaders/Data/Trailers(). Do not call in | ||
| * StreamDecoderFilter::decodeMetadata(). | ||
| * | ||
| * @return a reference to metadata map vector, where new metadata map can be added. | ||
| */ | ||
| virtual MetadataMapVector& addDecodedMetadata() PURE; | ||
|
|
||
| /** | ||
| * Create a locally generated response using the provided response_code and body_text parameters. | ||
| * If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200 | ||
|
|
@@ -396,6 +405,14 @@ class StreamDecoderFilter : public StreamFilterBase { | |
| */ | ||
| virtual FilterTrailersStatus decodeTrailers(HeaderMap& trailers) PURE; | ||
|
|
||
| /** | ||
| * Called with decoded metadata. Add new metadata to metadata_map directly. Do not call | ||
| * StreamDecoderFilterCallbacks::addDecodedMetadata() to add new metadata. | ||
| * | ||
| * @param metadata supplies the decoded metadata. | ||
| */ | ||
| virtual FilterMetadataStatus decodeMetadata(MetadataMap& metadata_map) PURE; | ||
|
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. Q: how do you expect extension developers to implement this? If they don't care metadata, should just return
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. Thanks for the feedback! Actually, because metadata is generally a hop-by-hop message, there is no need to wait for data or events to happen. All filters should return FilterMetadataStatus::Continue no matter they process metadata or not. As a result, metadata's process is kind of independent. When metadata is received or generated, it goes through all the filters's decodeMetadata() immediately, and then send to the next hop right away if the connection pool is ready. If the connection is not ready, metadata is stored in the router filter and wait for the connection to be ready. No further process of the metadata is needed. In summery, it doesn't interact with the process of other frame types, and is not part of the continueDecoding/commonContinue logic. Sorry I am not familiar with ext_authz filter. Could you please elaborate on the problems that can be caused by metadata? Thanks!
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. If Filters like Blindly returning
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 agree this is a problem and I'm not sure the best way of handling this, because very few filters are going to care about metadata. This will definitely require filters that do the stop/continue logic that @lizan talks about to be modified. I hate to say this, but I'm wondering if we should make metadata callbacks opt-in somehow and not a primary callback interface. If a filter does not opt-in to metadata, the HCM would deal with the correct stop/continue logic automatically. Thoughts?
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. What about a more general model for L7 events, where filters can opt-in to arbitrary events? The existing subclass/inheritance model works great for a few fixed events, but it seems that as we scale we don't want to force breaking API changes on all filters (plus, many don't care about most HTTP pipeline events beyond headers/data/trailers). This could be done with dynamic registration of callbacks or handler interface implementations at construction time.
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. Ah, got it. Will preserve data buffering. Thanks!!
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. With Matt's permission I'd lean towards making the base class implementation return Continue, and landing these and tests in a follow up PR. I did a recent change like that for the rc details and I think it is helpful to allow folks to fix their own filter implementations. Alternately for this one we might want to keep the base class impl around Either way I think we may have to move some of the existing Envoy filters to StopAllIteration before they work well with metadata.
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 a todo to remind myself. @alyssawilk, which filters would you recommend to check StopAllIteration? I can try to make the change. Thanks!
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.
Agreed. I think so few filters are going to care about metadata that we should try to handle this transparently. IMO having the base class return continue until we figure out a full solution makes sense to me.
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 default return. Thanks! |
||
|
|
||
| /** | ||
| * Called by the filter manager once to initialize the filter decoder callbacks that the | ||
| * filter should use. Callbacks will not be invoked by the filter after onDestroy() is called. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -774,6 +774,19 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte | |
| ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
|
|
||
| bool new_metadata_added = processNewlyAddedMetadata(); | ||
|
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: const
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. Fixed. Thanks! |
||
|
|
||
| // If end_stream is set in headers, and a filter adds new metadata, we need to delay end_stream | ||
| // in headers by inserting an empty data frame with end_stream set. The empty data frame is sent | ||
| // after the new metadata. | ||
| if ((*entry)->end_stream_ && new_metadata_added && !empty_data_end_stream_sent_) { | ||
| Buffer::OwnedImpl empty_data(""); | ||
| ENVOY_STREAM_LOG( | ||
| trace, "inserting an empty data frame for end_stream due metadata being added.", *this); | ||
| addDecodedData(*((*entry).get()), empty_data, true); | ||
|
mattklein123 marked this conversation as resolved.
|
||
| empty_data_end_stream_sent_ = true; | ||
| } | ||
|
|
||
| if (!(*entry)->commonHandleAfterHeadersCallback(status, decoding_headers_only_) && | ||
| std::next(entry) != decoder_filters_.end()) { | ||
| // Stop iteration IFF this is not the last filter. If it is the last filter, continue with | ||
|
|
@@ -885,6 +898,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter* | |
| ENVOY_STREAM_LOG(trace, "decode data called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
|
|
||
| processNewlyAddedMetadata(); | ||
|
|
||
| if (!trailers_exists_at_start && request_trailers_ && | ||
| trailers_added_entry == decoder_filters_.end()) { | ||
| trailers_added_entry = entry; | ||
|
|
@@ -942,6 +957,10 @@ void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilt | |
| } | ||
| } | ||
|
|
||
| MetadataMapVector& ConnectionManagerImpl::ActiveStream::addDecodedMetadata() { | ||
| return request_metadata_map_vector_; | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::decodeTrailers(HeaderMapPtr&& trailers) { | ||
| resetIdleTimer(); | ||
| maybeEndDecode(true); | ||
|
|
@@ -976,13 +995,41 @@ void ConnectionManagerImpl::ActiveStream::decodeTrailers(ActiveStreamDecoderFilt | |
| state_.filter_call_state_ &= ~FilterCallState::DecodeTrailers; | ||
| ENVOY_STREAM_LOG(trace, "decode trailers called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
|
|
||
| processNewlyAddedMetadata(); | ||
|
|
||
| if (!(*entry)->commonHandleAfterTrailersCallback(status)) { | ||
| return; | ||
| } | ||
| } | ||
| disarmRequestTimeout(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::decodeMetadata(MetadataMapPtr&& metadata_map) { | ||
| resetIdleTimer(); | ||
| // After going through filters, the ownership of metadata_map will be passed to router filter. | ||
|
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: I would probably not talk about router filter in this code as it is not explicitly required (e.g., in the case of another terminal 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. Fixed. Thanks! |
||
| // Router filter may encode metadata_map to the next hop immediately or store metadata_map and | ||
| // encode later when connection pool is ready. | ||
| decodeMetadata(nullptr, *metadata_map); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::decodeMetadata(ActiveStreamDecoderFilter* filter, | ||
| MetadataMap& metadata_map) { | ||
| std::list<ActiveStreamDecoderFilterPtr>::iterator entry; | ||
| if (!filter) { | ||
| entry = decoder_filters_.begin(); | ||
| } else { | ||
| entry = std::next(filter->entry()); | ||
| } | ||
|
|
||
| for (; entry != decoder_filters_.end(); entry++) { | ||
| FilterMetadataStatus status = (*entry)->handle_->decodeMetadata(metadata_map); | ||
| ENVOY_STREAM_LOG(trace, "decode metadata called: filter={} status={}, metadata: {}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status), | ||
| metadata_map); | ||
| } | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::maybeEndDecode(bool end_stream) { | ||
| ASSERT(!state_.remote_complete_); | ||
| state_.remote_complete_ = end_stream; | ||
|
|
@@ -1398,6 +1445,17 @@ void ConnectionManagerImpl::ActiveStream::maybeEndEncode(bool end_stream) { | |
| } | ||
| } | ||
|
|
||
| bool ConnectionManagerImpl::ActiveStream::processNewlyAddedMetadata() { | ||
| if (request_metadata_map_vector_.empty()) { | ||
| return false; | ||
| } | ||
| for (const auto& metadata_map : request_metadata_map_vector_) { | ||
| decodeMetadata(nullptr, *metadata_map); | ||
| } | ||
| request_metadata_map_vector_.clear(); | ||
| return true; | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason) { | ||
| // NOTE: This function gets called in all of the following cases: | ||
| // 1) We TX an app level reset | ||
|
|
@@ -1686,6 +1744,10 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedData(Buffer::In | |
| parent_.addDecodedData(*this, data, streaming); | ||
| } | ||
|
|
||
| MetadataMapVector& ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedMetadata() { | ||
| return parent_.addDecodedMetadata(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::continueDecoding() { commonContinue(); } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::encode100ContinueHeaders( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,6 +172,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| // Http::StreamDecoderFilterCallbacks | ||
| void addDecodedData(Buffer::Instance& data, bool streaming) override; | ||
| HeaderMap& addDecodedTrailers() override; | ||
| MetadataMapVector& addDecodedMetadata() override; | ||
| void continueDecoding() override; | ||
| const Buffer::Instance* decodingBuffer() override { | ||
| return parent_.buffered_request_data_.get(); | ||
|
|
@@ -282,9 +283,11 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| const Network::Connection* connection(); | ||
| void addDecodedData(ActiveStreamDecoderFilter& filter, Buffer::Instance& data, bool streaming); | ||
| HeaderMap& addDecodedTrailers(); | ||
| MetadataMapVector& addDecodedMetadata(); | ||
| void decodeHeaders(ActiveStreamDecoderFilter* filter, HeaderMap& headers, bool end_stream); | ||
| void decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream); | ||
| void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); | ||
| void decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMap& metadata_map); | ||
| void disarmRequestTimeout(); | ||
| void maybeEndDecode(bool end_stream); | ||
| void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); | ||
|
|
@@ -299,6 +302,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| void encodeTrailers(ActiveStreamEncoderFilter* filter, HeaderMap& trailers); | ||
| void encodeMetadata(ActiveStreamEncoderFilter* filter, MetadataMapPtr&& metadata_map_ptr); | ||
| void maybeEndEncode(bool end_stream); | ||
| // Returns true if new metadata is inserted. Otherwise, returns false. | ||
|
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. "is inserted" -> "decoded", or "was inserted"? It already had been inserted if we're talking about request_metadata_map_vector_
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. Fixed. Thanks! |
||
| bool processNewlyAddedMetadata(); | ||
| uint64_t streamId() { return stream_id_; } | ||
|
|
||
| // Http::StreamCallbacks | ||
|
|
@@ -311,7 +316,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; | ||
| void decodeData(Buffer::Instance& data, bool end_stream) override; | ||
| void decodeTrailers(HeaderMapPtr&& trailers) override; | ||
| void decodeMetadata(MetadataMapPtr&&) override { NOT_REACHED_GCOVR_EXCL_LINE; } | ||
| void decodeMetadata(MetadataMapPtr&&) override; | ||
|
|
||
| // Http::FilterChainFactoryCallbacks | ||
| void addStreamDecoderFilter(StreamDecoderFilterSharedPtr filter) override { | ||
|
|
@@ -423,6 +428,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| absl::optional<Router::RouteConstSharedPtr> cached_route_; | ||
| absl::optional<Upstream::ClusterInfoConstSharedPtr> cached_cluster_info_; | ||
| DownstreamWatermarkCallbacks* watermark_callbacks_{nullptr}; | ||
| // Stores metadata added in the decoding filter that is being processed. Will be cleared before | ||
|
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 notice that we have similar storage on a per-filter basis as well as on a connection basis. Is it possible to collapse that somehow? If not can you add some clarifying comments on why we need both types of storage? It's not immediately clear.
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 comments and removed not used variables. |
||
| // processing the next filter. | ||
| MetadataMapVector request_metadata_map_vector_; | ||
|
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. mind commenting what metadata ends up here and why?
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. Fixed. Thanks!
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. 2 things for thought:
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. To answer 1: Storing metadata as unique_ptr instead of MetadataMapVector (aka std::vector<unique_ptr>) will merge two metadata with the same key value into one. This may cause some problem when multiple copies of the same metadata are expected, for example, for logging purpose. To answer 2: I think that's good suggestion. Do you mind I make that change in a followup PR? Added a TODO.
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 that's OK, but will you do it relatively quickly? I think we need to be mindful of the performance/memory impact of rarely used code in the hottest paths.
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 see. Fixed in this PR. |
||
| uint32_t buffer_limit_{0}; | ||
| uint32_t high_watermark_count_{0}; | ||
| const std::string* decorated_operation_{nullptr}; | ||
|
|
@@ -435,6 +443,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| // Whether a filter has indicated that the response should be treated as a headers only | ||
| // response. | ||
| bool encoding_headers_only_{}; | ||
| // If an empty data frame with end_stream true is sent. | ||
| bool empty_data_end_stream_sent_{false}; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<ActiveStream> ActiveStreamPtr; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -453,6 +453,22 @@ Http::FilterTrailersStatus Filter::decodeTrailers(Http::HeaderMap& trailers) { | |||
| return Http::FilterTrailersStatus::StopIteration; | ||||
| } | ||||
|
|
||||
| Http::FilterMetadataStatus Filter::decodeMetadata(Http::MetadataMap& metadata_map) { | ||||
| Http::MetadataMap metadata; | ||||
| metadata.insert(metadata_map.begin(), metadata_map.end()); | ||||
| Http::MetadataMapPtr metadata_map_ptr = std::make_unique<Http::MetadataMap>(metadata); | ||||
|
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. So just to sanity check, we don't need to do the copy for headers because the HCM permanently stores request headers, but we copy metadata because right now the HCM doesn't store metadata, yes? If so this will get fixed when we handle metadata retries and I'm good with both not creating a copy utility and Ok with the temporary perf hit. Might be worth a comment that the copy will go away as we continue with metadata functionality.
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. That's correct! I changed this copy to move since router is the last filter and no one will refer to metadata_map afterwards. In the future, the move will happen in HCM. |
||||
|
|
||||
| if (upstream_request_ == nullptr) { | ||||
| ENVOY_STREAM_LOG(trace, "upstream_request_ not ready. Storing metadata_map to encode later: {}", | ||||
| *callbacks_, metadata_map); | ||||
| downstream_metadata_map_vector_.emplace_back(std::move(metadata_map_ptr)); | ||||
| return Http::FilterMetadataStatus::Continue; | ||||
| } | ||||
|
|
||||
| upstream_request_->encodeMetadata(std::move(metadata_map_ptr)); | ||||
| return Http::FilterMetadataStatus::Continue; | ||||
| } | ||||
|
|
||||
| void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { | ||||
| callbacks_ = &callbacks; | ||||
| // As the decoder filter only pushes back via watermarks once data has reached | ||||
|
|
@@ -1024,6 +1040,14 @@ void Filter::UpstreamRequest::encodeData(Buffer::Instance& data, bool end_stream | |||
|
|
||||
| buffered_request_body_->move(data); | ||||
| } else { | ||||
| // Encodes metadata if exists. | ||||
|
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. When does this happen here and below? In the case where we don't have an encoder, won't we deal with any pending metadata when the pool is ready? Can we instead just assert that we don't have any pending metadata here and below?
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. Good catch! Removed and added asserts. |
||||
| if (!parent_.downstream_metadata_map_vector_.empty()) { | ||||
| ENVOY_STREAM_LOG(trace, "Send metadata before sending data. {}", *parent_.callbacks_, | ||||
| parent_.downstream_metadata_map_vector_); | ||||
| request_encoder_->encodeMetadata(parent_.downstream_metadata_map_vector_); | ||||
| parent_.downstream_metadata_map_vector_.clear(); | ||||
|
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. Can we have a test for metadata and upstream retries? I think we're going to have to store this somewhere. Ditto shadowing, and internal redirects, where we call recreateStream. I'm also OK TODOing either or both since both are complicated and this PR is already complicated. It's possible designing for them may inform how and where we store and clear frames so it's worth at least thinking about it now. Happy to give you pointers to code or tests if you have trouble finding them.
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. Great point!! Thanks! I didn't think of those cases at all. I am still working on this part. But I agree we should have a design for them before submitting this PR in case there are changes. Will update this thread when I make progress.
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 only have looked at the internal redirect case. Since those cases all call recreateStream(), I think they can be solved similarly. recreateStream() calls decodeHeaders() to go through all filters at the end of the the function. envoy/source/common/http/conn_manager_impl.cc Line 1789 in 4f4280c
The issue is in nghttp2. In the redirect case, metadata ends up in session->ob_reg: https://github.com/nghttp2/nghttp2/blob/d93842db3eb481fc6953ec70a81ed2f7863ffb45/lib/nghttp2_session.c#L2352, and the redirect headers ends up in session->ob_syn: https://github.com/nghttp2/nghttp2/blob/d93842db3eb481fc6953ec70a81ed2f7863ffb45/lib/nghttp2_session.c#L2360 . Because session->ob_reg is parsed before session->ob_syn, metadata is received before headers (no new stream has created), and caused failure. I switched the positions of the pointed code, and I don't see new test failures in nghttp2. And a simple internal redirect test case passed with metadata in envoy. (cl/231599606. If others are interested in the change, I can make a public available PR). What do you think about making the nghttp2 change? If we go with this, we can submit this PR first, and deal with shadowing, internal rediects and retries after the change is pushed to nghttp2. But it may take longer. I notice envoy still uses an old version of nghttp2.
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. To add more details about why the nghttp2 fix (here) is needed in redirect, etc cases, but not needed in regular case. In the regular case, when we call encodeHeaders() in onPoolReady(), we will submit the headers by calling sendPendingFrames() before encoding any other frames. Calling sendPendingFrames() will make sure headers (session->ob_syn) are popped for processing first, because that's the only frame available. In redirect case, sendPendingFrames() in encodeHeaders() will be skipped because we are still dispatching (dispatching_ is true here). So we accumulate both headers and metadata. When sendPendingFrames is executed at the end of dispatch(), metadata shows up first because it is in session->ob_reg (here), which is processed before session->ob_syn(here). It's easiest, and I think it also makes sense to fix in nghttp2. (sync frame should always process before regular frames?). If it takes too long, I will try to find some hacky ways to fix it in Envoy.
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. As said I'm totally fine dealing with this later, I'd just like a tracking TODO :-)
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. Fixed. Thanks! |
||||
| } | ||||
|
|
||||
| ENVOY_STREAM_LOG(trace, "proxying {} bytes", *parent_.callbacks_, data.length()); | ||||
| stream_info_.addBytesSent(data.length()); | ||||
| request_encoder_->encodeData(data, end_stream); | ||||
|
|
@@ -1042,13 +1066,34 @@ void Filter::UpstreamRequest::encodeTrailers(const Http::HeaderMap& trailers) { | |||
| if (!request_encoder_) { | ||||
| ENVOY_STREAM_LOG(trace, "buffering trailers", *parent_.callbacks_); | ||||
| } else { | ||||
| // Encodes metadata if exists. | ||||
| if (!parent_.downstream_metadata_map_vector_.empty()) { | ||||
|
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. Oops - I'd missed that we were using the same map location for buffered metadata. At that point we'll create a copy of metadata for each upstream and each upstream will keep its own queue.
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. Good catch! Moved metadata storage to upstream. |
||||
| ENVOY_STREAM_LOG(trace, "Send metadata before sending trailers. {}", *parent_.callbacks_, | ||||
| parent_.downstream_metadata_map_vector_); | ||||
| request_encoder_->encodeMetadata(parent_.downstream_metadata_map_vector_); | ||||
| parent_.downstream_metadata_map_vector_.clear(); | ||||
| } | ||||
|
|
||||
| ENVOY_STREAM_LOG(trace, "proxying trailers", *parent_.callbacks_); | ||||
| request_encoder_->encodeTrailers(trailers); | ||||
| stream_info_.onLastUpstreamTxByteSent(); | ||||
| parent_.callbacks_->streamInfo().onLastUpstreamTxByteSent(); | ||||
| } | ||||
| } | ||||
|
|
||||
| void Filter::UpstreamRequest::encodeMetadata(Http::MetadataMapPtr&& metadata_map_ptr) { | ||||
| if (!request_encoder_) { | ||||
| ENVOY_STREAM_LOG(trace, "request_encoder_ not ready. Store metadata_map to encode later: {}", | ||||
| *parent_.callbacks_, *metadata_map_ptr); | ||||
| parent_.downstream_metadata_map_vector_.emplace_back(std::move(metadata_map_ptr)); | ||||
| } else { | ||||
| ENVOY_STREAM_LOG(trace, "Encode metadata: {}", *parent_.callbacks_, *metadata_map_ptr); | ||||
| Http::MetadataMapVector metadata_map_vector; | ||||
|
mattklein123 marked this conversation as resolved.
|
||||
| metadata_map_vector.emplace_back(std::move(metadata_map_ptr)); | ||||
| request_encoder_->encodeMetadata(metadata_map_vector); | ||||
| } | ||||
| } | ||||
|
|
||||
| void Filter::UpstreamRequest::onResetStream(Http::StreamResetReason reason) { | ||||
| clearRequestEncoder(); | ||||
| if (!calling_encode_headers_) { | ||||
|
|
@@ -1145,8 +1190,12 @@ void Filter::UpstreamRequest::onPoolReady(Http::StreamEncoder& request_encoder, | |||
|
|
||||
| stream_info_.onFirstUpstreamTxByteSent(); | ||||
| parent_.callbacks_->streamInfo().onFirstUpstreamTxByteSent(); | ||||
| bool end_stream = !buffered_request_body_ && encode_complete_ && !encode_trailers_; | ||||
|
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: const
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. Fixed. Thanks! |
||||
| // If end_stream is set in headers, and there are metadata to send, delays end_stream. The case | ||||
| // only happens when decoding headers filters return ContinueAndEndStream. | ||||
|
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. Ah, I think this answers my question up above on how we handle the headers only condition. I think we should probably handle it in the HCM so that other filters don't have to all one-off this.
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. There are two cases where an empty data frame is needed: 1. the request is headers only request. 2. a filter decides to ignore data and trailers even if the request has data or trailers, aka the decoding_headers_only_ case. Case 1 can be easily handled in HCM. But case 2 is not easy to deal with in HCM. None of the methods I came up with is as simple as the current solution. I added tests for both cases in the latest version. The purpose of decoding_headers_only_ filter is to skip passing data to the following filters. But if a filter inserts metadata in decodeHeaders() before or after decoding_headers_only_ filter, to deal with it in HCM, we need to pass the empty data frame through the following filters, which conflicts what decoding_headers_only_ filter is doing. So we need to add conditions to make sure only empty frames pass through the following filters, not the data from request, and it only passes through once. Another problem I encountered is which filter the empty data frame should start iterating from. Sometimes, decodeHeaders() returns stop in a filter after decoding_headers_only_ filter. If the continue_data_entry(here) is set to decoding_headers_only_ filter (it should be, because that's when we know if we need the empty data frame or not), commonContinue() will call the decodeHeaders in the stopped filter again without reset the iteration state. Since we don't need to pass the empty frame through filters, the safest way is to iterate the empty frame from the router filter (the last one), which I think may be equivalent to the current solution. I am open to any ideas or suggestions.
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. I think it's possible this comment is confusing me. If any intermediate filter added metadata, end stream would not have been set along with the headers by the time the content got to the router filter (or I think we have a problem). So if just-before-router filter added metadata, the router filter would get headers with end stream false. The way I see it what this code is doing is determining if this is a headers only request to send to upstream. If there encode is complete but there's body, trailers, or metadata, it's not a headers only stream. If it is the case that ContinueAndEndStream results in headers-with-end-stream-true going through the filter chain, wouldn't every filter up to the router filter be assuming it's done before it gets metadata?
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. When the request is a header-only request, added metadata will unset end stream in headers, and router filter receives headers without end stream set. But if it's a ContinueAndEndStream case, and decodeHeaders() added metadata, I keep the end stream in headers to tell the following filters that request's data and trailers should not be parsed. In this case, metadata are guaranteed to arrive before headers at the router filter. Because addDecodedMetadata() calls decodeMetadata() in every filters before return. And decodeMetadata() can only return continue. The router filter will always get metadata before headers if decodeHeaders() adds metadata. Does that make sense? If we unset end stream in ContinueAndEndStream case, I think it would be hard/messy to let the following filters skip parsing data and trailers.
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. "The router filter will always get metadata before headers if decodeHeaders() adds metadata." Is this still true with the decode_headers_called_ check in the HCM? I thought that made sure that metadata never races before headers?
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. Ah, you are right! With decode_headers_called_, metadata should always arrive after headers! decode_headers_called_ was created to handle a very rare case where a filter inserts new metadata, and another filter follows it returns StopAllIteration. To prevent metadata passing through a filter where the filter's header callback hasn't been called yet, decode_headers_called_ is used to indicate if header callback has been called. I only thought its use in StopAll cases, and didn't thought it has the impact of delay metadata for all statuses. Good catch! It would simplify router filter's code then. We don't need to consider upstream request creation in decodeMetadata()! But back to the main point in this comment thread, I think it's still possible that router filter receives headers with end stream set, and then it receives metadata because end_stream is used to tell the following filters that data and trailers should not be parsed. |
||||
| bool delay_headers_end_stream = end_stream && !parent_.downstream_metadata_map_vector_.empty(); | ||||
| request_encoder.encodeHeaders(*parent_.downstream_headers_, | ||||
| !buffered_request_body_ && encode_complete_ && !encode_trailers_); | ||||
| end_stream && !delay_headers_end_stream); | ||||
| calling_encode_headers_ = false; | ||||
|
|
||||
| // It is possible to get reset in the middle of an encodeHeaders() call. This happens for example | ||||
|
|
@@ -1157,6 +1206,18 @@ void Filter::UpstreamRequest::onPoolReady(Http::StreamEncoder& request_encoder, | |||
| if (deferred_reset_reason_) { | ||||
| onResetStream(deferred_reset_reason_.value()); | ||||
| } else { | ||||
| // Encode metadata after headers and before any other frame type. | ||||
| if (!parent_.downstream_metadata_map_vector_.empty()) { | ||||
| ENVOY_STREAM_LOG(trace, "Send metadata onPoolReady. {}", *parent_.callbacks_, | ||||
|
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 might bump this to debug but nbd if you feel trace is better.
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. Fixed. Thanks! |
||||
| parent_.downstream_metadata_map_vector_); | ||||
| request_encoder.encodeMetadata(parent_.downstream_metadata_map_vector_); | ||||
| parent_.downstream_metadata_map_vector_.clear(); | ||||
| if (delay_headers_end_stream) { | ||||
| Buffer::OwnedImpl empty_data(""); | ||||
| request_encoder.encodeData(empty_data, true); | ||||
| } | ||||
| } | ||||
|
|
||||
| if (buffered_request_body_) { | ||||
| stream_info_.addBytesSent(buffered_request_body_->length()); | ||||
| request_encoder.encodeData(*buffered_request_body_, encode_complete_ && !encode_trailers_); | ||||
|
|
||||
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.
I guess I missed some context, but this method takes no argument which means it can at most serve as a getter, why was it named as addDecodedMetadata()?
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.
It is to be consistent with addDecodedData() and addDecodedTrailers(). But, I agree, it is a little less straightforward, :)