Skip to content
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.

Sign up for GitHub

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

http2: support custom SETTINGS parameters #9964

Merged
merged 34 commits into from
Mar 19, 2020
Merged

http2: support custom SETTINGS parameters #9964

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9691ae7
Refactor H2 codec Http2Settings config processing.
AndresGuedez Jan 31, 2020
28cdf4d
Support custom H/2 settings parameters.
AndresGuedez Feb 6, 2020
bad389c
Add Http2ProtocolOptions cluster config test.
AndresGuedez Feb 6, 2020
4b78a16
Cleanup.
AndresGuedez Feb 7, 2020
0f50401
Cleanup.
AndresGuedez Feb 7, 2020
16b8ba7
Fix comment.
AndresGuedez Feb 7, 2020
08ab741
Fix format.
AndresGuedez Feb 11, 2020
b553ef5
Fix docs.
AndresGuedez Feb 13, 2020
fdc7c9b
Refactor SETTINGS frame received callback handling.
AndresGuedez Feb 13, 2020
cc5fe09
Fix format.
AndresGuedez Feb 13, 2020
6def08e
Fix format.
AndresGuedez Feb 13, 2020
1cf9c32
Fix spelling.
AndresGuedez Feb 13, 2020
6bfbfc6
Remove test debug logs.
AndresGuedez Feb 13, 2020
5632a90
Merge remote-tracking branch 'upstream/master' into h2-user-defined-s…
AndresGuedez Feb 18, 2020
b764d34
Regenerate API shadow.
AndresGuedez Feb 18, 2020
9455741
Refactor newly introduced references to Http2Settings.
AndresGuedez Feb 18, 2020
78d3e63
Adjust expected memory usage for cluster memory tests.
AndresGuedez Feb 18, 2020
8b7621c
Kick CI
AndresGuedez Feb 25, 2020
7f18239
Merge remote-tracking branch 'upstream/master' into h2-user-defined-s…
AndresGuedez Feb 25, 2020
07c1814
Merge conflict resolution.
AndresGuedez Feb 27, 2020
8293133
Disallow user defined and named parameter collisions in Http2Protocol…
AndresGuedez Mar 12, 2020
31c56a0
Merge remote-tracking branch 'upstream/master' into h2-user-defined-s…
AndresGuedez Mar 12, 2020
cb35501
Spelling fixes.
AndresGuedez Mar 12, 2020
43b83bf
Simplify custom settings parameters validation logic and minor cleanup.
AndresGuedez Mar 14, 2020
be39274
Minor cleanup.
AndresGuedez Mar 14, 2020
e48de85
Fix spelling.
AndresGuedez Mar 16, 2020
59e2dfe
Fix cluster memory usage expectations.
AndresGuedez Mar 16, 2020
3f513d4
Avoid using deprecated router filter name.
AndresGuedez Mar 16, 2020
53bf245
Change warning to debug log.
AndresGuedez Mar 17, 2020
cf8f7f2
Simplify H/2 custom settings parameters validation logic.
AndresGuedez Mar 18, 2020
ad297e2
Change comment wording.
AndresGuedez Mar 18, 2020
6ae55cd
Minor cleaup.
AndresGuedez Mar 18, 2020
a3dd8bf
Enforce stricter validation of HTTP/2 custom settings parameters.
AndresGuedez Mar 18, 2020
e5b5e53
Fix format.
AndresGuedez Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion api/envoy/api/v2/core/protocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,18 @@ message Http1ProtocolOptions {
bool enable_trailers = 5;
}

