-
Notifications
You must be signed in to change notification settings - Fork 5.4k
http: adding 100-Continue support to Envoy #2497
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 all commits
7b436dc
2003df2
c02d392
66f25fe
e269ee4
cdbebeb
dcb0581
805e4d2
80e1108
be679bf
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 |
|---|---|---|
|
|
@@ -217,6 +217,9 @@ 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(); } | ||
| // The async client won't pause if sending an Expect: 100-Continue so simply | ||
|
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. Is it worth asserting somewhere that no one actually sends expect continue on async client? Might be a nice sanity check somewhere.
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 can imagine use cases where we might legitimately get one (forwarding logging requests with original headers) so I think swallowing is the right thing to do.
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. OK SGTM. |
||
| // swallows any incoming encode100Continue. | ||
| void encode100ContinueHeaders(HeaderMapPtr&&) override {} | ||
| void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; | ||
| void encodeData(Buffer::Instance& data, bool end_stream) override; | ||
| void encodeTrailers(HeaderMapPtr&& trailers) override; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,12 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, | |
| runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager), | ||
| listener_stats_(config_.listenerStats()) {} | ||
|
|
||
| const HeaderMapImpl& ConnectionManagerImpl::continueHeader() { | ||
| CONSTRUCT_ON_FIRST_USE(HeaderMapImpl, | ||
| Http::HeaderMapImpl{{Http::Headers::get().Status, | ||
| std::to_string(enumToInt(Code::Continue))}}); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) { | ||
| read_callbacks_ = &callbacks; | ||
| stats_.named_.downstream_cx_total_.inc(); | ||
|
|
@@ -395,15 +401,18 @@ void ConnectionManagerImpl::ActiveStream::addAccessLogHandler( | |
| access_log_handlers_.push_back(handler); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) { | ||
| void ConnectionManagerImpl::ActiveStream::chargeStats(const HeaderMap& headers) { | ||
| uint64_t response_code = Utility::getResponseStatus(headers); | ||
| request_info_.response_code_.value(response_code); | ||
|
|
||
| if (request_info_.hc_request_) { | ||
| return; | ||
| } | ||
|
|
||
| if (CodeUtility::is2xx(response_code)) { | ||
| if (CodeUtility::is1xx(response_code)) { | ||
| connection_manager_.stats_.named_.downstream_rq_1xx_.inc(); | ||
| connection_manager_.listener_stats_.downstream_rq_1xx_.inc(); | ||
| } else if (CodeUtility::is2xx(response_code)) { | ||
| connection_manager_.stats_.named_.downstream_rq_2xx_.inc(); | ||
| connection_manager_.listener_stats_.downstream_rq_2xx_.inc(); | ||
| } else if (CodeUtility::is3xx(response_code)) { | ||
|
|
@@ -446,6 +455,14 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
| this); | ||
| #endif | ||
|
|
||
| if (!connection_manager_.config_.proxy100Continue() && request_headers_->Expect() && | ||
| request_headers_->Expect()->value() == Headers::get().ExpectValues._100Continue.c_str()) { | ||
| // Note in the case Envoy is handling 100-Continue complexity, it skips the filter chain | ||
|
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 we inc 1xx stat here also? It was more of a function of where it was done previously that there was no stat. |
||
| // and sends the 100-Continue directly to the encoder. | ||
| chargeStats(continueHeader()); | ||
| response_encoder_->encode100ContinueHeaders(continueHeader()); | ||
| } | ||
|
|
||
| connection_manager_.user_agent_.initializeFromHeaders( | ||
| *request_headers_, connection_manager_.stats_.prefix_, connection_manager_.stats_.scope_); | ||
|
|
||
|
|
@@ -773,6 +790,50 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { | |
| cached_route_.value(std::move(route)); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( | ||
| ActiveStreamEncoderFilter* filter, HeaderMap& headers) { | ||
| // Make sure commonContinue continues encode100ContinueHeaders. | ||
| has_continue_headers_ = true; | ||
|
|
||
| // Similar to the block in encodeHeaders, run encode100ContinueHeaders on each | ||
| // filter. This is simpler than that case because 100 continue implies no | ||
| // end-stream, and because there are normal headers coming there's no need for | ||
| // complex continuation logic. | ||
| std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, false); | ||
| for (; entry != encoder_filters_.end(); entry++) { | ||
| ASSERT(!(state_.filter_call_state_ & FilterCallState::Encode100ContinueHeaders)); | ||
| state_.filter_call_state_ |= FilterCallState::Encode100ContinueHeaders; | ||
| FilterHeadersStatus status = (*entry)->handle_->encode100ContinueHeaders(headers); | ||
| state_.filter_call_state_ &= ~FilterCallState::Encode100ContinueHeaders; | ||
| ENVOY_STREAM_LOG(trace, "encode 100 continue headers called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
| if (!(*entry)->commonHandleAfter100ContinueHeadersCallback(status)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Strip the T-E headers etc. Defer other header additions as well as drain-close logic to the | ||
| // continuation headers. | ||
| ConnectionManagerUtility::mutateResponseHeaders(headers, *request_headers_); | ||
|
|
||
| // Count both the 1xx and follow-up response code in stats. | ||
| chargeStats(headers); | ||
|
|
||
| ENVOY_STREAM_LOG(debug, "encoding 100 continue headers via codec", *this); | ||
| #ifndef NVLOG | ||
| headers.iterate( | ||
| [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { | ||
| ENVOY_STREAM_LOG(debug, " '{}':'{}'", *static_cast<ActiveStream*>(context), | ||
| header.key().c_str(), header.value().c_str()); | ||
| return HeaderMap::Iterate::Continue; | ||
| }, | ||
| this); | ||
| #endif | ||
|
|
||
| // Now actually encode via the codec. | ||
| response_encoder_->encode100ContinueHeaders(headers); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilter* filter, | ||
| HeaderMap& headers, bool end_stream) { | ||
| std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream); | ||
|
|
@@ -1039,6 +1100,17 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() { | |
| ASSERT(stopped_); | ||
| stopped_ = false; | ||
|
|
||
| // Only resume with do100ContinueHeaders() if we've actually seen a 100-Continue. | ||
| if (parent_.has_continue_headers_ && !continue_headers_continued_) { | ||
| continue_headers_continued_ = true; | ||
| do100ContinueHeaders(); | ||
| // If the response headers have not yet come in, don't continue on with | ||
| // headers and body. doHeaders expects request headers to exist. | ||
| if (!parent_.response_headers_.get()) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Make sure that we handle the zero byte data frame case. We make no effort to optimize this | ||
| // case in terms of merging it into a header only request/response. This could be done in the | ||
| // future. | ||
|
|
@@ -1058,9 +1130,24 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() { | |
| } | ||
| } | ||
|
|
||
| bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterHeadersCallback( | ||
| bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfter100ContinueHeadersCallback( | ||
| FilterHeadersStatus status) { | ||
| ASSERT(parent_.has_continue_headers_); | ||
| ASSERT(!continue_headers_continued_); | ||
| ASSERT(!stopped_); | ||
|
|
||
| if (status == FilterHeadersStatus::StopIteration) { | ||
| stopped_ = true; | ||
| return false; | ||
| } else { | ||
| ASSERT(status == FilterHeadersStatus::Continue); | ||
| continue_headers_continued_ = true; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| bool ConnectionManagerImpl::ActiveStreamFilterBase::commonHandleAfterHeadersCallback( | ||
| FilterHeadersStatus status) { | ||
| ASSERT(!headers_continued_); | ||
| ASSERT(!stopped_); | ||
|
|
||
|
|
@@ -1182,6 +1269,12 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedData(Buffer::In | |
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::continueDecoding() { commonContinue(); } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::encode100ContinueHeaders( | ||
| HeaderMapPtr&& headers) { | ||
| parent_.continue_headers_ = std::move(headers); | ||
| parent_.encode100ContinueHeaders(nullptr, *parent_.continue_headers_); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::encodeHeaders(HeaderMapPtr&& headers, | ||
| bool end_stream) { | ||
| parent_.response_headers_ = std::move(headers); | ||
|
|
||
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.
General design question: Do you think it's worthwhile passing around the HeaderMap everywhere? Is anyone ever going to look at it or do anything with it? I'm wondering if we could just make this void, and then have the header map that is encoded by effectively static on either side? Thoughts?
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 don't ever expect to do anything with it. I couldn't find anything in the spec about 100 continue only being firstline, and IMO if it's allowed to have other headers we're better off passing it around than changing the API in a year when someone wants to add trace headers or some such.
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.
Fair enough. SGTM.