From 92f9bf25eff6edf925f47eebd1ddfe6f6957af1e Mon Sep 17 00:00:00 2001 From: Jose Ulises Nino Rivera Date: Fri, 21 Feb 2020 10:13:01 -0800 Subject: [PATCH] upsteam protocol: add the ability to decide what protocol to use for upstream requests (#697) 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 (https://github.com/lyft/envoy-mobile/issues/502) Risk Level: low Testing: updated unit tests Docs Changes: updated docs Fixes #679 Signed-off-by: Michael Rebello Co-authored-by: Michael Rebello --- docs/root/api/http.rst | 2 + examples/java/hello_world/MainActivity.java | 3 + examples/kotlin/hello_world/MainActivity.kt | 5 + .../objective-c/hello_world/ViewController.m | 3 + .../swift/hello_world/ViewController.swift | 7 +- library/common/config_template.cc | 42 ++++++ library/common/http/dispatcher.cc | 32 ++++- library/common/http/dispatcher.h | 2 +- .../src/io/envoyproxy/envoymobile/BUILD | 1 + .../envoymobile/GRPCRequestBuilder.kt | 2 +- .../src/io/envoyproxy/envoymobile/Request.kt | 4 +- .../envoyproxy/envoymobile/RequestBuilder.kt | 37 +++-- .../envoyproxy/envoymobile/RequestMapper.kt | 6 +- .../envoymobile/UpstreamHttpProtocol.kt | 9 ++ .../envoymobile/GRPCRequestBuilderTest.kt | 9 ++ .../envoymobile/RequestBuilderTest.kt | 19 +++ .../envoymobile/RequestMapperTest.kt | 27 +++- .../io/envoyproxy/envoymobile/RequestTest.kt | 1 + library/swift/src/BUILD | 1 + library/swift/src/GRPCRequestBuilder.swift | 1 + library/swift/src/Request.swift | 9 +- library/swift/src/RequestBuilder.swift | 17 ++- library/swift/src/RequestMapper.swift | 8 +- library/swift/src/UpstreamHttpProtocol.swift | 18 +++ .../swift/test/GRPCRequestBuilderTests.swift | 8 ++ library/swift/test/RequestBuilderTests.swift | 17 +++ library/swift/test/RequestMapperTests.swift | 27 ++++ test/common/http/dispatcher_test.cc | 134 +++++++++++++++++- 28 files changed, 419 insertions(+), 32 deletions(-) create mode 100644 library/kotlin/src/io/envoyproxy/envoymobile/UpstreamHttpProtocol.kt create mode 100644 library/swift/src/UpstreamHttpProtocol.swift diff --git a/docs/root/api/http.rst b/docs/root/api/http.rst index 5c5f9ec314..75a77e2ce2 100644 --- a/docs/root/api/http.rst +++ b/docs/root/api/http.rst @@ -19,6 +19,7 @@ a previously created :ref:`Envoy instance `. val request = RequestBuilder(RequestMethod.POST, "https", "api.envoyproxy.io", "/foo") .addRetryPolicy(RetryPolicy(...)) + .addUpstreamHttpProtocol(UpstreamRequestProtocol.HTTP2) .addHeader("x-custom-header", "foobar") ... .build() @@ -27,6 +28,7 @@ a previously created :ref:`Envoy instance `. let request = RequestBuilder(method: .post, scheme: "https", authority: "api.envoyproxy.io", path: "/foo") .addRetryPolicy(RetryPolicy(...)) + .addUpstreamHttpProtocol(.http2) .addHeader(name: "x-custom-header", value: "foobar") ... .build() diff --git a/examples/java/hello_world/MainActivity.java b/examples/java/hello_world/MainActivity.java index 7eb7f244bf..5a1a9d1b6d 100644 --- a/examples/java/hello_world/MainActivity.java +++ b/examples/java/hello_world/MainActivity.java @@ -74,6 +74,9 @@ protected void onDestroy() { } private void makeRequest() { + // Note: this request will use an http/1.1 stream for the upstream request. + // The Kotlin example uses h2. This is done on purpose to test both paths in end-to-end tests + // in CI. Request request = new RequestBuilder(RequestMethod.GET, REQUEST_SCHEME, REQUEST_AUTHORITY, REQUEST_PATH) .build(); diff --git a/examples/kotlin/hello_world/MainActivity.kt b/examples/kotlin/hello_world/MainActivity.kt index 4f3b43d3c2..8fea25fd91 100644 --- a/examples/kotlin/hello_world/MainActivity.kt +++ b/examples/kotlin/hello_world/MainActivity.kt @@ -13,6 +13,7 @@ import io.envoyproxy.envoymobile.Envoy import io.envoyproxy.envoymobile.RequestBuilder import io.envoyproxy.envoymobile.RequestMethod import io.envoyproxy.envoymobile.ResponseHandler +import io.envoyproxy.envoymobile.UpstreamHttpProtocol import io.envoyproxy.envoymobile.shared.Failure import io.envoyproxy.envoymobile.shared.ResponseRecyclerViewAdapter @@ -73,7 +74,11 @@ class MainActivity : Activity() { } private fun makeRequest() { + // Note: this request will use an h2 stream for the upstream request. + // The Java example uses http/1.1. This is done on purpose to test both paths in end-to-end + // tests in CI. val request = RequestBuilder(RequestMethod.GET, REQUEST_SCHEME, REQUEST_AUTHORITY, REQUEST_PATH) + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) .build() val responseHeaders = HashMap>() val responseStatus = AtomicInteger() diff --git a/examples/objective-c/hello_world/ViewController.m b/examples/objective-c/hello_world/ViewController.m index 7f1539797e..798f7e2975 100644 --- a/examples/objective-c/hello_world/ViewController.m +++ b/examples/objective-c/hello_world/ViewController.m @@ -66,6 +66,9 @@ - (void)performRequest { NSLog(@"Starting request to '%@'", _REQUEST_PATH); int requestID = self.requestCount; + // Note: this request will use an http/1.1 stream for the upstream request. + // The Swift example uses h2. This is done on purpose to test both paths in end-to-end tests + // in CI. RequestBuilder *builder = [[RequestBuilder alloc] initWithMethod:RequestMethodGet scheme:_REQUEST_SCHEME authority:_REQUEST_AUTHORITY diff --git a/examples/swift/hello_world/ViewController.swift b/examples/swift/hello_world/ViewController.swift index 9dff26327a..b8fd90ab2a 100644 --- a/examples/swift/hello_world/ViewController.swift +++ b/examples/swift/hello_world/ViewController.swift @@ -47,9 +47,14 @@ final class ViewController: UITableViewController { NSLog("Starting request to '\(kRequestPath)'") let requestID = self.requestCount + // Note: this request will use an h2 stream for the upstream request. + // The Objective-C example uses http/1.1. This is done on purpose to test both paths in + // end-to-end tests in CI. let request = RequestBuilder(method: .get, scheme: kRequestScheme, authority: kRequestAuthority, - path: kRequestPath).build() + path: kRequestPath) + .addUpstreamHttpProtocol(.http2) + .build() let handler = ResponseHandler() .onHeaders { [weak self] headers, statusCode, _ in NSLog("Response status (\(requestID)): \(statusCode)\n\(headers)") diff --git a/library/common/config_template.cc b/library/common/config_template.cc index efadd136bc..916817acd8 100644 --- a/library/common/config_template.cc +++ b/library/common/config_template.cc @@ -96,6 +96,48 @@ const char* config_template = R"( dns_refresh_rate: {{ dns_refresh_rate_seconds }}s transport_socket: *base_transport_socket upstream_connection_options: *upstream_opts + - name: base_h2 + http2_protocol_options: {} + connect_timeout: {{ connect_timeout_seconds }}s + lb_policy: CLUSTER_PROVIDED + cluster_type: + name: envoy.clusters.dynamic_forward_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.clusters.dynamic_forward_proxy.v3.ClusterConfig + dns_cache_config: + name: dynamic_forward_proxy_cache_config + dns_lookup_family: AUTO + dns_refresh_rate: {{ dns_refresh_rate_seconds }}s + transport_socket: *base_transport_socket + upstream_connection_options: *upstream_opts + - name: base_wlan_h2 + http2_protocol_options: {} + connect_timeout: {{ connect_timeout_seconds }}s + lb_policy: CLUSTER_PROVIDED + cluster_type: + name: envoy.clusters.dynamic_forward_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.clusters.dynamic_forward_proxy.v3.ClusterConfig + dns_cache_config: + name: dynamic_forward_proxy_cache_config + dns_lookup_family: AUTO + dns_refresh_rate: {{ dns_refresh_rate_seconds }}s + transport_socket: *base_transport_socket + upstream_connection_options: *upstream_opts + - name: base_wwan_h2 + http2_protocol_options: {} + connect_timeout: {{ connect_timeout_seconds }}s + lb_policy: CLUSTER_PROVIDED + cluster_type: + name: envoy.clusters.dynamic_forward_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.clusters.dynamic_forward_proxy.v3.ClusterConfig + dns_cache_config: + name: dynamic_forward_proxy_cache_config + dns_lookup_family: AUTO + dns_refresh_rate: {{ dns_refresh_rate_seconds }}s + transport_socket: *base_transport_socket + upstream_connection_options: *upstream_opts - name: stats connect_timeout: {{ connect_timeout_seconds }}s dns_refresh_rate: {{ dns_refresh_rate_seconds }}s diff --git a/library/common/http/dispatcher.cc b/library/common/http/dispatcher.cc index c07771f68c..15443423c6 100644 --- a/library/common/http/dispatcher.cc +++ b/library/common/http/dispatcher.cc @@ -321,7 +321,7 @@ envoy_status_t Dispatcher::sendHeaders(envoy_stream_t stream, envoy_headers head RequestHeaderMapPtr internal_headers = Utility::toRequestHeaders(headers); ENVOY_LOG(debug, "[S{}] request headers for stream (end_stream={}):\n{}", stream, end_stream, *internal_headers); - attachPreferredNetwork(*internal_headers); + setDestinationCluster(*internal_headers); direct_stream->request_decoder_->decodeHeaders(std::move(internal_headers), end_stream); direct_stream->closeLocal(end_stream); } @@ -478,19 +478,41 @@ 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"}; +const std::string H2Suffix = "_h2"; +const std::string BaseClusterH2 = BaseCluster + H2Suffix; +const std::string BaseWlanClusterH2 = BaseWlanCluster + H2Suffix; +const std::string BaseWwanClusterH2 = BaseWwanCluster + H2Suffix; } // namespace -void Dispatcher::attachPreferredNetwork(HeaderMap& headers) { +void Dispatcher::setDestinationCluster(HeaderMap& headers) { + + // Determine upstream protocol. Use http2 if selected for explicitly, otherwise (any other value, + // absence of value) select http1. + // TODO(junr03): once http3 is available this would probably benefit from some sort of struct that + // maps to appropriate cluster names. However, with only two options (http1/2) this suffices. + bool use_h2_cluster{}; + const HeaderEntry* entry = headers.get(H2UpstreamHeader); + if (entry) { + if (entry->value() == "http2") { + use_h2_cluster = true; + } else { + ASSERT(entry->value() == "http1", + fmt::format("using unsupported protocol version {}", entry->value().getStringView())); + } + headers.remove(H2UpstreamHeader); + } + switch (preferred_network_.load()) { case ENVOY_NET_WLAN: - headers.addReference(ClusterHeader, BaseWlanCluster); + headers.addReference(ClusterHeader, use_h2_cluster ? BaseWlanClusterH2 : BaseWlanCluster); break; case ENVOY_NET_WWAN: - headers.addReference(ClusterHeader, BaseWwanCluster); + headers.addReference(ClusterHeader, use_h2_cluster ? BaseWwanClusterH2 : BaseWwanCluster); break; case ENVOY_NET_GENERIC: default: - headers.addReference(ClusterHeader, BaseCluster); + headers.addReference(ClusterHeader, use_h2_cluster ? BaseClusterH2 : BaseCluster); } } diff --git a/library/common/http/dispatcher.h b/library/common/http/dispatcher.h index a639ab3d90..81bb4b1db4 100644 --- a/library/common/http/dispatcher.h +++ b/library/common/http/dispatcher.h @@ -195,7 +195,7 @@ class Dispatcher : public Logger::Loggable { void post(Event::PostCb callback); DirectStreamSharedPtr getStream(envoy_stream_t stream_handle); void cleanup(envoy_stream_t stream_handle); - void attachPreferredNetwork(HeaderMap& headers); + void setDestinationCluster(HeaderMap& headers); Thread::MutexBasicLockable ready_lock_; std::list init_queue_ GUARDED_BY(ready_lock_); diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/BUILD b/library/kotlin/src/io/envoyproxy/envoymobile/BUILD index 6a1147fedf..2e5037eaa6 100644 --- a/library/kotlin/src/io/envoyproxy/envoymobile/BUILD +++ b/library/kotlin/src/io/envoyproxy/envoymobile/BUILD @@ -52,6 +52,7 @@ envoy_mobile_kt_library( "RetryPolicy.kt", "RetryPolicyMapper.kt", "StreamEmitter.kt", + "UpstreamHttpProtocol.kt", ], visibility = ["//visibility:public"], deps = [ diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/GRPCRequestBuilder.kt b/library/kotlin/src/io/envoyproxy/envoymobile/GRPCRequestBuilder.kt index 1c7ec24daa..202cc56193 100644 --- a/library/kotlin/src/io/envoyproxy/envoymobile/GRPCRequestBuilder.kt +++ b/library/kotlin/src/io/envoyproxy/envoymobile/GRPCRequestBuilder.kt @@ -15,7 +15,7 @@ class GRPCRequestBuilder( path = path) init { - underlyingBuilder.addHeader("content-type", "application/grpc") + underlyingBuilder.addHeader("content-type", "application/grpc").addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) } /** diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/Request.kt b/library/kotlin/src/io/envoyproxy/envoymobile/Request.kt index ea5b643e31..09a70ebf0d 100644 --- a/library/kotlin/src/io/envoyproxy/envoymobile/Request.kt +++ b/library/kotlin/src/io/envoyproxy/envoymobile/Request.kt @@ -14,7 +14,8 @@ data class Request internal constructor( val authority: String, val path: String, val headers: Map>, - val retryPolicy: RetryPolicy? + val retryPolicy: RetryPolicy?, + val upstreamHttpProtocol: UpstreamHttpProtocol? ) { /** @@ -27,5 +28,6 @@ data class Request internal constructor( return RequestBuilder(method, scheme, authority, path) .setHeaders(headers) .addRetryPolicy(retryPolicy) + .addUpstreamHttpProtocol(upstreamHttpProtocol) } } diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt b/library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt index 6767dcc916..3f0dbc224f 100644 --- a/library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt +++ b/library/kotlin/src/io/envoyproxy/envoymobile/RequestBuilder.kt @@ -22,16 +22,8 @@ class RequestBuilder( // Retry policy to use for this request. private var retryPolicy: RetryPolicy? = null - /** - * Add a retry policy to use for this request. - * - * @param retryPolicy the {@link io.envoyproxy.envoymobile.RetryPolicy} for this request. - * @return this builder. - */ - fun addRetryPolicy(retryPolicy: RetryPolicy?): RequestBuilder { - this.retryPolicy = retryPolicy - return this - } + // The protocol version to use for upstream requests. + private var upstreamHttpProtocol: UpstreamHttpProtocol? = null /** * Append a value to the header key. @@ -77,6 +69,28 @@ class RequestBuilder( return this } + /** + * Add a retry policy to use for this request. + * + * @param retryPolicy the {@link io.envoyproxy.envoymobile.RetryPolicy} for this request. + * @return this builder. + */ + fun addRetryPolicy(retryPolicy: RetryPolicy?): RequestBuilder { + this.retryPolicy = retryPolicy + return this + } + + /** + * Add an HTTP protocol hint for this request. + * + * @param upstreamHttpProtocol the {@link io.envoyproxy.envoymobile.UpstreamHttpProtocol} for this request. + * @return this builder. + */ + fun addUpstreamHttpProtocol(upstreamHttpProtocol: UpstreamHttpProtocol?): RequestBuilder { + this.upstreamHttpProtocol = upstreamHttpProtocol + return this + } + /** * Creates the {@link io.envoyproxy.envoymobile.Request} object using the data set in the builder. * @@ -89,7 +103,8 @@ class RequestBuilder( authority, path, headers, - retryPolicy + retryPolicy, + upstreamHttpProtocol ) } diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/RequestMapper.kt b/library/kotlin/src/io/envoyproxy/envoymobile/RequestMapper.kt index 4685607ed7..24f146351c 100644 --- a/library/kotlin/src/io/envoyproxy/envoymobile/RequestMapper.kt +++ b/library/kotlin/src/io/envoyproxy/envoymobile/RequestMapper.kt @@ -4,13 +4,17 @@ internal fun Request.outboundHeaders(): Map> { val retryPolicyHeaders = retryPolicy?.outboundHeaders() ?: emptyMap() val result = mutableMapOf>() - result.putAll(headers.filter { entry -> !entry.key.startsWith(":") }) + result.putAll(headers.filter { entry -> !entry.key.startsWith(":") && !entry.key.startsWith("x-envoy-mobile")}) result.putAll(retryPolicyHeaders) result[":method"] = listOf(method.stringValue()) result[":scheme"] = listOf(scheme) result[":authority"] = listOf(authority) result[":path"] = listOf(path) + if (upstreamHttpProtocol != null) { + result["x-envoy-mobile-upstream-protocol"] = listOf(upstreamHttpProtocol.stringValue) + } + return result } diff --git a/library/kotlin/src/io/envoyproxy/envoymobile/UpstreamHttpProtocol.kt b/library/kotlin/src/io/envoyproxy/envoymobile/UpstreamHttpProtocol.kt new file mode 100644 index 0000000000..72462d24fb --- /dev/null +++ b/library/kotlin/src/io/envoyproxy/envoymobile/UpstreamHttpProtocol.kt @@ -0,0 +1,9 @@ +package io.envoyproxy.envoymobile + +/** + * Available upstream HTTP protocols. + */ +enum class UpstreamHttpProtocol(internal val stringValue: String) { + HTTP1("http1"), + HTTP2("http2"), +} diff --git a/library/kotlin/test/io/envoyproxy/envoymobile/GRPCRequestBuilderTest.kt b/library/kotlin/test/io/envoyproxy/envoymobile/GRPCRequestBuilderTest.kt index 32fc905d82..754a9fb468 100644 --- a/library/kotlin/test/io/envoyproxy/envoymobile/GRPCRequestBuilderTest.kt +++ b/library/kotlin/test/io/envoyproxy/envoymobile/GRPCRequestBuilderTest.kt @@ -51,4 +51,13 @@ class GRPCRequestBuilderTest { assertThat(request.headers.containsKey("grpc-timeout")).isFalse() } + + @Test + fun `h2 header is present`() { + val headers = GRPCRequestBuilder("/pb.api.v1.Foo/GetBar", "foo.bar.com", false) + .build() + .outboundHeaders() + + assertThat(headers["x-envoy-mobile-upstream-protocol"]).containsExactly("http2") + } } diff --git a/library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt b/library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt index 936febbbbd..c8640f960c 100644 --- a/library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt +++ b/library/kotlin/test/io/envoyproxy/envoymobile/RequestBuilderTest.kt @@ -24,6 +24,25 @@ class RequestBuilderTest { assertThat(request.retryPolicy).isNull() } + @Test + fun `adding upstream http protocol should have hint present in request`() { + + val retryPolicy = RetryPolicy(maxRetryCount = 23, retryOn = listOf(RetryRule.STATUS_5XX, RetryRule.CONNECT_FAILURE), perRetryTimeoutMS = 1234) + val request = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "foo") + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) + .build() + + assertThat(request.upstreamHttpProtocol).isEqualTo(UpstreamHttpProtocol.HTTP2) + } + + @Test + fun `not adding upstream http protocol should have null upstream protocol in request`() { + val request = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "foo") + .build() + + assertThat(request.upstreamHttpProtocol).isNull() + } + @Test fun `adding new headers should append to the list of header keys`() { val request = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "foo") diff --git a/library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt b/library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt index 899b60d819..2ad27c0388 100644 --- a/library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt +++ b/library/kotlin/test/io/envoyproxy/envoymobile/RequestMapperTest.kt @@ -42,6 +42,26 @@ class RequestMapperTest { assertThat(requestHeaders[":path"]).containsExactly("/foo") } + @Test + fun `h1 is added to outbound request headers`() { + val requestHeaders = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "/foo") + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP1) + .build() + .outboundHeaders() + + assertThat(requestHeaders["x-envoy-mobile-upstream-protocol"]).containsExactly("http1") + } + + @Test + fun `h2 is added to outbound request headers`() { + val requestHeaders = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "/foo") + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) + .build() + .outboundHeaders() + + assertThat(requestHeaders["x-envoy-mobile-upstream-protocol"]).containsExactly("http2") + } + @Test fun `same key headers are joined to outbound request headers`() { val requestHeaders = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "/foo") @@ -54,27 +74,32 @@ class RequestMapperTest { } @Test - fun `invalid semicolon prefixed header keys are filtered out of outbound request headers`() { + fun `restricted header keys are filtered out of outbound request headers`() { val requestHeaders = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "/foo") .addHeader(":restricted", "value") + .addHeader("x-envoy-mobile-test", "value") .build() .outboundHeaders() assertThat(requestHeaders).doesNotContainKey(":restricted") + assertThat(requestHeaders).doesNotContainKey("x-envoy-mobile-test") } @Test fun `restricted headers are not overwritten in outbound request headers`() { val requestHeaders = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "/foo") + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) .addHeader(":scheme", "override") .addHeader(":authority", "override") .addHeader(":path", "override") + .addHeader("x-envoy-mobile-upstream-protocol", "override") .build() .outboundHeaders() assertThat(requestHeaders[":scheme"]).containsExactly("https") assertThat(requestHeaders[":authority"]).containsExactly("api.foo.com") assertThat(requestHeaders[":path"]).containsExactly("/foo") + assertThat(requestHeaders["x-envoy-mobile-upstream-protocol"]).containsExactly("http2") } @Test diff --git a/library/kotlin/test/io/envoyproxy/envoymobile/RequestTest.kt b/library/kotlin/test/io/envoyproxy/envoymobile/RequestTest.kt index 05493eebc1..366cb49b1f 100644 --- a/library/kotlin/test/io/envoyproxy/envoymobile/RequestTest.kt +++ b/library/kotlin/test/io/envoyproxy/envoymobile/RequestTest.kt @@ -27,6 +27,7 @@ class RequestTest { fun `requests converted to a builder should build to the same request`() { val request = RequestBuilder(method = RequestMethod.POST, scheme = "https", authority = "api.foo.com", path = "foo") .addRetryPolicy(RetryPolicy(23, listOf(RetryRule.STATUS_5XX, RetryRule.CONNECT_FAILURE), 1234)) + .addUpstreamHttpProtocol(UpstreamHttpProtocol.HTTP2) .addHeader("header_a", "value_a1") .addHeader("header_a", "value_a2") .addHeader("header_b", "value_b1") diff --git a/library/swift/src/BUILD b/library/swift/src/BUILD index 66b4a3a3ec..126c08154d 100644 --- a/library/swift/src/BUILD +++ b/library/swift/src/BUILD @@ -24,6 +24,7 @@ swift_static_framework( "RetryPolicy.swift", "RetryPolicyMapper.swift", "StreamEmitter.swift", + "UpstreamHttpProtocol.swift", ], module_name = "Envoy", objc_includes = [ diff --git a/library/swift/src/GRPCRequestBuilder.swift b/library/swift/src/GRPCRequestBuilder.swift index 29ee0b9b96..96eadf0152 100644 --- a/library/swift/src/GRPCRequestBuilder.swift +++ b/library/swift/src/GRPCRequestBuilder.swift @@ -16,6 +16,7 @@ public final class GRPCRequestBuilder: NSObject { authority: authority, path: path) self.underlyingBuilder.addHeader(name: "content-type", value: "application/grpc") + self.underlyingBuilder.addUpstreamHttpProtocol(.http2) } /// Append a value to the header key. diff --git a/library/swift/src/Request.swift b/library/swift/src/Request.swift index 49c124c30b..6e8b80e5aa 100644 --- a/library/swift/src/Request.swift +++ b/library/swift/src/Request.swift @@ -14,8 +14,10 @@ public final class Request: NSObject { /// Headers to send with the request. /// Multiple values for a given name are valid, and will be sent as comma-separated values. public let headers: [String: [String]] - // Retry policy to use for this request. + /// Retry policy to use for this request. public let retryPolicy: RetryPolicy? + /// The protocol version to use for upstream requests. + public let upstreamHttpProtocol: UpstreamHttpProtocol? /// Converts the request back to a builder so that it can be modified (i.e., by a filter). /// @@ -30,7 +32,8 @@ public final class Request: NSObject { authority: String, path: String, headers: [String: [String]] = [:], - retryPolicy: RetryPolicy?) + retryPolicy: RetryPolicy?, + upstreamHttpProtocol: UpstreamHttpProtocol?) { self.method = method self.scheme = scheme @@ -38,6 +41,7 @@ public final class Request: NSObject { self.path = path self.headers = headers self.retryPolicy = retryPolicy + self.upstreamHttpProtocol = upstreamHttpProtocol } } @@ -55,5 +59,6 @@ extension Request { && self.path == other.path && self.headers == other.headers && self.retryPolicy == other.retryPolicy + && self.upstreamHttpProtocol == other.upstreamHttpProtocol } } diff --git a/library/swift/src/RequestBuilder.swift b/library/swift/src/RequestBuilder.swift index f07c65b7e1..a5a20ee5a1 100644 --- a/library/swift/src/RequestBuilder.swift +++ b/library/swift/src/RequestBuilder.swift @@ -14,8 +14,10 @@ public final class RequestBuilder: NSObject { /// Headers to send with the request. /// Multiple values for a given name are valid, and will be sent as comma-separated values. public private(set) var headers: [String: [String]] = [:] - // Retry policy to use for this request. + /// Retry policy to use for this request. public private(set) var retryPolicy: RetryPolicy? + /// The protocol version to use for upstream requests. + public private(set) var upstreamHttpProtocol: UpstreamHttpProtocol? // MARK: - Initializers @@ -25,6 +27,7 @@ public final class RequestBuilder: NSObject { self.scheme = request.scheme self.authority = request.authority self.path = request.path + self.upstreamHttpProtocol = request.upstreamHttpProtocol self.headers = request.headers self.retryPolicy = request.retryPolicy } @@ -61,7 +64,6 @@ public final class RequestBuilder: NSObject { if self.headers[name]?.isEmpty == true { self.headers.removeValue(forKey: name) } - return self } @@ -71,13 +73,22 @@ public final class RequestBuilder: NSObject { return self } + @discardableResult + public func addUpstreamHttpProtocol(_ upstreamHttpProtocol: UpstreamHttpProtocol) + -> RequestBuilder + { + self.upstreamHttpProtocol = upstreamHttpProtocol + return self + } + public func build() -> Request { return Request(method: self.method, scheme: self.scheme, authority: self.authority, path: self.path, headers: self.headers, - retryPolicy: self.retryPolicy) + retryPolicy: self.retryPolicy, + upstreamHttpProtocol: self.upstreamHttpProtocol) } } diff --git a/library/swift/src/RequestMapper.swift b/library/swift/src/RequestMapper.swift index d98a86d9ad..21ef117999 100644 --- a/library/swift/src/RequestMapper.swift +++ b/library/swift/src/RequestMapper.swift @@ -1,4 +1,4 @@ -private let kRestrictedHeaderPrefix = ":" +private let kRestrictedHeaderPrefixes = [":", "x-envoy-mobile"] extension Request { /// Returns a set of outbound headers that include HTTP @@ -7,7 +7,7 @@ extension Request { /// - returns: Outbound headers to send with an HTTP request. func outboundHeaders() -> [String: [String]] { var headers = self.headers - .filter { !$0.key.hasPrefix(kRestrictedHeaderPrefix) } + .filter { !kRestrictedHeaderPrefixes.contains(where: $0.key.hasPrefix) } .reduce(into: [ ":method": [self.method.stringValue], ":scheme": [self.scheme], @@ -19,6 +19,10 @@ extension Request { headers = headers.merging(retryPolicy.outboundHeaders()) { _, retryHeader in retryHeader } } + if let upstreamHttpProtocol = self.upstreamHttpProtocol { + headers["x-envoy-mobile-upstream-protocol"] = [upstreamHttpProtocol.stringValue] + } + return headers } } diff --git a/library/swift/src/UpstreamHttpProtocol.swift b/library/swift/src/UpstreamHttpProtocol.swift new file mode 100644 index 0000000000..790b8a26e5 --- /dev/null +++ b/library/swift/src/UpstreamHttpProtocol.swift @@ -0,0 +1,18 @@ +import Foundation + +/// Available upstream HTTP protocols. +@objc +public enum UpstreamHttpProtocol: Int { + case http1 + case http2 + + /// String representation of the protocol. + var stringValue: String { + switch self { + case .http1: + return "http1" + case .http2: + return "http2" + } + } +} diff --git a/library/swift/test/GRPCRequestBuilderTests.swift b/library/swift/test/GRPCRequestBuilderTests.swift index 4b2e2bbc8d..12fe3464b0 100644 --- a/library/swift/test/GRPCRequestBuilderTests.swift +++ b/library/swift/test/GRPCRequestBuilderTests.swift @@ -26,6 +26,14 @@ final class GRPCRequestBuilderTests: XCTestCase { XCTAssertEqual(["application/grpc"], request.headers["content-type"]) } + func testAddsH2UpstreamHeader() { + let headers = GRPCRequestBuilder(path: "/pb.api.v1.Foo/GetBar", + authority: "foo.bar.com") + .build() + .outboundHeaders() + XCTAssertEqual(["http2"], headers["x-envoy-mobile-upstream-protocol"]) + } + func testUsesHTTPPOST() { let request = GRPCRequestBuilder(path: "/pb.api.v1.Foo/GetBar", authority: "foo.bar.com") diff --git a/library/swift/test/RequestBuilderTests.swift b/library/swift/test/RequestBuilderTests.swift index c1dcdb9713..571d1bdbaa 100644 --- a/library/swift/test/RequestBuilderTests.swift +++ b/library/swift/test/RequestBuilderTests.swift @@ -96,6 +96,23 @@ final class RequestBuilderTests: XCTestCase { XCTAssertNil(request.headers["foo"]) } + // MARK: - Upstream HTTP Protocol + + func testAddingUpstreamHttpProtocolHasUpstreamHttpProtocolInRequest() { + let request = RequestBuilder(method: .post, scheme: "https", + authority: "api.foo.com", path: "/foo") + .addUpstreamHttpProtocol(.http1) + .build() + XCTAssertEqual(.http1, request.upstreamHttpProtocol) + } + + func testNotAddingUpstreamHttpProtocolHasNilUpstreamHttpProtocolInRequest() { + let request = RequestBuilder(method: .post, scheme: "https", + authority: "api.foo.com", path: "/foo") + .build() + XCTAssertNil(request.upstreamHttpProtocol) + } + // MARK: - Request conversion func testRequestsAreEqualWhenPropertiesAreEqual() { diff --git a/library/swift/test/RequestMapperTests.swift b/library/swift/test/RequestMapperTests.swift index a82ccc5d10..1a836bf074 100644 --- a/library/swift/test/RequestMapperTests.swift +++ b/library/swift/test/RequestMapperTests.swift @@ -30,6 +30,22 @@ final class RequestMapperTests: XCTestCase { XCTAssertEqual(["/foo"], headers[":path"]) } + func testAddsH1ToHeaders() { + let headers = RequestBuilder(method: .post, scheme: "https", authority: "x.y.z", path: "/foo") + .addUpstreamHttpProtocol(.http1) + .build() + .outboundHeaders() + XCTAssertEqual(["http1"], headers["x-envoy-mobile-upstream-protocol"]) + } + + func testAddsH2ToHeaders() { + let headers = RequestBuilder(method: .post, scheme: "https", authority: "x.y.z", path: "/foo") + .addUpstreamHttpProtocol(.http2) + .build() + .outboundHeaders() + XCTAssertEqual(["http2"], headers["x-envoy-mobile-upstream-protocol"]) + } + func testJoinsHeaderValuesWithTheSameKey() { let headers = RequestBuilder(method: .post, scheme: "https", authority: "x.y.z", path: "/foo") .addHeader(name: "foo", value: "1") @@ -47,17 +63,28 @@ final class RequestMapperTests: XCTestCase { XCTAssertNil(headers[":restricted"]) } + func testStripsHeadersWithXEnvoyMobilePrefix() { + let headers = RequestBuilder(method: .post, scheme: "https", authority: "x.y.z", path: "/foo") + .addHeader(name: "x-envoy-mobile-test", value: "someValue") + .build() + .outboundHeaders() + XCTAssertNil(headers["x-envoy-mobile-test"]) + } + func testCannotOverrideStandardRestrictedHeaders() { let headers = RequestBuilder(method: .post, scheme: "https", authority: "x.y.z", path: "/foo") + .addUpstreamHttpProtocol(.http2) .addHeader(name: ":scheme", value: "override") .addHeader(name: ":authority", value: "override") .addHeader(name: ":path", value: "override") + .addHeader(name: "x-envoy-mobile-upstream-protocol", value: "override") .build() .outboundHeaders() XCTAssertEqual(["https"], headers[":scheme"]) XCTAssertEqual(["x.y.z"], headers[":authority"]) XCTAssertEqual(["/foo"], headers[":path"]) + XCTAssertEqual(["http2"], headers["x-envoy-mobile-upstream-protocol"]) } func testIncludesRetryPolicyHeaders() { diff --git a/test/common/http/dispatcher_test.cc b/test/common/http/dispatcher_test.cc index ab3715ecb2..130d5e8350 100644 --- a/test/common/http/dispatcher_test.cc +++ b/test/common/http/dispatcher_test.cc @@ -61,7 +61,7 @@ class DispatcherTest : public testing::Test { Dispatcher http_dispatcher_{preferred_network_}; }; -TEST_F(DispatcherTest, PreferredNetwork) { +TEST_F(DispatcherTest, SetDestinationCluster) { envoy_stream_t stream = 1; // Setup bridge_callbacks to handle the response headers. envoy_http_callbacks bridge_callbacks; @@ -103,7 +103,7 @@ TEST_F(DispatcherTest, PreferredNetwork) { HttpTestUtility::addDefaultHeaders(headers); envoy_headers c_headers = Utility::toBridgeHeaders(headers); - preferred_network_.store(ENVOY_NET_WLAN); + preferred_network_.store(ENVOY_NET_GENERIC); Event::PostCb send_headers_post_cb; EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&send_headers_post_cb)); http_dispatcher_.sendHeaders(stream, c_headers, false); @@ -113,7 +113,7 @@ TEST_F(DispatcherTest, PreferredNetwork) { {":method", "GET"}, {":authority", "host"}, {":path", "/"}, - {"x-envoy-mobile-cluster", "base_wlan"}, + {"x-envoy-mobile-cluster", "base"}, }; EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); send_headers_post_cb(); @@ -169,6 +169,134 @@ TEST_F(DispatcherTest, PreferredNetwork) { ASSERT_EQ(cc.on_complete_calls, 1); } +TEST_F(DispatcherTest, SetDestinationClusterUpstreamProtocol) { + envoy_stream_t stream = 1; + // Setup bridge_callbacks to handle the response headers. + envoy_http_callbacks bridge_callbacks; + callbacks_called cc = {0, 0, 0, 0, 0}; + bridge_callbacks.context = &cc; + bridge_callbacks.on_headers = [](envoy_headers c_headers, bool end_stream, + void* context) -> void { + ASSERT_TRUE(end_stream); + ResponseHeaderMapPtr response_headers = toResponseHeaders(c_headers); + EXPECT_EQ(response_headers->Status()->value().getStringView(), "200"); + callbacks_called* cc = static_cast(context); + cc->on_headers_calls++; + }; + bridge_callbacks.on_complete = [](void* context) -> void { + callbacks_called* cc = static_cast(context); + cc->on_complete_calls++; + }; + + // Create a stream. + Event::PostCb start_stream_post_cb; + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&start_stream_post_cb)); + EXPECT_EQ(http_dispatcher_.startStream(stream, bridge_callbacks), ENVOY_SUCCESS); + + // Grab the response encoder in order to dispatch responses on the stream. + // Return the request decoder to make sure calls are dispatched to the decoder via the dispatcher + // API. + EXPECT_CALL(api_listener_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder_ = &encoder; + return request_decoder_; + })); + start_stream_post_cb(); + + // Send request headers. Sending multiple headers is illegal and the upstream codec would not + // accept it. However, given we are just trying to test preferred network headers and using mocks + // this is fine. + + TestRequestHeaderMapImpl headers{{"x-envoy-mobile-upstream-protocol", "http2"}}; + HttpTestUtility::addDefaultHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); + + preferred_network_.store(ENVOY_NET_GENERIC); + Event::PostCb send_headers_post_cb; + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&send_headers_post_cb)); + http_dispatcher_.sendHeaders(stream, c_headers, false); + + TestResponseHeaderMapImpl expected_headers{ + {":scheme", "http"}, + {":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-envoy-mobile-cluster", "base_h2"}, + }; + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + send_headers_post_cb(); + + TestRequestHeaderMapImpl headers2{{"x-envoy-mobile-upstream-protocol", "http2"}}; + HttpTestUtility::addDefaultHeaders(headers2); + envoy_headers c_headers2 = Utility::toBridgeHeaders(headers2); + + preferred_network_.store(ENVOY_NET_WLAN); + Event::PostCb send_headers_post_cb2; + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&send_headers_post_cb2)); + http_dispatcher_.sendHeaders(stream, c_headers2, false); + + TestResponseHeaderMapImpl expected_headers2{ + {":scheme", "http"}, + {":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-envoy-mobile-cluster", "base_wlan_h2"}, + }; + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers2), false)); + send_headers_post_cb2(); + + TestRequestHeaderMapImpl headers3{{"x-envoy-mobile-upstream-protocol", "http2"}}; + HttpTestUtility::addDefaultHeaders(headers3); + envoy_headers c_headers3 = Utility::toBridgeHeaders(headers3); + + preferred_network_.store(ENVOY_NET_WWAN); + Event::PostCb send_headers_post_cb3; + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&send_headers_post_cb3)); + http_dispatcher_.sendHeaders(stream, c_headers3, true); + + TestResponseHeaderMapImpl expected_headers3{ + {":scheme", "http"}, + {":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-envoy-mobile-cluster", "base_wwan_h2"}, + }; + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers3), true)); + send_headers_post_cb3(); + + // Setting http1. + TestRequestHeaderMapImpl headers4{{"x-envoy-mobile-upstream-protocol", "http1"}}; + HttpTestUtility::addDefaultHeaders(headers4); + envoy_headers c_headers4 = Utility::toBridgeHeaders(headers4); + + preferred_network_.store(ENVOY_NET_WWAN); + Event::PostCb send_headers_post_cb4; + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&send_headers_post_cb4)); + http_dispatcher_.sendHeaders(stream, c_headers4, true); + + TestResponseHeaderMapImpl expected_headers4{ + {":scheme", "http"}, + {":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-envoy-mobile-cluster", "base_wwan"}, + }; + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers4), true)); + send_headers_post_cb4(); + + // Encode response headers. + Event::PostCb stream_deletion_post_cb; + EXPECT_CALL(event_dispatcher_, isThreadSafe()).Times(1).WillRepeatedly(Return(true)); + EXPECT_CALL(event_dispatcher_, post(_)).WillOnce(SaveArg<0>(&stream_deletion_post_cb)); + TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + response_encoder_->encodeHeaders(response_headers, true); + ASSERT_EQ(cc.on_headers_calls, 1); + stream_deletion_post_cb(); + + // Ensure that the callbacks on the bridge_callbacks were called. + ASSERT_EQ(cc.on_complete_calls, 1); +} + TEST_F(DispatcherTest, BasicStreamHeadersOnly) { envoy_stream_t stream = 1; // Setup bridge_callbacks to handle the response headers.