From 4d5f6f109e4ed0cb7e44a80fffd2cf67e300988b Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 9 Oct 2020 12:57:05 -0400 Subject: [PATCH 01/20] draft Signed-off-by: Asra Ali --- source/common/http/http1/codec_impl.cc | 14 +++-- source/common/router/BUILD | 1 + source/common/router/router.cc | 34 +++++++++++-- source/common/router/router.h | 7 +++ test/common/http/http2/http2_frame.cc | 20 ++++++++ test/common/http/http2/http2_frame.h | 3 ++ test/integration/BUILD | 3 ++ test/integration/filters/BUILD | 16 ++++++ .../filters/invalid_header_filter.cc | 42 +++++++++++++++ test/integration/http2_integration_test.cc | 27 ++++++++++ test/integration/protocol_integration_test.cc | 51 +++++++++++++++++++ 11 files changed, 206 insertions(+), 12 deletions(-) create mode 100644 test/integration/filters/invalid_header_filter.cc diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 382478f27f20f..ba906927889f2 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -378,13 +378,11 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end const HeaderEntry* host = headers.Host(); bool is_connect = HeaderUtility::isConnect(headers); - // TODO(#10878): Include missing host header for CONNECT. - // The RELEASE_ASSERT below does not change the existing behavior of `encodeHeaders`. - // The `encodeHeaders` used to throw on errors. Callers of `encodeHeaders()` do not catch - // exceptions and this would cause abnormal process termination in error cases. This change - // replaces abnormal process termination from unhandled exception with the RELEASE_ASSERT. Further - // work will replace this RELEASE_ASSERT with proper error handling. - RELEASE_ASSERT(method && (path || is_connect), ":method and :path must be specified"); + // Headers must be present. This would only happen from downstream traffic (in which case + // downstream codecs would reject) or a faulty filter (in which case the router would reject). + ASSERT(method, ":method must be specified."); + ASSERT(path || is_connect, "path must be specified"); + ASSERT(host || !is_connect, "host must be specified for a CONNECT request"); if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; @@ -398,7 +396,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); - if (is_connect) { + if (is_connect) { // Maybe add && host? connection_.copyToBuffer(host->value().getStringView().data(), host->value().size()); } else { connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); diff --git a/source/common/router/BUILD b/source/common/router/BUILD index d0d4294d44671..762e912036a88 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -305,6 +305,7 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", + "//source/common/http:status_lib", "//source/common/http:utility_lib", "//source/common/network:application_protocol_lib", "//source/common/network:transport_socket_options_lib", diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 5e6702c3cb750..943d864f72e1e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -263,6 +263,23 @@ FilterUtility::StrictHeaderChecker::checkHeader(Http::RequestHeaderMap& headers, NOT_REACHED_GCOVR_EXCL_LINE; } +Http::Status FilterUtility::checkHeaderMap(const Http::RequestHeaderMap& headers) { + if (!headers.Method()) { + // Must have a method header. + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get()); + } + bool is_connect = Http::HeaderUtility::isConnect(headers); + if (!headers.Path() && !is_connect) { + // Must have a path header unless it is a CONNECT request. + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get()); + } + if (!headers.Host() && is_connect) { + // Must have a host header with a CONNECT request. + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get()); + } + return Http::okStatus(); +} + Stats::StatName Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) { return upstream_host ? upstream_host->localityZoneStatName() : config_.empty_stat_name_; } @@ -329,10 +346,19 @@ void Filter::chargeUpstreamCode(Http::Code code, } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { - // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers. - // These get stripped by HTTP/1 codec where applicable. - ASSERT(headers.Method()); - ASSERT(headers.Host()); + // Do a common header check for minimum constraints. We make sure that all outgoing requests have + // all HTTP/2 headers. + const auto header_status = FilterUtility::checkHeaderMap(headers); + if (!header_status.ok()) { + callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); + ENVOY_LOG_MISC(info, "HEADER STATUS MESSAGE {}", header_status.message()); + const std::string body = fmt::format("missing required header: {}", header_status.message()); + const std::string details = absl::StrCat(StreamInfo::ResponseCodeDetails::get().DirectResponse, + "{", header_status.message(), "}"); + callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, + details); + return Http::FilterHeadersStatus::StopIteration; + } downstream_headers_ = &headers; diff --git a/source/common/router/router.h b/source/common/router/router.h index e1be0571e2b65..b66475107a5e8 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -27,6 +27,7 @@ #include "common/common/linked_object.h" #include "common/common/logger.h" #include "common/config/well_known_names.h" +#include "common/http/status.h" #include "common/http/utility.h" #include "common/router/config_impl.h" #include "common/router/upstream_request.h" @@ -121,6 +122,12 @@ class FilterUtility { } }; + /* Does a common header check ensuring required headers are present before request headers are + * forwarded. This will only fail if configured filters erroneously removes required headers. + * @return Status containing the result along with a message if false. + */ + static Http::Status checkHeaderMap(const Http::RequestHeaderMap& headers); + /** * Returns response_time / timeout, as a percentage as [0, 100]. Returns 0 * if there is no timeout. diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index b29956144b4bd..2a63653065345 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -287,6 +287,26 @@ Http2Frame Http2Frame::makeMalformedRequestWithZerolenHeader(uint32_t stream_ind return frame; } +Http2Frame Http2Frame::makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method, + absl::string_view host, + absl::string_view path) { + Http2Frame frame; + frame.buildHeader(Type::Headers, 0, orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders), + makeNetworkOrderStreamId(stream_index)); + if (method) { + frame.appendStaticHeader(StaticHeaderIndex::MethodGet); + } + frame.appendStaticHeader(StaticHeaderIndex::SchemeHttps); + if (!path.empty()) { + frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Path, path); + } + if (!host.empty()) { + frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Host, host); + } + frame.adjustPayloadSize(); + return frame; +} + Http2Frame Http2Frame::makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path) { Http2Frame frame; diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index fc585815d8503..2e0d217108ad4 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -132,6 +132,9 @@ class Http2Frame { static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index, absl::string_view host, absl::string_view path); + static Http2Frame makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method, + absl::string_view host, + absl::string_view path); static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path); static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host, diff --git a/test/integration/BUILD b/test/integration/BUILD index 14963546ec548..6cb7b7db1aad9 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -327,6 +327,7 @@ envoy_cc_test( "//source/extensions/filters/http/buffer:config", "//source/extensions/filters/http/health_check:config", "//test/common/http/http2:http2_frame", + "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:metadata_stop_all_filter_config_lib", "//test/integration/filters:request_metadata_filter_config_lib", "//test/integration/filters:response_metadata_filter_config_lib", @@ -420,6 +421,7 @@ envoy_cc_test( "//source/extensions/filters/http/health_check:config", "//test/integration/filters:continue_headers_only_inject_body", "//test/integration/filters:encoder_decoder_buffer_filter_lib", + "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:random_pause_filter_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", @@ -860,6 +862,7 @@ envoy_cc_test( "//source/extensions/filters/http/health_check:config", "//test/integration/filters:clear_route_cache_filter_lib", "//test/integration/filters:encoder_decoder_buffer_filter_lib", + "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:process_context_lib", "//test/integration/filters:stop_iteration_and_continue", "//test/mocks/http:http_mocks", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 1746a3952626e..a8b22e88c212e 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -373,3 +373,19 @@ envoy_cc_test_library( "@envoy_api//envoy/extensions/network/socket_interface/v3:pkg_cc_proto", ], ) + +envoy_cc_test_library( + name = "invalid_header_filter_lib", + srcs = [ + "invalid_header_filter.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/common/http:header_utility_lib", + "//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/invalid_header_filter.cc b/test/integration/filters/invalid_header_filter.cc new file mode 100644 index 0000000000000..2b470ab8db3d4 --- /dev/null +++ b/test/integration/filters/invalid_header_filter.cc @@ -0,0 +1,42 @@ +#include + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "common/http/header_utility.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 { + +// Faulty filter that may remove critical headers. +class InvalidHeaderFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "invalid-header-filter"; + + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override { + // Remove method when there is a "remove-method" header. + if (headers.get(Http::LowerCaseString("remove-method"))) { + headers.removeMethod(); + } + if (headers.get(Http::LowerCaseString("remove-path"))) { + headers.removePath(); + } + if (Http::HeaderUtility::isConnect(headers)) { + ENVOY_LOG_MISC(info, "REMOVING Host FROM CONNECT"); + headers.removeHost(); + } + return Http::FilterHeadersStatus::Continue; + } +}; + +constexpr char InvalidHeaderFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + decoder_register_; + +} // namespace Envoy diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index fda15444c1934..f4bf0dc484be9 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1612,6 +1612,33 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers")); } +TEST_P(Http2FrameIntegrationTest, MissingHeaders) { + config_helper_.addFilter(R"EOF( + name: invalid-header-filter + typed_config: + "@type": type.googleapis.com/google.protobuf.Empty + )EOF"); + + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_http2_protocol_options() + ->mutable_override_stream_error_on_invalid_http_message() + ->set_value(true); + }); + + beginSession(); + + uint32_t stream_index = 0; + auto request = Http::Http2::Http2Frame::makeMalformedRequestWithMissingHeaders( + Http2Frame::makeClientStreamId(stream_index), false, "host", "/"); + sendFrame(request); + auto response = readFrame(); + // Make sure we've got RST_STREAM from the server + EXPECT_EQ(Http2Frame::Type::RstStream, response.type()); + tcp_client_->close(); +} + INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 381c968c75d27..7f060ed3e3218 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -333,6 +333,57 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) { response->headers().get(Http::LowerCaseString("host"))->value().getStringView()); } +// Tests missing headers needed for H/1 codec first line. +// Missing first line headers can either occur from bad requests, or from a faulty filter. +TEST_P(ProtocolIntegrationTest, Http1DownstreamRequestWithMissingHeaders) { + std::function + sendRequestAndExpectResponse = [this](absl::string_view request, + absl::string_view expected_response, + absl::string_view expected_body) { + std::string response; + sendRawHttpAndWaitForResponse(lookupPort("http"), request.data(), &response, true); + EXPECT_THAT(response, HasSubstr(expected_response)); + if (!expected_body.empty()) { + EXPECT_THAT(response, HasSubstr(expected_body)); + } + }; + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); + config_helper_.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + // Clone the whole listener. + auto static_resources = bootstrap.mutable_static_resources(); + auto* old_listener = static_resources->mutable_listeners(0); + auto* cloned_listener = static_resources->add_listeners(); + cloned_listener->CopyFrom(*old_listener); + old_listener->set_name("http_forward"); + }); + initialize(); + // Faulty downstream traffic. + sendRequestAndExpectResponse("/ HTTP/1.1\r\nHost: host\r\n\r\n", "HTTP/1.1 400 Bad Request\r\n", + ""); + sendRequestAndExpectResponse("GET HTTP/1.1\r\nHost: host\r\n\r\n", + "HTTP/1.1 400 Bad Request\r\n", ""); + sendRequestAndExpectResponse("CONNECT HTTP/1.1\r\nHost: host\r\n\r\n", + "HTTP/1.1 400 Bad Request\r\n", ""); + // Faulty filter code. + sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-method: yes\r\n\r\n", + "HTTP/1.1 503 Service Unavailable\r\n", + "missing required header: :method"); + sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-path: yes\r\n\r\n", + "HTTP/1.1 503 Service Unavailable\r\n", + "missing required header: :path"); + sendRequestAndExpectResponse("CONNECT www.host.com:80 HTTP/1.1\r\n\r\n", + "HTTP/1.1 503 Service Unavailable\r\n", + "missing required header: :authority"); + } +} + // Regression test for https://github.com/envoyproxy/envoy/issues/10270 TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that From bae336cd8d8b7a5ae9178f95c8d1dd4fef0ab812 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 26 Oct 2020 12:52:19 -0400 Subject: [PATCH 02/20] include tests for h/2 and add response code Signed-off-by: Asra Ali --- .../http_conn_man/response_code_details.rst | 1 + docs/root/version_history/current.rst | 1 + include/envoy/stream_info/stream_info.h | 2 + source/common/http/http1/codec_impl.cc | 7 +- source/common/router/router.cc | 16 +-- source/common/router/router.h | 5 +- test/common/http/http2/http2_frame.cc | 20 ---- test/common/http/http2/http2_frame.h | 3 - test/integration/BUILD | 2 - .../filters/invalid_header_filter.cc | 4 +- test/integration/protocol_integration_test.cc | 99 ++++++++++--------- 11 files changed, 74 insertions(+), 86 deletions(-) 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 350c0767f93fc..d110d3ec9280d 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 @@ -30,6 +30,7 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen 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. max_duration_timeout, The per-stream max duration timeout was exceeded. + missing_headers_after_filter_chain, The request was rejected by the router filter because configured filters removed required headers. missing_host_header, The request was rejected due to a missing Host: or :authority field. missing_path_rejected, The request was rejected due to a missing Path or :path header field. no_healthy_upstream, The request was rejected by the router filter because there was no healthy upstream found. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a7405c8f72a90..5b6411b691b82 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -19,6 +19,7 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* http: reject requests with missing required headers after filter chain processing. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. Removed Config or Runtime diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 858ff0f0952f3..03c1ca029e851 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -172,6 +172,8 @@ struct ResponseCodeDetailValues { const std::string AdminFilterResponse = "admin_filter_response"; // 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 MissingHeadersAfterFilterChain = "missing_headers_after_filter_chain"; // Changes or additions to details should be reflected in // docs/root/configuration/http/http_conn_man/response_code_details_details.rst }; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 08d49f0721d74..aaa3f19862e5c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -372,8 +372,9 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end const HeaderEntry* host = headers.Host(); bool is_connect = HeaderUtility::isConnect(headers); - // Headers must be present. This would only happen from downstream traffic (in which case - // downstream codecs would reject) or a faulty filter (in which case the router would reject). + // Headers must be present. If downstream traffic is missing headers, downstream codecs will + // reject the request. If a faulty filter removes these headers, the router will reject before + // forwarding the request headers here. ASSERT(method, ":method must be specified."); ASSERT(path || is_connect, "path must be specified"); ASSERT(host || !is_connect, "host must be specified for a CONNECT request"); @@ -390,7 +391,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); - if (is_connect) { // Maybe add && host? + if (is_connect) { connection_.copyToBuffer(host->value().getStringView().data(), host->value().size()); } else { connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4a5996b4661a4..82ab9ccc2fe37 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -265,16 +265,15 @@ FilterUtility::StrictHeaderChecker::checkHeader(Http::RequestHeaderMap& headers, Http::Status FilterUtility::checkHeaderMap(const Http::RequestHeaderMap& headers) { if (!headers.Method()) { - // Must have a method header. return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get()); } bool is_connect = Http::HeaderUtility::isConnect(headers); if (!headers.Path() && !is_connect) { - // Must have a path header unless it is a CONNECT request. + // :path header must be present for non-CONNECT requests. return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get()); } if (!headers.Host() && is_connect) { - // Must have a host header with a CONNECT request. + // Host header must be present for CONNECT request. return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get()); } return Http::okStatus(); @@ -346,15 +345,16 @@ void Filter::chargeUpstreamCode(Http::Code code, } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { - // Do a common header check for minimum constraints. We make sure that all outgoing requests have - // all HTTP/2 headers. + // Do a common header check ensuring required headers are present before forwarding. These mimic + // checks done in the HTTP Connection Manager before filter processing and ensure that filters did + // not erroneously remove required headers. const auto header_status = FilterUtility::checkHeaderMap(headers); if (!header_status.ok()) { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - ENVOY_LOG_MISC(info, "HEADER STATUS MESSAGE {}", header_status.message()); const std::string body = fmt::format("missing required header: {}", header_status.message()); - const std::string details = absl::StrCat(StreamInfo::ResponseCodeDetails::get().DirectResponse, - "{", header_status.message(), "}"); + const std::string details = + absl::StrCat(StreamInfo::ResponseCodeDetails::get().MissingHeadersAfterFilterChain, "{", + header_status.message(), "}"); callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); return Http::FilterHeadersStatus::StopIteration; diff --git a/source/common/router/router.h b/source/common/router/router.h index ecd91ec3c736e..dd58b8fca7314 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -124,7 +124,10 @@ class FilterUtility { /* Does a common header check ensuring required headers are present before request headers are * forwarded. This will only fail if configured filters erroneously removes required headers. - * @return Status containing the result along with a message if false. + * Required request headers include :method header, :path for non-CONNECT requests, and + * host/authority for CONNECT requests. + * @return Status containing the result. If failed, message includes details on which header was + * missing. */ static Http::Status checkHeaderMap(const Http::RequestHeaderMap& headers); diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index e099e88e57e4f..4d8a8c85cc158 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -287,26 +287,6 @@ Http2Frame Http2Frame::makeMalformedRequestWithZerolenHeader(uint32_t stream_ind return frame; } -Http2Frame Http2Frame::makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method, - absl::string_view host, - absl::string_view path) { - Http2Frame frame; - frame.buildHeader(Type::Headers, 0, orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders), - makeNetworkOrderStreamId(stream_index)); - if (method) { - frame.appendStaticHeader(StaticHeaderIndex::MethodGet); - } - frame.appendStaticHeader(StaticHeaderIndex::SchemeHttps); - if (!path.empty()) { - frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Path, path); - } - if (!host.empty()) { - frame.appendHeaderWithoutIndexing(StaticHeaderIndex::Host, host); - } - frame.adjustPayloadSize(); - return frame; -} - Http2Frame Http2Frame::makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path) { Http2Frame frame; diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index 889f688d7f9cf..d9a32fab64f21 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -132,9 +132,6 @@ class Http2Frame { static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index, absl::string_view host, absl::string_view path); - static Http2Frame makeMalformedRequestWithMissingHeaders(uint32_t stream_index, bool method, - absl::string_view host, - absl::string_view path); static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path); static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host, diff --git a/test/integration/BUILD b/test/integration/BUILD index ac1b0160ad53b..b6869343107de 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -350,8 +350,6 @@ envoy_cc_test( "//source/common/http:header_map_lib", "//source/extensions/filters/http/buffer:config", "//source/extensions/filters/http/health_check:config", - "//test/common/http/http2:http2_frame", - "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:metadata_stop_all_filter_config_lib", "//test/integration/filters:request_metadata_filter_config_lib", "//test/integration/filters:response_metadata_filter_config_lib", diff --git a/test/integration/filters/invalid_header_filter.cc b/test/integration/filters/invalid_header_filter.cc index 2b470ab8db3d4..dc0a1f595724b 100644 --- a/test/integration/filters/invalid_header_filter.cc +++ b/test/integration/filters/invalid_header_filter.cc @@ -20,10 +20,10 @@ class InvalidHeaderFilter : public Http::PassThroughFilter { Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override { // Remove method when there is a "remove-method" header. - if (headers.get(Http::LowerCaseString("remove-method"))) { + if (!headers.get(Http::LowerCaseString("remove-method")).empty()) { headers.removeMethod(); } - if (headers.get(Http::LowerCaseString("remove-path"))) { + if (!headers.get(Http::LowerCaseString("remove-path")).empty()) { headers.removePath(); } if (Http::HeaderUtility::isConnect(headers)) { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 54a2ac312fc47..d73da239dd4eb 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -334,54 +334,59 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) { } // Tests missing headers needed for H/1 codec first line. -// Missing first line headers can either occur from bad requests, or from a faulty filter. -TEST_P(ProtocolIntegrationTest, Http1DownstreamRequestWithMissingHeaders) { - std::function - sendRequestAndExpectResponse = [this](absl::string_view request, - absl::string_view expected_response, - absl::string_view expected_body) { - std::string response; - sendRawHttpAndWaitForResponse(lookupPort("http"), request.data(), &response, true); - EXPECT_THAT(response, HasSubstr(expected_response)); - if (!expected_body.empty()) { - EXPECT_THAT(response, HasSubstr(expected_body)); - } - }; +TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); + config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + // Clone the whole listener. + auto static_resources = bootstrap.mutable_static_resources(); + auto* old_listener = static_resources->mutable_listeners(0); + auto* cloned_listener = static_resources->add_listeners(); + cloned_listener->CopyFrom(*old_listener); + old_listener->set_name("http_forward"); + }); + initialize(); - if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { - config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " - "type.googleapis.com/google.protobuf.Empty } }"); - config_helper_.addConfigModifier( - [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); - config_helper_.addConfigModifier( - [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { - // Clone the whole listener. - auto static_resources = bootstrap.mutable_static_resources(); - auto* old_listener = static_resources->mutable_listeners(0); - auto* cloned_listener = static_resources->add_listeners(); - cloned_listener->CopyFrom(*old_listener); - old_listener->set_name("http_forward"); - }); - initialize(); - // Faulty downstream traffic. - sendRequestAndExpectResponse("/ HTTP/1.1\r\nHost: host\r\n\r\n", "HTTP/1.1 400 Bad Request\r\n", - ""); - sendRequestAndExpectResponse("GET HTTP/1.1\r\nHost: host\r\n\r\n", - "HTTP/1.1 400 Bad Request\r\n", ""); - sendRequestAndExpectResponse("CONNECT HTTP/1.1\r\nHost: host\r\n\r\n", - "HTTP/1.1 400 Bad Request\r\n", ""); - // Faulty filter code. - sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-method: yes\r\n\r\n", - "HTTP/1.1 503 Service Unavailable\r\n", - "missing required header: :method"); - sendRequestAndExpectResponse("GET / HTTP/1.1\r\nHost: host\r\nremove-path: yes\r\n\r\n", - "HTTP/1.1 503 Service Unavailable\r\n", - "missing required header: :path"); - sendRequestAndExpectResponse("CONNECT www.host.com:80 HTTP/1.1\r\n\r\n", - "HTTP/1.1 503 Service Unavailable\r\n", - "missing required header: :authority"); - } + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Missing method + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"remove-method", "yes"}}); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); + + // Missing path for non-CONNECT + response = + codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"remove-path", "yes"}}); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); + + // Missing host for CONNECT + response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, + {":path", "/test/url"}, + {":scheme", "http"}, + {":authority", "www.host.com:80"}}); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("503", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); } // Regression test for https://github.com/envoyproxy/envoy/issues/10270 From e6e06022e77649d8076964a75744b65c1a44d6ff Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 26 Oct 2020 13:00:03 -0400 Subject: [PATCH 03/20] change response detail name Signed-off-by: Asra Ali --- .../http_conn_man/response_code_details.rst | 2 +- include/envoy/stream_info/stream_info.h | 2 +- source/common/router/router.cc | 2 +- test/integration/http2_integration_test.cc | 27 ------------------- test/integration/protocol_integration_test.cc | 6 ++--- 5 files changed, 6 insertions(+), 33 deletions(-) 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 d110d3ec9280d..db396564906c5 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,11 +26,11 @@ 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 by the router filter because configured filters removed required headers. 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. max_duration_timeout, The per-stream max duration timeout was exceeded. - missing_headers_after_filter_chain, The request was rejected by the router filter because configured filters removed required headers. missing_host_header, The request was rejected due to a missing Host: or :authority field. missing_path_rejected, The request was rejected due to a missing Path or :path header field. no_healthy_upstream, The request was rejected by the router filter because there was no healthy upstream found. diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 03c1ca029e851..8210534653676 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -173,7 +173,7 @@ 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 MissingHeadersAfterFilterChain = "missing_headers_after_filter_chain"; + const std::string FilterRemovedRequiredHeaders = "filter_removed_required_headers"; // Changes or additions to details should be reflected in // docs/root/configuration/http/http_conn_man/response_code_details_details.rst }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 82ab9ccc2fe37..9abbaedf3aeb5 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -353,7 +353,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); const std::string body = fmt::format("missing required header: {}", header_status.message()); const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().MissingHeadersAfterFilterChain, "{", + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", header_status.message(), "}"); callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 2ecfc330dd0eb..1ad689a4849e4 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1561,33 +1561,6 @@ TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers")); } -TEST_P(Http2FrameIntegrationTest, MissingHeaders) { - config_helper_.addFilter(R"EOF( - name: invalid-header-filter - typed_config: - "@type": type.googleapis.com/google.protobuf.Empty - )EOF"); - - config_helper_.addConfigModifier( - [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { - hcm.mutable_http2_protocol_options() - ->mutable_override_stream_error_on_invalid_http_message() - ->set_value(true); - }); - - beginSession(); - - uint32_t stream_index = 0; - auto request = Http::Http2::Http2Frame::makeMalformedRequestWithMissingHeaders( - Http2Frame::makeClientStreamId(stream_index), false, "host", "/"); - sendFrame(request); - auto response = readFrame(); - // Make sure we've got RST_STREAM from the server - EXPECT_EQ(Http2Frame::Type::RstStream, response.type()); - tcp_client_->close(); -} - INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d73da239dd4eb..f811fe97de60d 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -363,7 +363,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); // Missing path for non-CONNECT response = @@ -375,7 +375,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); // Missing host for CONNECT response = codec_client_->makeHeaderOnlyRequest( @@ -386,7 +386,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_headers_after_filter_chain")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); } // Regression test for https://github.com/envoyproxy/envoy/issues/10270 From 11973d516dfe8371cefefd2b37f352f9e3cf51f6 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 28 Oct 2020 14:22:08 -0400 Subject: [PATCH 04/20] move to per iteration, in header utility Signed-off-by: Asra Ali --- source/common/http/BUILD | 1 + source/common/http/filter_manager.cc | 26 ++++++++++++++++ source/common/http/filter_manager.h | 9 +----- source/common/http/header_utility.cc | 19 ++++++++++++ source/common/http/header_utility.h | 9 ++++++ source/common/http/utility.h | 1 + source/common/router/router.cc | 31 ------------------- source/common/router/router.h | 10 ------ .../filters/invalid_header_filter.cc | 1 - test/integration/protocol_integration_test.cc | 16 +++++----- 10 files changed, 65 insertions(+), 58 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 726c333226142..e5a2d719d3692 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -405,6 +405,7 @@ envoy_cc_library( ], deps = [ ":header_map_lib", + ":status_lib", ":utility_lib", "//include/envoy/common:regex_interface", "//include/envoy/http:header_map_interface", diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index e866101e4a26d..c8db94ad99492 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -371,6 +371,32 @@ void ActiveStreamDecoderFilter::onDecoderFilterAboveWriteBufferHighWatermark() { parent_.filter_manager_callbacks_.onDecoderFilterAboveWriteBufferHighWatermark(); } +FilterHeadersStatus ActiveStreamDecoderFilter::decodeHeaders(RequestHeaderMap& headers, + bool end_stream) { + // Each decoder filter instance checks if the request passed to the filter is gRPC + // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() + // called here may change the content type, so we must check it before the call. + + is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); + FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); + + // Validate filters did not erroneously remove required headers. If they do, send a direct + // response. + const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); + if (!header_status.ok()) { + ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, + static_cast(this)); + parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); + const std::string body = fmt::format("missing required header: {}", header_status.message()); + const std::string details = + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", + header_status.message(), "}"); + sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); + return FilterHeadersStatus::StopIteration; + } + return status; +} + void ActiveStreamDecoderFilter::requestDataTooLarge() { ENVOY_STREAM_LOG(debug, "request data too large watermark exceeded", parent_); if (parent_.state_.decoder_filters_streaming_) { diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 507e3f4a86848..9713a46494944 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -190,14 +190,7 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override; - // Each decoder filter instance checks if the request passed to the filter is gRPC - // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() - // called here may change the content type, so we must check it before the call. - FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream) { - is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); - FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); - return status; - } + FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream); void requestDataTooLarge(); void requestDataDrained(); diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 3e030010de4fa..60c0d34192b5b 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -276,5 +276,24 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol, return false; } +Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) { + if (!headers.Method()) { + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get()); + } + bool is_connect = Http::HeaderUtility::isConnect(headers); + if (is_connect) { + if (!headers.Host()) { + // Host header must be present for CONNECT request. + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get()); + } + } else { + if (!headers.Path()) { + // :path header must be present for non-CONNECT requests. + return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get()); + } + } + return Http::okStatus(); +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 0a1e23c467173..864b1d942ad86 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -8,6 +8,7 @@ #include "envoy/http/protocol.h" #include "envoy/type/v3/range.pb.h" +#include "common/http/status.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -174,6 +175,14 @@ class HeaderUtility { * @brief Remove the port part from host/authority header if it is equal to provided port */ static void stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port); + + /* Does a common header check ensuring required 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); }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d677b097d86cb..b2f076cb9a763 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -555,6 +555,7 @@ struct AuthorityAttributes { * @return hostname parse result. that includes whether host is IP Address, hostname and port-name */ AuthorityAttributes parseAuthority(absl::string_view host); + } // namespace Utility } // namespace Http } // namespace Envoy diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 9abbaedf3aeb5..5ead3ab58acad 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -263,22 +263,6 @@ FilterUtility::StrictHeaderChecker::checkHeader(Http::RequestHeaderMap& headers, NOT_REACHED_GCOVR_EXCL_LINE; } -Http::Status FilterUtility::checkHeaderMap(const Http::RequestHeaderMap& headers) { - if (!headers.Method()) { - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get()); - } - bool is_connect = Http::HeaderUtility::isConnect(headers); - if (!headers.Path() && !is_connect) { - // :path header must be present for non-CONNECT requests. - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get()); - } - if (!headers.Host() && is_connect) { - // Host header must be present for CONNECT request. - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get()); - } - return Http::okStatus(); -} - Stats::StatName Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) { return upstream_host ? upstream_host->localityZoneStatName() : config_.empty_stat_name_; } @@ -345,21 +329,6 @@ void Filter::chargeUpstreamCode(Http::Code code, } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { - // Do a common header check ensuring required headers are present before forwarding. These mimic - // checks done in the HTTP Connection Manager before filter processing and ensure that filters did - // not erroneously remove required headers. - const auto header_status = FilterUtility::checkHeaderMap(headers); - if (!header_status.ok()) { - callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - const std::string body = fmt::format("missing required header: {}", header_status.message()); - const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - header_status.message(), "}"); - callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, - details); - return Http::FilterHeadersStatus::StopIteration; - } - downstream_headers_ = &headers; // Extract debug configuration from filter state. This is used further along to determine whether diff --git a/source/common/router/router.h b/source/common/router/router.h index dd58b8fca7314..f5512919d7daf 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -27,7 +27,6 @@ #include "common/common/linked_object.h" #include "common/common/logger.h" #include "common/config/well_known_names.h" -#include "common/http/status.h" #include "common/http/utility.h" #include "common/router/config_impl.h" #include "common/router/upstream_request.h" @@ -122,15 +121,6 @@ class FilterUtility { } }; - /* Does a common header check ensuring required headers are present before request headers are - * forwarded. This will only fail if configured filters erroneously removes required headers. - * Required request headers include :method header, :path for non-CONNECT requests, and - * host/authority for CONNECT requests. - * @return Status containing the result. If failed, message includes details on which header was - * missing. - */ - static Http::Status checkHeaderMap(const Http::RequestHeaderMap& headers); - /** * Returns response_time / timeout, as a percentage as [0, 100]. Returns 0 * if there is no timeout. diff --git a/test/integration/filters/invalid_header_filter.cc b/test/integration/filters/invalid_header_filter.cc index dc0a1f595724b..8fe9862ff2b60 100644 --- a/test/integration/filters/invalid_header_filter.cc +++ b/test/integration/filters/invalid_header_filter.cc @@ -27,7 +27,6 @@ class InvalidHeaderFilter : public Http::PassThroughFilter { headers.removePath(); } if (Http::HeaderUtility::isConnect(headers)) { - ENVOY_LOG_MISC(info, "REMOVING Host FROM CONNECT"); headers.removeHost(); } return Http::FilterHeadersStatus::Continue; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index f811fe97de60d..9f703535c8ed0 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -356,7 +356,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { // Missing method auto response = codec_client_->makeHeaderOnlyRequest( Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/url"}, + {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}, {"remove-method", "yes"}}); @@ -366,12 +366,12 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); // Missing path for non-CONNECT - response = - codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"remove-path", "yes"}}); + response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"remove-path", "yes"}}); response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); @@ -380,7 +380,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { // Missing host for CONNECT response = codec_client_->makeHeaderOnlyRequest( Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, - {":path", "/test/url"}, + {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "www.host.com:80"}}); response->waitForEndStream(); From 2e24391f9130636a9a4a07d21231eea06ed87a09 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 28 Oct 2020 14:42:33 -0400 Subject: [PATCH 05/20] fix comments Signed-off-by: Asra Ali --- include/envoy/http/codec.h | 3 ++- source/common/http/http1/codec_impl.cc | 10 ++++------ source/common/http/http2/codec_impl.cc | 1 + 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 5e977c8a7d095..5ccb59415aadc 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -105,7 +105,8 @@ class RequestEncoder : public virtual StreamEncoder { public: /** * Encode headers, optionally indicating end of stream. - * @param headers supplies the header map to encode. + * @param headers supplies the header map to encode. headers must have the required HTTP + * headers. * @param end_stream supplies whether this is a header only request. */ virtual void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index aaa3f19862e5c..eb664d2b76c92 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -372,12 +372,10 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end const HeaderEntry* host = headers.Host(); bool is_connect = HeaderUtility::isConnect(headers); - // Headers must be present. If downstream traffic is missing headers, downstream codecs will - // reject the request. If a faulty filter removes these headers, the router will reject before - // forwarding the request headers here. - ASSERT(method, ":method must be specified."); - ASSERT(path || is_connect, "path must be specified"); - ASSERT(host || !is_connect, "host must be specified for a CONNECT request"); + // Required headers must be present. If downstream traffic is missing headers, downstream codecs + // will reject the request. If a faulty filter removes these headers, the filter manager will + // reject before forwarding the request headers here. + ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 4af93895de4a0..ff96155c574a9 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -185,6 +185,7 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { + ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. std::vector final_headers; From ef6cf116bebdfbd9d7bbe15cd3b270489c51aff4 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 28 Oct 2020 14:44:17 -0400 Subject: [PATCH 06/20] remove extra line Signed-off-by: Asra Ali --- source/common/http/utility.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/http/utility.h b/source/common/http/utility.h index b2f076cb9a763..d677b097d86cb 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -555,7 +555,6 @@ struct AuthorityAttributes { * @return hostname parse result. that includes whether host is IP Address, hostname and port-name */ AuthorityAttributes parseAuthority(absl::string_view host); - } // namespace Utility } // namespace Http } // namespace Envoy From 0286898ad7b4e043c4ca1c8fcd813c3b896b7d2c Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 28 Oct 2020 14:45:13 -0400 Subject: [PATCH 07/20] remove extra build line Signed-off-by: Asra Ali --- source/common/router/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 762e912036a88..d0d4294d44671 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -305,7 +305,6 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", - "//source/common/http:status_lib", "//source/common/http:utility_lib", "//source/common/network:application_protocol_lib", "//source/common/network:transport_socket_options_lib", From fdeedbade269d3ac0fedacdd5c500af03dc5f839 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Thu, 29 Oct 2020 11:24:47 -0400 Subject: [PATCH 08/20] fix test and rel note Signed-off-by: Asra Ali --- docs/root/version_history/current.rst | 2 +- test/common/http/http1/codec_impl_test.cc | 16 +++++----------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7f9aec1bd7422..f3de4abc0be93 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,8 +20,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* http: reject requests with missing required headers after filter chain processing. * http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. +* http: reject requests with missing required headers after filter chain processing. * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 200ba24231914..0f4b8a6a8ddea 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2405,18 +2405,12 @@ TEST_P(Http1ClientConnectionImplTest, BadEncodeParams) { NiceMock response_decoder; - // Need to set :method and :path. - // New and legacy codecs will behave differently on errors from processing outbound data. The - // legacy codecs will throw an exception (that presently will be uncaught in contexts like - // sendLocalReply), while the new codecs temporarily RELEASE_ASSERT until Envoy handles errors on - // outgoing data. + // Invalid outbound data errors are impossible to trigger in normal processing, since bad + // downstream data would have been rejected by the codec, and erroneous filter processing would + // cause a direct response by the filter manager. The old codecs will still throw an exception + // (that presently will be uncaught in contexts like sendLocalReply). Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - if (testingNewCodec()) { - EXPECT_DEATH(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}}, true), - ":method and :path must be specified"); - EXPECT_DEATH(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}, true), - ":method and :path must be specified"); - } else { + if (!testingNewCodec()) { EXPECT_THROW(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}}, true), CodecClientException); EXPECT_THROW(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}, true), From 2f7aa2d9c724b040b240cdfa8e07885f1bffcc61 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 30 Oct 2020 15:58:11 -0400 Subject: [PATCH 09/20] change encodeheaders return val Signed-off-by: Asra Ali --- include/envoy/http/codec.h | 2 +- include/envoy/router/router.h | 2 +- source/common/http/codec_wrappers.h | 5 +- source/common/http/filter_manager.cc | 24 +-- source/common/http/header_utility.cc | 9 +- source/common/http/http1/codec_impl.cc | 12 +- source/common/http/http1/codec_impl.h | 2 +- source/common/http/http1/codec_impl_legacy.cc | 3 +- source/common/http/http1/codec_impl_legacy.h | 2 +- source/common/http/http2/codec_impl.cc | 9 +- source/common/http/http2/codec_impl.h | 2 +- source/common/http/http2/codec_impl_legacy.cc | 5 +- source/common/http/http2/codec_impl_legacy.h | 2 +- source/common/router/router.cc | 2 + source/common/router/upstream_request.cc | 12 +- source/common/tcp_proxy/upstream.cc | 4 +- source/common/upstream/health_checker_impl.cc | 8 +- .../upstreams/http/http/upstream_request.h | 5 +- .../upstreams/http/tcp/upstream_request.cc | 4 +- .../upstreams/http/tcp/upstream_request.h | 2 +- test/common/http/codec_impl_fuzz_test.cc | 10 +- test/common/http/codec_wrappers_test.cc | 24 ++- test/common/http/http1/codec_impl_test.cc | 95 +++++---- test/common/http/http1/conn_pool_test.cc | 72 ++++--- test/common/http/http2/codec_impl_test.cc | 178 ++++++++-------- test/common/http/http2/conn_pool_test.cc | 199 +++++++++++------- test/common/http/http2/frame_replay_test.cc | 2 +- .../http/http2/response_header_fuzz_test.cc | 2 +- test/integration/http_integration.cc | 6 +- test/integration/protocol_integration_test.cc | 45 ++-- test/integration/utility.cc | 2 +- test/mocks/http/BUILD | 1 + test/mocks/http/stream_encoder.cc | 7 +- test/mocks/http/stream_encoder.h | 4 +- 34 files changed, 454 insertions(+), 309 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 5ccb59415aadc..7cec31e98aa22 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -109,7 +109,7 @@ class RequestEncoder : public virtual StreamEncoder { * headers. * @param end_stream supplies whether this is a header only request. */ - virtual void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE; + virtual Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE; /** * Encode trailers. This implicitly ends the stream. diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index b4b3be9f3c379..856f0e096315b 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -1275,7 +1275,7 @@ class GenericUpstream { * @param headers supplies the header map to encode. * @param end_stream supplies whether this is a header only request. */ - virtual void encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) PURE; + virtual Http::Status encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) PURE; /** * Encode trailers. This implicitly ends the stream. * @param trailers supplies the trailers to encode. diff --git a/source/common/http/codec_wrappers.h b/source/common/http/codec_wrappers.h index 6a4503e534518..21390cd5647f7 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -68,11 +68,12 @@ class ResponseDecoderWrapper : public ResponseDecoder { class RequestEncoderWrapper : public RequestEncoder { public: // RequestEncoder - void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override { - inner_.encodeHeaders(headers, end_stream); + Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override { + RETURN_IF_ERROR(inner_.encodeHeaders(headers, end_stream)); if (end_stream) { onEncodeComplete(); } + return okStatus(); } void encodeData(Buffer::Instance& data, bool end_stream) override { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index c8db94ad99492..202f8440ed24d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -382,18 +382,18 @@ FilterHeadersStatus ActiveStreamDecoderFilter::decodeHeaders(RequestHeaderMap& h // Validate filters did not erroneously remove required headers. If they do, send a direct // response. - const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); - if (!header_status.ok()) { - ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, - static_cast(this)); - parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - const std::string body = fmt::format("missing required header: {}", header_status.message()); - const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - header_status.message(), "}"); - sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); - return FilterHeadersStatus::StopIteration; - } + // const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); + // if (!header_status.ok()) { + // ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, + // static_cast(this)); + // parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); + // const std::string body = fmt::format("missing required header: {}", header_status.message()); + // const std::string details = + // absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", + // header_status.message(), "}"); + // sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); + // return FilterHeadersStatus::StopIteration; + // } return status; } diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 60c0d34192b5b..c59efda824dbe 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -278,18 +278,21 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol, Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) { if (!headers.Method()) { - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Method.get()); + return absl::InvalidArgumentError( + absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get())); } bool is_connect = Http::HeaderUtility::isConnect(headers); if (is_connect) { if (!headers.Host()) { // Host header must be present for CONNECT request. - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Host.get()); + return absl::InvalidArgumentError( + absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); } } else { if (!headers.Path()) { // :path header must be present for non-CONNECT requests. - return absl::InvalidArgumentError(Envoy::Http::Headers::get().Path.get()); + return absl::InvalidArgumentError( + absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Path.get())); } } return Http::okStatus(); diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index eb664d2b76c92..6a803cca1e6cb 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -366,17 +366,16 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; -void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { +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)); + const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); const HeaderEntry* host = headers.Host(); bool is_connect = HeaderUtility::isConnect(headers); - // Required headers must be present. If downstream traffic is missing headers, downstream codecs - // will reject the request. If a faulty filter removes these headers, the filter manager will - // reject before forwarding the request headers here. - ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); - if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; } else if (method->value() == Headers::get().MethodValues.Connect) { @@ -397,6 +396,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end connection_.copyToBuffer(REQUEST_POSTFIX, sizeof(REQUEST_POSTFIX) - 1); encodeHeadersBase(headers, absl::nullopt, end_stream); + return okStatus(); } int ConnectionImpl::setAndCheckCallbackStatus(Status&& status) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 2ae00fb034009..867cd603f57ad 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -156,7 +156,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { bool connectRequest() const { return connect_request_; } // Http::RequestEncoder - void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; + Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); } private: diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index 332f1241eb7d3..474fd7ce1b0ca 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -365,7 +365,7 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; -void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { +Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); const HeaderEntry* host = headers.Host(); @@ -397,6 +397,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end connection_.copyToBuffer(REQUEST_POSTFIX, sizeof(REQUEST_POSTFIX) - 1); encodeHeadersBase(headers, absl::nullopt, end_stream); + return okStatus(); } http_parser_settings ConnectionImpl::settings_{ diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index 8a1b68b0fad4b..48382473f0bdd 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -160,7 +160,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { bool connectRequest() const { return connect_request_; } // Http::RequestEncoder - void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; + Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; void encodeTrailers(const RequestTrailerMap& trailers) override { encodeTrailersBase(trailers); } private: diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index bb3aea71f674c..b287555378e4a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -183,9 +183,11 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector } } -void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, - bool end_stream) { - ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); +Status ConnectionImpl::ClientStreamImpl::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)); // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. std::vector final_headers; @@ -212,6 +214,7 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea buildHeaders(final_headers, headers); } encodeHeadersBase(final_headers, end_stream); + return okStatus(); } void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers, diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 83f5d504e80bd..7fed071a8063c 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -351,7 +351,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable parent_.checkProtocolConstraintViolation(); } -void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, - bool end_stream) { +Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, + bool end_stream) { // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. std::vector final_headers; @@ -211,6 +211,7 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea buildHeaders(final_headers, headers); } encodeHeadersBase(final_headers, end_stream); + return okStatus(); } void ConnectionImpl::ServerStreamImpl::encodeHeaders(const ResponseHeaderMap& headers, diff --git a/source/common/http/http2/codec_impl_legacy.h b/source/common/http/http2/codec_impl_legacy.h index 238762c45df37..766724a36d53c 100644 --- a/source/common/http/http2/codec_impl_legacy.h +++ b/source/common/http/http2/codec_impl_legacy.h @@ -351,7 +351,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(*this, std::move(generic_conn_pool)); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); + upstream_requests_.front()->encodeHeaders(end_stream); + if (end_stream) { onRequestComplete(); } diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index b906df34fdc9a..f59c403388620 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -416,10 +416,18 @@ void UpstreamRequest::onPoolReady( } } - upstream_->encodeHeaders(*parent_.downstreamHeaders(), shouldSendEndStream()); - + const Http::Status status = + upstream_->encodeHeaders(*parent_.downstreamHeaders(), shouldSendEndStream()); calling_encode_headers_ = false; + if (!status.ok()) { + // It is possible that encodeHeaders() fails. This can happen if filters or other extensions + // erroneously remove required headers. + resetStream(); + onResetStream(Http::StreamResetReason::LocalReset, status.message()); + return; + } + if (!paused_for_connect_) { encodeBodyAndTrailers(); } diff --git a/source/common/tcp_proxy/upstream.cc b/source/common/tcp_proxy/upstream.cc index 1da6eb9157979..8c82b07852eb8 100644 --- a/source/common/tcp_proxy/upstream.cc +++ b/source/common/tcp_proxy/upstream.cc @@ -123,7 +123,9 @@ void HttpUpstream::setRequestEncoder(Http::RequestEncoder& request_encoder, bool {Http::Headers::get().Scheme, scheme}, {Http::Headers::get().Path, "/"}, {Http::Headers::get().Host, hostname_}}); - request_encoder_->encodeHeaders(*headers, false); + const auto status = request_encoder_->encodeHeaders(*headers, false); + // Encoding can only fail on missing required request headers. + ASSERT(status.ok()); } void HttpUpstream::resetEncoder(Network::ConnectionEvent event, bool inform_downstream) { diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 475f2c377e1fc..f9dfb221c9ce4 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -274,7 +274,9 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() { stream_info.setDownstreamRemoteAddress(local_address_); stream_info.onUpstreamHostSelected(host_); parent_.request_headers_parser_->evaluateHeaders(*request_headers, stream_info); - request_encoder->encodeHeaders(*request_headers, true); + auto status = request_encoder->encodeHeaders(*request_headers, true); + // Encoding will only fail if required request headers are missing. + ASSERT(status.ok()); } void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResetStream(Http::StreamResetReason, @@ -691,7 +693,9 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() { Router::FilterUtility::setUpstreamScheme( headers_message->headers(), host_->transportSocketFactory().implementsSecureTransport()); - request_encoder_->encodeHeaders(headers_message->headers(), false); + auto status = request_encoder_->encodeHeaders(headers_message->headers(), false); + // Encoding will only fail if required headers are missing. + ASSERT(status.ok()); grpc::health::v1::HealthCheckRequest request; if (parent_.service_name_.has_value()) { diff --git a/source/extensions/upstreams/http/http/upstream_request.h b/source/extensions/upstreams/http/http/upstream_request.h index 588efa93c7f6b..ff0650a3b231a 100644 --- a/source/extensions/upstreams/http/http/upstream_request.h +++ b/source/extensions/upstreams/http/http/upstream_request.h @@ -66,8 +66,9 @@ class HttpUpstream : public Router::GenericUpstream, public Envoy::Http::StreamC void encodeMetadata(const Envoy::Http::MetadataMapVector& metadata_map_vector) override { request_encoder_->encodeMetadata(metadata_map_vector); } - void encodeHeaders(const Envoy::Http::RequestHeaderMap& headers, bool end_stream) override { - request_encoder_->encodeHeaders(headers, end_stream); + Envoy::Http::Status encodeHeaders(const Envoy::Http::RequestHeaderMap& headers, + bool end_stream) override { + return request_encoder_->encodeHeaders(headers, end_stream); } void encodeTrailers(const Envoy::Http::RequestTrailerMap& trailers) override { request_encoder_->encodeTrailers(trailers); diff --git a/source/extensions/upstreams/http/tcp/upstream_request.cc b/source/extensions/upstreams/http/tcp/upstream_request.cc index 4284a2e5a13d6..0c4dd46ece640 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.cc +++ b/source/extensions/upstreams/http/tcp/upstream_request.cc @@ -43,7 +43,8 @@ void TcpUpstream::encodeData(Buffer::Instance& data, bool end_stream) { upstream_conn_data_->connection().write(data, end_stream); } -void TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) { +Envoy::Http::Status TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, + bool end_stream) { // Headers should only happen once, so use this opportunity to add the proxy // proto header, if configured. ASSERT(upstream_request_->routeEntry().connectConfig().has_value()); @@ -64,6 +65,7 @@ void TcpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_s Envoy::Http::createHeaderMap( {{Envoy::Http::Headers::get().Status, "200"}})}; upstream_request_->decodeHeaders(std::move(headers), false); + return Envoy::Http::okStatus(); } void TcpUpstream::encodeTrailers(const Envoy::Http::RequestTrailerMap&) { diff --git a/source/extensions/upstreams/http/tcp/upstream_request.h b/source/extensions/upstreams/http/tcp/upstream_request.h index 4f7cc1c212de1..3829fdead139a 100644 --- a/source/extensions/upstreams/http/tcp/upstream_request.h +++ b/source/extensions/upstreams/http/tcp/upstream_request.h @@ -70,7 +70,7 @@ class TcpUpstream : public Router::GenericUpstream, // GenericUpstream void encodeData(Buffer::Instance& data, bool end_stream) override; void encodeMetadata(const Envoy::Http::MetadataMapVector&) override {} - void encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) override; + Envoy::Http::Status encodeHeaders(const Envoy::Http::RequestHeaderMap&, bool end_stream) override; void encodeTrailers(const Envoy::Http::RequestTrailerMap&) override; void readDisable(bool disable) override; void resetStream() override; diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 3829d69b5dc46..6f9ee8c57a4bc 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -182,7 +182,7 @@ class HttpStream : public LinkedObject { request_.request_encoder_->getStream().addCallbacks(request_.stream_callbacks_); } - request_.request_encoder_->encodeHeaders(request_headers, end_stream); + request_.request_encoder_->encodeHeaders(request_headers, end_stream).IgnoreError(); request_.stream_state_ = end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers; response_.stream_state_ = StreamState::PendingHeaders; } @@ -219,9 +219,11 @@ class HttpStream : public LinkedObject { } state.response_encoder_->encodeHeaders(headers, end_stream); } else { - state.request_encoder_->encodeHeaders( - fromSanitizedHeaders(directional_action.headers()), - end_stream); + state.request_encoder_ + ->encodeHeaders( + fromSanitizedHeaders(directional_action.headers()), + end_stream) + .IgnoreError(); } if (end_stream) { state.closeLocal(); diff --git a/test/common/http/codec_wrappers_test.cc b/test/common/http/codec_wrappers_test.cc index ea481bfa2247f..02e47262da653 100644 --- a/test/common/http/codec_wrappers_test.cc +++ b/test/common/http/codec_wrappers_test.cc @@ -25,8 +25,12 @@ TEST(RequestEncoderWrapper, HeaderOnlyEncode) { MockRequestEncoderWrapper wrapper; EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, true)); - wrapper.encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, true); + EXPECT_TRUE( + wrapper + .encodeHeaders( + TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, + true) + .ok()); EXPECT_TRUE(wrapper.encodeComplete()); } @@ -34,8 +38,12 @@ TEST(RequestEncoderWrapper, HeaderAndBodyEncode) { MockRequestEncoderWrapper wrapper; EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, false)); - wrapper.encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, false); + EXPECT_TRUE( + wrapper + .encodeHeaders( + TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, + false) + .ok()); EXPECT_FALSE(wrapper.encodeComplete()); Buffer::OwnedImpl data; @@ -48,8 +56,12 @@ TEST(RequestEncoderWrapper, HeaderAndBodyAndTrailersEncode) { MockRequestEncoderWrapper wrapper; EXPECT_CALL(wrapper.innerEncoder(), encodeHeaders(_, false)); - wrapper.encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, false); + EXPECT_TRUE( + wrapper + .encodeHeaders( + TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}, {":authority", "foo"}}, + false) + .ok()); EXPECT_FALSE(wrapper.encodeComplete()); Buffer::OwnedImpl data; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 0f4b8a6a8ddea..68be164b507b2 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2010,7 +2010,7 @@ void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength(uint32_t Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); TestResponseHeaderMapImpl expected_headers{{":status", "200"}, {"transfer-encoding", "chunked"}}; Buffer::OwnedImpl expected_data("Hello World"); @@ -2051,7 +2051,7 @@ TEST_P(Http1ClientConnectionImplTest, SimpleGet) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ("GET / HTTP/1.1\r\ncontent-length: 0\r\n\r\n", output); } @@ -2067,7 +2067,7 @@ TEST_P(Http1ClientConnectionImplTest, SimpleGetWithHeaderCasing) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {"my-custom-header", "hey"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ("GET / HTTP/1.1\r\nMy-Custom-Header: hey\r\nContent-Length: 0\r\n\r\n", output); } @@ -2081,7 +2081,7 @@ TEST_P(Http1ClientConnectionImplTest, HostHeaderTranslate) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); } @@ -2114,7 +2114,7 @@ TEST_P(Http1ClientConnectionImplTest, FlowControlReadDisabledReenable) { // Request. TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder->encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder->encodeHeaders(headers, true).ok()); EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); output.clear(); @@ -2142,7 +2142,7 @@ TEST_P(Http1ClientConnectionImplTest, EmptyBodyResponse503) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 503 Service Unavailable\r\nContent-Length: 0\r\n\r\n"); @@ -2156,7 +2156,7 @@ TEST_P(Http1ClientConnectionImplTest, EmptyBodyResponse200) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n"); @@ -2170,7 +2170,7 @@ TEST_P(Http1ClientConnectionImplTest, HeadRequest) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "HEAD"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 20\r\n\r\n"); @@ -2184,7 +2184,7 @@ TEST_P(Http1ClientConnectionImplTest, 204Response) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\n\r\n"); @@ -2201,7 +2201,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseContentLengthNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 20\r\n\r\n"); auto status = codec_->dispatch(response); @@ -2219,7 +2219,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseContentLengthNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 20\r\n\r\n"); auto status = codec_->dispatch(response); @@ -2236,7 +2236,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseWithContentLength0) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 0\r\n\r\n"); @@ -2253,7 +2253,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseWithContentLength0) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, true)); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 0\r\n\r\n"); @@ -2271,7 +2271,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseTransferEncodingNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nTransfer-Encoding: chunked\r\n\r\n"); auto status = codec_->dispatch(response); @@ -2289,7 +2289,7 @@ TEST_P(Http1ClientConnectionImplTest, 204ResponseTransferEncodingNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nTransfer-Encoding: chunked\r\n\r\n"); auto status = codec_->dispatch(response); @@ -2304,7 +2304,7 @@ TEST_P(Http1ClientConnectionImplTest, ContinueHeaders) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decode100ContinueHeaders_(_)); EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); @@ -2326,7 +2326,7 @@ TEST_P(Http1ClientConnectionImplTest, MultipleContinueHeaders) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decode100ContinueHeaders_(_)); EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); @@ -2355,7 +2355,7 @@ TEST_P(Http1ClientConnectionImplTest, 1xxNonContinueHeaders) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl response("HTTP/1.1 102 Processing\r\n\r\n"); @@ -2372,7 +2372,7 @@ TEST_P(Http1ClientConnectionImplTest, 101ResponseTransferEncodingNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response( "HTTP/1.1 101 Switching Protocols\r\nTransfer-Encoding: chunked\r\n\r\n"); @@ -2391,7 +2391,7 @@ TEST_P(Http1ClientConnectionImplTest, 101ResponseTransferEncodingNotAllowed) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response( "HTTP/1.1 101 Switching Protocols\r\nTransfer-Encoding: chunked\r\n\r\n"); @@ -2410,10 +2410,19 @@ TEST_P(Http1ClientConnectionImplTest, BadEncodeParams) { // cause a direct response by the filter manager. The old codecs will still throw an exception // (that presently will be uncaught in contexts like sendLocalReply). Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - if (!testingNewCodec()) { - EXPECT_THROW(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}}, true), - CodecClientException); - EXPECT_THROW(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}, true), + if (testingNewCodec()) { + EXPECT_THAT( + request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}}, true).message(), + testing::HasSubstr("missing required")); + EXPECT_THAT( + request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}, true).message(), + testing::HasSubstr("missing required")); + } else { + EXPECT_THROW( + request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}}, true).IgnoreError(), + CodecClientException); + EXPECT_THROW(request_encoder.encodeHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}, true) + .IgnoreError(), CodecClientException); } } @@ -2424,7 +2433,7 @@ TEST_P(Http1ClientConnectionImplTest, NoContentLengthResponse) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl expected_data1("Hello World"); EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)); @@ -2446,7 +2455,7 @@ TEST_P(Http1ClientConnectionImplTest, ResponseWithTrailers) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\n\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " "World\r\n0\r\nhello: world\r\nsecond: header\r\n\r\n"); @@ -2462,7 +2471,7 @@ TEST_P(Http1ClientConnectionImplTest, GiantPath) { Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{ {":method", "GET"}, {":path", "/" + std::string(16384, 'a')}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 20\r\n\r\n"); @@ -2492,7 +2501,7 @@ TEST_P(Http1ClientConnectionImplTest, UpgradeResponse) { {":authority", "host"}, {"connection", "upgrade"}, {"upgrade", "websocket"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Send upgrade headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); @@ -2528,7 +2537,7 @@ TEST_P(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) { {":authority", "host"}, {"connection", "upgrade"}, {"upgrade", "websocket"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Send upgrade headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); @@ -2548,7 +2557,7 @@ TEST_P(Http1ClientConnectionImplTest, ConnectResponse) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Send response headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); @@ -2579,7 +2588,7 @@ TEST_P(Http1ClientConnectionImplTest, ConnectResponseWithEarlyData) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Send response headers and payload EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); @@ -2598,7 +2607,7 @@ TEST_P(Http1ClientConnectionImplTest, ConnectRejected) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl expected_data("12345abcd"); @@ -2629,7 +2638,7 @@ TEST_P(Http1ClientConnectionImplTest, WatermarkTest) { EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Fake out the underlying Network::Connection buffer being drained. EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()); @@ -2654,7 +2663,7 @@ TEST_P(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) { request_encoder.getStream().addCallbacks(stream_callbacks); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Fake a call from the underlying Network::Connection and verify the stream is notified. EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); @@ -2688,7 +2697,7 @@ TEST_P(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { request_encoder.getStream().addCallbacks(stream_callbacks); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); // Fake a call from the underlying Network::Connection and verify the stream is notified. EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); @@ -2861,7 +2870,7 @@ TEST_P(Http1ClientConnectionImplTest, ResponseHeadersWithLargeValueRejected) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); @@ -2881,7 +2890,7 @@ TEST_P(Http1ClientConnectionImplTest, ResponseHeadersWithLargeFieldRejected) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); @@ -2900,7 +2909,7 @@ TEST_P(Http1ClientConnectionImplTest, LargeResponseHeadersAccepted) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); @@ -2922,7 +2931,7 @@ TEST_P(Http1ClientConnectionImplTest, LargeMethodRequestEncode) { {":method", long_method}, {":path", "/"}, {":authority", "host"}}; std::string output; ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ(long_method + " / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); } @@ -2940,7 +2949,7 @@ TEST_P(Http1ClientConnectionImplTest, LargePathRequestEncode) { {":method", "GET"}, {":path", long_path}, {":authority", "host"}}; std::string output; ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ("GET " + long_path + " HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); } @@ -2956,7 +2965,7 @@ TEST_P(Http1ClientConnectionImplTest, LargeHeaderRequestEncode) { {":method", "GET"}, {"foo", long_header_value}, {":path", "/"}, {":authority", "host"}}; std::string output; ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\nfoo: " + long_header_value + "\r\ncontent-length: 0\r\n\r\n", output); @@ -2969,7 +2978,7 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersRejected) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); @@ -2989,7 +2998,7 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; - request_encoder.encodeHeaders(headers, true); + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index c919f9499cea4..a9c600703ebfb 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -212,8 +212,10 @@ struct ActiveTestRequest { } void startRequest() { - callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); } Http1ConnPoolImplTest& parent_; @@ -640,14 +642,18 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); EXPECT_CALL(callbacks2.pool_ready_, ready()); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); Http::ResponseHeaderMapPtr response_headers(new TestResponseHeaderMapImpl{{":status", "200"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); - callbacks2.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks2.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // N.B. clang_tidy insists that we use std::make_unique which can not infer std::initialize_list. response_headers = std::make_unique( std::initializer_list>{{":status", "200"}}); @@ -694,8 +700,10 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { // Finishing request 1 will schedule binding the connection to request 2. conn_pool_->expectEnableUpstreamReady(); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); Http::ResponseHeaderMapPtr response_headers(new TestResponseHeaderMapImpl{{":status", "200"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); @@ -712,8 +720,10 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks2.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks2.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // N.B. clang_tidy insists that we use std::make_unique which can not infer std::initialize_list. response_headers = std::make_unique( std::initializer_list>{{":status", "200"}}); @@ -745,8 +755,10 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseHeader) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Response with 'connection: close' which should cause the connection to go away. EXPECT_CALL(*conn_pool_, onClientDestroy()); @@ -779,8 +791,10 @@ TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeader) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(*conn_pool_, onClientDestroy()); // Response with 'proxy-connection: close' which should cause the connection to go away, even if @@ -817,8 +831,10 @@ TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeaderLegacy) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Response with 'proxy-connection: close' which should cause the connection to go away, even if // there are other tokens in that header. @@ -852,8 +868,10 @@ TEST_F(Http1ConnPoolImplTest, Http10NoConnectionKeepAlive) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Response without 'connection: keep-alive' which should cause the connection to go away. EXPECT_CALL(*conn_pool_, onClientDestroy()); @@ -889,8 +907,10 @@ TEST_F(Http1ConnPoolImplTest, Http10NoConnectionKeepAliveLegacy) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Response without 'connection: keep-alive' which should cause the connection to go away. EXPECT_CALL(*conn_pool_, onClientDestroy()); @@ -925,8 +945,10 @@ TEST_F(Http1ConnPoolImplTest, MaxRequestsPerConnection) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Response with 'connection: close' which should cause the connection to go away. EXPECT_CALL(*conn_pool_, onClientDestroy()); @@ -1030,8 +1052,10 @@ TEST_F(Http1ConnPoolImplTest, RemoteCloseToCompleteResponse) { EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - callbacks.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); inner_decoder->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, false); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index d8ec1e0adb55b..fe9be350397fb 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -285,7 +285,7 @@ class Http2CodecImplTest : public ::testing::TestWithParamencodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); nghttp2_priority_spec spec = {0, 10, 0}; // HTTP/2 codec adds 1 to the number of active streams when computing PRIORITY frames limit @@ -302,7 +302,7 @@ class Http2CodecImplTest : public ::testing::TestWithParamencodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); // Send one DATA frame back EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); @@ -329,7 +329,7 @@ class Http2CodecImplTest : public ::testing::TestWithParamencodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // HTTP/2 codec does not send empty DATA frames with no END_STREAM flag. // To make this work, send raw bytes representing empty DATA frames bypassing client codec. @@ -349,7 +349,7 @@ TEST_P(Http2CodecImplTest, ShutdownNotice) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); EXPECT_CALL(client_callbacks_, onGoAway(_)); server_->shutdownNotice(); @@ -367,7 +367,7 @@ TEST_P(Http2CodecImplTest, ContinueHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); @@ -385,7 +385,7 @@ TEST_P(Http2CodecImplTest, TrailerStatus) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); @@ -407,7 +407,7 @@ TEST_P(Http2CodecImplTest, MultipleContinueHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); @@ -428,7 +428,7 @@ TEST_P(Http2CodecImplTest, 1xxNonContinueHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl other_headers{{":status", "102"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); @@ -442,7 +442,7 @@ TEST_P(Http2CodecImplTest, Invalid101SwitchingProtocols) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl upgrade_headers{{":status", "101"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, _)).Times(0); @@ -456,7 +456,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError); @@ -473,7 +473,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); // Buffer client data to avoid mock recursion causing lifetime issues. ON_CALL(server_connection_, write(_, _)) @@ -499,7 +499,7 @@ TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfFalse) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); EXPECT_FALSE(response_encoder_->streamErrorOnInvalidHttpMessage()); } @@ -511,7 +511,7 @@ TEST_P(Http2CodecImplTest, CodecHasCorrectStreamErrorIfTrue) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); EXPECT_TRUE(response_encoder_->streamErrorOnInvalidHttpMessage()); } @@ -522,7 +522,7 @@ TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); @@ -542,7 +542,7 @@ TEST_P(Http2CodecImplTest, InvalidRepeatContinueAllowed) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl continue_headers{{":status", "100"}}; EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); @@ -571,7 +571,7 @@ TEST_P(Http2CodecImplTest, Invalid204WithContentLength) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl response_headers{{":status", "204"}, {"content-length", "3"}}; // What follows is a hack to get headers that should span into continuation frames. The default @@ -600,7 +600,7 @@ TEST_P(Http2CodecImplTest, Invalid204WithContentLengthAllowed) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); // Buffer client data to avoid mock recursion causing lifetime issues. ON_CALL(server_connection_, write(_, _)) @@ -635,7 +635,7 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); MockStreamCallbacks callbacks; request_encoder_->getStream().addCallbacks(callbacks); @@ -648,28 +648,30 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) { TEST_P(Http2CodecImplTest, InvalidHeadersFrame) { initialize(); - EXPECT_THROW(request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true), ServerCodecError); - EXPECT_EQ(1, server_stats_store_.counter("http2.rx_messaging_error").value()); + const auto status = request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true); + EXPECT_FALSE(status.ok()); + EXPECT_THAT(status.message(), testing::HasSubstr("missing required")); } -TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) { - stream_error_on_invalid_http_messaging_ = true; - initialize(); +// TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) { +// stream_error_on_invalid_http_messaging_ = true; +// initialize(); - MockStreamCallbacks request_callbacks; - request_encoder_->getStream().addCallbacks(request_callbacks); +// MockStreamCallbacks request_callbacks; +// request_encoder_->getStream().addCallbacks(request_callbacks); - ON_CALL(client_connection_, write(_, _)) - .WillByDefault( - Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); +// ON_CALL(client_connection_, write(_, _)) +// .WillByDefault( +// Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); +// })); - request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true); - EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset, _)); - EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::RemoteReset, _)); - auto status = server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_); - EXPECT_TRUE(status.ok()); - expectDetailsResponse("http2.violation.of.messaging.rule"); -} +// EXPECT_TRUE(request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true).ok()); +// EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset, _)); +// EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::RemoteReset, _)); +// auto status = server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_); +// EXPECT_TRUE(status.ok()); +// expectDetailsResponse("http2.violation.of.messaging.rule"); +// } TEST_P(Http2CodecImplTest, TrailingHeaders) { initialize(); @@ -677,7 +679,7 @@ TEST_P(Http2CodecImplTest, TrailingHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_CALL(request_decoder_, decodeData(_, false)); Buffer::OwnedImpl hello("hello"); request_encoder_->encodeData(hello, false); @@ -707,7 +709,7 @@ TEST_P(Http2CodecImplTest, IgnoreTrailingEmptyHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_CALL(request_decoder_, decodeData(_, false)); Buffer::OwnedImpl hello("hello"); request_encoder_->encodeData(hello, false); @@ -736,7 +738,7 @@ TEST_P(Http2CodecImplTest, SubmitTrailingEmptyHeaders) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_CALL(request_decoder_, decodeData(_, false)); Buffer::OwnedImpl hello("hello"); request_encoder_->encodeData(hello, false); @@ -764,7 +766,7 @@ TEST_P(Http2CodecImplTest, TrailingHeadersLargeClientBody) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1)); Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); request_encoder_->encodeData(body, false); @@ -794,7 +796,7 @@ TEST_P(Http2CodecImplTest, SmallMetadataVecTest) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); MetadataMapVector metadata_map_vector; const int size = 10; @@ -824,7 +826,7 @@ TEST_P(Http2CodecImplTest, LargeMetadataVecTest) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); MetadataMapVector metadata_map_vector; const int size = 10; @@ -851,7 +853,7 @@ TEST_P(Http2CodecImplTest, BadMetadataVecReceivedTest) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); MetadataMap metadata_map = { {"header_key1", "header_value1"}, @@ -894,7 +896,7 @@ TEST_P(Http2CodecImplTest, EncodeMetadataWhileDispatchingTest) { response_encoder_->encodeMetadata(metadata_map_vector); })); EXPECT_CALL(response_decoder_, decodeMetadata_(_)).Times(size); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } // Validate the keepalive PINGs are sent and received correctly. @@ -972,7 +974,7 @@ TEST_P(Http2CodecImplDeferredResetTest, DeferredResetClient) { Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); EXPECT_CALL(client_stream_callbacks, onAboveWriteBufferHighWatermark()).Times(AnyNumber()); request_encoder_->encodeData(body, true); @@ -1004,7 +1006,7 @@ TEST_P(Http2CodecImplDeferredResetTest, DeferredResetServer) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // In this case we do the same thing as DeferredResetClient but on the server side. ON_CALL(server_connection_, write(_, _)) @@ -1048,7 +1050,7 @@ TEST_P(Http2CodecImplFlowControlTest, TestFlowControlInPendingSendData) { TestRequestHeaderMapImpl expected_headers; HttpTestUtility::addDefaultHeaders(expected_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // Force the server stream to be read disabled. This will cause it to stop sending window // updates to the client. @@ -1109,7 +1111,7 @@ TEST_P(Http2CodecImplFlowControlTest, TestFlowControlInPendingSendData) { return request_decoder2; })); EXPECT_CALL(request_decoder2, decodeHeaders_(_, false)); - request_encoder2->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder2->encodeHeaders(request_headers, false).ok()); // Add the stream callbacks belatedly. On creation the stream should have // been noticed that the connection was backed up. Any new subscriber to @@ -1155,7 +1157,7 @@ TEST_P(Http2CodecImplFlowControlTest, EarlyResetRestoresWindow) { TestRequestHeaderMapImpl expected_headers; HttpTestUtility::addDefaultHeaders(expected_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // Force the server stream to be read disabled. This will cause it to stop sending window // updates to the client. @@ -1214,7 +1216,7 @@ TEST_P(Http2CodecImplFlowControlTest, FlowControlPendingRecvData) { TestRequestHeaderMapImpl expected_headers; HttpTestUtility::addDefaultHeaders(expected_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // Set artificially small watermarks to make the recv buffer easy to overrun. In production, // the recv buffer can be overrun by a client which negotiates a larger @@ -1236,7 +1238,7 @@ TEST_P(Http2CodecImplFlowControlTest, TrailingHeadersLargeServerBody) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); ON_CALL(client_connection_, write(_, _)) .WillByDefault( @@ -1273,7 +1275,7 @@ TEST_P(Http2CodecImplFlowControlTest, TrailingHeadersLargeServerBodyFlushTimeout TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); ON_CALL(client_connection_, write(_, _)) .WillByDefault( @@ -1308,7 +1310,7 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeout) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); ON_CALL(client_connection_, write(_, _)) .WillByDefault( @@ -1341,7 +1343,7 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeoutAfterGoaway) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); ON_CALL(client_connection_, write(_, _)) .WillByDefault( @@ -1375,7 +1377,7 @@ TEST_P(Http2CodecImplFlowControlTest, WindowUpdateOnReadResumingFlood) { TestRequestHeaderMapImpl expected_headers; HttpTestUtility::addDefaultHeaders(expected_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -1440,7 +1442,7 @@ TEST_P(Http2CodecImplFlowControlTest, RstStreamOnPendingFlushTimeoutFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -1492,7 +1494,7 @@ TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // The 'true' on encodeData will set local_end_stream_ on the client but not // the server. Verify that client watermark callbacks will not be called, but @@ -1567,7 +1569,7 @@ TEST_P(Http2CodecImplStreamLimitTest, MaxClientStreams) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } } @@ -1745,7 +1747,7 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); uint32_t hpack_table_size = ::testing::get(getSettingsTuple()); if (hpack_table_size != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { @@ -1784,7 +1786,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersInvokeResetStream) { std::string long_string = std::string(63 * 1024, 'q'); request_headers.addCopy("big", long_string); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(1); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); } // Large request headers are accepted when max limit configured. @@ -1799,7 +1801,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAccepted) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); } // Tests request headers with name containing underscore are dropped when the option is set to drop @@ -1813,7 +1815,7 @@ TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreDropped) { TestRequestHeaderMapImpl expected_headers(request_headers); request_headers.addCopy("bad_header", "something"); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), _)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_EQ(1, server_stats_store_.counter("http2.dropped_headers_with_underscores").value()); } @@ -1827,7 +1829,7 @@ TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreRejectedByDefault) { HttpTestUtility::addDefaultHeaders(request_headers); request_headers.addCopy("bad_header", "something"); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(1); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_EQ( 1, server_stats_store_.counter("http2.requests_rejected_with_underscores_in_headers").value()); @@ -1845,7 +1847,7 @@ TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAllowed) { TestRequestHeaderMapImpl expected_headers(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), _)); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_EQ(0, server_stats_store_.counter("http2.dropped_headers_with_underscores").value()); } @@ -1865,7 +1867,7 @@ TEST_P(Http2CodecImplTest, LargeMethodRequestEncode) { request_headers.setReferenceKey(Headers::get().Method, long_method); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&request_headers), false)); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); } // Tests stream reset when the number of request headers exceeds the default maximum of 100. @@ -1878,7 +1880,7 @@ TEST_P(Http2CodecImplTest, ManyRequestHeadersInvokeResetStream) { request_headers.addCopy(std::to_string(i), ""); } EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(1); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); } // Tests that max number of request headers is configurable. @@ -1893,7 +1895,7 @@ TEST_P(Http2CodecImplTest, ManyRequestHeadersAccepted) { } EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); } // Tests that max number of response headers is configurable. @@ -1904,7 +1906,7 @@ TEST_P(Http2CodecImplTest, ManyResponseHeadersAccepted) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"compression", "test"}}; for (int i = 0; i < 105; i++) { @@ -1934,7 +1936,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAtLimitAccepted) { ASSERT_EQ(request_headers.byteSize() + head_room, codec_limit_kb * 1024); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } TEST_P(Http2CodecImplTest, LargeRequestHeadersOverDefaultCodecLibraryLimit) { @@ -1948,7 +1950,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersOverDefaultCodecLibraryLimit) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } TEST_P(Http2CodecImplTest, LargeRequestHeadersExceedPerHeaderLimit) { @@ -1968,7 +1970,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersExceedPerHeaderLimit) { EXPECT_CALL(client_callbacks_, onGoAway(_)); server_->shutdownNotice(); server_->goAway(); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } TEST_P(Http2CodecImplTest, ManyLargeRequestHeadersUnderPerHeaderLimit) { @@ -1984,7 +1986,7 @@ TEST_P(Http2CodecImplTest, ManyLargeRequestHeadersUnderPerHeaderLimit) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } TEST_P(Http2CodecImplTest, LargeRequestHeadersAtMaxConfigurable) { @@ -2002,7 +2004,7 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAtMaxConfigurable) { EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(1); EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); } // Note this is Http2CodecImplTestAll not Http2CodecImplTest, to test @@ -2013,7 +2015,7 @@ TEST_P(Http2CodecImplTestAll, TestCodecHeaderCompression) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_->encodeHeaders(request_headers, true); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"compression", "test"}}; EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); @@ -2044,7 +2046,7 @@ TEST_P(Http2CodecImplTest, PingFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // Send one frame above the outbound control queue size limit for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; @@ -2077,7 +2079,7 @@ TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // Send one frame above the outbound control queue size limit for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; @@ -2102,7 +2104,7 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); for (int i = 0; i < kMaxOutboundControlFrames; ++i) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); @@ -2149,7 +2151,7 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2181,7 +2183,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2218,7 +2220,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); // +2 is to account for HEADERS and PING ACK, that is used to trigger mitigation EXPECT_CALL(server_connection_, write(_, _)) @@ -2248,7 +2250,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodCounterReset) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2290,7 +2292,7 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2322,7 +2324,7 @@ TEST_P(Http2CodecImplTest, ResponseTrailersFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2362,7 +2364,7 @@ TEST_P(Http2CodecImplTest, MetadataFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2472,7 +2474,7 @@ TEST_P(Http2CodecImplTest, GoAwayCausesOutboundFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2512,7 +2514,7 @@ TEST_P(Http2CodecImplTest, ShudowNoticeCausesOutboundFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2565,7 +2567,7 @@ TEST_P(Http2CodecImplTest, KeepAliveCausesOutboundFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2611,7 +2613,7 @@ TEST_P(Http2CodecImplTest, ResetStreamCausesOutboundFlood) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); int frame_count = 0; Buffer::OwnedImpl buffer; @@ -2662,7 +2664,7 @@ TEST_P(Http2CodecImplTest, ConnectTest) { Http::Headers::get().MethodValues.Connect); expected_headers.setReferenceKey(Headers::get().Protocol, "bytestream"); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); - request_encoder_->encodeHeaders(request_headers, false); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); EXPECT_CALL(callbacks, onResetStream(StreamResetReason::ConnectError, _)); EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::ConnectError, _)); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index c8ea9f0c89c7c..dd5952b2a2273 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -269,8 +269,10 @@ void Http2ConnPoolImplTest::closeAllClients() { void Http2ConnPoolImplTest::completeRequest(ActiveTestRequest& r) { EXPECT_CALL(r.inner_encoder_, encodeHeaders(_, true)); - r.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r.decoder_, decodeHeaders_(_, true)); r.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -335,8 +337,10 @@ TEST_F(Http2ConnPoolImplTest, VerifyAlpnFallback) { ActiveTestRequest r(*this, 0, false); expectClientConnect(0, r); EXPECT_CALL(r.inner_encoder_, encodeHeaders(_, true)); - r.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r.decoder_, decodeHeaders_(_, true)); EXPECT_CALL(*this, onClientDestroy()); @@ -359,8 +363,10 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionReadyWithRequest) { ActiveTestRequest r(*this, 0, false); expectClientConnect(0, r); EXPECT_CALL(r.inner_encoder_, encodeHeaders(_, true)); - r.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); pool_->drainConnections(); @@ -382,8 +388,10 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionBusy) { ActiveTestRequest r(*this, 0, false); expectClientConnect(0, r); EXPECT_CALL(r.inner_encoder_, encodeHeaders(_, true)); - r.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); pool_->drainConnections(); @@ -570,8 +578,10 @@ TEST_F(Http2ConnPoolImplTest, DrainConnections) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // With max_streams == 1, the second request moves the first connection // to draining. @@ -579,8 +589,10 @@ TEST_F(Http2ConnPoolImplTest, DrainConnections) { ActiveTestRequest r2(*this, 1, false); expectClientConnect(1, r2); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // This will move the second connection to draining. pool_->drainConnections(); @@ -665,16 +677,21 @@ TEST_F(Http2ConnPoolImplTest, PendingStreams) { // Send a request through each stream. EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r3.inner_encoder_, encodeHeaders(_, true)); - r3.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r3.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Since we now have an active connection, subsequent requests should connect immediately. ActiveTestRequest r4(*this, 0, true); @@ -709,16 +726,21 @@ TEST_F(Http2ConnPoolImplTest, PendingStreamsNumberConnectingTotalRequestsPerConn // Send a request through each stream. EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r3.inner_encoder_, encodeHeaders(_, true)); - r3.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r3.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Clean up everything. test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -748,16 +770,21 @@ TEST_F(Http2ConnPoolImplTest, PendingStreamsNumberConnectingConcurrentRequestsPe // Send a request through each stream. EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r3.inner_encoder_, encodeHeaders(_, true)); - r3.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r3.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); // Clean up everything. test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -899,8 +926,10 @@ TEST_F(Http2ConnPoolImplTest, VerifyConnectionTimingStats) { deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_cx_connect_ms"), _)); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -925,8 +954,10 @@ TEST_F(Http2ConnPoolImplTest, VerifyBufferLimits) { expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -946,8 +977,10 @@ TEST_F(Http2ConnPoolImplTest, RequestAndResponse) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_EQ(1U, cluster_->stats_.upstream_cx_active_.value()); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( @@ -955,8 +988,10 @@ TEST_F(Http2ConnPoolImplTest, RequestAndResponse) { ActiveTestRequest r2(*this, 0, true); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); r2.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -977,8 +1012,10 @@ TEST_F(Http2ConnPoolImplTest, LocalReset) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, false)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, false); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, false) + .ok()); r1.callbacks_.outer_encoder_->getStream().resetStream(Http::StreamResetReason::LocalReset); test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -998,8 +1035,10 @@ TEST_F(Http2ConnPoolImplTest, RemoteReset) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, false)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, false); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, false) + .ok()); r1.inner_encoder_.stream_.resetStream(Http::StreamResetReason::RemoteReset); test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -1020,9 +1059,10 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectWithActiveRequest) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); ReadyWatcher drained; pool_->addDrainedCallback([&]() -> void { drained.ready(); }); @@ -1046,15 +1086,18 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectDrainingWithActiveRequest) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); expectClientCreate(); ActiveTestRequest r2(*this, 1, false); expectClientConnect(1, r2); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); ReadyWatcher drained; pool_->addDrainedCallback([&]() -> void { drained.ready(); }); @@ -1086,15 +1129,18 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimary) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); expectClientCreate(); ActiveTestRequest r2(*this, 1, false); expectClientConnect(1, r2); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); ReadyWatcher drained; pool_->addDrainedCallback([&]() -> void { drained.ready(); }); @@ -1126,8 +1172,10 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimaryNoActiveRequest) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( @@ -1139,8 +1187,10 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimaryNoActiveRequest) { EXPECT_CALL(*this, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); r2.inner_decoder_->decodeHeaders( @@ -1174,8 +1224,10 @@ TEST_F(Http2ConnPoolImplTest, ConnectTimeout) { ActiveTestRequest r2(*this, 1, false); expectClientConnect(1, r2); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); r2.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -1201,9 +1253,10 @@ TEST_F(Http2ConnPoolImplTest, MaxGlobalRequests) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); - + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); ConnPoolCallbacks callbacks; MockResponseDecoder decoder; EXPECT_CALL(callbacks.pool_failure_, ready()); @@ -1224,8 +1277,10 @@ TEST_F(Http2ConnPoolImplTest, GoAway) { ActiveTestRequest r1(*this, 0, false); expectClientConnect(0, r1); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); - r1.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r1.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -1236,8 +1291,10 @@ TEST_F(Http2ConnPoolImplTest, GoAway) { ActiveTestRequest r2(*this, 1, false); expectClientConnect(1, r2); EXPECT_CALL(r2.inner_encoder_, encodeHeaders(_, true)); - r2.callbacks_.outer_encoder_->encodeHeaders( - TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true); + EXPECT_TRUE( + r2.callbacks_.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); r2.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); diff --git a/test/common/http/http2/frame_replay_test.cc b/test/common/http/http2/frame_replay_test.cc index b1931d350bb85..510002cf3fde2 100644 --- a/test/common/http/http2/frame_replay_test.cc +++ b/test/common/http/http2/frame_replay_test.cc @@ -33,7 +33,7 @@ void setupStream(ClientCodecFrameInjector& codec, TestClientConnectionImplNew& c // Setup a single stream to inject frames as a reply to. TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); - codec.request_encoder_->encodeHeaders(request_headers, true); + codec.request_encoder_->encodeHeaders(request_headers, true).IgnoreError(); } // Validate that a simple Huffman encoded request HEADERS frame can be decoded. diff --git a/test/common/http/http2/response_header_fuzz_test.cc b/test/common/http/http2/response_header_fuzz_test.cc index 4559aa06419b8..112f1392921af 100644 --- a/test/common/http/http2/response_header_fuzz_test.cc +++ b/test/common/http/http2/response_header_fuzz_test.cc @@ -27,7 +27,7 @@ void Replay(const Frame& frame, ClientCodecFrameInjector& codec) { // Setup a single stream to inject frames as a reply to. TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); - codec.request_encoder_->encodeHeaders(request_headers, true); + codec.request_encoder_->encodeHeaders(request_headers, true).IgnoreError(); // Send frames. status = codec.write(WellKnownFrames::defaultSettingsFrame(), connection); diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 6ba9e29461bff..bae6f3ba9acbe 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -87,7 +87,7 @@ IntegrationCodecClient::makeHeaderOnlyRequest(const Http::RequestHeaderMap& head auto response = std::make_unique(dispatcher_); Http::RequestEncoder& encoder = newStream(*response); encoder.getStream().addCallbacks(*response); - encoder.encodeHeaders(headers, true); + encoder.encodeHeaders(headers, true).IgnoreError(); flushWrite(); return response; } @@ -104,7 +104,7 @@ IntegrationCodecClient::makeRequestWithBody(const Http::RequestHeaderMap& header auto response = std::make_unique(dispatcher_); Http::RequestEncoder& encoder = newStream(*response); encoder.getStream().addCallbacks(*response); - encoder.encodeHeaders(headers, false); + encoder.encodeHeaders(headers, false).IgnoreError(); Buffer::OwnedImpl data(body); encoder.encodeData(data, true); flushWrite(); @@ -155,7 +155,7 @@ IntegrationCodecClient::startRequest(const Http::RequestHeaderMap& headers) { auto response = std::make_unique(dispatcher_); Http::RequestEncoder& encoder = newStream(*response); encoder.getStream().addCallbacks(*response); - encoder.encodeHeaders(headers, false); + encoder.encodeHeaders(headers, false).IgnoreError(); flushWrite(); return {encoder, std::move(response)}; } diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e7e2bf623c11a..d1ab35589150d 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -339,19 +339,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { useAccessLog("%RESPONSE_CODE_DETAILS%"); config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " "type.googleapis.com/google.protobuf.Empty } }"); - config_helper_.addConfigModifier( - [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); - config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { - // Clone the whole listener. - auto static_resources = bootstrap.mutable_static_resources(); - auto* old_listener = static_resources->mutable_listeners(0); - auto* cloned_listener = static_resources->add_listeners(); - cloned_listener->CopyFrom(*old_listener); - old_listener->set_name("http_forward"); - }); initialize(); - codec_client_ = makeHttpConnection(lookupPort("http")); // Missing method @@ -364,7 +352,7 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_required_header")); // Missing path for non-CONNECT response = codec_client_->makeHeaderOnlyRequest( @@ -376,18 +364,35 @@ TEST_P(ProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_required_header")); +} + +TEST_P(ProtocolIntegrationTest, FaultyFilterWithConnect) { + // Faulty filter that removed host in a CONNECT request. + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + // Clone the whole listener. + auto static_resources = bootstrap.mutable_static_resources(); + auto* old_listener = static_resources->mutable_listeners(0); + auto* cloned_listener = static_resources->add_listeners(); + cloned_listener->CopyFrom(*old_listener); + old_listener->set_name("http_forward"); + }); + useAccessLog("%RESPONSE_CODE_DETAILS%"); + config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); // Missing host for CONNECT - response = codec_client_->makeHeaderOnlyRequest( - Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "www.host.com:80"}}); + auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ + {":method", "CONNECT"}, {":scheme", "http"}, {":authority", "www.host.com:80"}}); response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("filter_removed_required_headers")); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_required_header")); } // Regression test for https://github.com/envoyproxy/envoy/issues/10270 diff --git a/test/integration/utility.cc b/test/integration/utility.cc index ddbd0816f53e6..83c963dcd1594 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -110,7 +110,7 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt if (!content_type.empty()) { headers.setContentType(content_type); } - encoder.encodeHeaders(headers, body.empty()); + encoder.encodeHeaders(headers, body.empty()).IgnoreError(); if (!body.empty()) { Buffer::OwnedImpl body_buffer(body); encoder.encodeData(body_buffer, true); diff --git a/test/mocks/http/BUILD b/test/mocks/http/BUILD index ca7705d8881f3..e5593a908c55b 100644 --- a/test/mocks/http/BUILD +++ b/test/mocks/http/BUILD @@ -85,6 +85,7 @@ envoy_cc_mock( deps = [ ":stream_mock", "//include/envoy/http:codec_interface", + "//source/common/http:header_utility_lib", ], ) diff --git a/test/mocks/http/stream_encoder.cc b/test/mocks/http/stream_encoder.cc index ad9b646af7d8e..5d872c64db15f 100644 --- a/test/mocks/http/stream_encoder.cc +++ b/test/mocks/http/stream_encoder.cc @@ -1,5 +1,7 @@ #include "test/mocks/http/stream_encoder.h" +#include "common/http/header_utility.h" + using testing::_; using testing::Invoke; @@ -12,10 +14,11 @@ MockHttp1StreamEncoderOptions::~MockHttp1StreamEncoderOptions() = default; MockRequestEncoder::MockRequestEncoder() { ON_CALL(*this, getStream()).WillByDefault(ReturnRef(stream_)); ON_CALL(*this, encodeHeaders(_, _)) - .WillByDefault(Invoke([](const RequestHeaderMap& headers, bool) { + .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_NE(nullptr, headers.Method()); + ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); + return okStatus(); })); } MockRequestEncoder::~MockRequestEncoder() = default; diff --git a/test/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index d2c682889ea32..df3d62c00e71d 100644 --- a/test/mocks/http/stream_encoder.h +++ b/test/mocks/http/stream_encoder.h @@ -2,6 +2,8 @@ #include "envoy/http/codec.h" +#include "common/http/status.h" + #include "test/mocks/http/stream.h" #include "gmock/gmock.h" @@ -23,7 +25,7 @@ class MockRequestEncoder : public RequestEncoder { ~MockRequestEncoder() override; // Http::RequestEncoder - MOCK_METHOD(void, encodeHeaders, (const RequestHeaderMap& headers, bool end_stream)); + MOCK_METHOD(Status, encodeHeaders, (const RequestHeaderMap& headers, bool end_stream)); MOCK_METHOD(void, encodeTrailers, (const RequestTrailerMap& trailers)); // Http::StreamEncoder From 0dc8a1dd24815529133d37a0cd08615ea485231e Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 2 Nov 2020 13:24:07 -0500 Subject: [PATCH 10/20] cleanup Signed-off-by: Asra Ali --- .../http_conn_man/response_code_details.rst | 2 +- include/envoy/http/codec.h | 5 ++-- include/envoy/router/router.h | 2 ++ source/common/http/filter_manager.cc | 24 +++++++++---------- source/common/router/router.cc | 6 +++-- test/common/http/http1/codec_impl_test.cc | 6 +++-- test/common/http/http2/codec_impl_test.cc | 20 ---------------- 7 files changed, 26 insertions(+), 39 deletions(-) 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 db396564906c5..898b8523f1858 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,7 @@ 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 by the router filter because configured filters removed required headers. + filter_removed_required_headers, The request was rejected in the filter manager because a configured filter removed required headers. 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 7cec31e98aa22..7a7994063cf25 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -105,9 +105,10 @@ class RequestEncoder : public virtual StreamEncoder { public: /** * Encode headers, optionally indicating end of stream. - * @param headers supplies the header map to encode. headers must have the required HTTP - * headers. + * @param headers supplies the header map to encode. Must have required HTTP headers. * @param end_stream supplies whether this is a header only request. + * @return Status indicating whether encoding succeeded. Encoding will not succeed if request + * headers are missing required headers. */ virtual Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE; diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 856f0e096315b..f9454eee452d8 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -1274,6 +1274,8 @@ class GenericUpstream { * Encode headers, optionally indicating end of stream. * @param headers supplies the header map to encode. * @param end_stream supplies whether this is a header only request. + * @return status indicating success. Encoding will fail if headers do not have required HTTP + * headers. */ virtual Http::Status encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) PURE; /** diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 202f8440ed24d..c8db94ad99492 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -382,18 +382,18 @@ FilterHeadersStatus ActiveStreamDecoderFilter::decodeHeaders(RequestHeaderMap& h // Validate filters did not erroneously remove required headers. If they do, send a direct // response. - // const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); - // if (!header_status.ok()) { - // ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, - // static_cast(this)); - // parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - // const std::string body = fmt::format("missing required header: {}", header_status.message()); - // const std::string details = - // absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - // header_status.message(), "}"); - // sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); - // return FilterHeadersStatus::StopIteration; - // } + const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); + if (!header_status.ok()) { + ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, + static_cast(this)); + parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); + const std::string body = fmt::format("missing required header: {}", header_status.message()); + const std::string details = + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", + header_status.message(), "}"); + sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); + return FilterHeadersStatus::StopIteration; + } return status; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 80d053de3e9d5..9e0353fd05c40 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -329,6 +329,10 @@ void Filter::chargeUpstreamCode(Http::Code code, } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { + // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers. This + // is validated by the filter manager. + ASSERT(headers.Method()); + ASSERT(headers.Host()); downstream_headers_ = &headers; // Extract debug configuration from filter state. This is used further along to determine whether @@ -586,9 +590,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, UpstreamRequestPtr upstream_request = std::make_unique(*this, std::move(generic_conn_pool)); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); - upstream_requests_.front()->encodeHeaders(end_stream); - if (end_stream) { onRequestComplete(); } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 68be164b507b2..3b3138173c955 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2407,8 +2407,10 @@ TEST_P(Http1ClientConnectionImplTest, BadEncodeParams) { // Invalid outbound data errors are impossible to trigger in normal processing, since bad // downstream data would have been rejected by the codec, and erroneous filter processing would - // cause a direct response by the filter manager. The old codecs will still throw an exception - // (that presently will be uncaught in contexts like sendLocalReply). + // cause a direct response by the filter manager. An invalid status is returned from new codecs + // which protects against future extensions or header modifications after the filter chain. The + // old codecs will still throw an exception (that presently will be uncaught in contexts like + // sendLocalReply). Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); if (testingNewCodec()) { EXPECT_THAT( diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index fe9be350397fb..4ca341c68c006 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -653,26 +653,6 @@ TEST_P(Http2CodecImplTest, InvalidHeadersFrame) { EXPECT_THAT(status.message(), testing::HasSubstr("missing required")); } -// TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) { -// stream_error_on_invalid_http_messaging_ = true; -// initialize(); - -// MockStreamCallbacks request_callbacks; -// request_encoder_->getStream().addCallbacks(request_callbacks); - -// ON_CALL(client_connection_, write(_, _)) -// .WillByDefault( -// Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); -// })); - -// EXPECT_TRUE(request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true).ok()); -// EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset, _)); -// EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::RemoteReset, _)); -// auto status = server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_); -// EXPECT_TRUE(status.ok()); -// expectDetailsResponse("http2.violation.of.messaging.rule"); -// } - TEST_P(Http2CodecImplTest, TrailingHeaders) { initialize(); From 664cd66d182b42c58e6fac3abd082ab3b4f01849 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 2 Nov 2020 13:50:15 -0500 Subject: [PATCH 11/20] fix tests Signed-off-by: Asra Ali --- test/common/router/router_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 00fdd783afc9f..c55c0a38a3969 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -1507,8 +1507,9 @@ TEST_F(RouterTest, ResetDuringEncodeHeaders) { EXPECT_CALL(callbacks_, removeDownstreamWatermarkCallbacks(_)); EXPECT_CALL(callbacks_, addDownstreamWatermarkCallbacks(_)); EXPECT_CALL(encoder, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> void { + .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> Http::Status { encoder.stream_.resetStream(Http::StreamResetReason::RemoteReset); + return Http::okStatus(); })); Http::TestRequestHeaderMapImpl headers; @@ -5837,8 +5838,9 @@ TEST_F(RouterTest, AutoHostRewriteEnabled) { // :authority header in the outgoing request should match the DNS name of // the selected upstream host EXPECT_CALL(encoder, encodeHeaders(HeaderMapEqualRef(&outgoing_headers), true)) - .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> void { + .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> Http::Status { encoder.stream_.resetStream(Http::StreamResetReason::RemoteReset); + return Http::okStatus(); })); EXPECT_CALL(callbacks_.stream_info_, onUpstreamHostSelected(_)) @@ -5874,8 +5876,9 @@ TEST_F(RouterTest, AutoHostRewriteDisabled) { // :authority header in the outgoing request should match the :authority header of // the incoming request EXPECT_CALL(encoder, encodeHeaders(HeaderMapEqualRef(&incoming_headers), true)) - .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> void { + .WillOnce(Invoke([&](const Http::HeaderMap&, bool) -> Http::Status { encoder.stream_.resetStream(Http::StreamResetReason::RemoteReset); + return Http::okStatus(); })); EXPECT_CALL(callbacks_.stream_info_, onUpstreamHostSelected(_)) From 0aed751fc73a9661d4490431e2d059d17222816d Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 2 Nov 2020 13:58:44 -0500 Subject: [PATCH 12/20] add what required headers are checked Signed-off-by: Asra Ali --- include/envoy/http/codec.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 7a7994063cf25..9ffa360bd72ed 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -107,8 +107,8 @@ class RequestEncoder : public virtual StreamEncoder { * Encode headers, optionally indicating end of stream. * @param headers supplies the header map to encode. Must have required HTTP headers. * @param end_stream supplies whether this is a header only request. - * @return Status indicating whether encoding succeeded. Encoding will not succeed if request - * headers are missing required headers. + * @return Status indicating whether encoding succeeded. Encoding will fail if request + * headers are missing required HTTP headers (method, path for non-CONNECT, host for CONNECT). */ virtual Status encodeHeaders(const RequestHeaderMap& headers, bool end_stream) PURE; From 979473c4bb8ecae789db59007d704e8901309eec Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 2 Nov 2020 16:07:47 -0500 Subject: [PATCH 13/20] fix quiche tests Signed-off-by: Asra Ali --- .../quic_listeners/quiche/envoy_quic_client_stream.cc | 4 +++- .../quic_listeners/quiche/envoy_quic_client_stream.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc index 866e35416b0b3..b52b411df7e75 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc @@ -46,7 +46,8 @@ EnvoyQuicClientStream::EnvoyQuicClientStream(quic::PendingStream* pending, 16 * 1024, [this]() { runLowWatermarkCallbacks(); }, [this]() { runHighWatermarkCallbacks(); }) {} -void EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) { +Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, + bool end_stream) { ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers); quic::QuicStream* writing_stream = quic::VersionUsesHttp3(transport_version()) @@ -60,6 +61,7 @@ void EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, // IETF QUIC sends HEADER frame on current stream. After writing headers, the // buffer may increase. maybeCheckWatermark(bytes_to_send_old, bytes_to_send_new, *filterManagerConnection()); + return Http::okStatus(); } void EnvoyQuicClientStream::encodeData(Buffer::Instance& data, bool end_stream) { diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h index 79003e4621f49..2446fbb0c3e9b 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h @@ -37,7 +37,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, } // Http::RequestEncoder - void encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) override; + Http::Status encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) override; void encodeTrailers(const Http::RequestTrailerMap& trailers) override; // Http::Stream From b2d4e1d8c5fd08976e1f95d9cc5b9ab7b6146bef Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 3 Nov 2020 08:35:12 -0500 Subject: [PATCH 14/20] fix all quiche Signed-off-by: Asra Ali --- .../quiche/envoy_quic_client_session_test.cc | 3 ++- .../quiche/envoy_quic_client_stream_test.cc | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc index e2d90d9164694..c95a61e5ace82 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_client_session_test.cc @@ -152,7 +152,8 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { std::string host("www.abc.com"); Http::TestRequestHeaderMapImpl request_headers{ {":authority", host}, {":method", "GET"}, {":path", "/"}}; - stream.encodeHeaders(request_headers, true); + const auto result = stream.encodeHeaders(request_headers, true); + ASSERT(result.ok()); return stream; } diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc index ac82239db0bba..7711b7c88bf0f 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc @@ -117,7 +117,8 @@ INSTANTIATE_TEST_SUITE_P(EnvoyQuicClientStreamTests, EnvoyQuicClientStreamTest, TEST_P(EnvoyQuicClientStreamTest, PostRequestAndResponse) { EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); - quic_stream_->encodeHeaders(request_headers_, false); + const auto result = quic_stream_->encodeHeaders(request_headers_, false); + EXPECT_TRUE(result.ok()); quic_stream_->encodeData(request_body_, false); quic_stream_->encodeTrailers(request_trailers_); @@ -167,7 +168,8 @@ TEST_P(EnvoyQuicClientStreamTest, OutOfOrderTrailers) { EXPECT_CALL(stream_callbacks_, onResetStream(_, _)); return; } - quic_stream_->encodeHeaders(request_headers_, true); + const auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); EXPECT_CALL(stream_decoder_, decodeHeaders_(_, /*end_stream=*/false)) .WillOnce(Invoke([](const Http::ResponseHeaderMapPtr& headers, bool) { EXPECT_EQ("200", headers->getStatusValue()); @@ -220,7 +222,8 @@ TEST_P(EnvoyQuicClientStreamTest, WatermarkSendBuffer) { quic_session_.OnWindowUpdateFrame(window_update); request_headers_.addCopy(":content-length", "32770"); // 32KB + 2 byte - quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/false); + const auto result = quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/false); + EXPECT_TRUE(result.ok()); // Encode 32kB request body. first 16KB should be written out right away. The // rest should be buffered. The high watermark is 16KB, so this call should // make the send buffer reach its high watermark. @@ -288,7 +291,8 @@ TEST_P(EnvoyQuicClientStreamTest, HeadersContributeToWatermarkIquic) { quiche::QuicheOptional) { return quic::QuicConsumedData{0u, state != quic::NO_FIN}; })); - quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/false); + const auto result = quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/false); + EXPECT_TRUE(result.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. From 4346a954c26e80f870b0a19616bd5691f3dabc26 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 3 Nov 2020 09:59:03 -0500 Subject: [PATCH 15/20] fix upstream tests Signed-off-by: Asra Ali --- .../upstream/health_checker_impl_test.cc | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 4a1ed5a7ae354..586b2a4e19b64 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -895,10 +895,11 @@ TEST_F(HttpHealthCheckerImplTest, ZeroRetryInterval) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -970,10 +971,11 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheck) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -1004,10 +1006,11 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServicePrefixPatternCheck) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -1038,10 +1041,11 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceExactPatternCheck) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -1072,10 +1076,11 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceRegexPatternCheck) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -1114,9 +1119,10 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValueOnTheHos expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); + return Http::okStatus(); })); health_checker_->start(); @@ -1158,9 +1164,10 @@ TEST_F(HttpHealthCheckerImplTest, expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); + return Http::okStatus(); })); health_checker_->start(); @@ -1192,9 +1199,10 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); + return Http::okStatus(); })); health_checker_->start(); @@ -1255,7 +1263,7 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithAdditionalHeaders) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillRepeatedly(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillRepeatedly(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.get(header_ok)[0]->value().getStringView(), value_ok); EXPECT_EQ(headers.get(header_cool)[0]->value().getStringView(), value_cool); EXPECT_EQ(headers.get(header_awesome)[0]->value().getStringView(), value_awesome); @@ -1278,6 +1286,7 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithAdditionalHeaders) { std::string current_start_time = date_formatter.fromTime(dispatcher_.timeSource().systemTime()); EXPECT_EQ(headers.get(start_time)[0]->value().getStringView(), current_start_time); + return Http::okStatus(); })); health_checker_->start(); @@ -1318,8 +1327,9 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithoutUserAgent) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillRepeatedly(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillRepeatedly(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.UserAgent(), nullptr); + return Http::okStatus(); })); health_checker_->start(); @@ -2325,9 +2335,10 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithAltPort) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); + return Http::okStatus(); })); health_checker_->start(); @@ -2627,10 +2638,11 @@ TEST_F(HttpHealthCheckerImplTest, DEPRECATED_FEATURE_TEST(ServiceNameMatch)) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(headers.getHostValue(), host); EXPECT_EQ(headers.getPathValue(), path); EXPECT_EQ(headers.getSchemeValue(), Http::Headers::get().SchemeValues.Http); + return Http::okStatus(); })); health_checker_->start(); @@ -3678,7 +3690,7 @@ class GrpcHealthCheckerImplTestBase : public GrpcHealthCheckerImplTestBaseUtils expectHealthcheckStart(0); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeHeaders(_, false)) - .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) { + .WillOnce(Invoke([&](const Http::RequestHeaderMap& headers, bool) -> Http::Status { EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, headers.getContentTypeValue()); EXPECT_EQ(std::string("/grpc.health.v1.Health/Check"), headers.getPathValue()); EXPECT_EQ(Http::Headers::get().SchemeValues.Http, headers.getSchemeValue()); @@ -3686,6 +3698,7 @@ class GrpcHealthCheckerImplTestBase : public GrpcHealthCheckerImplTestBaseUtils EXPECT_EQ(expected_host, headers.getHostValue()); EXPECT_EQ(std::chrono::milliseconds(1000).count(), Envoy::Grpc::Common::getGrpcTimeout(headers).value().count()); + return Http::okStatus(); })); EXPECT_CALL(test_sessions_[0]->request_encoder_, encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance& data, bool) { From db86e974e93c6a0d6c6f3be3f5cadd688da076d2 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 3 Nov 2020 12:13:15 -0500 Subject: [PATCH 16/20] add router test Signed-off-by: Asra Ali --- source/common/http/filter_manager.cc | 4 ++-- source/common/router/router.cc | 4 ---- source/common/router/upstream_request.cc | 9 +++++-- test/common/router/router_test.cc | 30 ++++++++++++++++++++++++ 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index c8db94ad99492..1d127ee14af1d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -387,11 +387,11 @@ FilterHeadersStatus ActiveStreamDecoderFilter::decodeHeaders(RequestHeaderMap& h ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, static_cast(this)); parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - const std::string body = fmt::format("missing required header: {}", header_status.message()); const std::string details = absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", header_status.message(), "}"); - sendLocalReply(Http::Code::ServiceUnavailable, body, nullptr, absl::nullopt, details); + sendLocalReply(Http::Code::ServiceUnavailable, header_status.message(), nullptr, absl::nullopt, + details); return FilterHeadersStatus::StopIteration; } return status; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 9e0353fd05c40..684b03226a440 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -329,10 +329,6 @@ void Filter::chargeUpstreamCode(Http::Code code, } Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { - // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers. This - // is validated by the filter manager. - ASSERT(headers.Method()); - ASSERT(headers.Host()); downstream_headers_ = &headers; // Extract debug configuration from filter state. This is used further along to determine whether diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index f59c403388620..fa35841dfacfb 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -423,8 +423,13 @@ void UpstreamRequest::onPoolReady( if (!status.ok()) { // It is possible that encodeHeaders() fails. This can happen if filters or other extensions // erroneously remove required headers. - resetStream(); - onResetStream(Http::StreamResetReason::LocalReset, status.message()); + stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); + const std::string details = + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", + status.message(), "}"); + ENVOY_LOG_MISC(info, "{}", status.message()); + parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr, + absl::nullopt, details); return; } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index c55c0a38a3969..11dd4f5ee2f5a 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -462,6 +462,36 @@ TEST_F(RouterTest, RouteNotFound) { EXPECT_EQ(callbacks_.details(), "route_not_found"); } +TEST_F(RouterTest, MissingRequiredHeaders) { + NiceMock encoder; + Http::ResponseDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + headers.removeMethod(); + + EXPECT_CALL(encoder, encodeHeaders(_, _)) + .WillOnce(Invoke([](const Http::RequestHeaderMap& headers, bool) -> Http::Status { + return Http::HeaderUtility::checkRequiredHeaders(headers); + })); + EXPECT_CALL(callbacks_, + sendLocalReply(Http::Code::ServiceUnavailable, + testing::Eq("missing required header: :method"), _, _, + "filter_removed_required_headers{missing required header: :method}")) + .WillOnce(testing::InvokeWithoutArgs([] {})); + router_.decodeHeaders(headers, true); + router_.onDestroy(); +} + TEST_F(RouterTest, ClusterNotFound) { EXPECT_CALL(callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound)); From 9372e5736f8bc4178eb8be8eb5cdf8904ce0d8a3 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 3 Nov 2020 14:21:21 -0500 Subject: [PATCH 17/20] fix extension test Signed-off-by: Asra Ali --- .../upstreams/http/tcp/upstream_request_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/upstreams/http/tcp/upstream_request_test.cc b/test/extensions/upstreams/http/tcp/upstream_request_test.cc index ef3d08ca908a0..d835151eab824 100644 --- a/test/extensions/upstreams/http/tcp/upstream_request_test.cc +++ b/test/extensions/upstreams/http/tcp/upstream_request_test.cc @@ -117,7 +117,7 @@ TEST_F(TcpUpstreamTest, Basic) { // Swallow the request headers and generate response headers. EXPECT_CALL(connection_, write(_, false)).Times(0); EXPECT_CALL(mock_router_filter_, onUpstreamHeaders(200, _, _, false)); - tcp_upstream_->encodeHeaders(request_, false); + EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, false).ok()); // Proxy the data. EXPECT_CALL(connection_, write(BufferStringEqual("foo"), false)); @@ -154,7 +154,7 @@ TEST_F(TcpUpstreamTest, V1Header) { // encodeHeaders now results in the proxy proto header being sent. EXPECT_CALL(connection_, write(BufferEqual(&expected_data), false)); - tcp_upstream_->encodeHeaders(request_, false); + EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, false).ok()); // Data is proxied as usual. EXPECT_CALL(connection_, write(BufferStringEqual("foo"), false)); @@ -177,7 +177,7 @@ TEST_F(TcpUpstreamTest, V2Header) { // encodeHeaders now results in the proxy proto header being sent. EXPECT_CALL(connection_, write(BufferEqual(&expected_data), false)); - tcp_upstream_->encodeHeaders(request_, false); + EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, false).ok()); // Data is proxied as usual. EXPECT_CALL(connection_, write(BufferStringEqual("foo"), false)); @@ -187,7 +187,7 @@ TEST_F(TcpUpstreamTest, V2Header) { TEST_F(TcpUpstreamTest, TrailersEndStream) { // Swallow the headers. - tcp_upstream_->encodeHeaders(request_, false); + EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, false).ok()); EXPECT_CALL(connection_, write(BufferStringEqual(""), true)); Envoy::Http::TestRequestTrailerMapImpl trailers{{"foo", "bar"}}; @@ -196,7 +196,7 @@ TEST_F(TcpUpstreamTest, TrailersEndStream) { TEST_F(TcpUpstreamTest, HeaderEndStreamHalfClose) { EXPECT_CALL(connection_, write(BufferStringEqual(""), true)); - tcp_upstream_->encodeHeaders(request_, true); + EXPECT_TRUE(tcp_upstream_->encodeHeaders(request_, true).ok()); } TEST_F(TcpUpstreamTest, ReadDisable) { From b0384af5425cb65c96cc5561a87bc9c0e6f930ab Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 4 Nov 2020 12:17:59 -0500 Subject: [PATCH 18/20] fix legacy tests Signed-off-by: Asra Ali --- test/common/http/http2/codec_impl_test.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 4ca341c68c006..e2c749c559406 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -648,9 +648,16 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) { TEST_P(Http2CodecImplTest, InvalidHeadersFrame) { initialize(); - const auto status = request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true); - EXPECT_FALSE(status.ok()); - EXPECT_THAT(status.message(), testing::HasSubstr("missing required")); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) { + const auto status = request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true); + + EXPECT_FALSE(status.ok()); + EXPECT_THAT(status.message(), testing::HasSubstr("missing required")); + } else { + EXPECT_THROW(request_encoder_->encodeHeaders(TestRequestHeaderMapImpl{}, true).IgnoreError(), + ServerCodecError); + EXPECT_EQ(1, server_stats_store_.counter("http2.rx_messaging_error").value()); + } } TEST_P(Http2CodecImplTest, TrailingHeaders) { From 6fd4d2fc9d5192fd54484c7903baf5e3ad215e80 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 4 Nov 2020 12:19:41 -0500 Subject: [PATCH 19/20] address comment Signed-off-by: Asra Ali --- test/integration/utility.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/utility.cc b/test/integration/utility.cc index 83c963dcd1594..bbcc7a3b33fd6 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -110,7 +110,8 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt if (!content_type.empty()) { headers.setContentType(content_type); } - encoder.encodeHeaders(headers, body.empty()).IgnoreError(); + const auto status = encoder.encodeHeaders(headers, body.empty()); + ASSERT(status.ok()); if (!body.empty()) { Buffer::OwnedImpl body_buffer(body); encoder.encodeData(body_buffer, true); From 7afd34bb2645503b8adf9c043aed6c5d57b2eb68 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 4 Nov 2020 16:06:17 -0500 Subject: [PATCH 20/20] remove FM checks and add a test with a direct response Signed-off-by: Asra Ali --- source/common/http/filter_manager.cc | 26 ------------------- source/common/http/filter_manager.h | 9 ++++++- source/common/router/upstream_request.cc | 1 - .../filters/invalid_header_filter.cc | 5 ++++ test/integration/protocol_integration_test.cc | 21 +++++++++++++++ 5 files changed, 34 insertions(+), 28 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 1d127ee14af1d..e866101e4a26d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -371,32 +371,6 @@ void ActiveStreamDecoderFilter::onDecoderFilterAboveWriteBufferHighWatermark() { parent_.filter_manager_callbacks_.onDecoderFilterAboveWriteBufferHighWatermark(); } -FilterHeadersStatus ActiveStreamDecoderFilter::decodeHeaders(RequestHeaderMap& headers, - bool end_stream) { - // Each decoder filter instance checks if the request passed to the filter is gRPC - // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() - // called here may change the content type, so we must check it before the call. - - is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); - FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); - - // Validate filters did not erroneously remove required headers. If they do, send a direct - // response. - const Http::Status header_status = HeaderUtility::checkRequiredHeaders(headers); - if (!header_status.ok()) { - ENVOY_STREAM_LOG(debug, "filter={} removed required headers", parent_, - static_cast(this)); - parent_.stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); - const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - header_status.message(), "}"); - sendLocalReply(Http::Code::ServiceUnavailable, header_status.message(), nullptr, absl::nullopt, - details); - return FilterHeadersStatus::StopIteration; - } - return status; -} - void ActiveStreamDecoderFilter::requestDataTooLarge() { ENVOY_STREAM_LOG(debug, "request data too large watermark exceeded", parent_); if (parent_.state_.decoder_filters_streaming_) { diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 9713a46494944..507e3f4a86848 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -190,7 +190,14 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase, Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override; - FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream); + // Each decoder filter instance checks if the request passed to the filter is gRPC + // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() + // called here may change the content type, so we must check it before the call. + FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream) { + is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers); + FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream); + return status; + } void requestDataTooLarge(); void requestDataDrained(); diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index fa35841dfacfb..57bcb5cc4feed 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -427,7 +427,6 @@ void UpstreamRequest::onPoolReady( const std::string details = absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", status.message(), "}"); - ENVOY_LOG_MISC(info, "{}", status.message()); parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr, absl::nullopt, details); return; diff --git a/test/integration/filters/invalid_header_filter.cc b/test/integration/filters/invalid_header_filter.cc index 8fe9862ff2b60..7c11203566624 100644 --- a/test/integration/filters/invalid_header_filter.cc +++ b/test/integration/filters/invalid_header_filter.cc @@ -29,6 +29,11 @@ class InvalidHeaderFilter : public Http::PassThroughFilter { if (Http::HeaderUtility::isConnect(headers)) { headers.removeHost(); } + if (!headers.get(Http::LowerCaseString("send-reply")).empty()) { + decoder_callbacks_->sendLocalReply(Envoy::Http::Code::OK, "", nullptr, absl::nullopt, + "InvalidHeaderFilter ready"); + return Http::FilterHeadersStatus::StopIteration; + } return Http::FilterHeadersStatus::Continue; } }; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d1ab35589150d..480b5b2e7cf5b 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -395,6 +395,27 @@ TEST_P(ProtocolIntegrationTest, FaultyFilterWithConnect) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("missing_required_header")); } +TEST_P(ProtocolIntegrationTest, MissingHeadersLocalReply) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); + config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Missing method + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"remove-method", "yes"}, + {"send-reply", "yes"}}); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("InvalidHeaderFilter_ready\n")); +} + // Regression test for https://github.com/envoyproxy/envoy/issues/10270 TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that