upsteam protocol: add the ability to decide what protocol to use for upstream requests #697

junr03 · 2020-02-20T06:40:56Z

Description: this PR adds the ability to decide what protocol to use for upstream requests. Note that this will be unnecessary once ALPN works for upstream requests (#502)
Risk Level: low
Testing: updated unit tests
Docs Changes: updated docs

Fixes #679

Signed-off-by: Jose Nino [email protected]

Signed-off-by: Jose Nino <[email protected]>

docs/root/api/http.rst

goaway · 2020-02-20T06:47:51Z

library/common/http/dispatcher.cc

@@ -477,19 +477,38 @@ const LowerCaseString ClusterHeader{"x-envoy-mobile-cluster"};
 const std::string BaseCluster = "base";
 const std::string BaseWlanCluster = "base_wlan";
 const std::string BaseWwanCluster = "base_wwan";
+const LowerCaseString H2UpstreamHeader{"x-envoy-mobile-upstream-protocol"};


I propose simplifying this with a x-envoy-mobile-force-h2 header. If a header with this name is present assume inferred h2 support, if not, default to h1.

Signed-off-by: Jose Nino <[email protected]>

library/common/http/dispatcher.cc

goaway · 2020-02-20T06:50:40Z

library/kotlin/src/io/envoyproxy/envoymobile/UpstreamHttpProtocol.kt

+enum class UpstreamHttpProtocol(internal val stringValue: String) {
+  HTTP1("http1"),
+  HTTP2("http2"),
+}


Can drop this enum if we just use a single header to force h2 (which is sort of non-standard anyways and only works with known destinations).

goaway

Nice work! Some suggestions to simplify this a bit.

goaway · 2020-02-20T06:55:47Z

Another approach would be to have an engine-level configuration option that allows you to specify authorities for which you know you can force h2. In some ways I like that better than a per-request approach, since support for this functionality is non-standard and relatively uncommon, and it avoids inadvertently dispatching requests to destinations that don't support it. It's more a property of the destination (:authority) than it is of the request.

That said, I'm okay with the per-request approach as well (it just means the application will have to implement its own whitelist).

junr03 · 2020-02-20T17:28:40Z

@goaway thanks for all the comments. I am answering to all of them in aggregate because I think they are all related.

I had originally written this PR with that approach, and actually with something similar to what you suggest below re: central configuration.

However, I moved to this approach for a few reasons:

The enum allows us to express potential expansion of future options: h3, auto (when alpn support lands on upstream connections).
The enum allows us to more easily migrate existing code to those future options without breaking the API if we find ourselves in a future where we care more strictly about API breakage as, hopefully, we see more adopters.
IMO the protocol support of your destination is an application level concern (if the library can't do protocol negotiation for you), and the application developer should tell us that prior knowledge. I am also not a big fan of centralized configuration given at boot time, as you know :)

Right now absence of the header defaults us to http1 per the implementation in the Http::Dispatcher::setDestinationCluster. I can do:

I'd suggest moving this out of the constructor, and adding it via a builder method.

per your suggestion make this setting a less strong requirement. I think this brings us closer to the sentiment of making this engine-level configuration without having to wire more configuration through.

lmk what you think.

Signed-off-by: Jose Nino <[email protected]>

library/common/http/dispatcher.cc

library/kotlin/src/io/envoyproxy/envoymobile/Request.kt

library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt

rebello95 · 2020-02-20T19:57:51Z

A couple comments based on the discussions above:

1 (enum vs bool): If http2 were to be the only protocol we need to differentiate going forward, I think a boolean would be fine. However, since http3 is coming in the near future and we probably don't want to make protocol negotiation a hard blocker or to make breaking changes to the interfaces being introduced here, an enum seems like it'd probably make sense.

2 (engine versus request): Adding this information on a per-request basis is a bit more in line with how other configurations are set up now in Envoy Mobile. We moved to the dynamic forward proxy so that we no longer have to specify a fixed set of domains when starting the library, and requiring an http2 whitelist up front when starting up seems like a step backwards as it doesn't allow for new http2 domains to be discovered without restarting the library (not currently supported). Allowing this information to be specified per-request as a builder seems reasonable, especially considering @junr03 is stripping all x-envoy-mobile* headers (keeping the header implementation internal to the library)

Signed-off-by: Jose Nino <[email protected]>

library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt

Signed-off-by: Jose Nino <[email protected]>

examples/swift/hello_world/ViewController.swift

library/common/http/dispatcher.cc

library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt

library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt

library/swift/src/RequestMapper.swift

library/swift/src/UpstreamHttpProtocol.swift

Signed-off-by: Jose Nino <[email protected]>

junr03 · 2020-02-20T23:13:59Z

@rebello95 sorry wasn't ready for a pass. I think I hit all your comments. Let me take a look

Signed-off-by: Jose Nino <[email protected]>

rebello95 · 2020-02-20T23:52:12Z

library/kotlin/src/io/envoyproxy/envoymobile/RequestMapper.kt

@@ -4,13 +4,17 @@ internal fun Request.outboundHeaders(): Map<String, List<String>> {
  val retryPolicyHeaders = retryPolicy?.outboundHeaders() ?: emptyMap()

  val result = mutableMapOf<String, List<String>>()
-  result.putAll(headers.filter { entry -> !entry.key.startsWith(":") })
+  result.putAll(headers.filter { entry -> !(entry.key.startsWith(":") || entry.key.startsWith("x-envoy-mobile"))})


Up to you but using this might be more readable: !entry.key.startsWith(":") && !entry.key.startsWith("x-envoy-mobile")

I am always ambivalent about which one is more readable. I'll change to kick CI... 💩

Signed-off-by: Jose Nino <[email protected]>

Signed-off-by: Michael Rebello <[email protected]>

Jose Nino added 3 commits February 19, 2020 22:11

upstream protocol: add the ability to use h2

602ecf0

Signed-off-by: Jose Nino <[email protected]>

build files

4b4e7eb

Signed-off-by: Jose Nino <[email protected]>

examples

9369b6c

Signed-off-by: Jose Nino <[email protected]>

goaway reviewed Feb 20, 2020

View reviewed changes

docs/root/api/http.rst Outdated Show resolved Hide resolved

goaway reviewed Feb 20, 2020

View reviewed changes

uncomment

28b7033

Signed-off-by: Jose Nino <[email protected]>

goaway reviewed Feb 20, 2020

View reviewed changes

library/common/http/dispatcher.cc Show resolved Hide resolved

goaway reviewed Feb 20, 2020

View reviewed changes

Jose Nino added 2 commits February 20, 2020 09:30

Merge branch 'master' into h2

6db464d

Signed-off-by: Jose Nino <[email protected]>

fix

088ce93

Signed-off-by: Jose Nino <[email protected]>

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/common/http/dispatcher.cc Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/kotlin/src/io/envoyproxy/envoymobile/Request.kt Outdated Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt Outdated Show resolved Hide resolved

Jose Nino added 3 commits February 20, 2020 13:04

move out of constructor

65d757e

Signed-off-by: Jose Nino <[email protected]>

fmt

9852268

Signed-off-by: Jose Nino <[email protected]>

examples

5e0d1e6

Signed-off-by: Jose Nino <[email protected]>

junr03 commented Feb 20, 2020

View reviewed changes

library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt Show resolved Hide resolved

Jose Nino added 2 commits February 20, 2020 13:29

java

059025d

Signed-off-by: Jose Nino <[email protected]>

fix

e9f87fc

Signed-off-by: Jose Nino <[email protected]>

rebello95 reviewed Feb 20, 2020

View reviewed changes

examples/swift/hello_world/ViewController.swift Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/common/http/dispatcher.cc Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt Outdated Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/swift/src/RequestMapper.swift Outdated Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/swift/src/UpstreamHttpProtocol.swift Outdated Show resolved Hide resolved

rebello95 reviewed Feb 20, 2020

View reviewed changes

library/swift/src/UpstreamHttpProtocol.swift Outdated Show resolved Hide resolved

polish

86c55fa

Signed-off-by: Jose Nino <[email protected]>

Jose Nino added 2 commits February 20, 2020 15:15

fmt

d4c65c1

Signed-off-by: Jose Nino <[email protected]>

example comment

1719bfa

Signed-off-by: Jose Nino <[email protected]>

rebello95 previously approved these changes Feb 20, 2020

View reviewed changes

demorgans

95cbe86

Signed-off-by: Jose Nino <[email protected]>

junr03 dismissed rebello95’s stale review via 95cbe86 February 21, 2020 00:14

fix example

a30fab9

Signed-off-by: Michael Rebello <[email protected]>

rebello95 approved these changes Feb 21, 2020

View reviewed changes

junr03 merged commit 92f9bf2 into master Feb 21, 2020

junr03 deleted the h2 branch February 21, 2020 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upsteam protocol: add the ability to decide what protocol to use for upstream requests #697

upsteam protocol: add the ability to decide what protocol to use for upstream requests #697

junr03 commented Feb 20, 2020 •

edited

Loading

goaway Feb 20, 2020

goaway Feb 20, 2020

goaway left a comment

goaway commented Feb 20, 2020

junr03 commented Feb 20, 2020

rebello95 commented Feb 20, 2020

junr03 commented Feb 20, 2020

rebello95 Feb 20, 2020

junr03 Feb 21, 2020

rebello95 Feb 21, 2020

upsteam protocol: add the ability to decide what protocol to use for upstream requests #697

upsteam protocol: add the ability to decide what protocol to use for upstream requests #697

Conversation

junr03 commented Feb 20, 2020 • edited Loading

goaway Feb 20, 2020

Choose a reason for hiding this comment

goaway Feb 20, 2020

Choose a reason for hiding this comment

goaway left a comment

Choose a reason for hiding this comment

goaway commented Feb 20, 2020

junr03 commented Feb 20, 2020

rebello95 commented Feb 20, 2020

junr03 commented Feb 20, 2020

rebello95 Feb 20, 2020

Choose a reason for hiding this comment

junr03 Feb 21, 2020

Choose a reason for hiding this comment

rebello95 Feb 21, 2020

Choose a reason for hiding this comment

junr03 commented Feb 20, 2020 •

edited

Loading