-
Notifications
You must be signed in to change notification settings - Fork 5.4k
make http/2 settings configurable (#988) #989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
77459a1
b75039a
6d1a199
bcd7ba6
794614b
431c909
94fad70
f4dcff7
d4251d4
b7ee834
164fe93
4e7251b
d02e90d
1fe2009
7709cd8
b072dfb
830a323
f9d117a
fc3a93b
342a049
8065aea
1458f54
eea3df1
3bd6cd2
36a9170
36466c0
1006e73
35bd52f
c55f885
2e5df72
e438286
01896d3
6aa47f7
d33b753
0d2348b
c97cae1
64609e6
f38e0f6
6a2f0a6
01228c4
fabcbed
c43c0b8
a948f43
5c6adf9
4d70686
8ead9ed
379ac46
f24fc09
808fb11
1300db2
c94a3d7
be54c33
4a301fc
756a322
032785d
2b17fc6
beacfe5
a1b23bc
685e811
6476bad
2a5116c
8b635f4
73dcf19
6c5be11
a38d334
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -506,24 +506,59 @@ void ConnectionImpl::sendPendingFrames() { | |
| } | ||
| } | ||
|
|
||
| void ConnectionImpl::sendSettings(uint64_t codec_options) { | ||
| std::vector<nghttp2_settings_entry> iv = { | ||
| {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, MAX_CONCURRENT_STREAMS}, | ||
| {NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, DEFAULT_WINDOW_SIZE}}; | ||
| 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_STREAM_WINDOW_SIZE <= http2_settings.initial_stream_window_size_ && | ||
| http2_settings.initial_stream_window_size_ <= Http2Settings::MAX_INITIAL_STREAM_WINDOW_SIZE); | ||
| ASSERT(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE <= | ||
| http2_settings.initial_connection_window_size_ && | ||
| http2_settings.initial_connection_window_size_ <= | ||
| Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE); | ||
|
|
||
| std::vector<nghttp2_settings_entry> iv; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For posterity, for perf we should switch this to a fixed size array with a pre-defined max size. Added to #210. |
||
|
|
||
| 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 (codec_options & CodecOptions::NoCompression) { | ||
| iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0}); | ||
| conn_log_debug("disabling header compression", connection_); | ||
| 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_); | ||
| } | ||
|
|
||
| int rc = nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, &iv[0], iv.size()); | ||
| ASSERT(rc == 0); | ||
| UNREFERENCED_PARAMETER(rc); | ||
| 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_stream_window_size_); | ||
| } | ||
|
|
||
| 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. | ||
| rc = nghttp2_submit_window_update(session_, NGHTTP2_FLAG_NONE, 0, | ||
| DEFAULT_WINDOW_SIZE - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE); | ||
| ASSERT(rc == 0); | ||
| if (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); | ||
| UNREFERENCED_PARAMETER(rc); | ||
| } | ||
| } | ||
|
|
||
| ConnectionImpl::Http2Callbacks::Http2Callbacks() { | ||
|
|
@@ -602,19 +637,19 @@ 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); | ||
| } | ||
|
|
||
| ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } | ||
|
|
||
| ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, | ||
| ConnectionCallbacks& callbacks, Stats::Scope& stats, | ||
| uint64_t 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) { | ||
|
|
@@ -648,11 +683,11 @@ 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 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) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,12 +136,34 @@ bool Utility::isInternalRequest(const HeaderMap& headers) { | |
| return Network::Utility::isInternalAddress(forwarded_for->value().c_str()); | ||
| } | ||
|
|
||
| uint64_t Utility::parseCodecOptions(const Json::Object& config) { | ||
| uint64_t ret = 0; | ||
| 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_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); | ||
|
|
||
| // http_codec_options config is DEPRECATED | ||
| std::string options = config.getString("http_codec_options", ""); | ||
| if (options != "") { | ||
| 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, ',')) { | ||
| if (option == "no_compression") { | ||
| ret |= CodecOptions::NoCompression; | ||
| if (http2_settings->hasObject("hpack_table_size") && ret.hpack_table_size_ != 0) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems you're seeing an outdated version, :) it's tested by this, if i am not misunderstanding you. |
||
| 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)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference would be to make this a class that hides a bit of it's own logic - I touched on this a bit in another comment. It would enable writing code like calling "http2settings.use_compression()" to find out if compression is enabled rather than requiring the caller to understand that codec_options_ is really a bit field.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i remove the CodecOptions entirely, and add a new hpack_table_size config.
if this is acceptable, then i will finish the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only the equivalent of the getter logic now I think it's fine (but @dnoe makes a valid point in general).