cluster_manager: Add support for USE_DOWNSTREAM_PROTOCOL. by jrajahalme · Pull Request #2328 · envoyproxy/envoy

jrajahalme · 2018-01-09T01:43:45Z

Currently clusters can not open both HTTP1.1 and HTTP2 upstream
connections at the same time. When the new cluster option
"protocol_selection" is set to "USE_DOWNSTREAM_PROTOCOL", the cluster
must open an HTTP2 upstream connection if the downstream connection is
HTTP2, and an HTTP1.1 upstream connection if the downstream connection
is HTTP1.1. This option is to have no effect if there is no corresponding
downstream connection.

This functionality removes the need to operate multiple clusters and
routing rules for them when the backends accept both HTTP1.1 and HTTP2
connections, and when the choice of the HTTP protocol is significant,
as with gRPC.

Signed-off-by: Jarno Rajahalme jarno@covalent.io

Risk Level: Medium
API Changes: envoyproxy/data-plane-api#399

Configuration options for different protocols are no longer mutually exclusive in the v2 API. Backwards compatibility is maintained by requiring that only one of HTTP/1 or HTTP/2 options be present when the new cluster option 'protocol_selection' is in it's default value ('EXCLUSIVE_AS_CONFIGURED'). Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

We already test for nullptr values of container pools elsewhere, make sense to do it here as well. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

htuch · 2018-01-09T15:07:01Z

source/common/config/cds_json.cc

  }

