diff --git a/RAW_RELEASE_NOTES.md b/RAW_RELEASE_NOTES.md index 28bfaed1ac3c1..e46975b36adb8 100644 --- a/RAW_RELEASE_NOTES.md +++ b/RAW_RELEASE_NOTES.md @@ -51,6 +51,7 @@ final version. :ref:`Squash ` allows debugging an incoming request to a microservice in the mesh. * lua: added headers replace() API. * Added support for direct responses -- i.e., sending a preconfigured HTTP response without proxying anywhere. +* Added support for proxying 100-Continue responses. * Added DOWNSTREAM_LOCAL_ADDRESS, DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT header formatters, and DOWNSTREAM_LOCAL_ADDRESS access log formatter. * Added support for HTTPS redirects on specific routes. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 80afd08424788..356b6201323c4 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -21,6 +21,12 @@ class StreamEncoder { public: virtual ~StreamEncoder() {} + /** + * Encode 100-Continue headers. + * @param headers supplies the 100-Continue header map to encode. + */ + virtual void encode100ContinueHeaders(const HeaderMap& headers) PURE; + /** * Encode headers, optionally indicating end of stream. * @param headers supplies the header map to encode. @@ -54,6 +60,12 @@ class StreamDecoder { public: virtual ~StreamDecoder() {} + /** + * Called with decoded 100-Continue headers. + * @param headers supplies the decoded 100-Continue headers map that is moved into the callee. + */ + virtual void decode100ContinueHeaders(HeaderMapPtr&& headers) PURE; + /** * Called with decoded headers, optionally indicating end of stream. * @param headers supplies the decoded headers map that is moved into the callee. diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index bd855329f6e6d..588c60d4d6577 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -190,6 +190,17 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { */ virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; + /** + * Called with 100-Continue headers to be encoded. + * + * This is not folded into encodeHeaders because most Envoy users and filters + * will not be proxying 100-continue and with it split out, can ignore the + * complexity of multiple encodeHeaders calls. + * + * @param headers supplies the headers to be encoded. + */ + virtual void encode100ContinueHeaders(HeaderMapPtr&& headers) PURE; + /** * Called with headers to be encoded, optionally indicating end of stream. * @@ -401,6 +412,19 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks { */ class StreamEncoderFilter : public StreamFilterBase { public: + /* + * Called with 100-continue headers. + * + * This is not folded into encodeHeaders because most Envoy users and filters + * will not be proxying 100-continue and with it split out, can ignore the + * complexity of multiple encodeHeaders calls. + * + * @param headers supplies the 100-continue response headers to be encoded. + * @return FilterHeadersStatus determines how filter chain iteration proceeds. + * + */ + virtual FilterHeadersStatus encode100ContinueHeaders(HeaderMap& headers) PURE; + /** * Called with headers to be encoded, optionally indicating end of stream. * @param headers supplies the headers to be encoded. diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index b827fbc402695..76d0604d825e3 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -38,6 +38,9 @@ class DynamoFilter : public Http::StreamFilter { } // Http::StreamEncoderFilter + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override; Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap&) override; diff --git a/source/common/grpc/grpc_web_filter.h b/source/common/grpc/grpc_web_filter.h index 0af050888f643..92943debd7663 100644 --- a/source/common/grpc/grpc_web_filter.h +++ b/source/common/grpc/grpc_web_filter.h @@ -34,6 +34,9 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { } // Implements StreamEncoderFilter. + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override; Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; diff --git a/source/common/grpc/http1_bridge_filter.h b/source/common/grpc/http1_bridge_filter.h index 9345e955f0578..c823fecbd57ca 100644 --- a/source/common/grpc/http1_bridge_filter.h +++ b/source/common/grpc/http1_bridge_filter.h @@ -31,6 +31,9 @@ class Http1BridgeFilter : public Http::StreamFilter { } // Http::StreamEncoderFilter + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override; Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; diff --git a/source/common/grpc/json_transcoder_filter.h b/source/common/grpc/json_transcoder_filter.h index 7530bcce0c0d8..1779a0be912b4 100644 --- a/source/common/grpc/json_transcoder_filter.h +++ b/source/common/grpc/json_transcoder_filter.h @@ -92,6 +92,9 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; // Http::StreamEncoderFilter + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override; Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 04ea3bd92f9bf..264291c90941e 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -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 + // 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; diff --git a/source/common/http/codec_wrappers.h b/source/common/http/codec_wrappers.h index 5b4dca2ea0d61..1053a31eb010c 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -11,6 +11,10 @@ namespace Http { class StreamDecoderWrapper : public StreamDecoder { public: // StreamDecoder + void decode100ContinueHeaders(HeaderMapPtr&& headers) override { + inner_.decode100ContinueHeaders(std::move(headers)); + } + void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override { if (end_stream) { onPreDecodeComplete(); @@ -60,6 +64,10 @@ class StreamDecoderWrapper : public StreamDecoder { class StreamEncoderWrapper : public StreamEncoder { public: // StreamEncoder + void encode100ContinueHeaders(const HeaderMap& headers) override { + inner_.encode100ContinueHeaders(headers); + } + void encodeHeaders(const HeaderMap& headers, bool end_stream) override { inner_.encodeHeaders(headers, end_stream); if (end_stream) { diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2115a7af6804a..8d3077809386b 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -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,7 +401,7 @@ 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); @@ -403,7 +409,10 @@ void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) { 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 + // 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::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((*entry).get()), static_cast(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(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::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); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 6933c6b05b1a2..b4d4a39f92a82 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -75,6 +75,7 @@ namespace Http { COUNTER (downstream_rq_non_relative_path) \ COUNTER (downstream_rq_ws_on_non_ws_route) \ COUNTER (downstream_rq_too_large) \ + COUNTER (downstream_rq_1xx) \ COUNTER (downstream_rq_2xx) \ COUNTER (downstream_rq_3xx) \ COUNTER (downstream_rq_4xx) \ @@ -132,6 +133,7 @@ typedef std::unique_ptr TracingConnectionManager */ // clang-format off #define CONN_MAN_LISTENER_STATS(COUNTER) \ + COUNTER(downstream_rq_1xx) \ COUNTER(downstream_rq_2xx) \ COUNTER(downstream_rq_3xx) \ COUNTER(downstream_rq_4xx) \ @@ -275,6 +277,11 @@ class ConnectionManagerConfig { * @return ConnectionManagerListenerStats& the stats to write to. */ virtual ConnectionManagerListenerStats& listenerStats() PURE; + + /** + * @return bool supplies if the HttpConnectionManager should proxy the Expect: 100-Continue + */ + virtual bool proxy100Continue() const PURE; }; /** @@ -300,6 +307,7 @@ class ConnectionManagerImpl : Logger::Loggable, ConnectionManagerTracingStats& tracing_stats); static ConnectionManagerListenerStats generateListenerStats(const std::string& prefix, Stats::Scope& scope); + static const HeaderMapImpl& continueHeader(); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data) override; @@ -330,8 +338,10 @@ class ConnectionManagerImpl : Logger::Loggable, */ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks { ActiveStreamFilterBase(ActiveStream& parent, bool dual_filter) - : parent_(parent), headers_continued_(false), stopped_(false), dual_filter_(dual_filter) {} + : parent_(parent), headers_continued_(false), continue_headers_continued_(false), + stopped_(false), dual_filter_(dual_filter) {} + bool commonHandleAfter100ContinueHeadersCallback(FilterHeadersStatus status); bool commonHandleAfterHeadersCallback(FilterHeadersStatus status); void commonHandleBufferData(Buffer::Instance& provided_data); bool commonHandleAfterDataCallback(FilterDataStatus status, Buffer::Instance& provided_data, @@ -343,6 +353,7 @@ class ConnectionManagerImpl : Logger::Loggable, virtual Buffer::WatermarkBufferPtr createBuffer() PURE; virtual Buffer::WatermarkBufferPtr& bufferedData() PURE; virtual bool complete() PURE; + virtual void do100ContinueHeaders() PURE; virtual void doHeaders(bool end_stream) PURE; virtual void doData(bool end_stream) PURE; virtual void doTrailers() PURE; @@ -361,6 +372,7 @@ class ConnectionManagerImpl : Logger::Loggable, ActiveStream& parent_; bool headers_continued_ : 1; + bool continue_headers_continued_ : 1; bool stopped_ : 1; const bool dual_filter_ : 1; }; @@ -387,6 +399,7 @@ class ConnectionManagerImpl : Logger::Loggable, Buffer::WatermarkBufferPtr createBuffer() override; Buffer::WatermarkBufferPtr& bufferedData() override { return parent_.buffered_request_data_; } bool complete() override { return parent_.state_.remote_complete_; } + void do100ContinueHeaders() override { NOT_REACHED; } void doHeaders(bool end_stream) override { parent_.decodeHeaders(this, *parent_.request_headers_, end_stream); } @@ -402,6 +415,7 @@ class ConnectionManagerImpl : Logger::Loggable, const Buffer::Instance* decodingBuffer() override { return parent_.buffered_request_data_.get(); } + void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; void encodeData(Buffer::Instance& data, bool end_stream) override; void encodeTrailers(HeaderMapPtr&& trailers) override; @@ -437,6 +451,9 @@ class ConnectionManagerImpl : Logger::Loggable, Buffer::WatermarkBufferPtr createBuffer() override; Buffer::WatermarkBufferPtr& bufferedData() override { return parent_.buffered_response_data_; } bool complete() override { return parent_.state_.local_complete_; } + void do100ContinueHeaders() override { + parent_.encode100ContinueHeaders(this, *parent_.continue_headers_); + } void doHeaders(bool end_stream) override { parent_.encodeHeaders(this, *parent_.response_headers_, end_stream); } @@ -481,7 +498,7 @@ class ConnectionManagerImpl : Logger::Loggable, void addStreamDecoderFilterWorker(StreamDecoderFilterSharedPtr filter, bool dual_filter); void addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr filter, bool dual_filter); - void chargeStats(HeaderMap& headers); + void chargeStats(const HeaderMap& headers); std::list::iterator commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_stream); uint64_t connectionId(); @@ -492,6 +509,7 @@ class ConnectionManagerImpl : Logger::Loggable, void decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream); void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); + 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); void encodeTrailers(ActiveStreamEncoderFilter* filter, HeaderMap& trailers); @@ -504,6 +522,7 @@ class ConnectionManagerImpl : Logger::Loggable, void onBelowWriteBufferLowWatermark() override; // Http::StreamDecoder + void decode100ContinueHeaders(HeaderMapPtr&&) override { NOT_REACHED; } void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance& data, bool end_stream) override; void decodeTrailers(HeaderMapPtr&& trailers) override; @@ -550,6 +569,11 @@ class ConnectionManagerImpl : Logger::Loggable, static constexpr uint32_t EncodeHeaders = 0x08; static constexpr uint32_t EncodeData = 0x10; static constexpr uint32_t EncodeTrailers = 0x20; + // Encode100ContinueHeaders is a bit of a special state as 100 continue + // headers may be sent during request processing. This state is only used + // to verify we do not encode100Continue headers more than once per + // filter. + static constexpr uint32_t Encode100ContinueHeaders = 0x40; }; // clang-format on @@ -577,6 +601,7 @@ class ConnectionManagerImpl : Logger::Loggable, Tracing::SpanPtr active_span_; const uint64_t stream_id_; StreamEncoder* response_encoder_{}; + HeaderMapPtr continue_headers_; HeaderMapPtr response_headers_; Buffer::WatermarkBufferPtr buffered_response_data_; HeaderMapPtr response_trailers_{}; @@ -594,6 +619,9 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t buffer_limit_{0}; uint32_t high_watermark_count_{0}; const std::string* decorated_operation_{nullptr}; + // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders + // is ever called, this is set to true so commonContinue resumes processing the 100-Continue. + bool has_continue_headers_{}; }; typedef std::unique_ptr ActiveStreamPtr; diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index c5c586c344990..a35fd91ecdce0 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -20,6 +20,10 @@ class ConnectionManagerUtility { * Mutates request headers in various ways. This functionality is broken out because of its * complexity for ease of testing. See the method itself for detailed comments on what * mutations are performed. + * + * Note this function may be called twice on the response path if there are + * 100-Continue headers. + * * @return the final trusted remote address. This depends on various settings and the * existence of the x-forwarded-for header. Again see the method for more details. */ diff --git a/source/common/http/filter/cors_filter.h b/source/common/http/filter/cors_filter.h index 21c3c15cb0e20..175c861818eaa 100644 --- a/source/common/http/filter/cors_filter.h +++ b/source/common/http/filter/cors_filter.h @@ -25,6 +25,9 @@ class CorsFilter : public StreamFilter { void setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) override; // Http::StreamEncoderFilter + FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override { return FilterDataStatus::Continue; diff --git a/source/common/http/filter/lua/lua_filter.h b/source/common/http/filter/lua/lua_filter.h index 9ae7351aeaa81..dd4c5cbb10cc6 100644 --- a/source/common/http/filter/lua/lua_filter.h +++ b/source/common/http/filter/lua/lua_filter.h @@ -250,6 +250,9 @@ class Filter : public StreamFilter, Logger::Loggable { } // Http::StreamEncoderFilter + FilterHeadersStatus encode100ContinueHeaders(HeaderMap&) override { + return FilterHeadersStatus::Continue; + } FilterHeadersStatus encodeHeaders(HeaderMap& headers, bool end_stream) override { return doHeaders(response_stream_wrapper_, encoder_callbacks_, config_->responseFunctionRef(), headers, end_stream); diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 4586edf018a71..497fcc4eb0677 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -35,6 +35,13 @@ void StreamEncoderImpl::encodeHeader(const char* key, uint32_t key_size, const c connection_.addCharToBuffer('\n'); } +void StreamEncoderImpl::encode100ContinueHeaders(const HeaderMap& headers) { + ASSERT(headers.Status()->value() == "100"); + processing_100_continue_ = true; + encodeHeaders(headers, false); + processing_100_continue_ = false; +} + void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { bool saw_content_length = false; headers.iterate( @@ -70,7 +77,10 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) if (saw_content_length) { chunk_encoding_ = false; } else { - if (end_stream) { + if (processing_100_continue_) { + // Make sure we don't serialize chunk information with 100-Continue headers. + chunk_encoding_ = false; + } else if (end_stream) { encodeHeader(Headers::get().ContentLength.get().c_str(), Headers::get().ContentLength.get().size(), "0", 1); chunk_encoding_ = false; @@ -474,16 +484,6 @@ int ServerConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { headers->insertMethod().value(method_string, strlen(method_string)); - // Deal with expect: 100-continue here since higher layers are never going to do anything other - // than say to continue so that we can respond before request complete if necessary. - if (headers->Expect() && - 0 == StringUtil::caseInsensitiveCompare(headers->Expect()->value().c_str(), - Headers::get().ExpectValues._100Continue.c_str())) { - Buffer::OwnedImpl continue_response("HTTP/1.1 100 Continue\r\n\r\n"); - connection_.write(continue_response); - headers->removeExpect(); - } - // Determine here whether we have a body or not. This uses the new RFC semantics where the // presence of content-length or chunked transfer-encoding indicates a body vs. a particular // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed @@ -625,7 +625,12 @@ int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { if (pending_responses_.empty() && !resetStreamCalled()) { throw PrematureResponseException(std::move(headers)); } else if (!pending_responses_.empty()) { - if (cannotHaveBody()) { + if (parser_.status_code == 100) { + // http-parser treats 100 continue headers as their own complete response. + // Swallow the spurious onMessageComplete and continue processing. + ignore_message_complete_for_100_continue_ = true; + pending_responses_.front().decoder_->decode100ContinueHeaders(std::move(headers)); + } else if (cannotHaveBody()) { deferred_end_stream_headers_ = std::move(headers); } else { pending_responses_.front().decoder_->decodeHeaders(std::move(headers), false); @@ -647,6 +652,10 @@ void ClientConnectionImpl::onBody(const char* data, size_t length) { } void ClientConnectionImpl::onMessageComplete() { + if (ignore_message_complete_for_100_continue_) { + ignore_message_complete_for_100_continue_ = false; + return; + } if (!pending_responses_.empty()) { // After calling decodeData() with end stream set to true, we should no longer be able to reset. PendingResponse response = pending_responses_.front(); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index ee0ec79e8b322..4315ba8a4cc70 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -33,6 +33,7 @@ class StreamEncoderImpl : public StreamEncoder, public StreamCallbackHelper { public: // Http::StreamEncoder + void encode100ContinueHeaders(const HeaderMap& headers) override; void encodeHeaders(const HeaderMap& headers, bool end_stream) override; void encodeData(Buffer::Instance& data, bool end_stream) override; void encodeTrailers(const HeaderMap& trailers) override; @@ -69,6 +70,7 @@ class StreamEncoderImpl : public StreamEncoder, void endEncode(); bool chunk_encoding_{true}; + bool processing_100_continue_{false}; }; /** @@ -340,6 +342,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { std::unique_ptr request_encoder_; std::list pending_responses_; + // Set true between receiving 100-Continue headers and receiving the spurious onMessageComplete. + bool ignore_message_complete_for_100_continue_{}; }; } // namespace Http1 diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 9d8ec43b06385..ffe0545404ef1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -38,9 +38,6 @@ bool Utility::reconstituteCrumbledCookies(const HeaderString& key, const HeaderS ConnectionImpl::Http2Callbacks ConnectionImpl::http2_callbacks_; ConnectionImpl::Http2Options ConnectionImpl::http2_options_; -const std::unique_ptr ConnectionImpl::CONTINUE_HEADER{ - new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Code::Continue))}}}; /** * Helper to remove const during a cast. nghttp2 takes non-const pointers for headers even though @@ -100,6 +97,11 @@ void ConnectionImpl::StreamImpl::buildHeaders(std::vector& final_hea &final_headers); } +void ConnectionImpl::StreamImpl::encode100ContinueHeaders(const HeaderMap& headers) { + ASSERT(headers.Status()->value() == "100"); + encodeHeaders(headers, false); +} + void ConnectionImpl::StreamImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { std::vector final_headers; buildHeaders(final_headers, headers); @@ -374,28 +376,19 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { stream->headers_->addViaMove(std::move(key), std::move(stream->cookies_)); } - if (frame->headers.cat == NGHTTP2_HCAT_REQUEST && stream->headers_->Expect() && - 0 == StringUtil::caseInsensitiveCompare(stream->headers_->Expect()->value().c_str(), - Headers::get().ExpectValues._100Continue.c_str())) { - // Deal with expect: 100-continue here since higher layers are never going to do anything - // other than say to continue so that we can respond before request complete if necessary. - std::vector final_headers; - StreamImpl::buildHeaders(final_headers, *CONTINUE_HEADER); - int rc = nghttp2_submit_headers(session_, 0, stream->stream_id_, nullptr, &final_headers[0], - final_headers.size(), nullptr); - ASSERT(rc == 0); - UNREFERENCED_PARAMETER(rc); - - stream->headers_->removeExpect(); - } - switch (frame->headers.cat) { case NGHTTP2_HCAT_RESPONSE: { if (CodeUtility::is1xx(Http::Utility::getResponseStatus(*stream->headers_))) { stream->waiting_for_non_informational_headers_ = true; } - FALLTHRU; + if (stream->headers_->Status()->value() == "100") { + ASSERT(!stream->remote_end_stream_); + stream->decoder_->decode100ContinueHeaders(std::move(stream->headers_)); + } else { + stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); + } + break; } case NGHTTP2_HCAT_REQUEST: { @@ -417,10 +410,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { // This can only happen in the client case in a response, when we received a 1xx to // start out with. In this case, raise as headers. nghttp2 message checking guarantees // proper flow here. - // TODO(mattklein123): Higher layers don't currently deal with a double decodeHeaders() - // call and will probably crash. We do this in the client path for testing when the server - // responds with 1xx. In the future, if needed, we can properly handle 1xx in higher layer - // code, or just eat it. + ASSERT(!stream->headers_->Status() || stream->headers_->Status()->value() != "100"); stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); } } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 08fade69165d9..bcb40575ad852 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -147,6 +147,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable CONTINUE_HEADER; - bool dispatching_ : 1; bool raised_goaway_ : 1; bool pending_deferred_reset_ : 1; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4b79e5114fa2c..b4b5244afb264 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -576,10 +576,22 @@ void Filter::handleNon5xxResponseHeaders(const Http::HeaderMap& headers, bool en } } +void Filter::onUpstream100ContinueHeaders(Http::HeaderMapPtr&& headers) { + ENVOY_STREAM_LOG(debug, "upstream 100 continue", *callbacks_); + + downstream_response_started_ = true; + // Don't send retries after 100-Continue has been sent on. Arguably we could attempt to do a + // retry, assume the next upstream would also send an 100-Continue and swallow the second one + // but it's sketchy (as the subsequent upstream might not send a 100-Continue) and not worth + // the complexity until someone asks for it. + retry_state_.reset(); + + callbacks_->encode100ContinueHeaders(std::move(headers)); +} + void Filter::onUpstreamHeaders(const uint64_t response_code, Http::HeaderMapPtr&& headers, bool end_stream) { ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={}", *callbacks_, end_stream); - ASSERT(!downstream_response_started_); upstream_request_->upstream_host_->outlierDetector().putHttpResponseCode(response_code); @@ -800,6 +812,11 @@ Filter::UpstreamRequest::~UpstreamRequest() { } } +void Filter::UpstreamRequest::decode100ContinueHeaders(Http::HeaderMapPtr&& headers) { + ASSERT(100 == Http::Utility::getResponseStatus(*headers)); + parent_.onUpstream100ContinueHeaders(std::move(headers)); +} + void Filter::UpstreamRequest::decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { upstream_headers_ = headers.get(); const uint64_t response_code = Http::Utility::getResponseStatus(*headers); diff --git a/source/common/router/router.h b/source/common/router/router.h index 7b98bf96c63bd..e21ca5246c51c 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -217,6 +217,7 @@ class Filter : Logger::Loggable, } // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&& headers) override; void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance& data, bool end_stream) override; void decodeTrailers(Http::HeaderMapPtr&& trailers) override; @@ -298,6 +299,7 @@ class Filter : Logger::Loggable, void maybeDoShadowing(); void onRequestComplete(); void onResponseTimeout(); + void onUpstream100ContinueHeaders(Http::HeaderMapPtr&& headers); void onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& headers, bool end_stream); void onUpstreamData(Buffer::Instance& data, bool end_stream); void onUpstreamTrailers(Http::HeaderMapPtr&& trailers); diff --git a/source/common/upstream/health_checker_impl.h b/source/common/upstream/health_checker_impl.h index cc9514deef615..c869e96599e7d 100644 --- a/source/common/upstream/health_checker_impl.h +++ b/source/common/upstream/health_checker_impl.h @@ -186,6 +186,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { void onTimeout() override; // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&&) override {} void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance&, bool end_stream) override { if (end_stream) { @@ -447,6 +448,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase { void onTimeout() override; // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&&) override {} void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance&, bool end_stream) override; void decodeTrailers(Http::HeaderMapPtr&&) override; diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 9f93974cee406..e5d789463a51a 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -126,8 +126,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( drain_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, drain_timeout, 5000)), generate_request_id_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, generate_request_id, true)), date_provider_(date_provider), - listener_stats_(Http::ConnectionManagerImpl::generateListenerStats( - stats_prefix_, context_.listenerScope())) { + listener_stats_(Http::ConnectionManagerImpl::generateListenerStats(stats_prefix_, + context_.listenerScope())), + proxy_100_continue_(config.proxy_100_continue()) { route_config_provider_ = Router::RouteConfigProviderUtil::create( config, context_.runtime(), context_.clusterManager(), context_.scope(), stats_prefix_, diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index 5978ea7b7f1a0..5d0cba9517539 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -100,6 +100,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, const Network::Address::Instance& localAddress() override; const Optional& userAgent() override { return user_agent_; } Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } + bool proxy100Continue() const override { return proxy_100_continue_; } static const std::string DEFAULT_SERVER_STRING; @@ -128,6 +129,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, bool generate_request_id_; Http::DateProvider& date_provider_; Http::ConnectionManagerListenerStats listener_stats_; + const bool proxy_100_continue_; }; } // namespace Configuration diff --git a/source/server/http/admin.h b/source/server/http/admin.h index b71de6e073bd1..eb24f6ec2806b 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -86,6 +86,7 @@ class AdminImpl : public Admin, const Optional& userAgent() override { return user_agent_; } const Http::TracingConnectionManagerConfig* tracingConfig() override { return nullptr; } Http::ConnectionManagerListenerStats& listenerStats() override { return listener_.stats_; } + bool proxy100Continue() const override { return false; } private: /** diff --git a/source/server/http/health_check.h b/source/server/http/health_check.h index b196d6780acd5..9ec1ca4aca705 100644 --- a/source/server/http/health_check.h +++ b/source/server/http/health_check.h @@ -96,6 +96,9 @@ class HealthCheckFilter : public Http::StreamFilter { } // Http::StreamEncoderFilter + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override { return Http::FilterDataStatus::Continue; diff --git a/test/common/dynamo/dynamo_filter_test.cc b/test/common/dynamo/dynamo_filter_test.cc index a105c2268c489..cbcd2c5a48190 100644 --- a/test/common/dynamo/dynamo_filter_test.cc +++ b/test/common/dynamo/dynamo_filter_test.cc @@ -53,6 +53,11 @@ TEST_F(DynamoFilterTest, operatorPresent) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(stats_, counter("prefix.dynamodb.operation_missing")).Times(0); EXPECT_CALL(stats_, counter("prefix.dynamodb.table_missing")); diff --git a/test/common/grpc/grpc_web_filter_test.cc b/test/common/grpc/grpc_web_filter_test.cc index d98f8d0aa64e2..a35769d03919a 100644 --- a/test/common/grpc/grpc_web_filter_test.cc +++ b/test/common/grpc/grpc_web_filter_test.cc @@ -184,6 +184,11 @@ TEST_P(GrpcWebFilterTest, StatsNormalResponse) { Http::TestHeaderMapImpl request_headers{{"content-type", request_content_type()}, {":path", "/lyft.users.BadCompanions/GetBadCompanions"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Http::TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); Buffer::OwnedImpl data("hello"); diff --git a/test/common/grpc/http1_bridge_filter_test.cc b/test/common/grpc/http1_bridge_filter_test.cc index 167f6316eafac..fa29bb43d354e 100644 --- a/test/common/grpc/http1_bridge_filter_test.cc +++ b/test/common/grpc/http1_bridge_filter_test.cc @@ -68,6 +68,10 @@ TEST_F(GrpcHttp1BridgeFilterTest, StatsHttp2HeaderOnlyResponse) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true)); + Http::TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{{":status", "200"}, {"grpc-status", "1"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); EXPECT_EQ(1UL, cm_.thread_local_cluster_.cluster_.info_->stats_store_ diff --git a/test/common/grpc/json_transcoder_filter_test.cc b/test/common/grpc/json_transcoder_filter_test.cc index a86729d681a18..7b970c3c1f6d6 100644 --- a/test/common/grpc/json_transcoder_filter_test.cc +++ b/test/common/grpc/json_transcoder_filter_test.cc @@ -295,6 +295,10 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPost) { EXPECT_EQ(expected_request.ByteSize(), frames[0].length_); EXPECT_TRUE(MessageDifferencer::Equals(expected_request, request)); + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, {":status", "200"}}; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 89872e3f0d8fb..5e4d18e97ad5c 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -53,6 +53,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "codec_wrappers_test", + srcs = ["codec_wrappers_test.cc"], + deps = [ + "//source/common/http:codec_wrappers_lib", + "//test/mocks/http:http_mocks", + "//test/test_common:utility_lib", + ], +) + envoy_cc_test( name = "codes_test", srcs = ["codes_test.cc"], diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index c4273f1b4242a..dfae13f50a98b 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -102,6 +102,8 @@ TEST_F(AsyncClientImplTest, BasicStream) { stream->sendHeaders(headers, false); stream->sendData(*body, true); + response_decoder_->decode100ContinueHeaders( + HeaderMapPtr(new TestHeaderMapImpl{{":status", "100"}})); response_decoder_->decodeHeaders(HeaderMapPtr(new TestHeaderMapImpl{{":status", "200"}}), false); response_decoder_->decodeData(*body, true); diff --git a/test/common/http/codec_wrappers_test.cc b/test/common/http/codec_wrappers_test.cc new file mode 100644 index 0000000000000..d660c4f85f206 --- /dev/null +++ b/test/common/http/codec_wrappers_test.cc @@ -0,0 +1,77 @@ +#include "common/http/codec_wrappers.h" + +#include "test/mocks/http/mocks.h" +#include "test/test_common/utility.h" + +using testing::_; + +namespace Envoy { +namespace Http { + +class MockStreamEncoderWrapper : public StreamEncoderWrapper { +public: + MockStreamEncoderWrapper() : StreamEncoderWrapper(inner_encoder_) {} + void onEncodeComplete() override { encode_complete_ = true; } + + MockStreamEncoder& innerEncoder() { return inner_encoder_; } + bool encodeComplete() const { return encode_complete_; } + +private: + MockStreamEncoder inner_encoder_; + bool encode_complete_{}; +}; + +TEST(StreamEncoderWrapper, HeaderOnlyEncode) { + MockStreamEncoderWrapper wrapper; + + EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, true)); + wrapper.encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); + EXPECT_TRUE(wrapper.encodeComplete()); +} + +TEST(StreamEncoderWrapper, HeaderAndBodyEncode) { + MockStreamEncoderWrapper wrapper; + + TestHeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, false)); + wrapper.encodeHeaders(response_headers, false); + EXPECT_FALSE(wrapper.encodeComplete()); + + Buffer::OwnedImpl data; + EXPECT_CALL(wrapper.innerEncoder(), encodeData(_, true)); + wrapper.encodeData(data, true); + EXPECT_TRUE(wrapper.encodeComplete()); +} + +TEST(StreamEncoderWrapper, HeaderAndBodyAndTrailersEncode) { + MockStreamEncoderWrapper wrapper; + + TestHeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, false)); + wrapper.encodeHeaders(response_headers, false); + EXPECT_FALSE(wrapper.encodeComplete()); + + Buffer::OwnedImpl data; + EXPECT_CALL(wrapper.innerEncoder(), encodeData(_, false)); + wrapper.encodeData(data, false); + EXPECT_FALSE(wrapper.encodeComplete()); + + EXPECT_CALL(wrapper.innerEncoder(), encodeTrailers(_)); + wrapper.encodeTrailers(TestHeaderMapImpl{{"trailing", "header"}}); + EXPECT_TRUE(wrapper.encodeComplete()); +} + +TEST(StreamEncoderWrapper, 100ContinueHeaderEncode) { + MockStreamEncoderWrapper wrapper; + + EXPECT_CALL(wrapper.innerEncoder(), encode100ContinueHeaders(_)); + wrapper.encode100ContinueHeaders(TestHeaderMapImpl{{":status", "100"}}); + EXPECT_FALSE(wrapper.encodeComplete()); + + EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, true)); + wrapper.encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); + EXPECT_TRUE(wrapper.encodeComplete()); +} + +} // namespace Http +} // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 27cf0a0c5cf84..6712f5d7ac405 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -243,6 +243,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi const Optional& userAgent() override { return user_agent_; } const TracingConnectionManagerConfig* tracingConfig() override { return tracing_config_.get(); } ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } + bool proxy100Continue() const override { return false; } NiceMock tracer_; NiceMock runtime_; @@ -346,6 +347,109 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) { EXPECT_EQ(1U, listener_stats_.downstream_rq_2xx_.value()); } +TEST_F(HttpConnectionManagerImplTest, 100ContinueResponse) { + setup(false, "envoy-custom-server", false); + + // Store the basic request encoder during filter chain setup. + std::shared_ptr filter(new NiceMock()); + + EXPECT_CALL(*filter, decodeHeaders(_, true)) + .WillRepeatedly(Invoke([&](HeaderMap& headers, bool) -> FilterHeadersStatus { + EXPECT_NE(nullptr, headers.ForwardedFor()); + EXPECT_STREQ("http", headers.ForwardedProto()->value().c_str()); + return FilterHeadersStatus::StopIteration; + })); + + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + })); + + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // When dispatch is called on the codec, we pretend to get a new stream and then fire a headers + // only request into it. Then we respond into the filter. + StreamDecoder* decoder = nullptr; + NiceMock encoder; + EXPECT_CALL(*codec_, dispatch(_)) + .Times(1) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + + // Test not charging stats on the second call. + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + decoder->decodeHeaders(std::move(headers), true); + + HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; + filter->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + filter->callbacks_->encodeHeaders(std::move(response_headers), true); + + data.drain(4); + })); + + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_1xx_.value()); + EXPECT_EQ(1U, listener_stats_.downstream_rq_1xx_.value()); + EXPECT_EQ(1U, stats_.named_.downstream_rq_2xx_.value()); + EXPECT_EQ(1U, listener_stats_.downstream_rq_2xx_.value()); +} + +TEST_F(HttpConnectionManagerImplTest, 100ContinueResponseWithEncoderFilters) { + setup(false, "envoy-custom-server", false); + setUpEncoderAndDecoder(); + sendReqestHeadersAndData(); + + EXPECT_CALL(*encoder_filters_[0], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)); + HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; + decoder_filters_[0]->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); + + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false); +} + +TEST_F(HttpConnectionManagerImplTest, PauseResume100Continue) { + setup(false, "envoy-custom-server", false); + setUpEncoderAndDecoder(); + sendReqestHeadersAndData(); + + // Stop the 100-Continue at filter 0. Filter 1 should not yet receive the 100-Continue + EXPECT_CALL(*encoder_filters_[0], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)).Times(0); + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)).Times(0); + HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; + decoder_filters_[0]->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); + + // Have the filter 0 continue. Make sure the 100-Continue is resumed as expected. + EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)); + encoder_filters_[0]->callbacks_->continueEncoding(); + + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false); +} + TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { InSequence s; setup(false, ""); diff --git a/test/common/http/filter/cors_filter_test.cc b/test/common/http/filter/cors_filter_test.cc index 785c15681f18a..5789546e84964 100644 --- a/test/common/http/filter/cors_filter_test.cc +++ b/test/common/http/filter/cors_filter_test.cc @@ -302,6 +302,9 @@ TEST_F(CorsFilterTest, EncodeWithAllowCredentialsTrue) { EXPECT_EQ(FilterDataStatus::Continue, filter_.decodeData(data_, false)); EXPECT_EQ(FilterTrailersStatus::Continue, filter_.decodeTrailers(request_headers_)); + Http::TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_EQ(FilterHeadersStatus::Continue, filter_.encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{}; EXPECT_EQ(FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false)); EXPECT_EQ("localhost", response_headers.get_("access-control-allow-origin")); diff --git a/test/common/http/filter/lua/lua_filter_test.cc b/test/common/http/filter/lua/lua_filter_test.cc index 12b0383b32169..331ddfd62d6cf 100644 --- a/test/common/http/filter/lua/lua_filter_test.cc +++ b/test/common/http/filter/lua/lua_filter_test.cc @@ -646,6 +646,11 @@ TEST_F(LuaHttpFilterTest, RequestAndResponse) { EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("bar"))); EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers)); + TestHeaderMapImpl continue_headers{{":status", "100"}}; + // No lua hooks for 100-continue + EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("100"))).Times(0); + EXPECT_EQ(FilterHeadersStatus::Continue, filter_->encode100ContinueHeaders(continue_headers)); + TestHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("200"))); EXPECT_EQ(FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index acc094c45547c..6c571dcfff6d4 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -423,31 +423,6 @@ TEST_F(Http1ServerConnectionImplTest, HeadRequestResponse) { EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 5\r\n\r\n", output); } -TEST_F(Http1ServerConnectionImplTest, ExpectContinueResponse) { - initialize(); - - NiceMock decoder; - Http::StreamEncoder* response_encoder = nullptr; - EXPECT_CALL(callbacks_, newStream(_)) - .WillOnce(Invoke([&](Http::StreamEncoder& encoder) -> Http::StreamDecoder& { - response_encoder = &encoder; - return decoder; - })); - - std::string output; - ON_CALL(connection_, write(_)).WillByDefault(AddBufferToString(&output)); - - TestHeaderMapImpl expected_headers{ - {"content-length", "100"}, {":path", "/"}, {":method", "POST"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); - - Buffer::OwnedImpl buffer( - "POST / HTTP/1.1\r\nExpect: 100-continue\r\ncontent-length: 100\r\n\r\n"); - codec_->dispatch(buffer); - EXPECT_EQ(0U, buffer.length()); - EXPECT_EQ("HTTP/1.1 100 Continue\r\n\r\n", output); -} - TEST_F(Http1ServerConnectionImplTest, DoubleRequest) { initialize(); @@ -637,6 +612,25 @@ TEST_F(Http1ClientConnectionImplTest, 204Response) { codec_->dispatch(response); } +TEST_F(Http1ClientConnectionImplTest, 100Response) { + initialize(); + + NiceMock response_decoder; + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + TestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + EXPECT_CALL(response_decoder, decode100ContinueHeaders_(_)); + EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); + Buffer::OwnedImpl initial_response("HTTP/1.1 100 Continue\r\n\r\n"); + codec_->dispatch(initial_response); + + EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); + EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 20\r\n\r\n"); + codec_->dispatch(response); +} + TEST_F(Http1ClientConnectionImplTest, BadEncodeParams) { initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 9f58507dfaa8e..f39cd20d32ce2 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -125,55 +125,53 @@ class Http2CodecImplTest : public testing::TestWithParam MockStreamCallbacks server_stream_callbacks_; }; -TEST_P(Http2CodecImplTest, ExpectContinueHeadersOnlyResponse) { +TEST_P(Http2CodecImplTest, ShutdownNotice) { initialize(); TestHeaderMapImpl request_headers; - request_headers.addCopy("expect", "100-continue"); HttpTestUtility::addDefaultHeaders(request_headers); - TestHeaderMapImpl expected_headers; - HttpTestUtility::addDefaultHeaders(expected_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - - TestHeaderMapImpl continue_headers{{":status", "100"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(HeaderMapEqual(&continue_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); - EXPECT_CALL(request_decoder_, decodeData(_, true)); - Buffer::OwnedImpl hello("hello"); - request_encoder_->encodeData(hello, true); + EXPECT_CALL(client_callbacks_, onGoAway()); + server_.shutdownNotice(); + server_.goAway(); TestHeaderMapImpl response_headers{{":status", "200"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(HeaderMapEqual(&response_headers), true)); + EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); response_encoder_->encodeHeaders(response_headers, true); } -TEST_P(Http2CodecImplTest, ExpectContinueTrailersResponse) { +TEST_P(Http2CodecImplTest, ContinueHeaders) { initialize(); TestHeaderMapImpl request_headers; - request_headers.addCopy("expect", "100-continue"); HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); TestHeaderMapImpl continue_headers{{":status", "100"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(HeaderMapEqual(&continue_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); - - EXPECT_CALL(request_decoder_, decodeData(_, true)); - Buffer::OwnedImpl hello("hello"); - request_encoder_->encodeData(hello, true); + EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); + response_encoder_->encode100ContinueHeaders(continue_headers); TestHeaderMapImpl response_headers{{":status", "200"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(HeaderMapEqual(&response_headers), false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); + response_encoder_->encodeHeaders(response_headers, true); +}; - TestHeaderMapImpl response_trailers{{"foo", "bar"}}; - EXPECT_CALL(response_decoder_, decodeTrailers_(HeaderMapEqual(&response_trailers))); - response_encoder_->encodeTrailers(response_trailers); -} +TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { + initialize(); -TEST_P(Http2CodecImplTest, ShutdownNotice) { + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException); +}; + +TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { initialize(); TestHeaderMapImpl request_headers; @@ -181,14 +179,12 @@ TEST_P(Http2CodecImplTest, ShutdownNotice) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); request_encoder_->encodeHeaders(request_headers, true); - EXPECT_CALL(client_callbacks_, onGoAway()); - server_.shutdownNotice(); - server_.goAway(); + TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); + response_encoder_->encode100ContinueHeaders(continue_headers); - TestHeaderMapImpl response_headers{{":status", "200"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); -} + EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException); +}; TEST_P(Http2CodecImplTest, RefusedStreamReset) { initialize(); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 94283fd47a936..715c2bdacde87 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -1146,6 +1146,32 @@ TEST_F(RouterTest, RetryUpstreamResetResponseStarted) { EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); } +TEST_F(RouterTest, RetryUpstreamReset100ContinueResponseStarted) { + NiceMock encoder1; + Http::StreamDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + // The 100-continue will result in resetting retry_state_, so when the stream + // is reset we won't even check shouldRetry(). + EXPECT_CALL(*router_.retry_state_, shouldRetry(_, _, _)).Times(0); + EXPECT_CALL(callbacks_, encode100ContinueHeaders_(_)); + Http::HeaderMapPtr continue_headers(new Http::TestHeaderMapImpl{{":status", "100"}}); + response_decoder->decode100ContinueHeaders(std::move(continue_headers)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503)); + encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset); +} + TEST_F(RouterTest, RetryUpstream5xx) { NiceMock encoder1; Http::StreamDecoder* response_decoder = nullptr; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 34fa8ce2afa00..acfd522df0692 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -321,6 +321,33 @@ TEST_F(HttpHealthCheckerImplTest, Success) { EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy()); } +TEST_F(HttpHealthCheckerImplTest, SuccessWithSpurious100Continue) { + setupNoServiceValidationHC(); + EXPECT_CALL(*this, onHostStatus(_, false)).Times(1); + + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->info_->stats().upstream_cx_total_.inc(); + expectSessionCreate(); + expectStreamCreate(0); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_)); + health_checker_->start(); + + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.max_interval", _)); + EXPECT_CALL(runtime_.snapshot_, getInteger("health_check.min_interval", _)) + .WillOnce(Return(45000)); + EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(std::chrono::milliseconds(45000))); + EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); + + std::unique_ptr continue_headers( + new Http::TestHeaderMapImpl{{":status", "100"}}); + test_sessions_[0]->stream_response_callbacks_->decode100ContinueHeaders( + std::move(continue_headers)); + + respond(0, "200", false, true); + EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy()); +} + // Test host check success with multiple hosts. TEST_F(HttpHealthCheckerImplTest, SuccessWithMultipleHosts) { setupNoServiceValidationHC(); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index d275814f37553..c5b5717b7bb1b 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -51,6 +51,13 @@ void FakeStream::decodeTrailers(Http::HeaderMapPtr&& trailers) { decoder_event_.notify_one(); } +void FakeStream::encode100ContinueHeaders(const Http::HeaderMapImpl& headers) { + std::shared_ptr headers_copy( + new Http::HeaderMapImpl(static_cast(headers))); + parent_.connection().dispatcher().post( + [this, headers_copy]() -> void { encoder_.encode100ContinueHeaders(*headers_copy); }); +} + void FakeStream::encodeHeaders(const Http::HeaderMapImpl& headers, bool end_stream) { std::shared_ptr headers_copy( new Http::HeaderMapImpl(static_cast(headers))); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 5056f5bfd1d01..8bdab331d0072 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -43,6 +43,7 @@ class FakeStream : public Http::StreamDecoder, uint64_t bodyLength() { return body_.length(); } Buffer::Instance& body() { return body_; } bool complete() { return end_stream_; } + void encode100ContinueHeaders(const Http::HeaderMapImpl& headers); void encodeHeaders(const Http::HeaderMapImpl& headers, bool end_stream); void encodeData(uint64_t size, bool end_stream); void encodeData(Buffer::Instance& data, bool end_stream); @@ -97,6 +98,7 @@ class FakeStream : public Http::StreamDecoder, } // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&&) override {} void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance& data, bool end_stream) override; void decodeTrailers(Http::HeaderMapPtr&& trailers) override; diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 4a9f177b73c54..5b3ee2a90d85c 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -104,6 +104,10 @@ TEST_P(Http2IntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(Http2IntegrationTest, Retry) { testRetry(); } +TEST_P(Http2IntegrationTest, EnvoyHandling100Continue) { testEnvoyHandling100Continue(); } + +TEST_P(Http2IntegrationTest, EnvoyProxying100Continue) { testEnvoyProxying100Continue(); } + TEST_P(Http2IntegrationTest, RetryHittingBufferLimit) { testRetryHittingBufferLimit(); } TEST_P(Http2IntegrationTest, HittingDecoderFilterLimit) { testHittingDecoderFilterLimit(); } diff --git a/test/integration/http2_upstream_integration_test.cc b/test/integration/http2_upstream_integration_test.cc index 3b959d6408b9f..71597b7d4ff4d 100644 --- a/test/integration/http2_upstream_integration_test.cc +++ b/test/integration/http2_upstream_integration_test.cc @@ -88,6 +88,10 @@ TEST_P(Http2UpstreamIntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(Http2UpstreamIntegrationTest, Retry) { testRetry(); } +TEST_P(Http2UpstreamIntegrationTest, EnvoyHandling100Continue) { testEnvoyHandling100Continue(); } + +TEST_P(Http2UpstreamIntegrationTest, EnvoyProxying100Continue) { testEnvoyProxying100Continue(); } + TEST_P(Http2UpstreamIntegrationTest, RetryHittingBufferLimit) { testRetryHittingBufferLimit(); } TEST_P(Http2UpstreamIntegrationTest, GrpcRetry) { testGrpcRetry(); } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 1ef2dc172c5c0..192d502b48dd9 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -740,6 +740,71 @@ void HttpIntegrationTest::testHittingEncoderFilterLimit() { EXPECT_STREQ("500", response_->headers().Status()->value().c_str()); } +void HttpIntegrationTest::testEnvoyHandling100Continue() { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"expect", "100-continue"}}, + *response_); + waitForNextUpstreamRequest(); + + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); + response_->waitForEndStream(); + ASSERT_TRUE(response_->complete()); + ASSERT(response_->continue_headers() != nullptr); + EXPECT_STREQ("100", response_->continue_headers()->Status()->value().c_str()); + + EXPECT_STREQ("200", response_->headers().Status()->value().c_str()); +} + +void HttpIntegrationTest::testEnvoyProxying100Continue(bool with_encoder_filter) { + if (with_encoder_filter) { + // Because 100-continue only affects encoder filters, make sure it plays well + // with one. + config_helper_.addFilter("name: envoy.cors"); + config_helper_.addConfigModifier( + [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + auto* route_config = hcm.mutable_route_config(); + auto* virtual_host = route_config->mutable_virtual_hosts(0); + { + auto* cors = virtual_host->mutable_cors(); + cors->add_allow_origin("*"); + cors->set_allow_headers("content-type,x-grpc-web"); + cors->set_allow_methods("GET,POST"); + } + }); + } + config_helper_.addConfigModifier( + [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { hcm.set_proxy_100_continue(true); }); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"expect", "100-continue"}}, + *response_); + waitForNextUpstreamRequest(); + + upstream_request_->encode100ContinueHeaders(Http::TestHeaderMapImpl{{":status", "100"}}); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); + response_->waitForEndStream(); + EXPECT_TRUE(response_->complete()); + ASSERT(response_->continue_headers() != nullptr); + EXPECT_STREQ("100", response_->continue_headers()->Status()->value().c_str()); + + EXPECT_STREQ("200", response_->headers().Status()->value().c_str()); +} + void HttpIntegrationTest::testTwoRequests() { initialize(); diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 3621a35489715..16d2894601a07 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -152,6 +152,8 @@ class HttpIntegrationTest : public BaseIntegrationTest { void testGrpcRetry(); void testHittingDecoderFilterLimit(); void testHittingEncoderFilterLimit(); + void testEnvoyHandling100Continue(); + void testEnvoyProxying100Continue(bool with_encoder_filter = false); // HTTP/2 client tests. void testDownstreamResetBeforeResponseComplete(); diff --git a/test/integration/integration.cc b/test/integration/integration.cc index f6b533bd518b1..3006d3f2b35f7 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -66,6 +66,10 @@ void IntegrationStreamDecoder::waitForReset() { } } +void IntegrationStreamDecoder::decode100ContinueHeaders(Http::HeaderMapPtr&& headers) { + continue_headers_ = std::move(headers); +} + void IntegrationStreamDecoder::decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { saw_end_stream_ = end_stream; headers_ = std::move(headers); diff --git a/test/integration/integration.h b/test/integration/integration.h index 77174694e4df1..ca58466ea61de 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -29,6 +29,7 @@ class IntegrationStreamDecoder : public Http::StreamDecoder, public Http::Stream bool complete() { return saw_end_stream_; } bool reset() { return saw_reset_; } Http::StreamResetReason reset_reason() { return reset_reason_; } + const Http::HeaderMap* continue_headers() { return continue_headers_.get(); } const Http::HeaderMap& headers() { return *headers_; } const Http::HeaderMapPtr& trailers() { return trailers_; } void waitForHeaders(); @@ -37,6 +38,7 @@ class IntegrationStreamDecoder : public Http::StreamDecoder, public Http::Stream void waitForReset(); // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&& headers) override; void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance& data, bool end_stream) override; void decodeTrailers(Http::HeaderMapPtr&& trailers) override; @@ -48,6 +50,7 @@ class IntegrationStreamDecoder : public Http::StreamDecoder, public Http::Stream private: Event::Dispatcher& dispatcher_; + Http::HeaderMapPtr continue_headers_; Http::HeaderMapPtr headers_; Http::HeaderMapPtr trailers_; bool waiting_for_end_stream_{}; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index dd993eb14955e..ffdb5852669f7 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -113,6 +113,14 @@ TEST_P(IntegrationTest, RouterUpstreamResponseBeforeRequestComplete) { TEST_P(IntegrationTest, Retry) { testRetry(); } +TEST_P(IntegrationTest, EnvoyHandling100Continue) { testEnvoyHandling100Continue(); } + +TEST_P(IntegrationTest, EnvoyProxying100Continue) { testEnvoyProxying100Continue(); } + +TEST_P(IntegrationTest, EnvoyProxying100ContinueWithEncoderFilter) { + testEnvoyProxying100Continue(true); +} + TEST_P(IntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(IntegrationTest, RetryHittingBufferLimit) { testRetryHittingBufferLimit(); } diff --git a/test/integration/utility.h b/test/integration/utility.h index 71ff29e48b7f1..8e585326c1919 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -28,6 +28,7 @@ class BufferingStreamDecoder : public Http::StreamDecoder, public Http::StreamCa const std::string& body() { return body_; } // Http::StreamDecoder + void decode100ContinueHeaders(Http::HeaderMapPtr&&) override {} void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override; void decodeData(Buffer::Instance&, bool end_stream) override; void decodeTrailers(Http::HeaderMapPtr&& trailers) override; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index cfea80066e2ca..f5ef41e149eb0 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -60,6 +60,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(userAgent, const Optional&()); MOCK_METHOD0(tracingConfig, const Http::TracingConnectionManagerConfig*()); MOCK_METHOD0(listenerStats, ConnectionManagerListenerStats&()); + MOCK_CONST_METHOD0(proxy100Continue, bool()); }; class MockConnectionCallbacks : public virtual ConnectionCallbacks { @@ -86,6 +87,9 @@ class MockStreamDecoder : public StreamDecoder { MockStreamDecoder(); ~MockStreamDecoder(); + void decode100ContinueHeaders(HeaderMapPtr&& headers) override { + decode100ContinueHeaders_(headers); + } void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override { decodeHeaders_(headers, end_stream); } @@ -93,6 +97,7 @@ class MockStreamDecoder : public StreamDecoder { // Http::StreamDecoder MOCK_METHOD2(decodeHeaders_, void(HeaderMapPtr& headers, bool end_stream)); + MOCK_METHOD1(decode100ContinueHeaders_, void(HeaderMapPtr& headers)); MOCK_METHOD2(decodeData, void(Buffer::Instance& data, bool end_stream)); MOCK_METHOD1(decodeTrailers_, void(HeaderMapPtr& trailers)); }; @@ -142,6 +147,7 @@ class MockStreamEncoder : public StreamEncoder { ~MockStreamEncoder(); // Http::StreamEncoder + MOCK_METHOD1(encode100ContinueHeaders, void(const HeaderMap& headers)); MOCK_METHOD2(encodeHeaders, void(const HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, void(Buffer::Instance& data, bool end_stream)); MOCK_METHOD1(encodeTrailers, void(const HeaderMap& trailers)); @@ -225,6 +231,9 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(decoderBufferLimit, uint32_t()); // Http::StreamDecoderFilterCallbacks + void encode100ContinueHeaders(HeaderMapPtr&& headers) override { + encode100ContinueHeaders_(*headers); + } void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override { encodeHeaders_(*headers, end_stream); } @@ -233,6 +242,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(continueDecoding, void()); MOCK_METHOD2(addDecodedData, void(Buffer::Instance& data, bool streaming)); MOCK_METHOD0(decodingBuffer, const Buffer::Instance*()); + MOCK_METHOD1(encode100ContinueHeaders_, void(HeaderMap& headers)); MOCK_METHOD2(encodeHeaders_, void(HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, void(Buffer::Instance& data, bool end_stream)); MOCK_METHOD1(encodeTrailers_, void(HeaderMap& trailers)); @@ -300,6 +310,7 @@ class MockStreamEncoderFilter : public StreamEncoderFilter { MOCK_METHOD0(onDestroy, void()); // Http::MockStreamEncoderFilter + MOCK_METHOD1(encode100ContinueHeaders, FilterHeadersStatus(HeaderMap& headers)); MOCK_METHOD2(encodeHeaders, FilterHeadersStatus(HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, FilterDataStatus(Buffer::Instance& data, bool end_stream)); MOCK_METHOD1(encodeTrailers, FilterTrailersStatus(HeaderMap& trailers)); @@ -323,6 +334,7 @@ class MockStreamFilter : public StreamFilter { MOCK_METHOD1(setDecoderFilterCallbacks, void(StreamDecoderFilterCallbacks& callbacks)); // Http::MockStreamEncoderFilter + MOCK_METHOD1(encode100ContinueHeaders, FilterHeadersStatus(HeaderMap& headers)); MOCK_METHOD2(encodeHeaders, FilterHeadersStatus(HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, FilterDataStatus(Buffer::Instance& data, bool end_stream)); MOCK_METHOD1(encodeTrailers, FilterTrailersStatus(HeaderMap& trailers)); diff --git a/test/server/http/health_check_test.cc b/test/server/http/health_check_test.cc index 5b1ac4405da15..3ebf7e625650d 100644 --- a/test/server/http/health_check_test.cc +++ b/test/server/http/health_check_test.cc @@ -214,6 +214,23 @@ TEST_F(HealthCheckFilterPassThroughTest, Ok) { service_hc_respnose.EnvoyUpstreamHealthCheckedCluster()->value().c_str()); } +TEST_F(HealthCheckFilterPassThroughTest, OkWithContinue) { + EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(false)); + EXPECT_CALL(callbacks_.request_info_, healthCheck(true)); + EXPECT_CALL(callbacks_, encodeHeaders_(_, _)).Times(0); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + + // Goodness only knows why there would be a 100-Continue response in health + // checks but we can still verify Envoy handles it. + Http::TestHeaderMapImpl continue_respnose{{":status", "100"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(continue_respnose)); + Http::TestHeaderMapImpl service_hc_respnose{{":status", "200"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(service_hc_respnose, true)); + EXPECT_STREQ("cluster_name", + service_hc_respnose.EnvoyUpstreamHealthCheckedCluster()->value().c_str()); +} + TEST_F(HealthCheckFilterPassThroughTest, Failed) { EXPECT_CALL(context_, healthCheckFailed()).WillOnce(Return(true)); EXPECT_CALL(callbacks_.request_info_, healthCheck(true));