use trust-dns for all DNS queries

Previously we were only using trust-dns for resolving SRV records in
server discovery, and then for resolving the hostname from the SRV
record target if one exists. With the previous behavior, admins need to
ensure that both their system resolver and trust-dns are working
correctly in order for outgoing traffic to work reliably. This can be
confusing to debug, because it's not obvious to the admin if or when
each resolver are being used. Now, everything goes through trust-dns and
outgoing federation DNS should fail/succeed more predictably.

I also expect some performance improvement from having an in-process DNS
cache, but haven't taken measurements yet.

Cargo.lock

@@ -929,7 +929,6 @@ dependencies = [
  "http",
  "http-body-util",
  "hyper",
- "hyper-util",
  "image",
  "insta",
  "jsonwebtoken",

Cargo.toml

@@ -102,7 +102,6 @@ html-escape = "0.2.13"
 http = "1.3.1"
 http-body-util = "0.1.3"
 hyper = "1.6.0"
-hyper-util = { version = "0.1.10", features = ["client", "client-legacy", "service"] }
 image = { version = "0.25.6", default-features = false, features = ["jpeg", "png", "gif"] }
 jsonwebtoken = "9.3.1"
 lru-cache = "0.1.2"

book/changelog.md

@@ -150,6 +150,9 @@ This will be the first release of Grapevine since it was forked from Conduit
     database path.
     ([!140](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/140),
     [!170](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/170))
+14. Use trust-dns for all DNS queries, instead of only for SRV records and SRV
+    record targets in server discovery.
+    ([!156](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/156))
 
 ### Fixed
 

src/service/globals.rs

@@ -1,6 +1,5 @@
 use std::{
     collections::{BTreeMap, HashMap},
-    error::Error as StdError,
     fs,
     future::{self, Future},
     iter,
@@ -14,11 +13,6 @@ use std::{
 };
 
 use base64::{engine::general_purpose, Engine as _};
-use futures_util::FutureExt;
-use hyper::service::Service as _;
-use hyper_util::{
-    client::legacy::connect::dns::GaiResolver, service::TowerToHyperService,
-};
 use reqwest::dns::{Addrs, Name, Resolve, Resolving};
 use ruma::{
     api::federation::discovery::ServerSigningKeys,
@@ -141,6 +135,38 @@ impl Default for RotationHandler {
     }
 }
 
+/// Wrapper around [`trust_dns_resolver`]'s [`TokioAsyncResolver`] that can be
+/// used with reqwest.
+pub(crate) struct DefaultResolver {
+    inner: Arc<TokioAsyncResolver>,
+}
+
+impl DefaultResolver {
+    fn new(inner: Arc<TokioAsyncResolver>) -> Self {
+        DefaultResolver {
+            inner,
+        }
+    }
+
+    fn resolve_inner(&self, name: Name) -> Resolving {
+        let inner = Arc::clone(&self.inner);
+        let future = async move {
+            let lookup = inner.lookup_ip(name.as_str()).await?;
+            let addrs: Addrs =
+                Box::new(lookup.into_iter().map(|ip| SocketAddr::new(ip, 0)));
+            Ok(addrs)
+        };
+        Box::pin(future.in_current_span())
+    }
+}
+
+impl Resolve for DefaultResolver {
+    #[tracing::instrument(skip(self))]
+    fn resolve(&self, name: Name) -> Resolving {
+        self.resolve_inner(name)
+    }
+}
+
 /// Resolver used for outgoing requests to the federation API.
 ///
 /// Hostnames that have been mapped to a different domain by SRV records in
@@ -150,14 +176,17 @@ impl Default for RotationHandler {
 ///
 /// [1]: https://spec.matrix.org/v1.12/server-server-api/#server-discovery
 pub(crate) struct FederationResolver {
-    inner: GaiResolver,
+    inner: Arc<DefaultResolver>,
     overrides: Arc<StdRwLock<TlsNameMap>>,
 }
 
 impl FederationResolver {
-    pub(crate) fn new(overrides: Arc<StdRwLock<TlsNameMap>>) -> Self {
+    pub(crate) fn new(
+        inner: Arc<DefaultResolver>,
+        overrides: Arc<StdRwLock<TlsNameMap>>,
+    ) -> Self {
         FederationResolver {
-            inner: GaiResolver::new(),
+            inner,
             overrides,
         }
     }
@@ -181,26 +210,7 @@ impl Resolve for FederationResolver {
                     x
                 })
             })
-            .unwrap_or_else(|| {
-                // This should never fail because reqwest's type is a wrapper
-                // around hyper-utils' type
-                let name = name.as_str().parse().expect("name should be valid");
-
-                Box::pin(
-                    TowerToHyperService::new(self.inner.clone())
-                        .call(name)
-                        .map(|result| {
-                            result
-                                .map(|addrs| -> Addrs { Box::new(addrs) })
-                                .map_err(
-                                    |err| -> Box<dyn StdError + Send + Sync> {
-                                        Box::new(err)
-                                    },
-                                )
-                        })
-                        .in_current_span(),
-                )
-            })
+            .unwrap_or_else(|| self.inner.resolve_inner(name))
     }
 }
 
@@ -235,16 +245,22 @@ impl Service {
                 )
             })?,
         );
+        let default_resolver =
+            Arc::new(DefaultResolver::new(Arc::clone(&dns_resolver)));
+        let federation_resolver = Arc::new(FederationResolver::new(
+            Arc::clone(&default_resolver),
+            Arc::clone(&tls_name_override),
+        ));
 
         let jwt_decoding_key = config.jwt_secret.as_ref().map(|secret| {
             jsonwebtoken::DecodingKey::from_secret(secret.as_bytes())
         });
 
-        let default_client = reqwest_client_builder(&config)?.build()?;
+        let default_client = reqwest_client_builder(&config)?
+            .dns_resolver(default_resolver)
+            .build()?;
         let federation_client = reqwest_client_builder(&config)?
-            .dns_resolver(Arc::new(FederationResolver::new(
-                tls_name_override.clone(),
-            )))
+            .dns_resolver(federation_resolver)
             .build()?;
 
         // Supported and stable room versions