From 6d69def257c0bc820223b335cfaa7dac2bbbfee1 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Fri, 7 Feb 2020 11:12:16 -0500 Subject: [PATCH] http: fixing a bug where responseDataTooLarge skipped HCM work (#9923) Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 72 ++++++++++--------- source/common/http/conn_manager_impl.h | 16 ++++- test/common/http/conn_manager_impl_test.cc | 12 ++-- test/integration/protocol_integration_test.cc | 3 + 4 files changed, 65 insertions(+), 38 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 947d511fd5de..966efb63065f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1524,6 +1524,23 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte } } + const bool modified_end_stream = + encoding_headers_only_ || (end_stream && continue_data_entry == encoder_filters_.end()); + encodeHeadersInternal(headers, modified_end_stream); + + if (continue_data_entry != encoder_filters_.end() && !modified_end_stream) { + // We use the continueEncoding() code since it will correctly handle not calling + // encodeHeaders() again. Fake setting StopSingleIteration since the continueEncoding() code + // expects it. + ASSERT(buffered_response_data_); + (*continue_data_entry)->iteration_state_ = + ActiveStreamFilterBase::IterationState::StopSingleIteration; + (*continue_data_entry)->continueEncoding(); + } +} + +void ConnectionManagerImpl::ActiveStream::encodeHeadersInternal(HeaderMap& headers, + bool end_stream) { // Base headers. connection_manager_.config_.dateProvider().setDateHeader(headers); // Following setReference() is safe because serverName() is constant for the life of the listener. @@ -1622,29 +1639,12 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte chargeStats(headers); - ENVOY_STREAM_LOG(debug, "encoding headers via codec (end_stream={}):\n{}", *this, - encoding_headers_only_ || - (end_stream && continue_data_entry == encoder_filters_.end()), - headers); + ENVOY_STREAM_LOG(debug, "encoding headers via codec (end_stream={}):\n{}", *this, end_stream); // Now actually encode via the codec. stream_info_.onFirstDownstreamTxByteSent(); - response_encoder_->encodeHeaders( - headers, - encoding_headers_only_ || (end_stream && continue_data_entry == encoder_filters_.end())); - if (continue_data_entry != encoder_filters_.end()) { - // We use the continueEncoding() code since it will correctly handle not calling - // encodeHeaders() again. Fake setting StopSingleIteration since the continueEncoding() code - // expects it. - ASSERT(buffered_response_data_); - (*continue_data_entry)->iteration_state_ = - ActiveStreamFilterBase::IterationState::StopSingleIteration; - (*continue_data_entry)->continueEncoding(); - } else { - // End encoding if this is a header only response, either due to a filter converting it to one - // or due to the upstream returning headers only. - maybeEndEncode(encoding_headers_only_ || end_stream); - } + response_encoder_->encodeHeaders(headers, end_stream); + maybeEndEncode(end_stream); } void ConnectionManagerImpl::ActiveStream::encodeMetadata(ActiveStreamEncoderFilter* filter, @@ -1772,22 +1772,27 @@ void ConnectionManagerImpl::ActiveStream::encodeData( } } - ENVOY_STREAM_LOG(trace, "encoding data via codec (size={} end_stream={})", *this, data.length(), - end_stream); - - stream_info_.addBytesSent(data.length()); + const bool modified_end_stream = end_stream && trailers_added_entry == encoder_filters_.end(); + encodeDataInternal(data, modified_end_stream); // If trailers were adding during encodeData we need to trigger decodeTrailers in order // to allow filters to process the trailers. if (trailers_added_entry != encoder_filters_.end()) { - response_encoder_->encodeData(data, false); encodeTrailers(trailers_added_entry->get(), *response_trailers_); - } else { - response_encoder_->encodeData(data, end_stream); - maybeEndEncode(end_stream); } } +void ConnectionManagerImpl::ActiveStream::encodeDataInternal(Buffer::Instance& data, + bool end_stream) { + ASSERT(!encoding_headers_only_); + ENVOY_STREAM_LOG(trace, "encoding data via codec (size={} end_stream={})", *this, data.length(), + end_stream); + + stream_info_.addBytesSent(data.length()); + response_encoder_->encodeData(data, end_stream); + maybeEndEncode(end_stream); +} + void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilter* filter, HeaderMap& trailers) { resetIdleTimer(); @@ -2398,17 +2403,18 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.stream_info_.setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().RequestHeadersTooLarge); + // This does not call the standard sendLocalReply because if there is already response data + // we do not want to pass a second set of response headers through the filter chain. + // Instead, call the encodeHeadersInternal / encodeDataInternal helpers + // directly, which maximizes shared code with the normal response path. Http::Utility::sendLocalReply( Grpc::Common::hasGrpcContentType(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { - parent_.chargeStats(*response_headers); parent_.response_headers_ = std::move(response_headers); - parent_.response_encoder_->encodeHeaders(*parent_.response_headers_, end_stream); - parent_.state_.local_complete_ = end_stream; + parent_.encodeHeadersInternal(*parent_.response_headers_, end_stream); }, [&](Buffer::Instance& data, bool end_stream) -> void { - parent_.response_encoder_->encodeData(data, end_stream); - parent_.state_.local_complete_ = end_stream; + parent_.encodeDataInternal(data, end_stream); }, parent_.state_.destroyed_, Http::Code::InternalServerError, CodeUtility::toString(Http::Code::InternalServerError), absl::nullopt, diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 072215fb1831..961632a6f046 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -480,13 +480,27 @@ class ConnectionManagerImpl : Logger::Loggable, const absl::optional grpc_status, absl::string_view details); void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); + // As with most of the encode functions, this runs encodeHeaders on various + // filters before calling encodeHeadersInternal which does final header munging and passes the + // headers to the encoder. void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream); // Sends data through encoding filter chains. filter_iteration_start_state indicates which - // filter to start the iteration with. + // filter to start the iteration with, and finally calls encodeDataInternal + // to update stats, do end stream bookkeeping, and send the data to encoder. void encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instance& data, bool end_stream, FilterIterationStartState filter_iteration_start_state); void encodeTrailers(ActiveStreamEncoderFilter* filter, HeaderMap& trailers); void encodeMetadata(ActiveStreamEncoderFilter* filter, MetadataMapPtr&& metadata_map_ptr); + + // This is a helper function for encodeHeaders and responseDataTooLarge which allows for shared + // code for the two headers encoding paths. It does header munging, updates timing stats, and + // sends the headers to the encoder. + void encodeHeadersInternal(HeaderMap& headers, bool end_stream); + // This is a helper function for encodeData and responseDataTooLarge which allows for shared + // code for the two data encoding paths. It does stats updates and tracks potential end of + // stream. + void encodeDataInternal(Buffer::Instance& data, bool end_stream); + void maybeEndEncode(bool end_stream); // Returns true if new metadata is decoded. Otherwise, returns false. bool processNewlyAddedMetadata(); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d22c3c058180..2c26582175f4 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -3808,16 +3808,20 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) { // StopIterationAndBuffer, which will trigger an early response. expectOnDestroy(); - Http::TestHeaderMapImpl expected_response_headers{ - {":status", "500"}, {"content-length", "21"}, {"content-type", "text/plain"}}; Buffer::OwnedImpl fake_response("A long enough string to go over watermarks"); // Fake response starts doing through the filter. EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); std::string response_body; // The 500 goes directly to the encoder. - EXPECT_CALL(response_encoder_, - encodeHeaders(HeaderMapEqualRef(&expected_response_headers), false)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> FilterHeadersStatus { + // Make sure this is a 500 + EXPECT_EQ("500", headers.Status()->value().getStringView()); + // Make sure Envoy standard sanitization has been applied. + EXPECT_TRUE(headers.Date() != nullptr); + return FilterHeadersStatus::Continue; + })); EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); decoder_filters_[0]->callbacks_->encodeData(fake_response, false); EXPECT_EQ("Internal Server Error", response_body); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 5f87953192a9..45b3055c2352 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -580,6 +580,9 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("500", response->headers().Status()->value().getStringView()); + // Regression test https://github.com/envoyproxy/envoy/issues/9881 by making + // sure this path does standard HCM header transformations. + EXPECT_TRUE(response->headers().Date() != nullptr); EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("500")); test_server_->waitForCounterEq("http.config_test.downstream_rq_5xx", 1); }