+  if (json_cluster.getString("features", "") == "use_downstream_protocol") {
+    cluster.set_protocol_selection(envoy::api::v2::Cluster::USE_DOWNSTREAM_PROTOCOL);


Can we avoid putting this in v1? We're trying to restrict new features to v2 only to encourage migration, unless there is a direct need for someone to have it in v1.

jrajahalme · 2018-01-09T17:38:16Z

On Jan 9, 2018, at 7:08 AM, htuch ***@***.***> wrote: @htuch commented on this pull request. In source/common/config/cds_json.cc <#2328 (comment)>: > @@ -181,7 +181,12 @@ void CdsJson::translateCluster(const Json::Object& json_cluster, *cluster.mutable_tls_context()); } + if (json_cluster.getString("features", "") == "use_downstream_protocol") { + cluster.set_protocol_selection(envoy::api::v2::Cluster::USE_DOWNSTREAM_PROTOCOL); Can we avoid putting this in v1? We're trying to restrict new features to v2 only to encourage migration, unless there is a direct need for someone to have it in v1.

Sure, will remove. Jarno

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Suggested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Add tests to check that the 'protocol' parameter to httpConnPoolForCluster() is passed correctly with and without the new cluster USE_DOWNSTREAM_PROTOCOL feature. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

LGTM, just a few nits. Can finalize once data-plane-api PR gets merged. Thank you for the detailed tests.

mattklein123 · 2018-01-10T16:26:04Z

include/envoy/upstream/cluster_manager.h

   */
  virtual Http::ConnectionPool::Instance* httpConnPoolForCluster(const std::string& cluster,
                                                                 ResourcePriority priority,
+                                                                 enum Http::Protocol protocol,


nit: C++ does not require enum. Please remove. Same elsewhere.

mattklein123 · 2018-01-10T16:26:28Z

include/envoy/upstream/upstream.h

    static const uint64_t HTTP2 = 0x1;
+    // Use the downstream protocol (HTTP1.1, HTTP2) for upstream connections as well, if available.
+    // This is used when creating connection pools.
+    static const uint64_t USE_DOWNSTREAM_PROTOCOL = 0x2;


note: Needs name update per @wora name change in data-plane-api PR.

mattklein123 · 2018-01-10T16:27:00Z

source/common/router/router.cc

+  ENVOY_STREAM_LOG(debug, "getConnPool cluster features: {:x}", *callbacks_, features);
+
+  const Optional<Http::Protocol>& downstream_protocol = callbacks_->requestInfo().protocol();
+  enum Http::Protocol protocol =


nit: This multiple tertiary operator is a bit of an eyeful. Can you break apart?

Refactored.

mattklein123 · 2018-01-10T16:28:34Z

source/common/upstream/upstream_impl.cc

  }
+
+  if (config.protocol_selection() == envoy::api::v2::Cluster::EXCLUSIVE_AS_CONFIGURED) {
+    // Make sure multiple protocol configurations are not present


nit: period end of sentence

Suggested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-10T21:45:31Z

mac test failed due to timeout, don't think it is related to this PR:

//test/integration:http2_integration_test                               TIMEOUT in 316.1s
  /private/var/tmp/_bazel_distiller/bedaa68a8664d1b29e96b826d058247f/execroot/envoy/bazel-out/darwin-fastbuild/testlogs/test/integration/http2_integration_test/test.log

mattklein123

LGTM after few nits. Please merge master also. Thank you!

mattklein123 · 2018-01-11T02:29:00Z

source/common/router/router.cc

+                                : Http::Protocol::Http11;
+  if (features & Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL) {
+    const Optional<Http::Protocol>& downstream_protocol = callbacks_->requestInfo().protocol();
+    if (downstream_protocol.valid()) {


IIRC protocol must be valid here. I think you can skip the if check.

Taking your word on this :-)

mattklein123 · 2018-01-11T02:29:23Z

source/common/upstream/cluster_manager_impl.h

                                                     HostConstSharedPtr host,
-                                                     ResourcePriority priority) override;
+                                                     ResourcePriority priority,
+                                                     enum Http::Protocol protocol) override;


nit: missed enum here, same below.

Removed extra enums here and elsewhere.

In this context Optional protocol is always valid. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

cool, thanks!

jrajahalme · 2018-01-11T19:46:58Z

On Jan 11, 2018, at 11:34 AM, Matt Klein ***@***.***> wrote: Merged #2328 <#2328>.

Thanks!

Adds an RTDS integration test for Envoy Mobile This test does a simple verification that the RTDS protocol within the xDS family is working correctly. A subsequent PR will improve upon the test coverage. As part of this change, the common parts of running a Envoy Mobile client integration test were refactored out into a new BaseClientIntegrationTest class. Both ClientIntegrationTest and RtdsIntegrationTest inherit from BaseClientIntegrationTest. NB: the parameterized test currently fails when using IPv6. This will be debugged and fixed in a subsequent PR. Signed-off-by: Ali Beyad <abeyad@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

jrajahalme added 3 commits January 8, 2018 17:37

cluster_manager_impl: Do not call deferredDelete() on nullptr.

ac6a6e5

We already test for nullptr values of container pools elsewhere, make sense to do it here as well. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cluster_manager: Add support for USE_DOWNSTREAM_PROTOCOL.

56b72b8

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme mentioned this pull request Jan 9, 2018

cds: Add "auto_http2" option. envoyproxy/data-plane-api#399

Merged

htuch requested review from alyssawilk and zuercher January 9, 2018 15:08

htuch reviewed Jan 9, 2018

View reviewed changes

jrajahalme added 3 commits January 9, 2018 10:27

cluster_manager: Style fixes.

ed9fdaa

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

config: Drop v1 support for the new 'use downstream protocol' feature.

f574f20

Suggested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

router_test: Test USE_DOWNSTREAM_PROTOCOL.

b51e7c6

Add tests to check that the 'protocol' parameter to httpConnPoolForCluster() is passed correctly with and without the new cluster USE_DOWNSTREAM_PROTOCOL feature. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 10, 2018

View reviewed changes

jrajahalme added 2 commits January 10, 2018 10:32

Review fixes.

427ed8c

Suggested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

bazel: data-plane-api envoyproxy#399.

1ffc33c

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Merge branch 'master' into auto_http2

35c3758

jrajahalme force-pushed the auto_http2 branch from e1a4ba2 to 35c3758 Compare January 11, 2018 01:04

mattklein123 reviewed Jan 11, 2018

View reviewed changes

jrajahalme added 3 commits January 10, 2018 19:31

Merge branch 'master' into auto_http2

952c42e

router: Don't check if Optional is valid.

091cdf6

In this context Optional protocol is always valid. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Drop 'enum' from 'enum Http::Protocol'

aec941e

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 approved these changes Jan 11, 2018

View reviewed changes

mattklein123 merged commit 172fb8a into envoyproxy:master Jan 11, 2018

jrajahalme deleted the auto_http2 branch January 11, 2018 20:09

tanzeeb mentioned this pull request Jan 28, 2019

HTTP2 and gRPC support knative/serving#2539

Merged

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

update envoy sha (envoyproxy#2328)

39d6b75

Conversation

jrajahalme commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 9, 2018 via email

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 10, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 11, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants