grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager #16385

RyanTheOptimist · 2021-05-07T19:57:02Z

grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager.

Create a new AlternateProtocolsCacheOptions proto message for configuring an AlternateProtocolCache, and add this message to AutoHttpConfig.
Create a new AlternateProtocolCacheManager class for fetching/creating a Cache based on the config.
Modify the AlternateProtocolCache to store state per thread. (Does not yet synchronize this state across threads)

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

…nfig AlternateProtocolsCaches Signed-off-by: Ryan Hamilton <[email protected]>

Signed-off-by: Ryan Hamilton <[email protected]>

repokitteh-read-only · 2021-05-07T19:57:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16385 was opened by RyanTheOptimist.

see: more, trace.

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2021-05-07T19:57:47Z

/assign @DavidSchinazi

RyanTheOptimist · 2021-05-07T19:58:42Z

@DavidSchinazi I need to discuss with Alyssa about how best to integration test this, but if you could do a first pass in the meantime, I'd appreciate it.

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi

Nice! Wow that was a lot of plumbing

api/envoy/config/core/v3/protocol.proto

DavidSchinazi · 2021-05-07T20:27:35Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


I'm having a hard time parsing this comment. What are you trying to say?

Right.. So if you provide alternate protocols cache options and you enable HTTP/3, you won't get HTTP/3. You also need an entry in the cache which says that the particular server advertises HTTP/3. I'd love a recommendation for better phrasing.

I agree that the comment here is hard to digest.
IIUC this field is not specific to HTTP/3, but the comment doesn't describe what's its purpose, and how it impacts the other protocols.
Also, not an expert here, but can the same cache in this case be for both HTTP/1.1 and HTTP/2 (or is it a different cache with a different configuration)?
If the cache isn't shared across the protocols, would it make sense to have this message inside each of the different Protocol-Options?

How about having the comment focus on the main logic, and only noting the caveat. For example: The presence of alternate protocols cache options causes the use of the alternate protocols cache, which is responsible for parsing and caching HTTP Alt-Svc headers. This enables the use of HTTP/3 for origins that advertise supporting it. Note that currently when the alternate protocols cache is absent we will always attempt HTTP/3 if HTTP/3 is enabled, but that behavior will be removed soon.

I thought if HTTP/3 was present we were going to require the cache to be present? Even if we TODO enforcement if that's the plan I think we can go with it, and comment that this will become required

Yes, I think the plan is that the cache be required when HTTP/3 is present in the auto pool. With this PR, of course, the cache is still optional. Also with this PR, the cache is not being populated by response headers. (That will come subsequently.) So if we want to document behavior in this PR, we should probably not talk about header parsing but would want to talk about missing cache behavior. If we want to document intended behavior, we should probably not talk about missing cache behavior but should probably talk about header parsing.

What do you both think?

That sounds reasonable to me

Let's doc up what the actual plan is, since it's hidden so the APIs won't be turned into docs.
You can TODO here or in code to enforce the bits which aren't done yet.

Sounds good to me! Done! (I hope :>) I took David's language and tweaked it slightly.

DavidSchinazi · 2021-05-07T20:29:35Z

generated_api_shadow/envoy/config/core/v3/protocol.proto

@@ -71,6 +71,24 @@ message UpstreamHttpProtocolOptions {
  bool auto_san_validation = 2;
 }

