From 77459a174b2fc265c24e4752b2fdb8b5f097e25a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 18 May 2017 20:20:42 +0800 Subject: [PATCH 01/58] Http::CodecOptions struct and interface changes, make it compiles --- include/envoy/http/codec.h | 8 ++++++- include/envoy/upstream/BUILD | 1 + include/envoy/upstream/upstream.h | 3 ++- source/common/http/http2/codec_impl.cc | 18 +++++++++------- source/common/http/http2/codec_impl.h | 11 +++++----- source/common/http/utility.cc | 21 ++++++++++--------- source/common/http/utility.h | 2 +- source/common/upstream/upstream_impl.h | 4 ++-- .../config/network/http_connection_manager.h | 2 +- 9 files changed, 42 insertions(+), 28 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index e0f8e803e195c..e7bb362e609b5 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -150,11 +150,17 @@ class ConnectionCallbacks { /** * A list of options that can be specified when creating a codec. */ -class CodecOptions { +class CodecOptionFlags { public: static const uint64_t NoCompression = 0x1; }; +struct CodecOptions { + uint64_t flag_options; + uint32_t max_concurrent_streams; + uint32_t initial_window_size; +}; + /** * A connection (client or server) that owns multiple streams. */ diff --git a/include/envoy/upstream/BUILD b/include/envoy/upstream/BUILD index 77dd3fe7fef3c..17d0168f2cfd3 100644 --- a/include/envoy/upstream/BUILD +++ b/include/envoy/upstream/BUILD @@ -74,6 +74,7 @@ envoy_cc_library( ":load_balancer_type_interface", ":resource_manager_interface", "//include/envoy/common:optional", + "//include/envoy/http:codec_interface", "//include/envoy/network:connection_interface", "//include/envoy/ssl:context_interface", ], diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index fbc2ce337d2e5..2eb5a5ca9d7bd 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -9,6 +9,7 @@ #include #include "envoy/common/optional.h" +#include "envoy/http/codec.h" #include "envoy/network/connection.h" #include "envoy/ssl/context.h" #include "envoy/upstream/load_balancer_type.h" @@ -246,7 +247,7 @@ class ClusterInfo { * @return uint64_t HTTP codec options for HTTP connections created on behalf of this cluster. * @see Http::CodecOptions. */ - virtual uint64_t httpCodecOptions() const PURE; + virtual Http::CodecOptions httpCodecOptions() const PURE; /** * @return the type of load balancing that the cluster should use. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 02e286344a187..97dcaaba5a2fb 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -506,12 +506,16 @@ void ConnectionImpl::sendPendingFrames() { } } -void ConnectionImpl::sendSettings(uint64_t codec_options) { +void ConnectionImpl::sendSettings(const CodecOptions& codec_options) { std::vector iv = { - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, MAX_CONCURRENT_STREAMS}, - {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, DEFAULT_WINDOW_SIZE}}; - - if (codec_options & CodecOptions::NoCompression) { + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, codec_options.max_concurrent_streams + ? codec_options.max_concurrent_streams + : MAX_CONCURRENT_STREAMS}, + {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, codec_options.initial_window_size + ? codec_options.initial_window_size + : DEFAULT_WINDOW_SIZE}}; + + if (codec_options.flag_options & CodecOptionFlags::NoCompression) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); conn_log_debug("disabling header compression", connection_); } @@ -610,7 +614,7 @@ ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks, Stats::Scope& stats, - uint64_t codec_options) + const CodecOptions& codec_options) : ConnectionImpl(connection, stats), callbacks_(callbacks) { nghttp2_session_client_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options_.options()); @@ -648,7 +652,7 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, - Stats::Store& stats, uint64_t codec_options) + Stats::Store& stats, const CodecOptions& codec_options) : ConnectionImpl(connection, stats), callbacks_(callbacks) { nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options_.options()); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 3997de689d2ea..3fd31f9d7381b 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -82,7 +82,7 @@ class ConnectionImpl : public virtual Connection, Logger::Loggable CONTINUE_HEADER; @@ -233,7 +234,7 @@ class ConnectionImpl : public virtual Connection, Logger::Loggablevalue().c_str()); } -uint64_t Utility::parseCodecOptions(const Json::Object& config) { - uint64_t ret = 0; - std::string options = config.getString("http_codec_options", ""); - for (const std::string& option : StringUtil::split(options, ',')) { - if (option == "no_compression") { - ret |= CodecOptions::NoCompression; - } else { - throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); - } - } +CodecOptions Utility::parseCodecOptions(const Json::Object& config) { + UNREFERENCED_PARAMETER(config); + CodecOptions ret{}; + // std::string options = config.getString("http_codec_options", ""); + // for (const std::string& option : StringUtil::split(options, ',')) { + // if (option == "no_compression") { + // ret |= CodecOptions::NoCompression; + // } else { + // throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); + // } + //} return ret; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 91ce05d82c426..38d2c99d2340a 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -67,7 +67,7 @@ class Utility { * @return uint64_t parse a "http_codec_options" JSON field and turn it into a bitmask of * CodecOption values. */ - static uint64_t parseCodecOptions(const Json::Object& config); + static CodecOptions parseCodecOptions(const Json::Object& config); /** * Create a locally generated response using filter callbacks. diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index c340cd0a88b5f..b8fe273c4f307 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -176,7 +176,7 @@ class ClusterInfoImpl : public ClusterInfo { return per_connection_buffer_limit_bytes_; } uint64_t features() const override { return features_; } - uint64_t httpCodecOptions() const override { return http_codec_options_; } + Http::CodecOptions httpCodecOptions() const override { return http_codec_options_; } LoadBalancerType lbType() const override { return lb_type_; } bool maintenanceMode() const override; uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } @@ -209,7 +209,7 @@ class ClusterInfoImpl : public ClusterInfo { mutable ClusterStats stats_; Ssl::ClientContextPtr ssl_ctx_; const uint64_t features_; - const uint64_t http_codec_options_; + const Http::CodecOptions http_codec_options_; mutable ResourceManagers resource_managers_; const std::string maintenance_mode_runtime_key_; LoadBalancerType lb_type_; diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index b040329d4d21e..dc11957eac7c0 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -128,7 +128,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerTracingStats tracing_stats_; bool use_remote_address_{}; CodecType codec_type_; - const uint64_t codec_options_; + const Http::CodecOptions codec_options_; std::string server_name_; Http::TracingConnectionManagerConfigPtr tracing_config_; Optional user_agent_; From b75039ab5d9112c4a5504d10170fa88cd1403fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Fri, 19 May 2017 10:58:08 +0800 Subject: [PATCH 02/58] following mattklein123's review --- include/envoy/http/codec.h | 8 ++++---- include/envoy/upstream/upstream.h | 2 +- source/common/http/http2/codec_impl.cc | 12 ++++++------ source/common/http/utility.cc | 2 +- source/common/upstream/upstream_impl.h | 2 +- test/common/http/http2/codec_impl_test.cc | 2 +- test/common/http/utility_test.cc | 4 ++-- test/common/upstream/upstream_impl_test.cc | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index e7bb362e609b5..206e2a155fb57 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -152,13 +152,13 @@ class ConnectionCallbacks { */ class CodecOptionFlags { public: - static const uint64_t NoCompression = 0x1; + static const uint64_t DisableDynamicHPACKTable = 0x1; }; struct CodecOptions { - uint64_t flag_options; - uint32_t max_concurrent_streams; - uint32_t initial_window_size; + uint64_t flag_options_; + uint32_t max_concurrent_streams_; + uint32_t initial_window_size_; }; /** diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 2eb5a5ca9d7bd..fc1041ff24f28 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -247,7 +247,7 @@ class ClusterInfo { * @return uint64_t HTTP codec options for HTTP connections created on behalf of this cluster. * @see Http::CodecOptions. */ - virtual Http::CodecOptions httpCodecOptions() const PURE; + virtual const Http::CodecOptions& httpCodecOptions() const PURE; /** * @return the type of load balancing that the cluster should use. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 97dcaaba5a2fb..9bcb9daeed5e4 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -508,16 +508,16 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const CodecOptions& codec_options) { std::vector iv = { - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, codec_options.max_concurrent_streams - ? codec_options.max_concurrent_streams + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, codec_options.max_concurrent_streams_ + ? codec_options.max_concurrent_streams_ : MAX_CONCURRENT_STREAMS}, - {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, codec_options.initial_window_size - ? codec_options.initial_window_size + {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, codec_options.initial_window_size_ + ? codec_options.initial_window_size_ : DEFAULT_WINDOW_SIZE}}; - if (codec_options.flag_options & CodecOptionFlags::NoCompression) { + if (codec_options.flag_options_ & CodecOptionFlags::DisableDynamicHPACKTable) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); - conn_log_debug("disabling header compression", connection_); + conn_log_debug("setting HPACK dynamic table size to 0", connection_); } int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 39cdd8340785b..fac20078853b0 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -142,7 +142,7 @@ CodecOptions Utility::parseCodecOptions(const Json::Object& config) { // std::string options = config.getString("http_codec_options", ""); // for (const std::string& option : StringUtil::split(options, ',')) { // if (option == "no_compression") { - // ret |= CodecOptions::NoCompression; + // ret |= CodecOptions::DisableDynamicHPACKTable; // } else { // throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); // } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index b8fe273c4f307..894ea7f133b3d 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -176,7 +176,7 @@ class ClusterInfoImpl : public ClusterInfo { return per_connection_buffer_limit_bytes_; } uint64_t features() const override { return features_; } - Http::CodecOptions httpCodecOptions() const override { return http_codec_options_; } + const Http::CodecOptions& httpCodecOptions() const override { return http_codec_options_; } LoadBalancerType lbType() const override { return lb_type_; } bool maintenanceMode() const override; uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 58538fd632693..c427f184ba223 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -237,7 +237,7 @@ TEST_P(Http2CodecImplTest, DeferredReset) { } INSTANTIATE_TEST_CASE_P(Http2CodecImplTest, Http2CodecImplTest, - testing::Values(0, CodecOptions::NoCompression)); + testing::Values(0, CodecOptions::DisableDynamicHPACKTable)); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 84d3f30979382..994d03e1d9353 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Http { // Satisfy linker -const uint64_t CodecOptions::NoCompression; +const uint64_t CodecOptions::DisableDynamicHPACKTable; TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); @@ -99,7 +99,7 @@ TEST(HttpUtility, parseCodecOptions) { { Json::ObjectPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}"); - EXPECT_EQ(CodecOptions::NoCompression, Utility::parseCodecOptions(*json)); + EXPECT_EQ(CodecOptions::DisableDynamicHPACKTable, Utility::parseCodecOptions(*json)); } { diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 19d0a5b7ce9a3..835a65ab32a09 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -146,7 +146,7 @@ TEST(StrictDnsClusterImplTest, Basic) { EXPECT_EQ(3U, cluster.info()->resourceManager(ResourcePriority::High).requests().max()); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(static_cast(Http::CodecOptions::NoCompression), + EXPECT_EQ(static_cast(Http::CodecOptions::DisableDynamicHPACKTable), cluster.info()->httpCodecOptions()); cluster.info()->stats().upstream_rq_total_.inc(); From 6d1a1993d3935b3d7fdfbb55d4fa742c39555b36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Fri, 19 May 2017 11:54:20 +0800 Subject: [PATCH 03/58] refactor around original CodecOptions 1. change original uint32_t CodecOptions to Http2Settings, and move CodecOptions into Http2Settings; 2. default ctor for Http2Settings, and move default consts into Http2Settings; 3. hard code maxConcurrentStreams return temporarily till PR #981. --- include/envoy/http/codec.h | 26 ++++++++++++++----- include/envoy/upstream/upstream.h | 6 ++--- source/common/http/codec_client.cc | 2 +- source/common/http/http2/codec_impl.cc | 26 +++++++++---------- source/common/http/http2/codec_impl.h | 13 +++------- source/common/http/http2/conn_pool.cc | 5 +++- source/common/http/utility.cc | 6 ++--- source/common/http/utility.h | 2 +- source/common/upstream/upstream_impl.cc | 2 +- source/common/upstream/upstream_impl.h | 4 +-- .../config/network/http_connection_manager.cc | 6 ++--- .../config/network/http_connection_manager.h | 2 +- test/common/http/http2/codec_impl_test.cc | 5 ++-- test/common/http/utility_test.cc | 9 ++++--- test/common/upstream/upstream_impl_test.cc | 6 ++--- 15 files changed, 64 insertions(+), 56 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 206e2a155fb57..b1796c0e5ab96 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -148,17 +148,29 @@ class ConnectionCallbacks { }; /** - * A list of options that can be specified when creating a codec. + * http/2 settings */ -class CodecOptionFlags { -public: - static const uint64_t DisableDynamicHPACKTable = 0x1; -}; +struct Http2Settings { + /** + * A list of options that can be specified when creating a codec. + */ + struct CodecOption { + static const uint64_t DisableDynamicHPACKTable = 0x1; + }; + + Http2Settings() + : codec_options_(0), max_concurrent_streams_(DEFAULT_MAX_CONCURRENT_STREAMS), + initial_window_size_(DEFAULT_INITIAL_WINDOW_SIZE) {} -struct CodecOptions { - uint64_t flag_options_; + uint64_t codec_options_; uint32_t max_concurrent_streams_; uint32_t initial_window_size_; + +private: + static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; + // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if + // needed. + static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024; }; /** diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index fc1041ff24f28..631ed63757af5 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -244,10 +244,10 @@ class ClusterInfo { virtual uint64_t features() const PURE; /** - * @return uint64_t HTTP codec options for HTTP connections created on behalf of this cluster. - * @see Http::CodecOptions. + * @return Http::Http2Settings for http/2 connections created on behalf of this cluster. + * @see Http::Http2Settings. */ - virtual const Http::CodecOptions& httpCodecOptions() const PURE; + virtual const Http::Http2Settings& http2Settings() const PURE; /** * @return the type of load balancing that the cluster should use. diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 3c0a402921834..9092f4ce37e28 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -128,7 +128,7 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne } case Type::HTTP2: { codec_.reset(new Http2::ClientConnectionImpl(*connection_, *this, host->cluster().statsScope(), - host->cluster().httpCodecOptions())); + host->cluster().http2Settings())); break; } } diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 9bcb9daeed5e4..e5c90b349ef53 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -506,16 +506,12 @@ void ConnectionImpl::sendPendingFrames() { } } -void ConnectionImpl::sendSettings(const CodecOptions& codec_options) { +void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { std::vector iv = { - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, codec_options.max_concurrent_streams_ - ? codec_options.max_concurrent_streams_ - : MAX_CONCURRENT_STREAMS}, - {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, codec_options.initial_window_size_ - ? codec_options.initial_window_size_ - : DEFAULT_WINDOW_SIZE}}; - - if (codec_options.flag_options_ & CodecOptionFlags::DisableDynamicHPACKTable) { + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, + {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; + + if (http2_settings.codec_options_ & Http::Http2Settings::CodecOption::DisableDynamicHPACKTable) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); conn_log_debug("setting HPACK dynamic table size to 0", connection_); } @@ -526,7 +522,9 @@ void ConnectionImpl::sendSettings(const CodecOptions& codec_options) { // Increase connection window size up to our default size. rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, - DEFAULT_WINDOW_SIZE - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); + http2_settings.initial_window_size_ - + NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); + // TODO: ensure mimium initial_window_size from json schema ASSERT(rc == 0); } @@ -614,11 +612,11 @@ ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks, Stats::Scope& stats, - const CodecOptions& codec_options) + const Http2Settings& http2_settings) : ConnectionImpl(connection, stats), callbacks_(callbacks) { nghttp2_session_client_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options_.options()); - sendSettings(codec_options); + sendSettings(http2_settings); } Http::StreamEncoder& ClientConnectionImpl::newStream(StreamDecoder& decoder) { @@ -652,11 +650,11 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, - Stats::Store& stats, const CodecOptions& codec_options) + Stats::Store& stats, const Http2Settings& http2_settings) : ConnectionImpl(connection, stats), callbacks_(callbacks) { nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options_.options()); - sendSettings(codec_options); + sendSettings(http2_settings); } 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 3fd31f9d7381b..29020b048a2c4 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -82,8 +82,6 @@ class ConnectionImpl : public virtual Connection, Logger::Loggable CONTINUE_HEADER; Network::Connection& connection_; @@ -234,7 +227,7 @@ class ConnectionImpl : public virtual Connection, Logger::Loggablevalue().c_str()); } -CodecOptions Utility::parseCodecOptions(const Json::Object& config) { +Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { UNREFERENCED_PARAMETER(config); - CodecOptions ret{}; + Http2Settings ret; // std::string options = config.getString("http_codec_options", ""); // for (const std::string& option : StringUtil::split(options, ',')) { // if (option == "no_compression") { - // ret |= CodecOptions::DisableDynamicHPACKTable; + // ret |= Http2Settings::CodecOptions::DisableDynamicHPACKTable; // } else { // throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); // } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 38d2c99d2340a..daef712cd0bd0 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -67,7 +67,7 @@ class Utility { * @return uint64_t parse a "http_codec_options" JSON field and turn it into a bitmask of * CodecOption values. */ - static CodecOptions parseCodecOptions(const Json::Object& config); + static Http2Settings parseHttp2Settings(const Json::Object& config); /** * Create a locally generated response using filter callbacks. diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 72a0b5342a211..9c03f255b3e2f 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -74,7 +74,7 @@ ClusterInfoImpl::ClusterInfoImpl(const Json::Object& config, Runtime::Loader& ru config.getInteger("per_connection_buffer_limit_bytes", 1024 * 1024)), stats_scope_(stats.createScope(fmt::format("cluster.{}.", name_))), stats_(generateStats(*stats_scope_)), features_(parseFeatures(config)), - http_codec_options_(Http::Utility::parseCodecOptions(config)), + http2_settings_(Http::Utility::parseHttp2Settings(config)), resource_managers_(config, runtime, name_), maintenance_mode_runtime_key_(fmt::format("upstream.maintenance_mode.{}", name_)) { diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 894ea7f133b3d..b94a3eae62b02 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -176,7 +176,7 @@ class ClusterInfoImpl : public ClusterInfo { return per_connection_buffer_limit_bytes_; } uint64_t features() const override { return features_; } - const Http::CodecOptions& httpCodecOptions() const override { return http_codec_options_; } + const Http::Http2Settings& http2Settings() const override { return http2_settings_; } LoadBalancerType lbType() const override { return lb_type_; } bool maintenanceMode() const override; uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } @@ -209,7 +209,7 @@ class ClusterInfoImpl : public ClusterInfo { mutable ClusterStats stats_; Ssl::ClientContextPtr ssl_ctx_; const uint64_t features_; - const Http::CodecOptions http_codec_options_; + const Http::Http2Settings http2_settings_; mutable ResourceManagers resource_managers_; const std::string maintenance_mode_runtime_key_; LoadBalancerType lb_type_; diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index c2a3e474addc8..305502592a45a 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -74,7 +74,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, server.stats())), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, server.stats())), - codec_options_(Http::Utility::parseCodecOptions(config)), + http2_settings_(Http::Utility::parseHttp2Settings(config)), drain_timeout_(config.getInteger("drain_timeout_ms", 5000)), generate_request_id_(config.getBoolean("generate_request_id", true)), date_provider_(server.dispatcher(), server.threadLocal()) { @@ -179,12 +179,12 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, return Http::ServerConnectionPtr{new Http::Http1::ServerConnectionImpl(connection, callbacks)}; case CodecType::HTTP2: return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( - connection, callbacks, server_.stats(), codec_options_)}; + connection, callbacks, server_.stats(), http2_settings_)}; case CodecType::AUTO: if (HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data) == Http::Http2::ALPN_STRING) { return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( - connection, callbacks, server_.stats(), codec_options_)}; + connection, callbacks, server_.stats(), http2_settings_)}; } else { return Http::ServerConnectionPtr{ new Http::Http1::ServerConnectionImpl(connection, callbacks)}; diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index dc11957eac7c0..a07596547a6d8 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -128,7 +128,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerTracingStats tracing_stats_; bool use_remote_address_{}; CodecType codec_type_; - const Http::CodecOptions codec_options_; + const Http::Http2Settings http2_settings_; std::string server_name_; Http::TracingConnectionManagerConfigPtr tracing_config_; Optional user_agent_; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c427f184ba223..d234dfd40e997 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -236,8 +236,9 @@ TEST_P(Http2CodecImplTest, DeferredReset) { server_wrapper_.dispatch(Buffer::OwnedImpl(), server_); } -INSTANTIATE_TEST_CASE_P(Http2CodecImplTest, Http2CodecImplTest, - testing::Values(0, CodecOptions::DisableDynamicHPACKTable)); +INSTANTIATE_TEST_CASE_P( + Http2CodecImplTest, Http2CodecImplTest, + testing::Values(0, Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable)); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 994d03e1d9353..4688c56774b05 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -90,21 +90,22 @@ TEST(HttpUtility, createSslRedirectPath) { } } -TEST(HttpUtility, parseCodecOptions) { +TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectPtr json = Json::Factory::loadFromString("{}"); - EXPECT_EQ(0UL, Utility::parseCodecOptions(*json)); + EXPECT_EQ(0UL, Utility::parseHttp2Settings(*json)); } { Json::ObjectPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}"); - EXPECT_EQ(CodecOptions::DisableDynamicHPACKTable, Utility::parseCodecOptions(*json)); + EXPECT_EQ(Http2Settings::CodecOptions::DisableDynamicHPACKTable, + Utility::parseHttp2Settings(*json)); } { Json::ObjectPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}"); - EXPECT_THROW(Utility::parseCodecOptions(*json), EnvoyException); + EXPECT_THROW(Utility::parseHttp2Settings(*json), EnvoyException); } } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 835a65ab32a09..81af87007e5ed 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -146,8 +146,8 @@ TEST(StrictDnsClusterImplTest, Basic) { EXPECT_EQ(3U, cluster.info()->resourceManager(ResourcePriority::High).requests().max()); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(static_cast(Http::CodecOptions::DisableDynamicHPACKTable), - cluster.info()->httpCodecOptions()); + EXPECT_EQ(static_cast(Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable), + cluster.info()->http2Settings()); cluster.info()->stats().upstream_rq_total_.inc(); EXPECT_EQ(1UL, stats.counter("cluster.name.upstream_rq_total").value()); @@ -429,7 +429,7 @@ TEST(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(0U, cluster.info()->httpCodecOptions()); + EXPECT_EQ(0U, cluster.info()->http2Settings()); EXPECT_EQ(LoadBalancerType::Random, cluster.info()->lbType()); EXPECT_THAT(std::list({"10.0.0.1:11001", "10.0.0.2:11002"}), ContainerEq(hostListToAddresses(cluster.hosts()))); From bcd7ba6e44019a120eae53b1108787d96b04a554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Fri, 19 May 2017 17:36:07 +0800 Subject: [PATCH 04/58] all orignal tests pass, except the strange integrate core dumps. //test/integration:http2_integration_test FAILED in 1 out of 2 in 0.2s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/http2_integration_test/test.log //test/integration:http2_upstream_integration_test FAILED in 1 out of 2 in 0.2s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/http2_upstream_integration_test/test.log //test/integration:ssl_integration_test FAILED in 1 out of 2 in 2.0s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/ssl_integration_test/test.log Executed 111 out of 111 tests: 108 tests pass and 3 fail locally. There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are. root@8766e71f0008:/source# ./tools/stack_decode.py /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/bin/test/integration/http2_upstream_integration_test.runfiles/ci/test/integration/http2_upstream_integration_test [2017-05-19 09:38:02.220][17271][critical][assert] assert failure: path != nullptr: test/test_common/environment.cc:48 [2017-05-19 09:38:02.220][17271][critical][backtrace] Caught Aborted, suspect faulting address 0x4377 [2017-05-19 09:38:02.223][17271][critical] Backtrace (most recent call first) from thread 0: #1 ?? ??:0 #2 ?? ??:0 #3 #4 Envoy::TestEnvironment::getCheckedEnvVar(std::__cxx11::basic_string, std::allocator > const&) at environment.cc:48 (discriminator 1) #5 main at main.cc:23 (discriminator 2) #6 #7 ?? ??:0 #8 #9 _start at ??:? #10 --- include/envoy/http/codec.h | 3 +-- source/common/http/http2/codec_impl.cc | 2 +- source/common/http/utility.cc | 17 ++++++++--------- source/common/http/utility.h | 4 ++-- test/common/http/http2/codec_impl_test.cc | 18 ++++++++++++++++-- test/common/http/utility_test.cc | 6 +++--- test/common/upstream/upstream_impl_test.cc | 4 ++-- test/integration/fake_upstream.cc | 3 ++- test/mocks/upstream/cluster_info.h | 2 +- 9 files changed, 36 insertions(+), 23 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index b1796c0e5ab96..c8d73f3076801 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -154,7 +154,7 @@ struct Http2Settings { /** * A list of options that can be specified when creating a codec. */ - struct CodecOption { + struct CodecOptions { static const uint64_t DisableDynamicHPACKTable = 0x1; }; @@ -166,7 +166,6 @@ struct Http2Settings { uint32_t max_concurrent_streams_; uint32_t initial_window_size_; -private: static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if // needed. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e5c90b349ef53..c91f7e95a1a7a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -511,7 +511,7 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; - if (http2_settings.codec_options_ & Http::Http2Settings::CodecOption::DisableDynamicHPACKTable) { + if (http2_settings.codec_options_ & Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); conn_log_debug("setting HPACK dynamic table size to 0", connection_); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index eb30674fb0513..4eac0243b3df0 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -137,16 +137,15 @@ bool Utility::isInternalRequest(const HeaderMap& headers) { } Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { - UNREFERENCED_PARAMETER(config); Http2Settings ret; - // std::string options = config.getString("http_codec_options", ""); - // for (const std::string& option : StringUtil::split(options, ',')) { - // if (option == "no_compression") { - // ret |= Http2Settings::CodecOptions::DisableDynamicHPACKTable; - // } else { - // throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); - // } - //} + std::string options = config.getString("http_codec_options", ""); + for (const std::string& option : StringUtil::split(options, ',')) { + if (option == "no_compression") { + ret.codec_options_ |= Http2Settings::CodecOptions::DisableDynamicHPACKTable; + } else { + throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); + } + } return ret; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index daef712cd0bd0..8d6d4c7252827 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -64,8 +64,8 @@ class Utility { static bool isInternalRequest(const HeaderMap& headers); /** - * @return uint64_t parse a "http_codec_options" JSON field and turn it into a bitmask of - * CodecOption values. + * @return Http2Settings parse "http_codec_options" and "http2_settings" JSON fields and turn + * a Http2Settings values. */ static Http2Settings parseHttp2Settings(const Json::Object& config); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index d234dfd40e997..9e5a9219f2623 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -25,7 +25,17 @@ using testing::NiceMock; namespace Http { namespace Http2 { -class Http2CodecImplTest : public testing::TestWithParam { +struct Http2SettingsTestParam : public Http2Settings { + + Http2SettingsTestParam(uint64_t codec_options, uint32_t max_concurrent_streams, + uint32_t initial_window_size) { + codec_options_ = codec_options; + max_concurrent_streams_ = max_concurrent_streams; + initial_window_size_ = initial_window_size; + } +}; + +class Http2CodecImplTest : public testing::TestWithParam { public: struct ConnectionWrapper { void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) { @@ -238,7 +248,11 @@ TEST_P(Http2CodecImplTest, DeferredReset) { INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, - testing::Values(0, Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable)); + testing::Values(Http2SettingsTestParam(0, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), + Http2SettingsTestParam(Http2Settings::CodecOptions::DisableDynamicHPACKTable, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE))); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 4688c56774b05..262cdffb26bcb 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Http { // Satisfy linker -const uint64_t CodecOptions::DisableDynamicHPACKTable; +const uint64_t Http2Settings::CodecOptions::DisableDynamicHPACKTable; TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); @@ -93,14 +93,14 @@ TEST(HttpUtility, createSslRedirectPath) { TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectPtr json = Json::Factory::loadFromString("{}"); - EXPECT_EQ(0UL, Utility::parseHttp2Settings(*json)); + EXPECT_EQ(0UL, Utility::parseHttp2Settings(*json).codec_options_); } { Json::ObjectPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}"); EXPECT_EQ(Http2Settings::CodecOptions::DisableDynamicHPACKTable, - Utility::parseHttp2Settings(*json)); + Utility::parseHttp2Settings(*json).codec_options_); } { diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 81af87007e5ed..92627939ab799 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -147,7 +147,7 @@ TEST(StrictDnsClusterImplTest, Basic) { EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); EXPECT_EQ(static_cast(Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable), - cluster.info()->http2Settings()); + cluster.info()->http2Settings().codec_options_); cluster.info()->stats().upstream_rq_total_.inc(); EXPECT_EQ(1UL, stats.counter("cluster.name.upstream_rq_total").value()); @@ -429,7 +429,7 @@ TEST(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(0U, cluster.info()->http2Settings()); + EXPECT_EQ(0U, cluster.info()->http2Settings().codec_options_); EXPECT_EQ(LoadBalancerType::Random, cluster.info()->lbType()); EXPECT_THAT(std::list({"10.0.0.1:11001", "10.0.0.2:11002"}), ContainerEq(hostListToAddresses(cluster.hosts()))); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 265eccaab95f5..cbabdcc1f4005 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -130,7 +130,8 @@ FakeHttpConnection::FakeHttpConnection(QueuedConnectionWrapperPtr connection_wra if (type == Type::HTTP1) { codec_.reset(new Http::Http1::ServerConnectionImpl(connection_, *this)); } else { - codec_.reset(new Http::Http2::ServerConnectionImpl(connection_, *this, store, 0)); + codec_.reset( + new Http::Http2::ServerConnectionImpl(connection_, *this, store, Http::Http2Settings())); ASSERT(type == Type::HTTP2); } diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index cf37107265ea1..b0aa24495196c 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -28,7 +28,7 @@ class MockClusterInfo : public ClusterInfo { MOCK_CONST_METHOD0(connectTimeout, std::chrono::milliseconds()); MOCK_CONST_METHOD0(perConnectionBufferLimitBytes, uint32_t()); MOCK_CONST_METHOD0(features, uint64_t()); - MOCK_CONST_METHOD0(httpCodecOptions, uint64_t()); + MOCK_CONST_METHOD0(http2Settings, const Http::Http2Settings&()); MOCK_CONST_METHOD0(lbType, LoadBalancerType()); MOCK_CONST_METHOD0(maintenanceMode, bool()); MOCK_CONST_METHOD0(maxRequestsPerConnection, uint64_t()); From 431c909dcd7d70686cc9986c9ef019bef8942ae2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Fri, 19 May 2017 20:15:27 +0800 Subject: [PATCH 05/58] correct comment; and try trigger another CI --- source/common/http/utility.h | 4 ++-- test/common/http/http2/codec_impl_test.cc | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 8d6d4c7252827..181f92854c28e 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -64,8 +64,8 @@ class Utility { static bool isInternalRequest(const HeaderMap& headers); /** - * @return Http2Settings parse "http_codec_options" and "http2_settings" JSON fields and turn - * a Http2Settings values. + * @return Http2Settings parse "http_codec_options" and "http2_settings" JSON fields and return + * a Http2Settings. */ static Http2Settings parseHttp2Settings(const Json::Object& config); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 9e5a9219f2623..df7cf90e99346 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,7 +26,6 @@ namespace Http { namespace Http2 { struct Http2SettingsTestParam : public Http2Settings { - Http2SettingsTestParam(uint64_t codec_options, uint32_t max_concurrent_streams, uint32_t initial_window_size) { codec_options_ = codec_options; From 94fad708b171cb6044f55ea2d86d3ca2401a6178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 11:43:21 +0800 Subject: [PATCH 06/58] adapted for latest code review, except ON_CALL --- include/envoy/http/codec.h | 14 +++++--------- include/envoy/upstream/upstream.h | 2 +- source/common/http/http2/codec_impl.cc | 3 ++- source/common/http/utility.cc | 2 +- test/common/http/http2/codec_impl_test.cc | 2 +- test/common/http/utility_test.cc | 4 ++-- test/common/upstream/upstream_impl_test.cc | 2 +- 7 files changed, 13 insertions(+), 16 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index c8d73f3076801..7b198d5830d46 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -148,23 +148,19 @@ class ConnectionCallbacks { }; /** - * http/2 settings + * http/2 codec settings */ struct Http2Settings { /** * A list of options that can be specified when creating a codec. */ struct CodecOptions { - static const uint64_t DisableDynamicHPACKTable = 0x1; + static const uint64_t DISABLE_DYNAMIC_HPACK_TABLE = 0x1; }; - Http2Settings() - : codec_options_(0), max_concurrent_streams_(DEFAULT_MAX_CONCURRENT_STREAMS), - initial_window_size_(DEFAULT_INITIAL_WINDOW_SIZE) {} - - uint64_t codec_options_; - uint32_t max_concurrent_streams_; - uint32_t initial_window_size_; + uint64_t codec_options_{0}; + uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; + uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 631ed63757af5..006ef0a15a27e 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -244,7 +244,7 @@ class ClusterInfo { virtual uint64_t features() const PURE; /** - * @return Http::Http2Settings for http/2 connections created on behalf of this cluster. + * @return const Http::Http2Settings& for http/2 connections created on behalf of this cluster. * @see Http::Http2Settings. */ virtual const Http::Http2Settings& http2Settings() const PURE; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index c91f7e95a1a7a..27924591ed17d 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -511,7 +511,8 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; - if (http2_settings.codec_options_ & Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable) { + if (http2_settings.codec_options_ & + Http::Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); conn_log_debug("setting HPACK dynamic table size to 0", connection_); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 4eac0243b3df0..1528ac7dda44e 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -141,7 +141,7 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { std::string options = config.getString("http_codec_options", ""); for (const std::string& option : StringUtil::split(options, ',')) { if (option == "no_compression") { - ret.codec_options_ |= Http2Settings::CodecOptions::DisableDynamicHPACKTable; + ret.codec_options_ |= Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE; } else { throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index df7cf90e99346..d998e87431028 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -249,7 +249,7 @@ INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, testing::Values(Http2SettingsTestParam(0, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::CodecOptions::DisableDynamicHPACKTable, + Http2SettingsTestParam(Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE))); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 86385cd5e440f..d83a294b34cd1 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Http { // Satisfy linker -const uint64_t Http2Settings::CodecOptions::DisableDynamicHPACKTable; +const uint64_t Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE; TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); @@ -99,7 +99,7 @@ TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectSharedPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}"); - EXPECT_EQ(Http2Settings::CodecOptions::DisableDynamicHPACKTable, + EXPECT_EQ(Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE, Utility::parseHttp2Settings(*json).codec_options_); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index ec0380767141a..12ff3d2975ed8 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -146,7 +146,7 @@ TEST(StrictDnsClusterImplTest, Basic) { EXPECT_EQ(3U, cluster.info()->resourceManager(ResourcePriority::High).requests().max()); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(static_cast(Http::Http2Settings::CodecOptions::DisableDynamicHPACKTable), + EXPECT_EQ(static_cast(Http::Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE), cluster.info()->http2Settings().codec_options_); cluster.info()->stats().upstream_rq_total_.inc(); From d4251d42e39b94b784aba00c1d40a2f4f6189409 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 12:29:47 +0800 Subject: [PATCH 07/58] fix integration test failures by add ON_CALL for MockClusterInfo::httpSettings() --- test/mocks/upstream/cluster_info.h | 1 + test/mocks/upstream/mocks.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index b0aa24495196c..c2cf7977da78a 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -39,6 +39,7 @@ class MockClusterInfo : public ClusterInfo { MOCK_CONST_METHOD0(statsScope, Stats::Scope&()); std::string name_{"fake_cluster"}; + Http::Http2Settings http2_settings_{}; uint64_t max_requests_per_connection_{}; NiceMock stats_store_; ClusterStats stats_; diff --git a/test/mocks/upstream/mocks.cc b/test/mocks/upstream/mocks.cc index dc5843503c0ae..aea9804e32f65 100644 --- a/test/mocks/upstream/mocks.cc +++ b/test/mocks/upstream/mocks.cc @@ -61,6 +61,7 @@ MockClusterInfo::MockClusterInfo() ON_CALL(*this, connectTimeout()).WillByDefault(Return(std::chrono::milliseconds(1))); ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); + ON_CALL(*this, http2Settings()).WillByDefault(ReturnRef(http2_settings_)); ON_CALL(*this, maxRequestsPerConnection()) .WillByDefault(ReturnPointee(&max_requests_per_connection_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_)); From b7ee8344b2e216b3d4638432db8086f2bb9826b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 17:25:47 +0800 Subject: [PATCH 08/58] parse http2_settings json config --- source/common/http/utility.cc | 7 ++++++ source/common/json/config_schemas.cc | 35 +++++++++++++++++++++++++++- test/common/http/utility_test.cc | 17 ++++++++++++-- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 1528ac7dda44e..eb9dde09f1557 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -138,6 +138,7 @@ bool Utility::isInternalRequest(const HeaderMap& headers) { Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { Http2Settings ret; + std::string options = config.getString("http_codec_options", ""); for (const std::string& option : StringUtil::split(options, ',')) { if (option == "no_compression") { @@ -147,6 +148,12 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { } } + Json::ObjectSharedPtr http2_settings = config.getObject("http2_settings", true); + ret.max_concurrent_streams_ = http2_settings->getInteger( + "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); + ret.initial_window_size_ = http2_settings->getInteger( + "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); + return ret; } diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 48e758386345c..0629faf7e1c41 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -262,6 +262,21 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "type" : "string", "enum" : ["no_compression"] }, + "http2_settings" : { + "type" : "object", + "properties" : { + "max_concurrent_streams" : { + "type": "ingeger", + "minimum": 1, + "maximum" : 536870912 + }, + "initial_window_size" : { + "type": "ingeger", + "minimum": 65536, + "maximum" : 2147483647 + } + } + }, "server_name" : {"type" : "string"}, "idle_timeout_s" : {"type" : "integer"}, "drain_timeout_ms" : {"type" : "integer"}, @@ -1204,7 +1219,25 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "type" : "string", "enum" : ["http2"] }, - "http_codec_options" : {"type" : "string"}, + "http_codec_options" : { + "type" : "string", + "enum" : ["no_compression"] + }, + "http2_settings" : { + "type" : "object", + "properties" : { + "max_concurrent_streams" : { + "type": "ingeger", + "minimum": 1, + "maximum" : 536870912 + }, + "initial_window_size" : { + "type": "ingeger", + "minimum": 65536, + "maximum" : 2147483647 + } + } + }, "dns_refresh_rate_ms" : { "type" : "integer", "minimum" : 0, diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index d83a294b34cd1..8c3bf9033d4e6 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -17,6 +17,8 @@ namespace Http { // Satisfy linker const uint64_t Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE; +const uint32_t Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS; +const uint32_t Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE; TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); @@ -94,13 +96,24 @@ TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectSharedPtr json = Json::Factory::loadFromString("{}"); EXPECT_EQ(0UL, Utility::parseHttp2Settings(*json).codec_options_); + EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Utility::parseHttp2Settings(*json).max_concurrent_streams_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, + Utility::parseHttp2Settings(*json).initial_window_size_); } { - Json::ObjectSharedPtr json = - Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}"); + Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression", + "http2_settings" : { + "max_concurrent_streams": 1234, + "initial_window_size": 5678 + } + })raw"); EXPECT_EQ(Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE, Utility::parseHttp2Settings(*json).codec_options_); + EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).max_concurrent_streams_); + EXPECT_EQ(5678U, Utility::parseHttp2Settings(*json).initial_window_size_); } { From 164fe936ded7df09fd013224bd02e420d8b482a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 18:23:51 +0800 Subject: [PATCH 09/58] fix schema type spell error --- source/common/json/config_schemas.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 0629faf7e1c41..b9964fe52ab2c 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -266,12 +266,12 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "type" : "object", "properties" : { "max_concurrent_streams" : { - "type": "ingeger", + "type": "integer", "minimum": 1, "maximum" : 536870912 }, "initial_window_size" : { - "type": "ingeger", + "type": "integer", "minimum": 65536, "maximum" : 2147483647 } @@ -1227,12 +1227,12 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "type" : "object", "properties" : { "max_concurrent_streams" : { - "type": "ingeger", + "type": "integer", "minimum": 1, "maximum" : 536870912 }, "initial_window_size" : { - "type": "ingeger", + "type": "integer", "minimum": 65536, "maximum" : 2147483647 } From 4e7251bac13a443158b30d7fb13b764791e15c0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 18:40:18 +0800 Subject: [PATCH 10/58] docs for http2_settings json config --- .../configuration/cluster_manager/cluster.rst | 8 ++++++++ .../http_conn_man/http_conn_man.rst | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index 117959eb8b0ee..f3574f3a4997e 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -19,6 +19,7 @@ Cluster "ssl_context": "{...}", "features": "...", "http_codec_options": "...", + "http2_settings": "{...}", "dns_refresh_rate_ms": "...", "outlier_detection": "..." } @@ -136,6 +137,13 @@ http_codec_options an HTTP/2 mesh, if it's desired to disable HTTP/2 header compression the *no_compression* option should be specified both here as well as in the HTTP connection manager. +.. _config_cluster_manager_cluster_http2_settings: + +http2_settings + *(optional, object)* Additional http/2 settings that are passed directly to the http/2 codec when + initiating HTTP connection pool connections. These are the same options supported in the HTTP connection + manager :ref:`http2_settings ` option. + .. _config_cluster_manager_cluster_dns_refresh_rate_ms: dns_refresh_rate_ms diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 7f71192df1dbb..6dd70f5a9a895 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -20,6 +20,7 @@ HTTP connection manager "add_user_agent": "...", "tracing": "{...}", "http_codec_options": "...", + "http2_settings": "{...}", "server_name": "...", "idle_timeout_s": "...", "drain_timeout_ms": "...", @@ -97,6 +98,24 @@ http_codec_options ` option. See the comment there about disabling HTTP/2 header compression. +.. _config_http_conn_man_http2_settings: + +http2_settings + *(optional, object)* Additional http/2 codec options that are passed directly to the http/2 codec. + Currently supported settings are: + + max_concurrent_streams + *(optional, integer)* Maximum concurrent streams allowed on http2 connection. Valid values + range from 1 to 536870912 (1 << 29). + + initial_window_size + *(optional, integer)* Initial window size on http2 connection. Valid values range from 65536 + (default windows size plus 1) to 2147483647 (1 << 31 - 1), so we only support increase default + initial window size now. + + These are the same options available in the upstream cluster :ref:`http2_settings + ` option. + .. _config_http_conn_man_server_name: server_name From d02e90d79a0ea43d5b3273a1ddccf1a58e006f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 19:45:45 +0800 Subject: [PATCH 11/58] doc defaults for max_concurrent_streams/initial_window_size --- docs/configuration/http_conn_man/http_conn_man.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 6dd70f5a9a895..ff67e94895b24 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -106,12 +106,12 @@ http2_settings max_concurrent_streams *(optional, integer)* Maximum concurrent streams allowed on http2 connection. Valid values - range from 1 to 536870912 (1 << 29). + range from 1 to 536870912 (1 << 29). Default is 1024. initial_window_size *(optional, integer)* Initial window size on http2 connection. Valid values range from 65536 (default windows size plus 1) to 2147483647 (1 << 31 - 1), so we only support increase default - initial window size now. + initial window size now. Default is 268435456 (256 * 1024 * 1024). These are the same options available in the upstream cluster :ref:`http2_settings ` option. From 1fe200982b9f87edf76351e8ae74b05421c81f53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Mon, 22 May 2017 20:17:55 +0800 Subject: [PATCH 12/58] docs updates --- docs/configuration/cluster_manager/cluster.rst | 2 +- docs/configuration/http_conn_man/http_conn_man.rst | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index f3574f3a4997e..b903638358e55 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -140,7 +140,7 @@ http_codec_options .. _config_cluster_manager_cluster_http2_settings: http2_settings - *(optional, object)* Additional http/2 settings that are passed directly to the http/2 codec when + *(optional, object)* Additional HTTP/2 settings that are passed directly to the HTTP/2 codec when initiating HTTP connection pool connections. These are the same options supported in the HTTP connection manager :ref:`http2_settings ` option. diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index ff67e94895b24..ad7dc2aab038f 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -101,21 +101,24 @@ http_codec_options .. _config_http_conn_man_http2_settings: http2_settings - *(optional, object)* Additional http/2 codec options that are passed directly to the http/2 codec. + *(optional, object)* Additional HTTP/2 codec options that are passed directly to the HTTP/2 codec. Currently supported settings are: max_concurrent_streams - *(optional, integer)* Maximum concurrent streams allowed on http2 connection. Valid values - range from 1 to 536870912 (1 << 29). Default is 1024. + *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values + range from 1 to 536870912 (2^29). Default is 1024. initial_window_size - *(optional, integer)* Initial window size on http2 connection. Valid values range from 65536 - (default windows size plus 1) to 2147483647 (1 << 31 - 1), so we only support increase default + *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65536 + (default window size plus 1) to 2147483647 (2^31 - 1), so we only support increase default initial window size now. Default is 268435456 (256 * 1024 * 1024). These are the same options available in the upstream cluster :ref:`http2_settings ` option. + .. _Maximum concurrent streams: http://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2 + .. _Initial flow-control window: http://httpwg.org/specs/rfc7540.html#InitialWindowSize + .. _config_http_conn_man_server_name: server_name From 7709cd8a0537bbbccfe19092d6d213850c2d63bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 10:40:03 +0800 Subject: [PATCH 13/58] only update initial window size when needed --- docs/configuration/http_conn_man/http_conn_man.rst | 6 +++--- source/common/http/http2/codec_impl.cc | 12 +++++++----- source/common/json/config_schemas.cc | 4 ++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index ad7dc2aab038f..c40c37790ff00 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -109,9 +109,9 @@ http2_settings range from 1 to 536870912 (2^29). Default is 1024. initial_window_size - *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65536 - (default window size plus 1) to 2147483647 (2^31 - 1), so we only support increase default - initial window size now. Default is 268435456 (256 * 1024 * 1024). + *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65535 + (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1), so we only support increase + default initial window size now. Default is 268435456 (256 * 1024 * 1024). These are the same options available in the upstream cluster :ref:`http2_settings ` option. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 27924591ed17d..e25625f771cd7 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -522,11 +522,13 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { UNREFERENCED_PARAMETER(rc); // Increase connection window size up to our default size. - rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, - http2_settings.initial_window_size_ - - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); - // TODO: ensure mimium initial_window_size from json schema - ASSERT(rc == 0); + ASSERT(http2_settings.initial_window_size_ >= NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); + if (http2_settings.initial_window_size_ > NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { + rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, + http2_settings.initial_window_size_ - + NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); + ASSERT(rc == 0); + } } ConnectionImpl::Http2Callbacks::Http2Callbacks() { diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index b9964fe52ab2c..a57adae53dfa8 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -272,7 +272,7 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( }, "initial_window_size" : { "type": "integer", - "minimum": 65536, + "minimum": 65535, "maximum" : 2147483647 } } @@ -1233,7 +1233,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( }, "initial_window_size" : { "type": "integer", - "minimum": 65536, + "minimum": 65535, "maximum" : 2147483647 } } From b072dfbe6395734808ffe0542befde300158f5ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 11:06:13 +0800 Subject: [PATCH 14/58] docs update from review --- docs/configuration/http_conn_man/http_conn_man.rst | 6 +++--- source/common/http/utility.h | 4 ++-- test/common/http/utility_test.cc | 3 ++- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index c40c37790ff00..4ebf5a59e31f8 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -106,12 +106,12 @@ http2_settings max_concurrent_streams *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values - range from 1 to 536870912 (2^29). Default is 1024. + range from 1 to 536870912 (2^29) and defaults to 1024. initial_window_size *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65535 - (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1), so we only support increase - default initial window size now. Default is 268435456 (256 * 1024 * 1024). + (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1), so only increases to the + default initial window size are supported. Default is 268435456 (256 * 1024 * 1024). These are the same options available in the upstream cluster :ref:`http2_settings ` option. diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 181f92854c28e..e7d16d8aaaae9 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -64,8 +64,8 @@ class Utility { static bool isInternalRequest(const HeaderMap& headers); /** - * @return Http2Settings parse "http_codec_options" and "http2_settings" JSON fields and return - * a Http2Settings. + * @return Http2Settings An Http2Settings populated from the "http_codec_options" and + * "http2_settings" JSON fields. */ static Http2Settings parseHttp2Settings(const Json::Object& config); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 8c3bf9033d4e6..a7be6326164ca 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -118,7 +118,8 @@ TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectSharedPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}"); - EXPECT_THROW(Utility::parseHttp2Settings(*json), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(Utility::parseHttp2Settings(*json), EnvoyException, + "unknown http codec option 'foo'"); } } From 830a323a951056ea039090559d7c04c92ae7079b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 12:46:06 +0800 Subject: [PATCH 15/58] don't do flags comparison for Http2Settings::CodecOptions 1. remove Http2Settings::CodecOptions entirely; 2. change DISABLE_DYNAMIC_HPACK_TABLE to Http2Settings.hpack_table_size_; 3. keep http_codec_options.no_compression config for compatibility, but allow overrides with http2_settings.hpack_table_size. --- include/envoy/http/codec.h | 10 ++-------- source/common/http/http2/codec_impl.cc | 7 +++---- source/common/http/utility.cc | 5 ++++- source/common/json/config_schemas.cc | 10 ++++++++++ test/common/http/http2/codec_impl_test.cc | 6 +++--- test/common/http/utility_test.cc | 19 +++++++++++++++---- test/common/upstream/upstream_impl_test.cc | 10 +++++++--- 7 files changed, 44 insertions(+), 23 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 7b198d5830d46..f7e06d1052bf9 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -151,17 +151,11 @@ class ConnectionCallbacks { * http/2 codec settings */ struct Http2Settings { - /** - * A list of options that can be specified when creating a codec. - */ - struct CodecOptions { - static const uint64_t DISABLE_DYNAMIC_HPACK_TABLE = 0x1; - }; - - uint64_t codec_options_{0}; + uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE}; uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; + static const uint32_t DEFAULT_HPACK_TABLE_SIZE = 4096; // from nghttp2 static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if // needed. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e25625f771cd7..7c3e69d50956a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -511,10 +511,9 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; - if (http2_settings.codec_options_ & - Http::Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE) { - iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); - conn_log_debug("setting HPACK dynamic table size to 0", connection_); + if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { + iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_settings.hpack_table_size_}); + conn_log_debug("setting HPACK table size to {}", connection_, http2_settings.hpack_table_size_); } int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index eb9dde09f1557..71dd64b8774ea 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -142,13 +142,16 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { std::string options = config.getString("http_codec_options", ""); for (const std::string& option : StringUtil::split(options, ',')) { if (option == "no_compression") { - ret.codec_options_ |= Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE; + ret.hpack_table_size_ = 0; } else { throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); } } Json::ObjectSharedPtr http2_settings = config.getObject("http2_settings", true); + ret.hpack_table_size_ = http2_settings->getInteger( + "hpack_table_size", + ret.hpack_table_size_ ? Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE : 0); ret.max_concurrent_streams_ = http2_settings->getInteger( "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); ret.initial_window_size_ = http2_settings->getInteger( diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index a57adae53dfa8..1b4d1277ca2b9 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -265,6 +265,11 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "http2_settings" : { "type" : "object", "properties" : { + "hpack_table_size" : { + "type": "integer", + "minimum": 0, + "maximum" : 16777216 + }, "max_concurrent_streams" : { "type": "integer", "minimum": 1, @@ -1226,6 +1231,11 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "http2_settings" : { "type" : "object", "properties" : { + "hpack_table_size" : { + "type": "integer", + "minimum": 0, + "maximum" : 16777216 + }, "max_concurrent_streams" : { "type": "integer", "minimum": 1, diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index d998e87431028..c5ea127a76f2e 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,9 +26,9 @@ namespace Http { namespace Http2 { struct Http2SettingsTestParam : public Http2Settings { - Http2SettingsTestParam(uint64_t codec_options, uint32_t max_concurrent_streams, + Http2SettingsTestParam(uint64_t hpack_table_size, uint32_t max_concurrent_streams, uint32_t initial_window_size) { - codec_options_ = codec_options; + hpack_table_size_ = hpack_table_size; max_concurrent_streams_ = max_concurrent_streams; initial_window_size_ = initial_window_size; } @@ -249,7 +249,7 @@ INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, testing::Values(Http2SettingsTestParam(0, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE, + Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE))); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index a7be6326164ca..877f3c0df4e6d 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Http { // Satisfy linker -const uint64_t Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE; +const uint32_t Http2Settings::DEFAULT_HPACK_TABLE_SIZE; const uint32_t Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS; const uint32_t Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE; @@ -95,7 +95,8 @@ TEST(HttpUtility, createSslRedirectPath) { TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectSharedPtr json = Json::Factory::loadFromString("{}"); - EXPECT_EQ(0UL, Utility::parseHttp2Settings(*json).codec_options_); + EXPECT_EQ(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Utility::parseHttp2Settings(*json).hpack_table_size_); EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Utility::parseHttp2Settings(*json).max_concurrent_streams_); EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, @@ -110,12 +111,22 @@ TEST(HttpUtility, parseHttp2Settings) { "initial_window_size": 5678 } })raw"); - EXPECT_EQ(Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE, - Utility::parseHttp2Settings(*json).codec_options_); + EXPECT_EQ(0U, Utility::parseHttp2Settings(*json).hpack_table_size_); EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).max_concurrent_streams_); EXPECT_EQ(5678U, Utility::parseHttp2Settings(*json).initial_window_size_); } + { + // http2_settings.hpack_table_size overrides http_codec_options.no_compression + Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression", + "http2_settings" : { + "hpack_table_size": 128 + } + })raw"); + EXPECT_EQ(128U, Utility::parseHttp2Settings(*json).hpack_table_size_); + } + { Json::ObjectSharedPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}"); EXPECT_THROW_WITH_MESSAGE(Utility::parseHttp2Settings(*json), EnvoyException, diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 12ff3d2975ed8..562fcae6e3ece 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -24,11 +24,15 @@ #include "gtest/gtest.h" namespace Envoy { + using testing::_; using testing::ContainerEq; using testing::Invoke; using testing::NiceMock; +// Satisfy linker +const uint32_t Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE; + namespace Upstream { static std::list hostListToAddresses(const std::vector& hosts) { @@ -146,8 +150,7 @@ TEST(StrictDnsClusterImplTest, Basic) { EXPECT_EQ(3U, cluster.info()->resourceManager(ResourcePriority::High).requests().max()); EXPECT_EQ(4U, cluster.info()->resourceManager(ResourcePriority::High).retries().max()); EXPECT_EQ(3U, cluster.info()->maxRequestsPerConnection()); - EXPECT_EQ(static_cast(Http::Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE), - cluster.info()->http2Settings().codec_options_); + EXPECT_EQ(0U, cluster.info()->http2Settings().hpack_table_size_); cluster.info()->stats().upstream_rq_total_.inc(); EXPECT_EQ(1UL, stats.counter("cluster.name.upstream_rq_total").value()); @@ -429,7 +432,8 @@ TEST(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(0U, cluster.info()->http2Settings().codec_options_); + EXPECT_EQ(Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + cluster.info()->http2Settings().hpack_table_size_); EXPECT_EQ(LoadBalancerType::Random, cluster.info()->lbType()); EXPECT_THAT(std::list({"10.0.0.1:11001", "10.0.0.2:11002"}), ContainerEq(hostListToAddresses(cluster.hosts()))); From f9d117a17a156df1ea14896fdcc4755480de9b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 15:05:28 +0800 Subject: [PATCH 16/58] expand Http2Settings tests for min/max of all fields --- include/envoy/http/codec.h | 13 ++++++++++--- test/common/http/http2/codec_impl_test.cc | 19 ++++++++++++++++--- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index f7e06d1052bf9..5a3e485b88308 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -155,11 +155,18 @@ struct Http2Settings { uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; - static const uint32_t DEFAULT_HPACK_TABLE_SIZE = 4096; // from nghttp2 + static const uint32_t MIN_HPACK_TABLE_SIZE = 0; + static const uint32_t DEFAULT_HPACK_TABLE_SIZE = 4 * 1024; // from nghttp2 + static const uint32_t MAX_HPACK_TABLE_SIZE = 16 * 1024 * 1024; + + static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; - // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if - // needed. + static const uint32_t MAX_MAX_CONCURRENT_STREAMS = 1 << 29; // from MAX_STREAMS + + static const uint32_t MIN_INITIAL_WINDOW_SIZE = (1 << 16) - 1; // from HTTP/2 spec static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024; + // ( 1 << 31 ) - 1 cause overflow warning, so hard-code the value + static const uint32_t MAX_INITIAL_WINDOW_SIZE = 2147483647; // from HTTP/2 spec }; /** diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c5ea127a76f2e..e21ac005c1c39 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,7 +26,7 @@ namespace Http { namespace Http2 { struct Http2SettingsTestParam : public Http2Settings { - Http2SettingsTestParam(uint64_t hpack_table_size, uint32_t max_concurrent_streams, + Http2SettingsTestParam(uint32_t hpack_table_size, uint32_t max_concurrent_streams, uint32_t initial_window_size) { hpack_table_size_ = hpack_table_size; max_concurrent_streams_ = max_concurrent_streams; @@ -247,11 +247,24 @@ TEST_P(Http2CodecImplTest, DeferredReset) { INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, - testing::Values(Http2SettingsTestParam(0, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + testing::Values(Http2SettingsTestParam(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), + Http2SettingsTestParam(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), + Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::MIN_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), + Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::MAX_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), + Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::MIN_INITIAL_WINDOW_SIZE), Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE))); + Http2Settings::MAX_INITIAL_WINDOW_SIZE))); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { From 342a0493462b20fe2c42acacf8870288c4f410f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 17:09:36 +0800 Subject: [PATCH 17/58] move Http2Settings static definitions to //test/test_common/utility_lib --- test/common/http/utility_test.cc | 5 ----- test/common/upstream/upstream_impl_test.cc | 3 --- test/test_common/BUILD | 1 + test/test_common/utility.cc | 7 +++++++ 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 877f3c0df4e6d..87e13fde75cf3 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -15,11 +15,6 @@ namespace Envoy { 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_WINDOW_SIZE; - TEST(HttpUtility, parseQueryString) { EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello")); EXPECT_EQ(Utility::QueryParams(), Utility::parseQueryString("/hello?")); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 562fcae6e3ece..9643011e6ff3a 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -30,9 +30,6 @@ using testing::ContainerEq; using testing::Invoke; using testing::NiceMock; -// Satisfy linker -const uint32_t Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE; - namespace Upstream { static std::list hostListToAddresses(const std::vector& hosts) { diff --git a/test/test_common/BUILD b/test/test_common/BUILD index c28731ba1ba23..04c093dbdb714 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -59,6 +59,7 @@ envoy_cc_test_library( hdrs = ["utility.h"], deps = [ "//include/envoy/buffer:buffer_interface", + "//include/envoy/http:codec_interface", "//include/envoy/network:address_interface", "//source/common/common:empty_string", "//source/common/http:header_map_lib", diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index f8d2b4934f909..8bcf08f449d55 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -11,6 +11,7 @@ #include #include "envoy/buffer/buffer.h" +#include "envoy/http/codec.h" #include "common/common/empty_string.h" #include "common/network/address_impl.h" @@ -21,6 +22,7 @@ #include "spdlog/spdlog.h" namespace Envoy { + bool TestUtility::buffersEqual(const Buffer::Instance& lhs, const Buffer::Instance& rhs) { if (lhs.length() != rhs.length()) { return false; @@ -118,6 +120,11 @@ ScopedFdCloser::~ScopedFdCloser() { ::close(fd_); } 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_WINDOW_SIZE; + TestHeaderMapImpl::TestHeaderMapImpl() : HeaderMapImpl() {} TestHeaderMapImpl::TestHeaderMapImpl( From 8065aeaf42680b6ef2acf61bac2fb106b726da34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 17:18:56 +0800 Subject: [PATCH 18/58] fix test cases --- 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 e21ac005c1c39..21026a7b159c7 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -250,7 +250,7 @@ INSTANTIATE_TEST_CASE_P( testing::Values(Http2SettingsTestParam(Http2Settings::MIN_HPACK_TABLE_SIZE, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2SettingsTestParam(Http2Settings::MAX_HPACK_TABLE_SIZE, Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, From 1458f54df01429f425fdc48da630dbddc0b5a8d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 17:26:36 +0800 Subject: [PATCH 19/58] docs update --- docs/configuration/http_conn_man/http_conn_man.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 4ebf5a59e31f8..be1b7292bc5f6 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -110,8 +110,8 @@ http2_settings initial_window_size *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65535 - (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1), so only increases to the - default initial window size are supported. Default is 268435456 (256 * 1024 * 1024). + (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1, also HTTP/2 maximum), so only + increases to the default initial window size are supported. Default is 268435456 (256 * 1024 * 1024). These are the same options available in the upstream cluster :ref:`http2_settings ` option. From eea3df1184c2c847e804a58edcf69d75da4e7b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 17:43:22 +0800 Subject: [PATCH 20/58] test http2_settings.hpack_table_size parses --- test/common/http/utility_test.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 87e13fde75cf3..d26f359b4b3b8 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -100,17 +100,24 @@ TEST(HttpUtility, parseHttp2Settings) { { Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ - "http_codec_options": "no_compression", "http2_settings" : { + "hpack_table_size": 1234, "max_concurrent_streams": 1234, "initial_window_size": 5678 } })raw"); - EXPECT_EQ(0U, Utility::parseHttp2Settings(*json).hpack_table_size_); + EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).hpack_table_size_); EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).max_concurrent_streams_); EXPECT_EQ(5678U, Utility::parseHttp2Settings(*json).initial_window_size_); } + { + Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression", + })raw"); + EXPECT_EQ(0, Utility::parseHttp2Settings(*json).hpack_table_size_); + } + { // http2_settings.hpack_table_size overrides http_codec_options.no_compression Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ From 3bd6cd242cfb09089d92c44af6f00a0cad58bd47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 19:54:23 +0800 Subject: [PATCH 21/58] http2settings test: parse json once and more checks --- test/common/http/utility_test.cc | 40 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index d26f359b4b3b8..97aa651621756 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -89,44 +89,48 @@ TEST(HttpUtility, createSslRedirectPath) { TEST(HttpUtility, parseHttp2Settings) { { - Json::ObjectSharedPtr json = Json::Factory::loadFromString("{}"); - EXPECT_EQ(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Utility::parseHttp2Settings(*json).hpack_table_size_); + auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString("{}")); + EXPECT_EQ(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, http2_settings.hpack_table_size_); EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Utility::parseHttp2Settings(*json).max_concurrent_streams_); - EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Utility::parseHttp2Settings(*json).initial_window_size_); + http2_settings.max_concurrent_streams_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); } { - Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ + auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString(R"raw({ "http2_settings" : { "hpack_table_size": 1234, "max_concurrent_streams": 1234, "initial_window_size": 5678 } - })raw"); - EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).hpack_table_size_); - EXPECT_EQ(1234U, Utility::parseHttp2Settings(*json).max_concurrent_streams_); - EXPECT_EQ(5678U, Utility::parseHttp2Settings(*json).initial_window_size_); + })raw")); + EXPECT_EQ(1234U, http2_settings.hpack_table_size_); + EXPECT_EQ(1234U, http2_settings.max_concurrent_streams_); + EXPECT_EQ(5678U, http2_settings.initial_window_size_); } { - Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ - "http_codec_options": "no_compression", - })raw"); - EXPECT_EQ(0, Utility::parseHttp2Settings(*json).hpack_table_size_); + auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression" + })raw")); + EXPECT_EQ(0, http2_settings.hpack_table_size_); + EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + http2_settings.max_concurrent_streams_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); } { // http2_settings.hpack_table_size overrides http_codec_options.no_compression - Json::ObjectSharedPtr json = Json::Factory::loadFromString(R"raw({ + auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString(R"raw({ "http_codec_options": "no_compression", "http2_settings" : { "hpack_table_size": 128 } - })raw"); - EXPECT_EQ(128U, Utility::parseHttp2Settings(*json).hpack_table_size_); + })raw")); + EXPECT_EQ(128U, http2_settings.hpack_table_size_); + EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + http2_settings.max_concurrent_streams_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); } { From 36a917042fb60c2ca2f77f4cda638f4e19fffd1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 21:06:06 +0800 Subject: [PATCH 22/58] add asserts for Http2Settings fields --- source/common/http/http2/codec_impl.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 7c3e69d50956a..f2c01966753a1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -507,6 +507,13 @@ void ConnectionImpl::sendPendingFrames() { } void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { + ASSERT(Http2Settings::MIN_HPACK_TABLE_SIZE <= http2_settings.hpack_table_size_ && + 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_WINDOW_SIZE <= http2_settings.initial_window_size_ && + http2_settings.initial_window_size_ <= Http2Settings::MAX_INITIAL_WINDOW_SIZE); + std::vector iv = { {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; From 36466c07b806af221f915787a1326b72eb0fd29e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 21:06:06 +0800 Subject: [PATCH 23/58] add asserts for Http2Settings fields --- source/common/http/http2/codec_impl.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 7c3e69d50956a..21a9e7c0e3bca 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -507,6 +507,12 @@ void ConnectionImpl::sendPendingFrames() { } void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { + 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_WINDOW_SIZE <= http2_settings.initial_window_size_ && + http2_settings.initial_window_size_ <= Http2Settings::MAX_INITIAL_WINDOW_SIZE); + std::vector iv = { {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; From 35bd52fec7d4ee6f4b1419c95764de39e26b2bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Tue, 23 May 2017 21:31:25 +0800 Subject: [PATCH 24/58] fix asserts --- source/common/http/http2/codec_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 6746aa13103c5..21a9e7c0e3bca 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -508,8 +508,6 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { ASSERT(http2_settings.hpack_table_size_ <= Http2Settings::MAX_HPACK_TABLE_SIZE); - ASSERT(Http2Settings::MIN_HPACK_TABLE_SIZE <= http2_settings.hpack_table_size_ && - 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_WINDOW_SIZE <= http2_settings.initial_window_size_ && From c55f88533b2570e45758c2b3442615d613c85383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Wed, 24 May 2017 14:21:26 +0800 Subject: [PATCH 25/58] fix hard-coded const --- include/envoy/http/codec.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 5a3e485b88308..a09b801f5536f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -165,8 +165,7 @@ struct Http2Settings { static const uint32_t MIN_INITIAL_WINDOW_SIZE = (1 << 16) - 1; // from HTTP/2 spec static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024; - // ( 1 << 31 ) - 1 cause overflow warning, so hard-code the value - static const uint32_t MAX_INITIAL_WINDOW_SIZE = 2147483647; // from HTTP/2 spec + static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; // from HTTP/2 spec }; /** From 2e5df72da3a56a1683e0b69c720405832fefb4e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 10:39:18 +0800 Subject: [PATCH 26/58] docs for hpack_table_size config --- docs/configuration/http_conn_man/http_conn_man.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index be1b7292bc5f6..5d8a503cd7dd8 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -104,6 +104,11 @@ http2_settings *(optional, object)* Additional HTTP/2 codec options that are passed directly to the HTTP/2 codec. Currently supported settings are: + hpack_table_size + *(optional, integer)* `Maximum Table Size`_ (in octets) that the encoder is permitted to use for + the dynamic HPACK table. Valid values range from 0 to 16777216 (2^24) and defaults to 4096, + with 0 effectively disables header compression. + max_concurrent_streams *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values range from 1 to 536870912 (2^29) and defaults to 1024. @@ -116,6 +121,7 @@ http2_settings These are the same options available in the upstream cluster :ref:`http2_settings ` option. + .. _Maximum Table Size: http://httpwg.org/specs/rfc7541.html#maximum.table.size .. _Maximum concurrent streams: http://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2 .. _Initial flow-control window: http://httpwg.org/specs/rfc7540.html#InitialWindowSize From 01896d3bc836dffcb9cdc6ab1ff5b0040ba549f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 12:03:55 +0800 Subject: [PATCH 27/58] adjust parse logic for http2_settings/http_codec_options 1. log DEPRECATED for http_codec_options, NOT DONE YET, NEED LOGGER; 2. throw exception when hpack_table_size conflicts with no_compression; 3. track http_codec_options in #1012, since can't find DEPRECATED.md yet --- source/common/http/utility.cc | 26 +++++++++++++++++--------- test/common/http/utility_test.cc | 26 ++++++++++++-------------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 71dd64b8774ea..0fc567a53418c 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -139,24 +139,32 @@ bool Utility::isInternalRequest(const HeaderMap& headers) { Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { Http2Settings ret; + Json::ObjectSharedPtr http2_settings = config.getObject("http2_settings", true); + ret.hpack_table_size_ = + http2_settings->getInteger("hpack_table_size", Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE); + ret.max_concurrent_streams_ = http2_settings->getInteger( + "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); + ret.initial_window_size_ = http2_settings->getInteger( + "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); + std::string options = config.getString("http_codec_options", ""); + if (options != "") { + // TODO: figure out how to get a loggger here + // log().warn("'http_codec_options' is DEPRECATED, please use 'http2_settings' instead"); + } + for (const std::string& option : StringUtil::split(options, ',')) { if (option == "no_compression") { + if (http2_settings->hasObject("hpack_table_size") && ret.hpack_table_size_ != 0) { + throw EnvoyException( + "'http_codec_options.no_compression' conflicts with 'http2_settings.hpack_table_size'"); + } ret.hpack_table_size_ = 0; } else { throw EnvoyException(fmt::format("unknown http codec option '{}'", option)); } } - Json::ObjectSharedPtr http2_settings = config.getObject("http2_settings", true); - ret.hpack_table_size_ = http2_settings->getInteger( - "hpack_table_size", - ret.hpack_table_size_ ? Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE : 0); - ret.max_concurrent_streams_ = http2_settings->getInteger( - "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); - ret.initial_window_size_ = http2_settings->getInteger( - "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); - return ret; } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 97aa651621756..e7b9ce2ffe7cb 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -120,23 +120,21 @@ TEST(HttpUtility, parseHttp2Settings) { } { - // http2_settings.hpack_table_size overrides http_codec_options.no_compression - auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString(R"raw({ - "http_codec_options": "no_compression", - "http2_settings" : { - "hpack_table_size": 128 - } - })raw")); - EXPECT_EQ(128U, http2_settings.hpack_table_size_); - EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - http2_settings.max_concurrent_streams_); - EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); + auto json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}"); + EXPECT_THROW_WITH_MESSAGE(Utility::parseHttp2Settings(*json), EnvoyException, + "unknown http codec option 'foo'"); } { - Json::ObjectSharedPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}"); - EXPECT_THROW_WITH_MESSAGE(Utility::parseHttp2Settings(*json), EnvoyException, - "unknown http codec option 'foo'"); + auto json = Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression", + "http2_settings" : { + "hpack_table_size": 1 + } + })raw"); + EXPECT_THROW_WITH_MESSAGE( + Utility::parseHttp2Settings(*json), EnvoyException, + "'http_codec_options.no_compression' conflicts with 'http2_settings.hpack_table_size'"); } } From 6aa47f763fffdf150fc51da7ea9615a93f64dc92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 12:21:27 +0800 Subject: [PATCH 28/58] globally change http/2 to HTTP/2 for all comments --- include/envoy/http/codec.h | 2 +- include/envoy/upstream/upstream.h | 2 +- source/common/http/http2/codec_impl.cc | 2 +- source/common/http/http2/codec_impl.h | 4 ++-- source/common/router/router.cc | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index a09b801f5536f..0021ea5079f4f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -148,7 +148,7 @@ class ConnectionCallbacks { }; /** - * http/2 codec settings + * HTTP/2 codec settings */ struct Http2Settings { uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE}; diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 006ef0a15a27e..1489e21386b1f 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -244,7 +244,7 @@ class ClusterInfo { virtual uint64_t features() const PURE; /** - * @return const Http::Http2Settings& for http/2 connections created on behalf of this cluster. + * @return const Http::Http2Settings& for HTTP/2 connections created on behalf of this cluster. * @see Http::Http2Settings. */ virtual const Http::Http2Settings& http2Settings() const PURE; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 21a9e7c0e3bca..d4b7e21fbc7e2 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -612,7 +612,7 @@ ConnectionImpl::Http2Options::Http2Options() { // 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. + // of kept alive HTTP/2 connections. nghttp2_option_set_no_closed_streams(options_, 1); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 29020b048a2c4..4e3041f779f3a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -32,7 +32,7 @@ const std::string ALPN_STRING = "h2"; const std::string CLIENT_MAGIC_PREFIX = "PRI * HTTP/2"; /** - * All stats for the http/2 codec. @see stats_macros.h + * All stats for the HTTP/2 codec. @see stats_macros.h */ // clang-format off #define ALL_HTTP2_CODEC_STATS(COUNTER) \ @@ -44,7 +44,7 @@ const std::string CLIENT_MAGIC_PREFIX = "PRI * HTTP/2"; // clang-format on /** - * Wrapper struct for the http/2 codec stats. @see stats_macros.h + * Wrapper struct for the HTTP/2 codec stats. @see stats_macros.h */ struct CodecStats { ALL_HTTP2_CODEC_STATS(GENERATE_COUNTER_STRUCT) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index cd294e0b17532..90938261fb5c2 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -722,7 +722,7 @@ void Filter::UpstreamRequest::onPoolReady(Http::StreamEncoder& request_encoder, calling_encode_headers_ = false; // It is possible to get reset in the middle of an encodeHeaders() call. This happens for example - // in the http/2 codec if the frame cannot be encoded for some reason. This should never happen + // in the HTTP/2 codec if the frame cannot be encoded for some reason. This should never happen // but it's unclear if we have covered all cases so protect against it and test for it. One // specific example of a case where this happens is if we try to encode a total header size that // is too big in HTTP/2 (64K currently). From d33b7530ddaba6ce8720facc1f6a6f4014ac7af1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 13:26:21 +0800 Subject: [PATCH 29/58] doc http_codec_options as DEPRECATED --- docs/configuration/http_conn_man/http_conn_man.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 5d8a503cd7dd8..2b3ca7701fc09 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -87,6 +87,8 @@ add_user_agent .. _config_http_conn_man_http_codec_options: http_codec_options + **DEPRECATED**, use :ref:`http2_settings ` instead. + *(optional, string)* Additional options that are passed directly to the codec. Not all options are applicable to all codecs. Possible values are: From 0d2348bc60ba25af77b4988d1b297fd62ba8abcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 13:57:47 +0800 Subject: [PATCH 30/58] doc rationals for http2 setting min/max/default --- include/envoy/http/codec.h | 16 ++++++++++++---- source/common/http/utility.cc | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 0021ea5079f4f..48b7fb274863f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -155,17 +155,25 @@ struct Http2Settings { uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; + // disable HPACK compression static const uint32_t MIN_HPACK_TABLE_SIZE = 0; - static const uint32_t DEFAULT_HPACK_TABLE_SIZE = 4 * 1024; // from nghttp2 + // initial value from HTTP/2 spec, same as NGHTTP2_DEFAULT_HEADER_TABLE_SIZE from nghttp2 + static const uint32_t DEFAULT_HPACK_TABLE_SIZE = 4 * 1024; + // a 16MiB maximum, hope it's more than enough static const uint32_t MAX_HPACK_TABLE_SIZE = 16 * 1024 * 1024; static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; - static const uint32_t MAX_MAX_CONCURRENT_STREAMS = 1 << 29; // from MAX_STREAMS + // All streams are 2^31. Client/Server streams are half that. Just to be on the safe + // side we do 2^29. Same as MAX_STREAMS in source/common/http/http2/conn_pool.h. + static const uint32_t MAX_MAX_CONCURRENT_STREAMS = 1 << 29; - static const uint32_t MIN_INITIAL_WINDOW_SIZE = (1 << 16) - 1; // from HTTP/2 spec + // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE. + // We only support increasing window size now, so this is also the minimum. + static const uint32_t MIN_INITIAL_WINDOW_SIZE = (1 << 16) - 1; static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024; - static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; // from HTTP/2 spec + // maximum from HTTP/2 spec + static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; }; /** diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 0fc567a53418c..f23e6cfd0019a 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -147,6 +147,7 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { ret.initial_window_size_ = http2_settings->getInteger( "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); + // http_codec_options config is DEPRECATED std::string options = config.getString("http_codec_options", ""); if (options != "") { // TODO: figure out how to get a loggger here From c97cae1aeaf5858a67822a47fa8f2a259db70617 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 14:22:33 +0800 Subject: [PATCH 31/58] update docs to explain min/max http2 settings --- docs/configuration/http_conn_man/http_conn_man.rst | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 2b3ca7701fc09..be572c915f155 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -111,14 +111,21 @@ http2_settings the dynamic HPACK table. Valid values range from 0 to 16777216 (2^24) and defaults to 4096, with 0 effectively disables header compression. + NOTE: A 16MiB (2^24) maximum table size seems to be more than enough. + max_concurrent_streams *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values range from 1 to 536870912 (2^29) and defaults to 1024. + NOTE: Total HTTP/2 streams is 2^31, one side (client/server) is 2^30. To be safe, we use 2^29. + initial_window_size *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65535 - (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1, also HTTP/2 maximum), so only - increases to the default initial window size are supported. Default is 268435456 (256 * 1024 * 1024). + (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1, also HTTP/2 maximum) + and defaults to 268435456 (256 * 1024 * 1024). + + NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window + size now, so it's also the minimum. These are the same options available in the upstream cluster :ref:`http2_settings ` option. From 64609e6dfe6ea433ef35ff7072e70b3e809c735c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 14:34:19 +0800 Subject: [PATCH 32/58] compare http2_settings.initial_window_size_ and NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE with not equal --- source/common/http/http2/codec_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index d4b7e21fbc7e2..dfd41bca20942 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -527,8 +527,7 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { UNREFERENCED_PARAMETER(rc); // Increase connection window size up to our default size. - ASSERT(http2_settings.initial_window_size_ >= NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); - if (http2_settings.initial_window_size_ > NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { + if (http2_settings.initial_window_size_ != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, http2_settings.initial_window_size_ - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); From f38e0f6fbccca60557f63b5fb4e1f0f5c894b45c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= Date: Thu, 25 May 2017 15:04:31 +0800 Subject: [PATCH 33/58] log by using Logger::Registry::getLog --- source/common/http/utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f23e6cfd0019a..7ed9e132e233b 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -150,8 +150,8 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { // http_codec_options config is DEPRECATED std::string options = config.getString("http_codec_options", ""); if (options != "") { - // TODO: figure out how to get a loggger here - // log().warn("'http_codec_options' is DEPRECATED, please use 'http2_settings' instead"); + spdlog::logger& logger = Logger::Registry::getLog(Logger::Id::config); + logger.warn("'http_codec_options' is DEPRECATED, please use 'http2_settings' instead"); } for (const std::string& option : StringUtil::split(options, ',')) { From 6a2f0a625dc10930fb03636ab594db0c1e5c2c37 Mon Sep 17 00:00:00 2001 From: jwfang Date: Fri, 26 May 2017 04:58:24 +0000 Subject: [PATCH 34/58] expand test use testing::Combine --- test/common/http/http2/codec_impl_test.cc | 44 ++++++++++++----------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 21026a7b159c7..2241be26fbf58 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -16,6 +16,7 @@ #include "gtest/gtest.h" namespace Envoy { + using testing::_; using testing::AtLeast; using testing::InSequence; @@ -26,6 +27,13 @@ namespace Http { namespace Http2 { struct Http2SettingsTestParam : public Http2Settings { + static Http2SettingsTestParam + fromTuple(const ::testing::tuple& tp) { + return Http2SettingsTestParam(::testing::get<0>(tp), ::testing::get<1>(tp), + ::testing::get<2>(tp)); + } + +private: Http2SettingsTestParam(uint32_t hpack_table_size, uint32_t max_concurrent_streams, uint32_t initial_window_size) { hpack_table_size_ = hpack_table_size; @@ -34,7 +42,8 @@ struct Http2SettingsTestParam : public Http2Settings { } }; -class Http2CodecImplTest : public testing::TestWithParam { +class Http2CodecImplTest + : public testing::TestWithParam<::testing::tuple> { public: struct ConnectionWrapper { void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) { @@ -53,8 +62,10 @@ class Http2CodecImplTest : public testing::TestWithParam }; Http2CodecImplTest() - : client_(client_connection_, client_callbacks_, stats_store_, GetParam()), - server_(server_connection_, server_callbacks_, stats_store_, GetParam()), + : client_(client_connection_, client_callbacks_, stats_store_, + Http2SettingsTestParam::fromTuple(GetParam())), + server_(server_connection_, server_callbacks_, stats_store_, + Http2SettingsTestParam::fromTuple(GetParam())), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); @@ -247,24 +258,15 @@ TEST_P(Http2CodecImplTest, DeferredReset) { INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, - testing::Values(Http2SettingsTestParam(Http2Settings::MIN_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::MAX_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::MIN_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::MAX_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::MIN_INITIAL_WINDOW_SIZE), - Http2SettingsTestParam(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::MAX_INITIAL_WINDOW_SIZE))); + ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::MAX_HPACK_TABLE_SIZE), + ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::MAX_MAX_CONCURRENT_STREAMS), + ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, + Http2Settings::MAX_INITIAL_WINDOW_SIZE))); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { From 01228c49f0ffbe66bb8a46b98155afe052a9eee0 Mon Sep 17 00:00:00 2001 From: jwfang Date: Fri, 26 May 2017 17:01:24 +0800 Subject: [PATCH 35/58] combine-combine to use different http2settings for client/server --- test/common/http/http2/codec_impl_test.cc | 43 +++++++++++++++-------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 2241be26fbf58..54f1baa1249bc 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,9 +26,11 @@ using testing::NiceMock; namespace Http { namespace Http2 { +typedef ::testing::tuple http2SettingsTuple; +typedef ::testing::tuple http2SettingsTupleTuple; + struct Http2SettingsTestParam : public Http2Settings { - static Http2SettingsTestParam - fromTuple(const ::testing::tuple& tp) { + static Http2SettingsTestParam fromTuple(const http2SettingsTuple& tp) { return Http2SettingsTestParam(::testing::get<0>(tp), ::testing::get<1>(tp), ::testing::get<2>(tp)); } @@ -42,8 +44,7 @@ struct Http2SettingsTestParam : public Http2Settings { } }; -class Http2CodecImplTest - : public testing::TestWithParam<::testing::tuple> { +class Http2CodecImplTest : public testing::TestWithParam { public: struct ConnectionWrapper { void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) { @@ -63,9 +64,9 @@ class Http2CodecImplTest Http2CodecImplTest() : client_(client_connection_, client_callbacks_, stats_store_, - Http2SettingsTestParam::fromTuple(GetParam())), + Http2SettingsTestParam::fromTuple(::testing::get<0>(GetParam()))), server_(server_connection_, server_callbacks_, stats_store_, - Http2SettingsTestParam::fromTuple(GetParam())), + Http2SettingsTestParam::fromTuple(::testing::get<1>(GetParam()))), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); @@ -258,15 +259,27 @@ TEST_P(Http2CodecImplTest, DeferredReset) { INSTANTIATE_TEST_CASE_P( Http2CodecImplTest, Http2CodecImplTest, - ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::MAX_HPACK_TABLE_SIZE), - ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::MAX_MAX_CONCURRENT_STREAMS), - ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_WINDOW_SIZE))); + ::testing::Combine( + ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::MAX_HPACK_TABLE_SIZE), + ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::MAX_MAX_CONCURRENT_STREAMS), + ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, + Http2Settings::MAX_INITIAL_WINDOW_SIZE)), + ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::MAX_HPACK_TABLE_SIZE), + ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::MAX_MAX_CONCURRENT_STREAMS), + ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, + Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, + Http2Settings::MAX_INITIAL_WINDOW_SIZE)) + + )); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { From fabcbedbb10bdc06aa3915fdb9818c940f8a893b Mon Sep 17 00:00:00 2001 From: jwfang Date: Fri, 26 May 2017 17:20:48 +0800 Subject: [PATCH 36/58] test param not needed now --- test/common/http/http2/codec_impl_test.cc | 31 ++++++++++------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 54f1baa1249bc..8bffefc8c15cd 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -29,20 +29,13 @@ namespace Http2 { typedef ::testing::tuple http2SettingsTuple; typedef ::testing::tuple http2SettingsTupleTuple; -struct Http2SettingsTestParam : public Http2Settings { - static Http2SettingsTestParam fromTuple(const http2SettingsTuple& tp) { - return Http2SettingsTestParam(::testing::get<0>(tp), ::testing::get<1>(tp), - ::testing::get<2>(tp)); - } - -private: - Http2SettingsTestParam(uint32_t hpack_table_size, uint32_t max_concurrent_streams, - uint32_t initial_window_size) { - hpack_table_size_ = hpack_table_size; - max_concurrent_streams_ = max_concurrent_streams; - initial_window_size_ = initial_window_size; - } -}; +Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { + Http2Settings ret; + ret.hpack_table_size_ = ::testing::get<0>(tp); + ret.max_concurrent_streams_ = ::testing::get<1>(tp); + ret.initial_window_size_ = ::testing::get<2>(tp); + return ret; +} class Http2CodecImplTest : public testing::TestWithParam { public: @@ -63,10 +56,10 @@ class Http2CodecImplTest : public testing::TestWithParam(GetParam()))), - server_(server_connection_, server_callbacks_, stats_store_, - Http2SettingsTestParam::fromTuple(::testing::get<1>(GetParam()))), + : client_http2settings_(http2SettingsFromTuple(::testing::get<0>(GetParam()))), + client_(client_connection_, client_callbacks_, stats_store_, client_http2settings_), + server_http2settings_(http2SettingsFromTuple(::testing::get<1>(GetParam()))), + server_(server_connection_, server_callbacks_, stats_store_, server_http2settings_), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); @@ -88,10 +81,12 @@ class Http2CodecImplTest : public testing::TestWithParam client_connection_; MockConnectionCallbacks client_callbacks_; ClientConnectionImpl client_; ConnectionWrapper client_wrapper_; + const Http2Settings server_http2settings_; NiceMock server_connection_; MockServerConnectionCallbacks server_callbacks_; ServerConnectionImpl server_; From c43c0b87b6412e488374f237f50f445b048960a2 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 17:47:45 +0800 Subject: [PATCH 37/58] relax min/max, defaults use http2 defaults --- .../http_conn_man/http_conn_man.rst | 18 +++++------ include/envoy/http/codec.h | 31 ++++++++++++------- source/common/http/http2/codec_impl.cc | 3 +- source/common/json/config_schemas.cc | 8 ++--- 4 files changed, 32 insertions(+), 28 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index be572c915f155..14dcbd0d18eca 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -108,21 +108,19 @@ http2_settings hpack_table_size *(optional, integer)* `Maximum Table Size`_ (in octets) that the encoder is permitted to use for - the dynamic HPACK table. Valid values range from 0 to 16777216 (2^24) and defaults to 4096, + the dynamic HPACK table. Valid values range from 0 to 4294967295 (2^32 - 1) and defaults to 4096, with 0 effectively disables header compression. - NOTE: A 16MiB (2^24) maximum table size seems to be more than enough. - max_concurrent_streams *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values - range from 1 to 536870912 (2^29) and defaults to 1024. + range from 0 to 2147483647 (2^31 - 1) and defaults to 1024. - NOTE: Total HTTP/2 streams is 2^31, one side (client/server) is 2^30. To be safe, we use 2^29. + NOTE: total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to + exclude stream 0. initial_window_size - *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65535 - (HTTP/2 default window size, also minimum) to 2147483647 (2^31 - 1, also HTTP/2 maximum) - and defaults to 268435456 (256 * 1024 * 1024). + *(optional, integer)* `Initial stream-level flow-control window`_ size. Valid values range from 65535 + (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 65535 (2^16 - 1). NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window size now, so it's also the minimum. @@ -130,9 +128,9 @@ http2_settings These are the same options available in the upstream cluster :ref:`http2_settings ` option. - .. _Maximum Table Size: http://httpwg.org/specs/rfc7541.html#maximum.table.size + .. _Maximum Table Size: http://httpwg.org/specs/rfc7541.html#rfc.section.4.2 .. _Maximum concurrent streams: http://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2 - .. _Initial flow-control window: http://httpwg.org/specs/rfc7540.html#InitialWindowSize + .. _Initial stream-level flow-control window: http://httpwg.org/specs/rfc7540.html#rfc.section.6.9.2 .. _config_http_conn_man_server_name: diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 48b7fb274863f..efe263f782dbb 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -151,6 +151,7 @@ class ConnectionCallbacks { * 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_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; @@ -158,21 +159,27 @@ struct Http2Settings { // 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 = 4 * 1024; - // a 16MiB maximum, hope it's more than enough - static const uint32_t MAX_HPACK_TABLE_SIZE = 16 * 1024 * 1024; + 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 = (1UL << 32) - 1; - static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; + // prevent creation of new streams from peer + static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 0; + // nghttp2 defaults to 100, but we want more static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; - // All streams are 2^31. Client/Server streams are half that. Just to be on the safe - // side we do 2^29. Same as MAX_STREAMS in source/common/http/http2/conn_pool.h. - static const uint32_t MAX_MAX_CONCURRENT_STREAMS = 1 << 29; - - // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE. - // We only support increasing window size now, so this is also the minimum. + // 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 + // NOTE: Http2::ProdConnPoolImpl::maxTotalStreams still return 2^29 + 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 increase 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_WINDOW_SIZE = (1 << 16) - 1; - static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024; - // maximum from HTTP/2 spec + // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 + static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = (1 << 16) - 1; + // maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2 static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; }; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index dfd41bca20942..ddcdaf1301ca5 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -508,8 +508,7 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { 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(http2_settings.max_concurrent_streams_ <= Http2Settings::MAX_MAX_CONCURRENT_STREAMS); ASSERT(Http2Settings::MIN_INITIAL_WINDOW_SIZE <= http2_settings.initial_window_size_ && http2_settings.initial_window_size_ <= Http2Settings::MAX_INITIAL_WINDOW_SIZE); diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 1b4d1277ca2b9..889b20c169616 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -268,12 +268,12 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "hpack_table_size" : { "type": "integer", "minimum": 0, - "maximum" : 16777216 + "maximum" : 4294967295 }, "max_concurrent_streams" : { "type": "integer", "minimum": 1, - "maximum" : 536870912 + "maximum" : 2147483647 }, "initial_window_size" : { "type": "integer", @@ -1234,12 +1234,12 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "hpack_table_size" : { "type": "integer", "minimum": 0, - "maximum" : 16777216 + "maximum" : 4294967295 }, "max_concurrent_streams" : { "type": "integer", "minimum": 1, - "maximum" : 536870912 + "maximum" : 2147483647 }, "initial_window_size" : { "type": "integer", From a948f43dcf956306cd002621c115a791b702b832 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 19:03:45 +0800 Subject: [PATCH 38/58] add initial_connection_window_size_ for connection-level flow-control window --- docs/configuration/http_conn_man/http_conn_man.rst | 3 +++ include/envoy/http/codec.h | 14 ++++++++++---- source/common/http/http2/codec_impl.cc | 4 ++-- source/common/json/config_schemas.cc | 10 ++++++++++ test/common/http/http2/codec_impl_test.cc | 13 ++++++++++--- 5 files changed, 35 insertions(+), 9 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 14dcbd0d18eca..d0e81bf8bddc3 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -125,6 +125,9 @@ http2_settings NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window size now, so it's also the minimum. + initial_connection_window_size + *(optional, integer)* Similar to initial_window_size, but for connection-level flow-control window. + These are the same options available in the upstream cluster :ref:`http2_settings ` option. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index efe263f782dbb..6dbb2cac8796b 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -155,6 +155,7 @@ struct Http2Settings { uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE}; uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; + uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; // disable HPACK compression static const uint32_t MIN_HPACK_TABLE_SIZE = 0; @@ -164,23 +165,28 @@ struct Http2Settings { static const uint32_t MAX_HPACK_TABLE_SIZE = (1UL << 32) - 1; // prevent creation of new streams from peer - static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 0; + static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; // nghttp2 defaults to 100, but we want more static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; // 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 - // NOTE: Http2::ProdConnPoolImpl::maxTotalStreams still return 2^29 + // same as NGHTTP2_DEFAULT_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 increase 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_WINDOW_SIZE = (1 << 16) - 1; - // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 + // initial stream-level value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = (1 << 16) - 1; // maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2 static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; + + // CONNECTION_WINDOW_SIZE is similar to WINDOW_SIZE, but for connection-level window + static const uint32_t MIN_INITIAL_CONNECTION_WINDOW_SIZE = (1 << 16) - 1; + // nghttp2's default connection-level window equals to its stream-level, but we want more + static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; + static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1; }; /** diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index ddcdaf1301ca5..0bc4e9f0b6d24 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -526,9 +526,9 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { UNREFERENCED_PARAMETER(rc); // Increase connection window size up to our default size. - if (http2_settings.initial_window_size_ != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { + if (http2_settings.initial_connection_window_size_ != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, - http2_settings.initial_window_size_ - + http2_settings.initial_connection_window_size_ - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); } diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 889b20c169616..24e17327f4623 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -279,6 +279,11 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "type": "integer", "minimum": 65535, "maximum" : 2147483647 + }, + "initial_connection_window_size" : { + "type": "integer", + "minimum": 65535, + "maximum" : 2147483647 } } }, @@ -1245,6 +1250,11 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "type": "integer", "minimum": 65535, "maximum" : 2147483647 + }, + "initial_connection_window_size" : { + "type": "integer", + "minimum": 65535, + "maximum" : 2147483647 } } }, diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 8bffefc8c15cd..42e605e622328 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,7 +26,7 @@ using testing::NiceMock; namespace Http { namespace Http2 { -typedef ::testing::tuple http2SettingsTuple; +typedef ::testing::tuple http2SettingsTuple; typedef ::testing::tuple http2SettingsTupleTuple; Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { @@ -34,6 +34,7 @@ Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { ret.hpack_table_size_ = ::testing::get<0>(tp); ret.max_concurrent_streams_ = ::testing::get<1>(tp); ret.initial_window_size_ = ::testing::get<2>(tp); + ret.initial_connection_window_size_ = ::testing::get<3>(tp); return ret; } @@ -263,7 +264,10 @@ INSTANTIATE_TEST_CASE_P( Http2Settings::MAX_MAX_CONCURRENT_STREAMS), ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_WINDOW_SIZE)), + Http2Settings::MAX_INITIAL_WINDOW_SIZE), + ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, + Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, + Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)), ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, Http2Settings::DEFAULT_HPACK_TABLE_SIZE, Http2Settings::MAX_HPACK_TABLE_SIZE), @@ -272,7 +276,10 @@ INSTANTIATE_TEST_CASE_P( Http2Settings::MAX_MAX_CONCURRENT_STREAMS), ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_WINDOW_SIZE)) + Http2Settings::MAX_INITIAL_WINDOW_SIZE), + ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, + Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, + Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) )); From 5c6adf9c6e352f1990975dd85db9ed3e86cb291c Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 19:18:25 +0800 Subject: [PATCH 39/58] only send NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE when needed --- source/common/http/http2/codec_impl.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 0bc4e9f0b6d24..2e8f9039902ff 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -511,16 +511,23 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { ASSERT(http2_settings.max_concurrent_streams_ <= Http2Settings::MAX_MAX_CONCURRENT_STREAMS); ASSERT(Http2Settings::MIN_INITIAL_WINDOW_SIZE <= http2_settings.initial_window_size_ && http2_settings.initial_window_size_ <= Http2Settings::MAX_INITIAL_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); std::vector iv = { - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}, - {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}}; + {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}}; if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_settings.hpack_table_size_}); conn_log_debug("setting HPACK table size to {}", connection_, http2_settings.hpack_table_size_); } + if (http2_settings.initial_window_size_ != NGHTTP2_INITIAL_WINDOW_SIZE) { + iv.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}); + } + int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); ASSERT(rc == 0); UNREFERENCED_PARAMETER(rc); From 4d70686e27a5e27286954f3c2f111e1baeb4d104 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 19:48:45 +0800 Subject: [PATCH 40/58] only send SETTINGS_MAX_CONCURRENT_STREAMS when needed --- include/envoy/http/codec.h | 6 ++--- source/common/http/http2/codec_impl.cc | 32 +++++++++++++++++++------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 6dbb2cac8796b..9de936b15338d 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -166,11 +166,11 @@ struct Http2Settings { // prevent creation of new streams from peer static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; - // nghttp2 defaults to 100, but we want more - static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024; + // defaults to no limit, 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_DEFAULT_MAX_CONCURRENT_STREAMS from nghttp2 + // 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 diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 2e8f9039902ff..c24b9132d44c1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -516,27 +516,43 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { http2_settings.initial_connection_window_size_ <= Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE); - std::vector iv = { - {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_}}; + std::vector iv; if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, http2_settings.hpack_table_size_}); conn_log_debug("setting HPACK table size to {}", connection_, http2_settings.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_}); + conn_log_debug("setting max concurrent streams to {}", connection_, + http2_settings.max_concurrent_streams_); + } + if (http2_settings.initial_window_size_ != NGHTTP2_INITIAL_WINDOW_SIZE) { iv.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}); + conn_log_debug("setting stream-level initial window size to {}", connection_, + http2_settings.initial_window_size_); } - int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); - ASSERT(rc == 0); - UNREFERENCED_PARAMETER(rc); + if (!iv.empty()) { + int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); + ASSERT(rc == 0); + UNREFERENCED_PARAMETER(rc); + } else { + // nghttp2_submit_settings need to be called at least once + int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, 0, 0); + ASSERT(rc == 0); + UNREFERENCED_PARAMETER(rc); + } // Increase connection window size up to our default size. if (http2_settings.initial_connection_window_size_ != NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { - rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, - http2_settings.initial_connection_window_size_ - - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); + conn_log_debug("updating connection-level initial window size to {}", connection_, + http2_settings.initial_connection_window_size_); + int rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, + http2_settings.initial_connection_window_size_ - + NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); } } From 8ead9edd7b5441bc1a05bcf77e6961cd225e5b63 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 21:49:01 +0800 Subject: [PATCH 41/58] change MIN_MAX_CONCURRENT_STREAMS to 0, skip test if server-side max streams is 0 --- include/envoy/http/codec.h | 2 +- test/common/http/http2/codec_impl_test.cc | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 9de936b15338d..cc85bdd4dc621 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -165,7 +165,7 @@ struct Http2Settings { static const uint32_t MAX_HPACK_TABLE_SIZE = (1UL << 32) - 1; // prevent creation of new streams from peer - static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1; + static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 0; // defaults to no limit, 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 diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 42e605e622328..41faf9508bdd3 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -38,6 +38,12 @@ Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { return ret; } +// skip test if server-side max concurrent streams is 0 +#define SKIP_TEST_IF_NOT_SUITABLE \ + if (server_http2settings_.max_concurrent_streams_ == 0) { \ + return; \ + } + class Http2CodecImplTest : public testing::TestWithParam { public: struct ConnectionWrapper { @@ -64,6 +70,8 @@ class Http2CodecImplTest : public testing::TestWithParam StreamDecoder& { response_encoder_ = &encoder; @@ -100,6 +108,8 @@ class Http2CodecImplTest : public testing::TestWithParam Date: Sat, 27 May 2017 22:04:23 +0800 Subject: [PATCH 42/58] add linke to initial_window_size --- docs/configuration/http_conn_man/http_conn_man.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index d0e81bf8bddc3..c51c794361687 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -118,6 +118,8 @@ http2_settings NOTE: total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to exclude stream 0. +.. _config_http_conn_man_http2_settings_initial_window_size: + initial_window_size *(optional, integer)* `Initial stream-level flow-control window`_ size. Valid values range from 65535 (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 65535 (2^16 - 1). @@ -126,7 +128,8 @@ http2_settings size now, so it's also the minimum. initial_connection_window_size - *(optional, integer)* Similar to initial_window_size, but for connection-level flow-control window. + *(optional, integer)* Similar to :ref:`initial_window_size + `, but for connection-level flow-control window. These are the same options available in the upstream cluster :ref:`http2_settings ` option. From f24fc0900cdcec5059635a55bc596df72b72b73f Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 22:08:55 +0800 Subject: [PATCH 43/58] fix bazel.release --- source/common/http/http2/codec_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index c24b9132d44c1..fb10bb81634f7 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -554,6 +554,7 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { http2_settings.initial_connection_window_size_ - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); ASSERT(rc == 0); + UNREFERENCED_PARAMETER(rc); } } From 808fb113e74476f7ffa06ff45671fd11ba241d30 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 22:14:07 +0800 Subject: [PATCH 44/58] fix max_concurrent_streams minimum in config_schemas.cc --- source/common/json/config_schemas.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 24e17327f4623..f5daa45ed19eb 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -272,7 +272,7 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( }, "max_concurrent_streams" : { "type": "integer", - "minimum": 1, + "minimum": 0, "maximum" : 2147483647 }, "initial_window_size" : { @@ -1243,7 +1243,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( }, "max_concurrent_streams" : { "type": "integer", - "minimum": 1, + "minimum": 0, "maximum" : 2147483647 }, "initial_window_size" : { From 1300db21b1772cc9d8e0e1be136bf4edd0d160ac Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 22:25:14 +0800 Subject: [PATCH 45/58] parse and test for initial_connection_window_size_ --- source/common/http/utility.cc | 3 +++ test/common/http/utility_test.cc | 16 ++++++++++------ test/test_common/utility.cc | 1 + 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 7ed9e132e233b..5c49b8815fa39 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -146,6 +146,9 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); ret.initial_window_size_ = http2_settings->getInteger( "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); + ret.initial_connection_window_size_ = + http2_settings->getInteger("initial_connection_window_size", + Http::Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); // http_codec_options config is DEPRECATED std::string options = config.getString("http_codec_options", ""); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e7b9ce2ffe7cb..9c8e6731fca46 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -94,19 +94,23 @@ TEST(HttpUtility, parseHttp2Settings) { EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_); EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, + http2_settings.initial_connection_window_size_); } { auto http2_settings = Utility::parseHttp2Settings(*Json::Factory::loadFromString(R"raw({ "http2_settings" : { - "hpack_table_size": 1234, - "max_concurrent_streams": 1234, - "initial_window_size": 5678 + "hpack_table_size": 1, + "max_concurrent_streams": 2, + "initial_window_size": 3, + "initial_connection_window_size": 4 } })raw")); - EXPECT_EQ(1234U, http2_settings.hpack_table_size_); - EXPECT_EQ(1234U, http2_settings.max_concurrent_streams_); - EXPECT_EQ(5678U, http2_settings.initial_window_size_); + EXPECT_EQ(1U, http2_settings.hpack_table_size_); + EXPECT_EQ(2U, http2_settings.max_concurrent_streams_); + EXPECT_EQ(3U, http2_settings.initial_window_size_); + EXPECT_EQ(4U, http2_settings.initial_connection_window_size_); } { diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 8bcf08f449d55..258d5f1d0e1c4 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -124,6 +124,7 @@ namespace Http { const uint32_t Http2Settings::DEFAULT_HPACK_TABLE_SIZE; const uint32_t Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS; const uint32_t Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE; +const uint32_t Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE; TestHeaderMapImpl::TestHeaderMapImpl() : HeaderMapImpl() {} From c94a3d7fea579479e4c90b3ed812ff4e0c0b459f Mon Sep 17 00:00:00 2001 From: jwfang Date: Sat, 27 May 2017 22:37:02 +0800 Subject: [PATCH 46/58] fix docs --- docs/configuration/http_conn_man/http_conn_man.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index c51c794361687..396f573fbfcf5 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -112,10 +112,10 @@ http2_settings with 0 effectively disables header compression. max_concurrent_streams - *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values - range from 0 to 2147483647 (2^31 - 1) and defaults to 1024. + *(optional, integer)* `Maximum concurrent streams`_ allowed for peer on one HTTP/2 connection. + Valid values range from 0 to 2147483647 (2^31 - 1) and defaults to 2147483647. - NOTE: total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to + NOTE: Total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to exclude stream 0. .. _config_http_conn_man_http2_settings_initial_window_size: From be54c338d9d8ba6780abf2d8bb3157addd25c7cd Mon Sep 17 00:00:00 2001 From: jwfang Date: Sun, 28 May 2017 12:53:32 +0800 Subject: [PATCH 47/58] minor tuning --- .../http_conn_man/http_conn_man.rst | 2 +- include/envoy/http/codec.h | 2 +- test/common/http/http2/codec_impl_test.cc | 53 ++++++++----------- test/common/http/utility_test.cc | 2 + 4 files changed, 27 insertions(+), 32 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 396f573fbfcf5..e89b9b2e9e077 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -103,7 +103,7 @@ http_codec_options .. _config_http_conn_man_http2_settings: http2_settings - *(optional, object)* Additional HTTP/2 codec options that are passed directly to the HTTP/2 codec. + *(optional, object)* Additional HTTP/2 settings that are passed directly to the HTTP/2 codec. Currently supported settings are: hpack_table_size diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index cc85bdd4dc621..ed0b5fd4c087f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -166,7 +166,7 @@ struct Http2Settings { // prevent creation of new streams from peer static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 0; - // defaults to no limit, same as nghttp2 + // 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 diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 41faf9508bdd3..2235854d1d477 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -29,7 +29,7 @@ namespace Http2 { typedef ::testing::tuple http2SettingsTuple; typedef ::testing::tuple http2SettingsTupleTuple; -Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { +static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { Http2Settings ret; ret.hpack_table_size_ = ::testing::get<0>(tp); ret.max_concurrent_streams_ = ::testing::get<1>(tp); @@ -275,35 +275,28 @@ TEST_P(Http2CodecImplTest, DeferredReset) { server_wrapper_.dispatch(Buffer::OwnedImpl(), server_); } -INSTANTIATE_TEST_CASE_P( - Http2CodecImplTest, Http2CodecImplTest, - ::testing::Combine( - ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::MAX_HPACK_TABLE_SIZE), - ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::MAX_MAX_CONCURRENT_STREAMS), - ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_WINDOW_SIZE), - ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, - Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)), - ::testing::Combine(::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, - Http2Settings::DEFAULT_HPACK_TABLE_SIZE, - Http2Settings::MAX_HPACK_TABLE_SIZE), - ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, - Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, - Http2Settings::MAX_MAX_CONCURRENT_STREAMS), - ::testing::Values(Http2Settings::MIN_INITIAL_WINDOW_SIZE, - Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_WINDOW_SIZE), - ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, - Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, - Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) - - )); +#define HTTP2SETTINGS_DEFAULT_COMBIME \ + ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ + ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ + ::testing::Values(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), \ + ::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) + +INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, + ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBIME, + HTTP2SETTINGS_DEFAULT_COMBIME)); + +#define HTTP2SETTINGS_EDGE_COMBIME \ + ::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_WINDOW_SIZE, \ + Http2Settings::MAX_INITIAL_WINDOW_SIZE), \ + ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, \ + Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) + +INSTANTIATE_TEST_CASE_P(Http2CodecImplTestEdgeSettings, Http2CodecImplTest, + ::testing::Combine(HTTP2SETTINGS_EDGE_COMBIME, HTTP2SETTINGS_EDGE_COMBIME)); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 9c8e6731fca46..907f3ef17c40e 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -121,6 +121,8 @@ TEST(HttpUtility, parseHttp2Settings) { EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_); EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); + EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, + http2_settings.initial_connection_window_size_); } { From 756a322663858e1d7d6458378539f78bdba94f3a Mon Sep 17 00:00:00 2001 From: jwfang Date: Sun, 28 May 2017 23:24:37 +0800 Subject: [PATCH 48/58] revert max_concurrent_stream to 1 --- .../http_conn_man/http_conn_man.rst | 2 +- include/envoy/http/codec.h | 4 ++-- test/common/http/http2/codec_impl_test.cc | 22 ------------------- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index e89b9b2e9e077..0240e4311826c 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -113,7 +113,7 @@ http2_settings max_concurrent_streams *(optional, integer)* `Maximum concurrent streams`_ allowed for peer on one HTTP/2 connection. - Valid values range from 0 to 2147483647 (2^31 - 1) and defaults to 2147483647. + Valid values range from 1 to 2147483647 (2^31 - 1) and defaults to 2147483647. NOTE: Total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to exclude stream 0. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ed0b5fd4c087f..ead462f766815 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -164,8 +164,8 @@ struct Http2Settings { // no maximum from HTTP/2 spec, use unsigned 32-bit maximum static const uint32_t MAX_HPACK_TABLE_SIZE = (1UL << 32) - 1; - // prevent creation of new streams from peer - static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 0; + // 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 diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 2235854d1d477..f9a0c9bbcf589 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -38,12 +38,6 @@ static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { return ret; } -// skip test if server-side max concurrent streams is 0 -#define SKIP_TEST_IF_NOT_SUITABLE \ - if (server_http2settings_.max_concurrent_streams_ == 0) { \ - return; \ - } - class Http2CodecImplTest : public testing::TestWithParam { public: struct ConnectionWrapper { @@ -70,8 +64,6 @@ class Http2CodecImplTest : public testing::TestWithParam StreamDecoder& { response_encoder_ = &encoder; @@ -108,8 +100,6 @@ class Http2CodecImplTest : public testing::TestWithParam Date: Sun, 28 May 2017 23:40:48 +0800 Subject: [PATCH 49/58] rename initial_window_size to initial_stream_window_size --- docs/configuration/http_conn_man/http_conn_man.rst | 8 ++++---- include/envoy/http/codec.h | 12 ++++++------ source/common/http/http2/codec_impl.cc | 10 +++++----- source/common/http/utility.cc | 4 ++-- source/common/json/config_schemas.cc | 4 ++-- test/common/http/http2/codec_impl_test.cc | 8 ++++---- test/common/http/utility_test.cc | 8 ++++---- test/test_common/utility.cc | 2 +- 8 files changed, 28 insertions(+), 28 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 0240e4311826c..b835687e6f994 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -118,9 +118,9 @@ http2_settings NOTE: Total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to exclude stream 0. -.. _config_http_conn_man_http2_settings_initial_window_size: +.. _config_http_conn_man_http2_settings_initial_stream_window_size: - initial_window_size + initial_stream_window_size *(optional, integer)* `Initial stream-level flow-control window`_ size. Valid values range from 65535 (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 65535 (2^16 - 1). @@ -128,8 +128,8 @@ http2_settings size now, so it's also the minimum. initial_connection_window_size - *(optional, integer)* Similar to :ref:`initial_window_size - `, but for connection-level flow-control window. + *(optional, integer)* Similar to :ref:`initial_stream_window_size + `, but for connection-level flow-control window. These are the same options available in the upstream cluster :ref:`http2_settings ` option. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ead462f766815..b982491d0c4c2 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -154,7 +154,7 @@ 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_window_size_{DEFAULT_INITIAL_WINDOW_SIZE}; + uint32_t initial_stream_window_size_{DEFAULT_INITIAL_STREAM_WINDOW_SIZE}; uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; // disable HPACK compression @@ -176,13 +176,13 @@ struct Http2Settings { // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 // NOTE: we only support increase 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_WINDOW_SIZE = (1 << 16) - 1; - // initial stream-level value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 - static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = (1 << 16) - 1; + static const uint32_t MIN_INITIAL_STREAM_WINDOW_SIZE = (1 << 16) - 1; + // initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 + static const uint32_t DEFAULT_INITIAL_STREAM_WINDOW_SIZE = (1 << 16) - 1; // maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2 - static const uint32_t MAX_INITIAL_WINDOW_SIZE = (1U << 31) - 1; + static const uint32_t MAX_INITIAL_STREAM_WINDOW_SIZE = (1U << 31) - 1; - // CONNECTION_WINDOW_SIZE is similar to WINDOW_SIZE, but for connection-level window + // CONNECTION_WINDOW_SIZE is similar to STREAM_WINDOW_SIZE, but for connection-level window static const uint32_t MIN_INITIAL_CONNECTION_WINDOW_SIZE = (1 << 16) - 1; // nghttp2's default connection-level window equals to its stream-level, but we want more static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index fb10bb81634f7..c6c9c597c4d7f 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -509,8 +509,8 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { ASSERT(http2_settings.hpack_table_size_ <= Http2Settings::MAX_HPACK_TABLE_SIZE); ASSERT(http2_settings.max_concurrent_streams_ <= Http2Settings::MAX_MAX_CONCURRENT_STREAMS); - ASSERT(Http2Settings::MIN_INITIAL_WINDOW_SIZE <= http2_settings.initial_window_size_ && - http2_settings.initial_window_size_ <= Http2Settings::MAX_INITIAL_WINDOW_SIZE); + 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_ <= @@ -529,10 +529,10 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { http2_settings.max_concurrent_streams_); } - if (http2_settings.initial_window_size_ != NGHTTP2_INITIAL_WINDOW_SIZE) { - iv.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_}); + 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_}); conn_log_debug("setting stream-level initial window size to {}", connection_, - http2_settings.initial_window_size_); + http2_settings.initial_stream_window_size_); } if (!iv.empty()) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5c49b8815fa39..2fe313e2929dd 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -144,8 +144,8 @@ Http2Settings Utility::parseHttp2Settings(const Json::Object& config) { http2_settings->getInteger("hpack_table_size", Http::Http2Settings::DEFAULT_HPACK_TABLE_SIZE); ret.max_concurrent_streams_ = http2_settings->getInteger( "max_concurrent_streams", Http::Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS); - ret.initial_window_size_ = http2_settings->getInteger( - "initial_window_size", Http::Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE); + ret.initial_stream_window_size_ = http2_settings->getInteger( + "initial_stream_window_size", Http::Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); ret.initial_connection_window_size_ = http2_settings->getInteger("initial_connection_window_size", Http::Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index d238631d879b4..123206fed0f9a 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -275,7 +275,7 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "minimum": 0, "maximum" : 2147483647 }, - "initial_window_size" : { + "initial_stream_window_size" : { "type": "integer", "minimum": 65535, "maximum" : 2147483647 @@ -1247,7 +1247,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "minimum": 0, "maximum" : 2147483647 }, - "initial_window_size" : { + "initial_stream_window_size" : { "type": "integer", "minimum": 65535, "maximum" : 2147483647 diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index f9a0c9bbcf589..ae99a94045cb8 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -33,7 +33,7 @@ static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { Http2Settings ret; ret.hpack_table_size_ = ::testing::get<0>(tp); ret.max_concurrent_streams_ = ::testing::get<1>(tp); - ret.initial_window_size_ = ::testing::get<2>(tp); + ret.initial_stream_window_size_ = ::testing::get<2>(tp); ret.initial_connection_window_size_ = ::testing::get<3>(tp); return ret; } @@ -256,7 +256,7 @@ TEST_P(Http2CodecImplTest, DeferredReset) { #define HTTP2SETTINGS_DEFAULT_COMBIME \ ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ - ::testing::Values(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE), \ + ::testing::Values(Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, @@ -268,8 +268,8 @@ INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, ::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_WINDOW_SIZE, \ - Http2Settings::MAX_INITIAL_WINDOW_SIZE), \ + ::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)) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 907f3ef17c40e..4bbaa646d70f9 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -93,7 +93,7 @@ TEST(HttpUtility, parseHttp2Settings) { 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_WINDOW_SIZE, http2_settings.initial_window_size_); + 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_); } @@ -103,13 +103,13 @@ TEST(HttpUtility, parseHttp2Settings) { "http2_settings" : { "hpack_table_size": 1, "max_concurrent_streams": 2, - "initial_window_size": 3, + "initial_stream_window_size": 3, "initial_connection_window_size": 4 } })raw")); EXPECT_EQ(1U, http2_settings.hpack_table_size_); EXPECT_EQ(2U, http2_settings.max_concurrent_streams_); - EXPECT_EQ(3U, http2_settings.initial_window_size_); + EXPECT_EQ(3U, http2_settings.initial_stream_window_size_); EXPECT_EQ(4U, http2_settings.initial_connection_window_size_); } @@ -120,7 +120,7 @@ TEST(HttpUtility, parseHttp2Settings) { EXPECT_EQ(0, http2_settings.hpack_table_size_); EXPECT_EQ(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, http2_settings.max_concurrent_streams_); - EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE, http2_settings.initial_window_size_); + 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_); } diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index de6de65ceb36c..f54a8feff8ee9 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -124,7 +124,7 @@ 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_WINDOW_SIZE; +const uint32_t Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE; const uint32_t Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE; TestHeaderMapImpl::TestHeaderMapImpl() : HeaderMapImpl() {} From 2b17fc69a16e910365c783a27c7b6a69ff95d321 Mon Sep 17 00:00:00 2001 From: jwfang Date: Sun, 28 May 2017 23:56:30 +0800 Subject: [PATCH 50/58] address review comment: doc notes & tests --- .../http_conn_man/http_conn_man.rst | 3 --- source/common/http/http2/codec_impl.cc | 8 +++++--- test/common/http/http2/codec_impl_test.cc | 20 ++++++++++--------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index b835687e6f994..c65dd9d5f4fb3 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -115,9 +115,6 @@ http2_settings *(optional, integer)* `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. - NOTE: Total streams is 32-bit unsigned, one-side (client/server) is half that, and we also need to - exclude stream 0. - .. _config_http_conn_man_http2_settings_initial_stream_window_size: initial_stream_window_size diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index c6c9c597c4d7f..d43d90c7dd3fd 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -509,8 +509,9 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { ASSERT(http2_settings.hpack_table_size_ <= Http2Settings::MAX_HPACK_TABLE_SIZE); ASSERT(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_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_ <= @@ -530,7 +531,8 @@ void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { } 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_}); + iv.push_back( + {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, http2_settings.initial_stream_window_size_}); conn_log_debug("setting stream-level initial window size to {}", connection_, http2_settings.initial_stream_window_size_); } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index ae99a94045cb8..f80868c8dea70 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -26,10 +26,11 @@ using testing::NiceMock; namespace Http { namespace Http2 { -typedef ::testing::tuple http2SettingsTuple; -typedef ::testing::tuple http2SettingsTupleTuple; +typedef ::testing::tuple Http2SettingsTuple; +typedef ::testing::tuple Http2SettingsTestParam; -static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { +namespace { +Http2Settings Http2SettingsFromTuple(const Http2SettingsTuple& tp) { Http2Settings ret; ret.hpack_table_size_ = ::testing::get<0>(tp); ret.max_concurrent_streams_ = ::testing::get<1>(tp); @@ -37,8 +38,9 @@ static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) { ret.initial_connection_window_size_ = ::testing::get<3>(tp); return ret; } +} -class Http2CodecImplTest : public testing::TestWithParam { +class Http2CodecImplTest : public testing::TestWithParam { public: struct ConnectionWrapper { void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) { @@ -57,9 +59,9 @@ class Http2CodecImplTest : public testing::TestWithParam(GetParam()))), + : client_http2settings_(Http2SettingsFromTuple(::testing::get<0>(GetParam()))), client_(client_connection_, client_callbacks_, stats_store_, client_http2settings_), - server_http2settings_(http2SettingsFromTuple(::testing::get<1>(GetParam()))), + server_http2settings_(Http2SettingsFromTuple(::testing::get<1>(GetParam()))), server_(server_connection_, server_callbacks_, stats_store_, server_http2settings_), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); @@ -256,7 +258,7 @@ TEST_P(Http2CodecImplTest, DeferredReset) { #define HTTP2SETTINGS_DEFAULT_COMBIME \ ::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_STREAM_WINDOW_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, @@ -268,8 +270,8 @@ INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, ::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_STREAM_WINDOW_SIZE, \ + Http2Settings::MAX_INITIAL_STREAM_WINDOW_SIZE), \ ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE, \ Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) From beacfe5a45b276882d2135ccfff72412c44040a5 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 00:14:45 +0800 Subject: [PATCH 51/58] change default stream/connection window size to 256MB --- docs/configuration/http_conn_man/http_conn_man.rst | 7 +++++-- include/envoy/http/codec.h | 8 +++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index c65dd9d5f4fb3..6d401a9264998 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -119,14 +119,17 @@ http2_settings initial_stream_window_size *(optional, integer)* `Initial stream-level flow-control window`_ size. Valid values range from 65535 - (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 65535 (2^16 - 1). + (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 268435456 + (256 * 1024 * 1024). NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window size now, so it's also the minimum. initial_connection_window_size *(optional, integer)* Similar to :ref:`initial_stream_window_size - `, but for connection-level flow-control window. + `, but for connection-level flow-control + window. Currently , this has the same minimum/maximum/default as :ref:`initial_stream_window_size + `. These are the same options available in the upstream cluster :ref:`http2_settings ` option. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index b982491d0c4c2..2ad28df791c4f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -177,14 +177,16 @@ struct Http2Settings { // NOTE: we only support increase 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, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2 - static const uint32_t DEFAULT_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, but we want more + // 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; }; From 685e8117718728a2f9c41258951f55c8055f0435 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 00:32:12 +0800 Subject: [PATCH 52/58] format fix --- test/common/http/utility_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 4bbaa646d70f9..b70152abbab43 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -93,7 +93,8 @@ TEST(HttpUtility, parseHttp2Settings) { 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_STREAM_WINDOW_SIZE, + http2_settings.initial_stream_window_size_); EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, http2_settings.initial_connection_window_size_); } @@ -120,7 +121,8 @@ TEST(HttpUtility, parseHttp2Settings) { EXPECT_EQ(0, 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_STREAM_WINDOW_SIZE, + http2_settings.initial_stream_window_size_); EXPECT_EQ(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE, http2_settings.initial_connection_window_size_); } From 6476bad396679569e69473548bc71502576c0456 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 00:38:59 +0800 Subject: [PATCH 53/58] add notes in DEPRECATED.md --- DEPRECATED.md | 1 + docs/configuration/http_conn_man/http_conn_man.rst | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/DEPRECATED.md b/DEPRECATED.md index 91ccc6259f73c..fc0df1f0aed11 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -9,3 +9,4 @@ The following features have been DEPRECATED and will be removed in the specified * Config option `statsd_local_udp_port` has been deprecated and has been replaced with `statsd_udp_ip_address`. * `HttpFilterConfigFactory` filter API has been deprecated in favor of `NamedHttpFilterConfigFactory`. + * Config option `http_codec_options` has been deprecated and has been replaced with `http2_settings`. diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 6d401a9264998..c65dbc7940123 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -86,7 +86,7 @@ add_user_agent .. _config_http_conn_man_http_codec_options: -http_codec_options +http_codec_options (Warning: DEPRECATED and will be removed in 1.4.0) **DEPRECATED**, use :ref:`http2_settings ` instead. *(optional, string)* Additional options that are passed directly to the codec. Not all options From 2a5116c470d0da31ca669c29a3b3e556e3192f80 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 00:46:04 +0800 Subject: [PATCH 54/58] fix max_concurrent_streams json schema --- source/common/json/config_schemas.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 123206fed0f9a..d2f47bb89c140 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -272,7 +272,7 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( }, "max_concurrent_streams" : { "type": "integer", - "minimum": 0, + "minimum": 1, "maximum" : 2147483647 }, "initial_stream_window_size" : { @@ -1244,7 +1244,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( }, "max_concurrent_streams" : { "type": "integer", - "minimum": 0, + "minimum": 1, "maximum" : 2147483647 }, "initial_stream_window_size" : { From 8b635f4ec59452a79768cfcc7d413b3365544235 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 01:03:04 +0800 Subject: [PATCH 55/58] minor tuning --- include/envoy/http/codec.h | 4 ++-- test/common/http/http2/codec_impl_test.cc | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 2ad28df791c4f..48e80b8ca7363 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -168,8 +168,8 @@ struct Http2Settings { 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 + // 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; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index f80868c8dea70..b8d5df88db964 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -255,6 +255,8 @@ TEST_P(Http2CodecImplTest, DeferredReset) { server_wrapper_.dispatch(Buffer::OwnedImpl(), server_); } +// we seperate default/edge cases here to avoid combinatorial explosion + #define HTTP2SETTINGS_DEFAULT_COMBIME \ ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ From 73dcf192a9ab68eb0489eb47df6db771dfe2ac12 Mon Sep 17 00:00:00 2001 From: jwfang Date: Mon, 29 May 2017 02:18:48 +0800 Subject: [PATCH 56/58] add missing assert --- source/common/http/http2/codec_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index d43d90c7dd3fd..284ffa11b9ec2 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -508,7 +508,8 @@ void ConnectionImpl::sendPendingFrames() { void ConnectionImpl::sendSettings(const Http2Settings& http2_settings) { ASSERT(http2_settings.hpack_table_size_ <= Http2Settings::MAX_HPACK_TABLE_SIZE); - ASSERT(http2_settings.max_concurrent_streams_ <= Http2Settings::MAX_MAX_CONCURRENT_STREAMS); + 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); From 6c5be110d6d53773c84453ced12d9458eb05cca2 Mon Sep 17 00:00:00 2001 From: jwfang Date: Thu, 1 Jun 2017 10:25:59 +0800 Subject: [PATCH 57/58] update docs link --- .../http_conn_man/http_conn_man.rst | 20 +++++++++---------- include/envoy/http/codec.h | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index c65dbc7940123..1b16c198e98c9 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -107,22 +107,26 @@ http2_settings Currently supported settings are: hpack_table_size - *(optional, integer)* `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, - with 0 effectively disables header compression. + *(optional, integer)* `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 compression. max_concurrent_streams - *(optional, integer)* `Maximum concurrent streams`_ allowed for peer on one HTTP/2 connection. + *(optional, integer)* `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. .. _config_http_conn_man_http2_settings_initial_stream_window_size: initial_stream_window_size - *(optional, integer)* `Initial stream-level flow-control window`_ size. Valid values range from 65535 + *(optional, integer)* `Initial stream-level flow-control window + `_ size. Valid values range from 65535 (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 268435456 (256 * 1024 * 1024). - NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window + NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increasing the default window size now, so it's also the minimum. initial_connection_window_size @@ -134,10 +138,6 @@ http2_settings These are the same options available in the upstream cluster :ref:`http2_settings ` option. - .. _Maximum Table Size: http://httpwg.org/specs/rfc7541.html#rfc.section.4.2 - .. _Maximum concurrent streams: http://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2 - .. _Initial stream-level flow-control window: http://httpwg.org/specs/rfc7540.html#rfc.section.6.9.2 - .. _config_http_conn_man_server_name: server_name diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 48e80b8ca7363..a1ced838c2075 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -174,7 +174,7 @@ struct Http2Settings { 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 increase window size now, so this is also the minimum + // 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) From a38d334a7a9c6da4047436e55b0bba467ad0a1d1 Mon Sep 17 00:00:00 2001 From: jwfang Date: Thu, 1 Jun 2017 10:31:28 +0800 Subject: [PATCH 58/58] fix docs caps --- docs/configuration/http_conn_man/http_conn_man.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst index 1b16c198e98c9..c2b109207cf29 100644 --- a/docs/configuration/http_conn_man/http_conn_man.rst +++ b/docs/configuration/http_conn_man/http_conn_man.rst @@ -107,7 +107,7 @@ http2_settings Currently supported settings are: hpack_table_size - *(optional, integer)* `Maximum Table Size `_ + *(optional, integer)* `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 compression.