From f13ce6bf2de9db8673eb64b55ffbef7ce025e241 Mon Sep 17 00:00:00 2001 From: Jack Wampler Date: Thu, 27 Feb 2025 09:05:10 -0700 Subject: [PATCH] HickoryDnsResolver use a shared instance by default to limit fd use (#5523) --- Cargo.lock | 8 +-- Cargo.toml | 4 +- common/http-api-client/src/dns.rs | 100 ++++++++++++++++++++++++++---- 3 files changed, 95 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dac0f0ff20..6cc16f142c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3162,9 +3162,9 @@ dependencies = [ [[package]] name = "hickory-resolver" -version = "0.24.3" +version = "0.24.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dcf287bde7b776e85d7188e6e5db7cf410a2f9531fe82817eb87feed034c8d14" +checksum = "cbb117a1ca520e111743ab2f6688eddee69db4e0ea242545a604dce8a66fd22e" dependencies = [ "cfg-if", "futures-util", @@ -9912,9 +9912,9 @@ checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" [[package]] name = "tar" -version = "0.4.43" +version = "0.4.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c65998313f8e17d0d553d28f91a0df93e4dbbbf770279c7bc21ca0f09ea1a1f6" +checksum = "1d863878d212c87a19c1a610eb53bb01fe12951c0501cf5a0d65f724914a667a" dependencies = [ "filetime", "libc", diff --git a/Cargo.toml b/Cargo.toml index bf65a433fa..14e86aadfc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -256,7 +256,7 @@ handlebars = "3.5.5" headers = "0.4.0" hex = "0.4.3" hex-literal = "0.3.3" -hickory-resolver = "0.24.3" +hickory-resolver = "0.24.4" hkdf = "0.12.3" hmac = "0.12.1" http = "1" @@ -329,7 +329,7 @@ subtle-encoding = "0.5" syn = "1" sysinfo = "0.33.0" tap = "1.0.1" -tar = "0.4.43" +tar = "0.4.44" tempfile = "3.15" thiserror = "2.0" time = "0.3.37" diff --git a/common/http-api-client/src/dns.rs b/common/http-api-client/src/dns.rs index d35d72487b..7e0519de98 100644 --- a/common/http-api-client/src/dns.rs +++ b/common/http-api-client/src/dns.rs @@ -13,15 +13,18 @@ use crate::ClientBuilder; -use std::{net::SocketAddr, sync::Arc}; +use std::{ + net::SocketAddr, + sync::{Arc, LazyLock}, +}; -use hickory_resolver::lookup_ip::LookupIp; use hickory_resolver::{ config::{LookupIpStrategy, NameServerConfigGroup, ResolverConfig, ResolverOpts}, error::ResolveError, lookup_ip::LookupIpIntoIter, TokioAsyncResolver, }; +use hickory_resolver::{error::ResolveErrorKind, lookup_ip::LookupIp}; use once_cell::sync::OnceCell; use reqwest::dns::{Addrs, Name, Resolve, Resolving}; use tracing::warn; @@ -38,6 +41,14 @@ struct SocketAddrs { iter: LookupIpIntoIter, } +// n.b. static items do not call [`Drop`] on program termination, so this won't be deallocated. +// this is fine, as the OS can deallocate the terminated program faster than we can free memory +// but tools like valgrind might report "memory leaks" as it isn't obvious this is intentional. +static SHARED_RESOLVER: LazyLock = LazyLock::new(|| { + tracing::debug!("Initializing shared DNS resolver"); + HickoryDnsResolver::default() +}); + #[derive(Debug, thiserror::Error)] #[error("hickory-dns resolver error: {hickory_error}")] /// Error occurring while resolving a hostname into an IP address. @@ -47,29 +58,62 @@ pub struct HickoryDnsError { } /// Wrapper around an `AsyncResolver`, which implements the `Resolve` trait. +/// +/// Typical use involves instantiating using the `Default` implementation and then resolving using +/// methods or trait implementations. +/// +/// The default initialization uses a shared underlying `AsyncResolver`. If a thread local resolver +/// is required use `thread_resolver()` to build a resolver with an independently instantiated +/// internal `AsyncResolver`. #[derive(Debug, Default, Clone)] pub struct HickoryDnsResolver { - /// Since we might not have been called in the context of a - /// Tokio Runtime in initialization, so we must delay the actual - /// construction of the resolver. + // Since we might not have been called in the context of a + // Tokio Runtime in initialization, so we must delay the actual + // construction of the resolver. state: Arc>, fallback: Arc>, + dont_use_shared: bool, } impl Resolve for HickoryDnsResolver { fn resolve(&self, name: Name) -> Resolving { let resolver = self.state.clone(); let fallback = self.fallback.clone(); + let independent = self.dont_use_shared; Box::pin(async move { - let resolver = resolver.get_or_try_init(new_resolver)?; + let resolver = resolver.get_or_try_init(|| { + // using a closure here is slightly gross, but this makes sure that if the + // lazy-init returns an error it can be handled by the client + if independent { + new_resolver() + } else { + Ok(SHARED_RESOLVER.state.get_or_try_init(new_resolver)?.clone()) + } + })?; // try the primary DNS resolver that we set up (DoH or DoT or whatever) let lookup = match resolver.lookup_ip(name.as_str()).await { Ok(res) => res, Err(e) => { // on failure use the fall back system configured DNS resolver - warn!("primary DNS failed w/ error {e}: using system fallback"); - let resolver = fallback.get_or_try_init(new_resolver_system)?; + match e.kind() { + ResolveErrorKind::NoRecordsFound { .. } => {} + _ => { + warn!("primary DNS failed w/ error {e}: using system fallback"); + } + } + let resolver = fallback.get_or_try_init(|| { + // using a closure here is slightly gross, but this makes sure that if the + // lazy-init returns an error it can be handled by the client + if independent { + new_resolver_system() + } else { + Ok(SHARED_RESOLVER + .fallback + .get_or_try_init(new_resolver_system)? + .clone()) + } + })?; resolver.lookup_ip(name.as_str()).await? } }; @@ -93,21 +137,55 @@ impl Iterator for SocketAddrs { impl HickoryDnsResolver { /// Attempt to resolve a domain name to a set of ['IpAddr']s pub async fn resolve_str(&self, name: &str) -> Result { - let resolver = self.state.get_or_try_init(new_resolver)?; + let resolver = self.state.get_or_try_init(|| self.new_resolver())?; // try the primary DNS resolver that we set up (DoH or DoT or whatever) let lookup = match resolver.lookup_ip(name).await { Ok(res) => res, Err(e) => { // on failure use the fall back system configured DNS resolver - warn!("primary DNS failed w/ error {e}: using system fallback"); - let resolver = self.fallback.get_or_try_init(new_resolver_system)?; + match e.kind() { + ResolveErrorKind::NoRecordsFound { .. } => {} + _ => { + warn!("primary DNS failed w/ error {e}: using system fallback"); + } + } + let resolver = self + .fallback + .get_or_try_init(|| self.new_resolver_system())?; resolver.lookup_ip(name).await? } }; Ok(lookup) } + + /// Create a (lazy-initialized) resolver that is not shared across threads. + pub fn thread_resolver() -> Self { + Self { + dont_use_shared: true, + ..Default::default() + } + } + + fn new_resolver(&self) -> Result { + if self.dont_use_shared { + new_resolver() + } else { + Ok(SHARED_RESOLVER.state.get_or_try_init(new_resolver)?.clone()) + } + } + + fn new_resolver_system(&self) -> Result { + if self.dont_use_shared { + new_resolver_system() + } else { + Ok(SHARED_RESOLVER + .fallback + .get_or_try_init(new_resolver_system)? + .clone()) + } + } } /// Create a new resolver with a custom DoT based configuration. The options are overridden to look