Support http1 for request-response protocol deployments (#1232)

* Support http1 for request-response protocol deployments

We already have the plumbing to set the request version appropriately based on discovered version (discovery goes through HTTP1.1 by default). We just end up ignoring that version at the last minute by setting the client to be http2 only. With this change, we will discover with http2, and fallback to http1.1 if it doesn't appear to be supported. The fallback request will respect ALPN, so it could technically still end up using http2 against HTTPS endpoints, but this would be odd.

* Accept a flag to force the use of http2

* Add flag to CLI

* Add comments

cli/src/commands/deployments/register.rs

@@ -52,6 +52,11 @@ pub struct Register {
     #[clap(long="extra-header", value_parser = parse_header, action = clap::ArgAction::Append)]
     extra_headers: Option<Vec<HeaderKeyValue>>,
 
+    /// Attempt discovery using a client that defaults to HTTP1.1 instead of a prior-knowledge HTTP2 client.
+    /// This may be necessary if you see `META0014` discovering local dev servers like `wrangler dev`.
+    #[clap(long = "use-http1.1")]
+    use_http_11: bool,
+
     /// The URL or ARN that Restate server needs to fetch service information from.
     ///
     /// The URL must be network-accessible from Restate server. In case of using
@@ -175,6 +180,7 @@ pub async fn run_register(State(env): State<CliEnv>, discover_opts: &Register) -
         DeploymentEndpoint::Uri(uri) => RegisterDeploymentRequest::Http {
             uri: uri.clone(),
             additional_headers: headers.clone().map(Into::into),
+            use_http_11: discover_opts.use_http_11,
             force,
             dry_run,
         },

crates/admin-rest-model/src/deployments.rs

@@ -104,6 +104,17 @@ pub enum RegisterDeploymentRequest {
         /// Additional headers added to the discover/invoke requests to the deployment.
         ///
         additional_headers: Option<SerdeableHeaderHashMap>,
+
+        /// # Use http1.1
+        ///
+        /// If `true`, discovery will be attempted using a client that defaults to HTTP1.1
+        /// instead of a prior-knowledge HTTP2 client. HTTP2 may still be used for TLS servers
+        /// that advertise HTTP2 support via ALPN. HTTP1.1 deployments will only work in
+        /// request-response mode.
+        ///
+        #[serde(default = "restate_serde_util::default::bool::<false>")]
+        use_http_11: bool,
+
         /// # Force
         ///
         /// If `true`, it will override, if existing, any deployment using the same `uri`.

crates/admin/src/rest_api/deployments.rs

@@ -48,11 +48,19 @@ pub async fn create_deployment<V>(
         RegisterDeploymentRequest::Http {
             uri,
             additional_headers,
+            use_http_11,
             force,
             dry_run,
         } => (
             DiscoverEndpoint::new(
-                Endpoint::Http(uri, Default::default()),
+                Endpoint::Http(
+                    uri,
+                    if use_http_11 {
+                        http::Version::HTTP_11
+                    } else {
+                        http::Version::HTTP_2
+                    },
+                ),
                 additional_headers.unwrap_or_default().into(),
             ),
             force,

crates/errors/src/error_codes/META0014.md

@@ -0,0 +1,9 @@
+## META0014
+
+Service discovery response failed, and the server may have responded in HTTP1.1.
+This can happen when discovering locally running dev servers from Faas platforms
+eg `wrangler dev`. FaaS platforms in generally will support HTTP2, however, so
+this is only a local development concern.
+
+You can try to discover the endpoint with `--use-http1.1` when working
+with these local dev servers. This should not be needed in production.

crates/errors/src/lib.rs

@@ -37,7 +37,7 @@ mod helper;
 declare_restate_error_codes!(
     RT0001, RT0002, RT0003, RT0004, RT0005, RT0006, RT0007, RT0009, RT0010, RT0011, RT0012, RT0013,
     RT0014, META0003, META0004, META0005, META0006, META0009, META0010, META0011, META0012,
-    META0013
+    META0013, META0014
 );
 
 // -- Some commonly used errors

crates/service-client/Cargo.toml

@@ -25,6 +25,7 @@ http-serde = { version = "1.1.2" }
 humantime = { workspace = true }
 hyper = { workspace = true }
 hyper-rustls = { workspace = true }
+h2 = { version = "0.3.20" }
 once_cell = { workspace = true }
 rustls = { workspace = true }
 serde = { workspace = true }

crates/service-client/src/http.rs

@@ -20,26 +20,34 @@ use hyper::http::HeaderValue;
 use hyper::{Body, HeaderMap, Method, Request, Response, Uri, Version};
 use hyper_rustls::HttpsConnector;
 use restate_types::config::HttpOptions;
+use std::error::Error;
 use std::fmt::Debug;
 use std::future;
 use std::future::Future;
-
 type Connector = ProxyConnector<HttpsConnector<HttpConnector>>;
 
 #[derive(Clone, Debug)]
 pub struct HttpClient {
-    client: hyper::Client<Connector, Body>,
+    // alpn client defaults to http1.1, but can upgrade to http2 using ALPN for TLS servers
+    alpn_client: hyper::Client<Connector, Body>,
+    // h2 client defaults to http2 and so supports unencrypted http2 servers
+    h2_client: hyper::Client<Connector, Body>,
 }
 
 impl HttpClient {
-    pub fn new(client: hyper::Client<Connector, Body>) -> Self {
-        Self { client }
+    pub fn new(
+        alpn_client: hyper::Client<Connector, Body>,
+        h2_client: hyper::Client<Connector, Body>,
+    ) -> Self {
+        Self {
+            alpn_client,
+            h2_client,
+        }
     }
 
     pub fn from_options(options: &HttpOptions) -> HttpClient {
         let mut builder = hyper::Client::builder();
         builder
-            .http2_only(true)
             .http2_keep_alive_timeout(options.http_keep_alive_options.timeout.into())
             .http2_keep_alive_interval(Some(options.http_keep_alive_options.interval.into()));
 
@@ -48,15 +56,20 @@ impl HttpClient {
         http_connector.set_nodelay(true);
         http_connector.set_connect_timeout(Some(options.connect_timeout.into()));
 
+        let https_connector = hyper_rustls::HttpsConnectorBuilder::new()
+            .with_native_roots()
+            .https_or_http()
+            .enable_http2()
+            .wrap_connector(http_connector);
+
+        let proxy_connector = ProxyConnector::new(options.http_proxy.clone(), https_connector);
+
         HttpClient::new(
-            builder.build::<_, hyper::Body>(ProxyConnector::new(
-                options.http_proxy.clone(),
-                hyper_rustls::HttpsConnectorBuilder::new()
-                    .with_native_roots()
-                    .https_or_http()
-                    .enable_http2()
-                    .wrap_connector(http_connector),
-            )),
+            builder.clone().build::<_, Body>(proxy_connector.clone()), // h1 client with alpn upgrade support
+            {
+                builder.http2_only(true);
+                builder.build::<_, hyper::Body>(proxy_connector) // h2-prior knowledge client
+            },
         )
     }
 
@@ -117,18 +130,44 @@ impl HttpClient {
             Err(err) => return future::ready(Err(err.into())).right_future(),
         };
 
-        let fut = self.client.request(request);
+        let client = match request.version() {
+            Version::HTTP_2 => &self.h2_client,
+            _ => &self.alpn_client,
+        };
 
-        Either::Left(async move { Ok(fut.await?) })
+        let fut = client.request(request);
+
+        Either::Left(async move {
+            match fut.await {
+                Ok(res) => Ok(res),
+                Err(err) if is_possible_h11_only_error(&err) => {
+                    Err(HttpError::PossibleHTTP11Only(err))
+                }
+                Err(err) => Err(HttpError::Hyper(err)),
+            }
+        })
     }
 }
 
+fn is_possible_h11_only_error(err: &hyper::Error) -> bool {
+    // this is the error we see from the h2 lib when the server sends back an http1.1 response
+    // to an http2 request. http2 is designed to start requests with what looks like an invalid
+    // HTTP1.1 method, so typically 1.1 servers respond with a 40x, and the h2 client sees
+    // this as an invalid frame.
+    err.source()
+        .and_then(|err| err.downcast_ref::<h2::Error>())
+        .and_then(|err| err.reason())
+        == Some(h2::Reason::FRAME_SIZE_ERROR)
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum HttpError {
     #[error(transparent)]
     Hyper(#[from] hyper::Error),
     #[error(transparent)]
     Http(#[from] hyper::http::Error),
+    #[error("server possibly only supports HTTP1.1, consider discovery with --use-http1.1: {0}")]
+    PossibleHTTP11Only(#[source] hyper::Error),
 }
 
 impl HttpError {
@@ -138,6 +177,7 @@ impl HttpError {
         match self {
             HttpError::Hyper(err) => err.is_retryable(),
             HttpError::Http(err) => err.is_retryable(),
+            HttpError::PossibleHTTP11Only(_) => false,
         }
     }
 }

crates/service-client/src/lib.rs

@@ -27,6 +27,7 @@ use std::future;
 use std::future::Future;
 use std::sync::Arc;
 
+pub use crate::http::HttpError;
 pub use crate::lambda::AssumeRoleCacheMode;
 use crate::request_identity::SignRequest;
 

crates/service-protocol/src/discovery.rs

@@ -17,7 +17,7 @@ use hyper::http::{HeaderName, HeaderValue};
 use hyper::{Body, HeaderMap, StatusCode};
 use itertools::Itertools;
 use once_cell::sync::Lazy;
-use restate_errors::{META0003, META0012, META0013};
+use restate_errors::{META0003, META0012, META0013, META0014};
 use restate_schema_api::deployment::ProtocolType;
 use restate_service_client::{Endpoint, Method, Parts, Request, ServiceClient, ServiceClientError};
 use restate_types::endpoint_manifest;
@@ -112,31 +112,42 @@ pub struct DiscoveredMetadata {
     pub supported_protocol_versions: RangeInclusive<i32>,
 }
 
-#[derive(Debug, thiserror::Error, CodedError)]
+#[derive(Debug, thiserror::Error)]
 pub enum DiscoveryError {
     // Errors most likely related to SDK bugs
     #[error("received a bad response from the SDK: {0}")]
-    #[code(META0013)]
     BadResponse(Cow<'static, str>),
     #[error(
         "received a bad response from the SDK that cannot be decoded: {0}. Discovery response: {}",
         String::from_utf8_lossy(.1)
     )]
-    #[code(unknown)]
     Decode(#[source] serde_json::Error, Bytes),
 
     // Network related retryable errors
     #[error("bad status code: {0}")]
-    #[code(META0003)]
     BadStatusCode(u16),
     #[error("client error: {0}")]
-    #[code(META0003)]
     Client(#[from] ServiceClientError),
     #[error("unsupported service protocol versions: [{min_version}, {max_version}]. Supported versions by this runtime are [{}, {}]", i32::from(MIN_SERVICE_PROTOCOL_VERSION), i32::from(MAX_SERVICE_PROTOCOL_VERSION))]
-    #[code(META0012)]
     UnsupportedServiceProtocol { min_version: i32, max_version: i32 },
 }
 
+impl CodedError for DiscoveryError {
+    fn code(&self) -> Option<&'static codederror::Code> {
+        match self {
+            DiscoveryError::BadResponse(_) => Some(&META0013),
+            DiscoveryError::Decode(_, _) => None,
+            DiscoveryError::BadStatusCode(_) => Some(&META0003),
+            // special code for possible http1.1 errors
+            DiscoveryError::Client(ServiceClientError::Http(
+                restate_service_client::HttpError::PossibleHTTP11Only(_),
+            )) => Some(&META0014),
+            DiscoveryError::Client(_) => Some(&META0003),
+            DiscoveryError::UnsupportedServiceProtocol { .. } => Some(&META0012),
+        }
+    }
+}
+
 impl DiscoveryError {
     /// Retryable errors are those which can be caused by transient faults and where
     /// retrying can succeed.