Allow for HTTP2.0 configuration (#62)

I had previously missed that without additional configuration, all downstream and upstream connections default to HTTP1.0 only. This PR fixes that, introducing new configuration with default-on support for H2 (where applicable)

Author: jamesmunns

source/river/assets/test-config.kdl

@@ -28,7 +28,7 @@ services {
         // as we'd like, at least one is required
         listeners {
             "0.0.0.0:8080"
-            "0.0.0.0:4443" cert-path="./assets/test.crt" key-path="./assets/test.key"
+            "0.0.0.0:4443" cert-path="./assets/test.crt" key-path="./assets/test.key" offer-h2=true
         }
 
         // Connectors are the "upstream" interfaces that we connect with. We can name as many
@@ -44,7 +44,7 @@ services {
                 discovery "Static"
                 health-check "None"
             }
-            "91.107.223.4:443" tls-sni="onevariable.com"
+            "91.107.223.4:443" tls-sni="onevariable.com" proto="h2-or-h1"
         }
 
         // Path control are optional modifiers for requests and responses
@@ -79,7 +79,7 @@ services {
         // at least one.
         listeners {
             "0.0.0.0:9000"
-            "0.0.0.0:9443" cert-path="./assets/test.crt" key-path="./assets/test.key"
+            "0.0.0.0:9443" cert-path="./assets/test.crt" key-path="./assets/test.key" offer-h2=true
         }
         // File servers have additional configuration items
         file-server {

source/river/src/config/internal.rs

@@ -158,6 +158,7 @@ pub enum ListenerKind {
     Tcp {
         addr: String,
         tls: Option<TlsConfig>,
+        offer_h2: bool,
     },
     Uds(PathBuf),
 }

source/river/src/config/kdl/mod.rs

@@ -6,7 +6,7 @@ use std::{
 
 use kdl::{KdlDocument, KdlEntry, KdlNode};
 use miette::{bail, Diagnostic, SourceSpan};
-use pingora::upstreams::peer::HttpPeer;
+use pingora::{protocols::ALPN, upstreams::peer::HttpPeer};
 
 use crate::{
     config::internal::{
@@ -392,21 +392,48 @@ fn extract_connector(
         return Err(Bad::docspan("Not a valid socket address", doc, node.span()).into());
     };
 
-    let args = utils::str_str_args(doc, args)?;
-    let (tls, sni) = match args.as_slice() {
-        [] => (false, String::new()),
-        [("tls-sni", sni)] => (true, sni.to_string()),
-        _ => {
+    // TODO: consistent enforcement of only-known args?
+    let args = utils::str_str_args(doc, args)?
+        .into_iter()
+        .collect::<HashMap<&str, &str>>();
+
+    let proto = match args.get("proto").copied() {
+        None => None,
+        Some("h1-only") => Some(ALPN::H1),
+        Some("h2-only") => Some(ALPN::H2),
+        Some("h1-or-h2") => {
+            tracing::warn!("accepting 'h1-or-h2' as meaning 'h2-or-h1'");
+            Some(ALPN::H2H1)
+        }
+        Some("h2-or-h1") => Some(ALPN::H2H1),
+        Some(other) => {
             return Err(Bad::docspan(
-                "This should have zero args or just 'tls-sni'",
+                format!(
+                    "'proto' should be one of 'h1-only', 'h2-only', or 'h2-or-h1', found '{other}'"
+                ),
                 doc,
                 node.span(),
             )
             .into());
         }
     };
+    let tls_sni = args.get("tls-sni");
+
+    let (tls, sni, alpn) = match (proto, tls_sni) {
+        (None, None) | (Some(ALPN::H1), None) => (false, String::new(), ALPN::H1),
+        (None, Some(sni)) => (true, sni.to_string(), ALPN::H2H1),
+        (Some(_), None) => {
+            return Err(
+                Bad::docspan("'tls-sni' is required for HTTP2 support", doc, node.span()).into(),
+            );
+        }
+        (Some(p), Some(sni)) => (true, sni.to_string(), p),
+    };
 
-    Ok(HttpPeer::new(sadd, tls, sni))
+    let mut peer = HttpPeer::new(sadd, tls, sni);
+    peer.options.alpn = alpn;
+
+    Ok(peer)
 }
 
 // services { Service { listeners { ... } } }
@@ -416,39 +443,57 @@ fn extract_listener(
     name: &str,
     args: &[KdlEntry],
 ) -> miette::Result<ListenerConfig> {
-    let mut args = utils::str_str_args(doc, args)?;
-    args.sort_by_key(|a| a.0);
+    let args = utils::str_value_args(doc, args)?
+        .into_iter()
+        .collect::<HashMap<&str, &KdlEntry>>();
 
     // Is this a bindable name?
     if name.parse::<SocketAddr>().is_ok() {
         // Cool: do we have reasonable args for this?
-        match args.as_slice() {
-            // No argument - it's a regular TCP listener
-            [] => Ok(ListenerConfig {
+        let cert_path = utils::map_ensure_str(doc, args.get("cert-path").copied())?;
+        let key_path = utils::map_ensure_str(doc, args.get("key-path").copied())?;
+        let offer_h2 = utils::map_ensure_bool(doc, args.get("offer-h2").copied())?;
+
+        match (cert_path, key_path, offer_h2) {
+            // No config? No problem!
+            (None, None, None) => Ok(ListenerConfig {
                 source: ListenerKind::Tcp {
                     addr: name.to_string(),
                     tls: None,
+                    offer_h2: false,
                 },
             }),
-            // exactly these two args: it's a TLS listener
-            [("cert-path", cpath), ("key-path", kpath)] => Ok(ListenerConfig {
+            // We must have both of cert-path and key-path if both are present
+            // ignore "offer-h2" if this is incorrect
+            (None, Some(_), _) | (Some(_), None, _) => {
+                return Err(Bad::docspan(
+                    "'cert-path' and 'key-path' must either BOTH be present, or NEITHER should be present",
+                    doc,
+                    node.span(),
+                )
+                .into());
+            }
+            // We can't offer H2 if we don't have TLS (at least for now, unless we
+            // expose H2C settings in pingora)
+            (None, None, Some(_)) => {
+                return Err(Bad::docspan(
+                    "'offer-h2' requires TLS, specify 'cert-path' and 'key-path'",
+                    doc,
+                    node.span(),
+                )
+                .into());
+            }
+            (Some(cpath), Some(kpath), offer_h2) => Ok(ListenerConfig {
                 source: ListenerKind::Tcp {
                     addr: name.to_string(),
                     tls: Some(TlsConfig {
                         cert_path: cpath.into(),
                         key_path: kpath.into(),
                     }),
+                    // Default to enabling H2 if unspecified
+                    offer_h2: offer_h2.unwrap_or(true),
                 },
             }),
-            // Otherwise, I dunno what this is
-            _ => {
-                return Err(Bad::docspan(
-                    "listeners must have no args or both cert-path and key-path",
-                    doc,
-                    node.span(),
-                )
-                .into())
-            }
         }
     } else if let Ok(pb) = name.parse::<PathBuf>() {
         // TODO: Should we check that this path exists? Otherwise it seems to always match

source/river/src/config/kdl/test.rs

@@ -31,6 +31,7 @@ fn load_test() {
                         source: crate::config::internal::ListenerKind::Tcp {
                             addr: "0.0.0.0:8080".into(),
                             tls: None,
+                            offer_h2: false,
                         },
                     },
                     ListenerConfig {
@@ -40,6 +41,7 @@ fn load_test() {
                                 cert_path: "./assets/test.crt".into(),
                                 key_path: "./assets/test.key".into(),
                             }),
+                            offer_h2: true,
                         },
                     },
                 ],
@@ -85,6 +87,7 @@ fn load_test() {
                     source: crate::config::internal::ListenerKind::Tcp {
                         addr: "0.0.0.0:8000".into(),
                         tls: None,
+                        offer_h2: false,
                     },
                 }],
                 upstreams: vec![HttpPeer::new("91.107.223.4:80", false, String::new())],
@@ -102,6 +105,7 @@ fn load_test() {
                     source: crate::config::internal::ListenerKind::Tcp {
                         addr: "0.0.0.0:9000".into(),
                         tls: None,
+                        offer_h2: false,
                     },
                 },
                 ListenerConfig {
@@ -111,6 +115,7 @@ fn load_test() {
                             cert_path: "./assets/test.crt".into(),
                             key_path: "./assets/test.key".into(),
                         }),
+                        offer_h2: true,
                     },
                 },
             ],
@@ -218,7 +223,8 @@ fn one_service() {
         val.basic_proxies[0].listeners[0].source,
         ListenerKind::Tcp {
             addr: "127.0.0.1:80".into(),
-            tls: None
+            tls: None,
+            offer_h2: false,
         }
     );
     assert_eq!(

source/river/src/config/kdl/utils.rs

@@ -1,6 +1,6 @@
 //! Various ad-hoc KDL document parsers used
 
-use super::OptExtParse;
+use super::{Bad, OptExtParse};
 use kdl::{KdlDocument, KdlEntry, KdlNode};
 use std::collections::HashMap;
 
@@ -94,6 +94,58 @@ pub(crate) fn str_str_args<'a>(
     Ok(out)
 }
 
+/// Useful for collecting all arguments as str:Value key pairs
+///
+/// KdlEntry is returned instead of KdlValue to allow for retaining the
+/// span for error messages
+pub(crate) fn str_value_args<'a>(
+    doc: &KdlDocument,
+    args: &'a [KdlEntry],
+) -> miette::Result<Vec<(&'a str, &'a KdlEntry)>> {
+    let mut out = vec![];
+    for arg in args {
+        let name =
+            arg.name()
+                .map(|a| a.value())
+                .or_bail("arguments should be named", doc, arg.span())?;
+
+        out.push((name, arg));
+    }
+    Ok(out)
+}
+
+/// If the argument exists, ensure it is a str
+///
+/// Useful with [`str_value_args()`].
+pub(crate) fn map_ensure_str<'a>(
+    doc: &'_ KdlDocument,
+    val: Option<&'a KdlEntry>,
+) -> miette::Result<Option<&'a str>> {
+    let Some(v) = val else {
+        return Ok(None);
+    };
+    match v.value().as_string() {
+        Some(vas) => Ok(Some(vas)),
+        None => Err(Bad::docspan("Expected string argument", doc, v.span()).into()),
+    }
+}
+
+/// If the argument exists, ensure it is a bool
+///
+/// Useful with [`str_value_args()`].
+pub(crate) fn map_ensure_bool(
+    doc: &KdlDocument,
+    val: Option<&KdlEntry>,
+) -> miette::Result<Option<bool>> {
+    let Some(v) = val else {
+        return Ok(None);
+    };
+    match v.value().as_bool() {
+        Some(vas) => Ok(Some(vas)),
+        None => Err(Bad::docspan("Expected bool argument", doc, v.span()).into()),
+    }
+}
+
 /// Extract a single un-named string argument, like `discovery "Static"`
 pub(crate) fn extract_one_str_arg<T, F: FnOnce(&str) -> Option<T>>(
     doc: &KdlDocument,

source/river/src/config/toml.rs

@@ -189,6 +189,7 @@ impl From<ListenerKind> for super::internal::ListenerKind {
             ListenerKind::Tcp { addr, tls } => super::internal::ListenerKind::Tcp {
                 addr,
                 tls: tls.map(Into::into),
+                offer_h2: false,
             },
             ListenerKind::Uds(a) => super::internal::ListenerKind::Uds(a),
         }
@@ -318,6 +319,7 @@ pub mod test {
                             source: internal::ListenerKind::Tcp {
                                 addr: "0.0.0.0:8080".into(),
                                 tls: None,
+                                offer_h2: false,
                             },
                         },
                         internal::ListenerConfig {
@@ -327,6 +329,7 @@ pub mod test {
                                     cert_path: "./assets/test.crt".into(),
                                     key_path: "./assets/test.key".into(),
                                 }),
+                                offer_h2: false,
                             },
                         },
                     ],
@@ -367,6 +370,7 @@ pub mod test {
                         source: internal::ListenerKind::Tcp {
                             addr: "0.0.0.0:8000".into(),
                             tls: None,
+                            offer_h2: false,
                         },
                     }],
                     upstreams: vec![HttpPeer::new("91.107.223.4:80", false, String::new())],

source/river/src/main.rs

@@ -5,6 +5,7 @@ mod proxy;
 use crate::{files::river_file_server, proxy::river_proxy_service};
 use config::internal::{ListenerConfig, ListenerKind};
 use pingora::{server::Server, services::Service};
+use pingora_core::listeners::TlsSettings;
 
 fn main() {
     // Set up tracing, including catching `log` crate logs from pingora crates
@@ -57,17 +58,31 @@ pub fn populate_listners<T>(
             ListenerKind::Tcp {
                 addr,
                 tls: Some(tls_cfg),
+                offer_h2,
             } => {
                 let cert_path = tls_cfg
                     .cert_path
                     .to_str()
                     .expect("cert path should be utf8");
                 let key_path = tls_cfg.key_path.to_str().expect("key path should be utf8");
-                service
-                    .add_tls(&addr, cert_path, key_path)
+
+                // TODO: Make conditional!
+                let mut settings = TlsSettings::intermediate(cert_path, key_path)
                     .expect("adding TLS listener shouldn't fail");
+                if offer_h2 {
+                    settings.enable_h2();
+                }
+
+                service.add_tls_with_settings(&addr, None, settings);
             }
-            ListenerKind::Tcp { addr, tls: None } => {
+            ListenerKind::Tcp {
+                addr,
+                tls: None,
+                offer_h2,
+            } => {
+                if offer_h2 {
+                    panic!("Unsupported configuration: {addr:?} configured without TLS, but H2 enabled which requires TLS");
+                }
                 service.add_tcp(&addr);
             }
             ListenerKind::Uds(path) => {