// [#next-free-field: 13]
// [#next-free-field: 14]
message Http2ProtocolOptions {
// Defines a parameter to be sent in the SETTINGS frame.
// See `RFC7540, sec. 6.5.1 <https://tools.ietf.org/html/rfc7540#section-6.5.1>`_ for details.
message SettingsParameter {
// The 16 bit parameter identifier.
google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be required? <=, >=, etc. I don't think makes it required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, missed that. This is now required along with the second field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended for setting just the standardized options? Or experimental as well? It is unclear from the comment below. If just for standardized options should we make the identifier enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not just for standardized options but any new options (whether RFC bound or experimental).


// The 32 bit parameter value.
google.protobuf.UInt32Value value = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default is 0 and that is OK can we switch this to a basic type, otherwise can we describe the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now required.

}

// `Maximum table size <https://httpwg.org/specs/rfc7541.html#rfc.section.4.2>`_
// (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
Expand Down Expand Up @@ -216,6 +226,29 @@ message Http2ProtocolOptions {
//
// See `RFC7540, sec. 8.1 <https://tools.ietf.org/html/rfc7540#section-8.1>`_ for details.
bool stream_error_on_invalid_http_messaging = 12;

// Specifies SETTINGS frame parameters to be sent to the peer, with one exception:
// SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy.
//
// Note that custom parameters specified through this field may not collide with the following
// named parameters:
//
// .. code-block:: text
//
// ID Field Name
// ----------------
// 0x1 hpack_table_size
// 0x3 max_concurrent_streams
// 0x4 initial_stream_window_size
//
// Collisions will trigger config validation failure on load/update.
// The only exception is '0x8 allow_connect', for which the named parameter field will be
// overridden by the value set in this field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we deprecate that field to avoid the confusion in the next api rev?
If so let's update the docs to make this the default way to configure etc.

Also if we can consider it a failure to configure both I would - best to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the deprecation process to follow? I found https://github.com/envoyproxy/envoy/blob/master//CONTRIBUTING.md#breaking-change-policy, and given the level of effort I would prefer to introduce the deprecation in a follow up PR if possible to avoid holding this up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes +1 on let's do this in a follow up. I would like to land this PR and it's complex enough as it is. I agree with @alyssawilk that we should do this. Can you open an issue to track?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Filed #10412.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, I'm fine with a follow-up for deprecation but I'd be inclined to mention the plan in the API comments as well.
I'd also still suggest we disallow it being set both ways for the 2 years until we can remove the deprecated field, which I think should land with this PR unless it's super complicated. Thoughts there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, is your proposal that allow_connect should only be allowed to be configured via a custom parameter?

//
// See `IANA HTTP/2 Settings
// <https://www.iana.org/assignments/http2-parameters/http2-parameters.xhtml#settings>`_ for
// standardized identifiers.
repeated SettingsParameter custom_settings_parameters = 13;
}

// [#not-implemented-hide:]
Expand Down
38 changes: 37 additions & 1 deletion api/envoy/config/core/v3/protocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,24 @@ message Http1ProtocolOptions {
bool enable_trailers = 5;
}

// [#next-free-field: 13]
// [#next-free-field: 14]
message Http2ProtocolOptions {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.core.Http2ProtocolOptions";

// Defines a parameter to be sent in the SETTINGS frame.
// See `RFC7540, sec. 6.5.1 <https://tools.ietf.org/html/rfc7540#section-6.5.1>`_ for details.
message SettingsParameter {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.core.Http2ProtocolOptions.SettingsParameter";

// The 16 bit parameter identifier.
google.protobuf.UInt32Value identifier = 1 [(validate.rules).uint32 = {lte: 65536 gte: 1}];

// The 32 bit parameter value.
google.protobuf.UInt32Value value = 2;
}

// `Maximum table size <https://httpwg.org/specs/rfc7541.html#rfc.section.4.2>`_
// (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
Expand Down Expand Up @@ -235,6 +248,29 @@ message Http2ProtocolOptions {
//
// See `RFC7540, sec. 8.1 <https://tools.ietf.org/html/rfc7540#section-8.1>`_ for details.
bool stream_error_on_invalid_http_messaging = 12;

// Specifies SETTINGS frame parameters to be sent to the peer, with one exception:
// SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by Envoy.
//
// Note that custom parameters specified through this field may not collide with the following
// named parameters:
//
// .. code-block:: text
//
// ID Field Name
// ----------------
// 0x1 hpack_table_size
// 0x3 max_concurrent_streams
// 0x4 initial_stream_window_size
//
// Collisions will trigger config validation failure on load/update.
// The only exception is '0x8 allow_connect', for which the named parameter field will be
// overridden by the value set in this field.
//
// See `IANA HTTP/2 Settings
// <https://www.iana.org/assignments/http2-parameters/http2-parameters.xhtml#settings>`_ for
// standardized identifiers.
repeated SettingsParameter custom_settings_parameters = 13;
}

// [#not-implemented-hide:]
Expand Down
35 changes: 34 additions & 1 deletion generated_api_shadow/envoy/api/v2/core/protocol.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 37 additions & 1 deletion generated_api_shadow/envoy/config/core/v3/protocol.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

72 changes: 0 additions & 72 deletions include/envoy/http/codec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,78 +326,6 @@ struct Http1Settings {
HeaderKeyFormat header_key_format_{HeaderKeyFormat::Default};
};

/**
* HTTP/2 codec settings
*/
struct Http2Settings {
// TODO(jwfang): support other HTTP/2 settings
uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE};
uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS};
uint32_t initial_stream_window_size_{DEFAULT_INITIAL_STREAM_WINDOW_SIZE};
uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE};
bool allow_connect_{DEFAULT_ALLOW_CONNECT};
bool allow_metadata_{DEFAULT_ALLOW_METADATA};
bool stream_error_on_invalid_http_messaging_{DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING};
uint32_t max_outbound_frames_{DEFAULT_MAX_OUTBOUND_FRAMES};
uint32_t max_outbound_control_frames_{DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES};
uint32_t max_consecutive_inbound_frames_with_empty_payload_{
DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD};
uint32_t max_inbound_priority_frames_per_stream_{DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM};
uint32_t max_inbound_window_update_frames_per_data_frame_sent_{
DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT};

