From 9691ae7264702d63248b30c52a3e9471169c49ef Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 31 Jan 2020 14:48:41 -0500 Subject: [PATCH 01/31] Refactor H2 codec Http2Settings config processing. The refactor removes a superfluous POD struct by storing and passing the underlying config proto directly to all consumers. This refactor enables a cleaner implementation of user defined settings parameters. Signed-off-by: Andres Guedez --- api/envoy/config/core/v3/protocol.proto | 13 ++ include/envoy/http/codec.h | 72 --------- include/envoy/upstream/upstream.h | 7 +- source/common/http/codec_client.cc | 2 +- source/common/http/conn_manager_utility.cc | 6 +- source/common/http/conn_manager_utility.h | 3 +- source/common/http/http2/codec_impl.cc | 125 ++++++++-------- source/common/http/http2/codec_impl.h | 17 ++- source/common/http/utility.cc | 110 ++++++++++---- source/common/http/utility.h | 64 +++++++- source/common/upstream/upstream_impl.cc | 2 +- source/common/upstream/upstream_impl.h | 6 +- .../network/http_connection_manager/config.cc | 6 +- .../network/http_connection_manager/config.h | 2 +- .../quiche/envoy_quic_dispatcher.cc | 7 +- source/server/http/admin.cc | 4 +- test/common/http/codec_impl_fuzz_test.cc | 50 ++++--- test/common/http/http2/BUILD | 1 + test/common/http/http2/codec_impl_test.cc | 138 ++++++++++-------- test/common/http/http2/codec_impl_test_util.h | 10 +- test/common/http/http2/frame_replay.cc | 11 +- test/common/http/http2/frame_replay.h | 2 +- test/common/http/http2/frame_replay_test.cc | 16 +- .../http/http2/request_header_fuzz_test.cc | 2 +- .../http/http2/response_header_fuzz_test.cc | 3 +- test/common/http/utility_test.cc | 54 +++---- test/common/upstream/upstream_impl_test.cc | 8 +- test/config/BUILD | 1 + test/config/utility.cc | 5 +- test/integration/fake_upstream.cc | 10 +- test/integration/http_integration.cc | 4 +- test/integration/protocol_integration_test.cc | 5 +- test/mocks/upstream/BUILD | 1 + test/mocks/upstream/cluster_info.cc | 7 +- test/mocks/upstream/cluster_info.h | 4 +- test/test_common/utility.cc | 12 -- 36 files changed, 426 insertions(+), 364 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index dbbb4318e0cfe..4baae1f2bf35e 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -225,6 +225,19 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Defines a parameter to be sent in the SETTINGS frame. + // See `RFC7540, sec. 6.5.1 `_ for details. + message SettingsParameter { + // The 16 bit parameter identifier. + google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {gte: 1 lte: 65536}]; + + // The 32 bit parameter value. + google.protobuf.UInt32Value value = 2; + } + + // TODO(AndresGuedez) + repeated SettingsParameter custom_settings_parameters = 13; } // [#not-implemented-hide:] diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 55841b7e14254..a50483ec1ef23 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -265,78 +265,6 @@ struct Http1Settings { HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default}; }; -/** - * HTTP/2 codec settings - */ -struct Http2Settings { - // TODO(jwfang): support other HTTP/2 settings - uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE}; - uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; - uint32_t initial_stream_window_size_{DEFAULT_INITIAL_STREAM_WINDOW_SIZE}; - uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; - bool allow_connect_{DEFAULT_ALLOW_CONNECT}; - bool allow_metadata_{DEFAULT_ALLOW_METADATA}; - bool stream_error_on_invalid_http_messaging_{DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING}; - uint32_t max_outbound_frames_{DEFAULT_MAX_OUTBOUND_FRAMES}; - uint32_t max_outbound_control_frames_{DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES}; - uint32_t max_consecutive_inbound_frames_with_empty_payload_{ - DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD}; - uint32_t max_inbound_priority_frames_per_stream_{DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM}; - uint32_t max_inbound_window_update_frames_per_data_frame_sent_{ - DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT}; - - // disable HPACK compression - static const uint32_t MIN_HPACK_TABLE_SIZE = 0; - // initial value from HTTP/2 spec, same as NGHTTP2_DEFAULT_HEADER_TABLE_SIZE from nghttp2 - static const uint32_t DEFAULT_HPACK_TABLE_SIZE = (1 << 12); - // no maximum from HTTP/2 spec, use unsigned 32-bit maximum - static const uint32_t MAX_HPACK_TABLE_SIZE = std::numeric_limits::max(); - - // TODO(jwfang): make this 0, the HTTP/2 spec minimum - static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; - // defaults to maximum, same as nghttp2 - static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = (1U << 31) - 1; - // no maximum from HTTP/2 spec, total streams is unsigned 32-bit maximum, - // one-side (client/server) is half that, and we need to exclude stream 0. - // same as NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS from nghttp2 - static const uint32_t MAX_MAX_CONCURRENT_STREAMS = (1U << 31) - 1; - - // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 - // NOTE: we only support increasing window size now, so this is also the minimum - // TODO(jwfang): make this 0 to support decrease window size - static const uint32_t MIN_INITIAL_STREAM_WINDOW_SIZE = (1 << 16) - 1; - // initial value from HTTP/2 spec is 65535, but we want more (256MiB) - static const uint32_t DEFAULT_INITIAL_STREAM_WINDOW_SIZE = 256 * 1024 * 1024; - // maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2 - static const uint32_t MAX_INITIAL_STREAM_WINDOW_SIZE = (1U << 31) - 1; - - // CONNECTION_WINDOW_SIZE is similar to STREAM_WINDOW_SIZE, but for connection-level window - // TODO(jwfang): make this 0 to support decrease window size - static const uint32_t MIN_INITIAL_CONNECTION_WINDOW_SIZE = (1 << 16) - 1; - // nghttp2's default connection-level window equals to its stream-level, - // our default connection-level window also equals to our stream-level - static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; - static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1; - // By default both nghttp2 and Envoy do not allow CONNECT over H2. - static const bool DEFAULT_ALLOW_CONNECT = false; - // By default Envoy does not allow METADATA support. - static const bool DEFAULT_ALLOW_METADATA = false; - // By default Envoy does not allow invalid headers. - static const bool DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING = false; - - // Default limit on the number of outbound frames of all types. - static const uint32_t DEFAULT_MAX_OUTBOUND_FRAMES = 10000; - // Default limit on the number of outbound frames of types PING, SETTINGS and RST_STREAM. - static const uint32_t DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES = 1000; - // Default limit on the number of consecutive inbound frames with an empty payload - // and no end stream flag. - static const uint32_t DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD = 1; - // Default limit on the number of inbound frames of type PRIORITY (per stream). - static const uint32_t DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM = 100; - // Default limit on the number of inbound frames of type WINDOW_UPDATE (per DATA frame sent). - static const uint32_t DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT = 10; -}; - /** * A connection (client or server) that owns multiple streams. */ diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 0571b33121e85..d8a41bae7d3d8 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -720,10 +720,11 @@ class ClusterInfo { virtual const Http::Http1Settings& http1Settings() const PURE; /** - * @return const Http::Http2Settings& for HTTP/2 connections created on behalf of this cluster. - * @see Http::Http2Settings. + * @return const envoy::config::core::v3::Http2ProtocolOptions& for HTTP/2 connections + * created on behalf of this cluster. + * @see envoy::config::core::v3::Http2ProtocolOptions. */ - virtual const Http::Http2Settings& http2Settings() const PURE; + virtual const envoy::config::core::v3::Http2ProtocolOptions& http2Options() const PURE; /** * @param name std::string containing the well-known name of the extension for which protocol diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 04a2ca0d7b047..c0a95a9ee70ee 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -156,7 +156,7 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne } case Type::HTTP2: { codec_ = std::make_unique( - *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings(), + *connection_, *this, host->cluster().statsScope(), host->cluster().http2Options(), Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount()); break; } diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index ce09baabd0e32..add02c51a7084 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -43,11 +43,11 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection& ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, - const Http2Settings& http2_settings, uint32_t max_request_headers_kb, - uint32_t max_request_headers_count) { + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + uint32_t max_request_headers_kb, uint32_t max_request_headers_count) { if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) { return std::make_unique(connection, callbacks, scope, - http2_settings, max_request_headers_kb, + http2_options, max_request_headers_kb, max_request_headers_count); } else { return std::make_unique(connection, scope, callbacks, diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index ee67111c8de8f..f5d8279ab9eaa 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -36,7 +36,8 @@ class ConnectionManagerUtility { static ServerConnectionPtr autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, - const Http1Settings& http1_settings, const Http2Settings& http2_settings, + const Http1Settings& http1_settings, + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, uint32_t max_request_headers_kb, uint32_t max_request_headers_count); /** diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e2fb633376cea..0c5481fbc1858 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -18,6 +18,7 @@ #include "common/http/codes.h" #include "common/http/exception.h" #include "common/http/headers.h" +#include "common/http/utility.h" #include "absl/container/fixed_array.h" @@ -348,27 +349,27 @@ void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map } ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, - const Http2Settings& http2_settings, const uint32_t max_headers_kb, - const uint32_t max_headers_count) + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + const uint32_t max_headers_kb, const uint32_t max_headers_count) : stats_{ALL_HTTP2_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http2."))}, connection_(connection), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count), - per_stream_buffer_limit_(http2_settings.initial_stream_window_size_), + per_stream_buffer_limit_(http2_options.initial_stream_window_size().value()), stream_error_on_invalid_http_messaging_( - http2_settings.stream_error_on_invalid_http_messaging_), - flood_detected_(false), max_outbound_frames_(http2_settings.max_outbound_frames_), + http2_options.stream_error_on_invalid_http_messaging()), + flood_detected_(false), max_outbound_frames_(http2_options.max_outbound_frames().value()), frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundFrame(fragment); }), - max_outbound_control_frames_(http2_settings.max_outbound_control_frames_), + max_outbound_control_frames_(http2_options.max_outbound_control_frames().value()), control_frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundControlFrame(fragment); }), max_consecutive_inbound_frames_with_empty_payload_( - http2_settings.max_consecutive_inbound_frames_with_empty_payload_), + http2_options.max_consecutive_inbound_frames_with_empty_payload().value()), max_inbound_priority_frames_per_stream_( - http2_settings.max_inbound_priority_frames_per_stream_), + http2_options.max_inbound_priority_frames_per_stream().value()), max_inbound_window_update_frames_per_data_frame_sent_( - http2_settings.max_inbound_window_update_frames_per_data_frame_sent_), + http2_options.max_inbound_window_update_frames_per_data_frame_sent().value()), dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {} ConnectionImpl::~ConnectionImpl() { nghttp2_session_del(session_); } @@ -835,41 +836,28 @@ void ConnectionImpl::sendPendingFrames() { } } -void ConnectionImpl::sendSettings(const Http2Settings& http2_settings, bool disable_push) { - ASSERT(http2_settings.hpack_table_size_ <= Http2Settings::MAX_HPACK_TABLE_SIZE); - ASSERT(Http2Settings::MIN_MAX_CONCURRENT_STREAMS <= http2_settings.max_concurrent_streams_ && - http2_settings.max_concurrent_streams_ <= Http2Settings::MAX_MAX_CONCURRENT_STREAMS); - ASSERT( - Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE <= http2_settings.initial_stream_window_size_ && - http2_settings.initial_stream_window_size_ <= Http2Settings::MAX_INITIAL_STREAM_WINDOW_SIZE); - ASSERT(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE <= - http2_settings.initial_connection_window_size_ && - http2_settings.initial_connection_window_size_ <= - Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE); - +void ConnectionImpl::sendSettings( + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { std::vector iv; - - if (http2_settings.allow_connect_) { + if (http2_options.allow_connect()) { iv.push_back({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, 1}); } - - if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { - iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_settings.hpack_table_size_}); - ENVOY_CONN_LOG(debug, "setting HPACK table size to {}", connection_, - http2_settings.hpack_table_size_); + const uint32_t hpack_table_size = http2_options.hpack_table_size().value(); + if (hpack_table_size != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { + iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, hpack_table_size}); + ENVOY_CONN_LOG(debug, "setting HPACK table size to {}", connection_, hpack_table_size); } - - if (http2_settings.max_concurrent_streams_ != NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS) { - iv.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}); + const uint32_t max_concurrent_streams = http2_options.max_concurrent_streams().value(); + if (max_concurrent_streams != NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS) { + iv.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, max_concurrent_streams}); ENVOY_CONN_LOG(debug, "setting max concurrent streams to {}", connection_, - http2_settings.max_concurrent_streams_); + max_concurrent_streams); } - - if (http2_settings.initial_stream_window_size_ != NGHTTP2_INITIAL_WINDOW_SIZE) { - iv.push_back( - {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_stream_window_size_}); + const uint32_t initial_stream_window_size = http2_options.initial_stream_window_size().value(); + if (initial_stream_window_size != NGHTTP2_INITIAL_WINDOW_SIZE) { + iv.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, initial_stream_window_size}); ENVOY_CONN_LOG(debug, "setting stream-level initial window size to {}", connection_, - http2_settings.initial_stream_window_size_); + initial_stream_window_size); } if (disable_push) { @@ -888,12 +876,14 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings, bool disa ASSERT(rc == 0); } + const uint32_t initial_connection_window_size = + http2_options.initial_connection_window_size().value(); // Increase connection window size up to our default size. - if (http2_settings.initial_connection_window_size_ != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { + if (initial_connection_window_size != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { ENVOY_CONN_LOG(debug, "updating connection-level initial window size to {}", connection_, - http2_settings.initial_connection_window_size_); + initial_connection_window_size); int rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, - http2_settings.initial_connection_window_size_ - + initial_connection_window_size - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); } @@ -1006,7 +996,8 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { ConnectionImpl::Http2Callbacks::~Http2Callbacks() { nghttp2_session_callbacks_del(callbacks_); } -ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings) { +ConnectionImpl::Http2Options::Http2Options( + const envoy::config::core::v3::Http2ProtocolOptions& http2_options) { nghttp2_option_new(&options_); // Currently we do not do anything with stream priority. Setting the following option prevents // nghttp2 from keeping around closed streams for use during stream priority dependency graph @@ -1019,11 +1010,12 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings) // trigger the check within nghttp2, as we check request headers length in codec_impl::saveHeader. nghttp2_option_set_max_send_header_block_length(options_, 0x2000000); - if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { - nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_); + if (http2_options.hpack_table_size().value() != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { + nghttp2_option_set_max_deflate_dynamic_table_size(options_, + http2_options.hpack_table_size().value()); } - if (http2_settings.allow_metadata_) { + if (http2_options.allow_metadata()) { nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE); } @@ -1037,29 +1029,29 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings) ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } -ConnectionImpl::ClientHttp2Options::ClientHttp2Options(const Http2Settings& http2_settings) - : Http2Options(http2_settings) { +ConnectionImpl::ClientHttp2Options::ClientHttp2Options( + const envoy::config::core::v3::Http2ProtocolOptions& http2_options) + : Http2Options(http2_options) { // Temporarily disable initial max streams limit/protection, since we might want to create // more than 100 streams before receiving the HTTP/2 SETTINGS frame from the server. // // TODO(PiotrSikora): remove this once multiple upstream connections or queuing are implemented. - nghttp2_option_set_peer_max_concurrent_streams(options_, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); + nghttp2_option_set_peer_max_concurrent_streams( + options_, ::Envoy::Http2::Utility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS); } -ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, - Http::ConnectionCallbacks& callbacks, - Stats::Scope& stats, const Http2Settings& http2_settings, - const uint32_t max_response_headers_kb, - const uint32_t max_response_headers_count) - : ConnectionImpl(connection, stats, http2_settings, max_response_headers_kb, +ClientConnectionImpl::ClientConnectionImpl( + Network::Connection& connection, Http::ConnectionCallbacks& callbacks, Stats::Scope& stats, + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + const uint32_t max_response_headers_kb, const uint32_t max_response_headers_count) + : ConnectionImpl(connection, stats, http2_options, max_response_headers_kb, max_response_headers_count), callbacks_(callbacks) { - ClientHttp2Options client_http2_options(http2_settings); + ClientHttp2Options client_http2_options(http2_options); nghttp2_session_client_new2(&session_, http2_callbacks_.callbacks(), base(), client_http2_options.options()); - sendSettings(http2_settings, true); - allow_metadata_ = http2_settings.allow_metadata_; + sendSettings(http2_options, true); + allow_metadata_ = http2_options.allow_metadata(); } Http::StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& decoder) { @@ -1098,19 +1090,18 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na return saveHeader(frame, std::move(name), std::move(value)); } -ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, - Http::ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings, - const uint32_t max_request_headers_kb, - const uint32_t max_request_headers_count) - : ConnectionImpl(connection, scope, http2_settings, max_request_headers_kb, +ServerConnectionImpl::ServerConnectionImpl( + Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, + Stats::Scope& scope, const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count) + : ConnectionImpl(connection, scope, http2_options, max_request_headers_kb, max_request_headers_count), callbacks_(callbacks) { - Http2Options http2_options(http2_settings); + Http2Options h2_options(http2_options); nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), - http2_options.options()); - sendSettings(http2_settings, false); - allow_metadata_ = http2_settings.allow_metadata_; + h2_options.options()); + sendSettings(http2_options, false); + allow_metadata_ = http2_options.allow_metadata(); } int ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index c5b8dc1ac8f4f..0fbb522a59d9e 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -79,8 +79,8 @@ class Utility { class ConnectionImpl : public virtual Connection, protected Logger::Loggable { public: ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, - const Http2Settings& http2_settings, const uint32_t max_headers_kb, - const uint32_t max_headers_count); + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, + const uint32_t max_headers_kb, const uint32_t max_headers_count); ~ConnectionImpl() override; @@ -123,7 +123,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggableset_value(OptionsLimits::DEFAULT_HPACK_TABLE_SIZE); + } + ASSERT(options_clone.hpack_table_size().value() <= OptionsLimits::MAX_HPACK_TABLE_SIZE); + if (!options_clone.has_max_concurrent_streams()) { + options_clone.mutable_max_concurrent_streams()->set_value( + OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS); + } + ASSERT( + options_clone.max_concurrent_streams().value() >= OptionsLimits::MIN_MAX_CONCURRENT_STREAMS && + options_clone.max_concurrent_streams().value() <= OptionsLimits::MAX_MAX_CONCURRENT_STREAMS); + if (!options_clone.has_initial_stream_window_size()) { + options_clone.mutable_initial_stream_window_size()->set_value( + OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); + } + ASSERT(options_clone.initial_stream_window_size().value() >= + OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE && + options_clone.initial_stream_window_size().value() <= + OptionsLimits::MAX_INITIAL_STREAM_WINDOW_SIZE); + if (!options_clone.has_initial_connection_window_size()) { + options_clone.mutable_initial_connection_window_size()->set_value( + OptionsLimits::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); + } + ASSERT(options_clone.initial_connection_window_size().value() >= + OptionsLimits::MIN_INITIAL_CONNECTION_WINDOW_SIZE && + options_clone.initial_connection_window_size().value() <= + OptionsLimits::MAX_INITIAL_CONNECTION_WINDOW_SIZE); + if (!options_clone.has_max_outbound_frames()) { + options_clone.mutable_max_outbound_frames()->set_value( + OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES); + } + if (!options_clone.has_max_outbound_control_frames()) { + options_clone.mutable_max_outbound_control_frames()->set_value( + OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES); + } + if (!options_clone.has_max_consecutive_inbound_frames_with_empty_payload()) { + options_clone.mutable_max_consecutive_inbound_frames_with_empty_payload()->set_value( + OptionsLimits::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD); + } + if (!options_clone.has_max_inbound_priority_frames_per_stream()) { + options_clone.mutable_max_inbound_priority_frames_per_stream()->set_value( + OptionsLimits::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM); + } + if (!options_clone.has_max_inbound_window_update_frames_per_data_frame_sent()) { + options_clone.mutable_max_inbound_window_update_frames_per_data_frame_sent()->set_value( + OptionsLimits::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT); + } + + return options_clone; +} + +} // namespace Utility +} // namespace Http2 + namespace Http { static const char kDefaultPath[] = "/"; @@ -240,38 +318,6 @@ bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { Http::Headers::get().UpgradeValues.WebSocket)); } -Http2Settings -Utility::parseHttp2Settings(const envoy::config::core::v3::Http2ProtocolOptions& config) { - Http2Settings ret; - ret.hpack_table_size_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, hpack_table_size, Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE); - ret.max_concurrent_streams_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_concurrent_streams, Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); - ret.initial_stream_window_size_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, initial_stream_window_size, Http::Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); - ret.initial_connection_window_size_ = - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, initial_connection_window_size, - Http::Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); - ret.max_outbound_frames_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_outbound_frames, Http::Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES); - ret.max_outbound_control_frames_ = - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_outbound_control_frames, - Http::Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES); - ret.max_consecutive_inbound_frames_with_empty_payload_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_consecutive_inbound_frames_with_empty_payload, - Http::Http2Settings::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD); - ret.max_inbound_priority_frames_per_stream_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_inbound_priority_frames_per_stream, - Http::Http2Settings::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM); - ret.max_inbound_window_update_frames_per_data_frame_sent_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, max_inbound_window_update_frames_per_data_frame_sent, - Http::Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT); - ret.allow_connect_ = config.allow_connect(); - ret.allow_metadata_ = config.allow_metadata(); - ret.stream_error_on_invalid_http_messaging_ = config.stream_error_on_invalid_http_messaging(); - return ret; -} - Http1Settings Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config) { Http1Settings ret; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 9b6f9ecbccc1f..99b3bce2e9a13 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -19,6 +19,64 @@ #include "absl/types/optional.h" namespace Envoy { +namespace Http2 { +namespace Utility { + +struct OptionsLimits { + // disable HPACK compression + static const uint32_t MIN_HPACK_TABLE_SIZE = 0; + // initial value from HTTP/2 spec, same as NGHTTP2_DEFAULT_HEADER_TABLE_SIZE from nghttp2 + static const uint32_t DEFAULT_HPACK_TABLE_SIZE = (1 << 12); + // no maximum from HTTP/2 spec, use unsigned 32-bit maximum + static const uint32_t MAX_HPACK_TABLE_SIZE = std::numeric_limits::max(); + // TODO(jwfang): make this 0, the HTTP/2 spec minimum + static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; + // defaults to maximum, same as nghttp2 + static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = (1U << 31) - 1; + // no maximum from HTTP/2 spec, total streams is unsigned 32-bit maximum, + // one-side (client/server) is half that, and we need to exclude stream 0. + // same as NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS from nghttp2 + static const uint32_t MAX_MAX_CONCURRENT_STREAMS = (1U << 31) - 1; + + // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 + // NOTE: we only support increasing window size now, so this is also the minimum + // TODO(jwfang): make this 0 to support decrease window size + static const uint32_t MIN_INITIAL_STREAM_WINDOW_SIZE = (1 << 16) - 1; + // initial value from HTTP/2 spec is 65535, but we want more (256MiB) + static const uint32_t DEFAULT_INITIAL_STREAM_WINDOW_SIZE = 256 * 1024 * 1024; + // maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2 + static const uint32_t MAX_INITIAL_STREAM_WINDOW_SIZE = (1U << 31) - 1; + + // CONNECTION_WINDOW_SIZE is similar to STREAM_WINDOW_SIZE, but for connection-level window + // TODO(jwfang): make this 0 to support decrease window size + static const uint32_t MIN_INITIAL_CONNECTION_WINDOW_SIZE = (1 << 16) - 1; + // nghttp2's default connection-level window equals to its stream-level, + // our default connection-level window also equals to our stream-level + static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; + static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1; + + // Default limit on the number of outbound frames of all types. + static const uint32_t DEFAULT_MAX_OUTBOUND_FRAMES = 10000; + // Default limit on the number of outbound frames of types PING, SETTINGS and RST_STREAM. + static const uint32_t DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES = 1000; + // Default limit on the number of consecutive inbound frames with an empty payload + // and no end stream flag. + static const uint32_t DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD = 1; + // Default limit on the number of inbound frames of type PRIORITY (per stream). + static const uint32_t DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM = 100; + // Default limit on the number of inbound frames of type WINDOW_UPDATE (per DATA frame sent). + static const uint32_t DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT = 10; +}; + +/** + * TODO(AndresGuedez): + */ +envoy::config::core::v3::Http2ProtocolOptions +initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions& options); + +} // namespace Utility +} // namespace Http2 + namespace Http { namespace Utility { @@ -163,12 +221,6 @@ bool isH2UpgradeRequest(const HeaderMap& headers); */ bool isWebSocketUpgradeRequest(const HeaderMap& headers); -/** - * @return Http2Settings An Http2Settings populated from the - * envoy::api::v2::core::Http2ProtocolOptions config. - */ -Http2Settings parseHttp2Settings(const envoy::config::core::v3::Http2ProtocolOptions& config); - /** * @return Http1Settings An Http1Settings populated from the * envoy::api::v2::core::Http1ProtocolOptions config. diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 3200453344213..4e32d54c7f004 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -660,7 +660,7 @@ ClusterInfoImpl::ClusterInfoImpl( : absl::nullopt), features_(parseFeatures(config)), http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())), - http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), + http2_options_(Http2::Utility::initializeAndValidateOptions(config.http2_protocol_options())), extension_protocol_options_(parseExtensionProtocolOptions(config, validation_visitor)), resource_managers_(config, runtime, name_, *stats_scope_), maintenance_mode_runtime_key_(absl::StrCat("upstream.maintenance_mode.", name_)), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index fb65fc79a71ae..f559d79f6a6b5 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -536,7 +536,9 @@ class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable timeout_budget_stats_; const uint64_t features_; const Http::Http1Settings http1_settings_; - const Http::Http2Settings http2_settings_; + const envoy::config::core::v3::Http2ProtocolOptions http2_options_; const std::map extension_protocol_options_; mutable ResourceManagers resource_managers_; const std::string maintenance_mode_runtime_key_; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index ba46e92c99f3b..d526913ec34b8 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -161,7 +161,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( skip_xff_append_(config.skip_xff_append()), via_(config.via()), route_config_provider_manager_(route_config_provider_manager), scoped_routes_config_provider_manager_(scoped_routes_config_provider_manager), - http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), + http2_options_(Http2::Utility::initializeAndValidateOptions(config.http2_protocol_options())), http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())), max_request_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config, max_request_headers_kb, Http::DEFAULT_MAX_REQUEST_HEADERS_KB)), @@ -449,7 +449,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, maxRequestHeadersCount()); case CodecType::HTTP2: return std::make_unique( - connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb(), + connection, callbacks, context_.scope(), http2_options_, maxRequestHeadersKb(), maxRequestHeadersCount()); case CodecType::HTTP3: // Hard code Quiche factory name here to instantiate a QUIC codec implemented. @@ -462,7 +462,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, .createQuicServerConnection(connection, callbacks)); case CodecType::AUTO: return Http::ConnectionManagerUtility::autoCreateCodec( - connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_, + connection, data, callbacks, context_.scope(), http1_settings_, http2_options_, maxRequestHeadersKb(), maxRequestHeadersCount()); } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 56993962cf2aa..8cf7329385836 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -174,7 +174,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Router::RouteConfigProviderManager& route_config_provider_manager_; Config::ConfigProviderManager& scoped_routes_config_provider_manager_; CodecType codec_type_; - const Http::Http2Settings http2_settings_; + envoy::config::core::v3::Http2ProtocolOptions http2_options_; const Http::Http1Settings http1_settings_; HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{ HttpConnectionManagerProto::OVERWRITE}; diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc b/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc index 1c295dfd88a24..9128b8f651824 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc @@ -3,6 +3,8 @@ #include "extensions/quic_listeners/quiche/envoy_quic_server_connection.h" #include "extensions/quic_listeners/quiche/envoy_quic_server_session.h" +#include "common/http/utility.h" + namespace Envoy { namespace Quic { @@ -28,8 +30,9 @@ EnvoyQuicDispatcher::EnvoyQuicDispatcher( // TODO(#8826) Ideally we should use the negotiated value from upstream which is not accessible // for now. 512MB is way to large, but the actual bytes buffered should be bound by the negotiated // upstream flow control window. - SetQuicFlag(FLAGS_quic_buffered_data_threshold, - 2 * Http::Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); // 512MB + SetQuicFlag( + FLAGS_quic_buffered_data_threshold, + 2 * ::Envoy::Http2::Utility::OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); // 512MB } void EnvoyQuicDispatcher::OnConnectionClosed(quic::QuicConnectionId connection_id, diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 4567ae55450cd..df2bc42c840bf 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1447,7 +1447,9 @@ Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) { return Http::ConnectionManagerUtility::autoCreateCodec( - connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings(), + connection, data, callbacks, server_.stats(), Http::Http1Settings(), + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()), maxRequestHeadersKb(), maxRequestHeadersCount()); } diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index ed592b66dc790..9314e720deddc 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -32,6 +32,8 @@ using testing::InvokeWithoutArgs; namespace Envoy { namespace Http { +namespace Http2Utility = ::Envoy::Http2::Utility; + // Force drain on each action, useful for figuring out what is going on when // debugging. constexpr bool DebugMode = false; @@ -51,27 +53,31 @@ Http1Settings fromHttp1Settings(const test::common::http::Http1ServerSettings& s return h1_settings; } -// Convert from test proto Http2Settings to Http2Settings. -Http2Settings fromHttp2Settings(const test::common::http::Http2Settings& settings) { - Http2Settings h2_settings; +envoy::config::core::v3::Http2ProtocolOptions +fromHttp2Settings(const test::common::http::Http2Settings& settings) { + envoy::config::core::v3::Http2ProtocolOptions options( + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions())); // We apply an offset and modulo interpretation to settings to ensure that // they are valid. Rejecting invalid settings is orthogonal to the fuzzed // code. - h2_settings.hpack_table_size_ = settings.hpack_table_size(); - h2_settings.max_concurrent_streams_ = - Http2Settings::MIN_MAX_CONCURRENT_STREAMS + - settings.max_concurrent_streams() % (1 + Http2Settings::MAX_MAX_CONCURRENT_STREAMS - - Http2Settings::MIN_MAX_CONCURRENT_STREAMS); - h2_settings.initial_stream_window_size_ = - Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE + - settings.initial_stream_window_size() % (1 + Http2Settings::MAX_INITIAL_STREAM_WINDOW_SIZE - - Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE); - h2_settings.initial_connection_window_size_ = - Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE + + options.mutable_hpack_table_size()->set_value(settings.hpack_table_size()); + options.mutable_max_concurrent_streams()->set_value( + Http2Utility::OptionsLimits::MIN_MAX_CONCURRENT_STREAMS + + settings.max_concurrent_streams() % + (1 + Http2Utility::OptionsLimits::MAX_MAX_CONCURRENT_STREAMS - + Http2Utility::OptionsLimits::MIN_MAX_CONCURRENT_STREAMS)); + options.mutable_initial_stream_window_size()->set_value( + Http2Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE + + settings.initial_stream_window_size() % + (1 + Http2Utility::OptionsLimits::MAX_INITIAL_STREAM_WINDOW_SIZE - + Http2Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE)); + options.mutable_initial_connection_window_size()->set_value( + Http2Utility::OptionsLimits::MIN_INITIAL_CONNECTION_WINDOW_SIZE + settings.initial_connection_window_size() % - (1 + Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE - - Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE); - return h2_settings; + (1 + Http2Utility::OptionsLimits::MAX_INITIAL_CONNECTION_WINDOW_SIZE - + Http2Utility::OptionsLimits::MIN_INITIAL_CONNECTION_WINDOW_SIZE)); + return options; } // Internal representation of stream state. Encapsulates the stream state, mocks @@ -347,7 +353,8 @@ enum class HttpVersion { Http1, Http2 }; void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersion http_version) { Stats::IsolatedStoreImpl stats_store; NiceMock client_connection; - const Http2Settings client_http2settings{fromHttp2Settings(input.h2_settings().client())}; + const envoy::config::core::v3::Http2ProtocolOptions client_http2_options{ + fromHttp2Settings(input.h2_settings().client())}; const Http1Settings client_http1settings; NiceMock client_callbacks; uint32_t max_request_headers_kb = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; @@ -359,7 +366,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi if (http2) { client = std::make_unique( - client_connection, client_callbacks, stats_store, client_http2settings, + client_connection, client_callbacks, stats_store, client_http2_options, max_request_headers_kb, max_response_headers_count); } else { client = std::make_unique(client_connection, stats_store, @@ -370,9 +377,10 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi NiceMock server_connection; NiceMock server_callbacks; if (http2) { - const Http2Settings server_http2settings{fromHttp2Settings(input.h2_settings().server())}; + const envoy::config::core::v3::Http2ProtocolOptions server_http2_options{ + fromHttp2Settings(input.h2_settings().server())}; server = std::make_unique( - server_connection, server_callbacks, stats_store, server_http2settings, + server_connection, server_callbacks, stats_store, server_http2_options, max_request_headers_kb, max_request_headers_count); } else { const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())}; diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index ded96525c02b0..0dfca348c4ecd 100644 --- a/test/common/http/http2/BUILD +++ b/test/common/http/http2/BUILD @@ -80,6 +80,7 @@ envoy_cc_test_library( deps = [ "//source/common/common:hex_lib", "//source/common/common:macros", + "//source/common/http:utility_lib", "//source/common/http/http2:codec_lib", "//test/common/http:common_lib", "//test/common/http/http2:codec_impl_test_util", diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 85e95250da6c6..7792f71532961 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -38,6 +38,7 @@ namespace Http2 { using Http2SettingsTuple = ::testing::tuple; using Http2SettingsTestParam = ::testing::tuple; +namespace CommonUtility = ::Envoy::Http2::Utility; class Http2CodecImplTestFixture { public: @@ -62,13 +63,13 @@ class Http2CodecImplTestFixture { virtual ~Http2CodecImplTestFixture() = default; virtual void initialize() { - Http2SettingsFromTuple(client_http2settings_, client_settings_); - Http2SettingsFromTuple(server_http2settings_, server_settings_); + Http2OptionsFromTuple(client_http2_options_, client_settings_); + Http2OptionsFromTuple(server_http2_options_, server_settings_); client_ = std::make_unique( - client_connection_, client_callbacks_, stats_store_, client_http2settings_, + client_connection_, client_callbacks_, stats_store_, client_http2_options_, max_request_headers_kb_, max_response_headers_count_); server_ = std::make_unique( - server_connection_, server_callbacks_, stats_store_, server_http2settings_, + server_connection_, server_callbacks_, stats_store_, server_http2_options_, max_request_headers_kb_, max_request_headers_count_); request_encoder_ = &client_->newStream(response_decoder_); @@ -96,20 +97,22 @@ class Http2CodecImplTestFixture { })); } - void Http2SettingsFromTuple(Http2Settings& setting, const Http2SettingsTuple& tp) { - setting.hpack_table_size_ = ::testing::get<0>(tp); - setting.max_concurrent_streams_ = ::testing::get<1>(tp); - setting.initial_stream_window_size_ = ::testing::get<2>(tp); - setting.initial_connection_window_size_ = ::testing::get<3>(tp); - setting.allow_metadata_ = allow_metadata_; - setting.stream_error_on_invalid_http_messaging_ = stream_error_on_invalid_http_messaging_; - setting.max_outbound_frames_ = max_outbound_frames_; - setting.max_outbound_control_frames_ = max_outbound_control_frames_; - setting.max_consecutive_inbound_frames_with_empty_payload_ = - max_consecutive_inbound_frames_with_empty_payload_; - setting.max_inbound_priority_frames_per_stream_ = max_inbound_priority_frames_per_stream_; - setting.max_inbound_window_update_frames_per_data_frame_sent_ = - max_inbound_window_update_frames_per_data_frame_sent_; + void Http2OptionsFromTuple(envoy::config::core::v3::Http2ProtocolOptions& options, + const Http2SettingsTuple& tp) { + options.mutable_hpack_table_size()->set_value(::testing::get<0>(tp)); + options.mutable_max_concurrent_streams()->set_value(::testing::get<1>(tp)); + options.mutable_initial_stream_window_size()->set_value(::testing::get<2>(tp)); + options.mutable_initial_connection_window_size()->set_value(::testing::get<3>(tp)); + options.set_allow_metadata(allow_metadata_); + options.set_stream_error_on_invalid_http_messaging(stream_error_on_invalid_http_messaging_); + options.mutable_max_outbound_frames()->set_value(max_outbound_frames_); + options.mutable_max_outbound_control_frames()->set_value(max_outbound_control_frames_); + options.mutable_max_consecutive_inbound_frames_with_empty_payload()->set_value( + max_consecutive_inbound_frames_with_empty_payload_); + options.mutable_max_inbound_priority_frames_per_stream()->set_value( + max_inbound_priority_frames_per_stream_); + options.mutable_max_inbound_window_update_frames_per_data_frame_sent()->set_value( + max_inbound_window_update_frames_per_data_frame_sent_); } // corruptMetadataFramePayload assumes data contains at least 10 bytes of the beginning of a @@ -136,12 +139,12 @@ class Http2CodecImplTestFixture { bool allow_metadata_ = false; bool stream_error_on_invalid_http_messaging_ = false; Stats::IsolatedStoreImpl stats_store_; - Http2Settings client_http2settings_; + envoy::config::core::v3::Http2ProtocolOptions client_http2_options_; NiceMock client_connection_; MockConnectionCallbacks client_callbacks_; std::unique_ptr client_; ConnectionWrapper client_wrapper_; - Http2Settings server_http2settings_; + envoy::config::core::v3::Http2ProtocolOptions server_http2_options_; NiceMock server_connection_; MockServerConnectionCallbacks server_callbacks_; std::unique_ptr server_; @@ -157,14 +160,15 @@ class Http2CodecImplTestFixture { uint32_t max_request_headers_kb_ = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; uint32_t max_request_headers_count_ = Http::DEFAULT_MAX_HEADERS_COUNT; uint32_t max_response_headers_count_ = Http::DEFAULT_MAX_HEADERS_COUNT; - uint32_t max_outbound_frames_ = Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES; - uint32_t max_outbound_control_frames_ = Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES; + uint32_t max_outbound_frames_ = CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES; + uint32_t max_outbound_control_frames_ = + CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES; uint32_t max_consecutive_inbound_frames_with_empty_payload_ = - Http2Settings::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD; + CommonUtility::OptionsLimits::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD; uint32_t max_inbound_priority_frames_per_stream_ = - Http2Settings::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM; + CommonUtility::OptionsLimits::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM; uint32_t max_inbound_window_update_frames_per_data_frame_sent_ = - Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT; + CommonUtility::OptionsLimits::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT; }; class Http2CodecImplTest : public ::testing::TestWithParam, @@ -185,7 +189,7 @@ class Http2CodecImplTest : public ::testing::TestWithParamsession(), NGHTTP2_FLAG_NONE, 1, &spec)); } @@ -210,7 +214,9 @@ class Http2CodecImplTest : public ::testing::TestWithParamsession(), NGHTTP2_FLAG_NONE, 1, 1)); } @@ -228,7 +234,7 @@ class Http2CodecImplTest : public ::testing::TestWithParam(GetParam())); - Http2SettingsFromTuple(server_http2settings_, ::testing::get<1>(GetParam())); + Http2OptionsFromTuple(client_http2_options_, ::testing::get<0>(GetParam())); + Http2OptionsFromTuple(server_http2_options_, ::testing::get<1>(GetParam())); client_ = std::make_unique( - client_connection_, client_callbacks_, stats_store_, client_http2settings_, + client_connection_, client_callbacks_, stats_store_, client_http2_options_, max_request_headers_kb_, max_response_headers_count_); server_ = std::make_unique( - server_connection_, server_callbacks_, stats_store_, server_http2settings_, + server_connection_, server_callbacks_, stats_store_, server_http2_options_, max_request_headers_kb_, max_request_headers_count_); for (int i = 0; i < 101; ++i) { @@ -951,10 +957,11 @@ TEST_P(Http2CodecImplStreamLimitTest, MaxClientStreams) { } #define HTTP2SETTINGS_SMALL_WINDOW_COMBINE \ - ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ - ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ - ::testing::Values(Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE), \ - ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE)) + ::testing::Combine( \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS), \ + ::testing::Values(CommonUtility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::MIN_INITIAL_CONNECTION_WINDOW_SIZE)) // Deferred reset tests use only small windows so that we can test certain conditions. INSTANTIATE_TEST_SUITE_P(Http2CodecImplDeferredResetTest, Http2CodecImplDeferredResetTest, @@ -968,10 +975,11 @@ INSTANTIATE_TEST_SUITE_P(Http2CodecImplFlowControlTest, Http2CodecImplFlowContro // we separate default/edge cases here to avoid combinatorial explosion #define HTTP2SETTINGS_DEFAULT_COMBINE \ - ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ - ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ - ::testing::Values(Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE), \ - ::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) + ::testing::Combine( \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS), \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) // Stream limit test only uses the default values because not all combinations of // edge settings allow for the number of streams needed by the test. @@ -985,13 +993,14 @@ INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, #define HTTP2SETTINGS_EDGE_COMBINE \ ::testing::Combine( \ - ::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, Http2Settings::MAX_HPACK_TABLE_SIZE), \ - ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, \ - Http2Settings::MAX_MAX_CONCURRENT_STREAMS), \ - ::testing::Values(Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE, \ - Http2Settings::MAX_INITIAL_STREAM_WINDOW_SIZE), \ - ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, \ - Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) + ::testing::Values(CommonUtility::OptionsLimits::MIN_HPACK_TABLE_SIZE, \ + CommonUtility::OptionsLimits::MAX_HPACK_TABLE_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::MIN_MAX_CONCURRENT_STREAMS, \ + CommonUtility::OptionsLimits::MAX_MAX_CONCURRENT_STREAMS), \ + ::testing::Values(CommonUtility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE, \ + CommonUtility::OptionsLimits::MAX_INITIAL_STREAM_WINDOW_SIZE), \ + ::testing::Values(CommonUtility::OptionsLimits::MIN_INITIAL_CONNECTION_WINDOW_SIZE, \ + CommonUtility::OptionsLimits::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) // Make sure we have coverage for high and low values for various combinations and permutations // of HTTP settings in at least one test fixture. @@ -1243,7 +1252,8 @@ TEST_P(Http2CodecImplTestAll, TestCodecHeaderCompression) { nghttp2_session_get_hd_inflate_dynamic_table_size(server_->session())); // Verify that headers are compressed only when both client and server advertise table size > 0: - if (client_http2settings_.hpack_table_size_ && server_http2settings_.hpack_table_size_) { + if (client_http2_options_.hpack_table_size().value() && + server_http2_options_.hpack_table_size().value()) { EXPECT_NE(0, nghttp2_session_get_hd_deflate_dynamic_table_size(client_->session())); EXPECT_NE(0, nghttp2_session_get_hd_deflate_dynamic_table_size(server_->session())); } else { @@ -1262,7 +1272,8 @@ TEST_P(Http2CodecImplTest, PingFlood) { request_encoder_->encodeHeaders(request_headers, false); // Send one frame above the outbound control queue size limit - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; + ++i) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); } @@ -1275,7 +1286,7 @@ TEST_P(Http2CodecImplTest, PingFlood) { })); EXPECT_THROW(client_->sendPendingFrames(), FrameFloodException); - EXPECT_EQ(ack_count, Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES); + EXPECT_EQ(ack_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES); EXPECT_EQ(1, stats_store_.counter("http2.outbound_control_flood").value()); } @@ -1290,12 +1301,13 @@ TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) { request_encoder_->encodeHeaders(request_headers, false); // Send one frame above the outbound control queue size limit - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1; + ++i) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); } EXPECT_CALL(server_connection_, write(_, _)) - .Times(Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1); + .Times(CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1); EXPECT_NO_THROW(client_->sendPendingFrames()); } @@ -1359,7 +1371,7 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) { })); TestHeaderMapImpl response_headers{{":status", "200"}}; - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES + 1; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1; ++i) { EXPECT_NO_THROW(response_encoder_->encodeHeaders(response_headers, false)); } // Presently flood mitigation is done only when processing downstream data @@ -1367,7 +1379,7 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); EXPECT_THROW(client_->sendPendingFrames(), FrameFloodException); - EXPECT_EQ(frame_count, Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES + 1); + EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1); EXPECT_EQ(1, stats_store_.counter("http2.outbound_flood").value()); } @@ -1391,7 +1403,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { TestHeaderMapImpl response_headers{{":status", "200"}}; response_encoder_->encodeHeaders(response_headers, false); // Account for the single HEADERS frame above - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { Buffer::OwnedImpl data("0"); EXPECT_NO_THROW(response_encoder_->encodeData(data, false)); } @@ -1400,7 +1412,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); EXPECT_THROW(client_->sendPendingFrames(), FrameFloodException); - EXPECT_EQ(frame_count, Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES + 1); + EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1); EXPECT_EQ(1, stats_store_.counter("http2.outbound_flood").value()); } @@ -1416,14 +1428,14 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) { // +2 is to account for HEADERS and PING ACK, that is used to trigger mitigation EXPECT_CALL(server_connection_, write(_, _)) - .Times(Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES + 2); + .Times(CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 2); EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)).Times(1); EXPECT_CALL(response_decoder_, decodeData(_, false)) - .Times(Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES); + .Times(CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES); TestHeaderMapImpl response_headers{{":status", "200"}}; response_encoder_->encodeHeaders(response_headers, false); // Account for the single HEADERS frame above - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES; ++i) { Buffer::OwnedImpl data("0"); EXPECT_NO_THROW(response_encoder_->encodeData(data, false)); } @@ -1495,7 +1507,7 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { TestHeaderMapImpl response_headers{{":status", "200"}}; response_encoder_->encodeHeaders(response_headers, false); // Account for the single HEADERS frame above - for (uint32_t i = 0; i < Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { + for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) { Buffer::OwnedImpl data("0"); EXPECT_NO_THROW(response_encoder_->encodeData(data, false)); } @@ -1503,7 +1515,7 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); EXPECT_THROW(client_->sendPendingFrames(), FrameFloodException); - EXPECT_EQ(frame_count, Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES); + EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES); EXPECT_EQ(1, stats_store_.counter("http2.outbound_flood").value()); } @@ -1542,7 +1554,9 @@ TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) { Buffer::OwnedImpl data; emptyDataFlood(data); EXPECT_CALL(request_decoder_, decodeData(_, false)) - .Times(Http2Settings::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD + 1); + .Times( + CommonUtility::OptionsLimits::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD + + 1); EXPECT_NO_THROW(server_wrapper_.dispatch(data, *server_)); } diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 67521db0fec86..494a856715323 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -9,9 +9,10 @@ namespace Http2 { class TestServerConnectionImpl : public ServerConnectionImpl { public: TestServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings, + Stats::Scope& scope, + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, uint32_t max_request_headers_kb, uint32_t max_request_headers_count) - : ServerConnectionImpl(connection, callbacks, scope, http2_settings, max_request_headers_kb, + : ServerConnectionImpl(connection, callbacks, scope, http2_options, max_request_headers_kb, max_request_headers_count) {} nghttp2_session* session() { return session_; } using ServerConnectionImpl::getStream; @@ -20,9 +21,10 @@ class TestServerConnectionImpl : public ServerConnectionImpl { class TestClientConnectionImpl : public ClientConnectionImpl { public: TestClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings, + Stats::Scope& scope, + const envoy::config::core::v3::Http2ProtocolOptions& http2_options, uint32_t max_request_headers_kb, uint32_t max_request_headers_count) - : ClientConnectionImpl(connection, callbacks, scope, http2_settings, max_request_headers_kb, + : ClientConnectionImpl(connection, callbacks, scope, http2_options, max_request_headers_kb, max_request_headers_count) {} nghttp2_session* session() { return session_; } using ClientConnectionImpl::getStream; diff --git a/test/common/http/http2/frame_replay.cc b/test/common/http/http2/frame_replay.cc index e7f846954a69a..ea1d9e9b596d4 100644 --- a/test/common/http/http2/frame_replay.cc +++ b/test/common/http/http2/frame_replay.cc @@ -2,6 +2,7 @@ #include "common/common/hex.h" #include "common/common/macros.h" +#include "common/http/utility.h" #include "test/common/http/common.h" #include "test/test_common/environment.h" @@ -58,13 +59,9 @@ void FrameUtils::fixupHeaders(Frame& frame) { } CodecFrameInjector::CodecFrameInjector(const std::string& injector_name) - : injector_name_(injector_name) { - settings_.hpack_table_size_ = Http2Settings::DEFAULT_HPACK_TABLE_SIZE; - settings_.max_concurrent_streams_ = Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS; - settings_.initial_stream_window_size_ = Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE; - settings_.initial_connection_window_size_ = Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE; - settings_.allow_metadata_ = false; -} + : options_(::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions())), + injector_name_(injector_name) {} ClientCodecFrameInjector::ClientCodecFrameInjector() : CodecFrameInjector("server") { ON_CALL(client_connection_, write(_, _)) diff --git a/test/common/http/http2/frame_replay.h b/test/common/http/http2/frame_replay.h index 4ab12bd93a381..97b8bcc62fc04 100644 --- a/test/common/http/http2/frame_replay.h +++ b/test/common/http/http2/frame_replay.h @@ -55,7 +55,7 @@ class CodecFrameInjector { // Writes the data using the Http::Connection's nghttp2 session. void write(const Frame& frame, Http::Connection& connection); - Http2Settings settings_; + envoy::config::core::v3::Http2ProtocolOptions options_; Stats::IsolatedStoreImpl stats_store_; const std::string injector_name_; }; diff --git a/test/common/http/http2/frame_replay_test.cc b/test/common/http/http2/frame_replay_test.cc index e9869b2963b7b..65613c88aa960 100644 --- a/test/common/http/http2/frame_replay_test.cc +++ b/test/common/http/http2/frame_replay_test.cc @@ -57,7 +57,7 @@ TEST_F(RequestFrameCommentTest, SimpleExampleHuffman) { // Validate HEADERS decode. ServerCodecFrameInjector codec; TestServerConnectionImpl connection( - codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, + codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); @@ -89,7 +89,7 @@ TEST_F(ResponseFrameCommentTest, SimpleExampleHuffman) { // Validate HEADERS decode. ClientCodecFrameInjector codec; TestClientConnectionImpl connection( - codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.settings_, + codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); setupStream(codec, connection); @@ -133,7 +133,7 @@ TEST_F(RequestFrameCommentTest, SimpleExamplePlain) { // Validate HEADERS decode. ServerCodecFrameInjector codec; TestServerConnectionImpl connection( - codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, + codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); @@ -167,7 +167,7 @@ TEST_F(ResponseFrameCommentTest, SimpleExamplePlain) { // Validate HEADERS decode. ClientCodecFrameInjector codec; TestClientConnectionImpl connection( - codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.settings_, + codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); setupStream(codec, connection); @@ -196,7 +196,7 @@ TEST_F(RequestFrameCommentTest, SingleByteNulCrLfInHeaderFrame) { // Play the frames back. ServerCodecFrameInjector codec; TestServerConnectionImpl connection( - codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, + codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); @@ -229,7 +229,7 @@ TEST_F(ResponseFrameCommentTest, SingleByteNulCrLfInHeaderFrame) { // Play the frames back. ClientCodecFrameInjector codec; TestClientConnectionImpl connection( - codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.settings_, + codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); setupStream(codec, connection); @@ -264,7 +264,7 @@ TEST_F(RequestFrameCommentTest, SingleByteNulCrLfInHeaderField) { // Play the frames back. ServerCodecFrameInjector codec; TestServerConnectionImpl connection( - codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, + codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); @@ -302,7 +302,7 @@ TEST_F(ResponseFrameCommentTest, SingleByteNulCrLfInHeaderField) { // Play the frames back. ClientCodecFrameInjector codec; TestClientConnectionImpl connection( - codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.settings_, + codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); setupStream(codec, connection); diff --git a/test/common/http/http2/request_header_fuzz_test.cc b/test/common/http/http2/request_header_fuzz_test.cc index 440014afb648b..35bce06813b33 100644 --- a/test/common/http/http2/request_header_fuzz_test.cc +++ b/test/common/http/http2/request_header_fuzz_test.cc @@ -17,7 +17,7 @@ namespace { void Replay(const Frame& frame, ServerCodecFrameInjector& codec) { // Create the server connection containing the nghttp2 session. TestServerConnectionImpl connection( - codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.settings_, + codec.server_connection_, codec.server_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); codec.write(WellKnownFrames::clientConnectionPrefaceFrame(), connection); codec.write(WellKnownFrames::defaultSettingsFrame(), connection); diff --git a/test/common/http/http2/response_header_fuzz_test.cc b/test/common/http/http2/response_header_fuzz_test.cc index f1f3ff57e4252..bf2421ebfcbd9 100644 --- a/test/common/http/http2/response_header_fuzz_test.cc +++ b/test/common/http/http2/response_header_fuzz_test.cc @@ -3,7 +3,6 @@ // uncompressed HEADERS fuzzing. #include "common/http/exception.h" - #include "test/common/http/common.h" #include "test/common/http/http2/frame_replay.h" #include "test/fuzz/fuzz_runner.h" @@ -18,7 +17,7 @@ namespace { void Replay(const Frame& frame, ClientCodecFrameInjector& codec) { // Create the client connection containing the nghttp2 session. TestClientConnectionImpl connection( - codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.settings_, + codec.client_connection_, codec.client_callbacks_, codec.stats_store_, codec.options_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT); // Create a new stream. codec.request_encoder_ = &connection.newStream(codec.response_decoder_); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index b967bdb90b584..5cc17a15e906e 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -248,33 +248,35 @@ TEST(HttpUtility, createSslRedirectPath) { namespace { -Http2Settings parseHttp2SettingsFromV2Yaml(const std::string& yaml) { - envoy::config::core::v3::Http2ProtocolOptions http2_protocol_options; - TestUtility::loadFromYamlAndValidate(yaml, http2_protocol_options); - return Utility::parseHttp2Settings(http2_protocol_options); +envoy::config::core::v3::Http2ProtocolOptions parseHttp2OptionsFromV2Yaml(const std::string& yaml) { + envoy::config::core::v3::Http2ProtocolOptions http2_options; + TestUtility::loadFromYamlAndValidate(yaml, http2_options); + return ::Envoy::Http2::Utility::initializeAndValidateOptions(http2_options); } } // namespace TEST(HttpUtility, parseHttp2Settings) { { - auto http2_settings = parseHttp2SettingsFromV2Yaml("{}"); - EXPECT_EQ(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, http2_settings.hpack_table_size_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - http2_settings.max_concurrent_streams_); - EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE, - http2_settings.initial_stream_window_size_); - EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, - http2_settings.initial_connection_window_size_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES, http2_settings.max_outbound_frames_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES, - http2_settings.max_outbound_control_frames_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD, - http2_settings.max_consecutive_inbound_frames_with_empty_payload_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM, - http2_settings.max_inbound_priority_frames_per_stream_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT, - http2_settings.max_inbound_window_update_frames_per_data_frame_sent_); + using ::Envoy::Http2::Utility::OptionsLimits; + auto http2_options = parseHttp2OptionsFromV2Yaml("{}"); + EXPECT_EQ(OptionsLimits::DEFAULT_HPACK_TABLE_SIZE, http2_options.hpack_table_size().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS, + http2_options.max_concurrent_streams().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE, + http2_options.initial_stream_window_size().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, + http2_options.initial_connection_window_size().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES, + http2_options.max_outbound_frames().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES, + http2_options.max_outbound_control_frames().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD, + http2_options.max_consecutive_inbound_frames_with_empty_payload().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM, + http2_options.max_inbound_priority_frames_per_stream().value()); + EXPECT_EQ(OptionsLimits::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT, + http2_options.max_inbound_window_update_frames_per_data_frame_sent().value()); } { @@ -284,11 +286,11 @@ max_concurrent_streams: 2 initial_stream_window_size: 65535 initial_connection_window_size: 65535 )EOF"; - auto http2_settings = parseHttp2SettingsFromV2Yaml(yaml); - EXPECT_EQ(1U, http2_settings.hpack_table_size_); - EXPECT_EQ(2U, http2_settings.max_concurrent_streams_); - EXPECT_EQ(65535U, http2_settings.initial_stream_window_size_); - EXPECT_EQ(65535U, http2_settings.initial_connection_window_size_); + auto http2_options = parseHttp2OptionsFromV2Yaml(yaml); + EXPECT_EQ(1U, http2_options.hpack_table_size().value()); + EXPECT_EQ(2U, http2_options.max_concurrent_streams().value()); + EXPECT_EQ(65535U, http2_options.initial_stream_window_size().value()); + EXPECT_EQ(65535U, http2_options.initial_connection_window_size().value()); } } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index e32fef2f110ec..97e668f6bc8c3 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -294,7 +294,7 @@ TEST_F(StrictDnsClusterImplTest, Basic) { EXPECT_CALL(runtime_.snapshot_, getInteger("circuit_breakers.name.high.max_retries", 4)); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(0U, cluster.info()->http2Settings().hpack_table_size_); + EXPECT_EQ(0U, cluster.info()->http2Options().hpack_table_size().value()); EXPECT_EQ(Http::Http1Settings::HeaderKeyFormat::ProperCase, cluster.info()->http1Settings().header_key_format_); @@ -610,7 +610,7 @@ TEST_F(StrictDnsClusterImplTest, LoadAssignmentBasic) { EXPECT_CALL(runtime_.snapshot_, getInteger("circuit_breakers.name.high.max_retries", 4)); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(0U, cluster.info()->http2Settings().hpack_table_size_); + EXPECT_EQ(0U, cluster.info()->http2Options().hpack_table_size().value()); cluster.info()->stats().upstream_rq_total_.inc(); EXPECT_EQ(1UL, stats_.counter("cluster.name.upstream_rq_total").value()); @@ -1547,8 +1547,8 @@ TEST_F(StaticClusterImplTest, UrlConfig) { EXPECT_EQ(1024U, cluster.info()->resourceManager(ResourcePriority::High).requests().max()); EXPECT_EQ(3U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(0U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - cluster.info()->http2Settings().hpack_table_size_); + EXPECT_EQ(::Envoy::Http2::Utility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE, + cluster.info()->http2Options().hpack_table_size().value()); EXPECT_EQ(LoadBalancerType::Random, cluster.info()->lbType()); EXPECT_THAT( std::list({"10.0.0.1:11001", "10.0.0.2:11002"}), diff --git a/test/config/BUILD b/test/config/BUILD index 9112b43249ac5..5b54ddaf13b1e 100644 --- a/test/config/BUILD +++ b/test/config/BUILD @@ -18,6 +18,7 @@ envoy_cc_test_library( ], deps = [ "//source/common/config:resources_lib", + "//source/common/http:utility_lib", "//source/common/network:address_lib", "//source/common/protobuf", "//source/common/protobuf:utility_lib", diff --git a/test/config/utility.cc b/test/config/utility.cc index e4514f59e1e58..9a72df9cfec4f 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -16,6 +16,7 @@ #include "common/common/assert.h" #include "common/config/resources.h" +#include "common/http/utility.h" #include "common/protobuf/utility.h" #include "test/config/integration/certs/client_ecdsacert_hash.h" @@ -582,8 +583,8 @@ void ConfigHelper::setBufferLimits(uint32_t upstream_buffer_limit, loadHttpConnectionManager(hcm_config); if (hcm_config.codec_type() == envoy::extensions::filters::network::http_connection_manager:: v3::HttpConnectionManager::HTTP2) { - const uint32_t size = - std::max(downstream_buffer_limit, Http::Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE); + const uint32_t size = std::max(downstream_buffer_limit, + Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE); auto* options = hcm_config.mutable_http2_protocol_options(); options->mutable_initial_stream_window_size()->set_value(size); storeHttpConnectionManager(hcm_config); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 1a2b7c6807716..42c66c11070be 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -229,11 +229,13 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio shared_connection_.connection(), store, *this, http1_settings, max_request_headers_kb, max_request_headers_count); } else { - auto settings = Http::Http2Settings(); - settings.allow_connect_ = true; - settings.allow_metadata_ = true; + envoy::config::core::v3::Http2ProtocolOptions http2_options = + ::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions()); + http2_options.set_allow_connect(true); + http2_options.set_allow_metadata(true); codec_ = std::make_unique( - shared_connection_.connection(), *this, store, settings, max_request_headers_kb, + shared_connection_.connection(), *this, store, http2_options, max_request_headers_kb, max_request_headers_count); ASSERT(type == Type::HTTP2); } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index b827486643a30..44ed65e7c2f18 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -205,8 +205,8 @@ IntegrationCodecClientPtr HttpIntegrationTest::makeRawHttpConnection(Network::ClientConnectionPtr&& conn) { std::shared_ptr cluster{new NiceMock()}; cluster->max_response_headers_count_ = 200; - cluster->http2_settings_.allow_connect_ = true; - cluster->http2_settings_.allow_metadata_ = true; + cluster->http2_options_.set_allow_connect(true); + cluster->http2_options_.set_allow_metadata(true); cluster->http1_settings_.enable_trailers_ = true; Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription( cluster, fmt::format("tcp://{}:80", Network::Test::getLoopbackAddressUrlString(version_)))}; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 7ba5b38064f9a..fa17b1dd57621 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -18,6 +18,7 @@ #include "common/common/fmt.h" #include "common/common/thread_annotations.h" #include "common/http/headers.h" +#include "common/http/utility.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" #include "common/runtime/runtime_impl.h" @@ -1183,7 +1184,7 @@ name: passthrough-filter [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { hcm.mutable_http2_protocol_options()->mutable_initial_stream_window_size()->set_value( - Http::Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE); + ::Envoy::Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE); }); initialize(); @@ -1321,7 +1322,7 @@ name: encode-headers-return-stop-all-filter [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { hcm.mutable_http2_protocol_options()->mutable_initial_stream_window_size()->set_value( - Http::Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE); + ::Envoy::Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE); }); initialize(); diff --git a/test/mocks/upstream/BUILD b/test/mocks/upstream/BUILD index 6d3be18e174b4..7319dbfb9ad54 100644 --- a/test/mocks/upstream/BUILD +++ b/test/mocks/upstream/BUILD @@ -17,6 +17,7 @@ envoy_cc_mock( "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/upstream:upstream_interface", "//source/common/config:metadata_lib", + "//source/common/http:utility_lib", "//source/common/network:raw_buffer_socket_lib", "//source/common/upstream:upstream_includes", "//source/common/upstream:upstream_lib", diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index 1a325c4ac9af4..55b0fffbb2b86 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -7,6 +7,7 @@ #include "envoy/upstream/upstream.h" #include "common/config/metadata.h" +#include "common/http/utility.h" #include "common/network/raw_buffer_socket.h" #include "common/upstream/upstream_impl.h" @@ -36,7 +37,9 @@ MockIdleTimeEnabledClusterInfo::MockIdleTimeEnabledClusterInfo() { MockIdleTimeEnabledClusterInfo::~MockIdleTimeEnabledClusterInfo() = default; MockClusterInfo::MockClusterInfo() - : stats_(ClusterInfoImpl::generateStats(stats_store_)), + : http2_options_(::Envoy::Http2::Utility::initializeAndValidateOptions( + envoy::config::core::v3::Http2ProtocolOptions())), + stats_(ClusterInfoImpl::generateStats(stats_store_)), transport_socket_matcher_(new NiceMock()), load_report_stats_(ClusterInfoImpl::generateLoadReportStats(load_report_stats_store_)), timeout_budget_stats_(absl::make_optional( @@ -51,7 +54,7 @@ MockClusterInfo::MockClusterInfo() ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); ON_CALL(*this, eds_service_name()).WillByDefault(ReturnPointee(&eds_service_name_)); ON_CALL(*this, http1Settings()).WillByDefault(ReturnRef(http1_settings_)); - ON_CALL(*this, http2Settings()).WillByDefault(ReturnRef(http2_settings_)); + ON_CALL(*this, http2Options()).WillByDefault(ReturnRef(http2_options_)); ON_CALL(*this, extensionProtocolOptions(_)).WillByDefault(Return(extension_protocol_options_)); ON_CALL(*this, maxResponseHeadersCount()) .WillByDefault(ReturnPointee(&max_response_headers_count_)); diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index ca238fc6da9bf..6e2e8c3b113f5 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -89,7 +89,7 @@ class MockClusterInfo : public ClusterInfo { MOCK_METHOD(uint32_t, perConnectionBufferLimitBytes, (), (const)); MOCK_METHOD(uint64_t, features, (), (const)); MOCK_METHOD(const Http::Http1Settings&, http1Settings, (), (const)); - MOCK_METHOD(const Http::Http2Settings&, http2Settings, (), (const)); + MOCK_METHOD(const envoy::config::core::v3::Http2ProtocolOptions&, http2Options, (), (const)); MOCK_METHOD(ProtocolOptionsConfigConstSharedPtr, extensionProtocolOptions, (const std::string&), (const)); MOCK_METHOD(const envoy::config::cluster::v3::Cluster::CommonLbConfig&, lbConfig, (), (const)); @@ -130,7 +130,7 @@ class MockClusterInfo : public ClusterInfo { std::string name_{"fake_cluster"}; absl::optional eds_service_name_; Http::Http1Settings http1_settings_; - Http::Http2Settings http2_settings_; + envoy::config::core::v3::Http2ProtocolOptions http2_options_; ProtocolOptionsConfigConstSharedPtr extension_protocol_options_; uint64_t max_requests_per_connection_{}; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 0281a62049df9..e564dae4be978 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -328,18 +328,6 @@ constexpr std::chrono::milliseconds TestUtility::DefaultTimeout; namespace Http { -// Satisfy linker -const uint32_t Http2Settings::DEFAULT_HPACK_TABLE_SIZE; -const uint32_t Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS; -const uint32_t Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE; -const uint32_t Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE; -const uint32_t Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE; -const uint32_t Http2Settings::DEFAULT_MAX_OUTBOUND_FRAMES; -const uint32_t Http2Settings::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES; -const uint32_t Http2Settings::DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD; -const uint32_t Http2Settings::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM; -const uint32_t Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT; - TestHeaderMapImpl::TestHeaderMapImpl() = default; TestHeaderMapImpl::TestHeaderMapImpl( From 28cdf4de6d5def32205bdae335ee17d2e0c8271b Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 5 Feb 2020 23:41:13 -0500 Subject: [PATCH 02/31] Support custom H/2 settings parameters. Custom parameters override named parameters, enabling forward compatibility. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 22 ++- api/envoy/config/core/v3/protocol.proto | 34 ++-- .../envoy/api/v2/core/protocol.proto | 22 ++- .../envoy/config/core/v3/protocol.proto | 25 ++- source/common/http/http2/codec_impl.cc | 75 +++++--- source/common/http/http2/codec_impl.h | 14 ++ .../quiche/envoy_quic_dispatcher.cc | 4 +- test/common/http/http2/codec_impl_test.cc | 180 +++++++++++++++++- .../http/http2/response_header_fuzz_test.cc | 1 + 9 files changed, 328 insertions(+), 49 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index c8b8cdc6dd354..a200003e7fec1 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -106,8 +106,18 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { + // Defines a parameter to be sent in the SETTINGS frame. + // See `RFC7540, sec. 6.5.1 `_ for details. + message SettingsParameter { + // The 16 bit parameter identifier. + google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + + // The 32 bit parameter value. + google.protobuf.UInt32Value value = 2; + } + // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values // range from 0 to 4294967295 (2^32 - 1) and defaults to 4096. 0 effectively disables header @@ -206,6 +216,16 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specifies SETTINGS frame parameters to be sent to the peer. + // Note that parameters specified through this config will override named parameters with matching + // identifiers. + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified + // in :ref:`hpack_table_size`. + // See `IANA HTTP/2 Settings + // `_ for + // standardized identifiers. + repeated SettingsParameter custom_settings_parameters = 13; } // [#not-implemented-hide:] diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 4baae1f2bf35e..9470619ac9d59 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -122,11 +122,24 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; + // Defines a parameter to be sent in the SETTINGS frame. + // See `RFC7540, sec. 6.5.1 `_ for details. + message SettingsParameter { + option (udpa.annotations.versioning).previous_message_type = + "envoy.api.v2.core.Http2ProtocolOptions.SettingsParameter"; + + // The 16 bit parameter identifier. + google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + + // The 32 bit parameter value. + google.protobuf.UInt32Value value = 2; + } + // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values // range from 0 to 4294967295 (2^32 - 1) and defaults to 4096. 0 effectively disables header @@ -226,17 +239,14 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Defines a parameter to be sent in the SETTINGS frame. - // See `RFC7540, sec. 6.5.1 `_ for details. - message SettingsParameter { - // The 16 bit parameter identifier. - google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {gte: 1 lte: 65536}]; - - // The 32 bit parameter value. - google.protobuf.UInt32Value value = 2; - } - - // TODO(AndresGuedez) + // Specifies SETTINGS frame parameters to be sent to the peer. + // Note that parameters specified through this config will override named parameters with matching + // identifiers. + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified + // in :ref:`hpack_table_size`. + // See `IANA HTTP/2 Settings + // `_ for + // standardized identifiers. repeated SettingsParameter custom_settings_parameters = 13; } diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index c8b8cdc6dd354..a200003e7fec1 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -106,8 +106,18 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { + // Defines a parameter to be sent in the SETTINGS frame. + // See `RFC7540, sec. 6.5.1 `_ for details. + message SettingsParameter { + // The 16 bit parameter identifier. + google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + + // The 32 bit parameter value. + google.protobuf.UInt32Value value = 2; + } + // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values // range from 0 to 4294967295 (2^32 - 1) and defaults to 4096. 0 effectively disables header @@ -206,6 +216,16 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specifies SETTINGS frame parameters to be sent to the peer. + // Note that parameters specified through this config will override named parameters with matching + // identifiers. + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified + // in :ref:`hpack_table_size`. + // See `IANA HTTP/2 Settings + // `_ for + // standardized identifiers. + repeated SettingsParameter custom_settings_parameters = 13; } // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index dbbb4318e0cfe..9470619ac9d59 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -122,11 +122,24 @@ message Http1ProtocolOptions { bool enable_trailers = 5; } -// [#next-free-field: 13] +// [#next-free-field: 14] message Http2ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http2ProtocolOptions"; + // Defines a parameter to be sent in the SETTINGS frame. + // See `RFC7540, sec. 6.5.1 `_ for details. + message SettingsParameter { + option (udpa.annotations.versioning).previous_message_type = + "envoy.api.v2.core.Http2ProtocolOptions.SettingsParameter"; + + // The 16 bit parameter identifier. + google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + + // The 32 bit parameter value. + google.protobuf.UInt32Value value = 2; + } + // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values // range from 0 to 4294967295 (2^32 - 1) and defaults to 4096. 0 effectively disables header @@ -225,6 +238,16 @@ message Http2ProtocolOptions { // // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + + // Specifies SETTINGS frame parameters to be sent to the peer. + // Note that parameters specified through this config will override named parameters with matching + // identifiers. + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified + // in :ref:`hpack_table_size`. + // See `IANA HTTP/2 Settings + // `_ for + // standardized identifiers. + repeated SettingsParameter custom_settings_parameters = 13; } // [#not-implemented-hide:] diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 0c5481fbc1858..7154b383949f8 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -475,6 +475,13 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { return 0; } + if (frame->hd.type == NGHTTP2_SETTINGS && frame->hd.flags == NGHTTP2_FLAG_NONE) { + if (test_only_on_settings_frame_cb_) { + ENVOY_CONN_LOG(trace, "issuing settings cb", connection_); + test_only_on_settings_frame_cb_(frame->settings); + } + } + StreamImpl* stream = getStream(frame->hd.stream_id); if (!stream) { return 0; @@ -838,36 +845,50 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { - std::vector iv; - if (http2_options.allow_connect()) { - iv.push_back({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, 1}); - } - const uint32_t hpack_table_size = http2_options.hpack_table_size().value(); - if (hpack_table_size != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { - iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, hpack_table_size}); - ENVOY_CONN_LOG(debug, "setting HPACK table size to {}", connection_, hpack_table_size); - } - const uint32_t max_concurrent_streams = http2_options.max_concurrent_streams().value(); - if (max_concurrent_streams != NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS) { - iv.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, max_concurrent_streams}); - ENVOY_CONN_LOG(debug, "setting max concurrent streams to {}", connection_, - max_concurrent_streams); - } - const uint32_t initial_stream_window_size = http2_options.initial_stream_window_size().value(); - if (initial_stream_window_size != NGHTTP2_INITIAL_WINDOW_SIZE) { - iv.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, initial_stream_window_size}); - ENVOY_CONN_LOG(debug, "setting stream-level initial window size to {}", connection_, - initial_stream_window_size); + std::unordered_set settings; + for (const auto it : http2_options.custom_settings_parameters()) { + ASSERT(it.identifier().value() <= std::numeric_limits::max()); + auto result = + settings.insert({static_cast(it.identifier().value()), it.value().value()}); + if (!result.second) { + ENVOY_CONN_LOG(debug, "duplicate user defined settings parameter with id {:#x}, value {}", + connection_, it.identifier().value(), it.value().value()); + continue; + } + ENVOY_CONN_LOG(debug, "setting user defined settings parameter with id {:#x} to {}", + connection_, it.identifier().value(), it.value().value()); } - if (disable_push) { - // Universally disable receiving push promise frames as we don't currently support them. nghttp2 - // will fail the connection if the other side still sends them. - // TODO(mattklein123): Remove this when we correctly proxy push promise. - iv.push_back({NGHTTP2_SETTINGS_ENABLE_PUSH, 0}); + struct SettingsDescriptor { + nghttp2_settings_entry entry; + uint32_t default_value; + }; + for (const auto& descriptor : std::vector{ + {{NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}, 0}, + {{NGHTTP2_SETTINGS_ENABLE_PUSH, disable_push ? 0U : 1U}, 1}, + {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, + NGHTTP2_DEFAULT_HEADER_TABLE_SIZE}, + {{NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, + http2_options.max_concurrent_streams().value()}, + NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS}, + {{NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, + http2_options.initial_stream_window_size().value()}, + NGHTTP2_INITIAL_WINDOW_SIZE}}) { + if (descriptor.entry.value != descriptor.default_value) { + auto result = settings.insert(descriptor.entry); + if (!result.second) { + ENVOY_CONN_LOG(debug, "duplicate named settings parameter with id {:#x}, value {}", + connection_, descriptor.entry.settings_id, descriptor.entry.value); + continue; + } + ENVOY_CONN_LOG(debug, "setting named settings parameter with id {:#x} to {}", connection_, + descriptor.entry.settings_id, descriptor.entry.value); + } } - if (!iv.empty()) { + if (!settings.empty()) { + std::vector iv; + std::copy(settings.begin(), settings.end(), std::back_inserter(iv)); int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); ASSERT(rc == 0); } else { @@ -887,7 +908,7 @@ void ConnectionImpl::sendSettings( NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); } -} +} // namespace Http2 ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_new(&callbacks_); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 0fbb522a59d9e..f8407a1a66d42 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -356,6 +356,19 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable()(entry.settings_id); + } + }; + struct SettingsEntryEquals { + bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { + return lhs.settings_id == rhs.settings_id; + } + }; + virtual ConnectionCallbacks& callbacks() PURE; virtual int onBeginHeaders(const nghttp2_frame* frame) PURE; int onData(int32_t stream_id, const uint8_t* data, size_t len); @@ -391,6 +404,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable test_only_on_settings_frame_cb_; }; /** diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc b/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc index 9128b8f651824..409d0a92c08bc 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc @@ -1,10 +1,10 @@ #include "extensions/quic_listeners/quiche/envoy_quic_dispatcher.h" +#include "common/http/utility.h" + #include "extensions/quic_listeners/quiche/envoy_quic_server_connection.h" #include "extensions/quic_listeners/quiche/envoy_quic_server_session.h" -#include "common/http/utility.h" - namespace Envoy { namespace Quic { diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 7792f71532961..e77e7e3a92c86 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -40,6 +40,37 @@ using Http2SettingsTuple = ::testing::tuple; namespace CommonUtility = ::Envoy::Http2::Utility; +class ConnectionImplTestPeer { +public: + void registerPeer(ConnectionImpl& connection) { + connection.test_only_on_settings_frame_cb_ = + std::bind(&ConnectionImplTestPeer::onSettingsFrame, this, std::placeholders::_1); + } + + void onSettingsFrame(const nghttp2_settings& settings_frame) { + ENVOY_LOG_MISC(error, "callback issued"); + for (uint32_t i = 0; i < settings_frame.niv; ++i) { + ENVOY_LOG_MISC(error, "adding setting id {} val {}", settings_frame.iv[i].settings_id, + settings_frame.iv[i].value); + auto result = settings_.insert(settings_frame.iv[i]); + ASSERT(result.second); + } + } + + absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { + const auto it = settings_.find({identifier, 0}); + if (it == settings_.end()) { + return absl::nullopt; + } + return it->value; + } + +private: + std::unordered_set + settings_; +}; + class Http2CodecImplTestFixture { public: struct ConnectionWrapper { @@ -58,19 +89,33 @@ class Http2CodecImplTestFixture { Buffer::OwnedImpl buffer_; }; + enum SettingsTupleIndex { + HpackTableSize = 0, + MaxConcurrentStreams, + InitialStreamWindowSize, + InitialConnectionWindowSize + }; + Http2CodecImplTestFixture(Http2SettingsTuple client_settings, Http2SettingsTuple server_settings) : client_settings_(client_settings), server_settings_(server_settings) {} virtual ~Http2CodecImplTestFixture() = default; - virtual void initialize() { + virtual void initialize(ConnectionImplTestPeer* client_test_peer = nullptr, + ConnectionImplTestPeer* server_test_peer = nullptr) { Http2OptionsFromTuple(client_http2_options_, client_settings_); Http2OptionsFromTuple(server_http2_options_, server_settings_); client_ = std::make_unique( client_connection_, client_callbacks_, stats_store_, client_http2_options_, max_request_headers_kb_, max_response_headers_count_); + if (client_test_peer != nullptr) { + client_test_peer->registerPeer(*client_); + } server_ = std::make_unique( server_connection_, server_callbacks_, stats_store_, server_http2_options_, max_request_headers_kb_, max_request_headers_count_); + if (server_test_peer != nullptr) { + server_test_peer->registerPeer(*server_); + } request_encoder_ = &client_->newStream(response_decoder_); setupDefaultConnectionMocks(); @@ -99,10 +144,14 @@ class Http2CodecImplTestFixture { void Http2OptionsFromTuple(envoy::config::core::v3::Http2ProtocolOptions& options, const Http2SettingsTuple& tp) { - options.mutable_hpack_table_size()->set_value(::testing::get<0>(tp)); - options.mutable_max_concurrent_streams()->set_value(::testing::get<1>(tp)); - options.mutable_initial_stream_window_size()->set_value(::testing::get<2>(tp)); - options.mutable_initial_connection_window_size()->set_value(::testing::get<3>(tp)); + options.mutable_hpack_table_size()->set_value( + ::testing::get(tp)); + options.mutable_max_concurrent_streams()->set_value( + ::testing::get(tp)); + options.mutable_initial_stream_window_size()->set_value( + ::testing::get(tp)); + options.mutable_initial_connection_window_size()->set_value( + ::testing::get(tp)); options.set_allow_metadata(allow_metadata_); options.set_stream_error_on_invalid_http_messaging(stream_error_on_invalid_http_messaging_); options.mutable_max_outbound_frames()->set_value(max_outbound_frames_); @@ -923,6 +972,9 @@ TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) { response_encoder_->encodeHeaders(response_headers, true); } +class Http2CodecImplSettingsBasicTest : public Http2CodecImplTest {}; +TEST_P(Http2CodecImplSettingsBasicTest, ) {} + class Http2CodecImplStreamLimitTest : public Http2CodecImplTest {}; // Regression test for issue #3076. @@ -1048,6 +1100,124 @@ TEST(Http2CodecUtility, reconstituteCrumbledCookies) { } } +MATCHER_P(HasValue, m, "") { + if (!arg.has_value()) { + *result_listener << "does not contain a value"; + return false; + } + const auto& value = arg.value(); + return ExplainMatchResult(m, value, result_listener); +}; + +class Http2CustomSettingsTest : public ::testing::TestWithParam< + ::testing::tuple>, + public Http2CodecImplTestFixture { +public: + struct SettingsParameter { + uint16_t identifier; + uint32_t value; + }; + + Http2CustomSettingsTest() + : Http2CodecImplTestFixture(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam())), + client_peer_(::testing::get<2>(GetParam())) {} + + // Sets the custom settings parameters specified by |parameters| in the |options| proto. + void setHttp2CustomSettingsParameters(envoy::config::core::v3::Http2ProtocolOptions& options, + std::vector parameters) { + for (const auto& parameter : parameters) { + envoy::config::core::v3::Http2ProtocolOptions::SettingsParameter* custom_param = + options.mutable_custom_settings_parameters()->Add(); + custom_param->mutable_identifier()->set_value(parameter.identifier); + custom_param->mutable_value()->set_value(parameter.value); + } + } + + // Returns the Http2ProtocolOptions proto which specifies the settings parameters to be sent to + // the peer under test. + envoy::config::core::v3::Http2ProtocolOptions& getCustomOptions() { + return client_peer_ ? server_http2_options_ : client_http2_options_; + } + + // Returns the peer under test. + ConnectionImplTestPeer* getPeer() { + return client_peer_ ? &client_test_peer_ : &server_test_peer_; + } + + // Returns the settings tuple which specifies a subset of the settings parameters to be sent to + // the peer under test. + const Http2SettingsTuple& getSettingsTuple() { + return client_peer_ ? server_settings_ : client_settings_; + } + +protected: + bool client_peer_{false}; + ConnectionImplTestPeer client_test_peer_; + ConnectionImplTestPeer server_test_peer_; +}; + +INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestEdgeSettings, Http2CustomSettingsTest, + ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE, + HTTP2SETTINGS_DEFAULT_COMBINE, ::testing::Bool())); + +// Validates that custom parameters (those which are not explicitly named in the +// envoy::config::core::v3::Http2ProtocolOptions proto) are properly sent and processed by client +// and server connections. +TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { + std::vector custom_parameters{{0x10, 10}, {0x11, 20}}; + setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); + initialize(&client_test_peer_, &server_test_peer_); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); + request_encoder_->encodeHeaders(request_headers, false); + uint32_t hpack_table_size = + ::testing::get(getSettingsTuple()); + if (hpack_table_size != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { + EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE), + HasValue(hpack_table_size)); + } + uint32_t max_concurrent_streams = + ::testing::get(getSettingsTuple()); + if (max_concurrent_streams != NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS) { + EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS), + HasValue(max_concurrent_streams)); + } + uint32_t initial_stream_window_size = + ::testing::get(getSettingsTuple()); + if (max_concurrent_streams != NGHTTP2_INITIAL_WINDOW_SIZE) { + EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE), + HasValue(initial_stream_window_size)); + } + // Validate that custom parameters are received by the endpoint (client or server) under test. + for (const auto& parameter : custom_parameters) { + EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(parameter.identifier), + HasValue(parameter.value)); + } +} + +// Validates that custom settings parameters override named parameter values (both when these are +// defaulted or explicitly set). +TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParameters) { + // These settings override named parameters which are defaulted when not specified via + // configuration. + std::vector named_parameter_overrides{ + {NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, + CommonUtility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE - 1}, + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, + CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS - 1}}; + setHttp2CustomSettingsParameters(getCustomOptions(), named_parameter_overrides); + initialize(&client_test_peer_, &server_test_peer_); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); + request_encoder_->encodeHeaders(request_headers, false); + for (const auto& parameter : named_parameter_overrides) { + EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(parameter.identifier), + HasValue(parameter.value)); + } +} + // Tests request headers whose size is larger than the default limit of 60K. TEST_P(Http2CodecImplTest, LargeRequestHeadersInvokeResetStream) { initialize(); diff --git a/test/common/http/http2/response_header_fuzz_test.cc b/test/common/http/http2/response_header_fuzz_test.cc index bf2421ebfcbd9..ca8209645100a 100644 --- a/test/common/http/http2/response_header_fuzz_test.cc +++ b/test/common/http/http2/response_header_fuzz_test.cc @@ -3,6 +3,7 @@ // uncompressed HEADERS fuzzing. #include "common/http/exception.h" + #include "test/common/http/common.h" #include "test/common/http/http2/frame_replay.h" #include "test/fuzz/fuzz_runner.h" From bad389ccf113218e501c9a9d9e091f2346db2f1f Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Feb 2020 14:59:13 -0500 Subject: [PATCH 03/31] Add Http2ProtocolOptions cluster config test. Some minor cleanup as well. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 34 +++++++++++++--------- test/common/upstream/upstream_impl_test.cc | 28 ++++++++++++++++++ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 7154b383949f8..de3ba5ed04be1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -846,6 +846,7 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { std::unordered_set settings; + // Custom parameters override named parameters, so they must be inserted first into the set. for (const auto it : http2_options.custom_settings_parameters()) { ASSERT(it.identifier().value() <= std::numeric_limits::max()); auto result = @@ -855,25 +856,30 @@ void ConnectionImpl::sendSettings( connection_, it.identifier().value(), it.value().value()); continue; } - ENVOY_CONN_LOG(debug, "setting user defined settings parameter with id {:#x} to {}", - connection_, it.identifier().value(), it.value().value()); + ENVOY_CONN_LOG(debug, "adding user defined settings parameter with id {:#x} to {}", connection_, + it.identifier().value(), it.value().value()); } struct SettingsDescriptor { nghttp2_settings_entry entry; uint32_t default_value; }; - for (const auto& descriptor : std::vector{ - {{NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}, 0}, - {{NGHTTP2_SETTINGS_ENABLE_PUSH, disable_push ? 0U : 1U}, 1}, - {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, - NGHTTP2_DEFAULT_HEADER_TABLE_SIZE}, - {{NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - http2_options.max_concurrent_streams().value()}, - NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS}, - {{NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - http2_options.initial_stream_window_size().value()}, - NGHTTP2_INITIAL_WINDOW_SIZE}}) { + auto descriptors = std::vector{ + {{/*nghttp2_settings_entry.settings_id=*/NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, + /*nghttp2_settings_entry.value=*/http2_options.allow_connect()}, + /*default_value=*/0}, + {// Universally disable receiving push promise frames as we don't currently support + // them. nghttp2 will fail the connection if the other side still sends them. + // TODO(mattklein123): Remove this when we correctly proxy push promise. + {NGHTTP2_SETTINGS_ENABLE_PUSH, disable_push ? 0U : 1U}, + 1}, + {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, + NGHTTP2_DEFAULT_HEADER_TABLE_SIZE}, + {{NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()}, + NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS}, + {{NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()}, + NGHTTP2_INITIAL_WINDOW_SIZE}}; + for (const auto& descriptor : descriptors) { if (descriptor.entry.value != descriptor.default_value) { auto result = settings.insert(descriptor.entry); if (!result.second) { @@ -881,7 +887,7 @@ void ConnectionImpl::sendSettings( connection_, descriptor.entry.settings_id, descriptor.entry.value); continue; } - ENVOY_CONN_LOG(debug, "setting named settings parameter with id {:#x} to {}", connection_, + ENVOY_CONN_LOG(debug, "adding named settings parameter with id {:#x} to {}", connection_, descriptor.entry.settings_id, descriptor.entry.value); } } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 97e668f6bc8c3..511aac812aea1 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -2132,6 +2132,34 @@ TEST_F(ClusterInfoImplTest, TestTrackTimeoutBudgets) { cluster->info()->timeoutBudgetStats()->upstream_rq_timeout_budget_percent_used_.unit()); } +// Validates HTTP2 SETTINGS config. +TEST_F(ClusterInfoImplTest, Http2ProtocolOptions) { + const std::string yaml = R"EOF( + name: name + connect_timeout: 0.25s + type: STRICT_DNS + lb_policy: ROUND_ROBIN + http2_protocol_options: + hpack_table_size: 2048 + initial_stream_window_size: 65536 + custom_settings_parameters: + - identifier: 0x10 + value: 10 + - identifier: 0x12 + value: 12 + )EOF"; + + auto cluster = makeCluster(yaml); + EXPECT_EQ(cluster->info()->http2Options().hpack_table_size().value(), 2048); + EXPECT_EQ(cluster->info()->http2Options().initial_stream_window_size().value(), 65536); + EXPECT_EQ(cluster->info()->http2Options().custom_settings_parameters()[0].identifier().value(), + 0x10); + EXPECT_EQ(cluster->info()->http2Options().custom_settings_parameters()[0].value().value(), 10); + EXPECT_EQ(cluster->info()->http2Options().custom_settings_parameters()[1].identifier().value(), + 0x12); + EXPECT_EQ(cluster->info()->http2Options().custom_settings_parameters()[1].value().value(), 12); +} + class TestFilterConfigFactoryBase { public: TestFilterConfigFactoryBase( From 4b78a165b61a71bd08d6a48d46d163cef5d91083 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Feb 2020 22:42:14 -0500 Subject: [PATCH 04/31] Cleanup. Signed-off-by: Andres Guedez --- source/common/http/utility.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 99b3bce2e9a13..692f337da5aa8 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -22,6 +22,7 @@ namespace Envoy { namespace Http2 { namespace Utility { +// Limits and defaults for `envoy::config::core::v3::Http2ProtocolOptions` protos. struct OptionsLimits { // disable HPACK compression static const uint32_t MIN_HPACK_TABLE_SIZE = 0; @@ -69,7 +70,8 @@ struct OptionsLimits { }; /** - * TODO(AndresGuedez): + * Validates settings/options already set in |options| and initializes any remaining fields with + * defaults. */ envoy::config::core::v3::Http2ProtocolOptions initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions& options); From 0f504014c631009716250beee0a768dfbf857ec5 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Feb 2020 23:13:10 -0500 Subject: [PATCH 05/31] Cleanup. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- test/common/http/http2/codec_impl_test.cc | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index de3ba5ed04be1..97723549e34d0 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -475,9 +475,9 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { return 0; } + // Only for tests, issue a callback when the SETTINGS frame is received. if (frame->hd.type == NGHTTP2_SETTINGS && frame->hd.flags == NGHTTP2_FLAG_NONE) { if (test_only_on_settings_frame_cb_) { - ENVOY_CONN_LOG(trace, "issuing settings cb", connection_); test_only_on_settings_frame_cb_(frame->settings); } } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index e77e7e3a92c86..f9d0665eadcc7 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -42,11 +42,13 @@ namespace CommonUtility = ::Envoy::Http2::Utility; class ConnectionImplTestPeer { public: + // Registers callbacks from this peer with |connection|. void registerPeer(ConnectionImpl& connection) { connection.test_only_on_settings_frame_cb_ = std::bind(&ConnectionImplTestPeer::onSettingsFrame, this, std::placeholders::_1); } + // Callback issued when nghttp2 processes a SETTINGS frame. void onSettingsFrame(const nghttp2_settings& settings_frame) { ENVOY_LOG_MISC(error, "callback issued"); for (uint32_t i = 0; i < settings_frame.niv; ++i) { @@ -57,6 +59,7 @@ class ConnectionImplTestPeer { } } + // Returns the value of the SETTINGS parameter with |identifier|. absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { const auto it = settings_.find({identifier, 0}); if (it == settings_.end()) { From 16b8ba7cb6e39c8a5eac61204895693979ea2ea9 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 7 Feb 2020 09:40:36 -0500 Subject: [PATCH 06/31] Fix comment. Signed-off-by: Andres Guedez --- test/common/http/http2/codec_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index f9d0665eadcc7..f63250fc25485 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -59,7 +59,7 @@ class ConnectionImplTestPeer { } } - // Returns the value of the SETTINGS parameter with |identifier|. + // Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint. absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { const auto it = settings_.find({identifier, 0}); if (it == settings_.end()) { From 08ab741e6a21dc7dd27e931e829fb9443d5cfbf1 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 10 Feb 2020 23:02:10 -0500 Subject: [PATCH 07/31] Fix format. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 97723549e34d0..99b2c3191fad1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -869,7 +869,7 @@ void ConnectionImpl::sendSettings( /*nghttp2_settings_entry.value=*/http2_options.allow_connect()}, /*default_value=*/0}, {// Universally disable receiving push promise frames as we don't currently support - // them. nghttp2 will fail the connection if the other side still sends them. + // them. nghttp2 will fail the connection if the other side still sends them. // TODO(mattklein123): Remove this when we correctly proxy push promise. {NGHTTP2_SETTINGS_ENABLE_PUSH, disable_push ? 0U : 1U}, 1}, From b553ef59d8943da7eda6fec37858b4280fb034a6 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 13:03:26 -0500 Subject: [PATCH 08/31] Fix docs. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 9 ++++++--- api/envoy/config/core/v3/protocol.proto | 6 ++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index a200003e7fec1..0e5a9b0a6670f 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -217,11 +217,14 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer. + // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: + // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. // Note that parameters specified through this config will override named parameters with matching // identifiers. - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified - // in :ref:`hpack_table_size`. + // + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in + // :ref:`hpack_table_size `. + // // See `IANA HTTP/2 Settings // `_ for // standardized identifiers. diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 9470619ac9d59..4454443276bfc 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -242,8 +242,10 @@ message Http2ProtocolOptions { // Specifies SETTINGS frame parameters to be sent to the peer. // Note that parameters specified through this config will override named parameters with matching // identifiers. - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified - // in :ref:`hpack_table_size`. + // + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in + // :ref:`hpack_table_size `. + // // See `IANA HTTP/2 Settings // `_ for // standardized identifiers. From fdc7c9b91f766352e2e0c3ca3710e5a76dd82774 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 13:04:36 -0500 Subject: [PATCH 09/31] Refactor SETTINGS frame received callback handling. Also, disallow custom SETTINGS parameters from overriding server push as it is not supported by Envoy. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 50 ++++++--- source/common/http/http2/codec_impl.h | 29 ++--- test/common/http/http2/codec_impl_test.cc | 105 +++++++----------- test/common/http/http2/codec_impl_test_util.h | 42 ++++++- 4 files changed, 130 insertions(+), 96 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 99b2c3191fad1..598ac6fdde056 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -475,11 +475,8 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { return 0; } - // Only for tests, issue a callback when the SETTINGS frame is received. if (frame->hd.type == NGHTTP2_SETTINGS && frame->hd.flags == NGHTTP2_FLAG_NONE) { - if (test_only_on_settings_frame_cb_) { - test_only_on_settings_frame_cb_(frame->settings); - } + onSettings(frame->settings); } StreamImpl* stream = getStream(frame->hd.stream_id); @@ -845,10 +842,30 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { + // Custom parameters override named parameters, so they must be inserted first into this set. std::unordered_set settings; - // Custom parameters override named parameters, so they must be inserted first into the set. + + // Universally disable receiving push promise frames as we don't currently support + // them. nghttp2 will fail the connection if the other side still sends them. + // TODO(mattklein123): Remove this when we correctly proxy push promise. + // NOTE: This is a special case with respect to custom parameter overrides in that server push is + // not supported and therefore not end user configurable. + if (disable_push) { + settings.insert({static_cast(NGHTTP2_SETTINGS_ENABLE_PUSH), disable_push ? 0U : 1U}); + } + for (const auto it : http2_options.custom_settings_parameters()) { ASSERT(it.identifier().value() <= std::numeric_limits::max()); + if (it.identifier().value() == NGHTTP2_SETTINGS_ENABLE_PUSH && it.value().value() == 1) { + // For simplicity, warn when server push is configured enabled on either client or server + // connections. It _is_ actually enabled on downstream connections although Envoy itself will + // never send push_promise frames, but the nuance could be confusing to end users. + ENVOY_CONN_LOG( + warn, + "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter.", + connection_); + continue; + } auto result = settings.insert({static_cast(it.identifier().value()), it.value().value()}); if (!result.second) { @@ -868,11 +885,6 @@ void ConnectionImpl::sendSettings( {{/*nghttp2_settings_entry.settings_id=*/NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, /*nghttp2_settings_entry.value=*/http2_options.allow_connect()}, /*default_value=*/0}, - {// Universally disable receiving push promise frames as we don't currently support - // them. nghttp2 will fail the connection if the other side still sends them. - // TODO(mattklein123): Remove this when we correctly proxy push promise. - {NGHTTP2_SETTINGS_ENABLE_PUSH, disable_push ? 0U : 1U}, - 1}, {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, NGHTTP2_DEFAULT_HEADER_TABLE_SIZE}, {{NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()}, @@ -1028,13 +1040,14 @@ ConnectionImpl::Http2Options::Http2Options( nghttp2_option_new(&options_); // Currently we do not do anything with stream priority. Setting the following option prevents // nghttp2 from keeping around closed streams for use during stream priority dependency graph - // calculations. This saves a tremendous amount of memory in cases where there are a large number - // of kept alive HTTP/2 connections. + // calculations. This saves a tremendous amount of memory in cases where there are a large + // number of kept alive HTTP/2 connections. nghttp2_option_set_no_closed_streams(options_, 1); nghttp2_option_set_no_auto_window_update(options_, 1); // The max send header block length is configured to an arbitrarily high number so as to never - // trigger the check within nghttp2, as we check request headers length in codec_impl::saveHeader. + // trigger the check within nghttp2, as we check request headers length in + // codec_impl::saveHeader. nghttp2_option_set_max_send_header_block_length(options_, 0x2000000); if (http2_options.hpack_table_size().value() != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { @@ -1046,11 +1059,12 @@ ConnectionImpl::Http2Options::Http2Options( nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE); } - // nghttp2 v1.39.2 lowered the internal flood protection limit from 10K to 1K of ACK frames. This - // new limit may cause the internal nghttp2 mitigation to trigger more often (as it requires just - // 9K of incoming bytes for smallest 9 byte SETTINGS frame), bypassing the same mitigation and its - // associated behavior in the envoy HTTP/2 codec. Since envoy does not rely on this mitigation, - // set back to the old 10K number to avoid any changes in the HTTP/2 codec behavior. + // nghttp2 v1.39.2 lowered the internal flood protection limit from 10K to 1K of ACK frames. + // This new limit may cause the internal nghttp2 mitigation to trigger more often (as it + // requires just 9K of incoming bytes for smallest 9 byte SETTINGS frame), bypassing the same + // mitigation and its associated behavior in the envoy HTTP/2 codec. Since envoy does not rely + // on this mitigation, set back to the old 10K number to avoid any changes in the HTTP/2 codec + // behavior. nghttp2_option_set_max_outbound_ack(options_, 10000); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index f8407a1a66d42..710fb2e97f69e 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -73,6 +73,18 @@ class Utility { HeaderString& cookies); }; +struct SettingsEntryHash { + size_t operator()(const nghttp2_settings_entry& entry) const { + return absl::Hash()(entry.settings_id); + } +}; + +struct SettingsEntryEquals { + bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { + return lhs.settings_id == rhs.settings_id; + } +}; + /** * Base class for HTTP/2 client and server codecs. */ @@ -286,6 +298,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable()(entry.settings_id); - } - }; - struct SettingsEntryEquals { - bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { - return lhs.settings_id == rhs.settings_id; - } - }; - virtual ConnectionCallbacks& callbacks() PURE; virtual int onBeginHeaders(const nghttp2_frame* frame) PURE; int onData(int32_t stream_id, const uint8_t* data, size_t len); @@ -404,7 +406,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable test_only_on_settings_frame_cb_; }; /** diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index f63250fc25485..63d00512e9af7 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -40,40 +40,6 @@ using Http2SettingsTuple = ::testing::tuple; namespace CommonUtility = ::Envoy::Http2::Utility; -class ConnectionImplTestPeer { -public: - // Registers callbacks from this peer with |connection|. - void registerPeer(ConnectionImpl& connection) { - connection.test_only_on_settings_frame_cb_ = - std::bind(&ConnectionImplTestPeer::onSettingsFrame, this, std::placeholders::_1); - } - - // Callback issued when nghttp2 processes a SETTINGS frame. - void onSettingsFrame(const nghttp2_settings& settings_frame) { - ENVOY_LOG_MISC(error, "callback issued"); - for (uint32_t i = 0; i < settings_frame.niv; ++i) { - ENVOY_LOG_MISC(error, "adding setting id {} val {}", settings_frame.iv[i].settings_id, - settings_frame.iv[i].value); - auto result = settings_.insert(settings_frame.iv[i]); - ASSERT(result.second); - } - } - - // Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint. - absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { - const auto it = settings_.find({identifier, 0}); - if (it == settings_.end()) { - return absl::nullopt; - } - return it->value; - } - -private: - std::unordered_set - settings_; -}; - class Http2CodecImplTestFixture { public: struct ConnectionWrapper { @@ -103,22 +69,15 @@ class Http2CodecImplTestFixture { : client_settings_(client_settings), server_settings_(server_settings) {} virtual ~Http2CodecImplTestFixture() = default; - virtual void initialize(ConnectionImplTestPeer* client_test_peer = nullptr, - ConnectionImplTestPeer* server_test_peer = nullptr) { + virtual void initialize() { Http2OptionsFromTuple(client_http2_options_, client_settings_); Http2OptionsFromTuple(server_http2_options_, server_settings_); client_ = std::make_unique( client_connection_, client_callbacks_, stats_store_, client_http2_options_, max_request_headers_kb_, max_response_headers_count_); - if (client_test_peer != nullptr) { - client_test_peer->registerPeer(*client_); - } server_ = std::make_unique( server_connection_, server_callbacks_, stats_store_, server_http2_options_, max_request_headers_kb_, max_request_headers_count_); - if (server_test_peer != nullptr) { - server_test_peer->registerPeer(*server_); - } request_encoder_ = &client_->newStream(response_decoder_); setupDefaultConnectionMocks(); @@ -1123,7 +1082,7 @@ class Http2CustomSettingsTest : public ::testing::TestWithParam< Http2CustomSettingsTest() : Http2CodecImplTestFixture(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam())), - client_peer_(::testing::get<2>(GetParam())) {} + validate_client_(::testing::get<2>(GetParam())) {} // Sets the custom settings parameters specified by |parameters| in the |options| proto. void setHttp2CustomSettingsParameters(envoy::config::core::v3::Http2ProtocolOptions& options, @@ -1137,26 +1096,27 @@ class Http2CustomSettingsTest : public ::testing::TestWithParam< } // Returns the Http2ProtocolOptions proto which specifies the settings parameters to be sent to - // the peer under test. + // the endpoint being validated. envoy::config::core::v3::Http2ProtocolOptions& getCustomOptions() { - return client_peer_ ? server_http2_options_ : client_http2_options_; + return validate_client_ ? server_http2_options_ : client_http2_options_; } - // Returns the peer under test. - ConnectionImplTestPeer* getPeer() { - return client_peer_ ? &client_test_peer_ : &server_test_peer_; + // Returns the endpoint being validated. + const TestCodecSettingsProvider& getSettingsProvider() { + if (validate_client_) { + return *client_; + } + return *server_; } // Returns the settings tuple which specifies a subset of the settings parameters to be sent to - // the peer under test. + // the endpoint being validated. const Http2SettingsTuple& getSettingsTuple() { - return client_peer_ ? server_settings_ : client_settings_; + return validate_client_ ? server_settings_ : client_settings_; } protected: - bool client_peer_{false}; - ConnectionImplTestPeer client_test_peer_; - ConnectionImplTestPeer server_test_peer_; + bool validate_client_{false}; }; INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestEdgeSettings, Http2CustomSettingsTest, @@ -1169,7 +1129,7 @@ INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestEdgeSettings, Http2CustomSettingsTest TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { std::vector custom_parameters{{0x10, 10}, {0x11, 20}}; setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); - initialize(&client_test_peer_, &server_test_peer_); + initialize(); TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); @@ -1177,24 +1137,27 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { uint32_t hpack_table_size = ::testing::get(getSettingsTuple()); if (hpack_table_size != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { - EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE), - HasValue(hpack_table_size)); + EXPECT_THAT( + getSettingsProvider().getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE), + HasValue(hpack_table_size)); } uint32_t max_concurrent_streams = ::testing::get(getSettingsTuple()); if (max_concurrent_streams != NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS) { - EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS), + EXPECT_THAT(getSettingsProvider().getRemoteSettingsParameterValue( + NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS), HasValue(max_concurrent_streams)); } uint32_t initial_stream_window_size = ::testing::get(getSettingsTuple()); if (max_concurrent_streams != NGHTTP2_INITIAL_WINDOW_SIZE) { - EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE), - HasValue(initial_stream_window_size)); + EXPECT_THAT( + getSettingsProvider().getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE), + HasValue(initial_stream_window_size)); } // Validate that custom parameters are received by the endpoint (client or server) under test. for (const auto& parameter : custom_parameters) { - EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(parameter.identifier), + EXPECT_THAT(getSettingsProvider().getRemoteSettingsParameterValue(parameter.identifier), HasValue(parameter.value)); } } @@ -1210,17 +1173,35 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParame {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS - 1}}; setHttp2CustomSettingsParameters(getCustomOptions(), named_parameter_overrides); - initialize(&client_test_peer_, &server_test_peer_); + initialize(); TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, false); for (const auto& parameter : named_parameter_overrides) { - EXPECT_THAT(getPeer()->getRemoteSettingsParameterValue(parameter.identifier), + EXPECT_THAT(getSettingsProvider().getRemoteSettingsParameterValue(parameter.identifier), HasValue(parameter.value)); } } +// Validates that server push can not be overriden via custom settings parameters. +TEST_P(Http2CustomSettingsTest, NeverAllowServerPushEnablementOnClientConnection) { + // Server push is always disabled only on the downstream (client) connections. + if (!validate_client_) { + return; + } + std::vector custom_parameters{{0x2, 1}}; + setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); + initialize(); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); + request_encoder_->encodeHeaders(request_headers, false); + // nghttp2 will not send the ENABLE_PUSH parameter to the peer when it is set to 0. + EXPECT_EQ(getSettingsProvider().getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_ENABLE_PUSH), + absl::nullopt); +} + // Tests request headers whose size is larger than the default limit of 60K. TEST_P(Http2CodecImplTest, LargeRequestHeadersInvokeResetStream) { initialize(); diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 494a856715323..ed3674b3911c9 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -1,3 +1,5 @@ +#pragma once + #include "envoy/http/codec.h" #include "common/http/http2/codec_impl.h" @@ -6,7 +8,35 @@ namespace Envoy { namespace Http { namespace Http2 { -class TestServerConnectionImpl : public ServerConnectionImpl { +class TestCodecSettingsProvider { +public: + // Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint. + absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { + const auto it = settings_.find({identifier, 0}); + if (it == settings_.end()) { + return absl::nullopt; + } + return it->value; + } + +protected: + // Stores SETTINGS parameters contained in |settings_frame| to make them available via + // getRemoteSettingsParameterValue(). + void onSettingsFrame(const nghttp2_settings& settings_frame) { + ENVOY_LOG_MISC(error, "callback issued"); + for (uint32_t i = 0; i < settings_frame.niv; ++i) { + ENVOY_LOG_MISC(error, "adding setting id {} val {}", settings_frame.iv[i].settings_id, + settings_frame.iv[i].value); + auto result = settings_.insert(settings_frame.iv[i]); + ASSERT(result.second); + } + } + +private: + std::unordered_set settings_; +}; + +class TestServerConnectionImpl : public ServerConnectionImpl, public TestCodecSettingsProvider { public: TestServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, @@ -16,9 +46,13 @@ class TestServerConnectionImpl : public ServerConnectionImpl { max_request_headers_count) {} nghttp2_session* session() { return session_; } using ServerConnectionImpl::getStream; + +protected: + // Overrides ServerConnectionImpl::onSettings(). + void onSettings(const nghttp2_settings& settings) override { onSettingsFrame(settings); } }; -class TestClientConnectionImpl : public ClientConnectionImpl { +class TestClientConnectionImpl : public ClientConnectionImpl, public TestCodecSettingsProvider { public: TestClientConnectionImpl(Network::Connection& connection, Http::ConnectionCallbacks& callbacks, Stats::Scope& scope, @@ -29,6 +63,10 @@ class TestClientConnectionImpl : public ClientConnectionImpl { nghttp2_session* session() { return session_; } using ClientConnectionImpl::getStream; using ConnectionImpl::sendPendingFrames; + +protected: + // Overrides ClientConnectionImpl::onSettings(). + void onSettings(const nghttp2_settings& settings) override { onSettingsFrame(settings); } }; } // namespace Http2 From cc5fe0956c49dd0f60e75d9637f015539d607fb4 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 14:12:21 -0500 Subject: [PATCH 10/31] Fix format. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 598ac6fdde056..01f67cfda2aff 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -846,7 +846,7 @@ void ConnectionImpl::sendSettings( std::unordered_set settings; // Universally disable receiving push promise frames as we don't currently support - // them. nghttp2 will fail the connection if the other side still sends them. + // them. nghttp2 will fail the connection if the other side still sends them. // TODO(mattklein123): Remove this when we correctly proxy push promise. // NOTE: This is a special case with respect to custom parameter overrides in that server push is // not supported and therefore not end user configurable. From 6def08e0396cee939197354405ab59c380b603b6 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 14:19:39 -0500 Subject: [PATCH 11/31] Fix format. Signed-off-by: Andres Guedez --- api/envoy/config/core/v3/protocol.proto | 3 ++- generated_api_shadow/envoy/api/v2/core/protocol.proto | 9 ++++++--- generated_api_shadow/envoy/config/core/v3/protocol.proto | 9 ++++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 4454443276bfc..94024faf12a4b 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -239,7 +239,8 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer. + // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: + // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. // Note that parameters specified through this config will override named parameters with matching // identifiers. // diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index a200003e7fec1..0e5a9b0a6670f 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -217,11 +217,14 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer. + // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: + // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. // Note that parameters specified through this config will override named parameters with matching // identifiers. - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified - // in :ref:`hpack_table_size`. + // + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in + // :ref:`hpack_table_size `. + // // See `IANA HTTP/2 Settings // `_ for // standardized identifiers. diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 9470619ac9d59..94024faf12a4b 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -239,11 +239,14 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer. + // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: + // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. // Note that parameters specified through this config will override named parameters with matching // identifiers. - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified - // in :ref:`hpack_table_size`. + // + // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in + // :ref:`hpack_table_size `. + // // See `IANA HTTP/2 Settings // `_ for // standardized identifiers. From 1cf9c3268c4e97e368d858fbe177388390fa9204 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 14:25:28 -0500 Subject: [PATCH 12/31] Fix spelling. Signed-off-by: Andres Guedez --- test/common/http/http2/codec_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 63d00512e9af7..07484608dd6f5 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1184,7 +1184,7 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParame } } -// Validates that server push can not be overriden via custom settings parameters. +// Validates that server push can not be overridden via custom settings parameters. TEST_P(Http2CustomSettingsTest, NeverAllowServerPushEnablementOnClientConnection) { // Server push is always disabled only on the downstream (client) connections. if (!validate_client_) { From 6bfbfc65d854922c6d89cb72aa530e97f9d220c5 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Feb 2020 14:36:33 -0500 Subject: [PATCH 13/31] Remove test debug logs. Signed-off-by: Andres Guedez --- test/common/http/http2/codec_impl_test_util.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index ed3674b3911c9..5147aca315910 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -23,10 +23,7 @@ class TestCodecSettingsProvider { // Stores SETTINGS parameters contained in |settings_frame| to make them available via // getRemoteSettingsParameterValue(). void onSettingsFrame(const nghttp2_settings& settings_frame) { - ENVOY_LOG_MISC(error, "callback issued"); for (uint32_t i = 0; i < settings_frame.niv; ++i) { - ENVOY_LOG_MISC(error, "adding setting id {} val {}", settings_frame.iv[i].settings_id, - settings_frame.iv[i].value); auto result = settings_.insert(settings_frame.iv[i]); ASSERT(result.second); } From b764d342e104e3e3f0cfa9746384b80e2898c6d7 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 18 Feb 2020 10:37:01 -0500 Subject: [PATCH 14/31] Regenerate API shadow. Signed-off-by: Andres Guedez --- generated_api_shadow/envoy/api/v2/core/protocol.proto | 10 ++++++++++ .../envoy/config/core/v3/protocol.proto | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 0e5a9b0a6670f..c561dde88a346 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -24,6 +24,12 @@ message UpstreamHttpProtocolOptions { // upstream connections based on the downstream HTTP host/authority header, as seen by the // :ref:`router filter `. bool auto_sni = 1; + + // Automatic validate upstream presented certificate for new upstream connections based on the + // downstream HTTP host/authority header, as seen by the + // :ref:`router filter `. + // This field is intended to set with `auto_sni` field. + bool auto_san_validation = 2; } message HttpProtocolOptions { @@ -127,6 +133,10 @@ message Http2ProtocolOptions { // `Maximum concurrent streams `_ // allowed for peer on one HTTP/2 connection. Valid values range from 1 to 2147483647 (2^31 - 1) // and defaults to 2147483647. + // + // For upstream connections, this also limits how many streams Envoy will initiate concurrently + // on a single connection. If the limit is reached, Envoy may queue requests or establish + // additional connections (as allowed per circuit breaker limits). google.protobuf.UInt32Value max_concurrent_streams = 2 [(validate.rules).uint32 = {lte: 2147483647 gte: 1}]; diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 94024faf12a4b..f64fbd8e8f0ed 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -29,6 +29,12 @@ message UpstreamHttpProtocolOptions { // upstream connections based on the downstream HTTP host/authority header, as seen by the // :ref:`router filter `. bool auto_sni = 1; + + // Automatic validate upstream presented certificate for new upstream connections based on the + // downstream HTTP host/authority header, as seen by the + // :ref:`router filter `. + // This field is intended to set with `auto_sni` field. + bool auto_san_validation = 2; } message HttpProtocolOptions { @@ -149,6 +155,10 @@ message Http2ProtocolOptions { // `Maximum concurrent streams `_ // allowed for peer on one HTTP/2 connection. Valid values range from 1 to 2147483647 (2^31 - 1) // and defaults to 2147483647. + // + // For upstream connections, this also limits how many streams Envoy will initiate concurrently + // on a single connection. If the limit is reached, Envoy may queue requests or establish + // additional connections (as allowed per circuit breaker limits). google.protobuf.UInt32Value max_concurrent_streams = 2 [(validate.rules).uint32 = {lte: 2147483647 gte: 1}]; From 9455741c24587ae08c16cb1cefbe503ee416d08e Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 18 Feb 2020 10:58:39 -0500 Subject: [PATCH 15/31] Refactor newly introduced references to Http2Settings. Refactor required now that Http2Settings -> Http2Options proto. Signed-off-by: Andres Guedez --- source/common/http/http2/conn_pool.cc | 2 +- test/common/http/http2/conn_pool_test.cc | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc index 090ac7d4d1de4..0a061ccc37514 100644 --- a/source/common/http/http2/conn_pool.cc +++ b/source/common/http/http2/conn_pool.cc @@ -67,7 +67,7 @@ uint64_t ConnPoolImpl::maxRequestsPerConnection() { ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) : ConnPoolImplBase::ActiveClient( parent, parent.maxRequestsPerConnection(), - parent.host_->cluster().http2Settings().max_concurrent_streams_) { + parent.host_->cluster().http2Options().max_concurrent_streams().value()) { codec_client_->setCodecClientCallbacks(*this); codec_client_->setCodecConnectionCallbacks(*this); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index 23fb0efc57e9a..4bc887c582671 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -248,7 +248,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnectionReadyWithRequest) { * complete. */ TEST_F(Http2ConnPoolImplTest, DrainConnectionBusy) { - cluster_->http2_settings_.max_concurrent_streams_ = 1; + cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(1); InSequence s; expectClientCreate(); @@ -331,7 +331,7 @@ TEST_F(Http2ConnPoolImplTest, DrainConnections) { // concurrent requests and causes additional connections to be created. TEST_F(Http2ConnPoolImplTest, MaxConcurrentRequestsPerStream) { cluster_->resetResourceManager(2, 1024, 1024, 1, 1); - cluster_->http2_settings_.max_concurrent_streams_ = 1; + cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(1); InSequence s; @@ -462,7 +462,7 @@ TEST_F(Http2ConnPoolImplTest, PendingRequestsNumberConnectingTotalRequestsPerCon // Verifies that the correct number of CONNECTING connections are created for // the pending requests, when the concurrent requests per connection is limited TEST_F(Http2ConnPoolImplTest, PendingRequestsNumberConnectingConcurrentRequestsPerConnection) { - cluster_->http2_settings_.max_concurrent_streams_ = 2; + cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(2); InSequence s; // Create three requests. The 3rd should create a 2nd connection due to the limit From 78d3e630a6b64ae8d13990cda4beb234e5fc9079 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 18 Feb 2020 15:08:27 -0500 Subject: [PATCH 16/31] Adjust expected memory usage for cluster memory tests. Signed-off-by: Andres Guedez --- test/integration/stats_integration_test.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index f2d0098bc01a5..0ff316a84a1d2 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -271,6 +271,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/01/09 9227 43637 44000 router: per-cluster histograms w/ timeout budget // 2020/01/12 9633 43797 44104 config: support recovery of original message when // upgrading. + // 2020/02/18 9964 44327 44600 http2: support custom SETTINGS parameters. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -284,8 +285,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 43797); // 104 bytes higher than a debug build. - EXPECT_MEMORY_LE(m_per_cluster, 44104); + EXPECT_MEMORY_EQ(m_per_cluster, 44327); + EXPECT_MEMORY_LE(m_per_cluster, 44600); } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -325,6 +326,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2019/01/09 9227 35772 36500 router: per-cluster histograms w/ timeout budget // 2020/01/12 9633 35932 36500 config: support recovery of original message when // upgrading. + // 2020/02/18 9964 36462 36800 http2: support custom SETTINGS parameters. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -338,8 +340,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 35932); // 104 bytes higher than a debug build. - EXPECT_MEMORY_LE(m_per_cluster, 36500); + EXPECT_MEMORY_EQ(m_per_cluster, 36462); + EXPECT_MEMORY_LE(m_per_cluster, 36800); } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From 8b7621cc65ab0bbadcb4aea3fb2896f50b4a7436 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 25 Feb 2020 16:40:15 -0500 Subject: [PATCH 17/31] Kick CI Signed-off-by: Andres Guedez From 07c18140acae6de1183b7c8c01ffea4227dedc99 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 27 Feb 2020 10:09:41 -0500 Subject: [PATCH 18/31] Merge conflict resolution. Signed-off-by: Andres Guedez --- test/common/http/http2/codec_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index e62bb8c185c4f..f10fb6fa4d56f 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1143,7 +1143,7 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { std::vector custom_parameters{{0x10, 10}, {0x11, 20}}; setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); initialize(); - TestHeaderMapImpl request_headers; + TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, false); @@ -1187,7 +1187,7 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParame CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS - 1}}; setHttp2CustomSettingsParameters(getCustomOptions(), named_parameter_overrides); initialize(); - TestHeaderMapImpl request_headers; + TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, false); @@ -1206,7 +1206,7 @@ TEST_P(Http2CustomSettingsTest, NeverAllowServerPushEnablementOnClientConnection std::vector custom_parameters{{0x2, 1}}; setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); initialize(); - TestHeaderMapImpl request_headers; + TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, false); From 8293133d916d5374c665920c26a4d7cd8b378d1e Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 12 Mar 2020 10:33:55 -0400 Subject: [PATCH 19/31] Disallow user defined and named parameter collisions in Http2ProtocolOptions. The only exception is the `allow_connect` field, which is a proto3 bool and can not be checked for presence. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 18 +- api/envoy/config/core/v3/protocol.proto | 18 +- .../envoy/api/v2/core/protocol.proto | 18 +- .../envoy/config/core/v3/protocol.proto | 18 +- source/common/http/BUILD | 1 + source/common/http/http2/codec_impl.cc | 9 +- source/common/http/utility.cc | 59 +++++++ test/common/upstream/upstream_impl_test.cc | 70 +++++++- .../http_connection_manager/config_test.cc | 155 ++++++++++++++++++ 9 files changed, 339 insertions(+), 27 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index c561dde88a346..0ce86858797b7 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -229,11 +229,21 @@ message Http2ProtocolOptions { // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. - // Note that parameters specified through this config will override named parameters with matching - // identifiers. // - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in - // :ref:`hpack_table_size `. + // Note that custom parameters specified through this field may not collide with the following + // named parameters: + // + // .. code-block:: text + // + // ID Field Name + // ---------------- + // 0x1 hpack_table_size + // 0x3 max_concurrent_streams + // 0x4 initial_stream_window_size + // + // Collisions will trigger config validation failure on load/update. + // The only exception is '0x8 allow_connect', for which the named parameter field will be + // overriden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index f64fbd8e8f0ed..15692a3a30413 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -251,11 +251,21 @@ message Http2ProtocolOptions { // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. - // Note that parameters specified through this config will override named parameters with matching - // identifiers. // - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in - // :ref:`hpack_table_size `. + // Note that custom parameters specified through this field may not collide with the following + // named parameters: + // + // .. code-block:: text + // + // ID Field Name + // ---------------- + // 0x1 hpack_table_size + // 0x3 max_concurrent_streams + // 0x4 initial_stream_window_size + // + // Collisions will trigger config validation failure on load/update. + // The only exception is '0x8 allow_connect', for which the named parameter field will be + // overriden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index c561dde88a346..0ce86858797b7 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -229,11 +229,21 @@ message Http2ProtocolOptions { // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. - // Note that parameters specified through this config will override named parameters with matching - // identifiers. // - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in - // :ref:`hpack_table_size `. + // Note that custom parameters specified through this field may not collide with the following + // named parameters: + // + // .. code-block:: text + // + // ID Field Name + // ---------------- + // 0x1 hpack_table_size + // 0x3 max_concurrent_streams + // 0x4 initial_stream_window_size + // + // Collisions will trigger config validation failure on load/update. + // The only exception is '0x8 allow_connect', for which the named parameter field will be + // overriden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index f64fbd8e8f0ed..15692a3a30413 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -251,11 +251,21 @@ message Http2ProtocolOptions { // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. - // Note that parameters specified through this config will override named parameters with matching - // identifiers. // - // e.g., A SettingsParameter { identifier = 1, value = 1000 } will override the value specified in - // :ref:`hpack_table_size `. + // Note that custom parameters specified through this field may not collide with the following + // named parameters: + // + // .. code-block:: text + // + // ID Field Name + // ---------------- + // 0x1 hpack_table_size + // 0x3 max_concurrent_streams + // 0x4 initial_stream_window_size + // + // Collisions will trigger config validation failure on load/update. + // The only exception is '0x8 allow_connect', for which the named parameter field will be + // overriden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 1c4068178f4da..68f7f6a6b3010 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -327,6 +327,7 @@ envoy_cc_library( external_deps = [ "abseil_optional", "http_parser", + "nghttp2", ], deps = [ ":exception_lib", diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index ad57faad4fd9a..5e4a66c6e1798 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -891,7 +891,6 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { - // Custom parameters override named parameters, so they must be inserted first into this set. std::unordered_set settings; // Universally disable receiving push promise frames as we don't currently support @@ -943,8 +942,14 @@ void ConnectionImpl::sendSettings( for (const auto& descriptor : descriptors) { if (descriptor.entry.value != descriptor.default_value) { auto result = settings.insert(descriptor.entry); + // Config validation should ensure a collision with user defined parameters is not posible. + // There is one exception, `allow_connect`, which requires a breaking API change to enable + // presence checks during validation. if (!result.second) { - ENVOY_CONN_LOG(debug, "duplicate named settings parameter with id {:#x}, value {}", + ASSERT(descriptor.entry.settings_id == NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL); + ENVOY_CONN_LOG(debug, + "overriding named settings parameter with id {:#x}, value {} due to " + "collision with user defined parameter", connection_, descriptor.entry.settings_id, descriptor.entry.value); continue; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 14c56e2483090..7806bbb51befd 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -29,11 +29,67 @@ #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "nghttp2/nghttp2.h" namespace Envoy { namespace Http2 { namespace Utility { +namespace { + +void validateCustomSettingsParameters( + const envoy::config::core::v3::Http2ProtocolOptions& options) { + std::vector duplicate_parameters; + // User defined and named parameters with the same SETTINGS identifier can not both be set. + for (const auto it : options.custom_settings_parameters()) { + ASSERT(it.identifier().value() <= std::numeric_limits::max()); + switch (it.identifier().value()) { + case NGHTTP2_SETTINGS_ENABLE_PUSH: + if (it.value().value() == 1) { + throw EnvoyException("server push is not supported by Envoy and can not be enabled via a " + "SETTINGS parameter."); + } + break; + case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL: + // This parameter can not be validated due to the use of proto3 bool, which doesn't allow + // checking for presence. + // TODO: This can be fixed via a breaking API change of the type to + // `google.protobuf.BoolValue`. + break; + case NGHTTP2_SETTINGS_HEADER_TABLE_SIZE: + if (options.has_hpack_table_size()) { + duplicate_parameters.push_back("hpack_table_size"); + } + break; + case NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS: + if (options.has_max_concurrent_streams()) { + duplicate_parameters.push_back("max_concurrent_streams"); + } + break; + case NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE: + if (options.has_initial_stream_window_size()) { + duplicate_parameters.push_back("initial_stream_window_size"); + } + break; + default: + // Ignore unknown parameters. + break; + } + } + + if (!duplicate_parameters.empty()) { + throw EnvoyException(fmt::format( + "the {{{}}} HTTP/2 SETTINGS parameter(s) can not be configured through both named and " + "custom " + "parameters", + absl::StrJoin(duplicate_parameters, ",", [](std::string* out, const std::string& str) { + absl::StrAppend(out, str); + }))); + } +} + +} // namespace + const uint32_t OptionsLimits::MIN_HPACK_TABLE_SIZE; const uint32_t OptionsLimits::DEFAULT_HPACK_TABLE_SIZE; const uint32_t OptionsLimits::MAX_HPACK_TABLE_SIZE; @@ -55,6 +111,9 @@ const uint32_t OptionsLimits::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_ envoy::config::core::v3::Http2ProtocolOptions initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions& options) { envoy::config::core::v3::Http2ProtocolOptions options_clone(options); + // This will throw an exception when a custom parameter and a named parameter collide. + validateCustomSettingsParameters(options); + if (!options_clone.has_hpack_table_size()) { options_clone.mutable_hpack_table_size()->set_value(OptionsLimits::DEFAULT_HPACK_TABLE_SIZE); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index bfc7b2635d367..78ca46dd9640a 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -982,6 +982,57 @@ TEST_F(StrictDnsClusterImplTest, DefaultTtlAsDnsRefreshRateWhenResponseEmpty) { resolver.dns_callback_(TestUtility::makeDnsResponse({}, std::chrono::seconds(5))); } +// Ensures that HTTP/2 user defined SETTINGS parameter validation is enforced on clusters. +TEST_F(StrictDnsClusterImplTest, Http2UserDefinedSettingsParametersValidation) { + const std::string yaml = R"EOF( + name: name + connect_timeout: 0.25s + type: strict_dns + dns_refresh_rate: 4s + dns_failure_refresh_rate: + base_interval: 7s + max_interval: 10s + lb_policy: round_robin + circuit_breakers: + thresholds: + - priority: DEFAULT + max_connections: 43 + max_pending_requests: 57 + max_requests: 50 + max_retries: 10 + - priority: HIGH + max_connections: 1 + max_pending_requests: 2 + max_requests: 3 + max_retries: 4 + max_requests_per_connection: 3 + protocol_selection: USE_DOWNSTREAM_PROTOCOL + http2_protocol_options: + hpack_table_size: 2048 + custom_settings_parameters: { identifier: 1, value: 1024 } + http_protocol_options: + header_key_format: + proper_case_words: {} + hosts: + - { socket_address: { address: localhost1, port_value: 11001 }} + - { socket_address: { address: localhost2, port_value: 11002 }} + )EOF"; + + envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats_.createScope(fmt::format( + "cluster.{}.", cluster_config.alt_stat_name().empty() ? cluster_config.name() + : cluster_config.alt_stat_name())); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, random_, stats_, + singleton_manager_, tls_, validation_visitor_, *api_); + EXPECT_THROW_WITH_REGEX( + StrictDnsClusterImpl(cluster_config, runtime_, dns_resolver_, factory_context, + std::move(scope), false), + EnvoyException, + R"(the \{hpack_table_size\} HTTP/2 SETTINGS parameter\(s\) can not be configured through)" + " both"); +} + TEST(HostImplTest, HostCluster) { MockClusterMockPrioritySet cluster; HostSharedPtr host = makeTestHost(cluster.info_, "tcp://10.0.0.1:1234", 1); @@ -1250,7 +1301,8 @@ TEST_F(StaticClusterImplTest, LoadAssignmentLocality) { EXPECT_FALSE(cluster.info()->addedViaApi()); } -// Validates that setting an EDS health value through LoadAssignment is honored for static clusters. +// Validates that setting an EDS health value through LoadAssignment is honored for static +// clusters. TEST_F(StaticClusterImplTest, LoadAssignmentEdsHealth) { const std::string yaml = R"EOF( name: staticcluster @@ -1814,8 +1866,8 @@ TEST(PrioritySet, Extend) { // Test batch host updates. Verify that we can move a host without triggering intermediate host // updates. - // We're going to do a noop host change, so add a callback to assert that we're not announcing any - // host changes. + // We're going to do a noop host change, so add a callback to assert that we're not announcing + // any host changes. priority_set.addMemberUpdateCb([&](const HostVector& added, const HostVector& removed) -> void { EXPECT_TRUE(added.empty() && removed.empty()); }); @@ -1978,9 +2030,9 @@ TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForUnknownFilter) { no_such_filter: { option: value } )EOF"; - EXPECT_THROW_WITH_MESSAGE( - makeCluster(yaml), EnvoyException, - "Didn't find a registered network or http filter implementation for name: 'no_such_filter'"); + EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException, + "Didn't find a registered network or http filter implementation for " + "name: 'no_such_filter'"); } TEST_F(ClusterInfoImplTest, TypedExtensionProtocolOptionsForUnknownFilter) { @@ -1995,9 +2047,9 @@ TEST_F(ClusterInfoImplTest, TypedExtensionProtocolOptionsForUnknownFilter) { "@type": type.googleapis.com/google.protobuf.Struct )EOF"; - EXPECT_THROW_WITH_MESSAGE( - makeCluster(yaml), EnvoyException, - "Didn't find a registered network or http filter implementation for name: 'no_such_filter'"); + EXPECT_THROW_WITH_MESSAGE(makeCluster(yaml), EnvoyException, + "Didn't find a registered network or http filter implementation for " + "name: 'no_such_filter'"); } TEST_F(ClusterInfoImplTest, OneofExtensionProtocolOptionsForUnknownFilter) { diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 285bce950f424..0d36f0e3acd4c 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -956,6 +956,161 @@ stat_prefix: my_stat_prefix "bad_type: Cannot find field"); } +// Validates that HttpConnectionManagerConfig construction succeeds when there are no collisions +// between named and user defined parameters, and server push is not modified. +TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsNoCollision) { + const std::string yaml_string = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.router + typed_config: {} +http2_protocol_options: + hpack_table_size: 1024 + custom_settings_parameters: { identifier: 3, value: 2048 } + )EOF"; + // This will throw when Http2ProtocolOptions validation fails. + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); +} + +// Validates that named and user defined parameter collisions will trigger a config validation +// failure. +TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsNamedParameterCollision) { + // Override both hpack_table_size (id = 0x1) and max_concurrent_streams (id = 0x3) with custom + // parameters of the same and different values (respectively). + const std::string yaml_string = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.http_dynamo_filter + typed_config: {} +http2_protocol_options: + hpack_table_size: 2048 + max_concurrent_streams: 4096 + custom_settings_parameters: + - { identifier: 1, value: 2048 } + - { identifier: 3, value: 1024 } + )EOF"; + EXPECT_THROW_WITH_REGEX( + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_), + EnvoyException, + R"(the \{hpack_table_size,max_concurrent_streams\} HTTP/2 SETTINGS parameter\(s\) can not be)" + " configured"); + + const std::string yaml_string2 = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.router + typed_config: {} +http2_protocol_options: + allow_connect: true + custom_settings_parameters: + - { identifier: 8, value: 0 } + )EOF"; + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string2), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); +} + +// Validates that setting the server push parameter via user defined parameters is disallowed. +TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsDisallowServerPush) { + const std::string yaml_string = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.http_dynamo_filter + typed_config: {} +http2_protocol_options: + custom_settings_parameters: { identifier: 2, value: 1 } + )EOF"; + + EXPECT_THROW_WITH_REGEX( + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_), + EnvoyException, + "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter."); + + // Specify both the server push parameter and colliding named and user defined parameters. + const std::string yaml_string2 = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.http_dynamo_filter + typed_config: {} +http2_protocol_options: + hpack_table_size: 2048 + max_concurrent_streams: 4096 + custom_settings_parameters: + - { identifier: 1, value: 2048 } + - { identifier: 2, value: 1 } + - { identifier: 3, value: 1024 } + )EOF"; + + // The server push exception is thrown first. + EXPECT_THROW_WITH_REGEX( + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_), + EnvoyException, + "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter."); +} + // Test that the deprecated extension name still functions. TEST_F(HttpConnectionManagerConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) { const std::string deprecated_name = "envoy.http_connection_manager"; From cb3550180c35e049898acc0b17ab275b5f5da95f Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 12 Mar 2020 11:27:40 -0400 Subject: [PATCH 20/31] Spelling fixes. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 2 +- api/envoy/config/core/v3/protocol.proto | 2 +- generated_api_shadow/envoy/api/v2/core/protocol.proto | 2 +- .../envoy/config/core/v3/protocol.proto | 2 +- source/common/http/http2/codec_impl.cc | 2 +- .../network/http_connection_manager/config_test.cc | 10 +++++----- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 0ce86858797b7..6b49ac7aea5c6 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -243,7 +243,7 @@ message Http2ProtocolOptions { // // Collisions will trigger config validation failure on load/update. // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overriden by the value set in this field. + // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 15692a3a30413..8b5c30c0c3631 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -265,7 +265,7 @@ message Http2ProtocolOptions { // // Collisions will trigger config validation failure on load/update. // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overriden by the value set in this field. + // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 0ce86858797b7..6b49ac7aea5c6 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -243,7 +243,7 @@ message Http2ProtocolOptions { // // Collisions will trigger config validation failure on load/update. // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overriden by the value set in this field. + // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 15692a3a30413..8b5c30c0c3631 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -265,7 +265,7 @@ message Http2ProtocolOptions { // // Collisions will trigger config validation failure on load/update. // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overriden by the value set in this field. + // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 048fbe07bd88f..a80f75f038173 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -958,7 +958,7 @@ void ConnectionImpl::sendSettings( for (const auto& descriptor : descriptors) { if (descriptor.entry.value != descriptor.default_value) { auto result = settings.insert(descriptor.entry); - // Config validation should ensure a collision with user defined parameters is not posible. + // Config validation should ensure a collision with user defined parameters is not possible. // There is one exception, `allow_connect`, which requires a breaking API change to enable // presence checks during validation. if (!result.second) { diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 332f2f631134d..f0cc96531034e 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1164,7 +1164,7 @@ stat_prefix: my_stat_prefix // This will throw when Http2ProtocolOptions validation fails. HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_, - scoped_routes_config_provider_manager_); + scoped_routes_config_provider_manager_, http_tracer_manager_); } // Validates that named and user defined parameter collisions will trigger a config validation @@ -1198,7 +1198,7 @@ stat_prefix: my_stat_prefix EXPECT_THROW_WITH_REGEX( HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_, - scoped_routes_config_provider_manager_), + scoped_routes_config_provider_manager_, http_tracer_manager_), EnvoyException, R"(the \{hpack_table_size,max_concurrent_streams\} HTTP/2 SETTINGS parameter\(s\) can not be)" " configured"); @@ -1226,7 +1226,7 @@ stat_prefix: my_stat_prefix )EOF"; HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string2), context_, date_provider_, route_config_provider_manager_, - scoped_routes_config_provider_manager_); + scoped_routes_config_provider_manager_, http_tracer_manager_); } // Validates that setting the server push parameter via user defined parameters is disallowed. @@ -1254,7 +1254,7 @@ stat_prefix: my_stat_prefix EXPECT_THROW_WITH_REGEX( HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_, - scoped_routes_config_provider_manager_), + scoped_routes_config_provider_manager_, http_tracer_manager_), EnvoyException, "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter."); @@ -1288,7 +1288,7 @@ stat_prefix: my_stat_prefix EXPECT_THROW_WITH_REGEX( HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_, - scoped_routes_config_provider_manager_), + scoped_routes_config_provider_manager_, http_tracer_manager_), EnvoyException, "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter."); } From 43b83bfa330eb6be390b71f5ee42b01ee8544110 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Sat, 14 Mar 2020 12:23:48 -0400 Subject: [PATCH 21/31] Simplify custom settings parameters validation logic and minor cleanup. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 7 +- api/envoy/config/core/v3/protocol.proto | 7 +- bazel/repositories.bzl | 5 + .../envoy/api/v2/core/protocol.proto | 7 +- .../envoy/config/core/v3/protocol.proto | 7 +- source/common/http/http2/BUILD | 2 + source/common/http/http2/codec_impl.cc | 91 ++++++++----------- test/common/http/http2/codec_impl_test.cc | 91 +++++++++++-------- 8 files changed, 122 insertions(+), 95 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 6b49ac7aea5c6..f4472496f7e74 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -118,10 +118,13 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 6.5.1 `_ for details. message SettingsParameter { // The 16 bit parameter identifier. - google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + google.protobuf.UInt32Value identifier = 1 [ + (validate.rules).uint32 = {lte: 65536 gte: 1}, + (validate.rules).message = {required: true} + ]; // The 32 bit parameter value. - google.protobuf.UInt32Value value = 2; + google.protobuf.UInt32Value value = 2 [(validate.rules).message = {required: true}]; } // `Maximum table size `_ diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 8b5c30c0c3631..cec11cf01a792 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -140,10 +140,13 @@ message Http2ProtocolOptions { "envoy.api.v2.core.Http2ProtocolOptions.SettingsParameter"; // The 16 bit parameter identifier. - google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + google.protobuf.UInt32Value identifier = 1 [ + (validate.rules).uint32 = {lte: 65536 gte: 1}, + (validate.rules).message = {required: true} + ]; // The 32 bit parameter value. - google.protobuf.UInt32Value value = 2; + google.protobuf.UInt32Value value = 2 [(validate.rules).message = {required: true}]; } // `Maximum table size `_ diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 89737cec0c20e..cc95873611907 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -505,6 +505,11 @@ def _com_google_absl(): actual = "@com_google_absl//absl/time:time", ) + native.bind( + name = "abseil_algorithm", + actual = "@com_google_absl//absl/algorithm:algorithm", + ) + def _com_google_protobuf(): _repository_impl("rules_python") _repository_impl( diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 6b49ac7aea5c6..f4472496f7e74 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -118,10 +118,13 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 6.5.1 `_ for details. message SettingsParameter { // The 16 bit parameter identifier. - google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + google.protobuf.UInt32Value identifier = 1 [ + (validate.rules).uint32 = {lte: 65536 gte: 1}, + (validate.rules).message = {required: true} + ]; // The 32 bit parameter value. - google.protobuf.UInt32Value value = 2; + google.protobuf.UInt32Value value = 2 [(validate.rules).message = {required: true}]; } // `Maximum table size `_ diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 8b5c30c0c3631..cec11cf01a792 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -140,10 +140,13 @@ message Http2ProtocolOptions { "envoy.api.v2.core.Http2ProtocolOptions.SettingsParameter"; // The 16 bit parameter identifier. - google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}]; + google.protobuf.UInt32Value identifier = 1 [ + (validate.rules).uint32 = {lte: 65536 gte: 1}, + (validate.rules).message = {required: true} + ]; // The 32 bit parameter value. - google.protobuf.UInt32Value value = 2; + google.protobuf.UInt32Value value = 2 [(validate.rules).message = {required: true}]; } // `Maximum table size `_ diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD index ad8d93119ae51..a6e03c97083b2 100644 --- a/source/common/http/http2/BUILD +++ b/source/common/http/http2/BUILD @@ -15,6 +15,8 @@ envoy_cc_library( external_deps = [ "nghttp2", "abseil_optional", + "abseil_inlined_vector", + "abseil_algorithm", ], deps = [ ":metadata_decoder_lib", diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index a80f75f038173..132fefe9dda6b 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -907,7 +907,18 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) { - std::unordered_set settings; + absl::InlinedVector settings; + auto insertParameter = [&settings](const nghttp2_settings_entry& entry) mutable -> bool { + const auto it = std::find_if(settings.cbegin(), settings.cend(), + [&entry](const nghttp2_settings_entry& existing) { + return entry.settings_id == existing.settings_id; + }); + if (it != settings.end()) { + return false; + } + settings.push_back(entry); + return true; + }; // Universally disable receiving push promise frames as we don't currently support // them. nghttp2 will fail the connection if the other side still sends them. @@ -915,69 +926,47 @@ void ConnectionImpl::sendSettings( // NOTE: This is a special case with respect to custom parameter overrides in that server push is // not supported and therefore not end user configurable. if (disable_push) { - settings.insert({static_cast(NGHTTP2_SETTINGS_ENABLE_PUSH), disable_push ? 0U : 1U}); + settings.push_back( + {static_cast(NGHTTP2_SETTINGS_ENABLE_PUSH), disable_push ? 0U : 1U}); } for (const auto it : http2_options.custom_settings_parameters()) { + // Validate that named parameters (with the exception of `allow_connect`) are not being + // overridden by custom parameters. + ASSERT((absl::c_none_of>( + {NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, NGHTTP2_SETTINGS_ENABLE_PUSH, + NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE}, + [it](const uint16_t& entry) { return entry == it.identifier().value(); }))); ASSERT(it.identifier().value() <= std::numeric_limits::max()); - if (it.identifier().value() == NGHTTP2_SETTINGS_ENABLE_PUSH && it.value().value() == 1) { - // For simplicity, warn when server push is configured enabled on either client or server - // connections. It _is_ actually enabled on downstream connections although Envoy itself will - // never send push_promise frames, but the nuance could be confusing to end users. - ENVOY_CONN_LOG( - warn, - "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter.", - connection_); - continue; - } - auto result = - settings.insert({static_cast(it.identifier().value()), it.value().value()}); - if (!result.second) { - ENVOY_CONN_LOG(debug, "duplicate user defined settings parameter with id {:#x}, value {}", + + const bool result = + insertParameter({static_cast(it.identifier().value()), it.value().value()}); + if (!result) { + ENVOY_CONN_LOG(debug, "duplicate custom settings parameter with id {:#x}, value {}", connection_, it.identifier().value(), it.value().value()); continue; } - ENVOY_CONN_LOG(debug, "adding user defined settings parameter with id {:#x} to {}", connection_, + ENVOY_CONN_LOG(debug, "adding custom settings parameter with id {:#x} to {}", connection_, it.identifier().value(), it.value().value()); } - struct SettingsDescriptor { - nghttp2_settings_entry entry; - uint32_t default_value; - }; - auto descriptors = std::vector{ - {{/*nghttp2_settings_entry.settings_id=*/NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, - /*nghttp2_settings_entry.value=*/http2_options.allow_connect()}, - /*default_value=*/0}, + // Insert named parameters. + settings.insert( + settings.end(), {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, - NGHTTP2_DEFAULT_HEADER_TABLE_SIZE}, - {{NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()}, - NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS}, - {{NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()}, - NGHTTP2_INITIAL_WINDOW_SIZE}}; - for (const auto& descriptor : descriptors) { - if (descriptor.entry.value != descriptor.default_value) { - auto result = settings.insert(descriptor.entry); - // Config validation should ensure a collision with user defined parameters is not possible. - // There is one exception, `allow_connect`, which requires a breaking API change to enable - // presence checks during validation. - if (!result.second) { - ASSERT(descriptor.entry.settings_id == NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL); - ENVOY_CONN_LOG(debug, - "overriding named settings parameter with id {:#x}, value {} due to " - "collision with user defined parameter", - connection_, descriptor.entry.settings_id, descriptor.entry.value); - continue; - } - ENVOY_CONN_LOG(debug, "adding named settings parameter with id {:#x} to {}", connection_, - descriptor.entry.settings_id, descriptor.entry.value); - } + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()}, + {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()}}); + const bool result = + insertParameter({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}); + if (!result) { + ENVOY_CONN_LOG(warn, + "the named allow_connect SETTINGS parameter is being overriden by a custom " + "parameter with value {}", + connection_, http2_options.allow_connect()); } if (!settings.empty()) { - std::vector iv; - std::copy(settings.begin(), settings.end(), std::back_inserter(iv)); - int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); + int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &settings[0], settings.size()); ASSERT(rc == 0); } else { // nghttp2_submit_settings need to be called at least once @@ -996,7 +985,7 @@ void ConnectionImpl::sendSettings( NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); } -} // namespace Http2 +} ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_new(&callbacks_); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 7368ab0327bf2..a6ffd588216e8 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1113,18 +1113,19 @@ MATCHER_P(HasValue, m, "") { return ExplainMatchResult(m, value, result_listener); }; -class Http2CustomSettingsTest : public ::testing::TestWithParam< - ::testing::tuple>, - public Http2CodecImplTestFixture { +class Http2CustomSettingsTestBase : public Http2CodecImplTestFixture { public: struct SettingsParameter { uint16_t identifier; uint32_t value; }; - Http2CustomSettingsTest() - : Http2CodecImplTestFixture(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam())), - validate_client_(::testing::get<2>(GetParam())) {} + Http2CustomSettingsTestBase(Http2SettingsTuple client_settings, + Http2SettingsTuple server_settings, bool validate_client) + : Http2CodecImplTestFixture(client_settings, server_settings), + validate_client_(validate_client) {} + + virtual ~Http2CustomSettingsTestBase() = default; // Sets the custom settings parameters specified by |parameters| in the |options| proto. void setHttp2CustomSettingsParameters(envoy::config::core::v3::Http2ProtocolOptions& options, @@ -1161,13 +1162,22 @@ class Http2CustomSettingsTest : public ::testing::TestWithParam< bool validate_client_{false}; }; +class Http2CustomSettingsTest + : public Http2CustomSettingsTestBase, + public ::testing::TestWithParam< + ::testing::tuple> { +public: + Http2CustomSettingsTest() + : Http2CustomSettingsTestBase(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam()), + ::testing::get<2>(GetParam())) {} +}; INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestEdgeSettings, Http2CustomSettingsTest, ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE, HTTP2SETTINGS_DEFAULT_COMBINE, ::testing::Bool())); // Validates that custom parameters (those which are not explicitly named in the -// envoy::config::core::v3::Http2ProtocolOptions proto) are properly sent and processed by client -// and server connections. +// envoy::config::core::v3::Http2ProtocolOptions proto) are properly sent and processed by +// client and server connections. TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { std::vector custom_parameters{{0x10, 10}, {0x11, 20}}; setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); @@ -1197,23 +1207,20 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { getSettingsProvider().getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE), HasValue(initial_stream_window_size)); } - // Validate that custom parameters are received by the endpoint (client or server) under test. + // Validate that custom parameters are received by the endpoint (client or server) under + // test. for (const auto& parameter : custom_parameters) { EXPECT_THAT(getSettingsProvider().getRemoteSettingsParameterValue(parameter.identifier), HasValue(parameter.value)); } } -// Validates that custom settings parameters override named parameter values (both when these are -// defaulted or explicitly set). -TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParameters) { - // These settings override named parameters which are defaulted when not specified via - // configuration. +// Validates that the only custom settings parameter allowed to override a named parameter is +// 'allow_connect' (id = 0x8). +TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersAllowConnectOverrideOnly) { + // The named allow_connect value defaults to 0. std::vector named_parameter_overrides{ - {NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, - CommonUtility::OptionsLimits::DEFAULT_HPACK_TABLE_SIZE - 1}, - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - CommonUtility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS - 1}}; + {NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, 1}}; setHttp2CustomSettingsParameters(getCustomOptions(), named_parameter_overrides); initialize(); TestRequestHeaderMapImpl request_headers; @@ -1226,22 +1233,32 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersOverrideNamedParame } } -// Validates that server push can not be overridden via custom settings parameters. -TEST_P(Http2CustomSettingsTest, NeverAllowServerPushEnablementOnClientConnection) { - // Server push is always disabled only on the downstream (client) connections. - if (!validate_client_) { - return; - } - std::vector custom_parameters{{0x2, 1}}; - setHttp2CustomSettingsParameters(getCustomOptions(), custom_parameters); - initialize(); - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); - request_encoder_->encodeHeaders(request_headers, false); - // nghttp2 will not send the ENABLE_PUSH parameter to the peer when it is set to 0. - EXPECT_EQ(getSettingsProvider().getRemoteSettingsParameterValue(NGHTTP2_SETTINGS_ENABLE_PUSH), - absl::nullopt); +class Http2CustomSettingsDeathTest + : public Http2CustomSettingsTestBase, + public ::testing::TestWithParam< + ::testing::tuple> { + +public: + Http2CustomSettingsDeathTest() + : Http2CustomSettingsTestBase(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam()), + ::testing::get<2>(GetParam())), + custom_parameter_identifier_(::testing::get<3>(GetParam())) {} + +protected: + uint16_t custom_parameter_identifier_{0}; +}; +INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestDefaultSettings, Http2CustomSettingsDeathTest, + ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE, + HTTP2SETTINGS_DEFAULT_COMBINE, ::testing::Bool(), + ::testing::Values(0x1, 0x2, 0x3, 0x4))); + +// Validates that overriding named parameters with custom parameters will trigger an ASSERT(). +// NOTE: `allow_connect` is the only exception since presence can not be checked without a breaking +// API change. +TEST_P(Http2CustomSettingsDeathTest, DisallowNamedParameterOverrides) { + std::vector custom_parameters{{custom_parameter_identifier_, 1}}; + setHttp2CustomSettingsParameters(client_http2_options_, custom_parameters); + EXPECT_DEBUG_DEATH(initialize(), ""); } // Tests request headers whose size is larger than the default limit of 60K. @@ -1447,7 +1464,8 @@ TEST_P(Http2CodecImplTestAll, TestCodecHeaderCompression) { EXPECT_EQ(nghttp2_session_get_hd_deflate_dynamic_table_size(client_->session()), nghttp2_session_get_hd_inflate_dynamic_table_size(server_->session())); - // Verify that headers are compressed only when both client and server advertise table size > 0: + // Verify that headers are compressed only when both client and server advertise table size + // > 0: if (client_http2_options_.hpack_table_size().value() && server_http2_options_.hpack_table_size().value()) { EXPECT_NE(0, nghttp2_session_get_hd_deflate_dynamic_table_size(client_->session())); @@ -1541,7 +1559,8 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) { for (int i = 0; i < kMaxOutboundControlFrames / 2; ++i) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); } - // The number of outbound frames should be half of max so the connection should not be terminated. + // The number of outbound frames should be half of max so the connection should not be + // terminated. EXPECT_NO_THROW(client_->sendPendingFrames()); // 1 more ping frame should overflow the outbound frame limit. From be392743f13d3b5a6ac9486570af0cd60968d45a Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Sat, 14 Mar 2020 12:41:46 -0400 Subject: [PATCH 22/31] Minor cleanup. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- source/common/http/http2/codec_impl.h | 14 +------------ test/common/http/http2/codec_impl_test_util.h | 20 +++++++++++++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 132fefe9dda6b..e0c4769e82c29 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -551,7 +551,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { } if (frame->hd.type == NGHTTP2_SETTINGS && frame->hd.flags == NGHTTP2_FLAG_NONE) { - onSettings(frame->settings); + onSettingsForTest(frame->settings); } StreamImpl* stream = getStream(frame->hd.stream_id); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index c946d63012c34..e469539f40aa4 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -73,18 +73,6 @@ class Utility { HeaderString& cookies); }; -struct SettingsEntryHash { - size_t operator()(const nghttp2_settings_entry& entry) const { - return absl::Hash()(entry.settings_id); - } -}; - -struct SettingsEntryEquals { - bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { - return lhs.settings_id == rhs.settings_id; - } -}; - /** * Base class for HTTP/2 client and server codecs. */ @@ -350,7 +338,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable()(entry.settings_id); + } + }; + + struct SettingsEntryEquals { + bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { + return lhs.settings_id == rhs.settings_id; + } + }; + // Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint. absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { const auto it = settings_.find({identifier, 0}); @@ -45,8 +57,8 @@ class TestServerConnectionImpl : public ServerConnectionImpl, public TestCodecSe using ServerConnectionImpl::getStream; protected: - // Overrides ServerConnectionImpl::onSettings(). - void onSettings(const nghttp2_settings& settings) override { onSettingsFrame(settings); } + // Overrides ServerConnectionImpl::onSettingsForTest(). + void onSettingsForTest(const nghttp2_settings& settings) override { onSettingsFrame(settings); } }; class TestClientConnectionImpl : public ClientConnectionImpl, public TestCodecSettingsProvider { @@ -62,8 +74,8 @@ class TestClientConnectionImpl : public ClientConnectionImpl, public TestCodecSe using ConnectionImpl::sendPendingFrames; protected: - // Overrides ClientConnectionImpl::onSettings(). - void onSettings(const nghttp2_settings& settings) override { onSettingsFrame(settings); } + // Overrides ClientConnectionImpl::onSettingsForTest(). + void onSettingsForTest(const nghttp2_settings& settings) override { onSettingsFrame(settings); } }; } // namespace Http2 From e48de85ab9f6553a8252890d3d67dc8ac4e8f170 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 16 Mar 2020 00:35:11 -0400 Subject: [PATCH 23/31] Fix spelling. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e0c4769e82c29..d2fb107a53530 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -960,7 +960,7 @@ void ConnectionImpl::sendSettings( insertParameter({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}); if (!result) { ENVOY_CONN_LOG(warn, - "the named allow_connect SETTINGS parameter is being overriden by a custom " + "the named allow_connect SETTINGS parameter is being overridden by a custom " "parameter with value {}", connection_, http2_options.allow_connect()); } From 59e2dfe73f26dcb4798ddda88d7b1c45011c5990 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 16 Mar 2020 11:52:58 -0400 Subject: [PATCH 24/31] Fix cluster memory usage expectations. Signed-off-by: Andres Guedez --- test/integration/stats_integration_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 7e5af99d8121e..ce12a2bde1722 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -267,7 +267,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // upgrading. // 2020/02/13 10042 43797 44136 Metadata: Metadata are shared across different // clusters and hosts. - // 2020/02/18 9964 44327 44600 http2: support custom SETTINGS parameters. + // 2020/03/16 9964 44085 44600 http2: support custom SETTINGS parameters. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -281,7 +281,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 44327); + EXPECT_MEMORY_EQ(m_per_cluster, 44085); EXPECT_MEMORY_LE(m_per_cluster, 44600); } @@ -322,7 +322,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2019/01/09 9227 35772 36500 router: per-cluster histograms w/ timeout budget // 2020/01/12 9633 35932 36500 config: support recovery of original message when // upgrading. - // 2020/02/18 9964 36462 36800 http2: support custom SETTINGS parameters. + // 2020/03/16 9964 36220 36800 http2: support custom SETTINGS parameters. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -336,7 +336,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 36462); + EXPECT_MEMORY_EQ(m_per_cluster, 36220); EXPECT_MEMORY_LE(m_per_cluster, 36800); } From 3f513d4088241b721cfd80abf5982c6b57a2a34b Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 16 Mar 2020 12:50:49 -0400 Subject: [PATCH 25/31] Avoid using deprecated router filter name. Signed-off-by: Andres Guedez --- .../filters/network/http_connection_manager/config_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index f0cc96531034e..ed31536054dff 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1155,7 +1155,7 @@ stat_prefix: my_stat_prefix route: cluster: fake_cluster http_filters: -- name: envoy.router +- name: envoy.filters.http.router typed_config: {} http2_protocol_options: hpack_table_size: 1024 @@ -1217,7 +1217,7 @@ stat_prefix: my_stat_prefix route: cluster: fake_cluster http_filters: -- name: envoy.router +- name: envoy.filters.http.router typed_config: {} http2_protocol_options: allow_connect: true From 53bf245cfeb79a07ca4b9292071b1b4cdc018174 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 16 Mar 2020 23:20:10 -0400 Subject: [PATCH 26/31] Change warning to debug log. Signed-off-by: Andres Guedez --- source/common/http/http2/codec_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index d2fb107a53530..aed4043a1c21d 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -959,7 +959,7 @@ void ConnectionImpl::sendSettings( const bool result = insertParameter({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}); if (!result) { - ENVOY_CONN_LOG(warn, + ENVOY_CONN_LOG(debug, "the named allow_connect SETTINGS parameter is being overridden by a custom " "parameter with value {}", connection_, http2_options.allow_connect()); From cf8f7f2250f6301a3ddd3a1e025b40fcc505584e Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 18 Mar 2020 09:06:11 -0400 Subject: [PATCH 27/31] Simplify H/2 custom settings parameters validation logic. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 15 +++--- api/envoy/config/core/v3/protocol.proto | 15 +++--- .../envoy/api/v2/core/protocol.proto | 15 +++--- .../envoy/config/core/v3/protocol.proto | 15 +++--- source/common/http/http2/codec_impl.cc | 17 +------ source/common/http/utility.cc | 9 ++-- test/common/http/http2/codec_impl_test.cc | 46 ------------------- .../http_connection_manager/config_test.cc | 34 +++++++++++++- 8 files changed, 73 insertions(+), 93 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index f4472496f7e74..53aedf80c2db8 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -230,11 +230,16 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: - // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. + // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // - // Note that custom parameters specified through this field may not collide with the following - // named parameters: + // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by + // Envoy. + // + // 2. SETTINGS_ENABLE_CONNECT_PROTOCOL (0x8) is only configurable through the named field + // 'allow_connect'. + // + // Note that custom parameters specified through this field can not also be set in the + // corresponding named parameters: // // .. code-block:: text // @@ -245,8 +250,6 @@ message Http2ProtocolOptions { // 0x4 initial_stream_window_size // // Collisions will trigger config validation failure on load/update. - // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index cec11cf01a792..1df94ab0419ec 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -252,11 +252,16 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: - // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. + // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // - // Note that custom parameters specified through this field may not collide with the following - // named parameters: + // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by + // Envoy. + // + // 2. SETTINGS_ENABLE_CONNECT_PROTOCOL (0x8) is only configurable through the named field + // 'allow_connect'. + // + // Note that custom parameters specified through this field can not also be set in the + // corresponding named parameters: // // .. code-block:: text // @@ -267,8 +272,6 @@ message Http2ProtocolOptions { // 0x4 initial_stream_window_size // // Collisions will trigger config validation failure on load/update. - // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index f4472496f7e74..53aedf80c2db8 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -230,11 +230,16 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: - // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. + // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // - // Note that custom parameters specified through this field may not collide with the following - // named parameters: + // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by + // Envoy. + // + // 2. SETTINGS_ENABLE_CONNECT_PROTOCOL (0x8) is only configurable through the named field + // 'allow_connect'. + // + // Note that custom parameters specified through this field can not also be set in the + // corresponding named parameters: // // .. code-block:: text // @@ -245,8 +250,6 @@ message Http2ProtocolOptions { // 0x4 initial_stream_window_size // // Collisions will trigger config validation failure on load/update. - // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index cec11cf01a792..1df94ab0419ec 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -252,11 +252,16 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; - // Specifies SETTINGS frame parameters to be sent to the peer, with one exception: - // SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy. + // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // - // Note that custom parameters specified through this field may not collide with the following - // named parameters: + // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by + // Envoy. + // + // 2. SETTINGS_ENABLE_CONNECT_PROTOCOL (0x8) is only configurable through the named field + // 'allow_connect'. + // + // Note that custom parameters specified through this field can not also be set in the + // corresponding named parameters: // // .. code-block:: text // @@ -267,8 +272,6 @@ message Http2ProtocolOptions { // 0x4 initial_stream_window_size // // Collisions will trigger config validation failure on load/update. - // The only exception is '0x8 allow_connect', for which the named parameter field will be - // overridden by the value set in this field. // // See `IANA HTTP/2 Settings // `_ for diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index aed4043a1c21d..460d2bbda4419 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -931,14 +931,7 @@ void ConnectionImpl::sendSettings( } for (const auto it : http2_options.custom_settings_parameters()) { - // Validate that named parameters (with the exception of `allow_connect`) are not being - // overridden by custom parameters. - ASSERT((absl::c_none_of>( - {NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, NGHTTP2_SETTINGS_ENABLE_PUSH, - NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE}, - [it](const uint16_t& entry) { return entry == it.identifier().value(); }))); ASSERT(it.identifier().value() <= std::numeric_limits::max()); - const bool result = insertParameter({static_cast(it.identifier().value()), it.value().value()}); if (!result) { @@ -954,17 +947,9 @@ void ConnectionImpl::sendSettings( settings.insert( settings.end(), {{NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()}, + {NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}, {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()}}); - const bool result = - insertParameter({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()}); - if (!result) { - ENVOY_CONN_LOG(debug, - "the named allow_connect SETTINGS parameter is being overridden by a custom " - "parameter with value {}", - connection_, http2_options.allow_connect()); - } - if (!settings.empty()) { int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &settings[0], settings.size()); ASSERT(rc == 0); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index fd9ecc565099f..3f9b35f7a7678 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -51,11 +51,10 @@ void validateCustomSettingsParameters( } break; case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL: - // This parameter can not be validated due to the use of proto3 bool, which doesn't allow - // checking for presence. - // TODO: This can be fixed via a breaking API change of the type to - // `google.protobuf.BoolValue`. - break; + // An exception is made for `allow_connect` which can't be checked for presence due to the use + // of a primitive type (bool). + throw EnvoyException("the \"allow_connect\" SETTINGS parameter should only be configured " + "through the named field"); case NGHTTP2_SETTINGS_HEADER_TABLE_SIZE: if (options.has_hpack_table_size()) { duplicate_parameters.push_back("hpack_table_size"); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index a6ffd588216e8..05653042c91be 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1215,52 +1215,6 @@ TEST_P(Http2CustomSettingsTest, UserDefinedSettings) { } } -// Validates that the only custom settings parameter allowed to override a named parameter is -// 'allow_connect' (id = 0x8). -TEST_P(Http2CustomSettingsTest, UserDefinedSettingsParametersAllowConnectOverrideOnly) { - // The named allow_connect value defaults to 0. - std::vector named_parameter_overrides{ - {NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, 1}}; - setHttp2CustomSettingsParameters(getCustomOptions(), named_parameter_overrides); - initialize(); - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); - request_encoder_->encodeHeaders(request_headers, false); - for (const auto& parameter : named_parameter_overrides) { - EXPECT_THAT(getSettingsProvider().getRemoteSettingsParameterValue(parameter.identifier), - HasValue(parameter.value)); - } -} - -class Http2CustomSettingsDeathTest - : public Http2CustomSettingsTestBase, - public ::testing::TestWithParam< - ::testing::tuple> { - -public: - Http2CustomSettingsDeathTest() - : Http2CustomSettingsTestBase(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam()), - ::testing::get<2>(GetParam())), - custom_parameter_identifier_(::testing::get<3>(GetParam())) {} - -protected: - uint16_t custom_parameter_identifier_{0}; -}; -INSTANTIATE_TEST_SUITE_P(Http2CodecImplTestDefaultSettings, Http2CustomSettingsDeathTest, - ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE, - HTTP2SETTINGS_DEFAULT_COMBINE, ::testing::Bool(), - ::testing::Values(0x1, 0x2, 0x3, 0x4))); - -// Validates that overriding named parameters with custom parameters will trigger an ASSERT(). -// NOTE: `allow_connect` is the only exception since presence can not be checked without a breaking -// API change. -TEST_P(Http2CustomSettingsDeathTest, DisallowNamedParameterOverrides) { - std::vector custom_parameters{{custom_parameter_identifier_, 1}}; - setHttp2CustomSettingsParameters(client_http2_options_, custom_parameters); - EXPECT_DEBUG_DEATH(initialize(), ""); -} - // Tests request headers whose size is larger than the default limit of 60K. TEST_P(Http2CodecImplTest, LargeRequestHeadersInvokeResetStream) { initialize(); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index ed31536054dff..805ad433017fa 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1202,8 +1202,13 @@ stat_prefix: my_stat_prefix EnvoyException, R"(the \{hpack_table_size,max_concurrent_streams\} HTTP/2 SETTINGS parameter\(s\) can not be)" " configured"); +} - const std::string yaml_string2 = R"EOF( +// Validates that `allow_connect` can only be configured through the named field. All other +// SETTINGS parameters can be set via the named _or_ custom parameters fields, but `allow_connect` +// required an exception due to the use of a primitive type for which presence can not be checked. +TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsAllowConnectOnlyViaNamedField) { + const std::string yaml_string = R"EOF( codec_type: http2 stat_prefix: my_stat_prefix route_config: @@ -1220,10 +1225,35 @@ stat_prefix: my_stat_prefix - name: envoy.filters.http.router typed_config: {} http2_protocol_options: - allow_connect: true custom_settings_parameters: - { identifier: 8, value: 0 } )EOF"; + EXPECT_THROW_WITH_REGEX( + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_, http_tracer_manager_), + EnvoyException, + "the \"allow_connect\" SETTINGS parameter should only be configured through the named field"); + + const std::string yaml_string2 = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.filters.http.router + typed_config: {} +http2_protocol_options: + allow_connect: true + )EOF"; HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string2), context_, date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_); From ad297e2fd610e0d752ee4b794b8239764c9e147c Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 18 Mar 2020 09:10:38 -0400 Subject: [PATCH 28/31] Change comment wording. Signed-off-by: Andres Guedez --- source/common/http/utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 3f9b35f7a7678..e2bda62a99fab 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -53,7 +53,7 @@ void validateCustomSettingsParameters( case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL: // An exception is made for `allow_connect` which can't be checked for presence due to the use // of a primitive type (bool). - throw EnvoyException("the \"allow_connect\" SETTINGS parameter should only be configured " + throw EnvoyException("the \"allow_connect\" SETTINGS parameter must only be configured " "through the named field"); case NGHTTP2_SETTINGS_HEADER_TABLE_SIZE: if (options.has_hpack_table_size()) { From 6ae55cd08cf2d23d25991d2e4d929982e1016695 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 18 Mar 2020 09:20:32 -0400 Subject: [PATCH 29/31] Minor cleaup. Signed-off-by: Andres Guedez --- .../filters/network/http_connection_manager/config_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 805ad433017fa..ffb23cf83edad 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1206,7 +1206,8 @@ stat_prefix: my_stat_prefix // Validates that `allow_connect` can only be configured through the named field. All other // SETTINGS parameters can be set via the named _or_ custom parameters fields, but `allow_connect` -// required an exception due to the use of a primitive type for which presence can not be checked. +// required an exception due to the use of a primitive type which does not support presence +// checks. TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsAllowConnectOnlyViaNamedField) { const std::string yaml_string = R"EOF( codec_type: http2 From a3dd8bfae48109bd461eeda6700de2702e08e697 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 18 Mar 2020 16:21:55 -0400 Subject: [PATCH 30/31] Enforce stricter validation of HTTP/2 custom settings parameters. Signed-off-by: Andres Guedez --- api/envoy/api/v2/core/protocol.proto | 4 +- api/envoy/config/core/v3/protocol.proto | 4 +- .../envoy/api/v2/core/protocol.proto | 4 +- .../envoy/config/core/v3/protocol.proto | 4 +- source/common/http/http2/codec_impl.cc | 6 +-- source/common/http/utility.cc | 38 +++++++++++++------ source/common/http/utility.h | 13 +++++++ test/common/http/http2/codec_impl_test_util.h | 17 ++------- .../http_connection_manager/config_test.cc | 36 +++++++++++++++++- 9 files changed, 91 insertions(+), 35 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 53aedf80c2db8..e6a7ddaf34149 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -230,6 +230,7 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + // [#not-implemented-hide:] // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by @@ -249,7 +250,8 @@ message Http2ProtocolOptions { // 0x3 max_concurrent_streams // 0x4 initial_stream_window_size // - // Collisions will trigger config validation failure on load/update. + // Collisions will trigger config validation failure on load/update. Likewise, inconsistencies + // between custom parameters with the same identifier will trigger a failure. // // See `IANA HTTP/2 Settings // `_ for diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 1df94ab0419ec..64c5ec1ea759f 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -252,6 +252,7 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + // [#not-implemented-hide:] // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by @@ -271,7 +272,8 @@ message Http2ProtocolOptions { // 0x3 max_concurrent_streams // 0x4 initial_stream_window_size // - // Collisions will trigger config validation failure on load/update. + // Collisions will trigger config validation failure on load/update. Likewise, inconsistencies + // between custom parameters with the same identifier will trigger a failure. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/api/v2/core/protocol.proto b/generated_api_shadow/envoy/api/v2/core/protocol.proto index 53aedf80c2db8..e6a7ddaf34149 100644 --- a/generated_api_shadow/envoy/api/v2/core/protocol.proto +++ b/generated_api_shadow/envoy/api/v2/core/protocol.proto @@ -230,6 +230,7 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + // [#not-implemented-hide:] // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by @@ -249,7 +250,8 @@ message Http2ProtocolOptions { // 0x3 max_concurrent_streams // 0x4 initial_stream_window_size // - // Collisions will trigger config validation failure on load/update. + // Collisions will trigger config validation failure on load/update. Likewise, inconsistencies + // between custom parameters with the same identifier will trigger a failure. // // See `IANA HTTP/2 Settings // `_ for diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 1df94ab0419ec..64c5ec1ea759f 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -252,6 +252,7 @@ message Http2ProtocolOptions { // See `RFC7540, sec. 8.1 `_ for details. bool stream_error_on_invalid_http_messaging = 12; + // [#not-implemented-hide:] // Specifies SETTINGS frame parameters to be sent to the peer, with two exceptions: // // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by @@ -271,7 +272,8 @@ message Http2ProtocolOptions { // 0x3 max_concurrent_streams // 0x4 initial_stream_window_size // - // Collisions will trigger config validation failure on load/update. + // Collisions will trigger config validation failure on load/update. Likewise, inconsistencies + // between custom parameters with the same identifier will trigger a failure. // // See `IANA HTTP/2 Settings // `_ for diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 460d2bbda4419..cd3235d0c611d 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -934,11 +934,7 @@ void ConnectionImpl::sendSettings( ASSERT(it.identifier().value() <= std::numeric_limits::max()); const bool result = insertParameter({static_cast(it.identifier().value()), it.value().value()}); - if (!result) { - ENVOY_CONN_LOG(debug, "duplicate custom settings parameter with id {:#x}, value {}", - connection_, it.identifier().value(), it.value().value()); - continue; - } + ASSERT(result); ENVOY_CONN_LOG(debug, "adding custom settings parameter with id {:#x} to {}", connection_, it.identifier().value(), it.value().value()); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e2bda62a99fab..ea4aa41cd4e80 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -39,10 +39,22 @@ namespace { void validateCustomSettingsParameters( const envoy::config::core::v3::Http2ProtocolOptions& options) { - std::vector duplicate_parameters; + std::vector parameter_collisions, custom_parameter_collisions; + std::unordered_set + custom_parameters; // User defined and named parameters with the same SETTINGS identifier can not both be set. for (const auto it : options.custom_settings_parameters()) { ASSERT(it.identifier().value() <= std::numeric_limits::max()); + // Check for custom parameter inconsistencies. + const auto result = custom_parameters.insert( + {static_cast(it.identifier().value()), it.value().value()}); + if (!result.second) { + if (result.first->value != it.value().value()) { + custom_parameter_collisions.push_back( + absl::StrCat("0x", absl::Hex(it.identifier().value(), absl::kZeroPad2))); + // Fall through to allow unbatched exceptions to throw first. + } + } switch (it.identifier().value()) { case NGHTTP2_SETTINGS_ENABLE_PUSH: if (it.value().value() == 1) { @@ -51,23 +63,23 @@ void validateCustomSettingsParameters( } break; case NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL: - // An exception is made for `allow_connect` which can't be checked for presence due to the use - // of a primitive type (bool). + // An exception is made for `allow_connect` which can't be checked for presence due to the + // use of a primitive type (bool). throw EnvoyException("the \"allow_connect\" SETTINGS parameter must only be configured " "through the named field"); case NGHTTP2_SETTINGS_HEADER_TABLE_SIZE: if (options.has_hpack_table_size()) { - duplicate_parameters.push_back("hpack_table_size"); + parameter_collisions.push_back("hpack_table_size"); } break; case NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS: if (options.has_max_concurrent_streams()) { - duplicate_parameters.push_back("max_concurrent_streams"); + parameter_collisions.push_back("max_concurrent_streams"); } break; case NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE: if (options.has_initial_stream_window_size()) { - duplicate_parameters.push_back("initial_stream_window_size"); + parameter_collisions.push_back("initial_stream_window_size"); } break; default: @@ -76,14 +88,16 @@ void validateCustomSettingsParameters( } } - if (!duplicate_parameters.empty()) { + if (!custom_parameter_collisions.empty()) { + throw EnvoyException(fmt::format( + "inconsistent HTTP/2 custom SETTINGS parameter(s) detected; identifiers = {{{}}}", + absl::StrJoin(custom_parameter_collisions, ","))); + } + if (!parameter_collisions.empty()) { throw EnvoyException(fmt::format( "the {{{}}} HTTP/2 SETTINGS parameter(s) can not be configured through both named and " - "custom " - "parameters", - absl::StrJoin(duplicate_parameters, ",", [](std::string* out, const std::string& str) { - absl::StrAppend(out, str); - }))); + "custom parameters", + absl::StrJoin(parameter_collisions, ","))); } } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 16a3b6b00ed92..2daa237894d17 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -17,11 +17,24 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "nghttp2/nghttp2.h" namespace Envoy { namespace Http2 { namespace Utility { +struct SettingsEntryHash { + size_t operator()(const nghttp2_settings_entry& entry) const { + return absl::Hash()(entry.settings_id); + } +}; + +struct SettingsEntryEquals { + bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { + return lhs.settings_id == rhs.settings_id; + } +}; + // Limits and defaults for `envoy::config::core::v3::Http2ProtocolOptions` protos. struct OptionsLimits { // disable HPACK compression diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index d2bfbf6f9285a..3ab1af4974d33 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -2,6 +2,7 @@ #include "envoy/http/codec.h" +#include "common/http/utility.h" #include "common/http/http2/codec_impl.h" namespace Envoy { @@ -10,18 +11,6 @@ namespace Http2 { class TestCodecSettingsProvider { public: - struct SettingsEntryHash { - size_t operator()(const nghttp2_settings_entry& entry) const { - return absl::Hash()(entry.settings_id); - } - }; - - struct SettingsEntryEquals { - bool operator()(const nghttp2_settings_entry& lhs, const nghttp2_settings_entry& rhs) const { - return lhs.settings_id == rhs.settings_id; - } - }; - // Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint. absl::optional getRemoteSettingsParameterValue(int32_t identifier) const { const auto it = settings_.find({identifier, 0}); @@ -42,7 +31,9 @@ class TestCodecSettingsProvider { } private: - std::unordered_set settings_; + std::unordered_set + settings_; }; class TestServerConnectionImpl : public ServerConnectionImpl, public TestCodecSettingsProvider { diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index ffb23cf83edad..9dcef518d2cc2 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1234,7 +1234,7 @@ stat_prefix: my_stat_prefix date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_), EnvoyException, - "the \"allow_connect\" SETTINGS parameter should only be configured through the named field"); + "the \"allow_connect\" SETTINGS parameter must only be configured through the named field"); const std::string yaml_string2 = R"EOF( codec_type: http2 @@ -1324,6 +1324,40 @@ stat_prefix: my_stat_prefix "server push is not supported by Envoy and can not be enabled via a SETTINGS parameter."); } +// Validates that inconsistent custom parameters are rejected. +TEST_F(HttpConnectionManagerConfigTest, UserDefinedSettingsRejectInconsistentCustomParameters) { + const std::string yaml_string = R"EOF( +codec_type: http2 +stat_prefix: my_stat_prefix +route_config: + virtual_hosts: + - name: default + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: fake_cluster +http_filters: +- name: envoy.filters.http.router + typed_config: {} +http2_protocol_options: + custom_settings_parameters: + - { identifier: 10, value: 0 } + - { identifier: 10, value: 1 } + - { identifier: 12, value: 10 } + - { identifier: 14, value: 1 } + - { identifier: 12, value: 10 } + )EOF"; + EXPECT_THROW_WITH_REGEX( + HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_, http_tracer_manager_), + EnvoyException, + R"(inconsistent HTTP/2 custom SETTINGS parameter\(s\) detected; identifiers = \{0x0a\})"); +} + // Test that the deprecated extension name still functions. TEST_F(HttpConnectionManagerConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) { const std::string deprecated_name = "envoy.http_connection_manager"; From e5b5e537816bf457960d2e24ba2ebffc595c18b1 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 18 Mar 2020 20:41:11 -0400 Subject: [PATCH 31/31] Fix format. Signed-off-by: Andres Guedez --- test/common/http/http2/codec_impl_test_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 3ab1af4974d33..80e3c2d096b19 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -2,8 +2,8 @@ #include "envoy/http/codec.h" -#include "common/http/utility.h" #include "common/http/http2/codec_impl.h" +#include "common/http/utility.h" namespace Envoy { namespace Http {