From a1c22cfbe09a3724a50db909315899d357defb24 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Sat, 28 Jul 2018 18:51:47 -0700 Subject: [PATCH 1/9] fix sometimes returns response body to HEAD requests Signed-off-by: tianqian.zyf --- source/common/http/async_client_impl.cc | 4 + source/common/http/async_client_impl.h | 3 +- source/common/http/conn_manager_impl.cc | 48 +++---- source/common/http/conn_manager_impl.h | 7 +- source/common/http/http1/codec_impl.cc | 9 +- source/common/http/utility.cc | 16 ++- source/common/http/utility.h | 6 +- .../filters/http/grpc_web/grpc_web_filter.cc | 1 - .../filters/http/jwt_authn/filter.cc | 5 +- test/common/http/async_client_impl_test.cc | 25 ++++ test/common/http/common.cc | 5 +- test/common/http/common.h | 2 +- test/common/http/conn_manager_impl_test.cc | 121 +++++++++++++----- test/common/http/utility_test.cc | 17 ++- .../http/jwt_authn/filter_integration_test.cc | 21 +++ .../http/rbac/rbac_filter_integration_test.cc | 23 ++++ test/integration/http_integration.cc | 1 + .../idle_timeout_integration_test.cc | 32 ++++- test/mocks/http/mocks.h | 3 +- 19 files changed, 273 insertions(+), 76 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index e9e435cd7121..b4a875ac5457 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -112,6 +112,10 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { + if (Http::Headers::get().MethodValues.Head == headers.Method()->value().c_str()) { + is_head_request_ = true; + } + is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 801479bd43af..c1c578f5251c 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -282,7 +282,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - remote_closed_, code, body); + remote_closed_, code, body, is_head_request_); } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. @@ -308,6 +308,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, bool remote_closed_{}; Buffer::InstancePtr buffered_body_; bool is_grpc_request_{}; + bool is_head_request_{false}; friend class AsyncClientImpl; }; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3c5a9d56c8a3..c0fb71b5bf6e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -425,7 +425,7 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { } else { sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::RequestTimeout, "stream timeout", nullptr); + Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_); } } @@ -482,6 +482,10 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() { void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { request_headers_ = std::move(headers); + if (Http::Headers::get().MethodValues.Head == request_headers_->Method()->value().c_str()) { + is_head_request_ = true; + } + const bool upgrade_rejected = createFilterChain() == false; maybeEndDecode(end_stream); @@ -513,7 +517,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, request_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on. - sendLocalReply(false, Code::UpgradeRequired, "", nullptr); + sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -537,7 +541,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr); + nullptr, is_head_request_); return; } } @@ -551,7 +555,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // envoy users who do not wish to proxy large headers. if (request_headers_->byteSize() > (60 * 1024)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Code::RequestHeaderFieldsTooLarge, "", nullptr); + Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_); return; } @@ -562,8 +566,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", - nullptr); + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr, + is_head_request_); return; } @@ -608,7 +612,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Do not allow WebSocket upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", - nullptr); + nullptr, is_head_request_); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -924,7 +928,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, const std::string& body, - std::function modify_headers) { + std::function modify_headers, bool is_head_request) { Utility::sendLocalReply(is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { @@ -940,7 +944,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( // request instead. encodeData(nullptr, data, end_stream); }, - state_.destroyed_, code, body); + state_.destroyed_, code, body, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( @@ -1586,19 +1590,19 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.encoder_filters_streaming_ = true; stopped_ = false; - Http::Utility::sendLocalReply(Grpc::Common::hasGrpcContentType(*parent_.request_headers_), - [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { - parent_.response_headers_ = std::move(response_headers); - parent_.response_encoder_->encodeHeaders( - *parent_.response_headers_, end_stream); - parent_.state_.local_complete_ = end_stream; - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - parent_.response_encoder_->encodeData(data, end_stream); - parent_.state_.local_complete_ = end_stream; - }, - parent_.state_.destroyed_, Http::Code::InternalServerError, - CodeUtility::toString(Http::Code::InternalServerError)); + Http::Utility::sendLocalReply( + Grpc::Common::hasGrpcContentType(*parent_.request_headers_), + [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { + parent_.response_headers_ = std::move(response_headers); + parent_.response_encoder_->encodeHeaders(*parent_.response_headers_, end_stream); + parent_.state_.local_complete_ = end_stream; + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + parent_.response_encoder_->encodeData(data, end_stream); + parent_.state_.local_complete_ = end_stream; + }, + parent_.state_.destroyed_, Http::Code::InternalServerError, + CodeUtility::toString(Http::Code::InternalServerError), parent_.is_head_request_); parent_.maybeEndEncode(parent_.state_.local_complete_); } else { resetStream(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index f7a0b0fa6b92..1aeaa303bea5 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -170,7 +170,8 @@ class ConnectionManagerImpl : Logger::Loggable, } void sendLocalReply(Code code, const std::string& body, std::function modify_headers) override { - parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers); + parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, + parent_.is_head_request_); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -278,7 +279,8 @@ class ConnectionManagerImpl : Logger::Loggable, void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); HeaderMap& addEncodedTrailers(); void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, - std::function modify_headers); + std::function modify_headers, + bool is_head_request); 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); @@ -404,6 +406,7 @@ class ConnectionManagerImpl : Logger::Loggable, // 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_{}; + bool is_head_request_{false}; }; typedef std::unique_ptr ActiveStreamPtr; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index a08878e609d1..23fad3831cb9 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -119,6 +119,11 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) if (end_stream) { endEncode(); } else { + // Request is likely to be incomplete, in this case need to transfer the head request state into + // the pending response. + if (headers.Method() && + headers.Method()->value().c_str() == Http::Headers::get().MethodValues.Head) + connection_.onEncodeComplete(); connection_.flushOutput(); } } @@ -687,7 +692,9 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) void ClientConnectionImpl::onEncodeComplete() { // Transfer head request state into the pending response before we reuse the encoder. - pending_responses_.back().head_request_ = request_encoder_->headRequest(); + if (!pending_responses_.empty()) { + pending_responses_.back().head_request_ = request_encoder_->headRequest(); + } } int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e3ff89ad8594..0204f0fd32e9 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -240,8 +240,8 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co } void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, - const bool& is_reset, Code response_code, - const std::string& body_text) { + const bool& is_reset, Code response_code, const std::string& body_text, + bool is_head_request) { sendLocalReply(is_grpc, [&](HeaderMapPtr&& headers, bool end_stream) -> void { callbacks.encodeHeaders(std::move(headers), end_stream); @@ -249,13 +249,13 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac [&](Buffer::Instance& data, bool end_stream) -> void { callbacks.encodeData(data, end_stream); }, - is_reset, response_code, body_text); + is_reset, response_code, body_text, is_head_request); } void Utility::sendLocalReply( bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, - Code response_code, const std::string& body_text) { + Code response_code, const std::string& body_text, bool is_head_request) { // encode_headers() may reset the stream, so the stream must not be reset before calling it. ASSERT(!is_reset); // Respond with a gRPC trailers-only response if the request is gRPC @@ -265,7 +265,7 @@ void Utility::sendLocalReply( {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, {Headers::get().GrpcStatus, std::to_string(enumToInt(Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}}; - if (!body_text.empty()) { + if (!body_text.empty() && !is_head_request) { // TODO: GrpcMessage should be percent-encoded response_headers->insertGrpcMessage().value(body_text); } @@ -279,6 +279,12 @@ void Utility::sendLocalReply( response_headers->insertContentLength().value(body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } + + if (is_head_request) { + encode_headers(std::move(response_headers), true); + return; + } + encode_headers(std::move(response_headers), body_text.empty()); // encode_headers()) may have changed the referenced is_reset so we need to test it if (!body_text.empty() && !is_reset) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 8a11ae5129f2..65787cb24e0a 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -131,9 +131,10 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @param response_code supplies the HTTP response code. * @param body_text supplies the optional body text which is sent using the text/plain content * type. + * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text); + Code response_code, const std::string& body_text, bool is_head_request); /** * Create a locally generated response using the provided lambdas. @@ -150,7 +151,8 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const void sendLocalReply(bool is_grpc, std::function encode_headers, std::function encode_data, - const bool& is_reset, Code response_code, const std::string& body_text); + const bool& is_reset, Code response_code, const std::string& body_text, + bool is_head_request = false); struct GetLastAddressFromXffInfo { // Last valid address pulled from the XFF header. diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index ffc0c0c20cfa..7bb94ec36541 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -45,7 +45,6 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, return Http::FilterHeadersStatus::Continue; } is_grpc_web_request_ = true; - // Remove content-length header since it represents http1.1 payload size, not the sum of the h2 // DATA frame payload lengths. https://http2.github.io/http2-spec/#malformed This effectively // switches to chunked encoding which is the default for h2 diff --git a/source/extensions/filters/http/jwt_authn/filter.cc b/source/extensions/filters/http/jwt_authn/filter.cc index 9ae057236d68..ac23444423e9 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -21,7 +21,6 @@ void Filter::onDestroy() { Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) { ENVOY_LOG(debug, "Called Filter : {}", __func__); - // Remove headers configured to pass payload auth_->sanitizePayloadHeaders(headers); @@ -51,8 +50,8 @@ void Filter::onComplete(const Status& status) { // verification failed Http::Code code = Http::Code::Unauthorized; // return failure reason as message body - Http::Utility::sendLocalReply(false, *decoder_callbacks_, false, code, - ::google::jwt_verify::getStatusString(status)); + decoder_callbacks_->sendLocalReply(code, ::google::jwt_verify::getStatusString(status), + nullptr); return; } diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 7ade4bbefba3..c81ac4dffcfa 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -741,6 +741,31 @@ TEST_F(AsyncClientImplTest, StreamTimeout) { cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("upstream_rq_504").value()); } +TEST_F(AsyncClientImplTest, StreamTimeoutHeadReply) { + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](StreamDecoder&, + ConnectionPool::Callbacks& callbacks) -> ConnectionPool::Cancellable* { + callbacks.onPoolReady(stream_encoder_, cm_.conn_pool_.host_); + return nullptr; + })); + + MessagePtr message{new RequestMessageImpl()}; + HttpTestUtility::addDefaultHeaders(message->headers(), "HEAD"); + EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(&message->headers()), true)); + timer_ = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(40))); + EXPECT_CALL(stream_encoder_.stream_, resetStream(_)); + + TestHeaderMapImpl expected_timeout{ + {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; + EXPECT_CALL(stream_callbacks_, onHeaders_(HeaderMapEqualRef(&expected_timeout), true)); + + AsyncClient::Stream* stream = + client_.start(stream_callbacks_, std::chrono::milliseconds(40), false); + stream->sendHeaders(message->headers(), true); + timer_->callback_(); +} + TEST_F(AsyncClientImplTest, RequestTimeout) { EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) .WillOnce(Invoke([&](StreamDecoder&, diff --git a/test/common/http/common.cc b/test/common/http/common.cc index 07628caf9661..ca5cca0d98e2 100644 --- a/test/common/http/common.cc +++ b/test/common/http/common.cc @@ -5,9 +5,10 @@ #include "envoy/http/header_map.h" namespace Envoy { -void HttpTestUtility::addDefaultHeaders(Http::HeaderMap& headers) { +void HttpTestUtility::addDefaultHeaders(Http::HeaderMap& headers, + const std::string default_method) { headers.insertScheme().value(std::string("http")); - headers.insertMethod().value(std::string("GET")); + headers.insertMethod().value(default_method); headers.insertHost().value(std::string("host")); headers.insertPath().value(std::string("/")); } diff --git a/test/common/http/common.h b/test/common/http/common.h index f24df9f94f94..f3180fa4c49e 100644 --- a/test/common/http/common.h +++ b/test/common/http/common.h @@ -63,6 +63,6 @@ struct ConnPoolCallbacks : public Http::ConnectionPool::Callbacks { */ class HttpTestUtility { public: - static void addDefaultHeaders(Http::HeaderMap& headers); + static void addDefaultHeaders(Http::HeaderMap& headers, const std::string default_method = "GET"); }; } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 9e23e99d74be..dc512d985f14 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -162,7 +162,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi setUpBufferLimits(); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); @@ -359,11 +360,12 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) { // Test not charging stats on the second call. if (data.length() == 4) { - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); } else { - HeaderMapPtr headers{ - new TestHeaderMapImpl{{":authority", "host"}, {":path", "/healthcheck"}}}; + HeaderMapPtr headers{new TestHeaderMapImpl{ + {":authority", "host"}, {":path", "/healthcheck"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); } @@ -417,7 +419,8 @@ TEST_F(HttpConnectionManagerImplTest, 100ContinueResponse) { decoder = &conn_manager_->newStream(encoder); // Test not charging stats on the second call. - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; @@ -522,8 +525,8 @@ TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{ - new TestHeaderMapImpl{{":authority", "host"}, {":path", "http://api.lyft.com/"}}}; + HeaderMapPtr headers{new TestHeaderMapImpl{ + {":authority", "host"}, {":path", "http://api.lyft.com/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); data.drain(4); })); @@ -1112,7 +1115,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) { .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); data.drain(4); @@ -1172,7 +1176,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteOverride) { EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(30))); decoder->decodeHeaders(std::move(headers), false); @@ -1200,7 +1205,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteZeroOverride) { EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(10))); StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, disableTimer()); decoder->decodeHeaders(std::move(headers), false); @@ -1224,7 +1230,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(_)); decoder->decodeHeaders(std::move(headers), false); @@ -1263,7 +1270,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNormalTermination) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(_)); decoder->decodeHeaders(std::move(headers), false); @@ -1291,7 +1299,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(_)); decoder->decodeHeaders(std::move(headers), false); @@ -1343,7 +1352,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterUpstreamHeaders) StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); Event::MockTimer* idle_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(_)); decoder->decodeHeaders(std::move(headers), false); @@ -1391,7 +1401,8 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) { StreamDecoder* decoder; EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; EXPECT_CALL(*idle_timer, enableTimer(_)); decoder->decodeHeaders(std::move(headers), false); @@ -1898,7 +1909,8 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { NiceMock encoder; EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); @@ -1931,7 +1943,8 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); })); @@ -1974,7 +1987,8 @@ TEST_F(HttpConnectionManagerImplTest, ResponseStartBeforeRequestComplete) { NiceMock encoder; EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); })); @@ -2077,7 +2091,8 @@ TEST_F(HttpConnectionManagerImplTest, IdleTimeout) { StreamDecoder* decoder = nullptr; EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); @@ -2117,7 +2132,8 @@ TEST_F(HttpConnectionManagerImplTest, IntermediateBufferingEarlyResponse) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); @@ -2161,7 +2177,8 @@ TEST_F(HttpConnectionManagerImplTest, DoubleBuffering) { Buffer::OwnedImpl fake_data_copy("hello"); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); decoder->decodeData(fake_data, true); })); @@ -2200,7 +2217,8 @@ TEST_F(HttpConnectionManagerImplTest, ZeroByteDataFiltering) { StreamDecoder* decoder = nullptr; EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); })); @@ -2403,7 +2421,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyInTrailersCallback) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); @@ -2479,7 +2498,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyDuringDecodeData) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl data1("hello"); @@ -2542,7 +2562,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyInline) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); @@ -2587,7 +2608,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterClearRouteCache) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); @@ -2783,7 +2805,8 @@ TEST_F(HttpConnectionManagerImplTest, UnderlyingConnectionWatermarksPassedOnWith setupFilterChain(2, 2); EXPECT_CALL(filter_callbacks_.connection_, aboveHighWatermark()).Times(0); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) @@ -2843,7 +2866,8 @@ TEST_F(HttpConnectionManagerImplTest, UnderlyingConnectionWatermarksUnwoundWithL setupFilterChain(2, 2); EXPECT_CALL(filter_callbacks_.connection_, aboveHighWatermark()).Times(0); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) @@ -2956,7 +2980,8 @@ TEST_F(HttpConnectionManagerImplTest, HitRequestBufferLimitsIntermediateFilter) EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); @@ -3049,13 +3074,46 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsAfterHeaders) { EXPECT_EQ(1U, stats_.named_.rs_too_large_.value()); } +TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { + InSequence s; + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "HEAD"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(4); + })); + + setupFilterChain(1, 1); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) + .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { + decoder_filters_[0]->callbacks_->sendLocalReply(Code::BadRequest, "Bad request", nullptr); + return FilterHeadersStatus::Continue; + })); + + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, true)) + .WillOnce(Invoke([&](HeaderMap& headers, bool) -> FilterHeadersStatus { + EXPECT_STREQ("11", headers.ContentLength()->value().c_str()); + return FilterHeadersStatus::Continue; + })); + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)); + expectOnDestroy(); + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + TEST_F(HttpConnectionManagerImplTest, FilterAddBodyContinuation) { InSequence s; setup(false, ""); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), true); })); @@ -3102,7 +3160,8 @@ TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 2a3473d80168..93476a8f8a33 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -333,7 +333,7 @@ TEST(HttpUtility, SendLocalReply) { EXPECT_CALL(callbacks, encodeHeaders_(_, false)); EXPECT_CALL(callbacks, encodeData(_, true)); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); } TEST(HttpUtility, SendLocalGrpcReply) { @@ -348,7 +348,7 @@ TEST(HttpUtility, SendLocalGrpcReply) { EXPECT_NE(headers.GrpcMessage(), nullptr); EXPECT_STREQ(headers.GrpcMessage()->value().c_str(), "large"); })); - Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); + Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { @@ -359,7 +359,18 @@ TEST(HttpUtility, SendLocalReplyDestroyedEarly) { is_reset = true; })); EXPECT_CALL(callbacks, encodeData(_, true)).Times(0); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); +} + +TEST(HttpUtility, SendLocalReplyHeadRequest) { + MockStreamDecoderFilterCallbacks callbacks; + bool is_reset = false; + EXPECT_CALL(callbacks, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ(headers.ContentLength()->value().c_str(), + fmt::format("{}", strlen("large")).c_str()); + })); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", true); } TEST(HttpUtility, TestExtractHostPathFromUri) { diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index eb3f8803de90..536d373f21aa 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -87,6 +87,27 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) { EXPECT_STREQ("401", response->headers().Status()->value().c_str()); } +TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { + config_helper_.addFilter(getFilterConfig(true)); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeHeaderOnlyRequest(Http::TestHeaderMapImpl{ + {":method", "HEAD"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"Authorization", "Bearer " + std::string(ExpiredToken)}, + }); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("401", response->headers().Status()->value().c_str()); + EXPECT_STRNE("0", response->headers().ContentLength()->value().c_str()); + EXPECT_STREQ("", response->body().c_str()); +} + // The test case with a fake upstream for remote Jwks server. class RemoteJwksIntegrationTest : public HttpProtocolIntegrationTest { public: diff --git a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc index 9e766f7bc4f5..4f90220d13ac 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc @@ -66,6 +66,29 @@ TEST_P(RBACIntegrationTest, Denied) { EXPECT_STREQ("403", response->headers().Status()->value().c_str()); } +TEST_P(RBACIntegrationTest, DeniedHeadReply) { + config_helper_.addFilter(RBAC_CONFIG); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestHeaderMapImpl{ + {":method", "HEAD"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + }, + 1024); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("403", response->headers().Status()->value().c_str()); + ASSERT_TRUE(response->headers().ContentLength()); + EXPECT_STRNE("0", response->headers().ContentLength()->value().c_str()); + EXPECT_STREQ("", response->body().c_str()); +} + TEST_P(RBACIntegrationTest, RouteOverride) { config_helper_.addConfigModifier( [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& cfg) { diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 67ec481704df..1761bfd00638 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -14,6 +14,7 @@ #include "common/api/api_impl.h" #include "common/buffer/buffer_impl.h" #include "common/common/fmt.h" +#include "common/http/headers.h" #include "common/network/connection_impl.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 3006fcf25a28..b12bb305d7e3 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -47,6 +47,23 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { return response; } + IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutHeadRequestTest() { + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "HEAD"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_); + upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_); + upstream_request_->waitForHeadersComplete(); + return response; + } + void sleep() { std::this_thread::sleep_for(std::chrono::milliseconds(TimeoutMs / 2)); } void waitForTimeout(IntegrationStreamDecoder& response) { @@ -75,7 +92,6 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { auto response = setupPerStreamIdleTimeoutTest(); waitForTimeout(*response); - EXPECT_FALSE(upstream_request_->complete()); EXPECT_EQ(0U, upstream_request_->bodyLength()); EXPECT_TRUE(response->complete()); @@ -83,6 +99,20 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { EXPECT_EQ("stream timeout", response->body()); } +// Per-stream idle timeout after having sent downstream head request. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstreamHeadRequest) { + auto response = setupPerStreamIdleTimeoutHeadRequestTest(); + + waitForTimeout(*response); + EXPECT_FALSE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + EXPECT_TRUE(response->complete()); + EXPECT_STREQ("408", response->headers().Status()->value().c_str()); + EXPECT_STREQ(fmt::format("{}", strlen("stream timeout")).c_str(), + response->headers().ContentLength()->value().c_str()); + EXPECT_EQ("", response->body()); +} + // Global per-stream idle timeout applies if there is no per-stream idle timeout. TEST_P(IdleTimeoutIntegrationTest, GlobalPerStreamIdleTimeoutAfterDownstreamHeaders) { enable_global_idle_timeout_ = true; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 90e49e5d03d6..0474cf74a0b7 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -210,7 +210,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - stream_destroyed_, code, body); + stream_destroyed_, code, body, is_head_request_); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override { encode100ContinueHeaders_(*headers); @@ -234,6 +234,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, testing::NiceMock active_span_; testing::NiceMock tracing_config_; bool is_grpc_request_{}; + bool is_head_request_{false}; bool stream_destroyed_{}; }; From cf8bd8749d195684c79cbc75a927e776d80a6b83 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 Aug 2018 10:43:49 +0800 Subject: [PATCH 2/9] Modify the call point of connection_.onEncodeComplete() Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 23fad3831cb9..f7a95db426d1 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -119,11 +119,6 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) if (end_stream) { endEncode(); } else { - // Request is likely to be incomplete, in this case need to transfer the head request state into - // the pending response. - if (headers.Method() && - headers.Method()->value().c_str() == Http::Headers::get().MethodValues.Head) - connection_.onEncodeComplete(); connection_.flushOutput(); } } @@ -252,6 +247,11 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ if (method->value() == Headers::get().MethodValues.Head.c_str()) { head_request_ = true; + // If this is an incomplete request (end_stream=false), then connection_.onEncodeComplete(); + // will not be called in StreamEncoderImpl::encodeHeaders, head request state will not be passed + // to the corresponding response so here need to call this method + if (!end_stream) + connection_.onEncodeComplete(); } connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); @@ -692,9 +692,7 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) void ClientConnectionImpl::onEncodeComplete() { // Transfer head request state into the pending response before we reuse the encoder. - if (!pending_responses_.empty()) { - pending_responses_.back().head_request_ = request_encoder_->headRequest(); - } + pending_responses_.back().head_request_ = request_encoder_->headRequest(); } int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { From de67043a3d5bae5cd6b8ec32fd156400c09371ac Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Thu, 9 Aug 2018 23:27:10 +0800 Subject: [PATCH 3/9] Remove duplicated code Signed-off-by: tianqian.zyf --- .../idle_timeout_integration_test.cc | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index b12bb305d7e3..f03006dcb116 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -26,12 +26,12 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { HttpProtocolIntegrationTest::initialize(); } - IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest() { + IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutTest(const char* method = "GET") { initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto encoder_decoder = - codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", method}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}}); @@ -47,23 +47,6 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest { return response; } - IntegrationStreamDecoderPtr setupPerStreamIdleTimeoutHeadRequestTest() { - initialize(); - fake_upstreams_[0]->set_allow_unexpected_disconnects(true); - codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto encoder_decoder = - codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "HEAD"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_); - upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_); - upstream_request_->waitForHeadersComplete(); - return response; - } - void sleep() { std::this_thread::sleep_for(std::chrono::milliseconds(TimeoutMs / 2)); } void waitForTimeout(IntegrationStreamDecoder& response) { @@ -101,7 +84,7 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) { // Per-stream idle timeout after having sent downstream head request. TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutHeadRequestAfterDownstreamHeadRequest) { - auto response = setupPerStreamIdleTimeoutHeadRequestTest(); + auto response = setupPerStreamIdleTimeoutTest("HEAD"); waitForTimeout(*response); EXPECT_FALSE(upstream_request_->complete()); From 6cc889f06d64fa85c2ecc55d8bdd4df43429bc62 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Fri, 10 Aug 2018 09:53:46 +0800 Subject: [PATCH 4/9] Add setHeadStateForLastResponse method Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 10 +++++++--- source/common/http/http1/codec_impl.h | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index f7a95db426d1..7ee4d4b3f9b2 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -250,8 +250,12 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ // If this is an incomplete request (end_stream=false), then connection_.onEncodeComplete(); // will not be called in StreamEncoderImpl::encodeHeaders, head request state will not be passed // to the corresponding response so here need to call this method - if (!end_stream) - connection_.onEncodeComplete(); + if (!end_stream) { + auto c = dynamic_cast(&connection_); + if (c) { + c->setHeadStateForLastResponse(true); + } + } } connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); @@ -692,7 +696,7 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) void ClientConnectionImpl::onEncodeComplete() { // Transfer head request state into the pending response before we reuse the encoder. - pending_responses_.back().head_request_ = request_encoder_->headRequest(); + setHeadStateForLastResponse(request_encoder_->headRequest()); } int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 63a8d5540db3..d7d4f624c5fa 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -98,7 +98,6 @@ class ResponseStreamEncoderImpl : public StreamEncoderImpl { class RequestStreamEncoderImpl : public StreamEncoderImpl { public: RequestStreamEncoderImpl(ConnectionImpl& connection) : StreamEncoderImpl(connection) {} - bool headRequest() { return head_request_; } // Http::StreamEncoder @@ -327,6 +326,9 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { // Http::ClientConnection StreamEncoder& newStream(StreamDecoder& response_decoder) override; + void setHeadStateForLastResponse(bool is_head) { + pending_responses_.back().head_request_ = is_head; + } private: struct PendingResponse { From 5e8c9187e779e1e3a17a33d35390927a9abb3a2e Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Tue, 14 Aug 2018 20:50:34 +0800 Subject: [PATCH 5/9] Add onEncodeHeader interface Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 21 +++++++-------------- source/common/http/http1/codec_impl.h | 10 +++++++--- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 7ee4d4b3f9b2..38a884e09ba2 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -244,20 +244,10 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ if (!method || !path) { throw CodecClientException(":method and :path must be specified"); } - if (method->value() == Headers::get().MethodValues.Head.c_str()) { head_request_ = true; - // If this is an incomplete request (end_stream=false), then connection_.onEncodeComplete(); - // will not be called in StreamEncoderImpl::encodeHeaders, head request state will not be passed - // to the corresponding response so here need to call this method - if (!end_stream) { - auto c = dynamic_cast(&connection_); - if (c) { - c->setHeadStateForLastResponse(true); - } - } } - + connection_.onEncodeHeader(headers); connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); connection_.copyToBuffer(method->value().c_str(), method->value().size()); connection_.addCharToBuffer(' '); @@ -694,9 +684,12 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) return *request_encoder_; } -void ClientConnectionImpl::onEncodeComplete() { - // Transfer head request state into the pending response before we reuse the encoder. - setHeadStateForLastResponse(request_encoder_->headRequest()); +void ClientConnectionImpl::onEncodeComplete() {} + +void ClientConnectionImpl::onEncodeHeader(const HeaderMapImpl& headers) { + if (headers.Method()->value() == Headers::get().MethodValues.Head.c_str()) { + pending_responses_.back().head_request_ = true; + } } int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index d7d4f624c5fa..a9b88e3965ce 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -122,6 +122,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Tue, 14 Aug 2018 21:38:21 +0800 Subject: [PATCH 6/9] Fix the bad code according to the review Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 7 +++---- source/common/http/http1/codec_impl.h | 10 +++++----- .../filters/http/grpc_web/grpc_web_filter.cc | 1 + 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 38a884e09ba2..e68e8f9487f4 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -247,7 +247,7 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ if (method->value() == Headers::get().MethodValues.Head.c_str()) { head_request_ = true; } - connection_.onEncodeHeader(headers); + connection_.onEncodeHeader(headers, end_stream); connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); connection_.copyToBuffer(method->value().c_str(), method->value().size()); connection_.addCharToBuffer(' '); @@ -684,9 +684,8 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) return *request_encoder_; } -void ClientConnectionImpl::onEncodeComplete() {} - -void ClientConnectionImpl::onEncodeHeader(const HeaderMapImpl& headers) { +void ClientConnectionImpl::onEncodeHeader(const HeaderMapImpl& headers, bool end_stream) { + UNREFERENCED_PARAMETER(end_stream); if (headers.Method()->value() == Headers::get().MethodValues.Head.c_str()) { pending_responses_.back().head_request_ = true; } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index a9b88e3965ce..c950370def95 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -123,9 +123,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Mon, 20 Aug 2018 13:38:56 +0800 Subject: [PATCH 7/9] Change onEncodeHeader to onEncodeHeaders Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 5 ++--- source/common/http/http1/codec_impl.h | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index e68e8f9487f4..ceb44446282a 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -247,7 +247,7 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ if (method->value() == Headers::get().MethodValues.Head.c_str()) { head_request_ = true; } - connection_.onEncodeHeader(headers, end_stream); + connection_.onEncodeHeaders(headers); connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); connection_.copyToBuffer(method->value().c_str(), method->value().size()); connection_.addCharToBuffer(' '); @@ -684,8 +684,7 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) return *request_encoder_; } -void ClientConnectionImpl::onEncodeHeader(const HeaderMapImpl& headers, bool end_stream) { - UNREFERENCED_PARAMETER(end_stream); +void ClientConnectionImpl::onEncodeHeaders(const HeaderMapImpl& headers) { if (headers.Method()->value() == Headers::get().MethodValues.Head.c_str()) { pending_responses_.back().head_request_ = true; } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index c950370def95..b9b81aae7cfe 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -125,7 +125,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Tue, 21 Aug 2018 00:18:08 +0800 Subject: [PATCH 8/9] Fix build errors Signed-off-by: tianqian.zyf --- source/common/http/http1/codec_impl.cc | 2 +- source/common/http/http1/codec_impl.h | 6 +++--- test/common/http/conn_manager_impl_test.cc | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index ceb44446282a..24d9d6d49c3e 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -684,7 +684,7 @@ StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& response_decoder) return *request_encoder_; } -void ClientConnectionImpl::onEncodeHeaders(const HeaderMapImpl& headers) { +void ClientConnectionImpl::onEncodeHeaders(const HeaderMap& headers) { if (headers.Method()->value() == Headers::get().MethodValues.Head.c_str()) { pending_responses_.back().head_request_ = true; } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index b9b81aae7cfe..6f8cafd2c971 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -125,7 +125,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); @@ -2335,7 +2336,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInDataCallbackNoTrailers) EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; decoder->decodeHeaders(std::move(headers), false); Buffer::OwnedImpl fake_data("hello"); From 2f5deb2677b32a2a106543fae21a695ed8b51595 Mon Sep 17 00:00:00 2001 From: "tianqian.zyf" Date: Tue, 21 Aug 2018 00:51:33 +0800 Subject: [PATCH 9/9] Add method for fuzz test Signed-off-by: tianqian.zyf --- test/common/http/conn_manager_impl_corpus/codec_exception | 4 ++++ test/common/http/conn_manager_impl_corpus/missing_host | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/test/common/http/conn_manager_impl_corpus/codec_exception b/test/common/http/conn_manager_impl_corpus/codec_exception index b82a0312dadb..5100a94dd4c5 100644 --- a/test/common/http/conn_manager_impl_corpus/codec_exception +++ b/test/common/http/conn_manager_impl_corpus/codec_exception @@ -1,6 +1,10 @@ actions { new_stream { request_headers { + headers { + key: ":method" + value: "GET" + } headers { key: "foo" value: "bar" diff --git a/test/common/http/conn_manager_impl_corpus/missing_host b/test/common/http/conn_manager_impl_corpus/missing_host index 4e2f4d8dd6f2..2a95cdd3837c 100644 --- a/test/common/http/conn_manager_impl_corpus/missing_host +++ b/test/common/http/conn_manager_impl_corpus/missing_host @@ -1,6 +1,10 @@ actions { new_stream { request_headers { + headers { + key: ":method" + value: "GET" + } headers { key: "foo" value: "bar"