+message AlternateProtocolsCacheOptions {


(This comment not specific to your PR, just curiosity.) Are the protos in generated_api_shadow automatically generated? If yes do you know why they're in the repository as opposed to generated at build time?

Oh, I missed this one. They're generated by running:

./tools/proto_format/proto_format.sh fix

I don't understand why that doesn't happen at build time, but it is SUPER slow so that's probably part of it :/

include/envoy/upstream/cluster_manager.h

source/common/http/alternate_protocols_cache.h

source/common/http/alternate_protocols_cache_impl.cc

source/common/http/alternate_protocols_cache_manager_impl.h

source/common/http/conn_pool_grid.h

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist

Thanks!

api/envoy/config/core/v3/protocol.proto

RyanTheOptimist · 2021-05-07T20:59:11Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


Right.. So if you provide alternate protocols cache options and you enable HTTP/3, you won't get HTTP/3. You also need an entry in the cache which says that the particular server advertises HTTP/3. I'd love a recommendation for better phrasing.

include/envoy/upstream/cluster_manager.h

source/common/http/alternate_protocols_cache.h

source/common/http/alternate_protocols_cache_impl.h

source/common/http/alternate_protocols_cache_manager_impl.h

source/common/http/conn_pool_grid.h

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2021-05-08T14:49:56Z

/retest

repokitteh-read-only · 2021-05-08T14:50:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16385 (comment) was created by @RyanTheOptimist.

see: more, trace.

adisuissa

Thanks for working on this.
Added an API question here.

adisuissa · 2021-05-10T14:31:46Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


I agree that the comment here is hard to digest.
IIUC this field is not specific to HTTP/3, but the comment doesn't describe what's its purpose, and how it impacts the other protocols.
Also, not an expert here, but can the same cache in this case be for both HTTP/1.1 and HTTP/2 (or is it a different cache with a different configuration)?
If the cache isn't shared across the protocols, would it make sense to have this message inside each of the different Protocol-Options?

DavidSchinazi

Thanks!

alyssawilk

Early drive by since you haven't assigned me but I saw David's approval :-P

alyssawilk · 2021-05-10T17:01:19Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


I thought if HTTP/3 was present we were going to require the cache to be present? Even if we TODO enforcement if that's the plan I think we can go with it, and comment that this will become required

include/envoy/upstream/cluster_manager.h

source/common/http/alternate_protocols_cache.h

source/common/http/alternate_protocols_cache_impl.cc

source/common/http/alternate_protocols_cache_impl.h

source/common/http/conn_pool_grid.h

alyssawilk · 2021-05-10T17:39:39Z

api/envoy/config/core/v3/protocol.proto

+  //   The implementation is approximate and enforced independently on each worker thread, thus
+  //   it is possible for the maximum hosts in the cache to go slightly above the configured
+  //   value depending on timing. This is similar to how other circuit breakers work.
+  google.protobuf.UInt32Value max_hosts = 2 [(validate.rules).uint32 = {gt: 0}];


I don't see this as used yet, or a TODO to use the field. is it functionally the same TODO "Add a limit to the size of this map and evict based on usage" and added in this PR so you can test config conflicts?
If so the map is from Origin and I wonder if max_entries would be a bit more consistent than max_hosts?

Yes, this is the same as the TODO. Changed to max_entries. Good idea.

api/envoy/config/core/v3/protocol.proto

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi · 2021-05-11T14:25:59Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


That sounds reasonable to me

alyssawilk

Thanks for that other PR - the changes here are way easier to see now!

api/envoy/config/core/v3/protocol.proto

alyssawilk · 2021-05-11T14:36:06Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+
+    // [#not-implemented-hide:]
+    // If HTTP/3 is present and an alternate protocol cache is present, attempts to connect
+    // will be via HTTP/3 only if the alternate protocol cache specifies an HTTP/3


Let's doc up what the actual plan is, since it's hidden so the APIs won't be turned into docs.
You can TODO here or in code to enforce the bits which aren't done yet.

source/common/http/alternate_protocols_cache_impl.cc

alyssawilk · 2021-05-11T14:42:37Z

source/common/http/alternate_protocols_cache_manager_impl.cc

+  if (existing_cache != caches_.end()) {
+    if (!Protobuf::util::MessageDifferencer::Equivalent(options, existing_cache->second.options_)) {
+      throw EnvoyException(
+          fmt::format("options specified alternate protocols cache '{}' with different settings",


worth logging both sets of options since we have them?

include/envoy/http/alternate_protocols_cache.h

alyssawilk · 2021-05-11T14:49:20Z

test/common/http/alternate_protocols_cache_manager_test.cc

@@ -0,0 +1,58 @@
+#include "common/http/alternate_protocols_cache_manager_impl.h"


we can do end to end tests with this PR too, no?

Meaning we can test that HTTP/3 works via the AlternateProtocolsCache? I think though though I'd love to spend some time discussing how to do this in an integration test. I looked at what I think are the HTTP/3 multiplexed integrations tests and struggled to understand quite how to wire it up.

So you found the tests (yay!) but I agree the way config happens is pretty opaque.
The important thing is that "use_alpn" in the set up here, which gets passed to ConfigHelper::configureUpstreamTls()

Since we plan on making it required, I'd be nice to add default alpn cache by default, and tweak the existing 2 tests using the grid but the tweaking is going to be the tricky bit.

right now, we create exactly one fake upstream, either http2 or http3. now that we'll have alt-svc I guess we'd configure the HTTP/2 upstream as the upstream of choice, and it'll alt-svc to the http3 upstream.

I think you can decompose what happens in MixedUpstreamIntegrationTest::createUpstreams which currently creates either h2 or h3 to create h2 then h3 (just skip the base class call and do 'em manually I think, and create http3 as well unless the test specifies testing http3 broken)

the changes to do alt-svc, and wait for h3 to be established may get complicated, so happy to help out there

Discussed offline... Since we don't yet have anything reading alt-svc headers, we can't really do a bona-fide end-to-end test. Once we wire up alt-svc header processing, we'll do that.

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk

yay docs! Thanks for kicking those off :-)

source/docs/http3_upstream.md

alyssawilk · 2021-05-11T19:52:35Z

api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

+    // HTTP Alt-Svc headers. This enables the use of HTTP/3 for origins that
+    // advertise supporting it.
+    // TODO(RyanTheOptimist): Make this field required when HTTP/3 is enabled.
+    config.core.v3.AlternateProtocolsCacheOptions alternate_protocols_cache_options = 4;


here or message definition link to spec?

include/envoy/http/alternate_protocols_cache.h

source/common/http/alternate_protocols_cache_impl.h

alyssawilk · 2021-05-11T19:55:10Z

source/common/http/alternate_protocols_cache_manager_impl.cc

+    if (!Protobuf::util::MessageDifferencer::Equivalent(options, existing_cache->second.options_)) {
+      throw EnvoyException(fmt::format(
+          "options specified alternate protocols cache '{}' with different settings"
+          " old '{}' new '{}'",


on second thought, old/new -> first/second? otherwise it maybe looks like a config reload. Sorry for not thinking of that up front.

Sure. Done!

alyssawilk · 2021-05-11T19:59:12Z

source/docs/http3_upstream.md

+
+#### Connectivity Grid
+The `ConnectivityGrid` is a `ConnectionPool` which wraps a `Http3ConnPoolImpl` connection pool
+(QUIC) and a `HttpConnPoolImplMixed` connection pool (TCP). If HTTP/3 is currently broken, stream


Oh, you have much of that here. Maybe again move it up top and then just call out the grid handles which of TCP/UDP to try etc.?

I'm inclined to keep this text here as it has a bit more technical detail than what I added about, but happy to nuke / rephrase if you think that would be better?

alyssawilk · 2021-05-11T20:00:27Z

source/docs/http3_upstream.md

+
+### Required configuration
+
+Specific Envoy configuration is required in order to enable the previous. components


I think we can call this out in the top overview section

alyssawilk · 2021-05-11T20:40:50Z

test/common/http/alternate_protocols_cache_manager_test.cc

@@ -0,0 +1,58 @@
+#include "common/http/alternate_protocols_cache_manager_impl.h"


So you found the tests (yay!) but I agree the way config happens is pretty opaque.
The important thing is that "use_alpn" in the set up here, which gets passed to ConfigHelper::configureUpstreamTls()

Since we plan on making it required, I'd be nice to add default alpn cache by default, and tweak the existing 2 tests using the grid but the tweaking is going to be the tricky bit.

right now, we create exactly one fake upstream, either http2 or http3. now that we'll have alt-svc I guess we'd configure the HTTP/2 upstream as the upstream of choice, and it'll alt-svc to the http3 upstream.

I think you can decompose what happens in MixedUpstreamIntegrationTest::createUpstreams which currently creates either h2 or h3 to create h2 then h3 (just skip the base class call and do 'em manually I think, and create http3 as well unless the test specifies testing http3 broken)

the changes to do alt-svc, and wait for h3 to be established may get complicated, so happy to help out there

api/envoy/config/core/v3/protocol.proto

Signed-off-by: Ryan Hamilton <[email protected]>

adisuissa · 2021-05-13T04:06:21Z

/lgtm api

alyssawilk · 2021-05-13T12:42:48Z

I'm back to under the weather and so I'm taking today off.
this is in pretty good shape - @snowp can you start on the !Google pass?

snowp

Some minor comments but otherwise this looks good!

snowp · 2021-05-14T15:01:17Z

source/common/upstream/BUILD

@@ -56,6 +56,7 @@ envoy_cc_library(
        ":subset_lb_lib",
        "//include/envoy/api:api_interface",
        "//include/envoy/event:dispatcher_interface",
+        "@envoy_api//envoy/extensions/clusters/dynamic_forward_proxy/v3:pkg_cc_proto",


This seems a bit odd, is this supposed to get added?

Oh, good point. Removed.

snowp · 2021-05-14T20:28:25Z

source/docs/http3_upstream.md

+
+### Required configuration
+
+Specific Envoy configuration is required in order to enable the previous. components


This sentence seems a bit off, is the period misplaced?

Gah. Yes, I think so. Cleaned up.

snowp · 2021-05-14T20:32:18Z

test/common/http/alternate_protocols_cache_manager_test.cc

+  try {
+    AlternateProtocolsCacheSharedPtr cache2 = manager_->getCache(options2_);
+    FAIL();
+  } catch (const EnvoyException& e) {
+    EXPECT_THAT(e.what(),
+                testing::HasSubstr(
+                    "options specified alternate protocols cache 'name1' with different settings"));
+  }


There are some helpers for this, EXPECT_THROW_* et al

Oh, awesome! Much simpler. Thanks!

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist

Thanks for the comments!

RyanTheOptimist · 2021-05-14T21:01:20Z

source/common/upstream/BUILD

@@ -56,6 +56,7 @@ envoy_cc_library(
        ":subset_lb_lib",
        "//include/envoy/api:api_interface",
        "//include/envoy/event:dispatcher_interface",
+        "@envoy_api//envoy/extensions/clusters/dynamic_forward_proxy/v3:pkg_cc_proto",


Oh, good point. Removed.

RyanTheOptimist · 2021-05-14T21:02:28Z

source/docs/http3_upstream.md

+
+### Required configuration
+
+Specific Envoy configuration is required in order to enable the previous. components


Gah. Yes, I think so. Cleaned up.

RyanTheOptimist · 2021-05-14T21:06:34Z

test/common/http/alternate_protocols_cache_manager_test.cc

+  try {
+    AlternateProtocolsCacheSharedPtr cache2 = manager_->getCache(options2_);
+    FAIL();
+  } catch (const EnvoyException& e) {
+    EXPECT_THAT(e.what(),
+                testing::HasSubstr(
+                    "options specified alternate protocols cache 'name1' with different settings"));
+  }


Oh, awesome! Much simpler. Thanks!

alyssawilk

LGTM modulo two docs nits!

alyssawilk · 2021-05-17T13:56:42Z

source/docs/http3_upstream.md

@@ -1,7 +1,17 @@
 ### Overview

 Support for Upstream HTTP/3 connections is slightly more complex than for HTTP/1 or HTTP/2
-and this document attempts to describe it.
+and requires specific configurations to enable it. HTTP/3 is only attempted to servers which


If HTTP/3 is configured in auto_config, HTTP/3 connections will only be attempted

Since it is force-attempted for explicit_http_config and "do what downstream did"

Good point. Done.

alyssawilk · 2021-05-17T13:57:25Z

source/docs/http3_upstream.md

+If no such advertisement exists, then HTTP/2 or HTTP/1 will be used instead. Further,
+HTTP/3 runs over QUIC (which uses UDP) and not over TCP (which HTTP/1 and HTTP/2 use).
+It is not uncommon for network devices to block UDP traffic, and hence block HTTP/3. This
+means that Upstream HTTP/3 connection attempts might be blocked by the network and should fall


upstream here and below

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk · 2021-05-17T14:33:56Z

source/docs/http3_upstream.md

-means that Upstream HTTP/3 connection attempts might be blocked by the network and should fall
-back to using HTTP/2. On such networks, the Upstream connection code needs to
+means that upstream HTTP/3 connection attempts might be blocked by the network and should fall
+back to using HTTP/2. On such networks, the upstream connection code needs to
 track that HTTP/3 connects attempts are not succeeding and avoid making connections
 which are doomed to fail. On networks where HTTP/3 is working correctly, however, the
 Upstream connection code should avoid attempting HTTP/2 unnecessarily.


Upstream -> upstream :-P

Right! (facepalm)

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist · 2021-05-18T01:29:31Z

/retest

repokitteh-read-only · 2021-05-18T01:29:35Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16385 (comment) was created by @RyanTheOptimist.

see: more, trace.

alyssawilk · 2021-05-18T12:55:29Z

@snowp any other comments or am I good to merge?

snowp

LGTM!

…reamClusterManager (envoyproxy#16385) grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager. Create a new AlternateProtocolsCacheOptions proto message for configuring an AlternateProtocolCache, and add this message to AutoHttpConfig. Create a new AlternateProtocolCacheManager class for fetching/creating a Cache based on the config. Modify the AlternateProtocolCache to store state per thread. (Does not yet synchronize this state across threads) Risk Level: Low Testing: Unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Ryan Hamilton <[email protected]> Signed-off-by: Sixiang Gu <[email protected]>

…reamClusterManager (envoyproxy#16385) grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager. Create a new AlternateProtocolsCacheOptions proto message for configuring an AlternateProtocolCache, and add this message to AutoHttpConfig. Create a new AlternateProtocolCacheManager class for fetching/creating a Cache based on the config. Modify the AlternateProtocolCache to store state per thread. (Does not yet synchronize this state across threads) Risk Level: Low Testing: Unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist added 5 commits May 6, 2021 20:33

grid: Create a new AlternateProtocolsCacheManager for creating per-co…

cb0e75d

…nfig AlternateProtocolsCaches Signed-off-by: Ryan Hamilton <[email protected]>

Proto comment

93dda2e

Signed-off-by: Ryan Hamilton <[email protected]>

More

44252b4

Signed-off-by: Ryan Hamilton <[email protected]>

More tests

6f2c78c

Signed-off-by: Ryan Hamilton <[email protected]>

Cleanup

b7dbe02

Signed-off-by: Ryan Hamilton <[email protected]>

repokitteh-read-only bot added the api label May 7, 2021

repokitteh-read-only bot assigned adisuissa May 7, 2021

comment

f0bcc9b

Signed-off-by: Ryan Hamilton <[email protected]>

repokitteh-read-only bot assigned DavidSchinazi May 7, 2021

AlternateProtocolsCacheManagerTest

a33ccf3

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi reviewed May 7, 2021

View reviewed changes

RyanTheOptimist added 3 commits May 7, 2021 20:55

formta

1276126

Signed-off-by: Ryan Hamilton <[email protected]>

Address comments from David

7c70aca

Signed-off-by: Ryan Hamilton <[email protected]>

Format

7617e40

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist commented May 7, 2021

View reviewed changes

RyanTheOptimist added 2 commits May 7, 2021 23:43

Fix upstream test

ffe4f9e

Signed-off-by: Ryan Hamilton <[email protected]>

Tidy

7b8baeb

Signed-off-by: Ryan Hamilton <[email protected]>

adisuissa reviewed May 10, 2021

View reviewed changes

DavidSchinazi previously approved these changes May 10, 2021

View reviewed changes

alyssawilk reviewed May 10, 2021

View reviewed changes

RyanTheOptimist mentioned this pull request May 10, 2021

grid: Rename alternate_protocols_cache.{h,cc} to alternate_protocols_cache_impl.{h,cc} #16424

Merged

Merge branch 'main' into AlternateProtocolsCache

f8e141e

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist dismissed DavidSchinazi’s stale review via f8e141e May 11, 2021 02:53

RyanTheOptimist added 2 commits May 11, 2021 04:01

Comments

2c20b92

Signed-off-by: Ryan Hamilton <[email protected]>

Comment cleanup

2054b07

Signed-off-by: Ryan Hamilton <[email protected]>

DavidSchinazi previously approved these changes May 11, 2021

View reviewed changes

alyssawilk reviewed May 11, 2021

View reviewed changes

RyanTheOptimist added 2 commits May 11, 2021 17:46

Comments from Alyssa

c8956e7

Signed-off-by: Ryan Hamilton <[email protected]>

Format

58581ba

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist dismissed DavidSchinazi’s stale review via 58581ba May 11, 2021 17:52

alyssawilk reviewed May 11, 2021

View reviewed changes

adisuissa reviewed May 12, 2021

View reviewed changes

api/envoy/config/core/v3/protocol.proto Outdated Show resolved Hide resolved

RyanTheOptimist added 2 commits May 12, 2021 20:18

Fix thread local usage, and address comments

b935875

Signed-off-by: Ryan Hamilton <[email protected]>

Cleanup

c4e9fac

Signed-off-by: Ryan Hamilton <[email protected]>

repokitteh-read-only bot removed the api label May 13, 2021

alyssawilk assigned snowp May 13, 2021

snowp suggested changes May 14, 2021

View reviewed changes

RyanTheOptimist added 2 commits May 14, 2021 21:06

Address comments from snowp

9dcd1b2

Signed-off-by: Ryan Hamilton <[email protected]>

Format

449407b

Signed-off-by: Ryan Hamilton <[email protected]>

RyanTheOptimist commented May 14, 2021

View reviewed changes

alyssawilk reviewed May 17, 2021

View reviewed changes

Address comments from alyssa

1391373

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk reviewed May 17, 2021

View reviewed changes

Upstream

6ca0e9a

Signed-off-by: Ryan Hamilton <[email protected]>

alyssawilk approved these changes May 17, 2021

View reviewed changes

snowp approved these changes May 18, 2021

View reviewed changes

alyssawilk merged commit 964de6c into envoyproxy:main May 18, 2021

		@@ -0,0 +1,58 @@
		#include "common/http/alternate_protocols_cache_manager_impl.h"


		### Required configuration

		Specific Envoy configuration is required in order to enable the previous. components

grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager #16385

grid: Plumb the AlternateProtocolCache down to the grid from the UpstreamClusterManager #16385

Conversation

RyanTheOptimist commented May 7, 2021

repokitteh-read-only bot commented May 7, 2021

RyanTheOptimist commented May 7, 2021

RyanTheOptimist commented May 7, 2021

DavidSchinazi 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

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented May 8, 2021

repokitteh-read-only bot commented May 8, 2021

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidSchinazi left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa commented May 13, 2021

alyssawilk commented May 13, 2021

snowp 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

RyanTheOptimist 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

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

RyanTheOptimist commented May 18, 2021