http2: support custom SETTINGS parameters #9964

AndresGuedez · 2020-02-07T15:03:33Z

Add support for user defined H/2 SETTINGS parameters for both downstream and upstream connections.

Changes include refactoring to remove a now unnecessary layer of abstraction (Http::Http2Settings) and instead passing through the envoy::config::core::v3::Http2ProtocolOptions proto to all consumers.

Risk Level: Medium
Testing: New tests added.
Docs Changes: Proto docs.

The refactor removes a superfluous POD struct by storing and passing the underlying config proto directly to all consumers. This refactor enables a cleaner implementation of user defined settings parameters. Signed-off-by: Andres Guedez <aguedez@google.com>

Custom parameters override named parameters, enabling forward compatibility. Signed-off-by: Andres Guedez <aguedez@google.com>

Some minor cleanup as well. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only · 2020-02-07T15:03:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9964 was opened by AndresGuedez.

see: more, trace.

Signed-off-by: Andres Guedez <aguedez@google.com>

alyssawilk

Cool, nice to have this TODO todone :-)

Couple of thoughts which I think may provoke refactors - will do a test pass once they're done.

alyssawilk · 2020-02-11T13:54:23Z

source/common/http/http2/codec_impl.cc

+void ConnectionImpl::sendSettings(
+    const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) {
+  std::unordered_set<nghttp2_settings_entry, SettingsEntryHash, SettingsEntryEquals> settings;
+  // Custom parameters override named parameters, so they must be inserted first into the set.


If they override, should we either force-disable server push first, or reject that custom setting?

Alternately it might be worth considering it a config error to set a field twice instead of debug logging. WDYT?

If they override, should we either force-disable server push first, or reject that custom setting?

Thanks for catching this, it does require special handling given that it is disabled for server connections but enabled for client connections. Unfortunately, this creates awkwardness around error handling when an end user attempts to enable it via a custom parameter, as Envoy claims not to support server push but does end up sending the parameter to downstream peers (even though it never actually sends PUSH_PROMISE frames).

Do you have context around why it is enabled for downstream clients?

Alternately it might be worth considering it a config error to set a field twice instead of debug logging. WDYT?

The intent behind the current logic is to simplify forward compatibility with future named parameters. If an end user is setting a custom parameter (e.g., 0x20) that Envoy eventually ends up supporting as a named parameter, I would rather avoid breaking those configs when the user upgrades Envoy given that this new field addition would not be considered a breaking change to the API under current guidelines.

Also no context for why this is enabled for downstream.

@mattklein123 and @goaway any thoughts, or can we just fix this?

I understand the idea behind trying to support existing configs when an Envoy upgrade adopts a named parameter, but I wonder if that could lead to hard-to-detect configuration bugs across clients/severs. My first reaction is that I'm somewhat in favor of it just being a configuration error, to aid with earlier detection of a potential conflict.

Send what downstream? disable_push is false for server connections?

(But yes I think it was done that way just because of a common function, feel free to fix as needed)

source/common/http/http2/codec_impl.cc

Signed-off-by: Andres Guedez <aguedez@google.com>

Also, disallow custom SETTINGS parameters from overriding server push as it is not supported by Envoy. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez · 2020-02-14T21:51:01Z

Build failures appear to be due to #9668 which was merged after my branch was created. I will merge and resolve conflicts.

…ettings Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

Refactor required now that Http2Settings -> Http2Options proto. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

alyssawilk · 2020-02-25T20:03:58Z

Sorry for lag here - CI still looks unhappy. Yan you up for taking a look once that's sorted out?

AndresGuedez · 2020-02-25T21:35:56Z

Sorry for lag here - CI still looks unhappy. Yan you up for taking a look once that's sorted out?

AFAICT the CI failures are environmental:

2020/02/18 20:55:50 Downloading https://releases.bazel.build/2.0.0/release/bazel-2.0.0-linux-x86_64... 2020/02/18 20:56:00 could not download Bazel: HTTP GET https://releases.bazel.build/2.0.0/release/bazel-2.0.0-linux-x86_64 failed: Get ci/../bazel/setup_clang.sh: line 32: /clang.bazelrc: Permission denied

I'll rerun CI.

AndresGuedez · 2020-02-25T21:36:01Z

/retest

mattklein123

Thanks, fantastic work. Just a few small remaining comments.

/wait

api/envoy/api/v2/core/protocol.proto

mattklein123 · 2020-03-16T20:47:19Z

api/envoy/api/v2/core/protocol.proto

+  //
+  // 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.


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?

mattklein123 · 2020-03-16T20:53:16Z

source/common/http/http2/codec_impl.cc

+  const bool result =
+      insertParameter({NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()});
+  if (!result) {
+    ENVOY_CONN_LOG(warn,


This could log at high rate in the data path, right? I think we need to do this at debug level like above?

To clarify, are both this case and the above case all to handle the allow_connect case? Can't we detect/warn about all of this during config validation? I'm unclear why we need to do this again here?

Thanks, this was an oversight. Changed it to a debug log. The ideal solution would be to have a LOG_EVERY_N_SEC() macro that would allow me to log at warn level but rate limit it to avoid the perf hit and spamming the log (note to self: implement this).

The issue is that allow_connect can not be validated during config load since its presence can not be checked. To enable forward API compatibility, I only want to fail config load when both the named field and a user defined parameter with the same identifier are set.

The ideal solution would be to have a LOG_EVERY_N_SEC() macro that would allow me to log at warn level but rate limit it to avoid the perf hit and spamming the log (note to self: implement this).

FWIW this comes up about once per month, but no one has implemented it yet. :) cc @jmarantz

The issue is that allow_connect can not be validated during config load since its presence can not be checked. To enable forward API compatibility, I only want to fail config load when both the named field and a user defined parameter with the same identifier are set.

I'm confused about this. Why can't the presence be checked? Isn't it also config?

allow_connect has a primitive (bool) type, so it's not possible to differentiate between the default value and "set with the same value as the default". We need to use a wrapped type (google.protobuf.BoolValue) to be able to check presence.

Ah I see, sorry. Can you leave more of a comment trail around that in the data plane code also about the proto type? That would have helped me.

Right but if we can't check for presence we can still fail on conflicting values no?
It's annoying to have it set in 2 places. It's buggy (IMO) if those two places disagree.

+1 if we can do the disagreement check. If so IMO we can drop the debug logs in the data path?

SG; I'll make the change to fail config on conflicting values.

The validation logic has now been simplified. PTAL.

Signed-off-by: Andres Guedez <aguedez@google.com>

alyssawilk · 2020-03-18T15:38:07Z

api/envoy/api/v2/core/protocol.proto

+  // 1. SETTINGS_ENABLE_PUSH (0x2) is not configurable as HTTP/2 server push is not supported by
+  // Envoy.
+  //
+  // 2. SETTINGS_ENABLE_CONNECT_PROTOCOL (0x8) is only configurable through the named field


I was going to suggest "must be consistent" but I think we can deal with this when we get to the deprecation PR and just land this now.

My only concern is API stability issues - I don't think we can land this and change the behavior, and if we want to deprecate allow_connect we can't leave this as-is.

If we flag this "not implemented hide" until we do the deprecation PR (and hash out what long term behavior is optimal) can we land this as-is now and work out details over next week or two as the deprecation PR goes out? If so I'd suggest making it invisible and landing now.

@mattklein123

Yes I agree that for deprecation we will have to check consistency as previously discussed, but I agree with @alyssawilk that if we hide this new feature for right now we can land this until we figure it out. I have a change blocked behind this one so I would love to get this in and iterate if possible, so that plan SGTM.

/wait

OK, I've hidden the API.

mattklein123 · 2020-03-18T17:03:08Z

source/common/http/http2/codec_impl.cc

+    if (!result) {
+      ENVOY_CONN_LOG(debug, "duplicate custom settings parameter with id {:#x}, value {}",
+                     connection_, it.identifier().value(), it.value().value());
+      continue;


Sorry can this still happen? Don't you validate against this?

I wasn't validating for inconsistencies, but I've now added it.

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123

Thanks LGTM other than format issue. Thank you!

/wait

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez · 2020-03-19T00:46:35Z

Thanks LGTM other than format issue. Thank you!

/wait

Done. Thanks!

mattklein123

Thanks!

mattklein123 · 2020-03-19T18:27:23Z

I'm going to go ahead and merge this so I can continue with my other PR and we can follow up with any additional @alyssawilk comments.

AndresGuedez added 6 commits January 31, 2020 14:48

Support custom H/2 settings parameters.

28cdf4d

Custom parameters override named parameters, enabling forward compatibility. Signed-off-by: Andres Guedez <aguedez@google.com>

Add Http2ProtocolOptions cluster config test.

bad389c

Some minor cleanup as well. Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

4b78a16

Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

0f50401

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix comment.

16b8ba7

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez requested review from alyssawilk and mattklein123 as code owners February 7, 2020 15:03

repokitteh-read-only bot added the api label Feb 7, 2020

mattklein123 assigned alyssawilk Feb 7, 2020

Fix format.

08ab741

Signed-off-by: Andres Guedez <aguedez@google.com>

alyssawilk reviewed Feb 11, 2020

View reviewed changes

alyssawilk added the waiting label Feb 11, 2020

yanavlasov self-assigned this Feb 12, 2020

AndresGuedez added 5 commits February 13, 2020 13:03

Fix docs.

b553ef5

Signed-off-by: Andres Guedez <aguedez@google.com>

Refactor SETTINGS frame received callback handling.

fdc7c9b

Also, disallow custom SETTINGS parameters from overriding server push as it is not supported by Envoy. Signed-off-by: Andres Guedez <aguedez@google.com>

Fix format.

cc5fe09

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix format.

6def08e

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix spelling.

1cf9c32

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label Feb 13, 2020

Remove test debug logs.

6bfbfc6

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez added 4 commits February 18, 2020 10:34

Merge remote-tracking branch 'upstream/master' into h2-user-defined-s…

5632a90

…ettings Signed-off-by: Andres Guedez <aguedez@google.com>

Regenerate API shadow.

b764d34

Signed-off-by: Andres Guedez <aguedez@google.com>

Refactor newly introduced references to Http2Settings.

9455741

Refactor required now that Http2Settings -> Http2Options proto. Signed-off-by: Andres Guedez <aguedez@google.com>

Adjust expected memory usage for cluster memory tests.

78d3e63

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 requested changes Mar 16, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 16, 2020

AndresGuedez mentioned this pull request Mar 17, 2020

Deprecate Http2ProtocolOptions.allow_connect #10412

Open

Change warning to debug log.

53bf245

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label Mar 17, 2020

mattklein123 added the waiting label Mar 17, 2020

AndresGuedez added 2 commits March 18, 2020 09:06

Simplify H/2 custom settings parameters validation logic.

cf8f7f2

Signed-off-by: Andres Guedez <aguedez@google.com>

Change comment wording.

ad297e2

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label Mar 18, 2020

Minor cleaup.

6ae55cd

Signed-off-by: Andres Guedez <aguedez@google.com>

alyssawilk reviewed Mar 18, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 18, 2020

mattklein123 reviewed Mar 18, 2020

View reviewed changes

Enforce stricter validation of HTTP/2 custom settings parameters.

a3dd8bf

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label Mar 18, 2020

mattklein123 requested changes Mar 18, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 18, 2020

Fix format.

e5b5e53

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label Mar 19, 2020

mattklein123 approved these changes Mar 19, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Mar 19, 2020

mattklein123 merged commit 56e5b84 into envoyproxy:master Mar 19, 2020

AndresGuedez deleted the h2-user-defined-settings branch March 20, 2020 15:04

sunjayBhatia mentioned this pull request Mar 26, 2020

Windows compilation: enable compiling expanded list of extensions in envoy-static #10542

Merged

wrowe mentioned this pull request Apr 17, 2020

Fix Windows compilation of test sources #10822

Merged

ramaraochavali mentioned this pull request Jul 23, 2022

unhide custom settings option in protocol options #22376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: support custom SETTINGS parameters #9964

http2: support custom SETTINGS parameters #9964

AndresGuedez commented Feb 7, 2020

repokitteh-read-only bot commented Feb 7, 2020

alyssawilk left a comment

alyssawilk Feb 11, 2020

AndresGuedez Feb 12, 2020

alyssawilk Feb 25, 2020

goaway Feb 25, 2020

mattklein123 Feb 26, 2020

mattklein123 Feb 26, 2020

AndresGuedez commented Feb 14, 2020

alyssawilk commented Feb 25, 2020

AndresGuedez commented Feb 25, 2020

AndresGuedez commented Feb 25, 2020

mattklein123 left a comment

mattklein123 Mar 16, 2020

mattklein123 Mar 16, 2020

AndresGuedez Mar 17, 2020

mattklein123 Mar 17, 2020

AndresGuedez Mar 17, 2020

mattklein123 Mar 17, 2020

AndresGuedez Mar 17, 2020

alyssawilk Mar 17, 2020

mattklein123 Mar 17, 2020

AndresGuedez Mar 17, 2020

AndresGuedez Mar 18, 2020

alyssawilk Mar 18, 2020

mattklein123 Mar 18, 2020

AndresGuedez Mar 18, 2020

mattklein123 Mar 18, 2020

AndresGuedez Mar 18, 2020

mattklein123 left a comment

AndresGuedez commented Mar 19, 2020

mattklein123 left a comment

mattklein123 commented Mar 19, 2020

http2: support custom SETTINGS parameters #9964

http2: support custom SETTINGS parameters #9964

Conversation

AndresGuedez commented Feb 7, 2020

repokitteh-read-only bot commented Feb 7, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresGuedez commented Feb 14, 2020

alyssawilk commented Feb 25, 2020

AndresGuedez commented Feb 25, 2020

AndresGuedez commented Feb 25, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

AndresGuedez commented Mar 19, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Mar 19, 2020