// disable HPACK compression
static const uint32_t MIN_HPACK_TABLE_SIZE = 0;
// initial value from HTTP/2 spec, same as NGHTTP2_DEFAULT_HEADER_TABLE_SIZE from nghttp2
static const uint32_t DEFAULT_HPACK_TABLE_SIZE = (1 << 12);
// no maximum from HTTP/2 spec, use unsigned 32-bit maximum
static const uint32_t MAX_HPACK_TABLE_SIZE = std::numeric_limits<uint32_t>::max();

// TODO(jwfang): make this 0, the HTTP/2 spec minimum
static const uint32_t MIN_MAX_CONCURRENT_STREAMS = 1;
// defaults to maximum, same as nghttp2
static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = (1U << 31) - 1;
// no maximum from HTTP/2 spec, total streams is unsigned 32-bit maximum,
// one-side (client/server) is half that, and we need to exclude stream 0.
// same as NGHTTP2_INITIAL_MAX_CONCURRENT_STREAMS from nghttp2
static const uint32_t MAX_MAX_CONCURRENT_STREAMS = (1U << 31) - 1;

// initial value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2
// NOTE: we only support increasing window size now, so this is also the minimum
// TODO(jwfang): make this 0 to support decrease window size
static const uint32_t MIN_INITIAL_STREAM_WINDOW_SIZE = (1 << 16) - 1;
// initial value from HTTP/2 spec is 65535, but we want more (256MiB)
static const uint32_t DEFAULT_INITIAL_STREAM_WINDOW_SIZE = 256 * 1024 * 1024;
// maximum from HTTP/2 spec, same as NGHTTP2_MAX_WINDOW_SIZE from nghttp2
static const uint32_t MAX_INITIAL_STREAM_WINDOW_SIZE = (1U << 31) - 1;

