diff --git a/docs/root/configuration/http/http_conn_man/response_code_details.rst b/docs/root/configuration/http/http_conn_man/response_code_details.rst index ebca3a321f2e7..25faa8bd94da2 100644 --- a/docs/root/configuration/http/http_conn_man/response_code_details.rst +++ b/docs/root/configuration/http/http_conn_man/response_code_details.rst @@ -26,7 +26,8 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen duration_timeout, The max connection duration was exceeded. direct_response, A direct response was generated by the router filter. filter_chain_not_found, The request was rejected due to no matching filter chain. - filter_removed_required_headers, The request was rejected in the filter manager because a configured filter removed required headers. + filter_removed_required_request_headers, The request was rejected in the filter manager because a configured filter removed required request headers. + filter_removed_required_response_headers, The response was rejected in the filter manager because a configured filter removed required response headers or these values were invalid (e.g. overflown). internal_redirect, The original stream was replaced with an internal redirect. low_version, The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured. maintenance_mode, The request was rejected by the router filter because the cluster was in maintenance mode. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 70544787ca44d..029794a9e4f29 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -139,16 +139,18 @@ class ResponseEncoder : public virtual StreamEncoder { /** * Encode 100-Continue headers. * @param headers supplies the 100-Continue header map to encode. + * @return indicates the result of encoding and has Http::StatusCode::Ok for success. */ - virtual void encode100ContinueHeaders(const ResponseHeaderMap& headers) PURE; + virtual Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) PURE; /** * Encode headers, optionally indicating end of stream. Response headers must - * have a valid :status set. + * have a valid :status set * @param headers supplies the header map to encode. * @param end_stream supplies whether this is a header only response. + * @return indicates the result of encoding and has Http::StatusCode::Ok for success. */ - virtual void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) PURE; + virtual Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) PURE; /** * Encode trailers. This implicitly ends the stream. diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 71d4fdbb2886b..2d523c157578e 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -180,9 +180,11 @@ struct ResponseCodeDetailValues { // The original stream was replaced with an internal redirect. const std::string InternalRedirect = "internal_redirect"; // The request was rejected because configured filters erroneously removed required headers. - const std::string FilterRemovedRequiredHeaders = "filter_removed_required_headers"; + const std::string FilterRemovedRequiredRequestHeaders = "filter_removed_required_request_headers"; + const std::string FilterRemovedRequiredResponseHeaders = + "filter_removed_required_response_headers"; // Changes or additions to details should be reflected in - // docs/root/configuration/http/http_conn_man/response_code_details_details.rst + // docs/root/configuration/http/http_conn_man/response_code_details.rst }; using ResponseCodeDetails = ConstSingleton; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 1a77d506c0eee..bf39e9b7a532d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -901,8 +901,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he 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()); + const auto& continue_headers = continueHeader(); + const auto status = response_encoder_->encode100ContinueHeaders(continue_headers); + // We must make sure static `continue_headers` is valid in its content and encoding not fail. + ASSERT(status.ok()); + chargeStats(continue_headers); // Remove the Expect header so it won't be handled again upstream. request_headers_->removeExpect(); } @@ -1334,13 +1337,15 @@ void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers_.get(), connection_manager_.config_, EMPTY_STRING); - // Count both the 1xx and follow-up response code in stats. - chargeStats(response_headers); - ENVOY_STREAM_LOG(debug, "encoding 100 continue headers via codec:\n{}", *this, response_headers); // Now actually encode via the codec. - response_encoder_->encode100ContinueHeaders(response_headers); + const auto status = response_encoder_->encode100ContinueHeaders(response_headers); + // encode100ContinueHeaders is always called with valid headers soon after parsing so must be OK. + ASSERT(status.ok()); + + // Count both the 1xx and follow-up response code in stats. + chargeStats(response_headers); } void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& headers, @@ -1456,14 +1461,26 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade } } - chargeStats(headers); - ENVOY_STREAM_LOG(debug, "encoding headers via codec (end_stream={}):\n{}", *this, end_stream, headers); // Now actually encode via the codec. + const auto status = response_encoder_->encodeHeaders(headers, end_stream); + if (!status.ok()) { + // This branch can happen when a misbehaving filter chain removed critical headers or set + // malformed header values. + // Also the bad http response status from upstream over HTTP2/3 (e.g. overflown status value) + // reaches here. + const auto request_headers = requestHeaders(); + sendLocalReply( + request_headers.has_value() && Grpc::Common::isGrpcRequestHeaders(request_headers.ref()), + Http::Code::BadGateway, status.message(), nullptr, absl::nullopt, + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredResponseHeaders, + "{", status.message(), "}")); + return; + } filter_manager_.streamInfo().onFirstDownstreamTxByteSent(); - response_encoder_->encodeHeaders(headers, end_stream); + chargeStats(headers); } void ConnectionManagerImpl::ActiveStream::encodeData(Buffer::Instance& data, bool end_stream) { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 86f99fa118cad..7255af4626e5e 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -250,12 +250,14 @@ class ConnectionManagerImpl : Logger::Loggable, } void endStream() override { + // This assertion must satisfy and be guarded by callers with streamEnded(). ASSERT(!state_.codec_saw_local_complete_); state_.codec_saw_local_complete_ = true; filter_manager_.streamInfo().onLastDownstreamTxByteSent(); request_response_timespan_->complete(); connection_manager_.doEndStream(*this); } + bool streamEnded() override { return state_.codec_saw_local_complete_; } void onDecoderFilterBelowWriteBufferLowWatermark() override; void onDecoderFilterAboveWriteBufferHighWatermark() override; void upgradeFilterChainCreated() override { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 754ff6e8f5942..cfd15c6f75870 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1011,8 +1011,10 @@ void FilterManager::maybeContinueEncoding( void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHeaderMap& headers, bool end_stream) { // See encodeHeaders() comments in include/envoy/http/filter.h for why the 1xx precondition holds. - ASSERT(!CodeUtility::is1xx(Utility::getResponseStatus(headers)) || - Utility::getResponseStatus(headers) == enumToInt(Http::Code::SwitchingProtocols)); + ASSERT(HeaderUtility::checkRequiredResponseHeaders(headers).ok() && + (!CodeUtility::is1xx(Utility::getResponseStatus(headers)) || + Utility::getResponseStatus(headers) == enumToInt(Http::Code::SwitchingProtocols))); + filter_manager_callbacks_.resetIdleTimer(); disarmRequestTimeout(); @@ -1065,8 +1067,8 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea } const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); - state_.non_100_response_headers_encoded_ = true; filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); + state_.non_100_response_headers_encoded_ = true; maybeEndEncode(modified_end_stream); if (!modified_end_stream) { @@ -1247,7 +1249,10 @@ void FilterManager::encodeTrailers(ActiveStreamEncoderFilter* filter, } void FilterManager::maybeEndEncode(bool end_stream) { - if (end_stream) { + // filter_manager_callbacks_.streamEnded() returns True here when the codec failed to encode + // headers due to the lack of required response headers, and filter_manager_callbacks already sent + // the local reply and ended the stream by itself. So in that case this function must be no-op. + if (end_stream && !filter_manager_callbacks_.streamEnded()) { filter_manager_callbacks_.endStream(); } } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index e72e800041d02..6888ce0eeb947 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -460,6 +460,11 @@ class FilterManagerCallbacks { */ virtual void endStream() PURE; + /** + * Called to check if endStream() is called by filter manager. + */ + virtual bool streamEnded() PURE; + /** * Called when the stream write buffer is no longer above the low watermark. */ diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 5d8dc4f3a8996..db838282f39f4 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -300,7 +300,7 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol, return false; } -Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) { +Http::Status HeaderUtility::checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers) { if (!headers.Method()) { return absl::InvalidArgumentError( absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get())); @@ -322,6 +322,16 @@ Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& h return Http::okStatus(); } +Http::Status HeaderUtility::checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers) { + const HeaderEntry* header = headers.Status(); + uint64_t response_code; + if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { + return absl::InvalidArgumentError( + absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Status.get())); + } + return Http::okStatus(); +} + bool HeaderUtility::isRemovableHeader(absl::string_view header) { return (header.empty() || header[0] != ':') && !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()); diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index e5d58f39ca8e4..b28250d2fbbdc 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -189,13 +189,20 @@ class HeaderUtility { */ static void stripPortFromHost(RequestHeaderMap& headers, absl::optional listener_port); - /* Does a common header check ensuring required headers are present. + /* Does a common header check ensuring required request headers are present. * Required request headers include :method header, :path for non-CONNECT requests, and * host/authority for HTTP/1.1 or CONNECT requests. * @return Status containing the result. If failed, message includes details on which header was * missing. */ - static Http::Status checkRequiredHeaders(const Http::RequestHeaderMap& headers); + static Http::Status checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers); + + /* Does a common header check ensuring required response headers are present. + * Current required request headers only includes :status. + * @return Status containing the result. If failed, message includes details on which header was + * missing. + */ + static Http::Status checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers); /** * Returns true if a header may be safely removed without causing additional diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3b245d4f8712e..75e6a36235220 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -119,9 +119,9 @@ void StreamEncoderImpl::encodeFormattedHeader(absl::string_view key, absl::strin } } -void ResponseEncoderImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) { +Http::Status ResponseEncoderImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) { ASSERT(headers.Status()->value() == "100"); - encodeHeaders(headers, false); + return encodeHeaders(headers, false); } void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& headers, @@ -366,11 +366,12 @@ const Network::Address::InstanceConstSharedPtr& StreamEncoderImpl::connectionLoc static const char RESPONSE_PREFIX[] = "HTTP/1.1 "; static const char HTTP_10_RESPONSE_PREFIX[] = "HTTP/1.0 "; -void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) { +Http::Status ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) { started_response_ = true; - // The contract is that client codecs must ensure that :status is present. - ASSERT(headers.Status() != nullptr); + // The contract is that client codecs must ensure that required headers are present. + RETURN_IF_ERROR(HeaderUtility::checkRequiredResponseHeaders(headers)); + uint64_t numeric_status = Utility::getResponseStatus(headers); if (connection_.protocol() == Protocol::Http10 && connection_.supportsHttp10()) { @@ -394,6 +395,7 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e } encodeHeadersBase(headers, absl::make_optional(numeric_status), end_stream, false); + return Http::okStatus(); } static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; @@ -401,7 +403,7 @@ static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers)); const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index f4c5e7d46f8e9..f85f0952ef0ee 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -133,8 +133,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { bool startedResponse() { return started_response_; } // Http::ResponseEncoder - void encode100ContinueHeaders(const ResponseHeaderMap& headers) override; - void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; + Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) override; + Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } bool streamErrorOnInvalidHttpMessage() const override { diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 41c083acf2acb..49aca7675e92d 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -24,6 +24,7 @@ #include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/http2/codec_stats.h" +#include "common/http/status.h" #include "common/http/utility.h" #include "common/runtime/runtime_features.h" @@ -172,9 +173,10 @@ void ConnectionImpl::StreamImpl::buildHeaders(std::vector& final_hea }); } -void ConnectionImpl::ServerStreamImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) { +Http::Status +ConnectionImpl::ServerStreamImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) { ASSERT(headers.Status()->value() == "100"); - encodeHeaders(headers, false); + return encodeHeaders(headers, false); } void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector& final_headers, @@ -201,7 +203,7 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers)); // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. std::vector final_headers; @@ -231,10 +233,10 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h return okStatus(); } -void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers, - bool end_stream) { - // The contract is that client codecs must ensure that :status is present. - ASSERT(headers.Status() != nullptr); +Http::Status ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers, + bool end_stream) { + // The contract is that client codecs must ensure that required headers are present. + RETURN_IF_ERROR(HeaderUtility::checkRequiredResponseHeaders(headers)); // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. @@ -248,6 +250,8 @@ void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& he buildHeaders(final_headers, headers); } encodeHeadersBase(final_headers, end_stream); + + return Http::okStatus(); } void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) { diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 1f73d14f423f5..6102dfccf5296 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -411,8 +411,8 @@ class ConnectionImpl : public virtual Connection, void createPendingFlushTimer() override; // ResponseEncoder - void encode100ContinueHeaders(const ResponseHeaderMap& headers) override; - void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; + Http::Status encode100ContinueHeaders(const ResponseHeaderMap& headers) override; + Http::Status encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override; void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 8c503832d816a..70f6017eca951 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -57,7 +57,7 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredRequestHeaders(headers)); ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers); local_end_stream_ = end_stream; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 1f7a668b0e1dd..ce9901483b445 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -5,6 +5,8 @@ #include +#include "common/http/status.h" + #if defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -64,12 +66,15 @@ EnvoyQuicServerStream::EnvoyQuicServerStream( stats, http3_options), headers_with_underscores_action_(headers_with_underscores_action) {} -void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) { +Http::Status +EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) { ASSERT(headers.Status()->value() == "100"); - encodeHeaders(headers, false); + return encodeHeaders(headers, false); } -void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) { +Http::Status EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers, + bool end_stream) { + RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredResponseHeaders(headers)); ENVOY_STREAM_LOG(debug, "encodeHeaders (end_stream={}) {}.", *this, end_stream, headers); // QUICHE guarantees to take all the headers. This could cause infinite data to // be buffered on headers stream in Google QUIC implementation because @@ -83,6 +88,7 @@ void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers local_end_stream_ = end_stream; SendBufferMonitor::ScopedWatermarkBufferUpdater updater(this, this); WriteHeaders(envoyHeadersToSpdyHeaderBlock(headers), end_stream, nullptr); + return Http::okStatus(); } void EnvoyQuicServerStream::encodeData(Buffer::Instance& data, bool end_stream) { diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index e9059861c5e28..95b4f804b631c 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -1,5 +1,7 @@ #pragma once +#include "common/http/status.h" + #if defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-parameter" @@ -37,8 +39,8 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, void setRequestDecoder(Http::RequestDecoder& decoder) { request_decoder_ = &decoder; } // Http::StreamEncoder - void encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) override; - void encodeHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) override; + Http::Status encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) override; + Http::Status encodeHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) override; void encodeData(Buffer::Instance& data, bool end_stream) override; void encodeTrailers(const Http::ResponseTrailerMap& trailers) override; void encodeMetadata(const Http::MetadataMapVector& metadata_map_vector) override; diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 3213f156aa812..19f3f37bf03ea 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -446,8 +446,8 @@ void UpstreamRequest::onPoolReady( // erroneously remove required headers. stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - status.message(), "}"); + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredRequestHeaders, + "{", status.message(), "}"); parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr, absl::nullopt, details); return; diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index d0fbdca11a4f8..6771b3a04c3d9 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -208,7 +208,7 @@ class HttpStream : public LinkedObject { auto headers = fromSanitizedHeaders(directional_action.continue_headers()); headers.setReferenceKey(Headers::get().Status, "100"); - state.response_encoder_->encode100ContinueHeaders(headers); + (void)state.response_encoder_->encode100ContinueHeaders(headers); } break; } @@ -222,7 +222,7 @@ class HttpStream : public LinkedObject { if (headers.Status() == nullptr) { headers.setReferenceKey(Headers::get().Status, "200"); } - state.response_encoder_->encodeHeaders(headers, end_stream); + (void)state.response_encoder_->encodeHeaders(headers, end_stream); } else { state.request_encoder_ ->encodeHeaders( diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index ed9f08f86a162..834313b5a96f3 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -20,6 +20,7 @@ #include "common/http/date_provider_impl.h" #include "common/http/exception.h" #include "common/http/header_utility.h" +#include "common/http/status.h" #include "common/network/address_impl.h" #include "common/network/utility.h" @@ -292,10 +293,12 @@ class FuzzStream { } // If sendLocalReply is called: ON_CALL(encoder_, encodeHeaders(_, true)) - .WillByDefault(Invoke([this](const ResponseHeaderMap&, bool end_stream) -> void { - response_state_ = - end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers; - })); + .WillByDefault( + Invoke([this](const ResponseHeaderMap&, bool end_stream) -> Http::Status { + response_state_ = + end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers; + return Http::okStatus(); + })); decoder_->decodeHeaders(std::move(headers), end_stream); return Http::okStatus(); })); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 53c1abe198fab..f10ce5c17386f 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1,3 +1,5 @@ +#include "common/http/status.h" + #include "test/common/http/conn_manager_impl_test_base.h" #include "test/test_common/logging.h" #include "test/test_common/test_runtime.h" @@ -384,10 +386,11 @@ TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("404", headers.getStatusValue()); EXPECT_EQ("absolute_path_rejected", filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); + return Http::okStatus(); })); EXPECT_CALL(*filter, onStreamComplete()); EXPECT_CALL(*filter, onDestroy()); @@ -425,10 +428,11 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("400", headers.getStatusValue()); EXPECT_EQ("path_normalization_failed", filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); + return Http::okStatus(); })); EXPECT_CALL(*filter, onStreamComplete()); EXPECT_CALL(*filter, onDestroy()); @@ -973,8 +977,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { // Should be no 'x-envoy-decorator-operation' response header. EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(nullptr, headers.EnvoyDecoratorOperation()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -1042,8 +1047,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat // Verify decorator operation response header has been defined. EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("testOp", headers.getEnvoyDecoratorOperationValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -1109,8 +1115,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat // Verify decorator operation response header has NOT been defined (i.e. not propagated). EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(nullptr, headers.EnvoyDecoratorOperation()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -1177,8 +1184,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat // Should be no 'x-envoy-decorator-operation' response header, as decorator // was overridden by request header. EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(nullptr, headers.EnvoyDecoratorOperation()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -1749,7 +1757,7 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest .WillOnce(Return(stream_error_on_invalid_http_message)); EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("400", headers.getStatusValue()); EXPECT_EQ("missing_host_header", filter->decoder_callbacks_->streamInfo().responseCodeDetails().value()); @@ -1759,6 +1767,7 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest } else { EXPECT_EQ(nullptr, headers.Connection()); } + return Http::okStatus(); })); EXPECT_CALL(*filter, onStreamComplete()); @@ -1881,8 +1890,9 @@ TEST_F(HttpConnectionManagerImplTest, NoPath) { })); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("404", headers.getStatusValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -1936,8 +1946,9 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutGlobal) { // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("408", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); @@ -2021,8 +2032,9 @@ TEST_F(HttpConnectionManagerImplTest, TestStreamIdleAccessLog) { // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("408", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; @@ -2324,8 +2336,9 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("408", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); @@ -2398,8 +2411,9 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders // 408 direct response after timeout. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("408", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); @@ -2450,8 +2464,9 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterUpstreamHeaders) // 200 upstream response. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("200", headers.getStatusValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -2519,8 +2534,9 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) { // 200 upstream response. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("200", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; @@ -2595,8 +2611,9 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutCallbackDisarmsAndReturns408 EXPECT_CALL(*request_timer, disableTimer()).Times(AtLeast(1)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("408", headers.getStatusValue()); + return Http::okStatus(); })); EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); @@ -2916,9 +2933,10 @@ TEST_F(HttpConnectionManagerImplTest, Http10Rejected) { })); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("426", headers.getStatusValue()); EXPECT_EQ("close", headers.getConnectionValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -2966,8 +2984,9 @@ TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { })); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("403", headers.getStatusValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -2994,9 +3013,10 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) { [&](HeaderMap&, bool) -> FilterHeadersStatus { return FilterHeadersStatus::Continue; })); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_NE(nullptr, headers.Connection()); EXPECT_EQ("upgrade", headers.getConnectionValue()); + return Http::okStatus(); })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index a2b6df75c31fb..6d16e22fbad67 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -1,3 +1,5 @@ +#include "common/http/status.h" + #include "test/common/http/conn_manager_impl_test_base.h" #include "test/test_common/logging.h" #include "test/test_common/test_runtime.h" @@ -26,9 +28,10 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) { startRequest(); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_NE(nullptr, headers.Server()); EXPECT_EQ("envoy-server-test", headers.getServerValue()); + return Http::okStatus(); })); EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); EXPECT_CALL(*decoder_filters_[0], onDestroy()); @@ -51,10 +54,11 @@ TEST_F(HttpConnectionManagerImplTest, DisconnectOnProxyConnectionDisconnect) { startRequest(); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_NE(nullptr, headers.Connection()); EXPECT_EQ("close", headers.getConnectionValue()); EXPECT_EQ(nullptr, headers.ProxyConnection()); + return Http::okStatus(); })); EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); EXPECT_CALL(*decoder_filters_[0], onDestroy()); @@ -95,9 +99,10 @@ TEST_F(HttpConnectionManagerImplTest, ResponseStartBeforeRequestComplete) { // Start the response ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_NE(nullptr, headers.Server()); EXPECT_EQ("", headers.getServerValue()); + return Http::okStatus(); })); filter->callbacks_->streamInfo().setResponseCodeDetails(""); filter->callbacks_->encodeHeaders(std::move(response_headers), false, "details"); @@ -1384,14 +1389,14 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) { std::string response_body; // The 500 goes directly to the encoder. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> FilterHeadersStatus { + .WillOnce(Invoke([&](const ResponseHeaderMap& headers, bool) -> Http::Status { // Make sure this is a 500 EXPECT_EQ("500", headers.getStatusValue()); // Make sure Envoy standard sanitization has been applied. EXPECT_TRUE(headers.Date() != nullptr); EXPECT_EQ("response_payload_too_large", decoder_filters_[0]->callbacks_->streamInfo().responseCodeDetails().value()); - return FilterHeadersStatus::Continue; + return Http::okStatus(); })); EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); decoder_filters_[0]->callbacks_->encodeData(fake_response, false); @@ -2079,8 +2084,9 @@ TEST_F(HttpConnectionManagerImplTest, NoNewStreamWhenOverloaded) { // 503 direct response when overloaded. EXPECT_CALL(response_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("503", headers.getStatusValue()); + return Http::okStatus(); })); std::string response_body; EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body)); @@ -2127,8 +2133,9 @@ TEST_F(HttpConnectionManagerImplTest, DisableHttp1KeepAliveWhenOverloaded) { })); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("close", headers.getConnectionValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input("1234"); @@ -2316,8 +2323,9 @@ TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenDraining) { })); EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ("close", headers.getConnectionValue()); + return Http::okStatus(); })); Buffer::OwnedImpl fake_input; diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 1ae991ff85c82..5b1743b96ce0f 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -33,6 +33,8 @@ class FilterManagerTest : public testing::Test { StreamInfo::FilterState::LifeSpan::Connection); } + ~FilterManagerTest() override { filter_manager_->destroyFilters(); } + std::unique_ptr filter_manager_; NiceMock filter_manager_callbacks_; Event::MockDispatcher dispatcher_; @@ -89,7 +91,6 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringDecodingGrpcClassiciation) { EXPECT_CALL(filter_manager_callbacks_, encodeHeaders(_, _)); EXPECT_CALL(filter_manager_callbacks_, endStream()); filter_manager_->decodeHeaders(*grpc_headers, true); - filter_manager_->destroyFilters(); } // Verifies that the local reply persists the gRPC classification even if the request headers are @@ -145,7 +146,6 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringEncodingGrpcClassiciation) { EXPECT_CALL(filter_manager_callbacks_, encodeHeaders(_, _)); EXPECT_CALL(filter_manager_callbacks_, endStream()); filter_manager_->decodeHeaders(*grpc_headers, true); - filter_manager_->destroyFilters(); } struct TestAction : Matcher::ActionBase {}; @@ -202,7 +202,6 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionDecodingHeaders) { filter_manager_->requestHeadersInitialized(); filter_manager_->decodeHeaders(*grpc_headers, true); - filter_manager_->destroyFilters(); } TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) { @@ -268,8 +267,6 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) { RequestTrailerMapPtr trailers{new TestRequestTrailerMapImpl{{"trailer", ""}}}; filter_manager_->decodeTrailers(*trailers); - - filter_manager_->destroyFilters(); } // Verify that we propagate custom match actions to a decoding filter. @@ -303,7 +300,6 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingHeaders) { filter_manager_->requestHeadersInitialized(); filter_manager_->decodeHeaders(*grpc_headers, true); - filter_manager_->destroyFilters(); } // Verify that we propagate custom match actions to a decoding filter when matching on request @@ -349,7 +345,6 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDecodingTrailers) { filter_manager_->decodeTrailers(*trailers); EXPECT_CALL(*decoder_filter, onDestroy()); - filter_manager_->destroyFilters(); } // Verify that we propagate custom match actions to an encoding filter when matching on response @@ -399,7 +394,6 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionEncodingTrailers) { EXPECT_CALL(*filter, encodeTrailers(_)); filter_manager_->decodeHeaders(*grpc_headers, true); EXPECT_CALL(*filter, onDestroy()); - filter_manager_->destroyFilters(); } // Verify that we propagate custom match actions exactly once to a dual filter. @@ -441,7 +435,6 @@ TEST_F(FilterManagerTest, MatchTreeFilterActionDualFilter) { EXPECT_CALL(*filter, encodeHeaders(_, true)); EXPECT_CALL(*filter, onMatchCallback(_)); filter_manager_->decodeHeaders(*grpc_headers, true); - filter_manager_->destroyFilters(); } TEST_F(FilterManagerTest, OnLocalReply) { @@ -482,8 +475,6 @@ TEST_F(FilterManagerTest, OnLocalReply) { ASSERT_TRUE(filter_manager_->streamInfo().responseCodeDetails().has_value()); EXPECT_EQ(filter_manager_->streamInfo().responseCodeDetails().value(), "details"); EXPECT_FALSE(filter_manager_->streamInfo().responseCode().has_value()); - - filter_manager_->destroyFilters(); } TEST_F(FilterManagerTest, MultipleOnLocalReply) { @@ -541,8 +532,25 @@ TEST_F(FilterManagerTest, MultipleOnLocalReply) { ASSERT_TRUE(filter_manager_->streamInfo().responseCodeDetails().has_value()); EXPECT_EQ(filter_manager_->streamInfo().responseCodeDetails().value(), "details2"); EXPECT_FALSE(filter_manager_->streamInfo().responseCode().has_value()); +} + +TEST_F(FilterManagerTest, MaybeEndEncode) { + initialize(); + + // endStream must not be called. + filter_manager_->maybeEndEncode(false); + + // endStream must be called since end_stream = true && streamEnded returns false. + EXPECT_CALL(filter_manager_callbacks_, streamEnded()).WillOnce(Invoke([]() -> bool { + return false; + })); + filter_manager_->maybeEndEncode(true); - filter_manager_->destroyFilters(); + // endStream must not be called since streamEnded returns true. + EXPECT_CALL(filter_manager_callbacks_, streamEnded()).WillOnce(Invoke([]() -> bool { + return true; + })); + filter_manager_->maybeEndEncode(true); } } // namespace diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index ca358e2a7a05d..aa37256b6e7d4 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -6,6 +6,7 @@ #include "envoy/json/json_object.h" #include "common/http/header_utility.h" +#include "common/http/status.h" #include "common/json/json_loader.h" #include "test/test_common/test_runtime.h" @@ -722,6 +723,37 @@ TEST(RequiredHeaders, IsModifiableHeader) { EXPECT_TRUE(HeaderUtility::isModifiableHeader("Content-Type")); } +TEST(CheckRequiredRequestHeaders, OK) { + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders( + TestRequestHeaderMapImpl{{":method", "GET"}, {":path", "/"}})); + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{ + {":method", "CONNECT"}, {":authority", "localhost:1234"}})); +} + +TEST(CheckRequiredRequestHeaders, NG) { + EXPECT_EQ(absl::InvalidArgumentError("missing required header: :method"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{})); + + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :path"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "GET"}})); + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :authority"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "CONNECT"}})); +} + +TEST(CheckRequiredResponseHeaders, OK) { + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredResponseHeaders( + TestResponseHeaderMapImpl{{":status", "200"}})); +} + +TEST(CheckRequiredResponseHeaders, NG) { + EXPECT_EQ(absl::InvalidArgumentError("missing required header: :status"), + HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{})); + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :status"), + HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{{":status", "abcd"}})); +} TEST(ValidateHeaders, HeaderNameWithUnderscores) { Stats::MockCounter dropped; Stats::MockCounter rejected; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index e8dd865582394..39febc3a62013 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -119,7 +119,8 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase { auto status = codec_->dispatch(buffer); EXPECT_TRUE(status.ok()); EXPECT_EQ(0U, buffer.length()); - response_encoder->encodeHeaders(TestResponseHeaderMapImpl{{":status", "200"}}, true); + EXPECT_TRUE( + response_encoder->encodeHeaders(TestResponseHeaderMapImpl{{":status", "200"}}, true).ok()); } protected: @@ -804,7 +805,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { std::string output; ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); EXPECT_EQ(Protocol::Http10, codec_->protocol()); } @@ -1061,7 +1062,7 @@ TEST_F(Http1ServerConnectionImplTest, FloodProtection) { })); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); } // Trying to accept a third request with two buffered responses in the queue should trigger flood @@ -1355,7 +1356,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponse) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); } @@ -1382,7 +1383,7 @@ TEST_F(Http1ServerConnectionImplTest, LargeHeaderResponseEncode) { const std::string long_header_value = std::string(79 * 1024, 'a'); TestResponseHeaderMapImpl headers{{":status", "200"}, {"foo", long_header_value}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\nfoo: " + long_header_value + "\r\ncontent-length: 0\r\n\r\n", output); } @@ -1409,7 +1410,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseTrainProperHeaders) { TestResponseHeaderMapImpl headers{ {":status", "200"}, {"some-header", "foo"}, {"some#header", "baz"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\nSome-Header: foo\r\nSome#Header: baz\r\nContent-Length: 0\r\n\r\n", output); } @@ -1434,7 +1435,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseWith204) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "204"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 204 No Content\r\n\r\n", output); } @@ -1458,14 +1459,14 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseWith100Then200) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; - response_encoder->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder->encode100ContinueHeaders(continue_headers).ok()); EXPECT_EQ("HTTP/1.1 100 Continue\r\n\r\n", output); output.clear(); // Test the special case where we encode 100 headers (no content length may be // appended) then 200 headers (content length 0 will be appended). TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); } @@ -1518,7 +1519,7 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedResponse) { })); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, false); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, false).ok()); Buffer::OwnedImpl data("Hello World"); response_encoder->encodeData(data, true); @@ -1548,7 +1549,7 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedResponseWithTrailers) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, false); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, false).ok()); Buffer::OwnedImpl data("Hello World"); response_encoder->encodeData(data, false); @@ -1581,7 +1582,7 @@ TEST_F(Http1ServerConnectionImplTest, ContentLengthResponse) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}, {"content-length", "11"}}; - response_encoder->encodeHeaders(headers, false); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, false).ok()); Buffer::OwnedImpl data("Hello World"); response_encoder->encodeData(data, true); @@ -1608,7 +1609,7 @@ TEST_F(Http1ServerConnectionImplTest, HeadRequestResponse) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}, {"content-length", "5"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 5\r\n\r\n", output); } @@ -1632,7 +1633,7 @@ TEST_F(Http1ServerConnectionImplTest, HeadChunkedRequestResponse) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\n", output); } @@ -1656,7 +1657,8 @@ TEST_F(Http1ServerConnectionImplTest, DoubleRequest) { EXPECT_TRUE(status.ok()); EXPECT_EQ(request.size(), buffer.length()); - response_encoder->encodeHeaders(TestResponseHeaderMapImpl{{":status", "200"}}, true); + EXPECT_TRUE( + response_encoder->encodeHeaders(TestResponseHeaderMapImpl{{":status", "200"}}, true).ok()); status = codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); @@ -1802,7 +1804,7 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestResponseHeaders) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestResponseHeaderMapImpl headers{{":status", "101"}}; - response_encoder->encodeHeaders(headers, false); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, false).ok()); EXPECT_EQ("HTTP/1.1 101 Switching Protocols\r\n\r\n", output); } @@ -1934,7 +1936,7 @@ TEST_F(Http1ServerConnectionImplTest, WatermarkTest) { EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()); TestResponseHeaderMapImpl headers{{":status", "200"}}; - response_encoder->encodeHeaders(headers, false); + EXPECT_TRUE(response_encoder->encodeHeaders(headers, false).ok()); // Fake out the underlying Network::Connection buffer being drained. EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()); @@ -3208,5 +3210,30 @@ TEST_F(Http1ClientConnectionImplTest, ShouldDumpCorrespondingRequestWithoutAlloc EXPECT_THAT(ostream.contents(), testing::HasSubstr("Dumping corresponding downstream request:")); } +// Verify that encodeHeaders() gracefully returned when invalid headers given. +TEST_F(Http1ServerConnectionImplTest, StatusResponseHeaderNotPresent) { + initialize(); + + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer{"GET / HTTP/1.1\r\n\r\n"}; + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(0U, buffer.length()); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + TestResponseHeaderMapImpl headers{}; // :status header does not exist. + EXPECT_EQ(response_encoder->encodeHeaders(headers, true), + absl::InvalidArgumentError("missing required header: :status")); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index b6bdf85aa97e8..5489a23a9e842 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -303,7 +303,7 @@ class Http2CodecImplTest : public ::testing::TestWithParamencodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); Buffer::OwnedImpl data("0"); EXPECT_NO_THROW(response_encoder_->encodeData(data, false)); @@ -352,7 +352,7 @@ TEST_P(Http2CodecImplTest, ShutdownNotice) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); } TEST_P(Http2CodecImplTest, ProtocolErrorForTest) { @@ -384,11 +384,24 @@ TEST_P(Http2CodecImplTest, ContinueHeaders) { TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); +}; + +// Codec must return InvalidArgument status when the required request headers not being present. +TEST_P(Http2CodecImplTest, InvalidResponseHeaders) { + initialize(); + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); + + TestResponseHeaderMapImpl response_headers; + EXPECT_EQ(response_encoder_->encodeHeaders(response_headers, true), + absl::InvalidArgumentError("missing required header: :status")); }; // nghttp2 rejects trailers with :status. @@ -402,14 +415,15 @@ TEST_P(Http2CodecImplTest, TrailerStatus) { TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // nghttp2 doesn't allow :status in trailers - EXPECT_THROW(response_encoder_->encode100ContinueHeaders(continue_headers), ClientCodecError); + EXPECT_THROW((void)response_encoder_->encode100ContinueHeaders(continue_headers), + ClientCodecError); EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value()); }; @@ -424,13 +438,13 @@ TEST_P(Http2CodecImplTest, MultipleContinueHeaders) { TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); }; // 101/102 headers etc. are passed to the response encoder (who is responsibly for deciding to @@ -445,7 +459,7 @@ TEST_P(Http2CodecImplTest, 1xxNonContinueHeaders) { TestResponseHeaderMapImpl other_headers{{":status", "102"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(other_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(other_headers, false).ok()); }; // nghttp2 treats 101 inside an HTTP/2 stream as an invalid HTTP header field. @@ -459,7 +473,7 @@ TEST_P(Http2CodecImplTest, Invalid101SwitchingProtocols) { TestResponseHeaderMapImpl upgrade_headers{{":status", "101"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, _)).Times(0); - EXPECT_THROW(response_encoder_->encodeHeaders(upgrade_headers, false), ClientCodecError); + EXPECT_THROW((void)response_encoder_->encodeHeaders(upgrade_headers, false), ClientCodecError); EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value()); } @@ -472,7 +486,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; - EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError); + EXPECT_THROW((void)response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError); EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value()); } @@ -494,7 +508,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) { Invoke([&](Buffer::Instance& data, bool) -> void { client_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; - response_encoder_->encodeHeaders(continue_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(continue_headers, true).ok()); // Flush pending data. EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::ProtocolError, _)); @@ -539,9 +553,9 @@ TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); - EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError); + EXPECT_THROW((void)response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError); EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value()); }; @@ -559,14 +573,14 @@ TEST_P(Http2CodecImplTest, InvalidRepeatContinueAllowed) { TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); - response_encoder_->encode100ContinueHeaders(continue_headers); + EXPECT_TRUE(response_encoder_->encode100ContinueHeaders(continue_headers).ok()); // Buffer client data to avoid mock recursion causing lifetime issues. ON_CALL(server_connection_, write(_, _)) .WillByDefault( Invoke([&](Buffer::Instance& data, bool) -> void { client_wrapper_.buffer_.add(data); })); - response_encoder_->encodeHeaders(continue_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(continue_headers, true).ok()); // Flush pending data. EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::ProtocolError, _)); @@ -599,7 +613,8 @@ TEST_P(Http2CodecImplTest, Invalid204WithContentLength) { "debug", "Invalid HTTP header field was received: frame type: 1, stream: 1, name: [content-length], " "value: [3]", - EXPECT_THROW(response_encoder_->encodeHeaders(response_headers, false), ClientCodecError)); + EXPECT_THROW((void)response_encoder_->encodeHeaders(response_headers, false), + ClientCodecError)); EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value()); }; @@ -629,7 +644,7 @@ TEST_P(Http2CodecImplTest, Invalid204WithContentLengthAllowed) { response_headers.addCopy(std::to_string(i), std::to_string(i)); } - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Flush pending data. EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::ProtocolError, _)); @@ -682,7 +697,7 @@ TEST_P(Http2CodecImplTest, TrailingHeaders) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)); Buffer::OwnedImpl world("world"); response_encoder_->encodeData(world, false); @@ -712,7 +727,7 @@ TEST_P(Http2CodecImplTest, IgnoreTrailingEmptyHeaders) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)); Buffer::OwnedImpl world("world"); response_encoder_->encodeData(world, false); @@ -741,7 +756,7 @@ TEST_P(Http2CodecImplTest, SubmitTrailingEmptyHeaders) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)); Buffer::OwnedImpl world("world"); response_encoder_->encodeData(world, false); @@ -774,7 +789,7 @@ TEST_P(Http2CodecImplTest, TrailingHeadersLargeClientBody) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)); Buffer::OwnedImpl world("world"); response_encoder_->encodeData(world, false); @@ -1021,7 +1036,7 @@ TEST_P(Http2CodecImplTest, ShouldDumpActiveStreamsWithoutAllocatingMemory) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); // Dump server { @@ -1141,7 +1156,7 @@ TEST_P(Http2CodecImplTest, ClientConnectionShouldDumpCorrespondingRequestWithout })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Check contents for the corresponding downstream request. EXPECT_THAT( @@ -1185,7 +1200,7 @@ TEST_P(Http2CodecImplDeferredResetTest, DeferredResetClient) { // seeing any headers as the stream should already be reset on the other side, even though // we don't know about it yet. TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); })); EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1)); EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::RemoteReset, _)); @@ -1210,7 +1225,7 @@ TEST_P(Http2CodecImplDeferredResetTest, DeferredResetServer) { .WillByDefault( Invoke([&](Buffer::Instance& data, bool) -> void { client_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); EXPECT_CALL(server_stream_callbacks_, onAboveWriteBufferHighWatermark()).Times(AnyNumber()); auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_); @@ -1442,7 +1457,7 @@ TEST_P(Http2CodecImplFlowControlTest, TrailingHeadersLargeServerBody) { Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(server_stream_callbacks_, onAboveWriteBufferHighWatermark()); EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1)); auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_); @@ -1479,7 +1494,7 @@ TEST_P(Http2CodecImplFlowControlTest, TrailingHeadersLargeServerBodyFlushTimeout Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(server_stream_callbacks_, onAboveWriteBufferHighWatermark()); EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1)); auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_); @@ -1514,7 +1529,7 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeout) { Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1)); auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_); EXPECT_CALL(*flush_timer, enableTimer(std::chrono::milliseconds(30000), _)); @@ -1547,7 +1562,7 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeoutAfterGoaway) { Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1)); auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_); EXPECT_CALL(*flush_timer, enableTimer(std::chrono::milliseconds(30000), _)); @@ -1609,7 +1624,7 @@ TEST_P(Http2CodecImplFlowControlTest, WindowUpdateOnReadResumingFlood) { // pre-fill downstream outbound frame queue TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above and pre-fill outbound queue with 1 byte DATA frames for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 2; ++i) { Buffer::OwnedImpl data("0"); @@ -1653,7 +1668,7 @@ TEST_P(Http2CodecImplFlowControlTest, RstStreamOnPendingFlushTimeoutFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above and pre-fill outbound queue with 6 byte DATA frames for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 2; ++i) { Buffer::OwnedImpl data(std::string(6, '0')); @@ -1724,7 +1739,7 @@ TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) { server_->onUnderlyingConnectionAboveWriteBufferHighWatermark(); server_->onUnderlyingConnectionBelowWriteBufferLowWatermark(); })); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); } class Http2CodecImplStreamLimitTest : public Http2CodecImplTest {}; @@ -2099,7 +2114,7 @@ TEST_P(Http2CodecImplTest, ManyResponseHeadersAccepted) { response_headers.addCopy(std::to_string(i), ""); } EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); } TEST_P(Http2CodecImplTest, LargeRequestHeadersAtLimitAccepted) { @@ -2205,7 +2220,7 @@ TEST_P(Http2CodecImplTestAll, TestCodecHeaderCompression) { TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"compression", "test"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, true).ok()); // Sanity check to verify that state of encoders and decoders matches. EXPECT_EQ(nghttp2_session_get_hd_deflate_dynamic_table_size(server_->session()), @@ -2343,7 +2358,7 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1; ++i) { - EXPECT_NO_THROW(response_encoder_->encodeHeaders(response_headers, false)); + EXPECT_NO_THROW((void)response_encoder_->encodeHeaders(response_headers, false)); } EXPECT_TRUE(violation_callback->enabled_); @@ -2375,7 +2390,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { Buffer::OwnedImpl data("0"); @@ -2407,7 +2422,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) { EXPECT_CALL(response_decoder_, decodeData(_, false)) .Times(CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { Buffer::OwnedImpl data("0"); @@ -2439,7 +2454,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodCounterReset) { })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < kMaxOutboundFrames - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2481,7 +2496,7 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { })); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2516,7 +2531,7 @@ TEST_P(Http2CodecImplTest, ResponseTrailersFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2556,7 +2571,7 @@ TEST_P(Http2CodecImplTest, MetadataFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2651,7 +2666,7 @@ TEST_P(Http2CodecImplTest, GoAwayCausesOutboundFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2691,7 +2706,7 @@ TEST_P(Http2CodecImplTest, ShudowNoticeCausesOutboundFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2744,7 +2759,7 @@ TEST_P(Http2CodecImplTest, KeepAliveCausesOutboundFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Pre-fill outbound frame queue 1 away from overflow (account for the single HEADERS frame above) for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); @@ -2786,7 +2801,7 @@ TEST_P(Http2CodecImplTest, ResetStreamCausesOutboundFlood) { new NiceMock(&server_connection_.dispatcher_); TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - response_encoder_->encodeHeaders(response_headers, false); + EXPECT_TRUE(response_encoder_->encodeHeaders(response_headers, false).ok()); // Account for the single HEADERS frame above for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); diff --git a/test/common/quic/envoy_quic_server_session_test.cc b/test/common/quic/envoy_quic_server_session_test.cc index c2c93111a1ddf..9ed40db4dc36e 100644 --- a/test/common/quic/envoy_quic_server_session_test.cc +++ b/test/common/quic/envoy_quic_server_session_test.cc @@ -526,7 +526,7 @@ TEST_P(EnvoyQuicServerSessionTest, WriteUpdatesDelayCloseTimer) { Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {":content-length", "32770"}}; // 32KB + 2 bytes - stream->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream->encodeHeaders(response_headers, false).ok()); std::string response(32 * 1024 + 1, 'a'); Buffer::OwnedImpl buffer(response); EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); @@ -620,7 +620,7 @@ TEST_P(EnvoyQuicServerSessionTest, FlushCloseNoTimeout) { Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {":content-length", "32770"}}; // 32KB + 2 bytes - stream->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream->encodeHeaders(response_headers, false).ok()); std::string response(32 * 1024 + 1, 'a'); Buffer::OwnedImpl buffer(response); stream->encodeData(buffer, true); @@ -902,7 +902,7 @@ TEST_P(EnvoyQuicServerSessionTest, SendBufferWatermark) { Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {":content-length", "32770"}}; // 32KB + 2 bytes - stream1->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream1->encodeHeaders(response_headers, false).ok()); std::string response(32 * 1024 + 1, 'a'); Buffer::OwnedImpl buffer(response); EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); @@ -930,7 +930,7 @@ TEST_P(EnvoyQuicServerSessionTest, SendBufferWatermark) { })); stream2->OnStreamHeaderList(/*fin=*/true, request_headers.uncompressed_header_bytes(), request_headers); - stream2->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream2->encodeHeaders(response_headers, false).ok()); // This response will trigger both stream and connection's send buffer watermark upper limits. Buffer::OwnedImpl buffer2(response); EXPECT_CALL(network_connection_callbacks_, onAboveWriteBufferHighWatermark) @@ -1089,7 +1089,7 @@ TEST_P(EnvoyQuicServerSessionTest, HeadersContributeToWatermarkGquic) { Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; // Make connection congestion control blocked so headers are buffered. EXPECT_CALL(*send_algorithm, CanSend(_)).WillRepeatedly(Return(false)); - stream1->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream1->encodeHeaders(response_headers, false).ok()); // Buffer a response slightly smaller than connection level watermark, but // with the previously buffered headers, this write should reach high // watermark. @@ -1197,7 +1197,7 @@ TEST_P(EnvoyQuicServerSessionTest, OnCanWriteUpdateWatermarkGquic) { request_headers); Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - stream1->encodeHeaders(response_headers, false); + EXPECT_TRUE(stream1->encodeHeaders(response_headers, false).ok()); // Make connection congestion control blocked. EXPECT_CALL(*send_algorithm, CanSend(_)).WillRepeatedly(Return(false)); // Buffer a response slightly smaller than connection level watermark, but diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index 3ec95ee9e2c67..7b0b5a41a253d 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -229,13 +229,13 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { request_headers); } EXPECT_TRUE(quic_stream_->FinishedReadingHeaders()); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true).ok()); } TEST_P(EnvoyQuicServerStreamTest, PostRequestAndResponse) { EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); receiveRequest(request_body_, true, request_body_.size() * 2); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false).ok()); quic_stream_->encodeTrailers(response_trailers_); } @@ -309,7 +309,7 @@ TEST_P(EnvoyQuicServerStreamTest, ResetStreamByHCM) { TEST_P(EnvoyQuicServerStreamTest, EarlyResponseWithStopSending) { receiveRequest(request_body_, false, request_body_.size() * 2); // Write response headers with FIN before finish receiving request. - quic_stream_->encodeHeaders(response_headers_, true); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, true).ok()); // Resetting the stream now means stop reading and sending QUIC_STREAM_NO_ERROR or STOP_SENDING. if (quic::VersionUsesHttp3(quic_version_.transport_version)) { EXPECT_CALL(quic_session_, MaybeSendStopSendingFrame(_, _)); @@ -412,7 +412,7 @@ TEST_P(EnvoyQuicServerStreamTest, WatermarkSendBuffer) { // 32KB + 2 byte. The initial stream flow control window is 16k. response_headers_.addCopy(":content-length", "32770"); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false).ok()); // Encode 32kB response body. first 16KB should be written out right away. The // rest should be buffered. The high watermark is 16KB, so this call should @@ -479,7 +479,7 @@ TEST_P(EnvoyQuicServerStreamTest, HeadersContributeToWatermarkIquic) { quic::StreamSendingState state, bool, absl::optional) { return quic::QuicConsumedData{0u, state != quic::NO_FIN}; })); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false).ok()); // Encode 16kB -10 bytes request body. Because the high watermark is 16KB, with previously // buffered headers, this call should make the send buffers reach their high watermark. @@ -603,7 +603,7 @@ TEST_P(EnvoyQuicServerStreamTest, RequestTrailerTooLarge) { // watermark buffer accounting. TEST_P(EnvoyQuicServerStreamTest, ConnectionCloseDuringEncoding) { receiveRequest(request_body_, true, request_body_.size() * 2); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false).ok()); std::string response(16 * 1024 + 1, 'a'); EXPECT_CALL(quic_session_, WritevData(_, _, _, _, _, _)) .Times(testing::AtLeast(1u)) @@ -655,7 +655,7 @@ TEST_P(EnvoyQuicServerStreamTest, ConnectionCloseAfterEndStreamEncoded) { return quic::QuicConsumedData{0, false}; })); EXPECT_CALL(quic_session_, MaybeSendRstStreamFrame(_, _, _)); - quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true); + EXPECT_TRUE(quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/true).ok()); } } // namespace Quic diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 8980e7cf7a7ae..1deed8a11a41c 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -169,7 +169,7 @@ TEST_F(RouterTest, MissingRequiredHeaders) { EXPECT_CALL(encoder, encodeHeaders(_, _)) .WillOnce(Invoke([](const Http::RequestHeaderMap& headers, bool) -> Http::Status { - return Http::HeaderUtility::checkRequiredHeaders(headers); + return Http::HeaderUtility::checkRequiredRequestHeaders(headers); })); EXPECT_CALL(callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, diff --git a/test/integration/BUILD b/test/integration/BUILD index 97e39f4d156cf..829ad502cad11 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -453,6 +453,7 @@ envoy_cc_test_library( "//test/integration/filters:local_reply_during_encoding_data_filter_lib", "//test/integration/filters:local_reply_during_encoding_filter_lib", "//test/integration/filters:random_pause_filter_lib", + "//test/integration/filters:remove_response_headers_lib", "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 7c47e739c3c07..ee8eed8996eb9 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -83,7 +83,7 @@ void FakeStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers return; } } - encoder_.encode100ContinueHeaders(*headers_copy); + (void)encoder_.encode100ContinueHeaders(*headers_copy); }); } @@ -103,7 +103,20 @@ void FakeStream::encodeHeaders(const Http::HeaderMap& headers, bool end_stream) return; } } - encoder_.encodeHeaders(*headers_copy, end_stream); + const auto status = encoder_.encodeHeaders(*headers_copy, end_stream); + // simulate the connection manager's behavior to handle encoding failures. + if (!status.ok()) { + bool is_grpc = false; + { + // To avoid deadlock. + absl::MutexLock lock(&lock_); + is_grpc = Grpc::Common::isGrpcRequestHeaders(*headers_); + } + sendLocalReply( + is_grpc, Http::Code::BadGateway, status.message(), nullptr, absl::nullopt, + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredResponseHeaders, + "{", status.message(), "}")); + }; }); } diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index ce92bcb06f5ee..d9f93fd7de420 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -114,7 +114,7 @@ class FakeStream : public Http::RequestDecoder, Http::Utility::EncodeFunctions( {nullptr, nullptr, [&](Http::ResponseHeaderMapPtr&& headers, bool end_stream) -> void { - encoder_.encodeHeaders(*headers, end_stream); + (void)encoder_.encodeHeaders(*headers, end_stream); }, [&](Buffer::Instance& data, bool end_stream) -> void { encoder_.encodeData(data, end_stream); diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 6ab4dbab5e54d..f5c6ccd139c56 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -447,3 +447,18 @@ envoy_cc_test_library( "//test/extensions/filters/http/common:empty_http_filter_config_lib", ], ) + +envoy_cc_test_library( + name = "remove_response_headers_lib", + srcs = [ + "remove_response_headers.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) diff --git a/test/integration/filters/remove_response_headers.cc b/test/integration/filters/remove_response_headers.cc new file mode 100644 index 0000000000000..20043cf7fd4b2 --- /dev/null +++ b/test/integration/filters/remove_response_headers.cc @@ -0,0 +1,33 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "test/integration/filters/common.h" + +namespace Envoy { + +// Registers the misbehaving filter which removes all response headers. +class RemoveResponseHeadersFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "remove-response-headers-filter"; + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override { + std::vector keys; + headers.iterate([&keys](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + keys.push_back(std::string(header.key().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); + for (auto& k : keys) { + const Http::LowerCaseString lower_key{k}; + headers.remove(lower_key); + } + return Http::FilterHeadersStatus::Continue; + } +}; + +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 36e048b10bc37..3145482a4e266 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1262,7 +1262,6 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) { } TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { - useAccessLog("%RESPONSE_CODE_DETAILS%"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -1281,12 +1280,10 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { default_response_headers_.setStatus( "11111111111111111111111111111111111111111111111111111111111111111"); upstream_request_->encodeHeaders(default_response_headers_, false); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } ASSERT_TRUE(response->waitForEndStream()); EXPECT_EQ("502", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("protocol_error")); } TEST_P(ProtocolIntegrationTest, MissingStatus) { @@ -1321,7 +1318,6 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) { waitForNextUpstreamRequest(); default_response_headers_.removeStatus(); upstream_request_->encodeHeaders(default_response_headers_, false); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } ASSERT_TRUE(response->waitForEndStream()); @@ -2326,4 +2322,44 @@ TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnectLegacy) { } } +static std::string remove_response_headers_filter = R"EOF( +name: remove-response-headers-filter +typed_config: + "@type": type.googleapis.com/google.protobuf.Empty +)EOF"; + +TEST_P(ProtocolIntegrationTest, HeadersOnlyRequestWithRemoveResponseHeadersFilter) { + config_helper_.addFilter(remove_response_headers_filter); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + response->waitForEndStream(); + // If a filter chain removes :status from the response headers, then Envoy must reply with + // InternalServerError and must not crash. + ASSERT_TRUE(codec_client_->connected()); + EXPECT_EQ("502", response->headers().getStatusValue()); + EXPECT_THAT(response->body(), HasSubstr("missing required header: :status")); +} + +TEST_P(ProtocolIntegrationTest, RemoveResponseHeadersFilter) { + config_helper_.addFilter(remove_response_headers_filter); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeRequestWithBody(default_request_headers_, 10); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + response->waitForEndStream(); + // If a filter chain removes :status from the response headers, then Envoy must reply with + // InternalServerError and not crash. + ASSERT_TRUE(codec_client_->connected()); + EXPECT_EQ("502", response->headers().getStatusValue()); + EXPECT_THAT(response->body(), HasSubstr("missing required header: :status")); +} + } // namespace Envoy diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 56e7f3ff80099..63fd3aff76260 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -86,6 +86,7 @@ class MockFilterManagerCallbacks : public FilterManagerCallbacks { MOCK_METHOD(ResponseHeaderMapOptRef, responseHeaders, ()); MOCK_METHOD(ResponseTrailerMapOptRef, responseTrailers, ()); MOCK_METHOD(void, endStream, ()); + MOCK_METHOD(bool, streamEnded, ()); MOCK_METHOD(void, onDecoderFilterBelowWriteBufferLowWatermark, ()); MOCK_METHOD(void, onDecoderFilterAboveWriteBufferHighWatermark, ()); MOCK_METHOD(void, upgradeFilterChainCreated, ()); diff --git a/test/mocks/http/stream_encoder.cc b/test/mocks/http/stream_encoder.cc index 5d872c64db15f..cc6ae50fca8fd 100644 --- a/test/mocks/http/stream_encoder.cc +++ b/test/mocks/http/stream_encoder.cc @@ -1,6 +1,7 @@ #include "test/mocks/http/stream_encoder.h" #include "common/http/header_utility.h" +#include "common/http/status.h" using testing::_; using testing::Invoke; @@ -17,7 +18,7 @@ MockRequestEncoder::MockRequestEncoder() { .WillByDefault(Invoke([](const RequestHeaderMap& headers, bool) -> Status { // Check to see that method is not-null. Path can be null for CONNECT and authority can be // null at the codec level. - ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); + ASSERT(HeaderUtility::checkRequiredRequestHeaders(headers).ok()); return okStatus(); })); } @@ -26,9 +27,10 @@ MockRequestEncoder::~MockRequestEncoder() = default; MockResponseEncoder::MockResponseEncoder() { ON_CALL(*this, getStream()).WillByDefault(ReturnRef(stream_)); ON_CALL(*this, encodeHeaders(_, _)) - .WillByDefault(Invoke([](const ResponseHeaderMap& headers, bool) { + .WillByDefault(Invoke([](const ResponseHeaderMap& headers, bool) -> Status { // Check for passing request headers as response headers in a test. - ASSERT_NE(nullptr, headers.Status()); + ASSERT(HeaderUtility::checkRequiredResponseHeaders(headers).ok()); + return Http::okStatus(); })); } MockResponseEncoder::~MockResponseEncoder() = default; diff --git a/test/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index 3277fe1d8a809..d22e10c76e553 100644 --- a/test/mocks/http/stream_encoder.h +++ b/test/mocks/http/stream_encoder.h @@ -45,8 +45,8 @@ class MockResponseEncoder : public ResponseEncoder { ~MockResponseEncoder() override; // Http::ResponseEncoder - MOCK_METHOD(void, encode100ContinueHeaders, (const ResponseHeaderMap& headers)); - MOCK_METHOD(void, encodeHeaders, (const ResponseHeaderMap& headers, bool end_stream)); + MOCK_METHOD(Http::Status, encode100ContinueHeaders, (const ResponseHeaderMap& headers)); + MOCK_METHOD(Http::Status, encodeHeaders, (const ResponseHeaderMap& headers, bool end_stream)); MOCK_METHOD(void, encodeTrailers, (const ResponseTrailerMap& trailers)); // Http::StreamEncoder