// CONNECTION_WINDOW_SIZE is similar to STREAM_WINDOW_SIZE, but for connection-level window
// TODO(jwfang): make this 0 to support decrease window size
static const uint32_t MIN_INITIAL_CONNECTION_WINDOW_SIZE = (1 << 16) - 1;
// nghttp2's default connection-level window equals to its stream-level,
// our default connection-level window also equals to our stream-level
static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024;
static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1;
// By default both nghttp2 and Envoy do not allow CONNECT over H2.
static const bool DEFAULT_ALLOW_CONNECT = false;
// By default Envoy does not allow METADATA support.
static const bool DEFAULT_ALLOW_METADATA = false;
// By default Envoy does not allow invalid headers.
static const bool DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING = false;

// Default limit on the number of outbound frames of all types.
static const uint32_t DEFAULT_MAX_OUTBOUND_FRAMES = 10000;
// Default limit on the number of outbound frames of types PING, SETTINGS and RST_STREAM.
static const uint32_t DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES = 1000;
// Default limit on the number of consecutive inbound frames with an empty payload
// and no end stream flag.
static const uint32_t DEFAULT_MAX_CONSECUTIVE_INBOUND_FRAMES_WITH_EMPTY_PAYLOAD = 1;
// Default limit on the number of inbound frames of type PRIORITY (per stream).
static const uint32_t DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM = 100;
// Default limit on the number of inbound frames of type WINDOW_UPDATE (per DATA frame sent).
static const uint32_t DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT = 10;
};

/**
* A connection (client or server) that owns multiple streams.
*/
Expand Down
7 changes: 4 additions & 3 deletions include/envoy/upstream/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,10 +720,11 @@ class ClusterInfo {
virtual const Http::Http1Settings& http1Settings() const PURE;

/**
* @return const Http::Http2Settings& for HTTP/2 connections created on behalf of this cluster.
* @see Http::Http2Settings.
* @return const envoy::config::core::v3::Http2ProtocolOptions& for HTTP/2 connections
* created on behalf of this cluster.
* @see envoy::config::core::v3::Http2ProtocolOptions.
*/
virtual const Http::Http2Settings& http2Settings() const PURE;
virtual const envoy::config::core::v3::Http2ProtocolOptions& http2Options() const PURE;

/**
* @param name std::string containing the well-known name of the extension for which protocol
Expand Down
1 change: 1 addition & 0 deletions source/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ envoy_cc_library(
external_deps = [
"abseil_optional",
"http_parser",
"nghttp2",
],
deps = [
":exception_lib",
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/codec_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
}
case Type::HTTP2: {
codec_ = std::make_unique<Http2::ClientConnectionImpl>(
*connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings(),
*connection_, *this, host->cluster().statsScope(), host->cluster().http2Options(),
Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount());
break;
}
Expand Down
6 changes: 3 additions & 3 deletions source/common/http/conn_manager_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection&
ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
Network::Connection& connection, const Buffer::Instance& data,
ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings,
const Http2Settings& http2_settings, uint32_t max_request_headers_kb,
uint32_t max_request_headers_count) {
const envoy::config::core::v3::Http2ProtocolOptions& http2_options,
uint32_t max_request_headers_kb, uint32_t max_request_headers_count) {
if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) {
return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
http2_settings, max_request_headers_kb,
http2_options, max_request_headers_kb,
max_request_headers_count);
} else {
return std::make_unique<Http1::ServerConnectionImpl>(connection, scope, callbacks,
Expand Down
3 changes: 2 additions & 1 deletion source/common/http/conn_manager_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class ConnectionManagerUtility {
static ServerConnectionPtr
autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data,
ServerConnectionCallbacks& callbacks, Stats::Scope& scope,
const Http1Settings& http1_settings, const Http2Settings& http2_settings,
const Http1Settings& http1_settings,
const envoy::config::core::v3::Http2ProtocolOptions& http2_options,
uint32_t max_request_headers_kb, uint32_t max_request_headers_count);

/**
Expand Down
Loading