Add configuration for trust-dns-resolver

[alexwl]

Currently, trust-dns-resolver is initialized with the system DNS configuration (/etc/resolv.conf on Unix OSes or registry on Windows) and there's no API to change the resolver configuration (#881).

This adds ClientBuilder::trust_dns_config() method that sets trust_dns_resolver::config::ResolverConfig and trust_dns_resolver::config::ResolverOpts.

[ajsyp]

This would be great to have, and makes working with specialized DNS tooling (e.g., Consul) much nicer. What do you think @seanmonstar ?

[seanmonstar]

I think providing the ability to configure it is a great idea! I'd suggest that we shouldn't expose publicly the exact trust_dns types, as that prevents reqwest from upgrading internally. I think we could do two things:

• Add some general DNS methods to ClientBuilder, which we use to configure trust-dns internally. If an option can be applied to both DNS resolvers, that's simple. For ones that only work with trust-dns, we could make them work similar to if you configure TLS options that don't work with the selected TLS backend. This is my preferred option.
• Provide an advanced (brittle) API taking an impl Any, kind of like use_preconfigured_tls(). This allows users the most control, but is complicated and can subtly break when reqwest upgrades trust-dns internally.

[alexwl]

@seanmonstar thanks for the response!

If an option can be applied to both DNS resolvers

As far as I understand, GaiResolver from hyper has no options.

Would it be acceptable to create 'wrapper' types for ResolverConfig and ResolverOpts? Thus, types from trust-dns-resolver would not be exposed publicly.

Example:

impl ClientBuilder {
   // ...
   #[cfg(feature = "trust-dns")]
   pub fn trust_dns_config(
       mut self,
       config: TrustDnsConfig,
       options: TrustDnsOpts,
   ) -> ClientBuilder {
       self.config.trust_dns_config = Some((config.config, options.opts));
       self
   }
}

#[derive(Debug)]
pub struct TrustDnsConfig {
    pub(crate) config: ResolverConfig,
}

#[derive(Debug)]
pub struct TrustDnsOpts {
    pub(crate) opts: ResolverOpts,
}

impl TrustDnsConfig {
    #[cfg(feature = "dns-over-tls")]
    pub fn from_ips_tls(
        ips: &[IpAddr],
        port: u16,
        tls_dns_name: String,
        domain: Option<String>,
        search: Vec<String>,
    ) -> Self {
        TrustDnsConfig {
            config: ResolverConfig::from_parts(
                domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
                search
                    .into_iter()
                    .map(|d| Name::from_str(&d).expect("Invalid domain"))
                    .collect(),
                NameServerConfigGroup::from_ips_tls(ips, port, tls_dns_name),
            ),
        }
    }
    pub fn from_ips_clear(
        ips: &[IpAddr],
        port: u16,
        domain: Option<String>,
        search: Vec<String>,
    ) -> Self {
        TrustDnsConfig {
            config: ResolverConfig::from_parts(
                domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
                search
                    .into_iter()
                    .map(|d| Name::from_str(&d).expect("Invalid domain"))
                    .collect(),
                NameServerConfigGroup::from_ips_clear(ips, port),
            ),
        }
    }
    #[cfg(feature = "dns-over-https")]
    pub fn from_ips_https(
        ips: &[IpAddr],
        port: u16,
        tls_dns_name: String,
        domain: Option<String>,
        search: Vec<String>,
    ) -> Self {
        TrustDnsConfig {
            config: ResolverConfig::from_parts(
                domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
                search
                    .into_iter()
                    .map(|d| Name::from_str(&d).expect("Invalid domain"))
                    .collect(),
                NameServerConfigGroup::from_ips_https(ips, port, tls_dns_name),
            ),
        }
    }
}

impl TrustDnsOpts {
    pub fn new() -> Self {
        TrustDnsOpts {
            opts: Default::default(),
        }
    }

    pub fn set_timeout(&mut self, timeout: std::time::Duration) {
        self.opts.timeout = timeout;
    }

    pub fn set_attempts(&mut self, attempts: usize) {
        self.opts.attempts = attempts;
    }

    pub fn set_cache_size(&mut self, cache_size: usize) {
        self.opts.cache_size = cache_size;
    }
}

[seanmonstar]

We could have wrapped types, but I find it a little leaky. What about having the methods directly on ClientBuilder? Are the ones you showed in the example all required?

[alexwl]

Sure, it is possible to have the methods directly on ClientBuilder:

impl ClientBuilder {
    // ...

    /// Sets the name servers for the `trust-dns` async resolver.
    ///
    /// # Optional
    ///
    #[cfg(feature = "trust-dns")]
    pub fn trust_dns_name_servers(
        mut self,
        ips: &[IpAddr],
        port: u16,
        domain: Option<String>,
        search: Vec<String>,
    ) -> ClientBuilder {
        let config = ResolverConfig::from_parts(
            domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
            search
                .into_iter()
                .map(|d| Name::from_str(&d).expect("Invalid domain"))
                .collect(),
            NameServerConfigGroup::from_ips_clear(ips, port),
        );
        self.config.trust_dns_config = Some((config, Default::default()));
        self
    }

    /// Sets the DNS-over-TLS name servers for the `trust-dns` async resolver.
    ///
    /// # Optional
    ///
    #[cfg(feature = "dns-over-tls")]
    pub fn trust_dns_name_servers_tls(
        mut self,
        ips: &[IpAddr],
        port: u16,
        tls_dns_name: String,
        domain: Option<String>,
        search: Vec<String>,
    ) -> ClientBuilder {
        let config = ResolverConfig::from_parts(
            domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
            search
                .into_iter()
                .map(|d| Name::from_str(&d).expect("Invalid domain"))
                .collect(),
            NameServerConfigGroup::from_ips_tls(ips, port, tls_dns_name),
        );
        self.config.trust_dns_config = Some((config, Default::default()));
        self
    }

    /// Sets the DNS-over-HTTPS name servers for the `trust-dns` async resolver.
    ///
    /// # Optional
    ///
    #[cfg(feature = "dns-over-https")]
    pub fn trust_dns_name_servers_https(
        mut self,
        ips: &[IpAddr],
        port: u16,
        tls_dns_name: String,
        domain: Option<String>,
        search: Vec<String>,
    ) -> ClientBuilder {
        let config = ResolverConfig::from_parts(
            domain.map(|d| Name::from_str(&d).expect("Invalid domain")),
            search
                .into_iter()
                .map(|d| Name::from_str(&d).expect("Invalid domain"))
                .collect(),
            NameServerConfigGroup::from_ips_https(ips, port, tls_dns_name),
        );
        self.config.trust_dns_config = Some((config, Default::default()));
        self
    }
}

The only required methods, in my opinion, are the ones that set upstream name servers (https://docs.rs/trust-dns-resolver/0.19.5/trust_dns_resolver/config/struct.ResolverConfig.html). Methods that set various options for the resolver (https://docs.rs/trust-dns-resolver/0.19.5/trust_dns_resolver/config/struct.ResolverOpts.html) are nice to have, but not essential.

[seanmonstar]

Awesome, yea I think the ResolverOpts stuff can probably be future additions if necessary.

So, here's what I'm thinking for the ResolverConfig stuff:

• Add a trust_dns: Option<Result<TrustDnsConfig, WhateverError>> to the Config struct. It starts as None by default. As users call methods to configure DNS, the (private) TrustDnsConfig object is updated. If we ever encounter an error, like Name::from_str, the config gets replaced with the error. Then in ClientBuilder::build, that error is returned via reqwest::error::builder().
• We can make methods that configure pieces of the DNS config at a time. This is a personal preference of mine, that the more arguments a function takes, the less readable it gets. It's worse when the types are similar, it's easy to mix up the order (such as mixing up domain and search). So something like:
  • trust_dns_name_servers(self, ips: &[IpAddr], port: u16)
  • trust_dns_local_domain(self, domain: &str)
  • trust_dns_over_https(self, tls_name: &str)
  • Etc

Whatcha think of this?

[alexwl]

Great suggestion, thanks! I'll update the PR accordingly.

[alexwl]

I've updated the PR.

Here's an example that configures name servers:

use reqwest::DnsTransport;
use std::net::{IpAddr, Ipv4Addr};

#[tokio::main]
async fn main() {
    let ips = vec![
        IpAddr::V4(Ipv4Addr::new(1, 1, 1, 1)),
        IpAddr::V4(Ipv4Addr::new(1, 0, 0, 1)),
    ];

    let client_builder = reqwest::Client::builder().trust_dns_name_servers(
        &ips,
        853,
        DnsTransport::Tls {
            tls_dns_name: "cloudflare-dns.com".to_owned(),
        },
    );

    let client = client_builder.build().unwrap();
    let result = client
        .get("https://www.rust-lang.org")
        .send()
        .await
        .unwrap();

    println!("{:?}", result);
}

Cargo.toml:

[dependencies]
tokio = {version = "0.2", features = ["full"] }
reqwest = {git = "https://github.com/alexwl/reqwest", branch = "trust-dns-config", features = ["trust-dns","trust-dns-over-rustls"] }

[jan-auer]

Are there still plans to pick up work on this, or is this going to be replaced with a plug-in resolver API proposed in #1125? I would love to contribute to whichever path has a future, as this turns out to be a required feature in some of my projects.

[alexwl]

@jan-auer
I think a generic resolver API (as suggested here #1125) is a better solution than ClientBuilder::trust_dns_* methods.

[dns2utf8]

I would like to have this feature with the plugin resolver. That would then allow one to access other methods of the resolver like flush of the cache dynamically

[dns2utf8]

I have defined an interface to abstract the resolvers: https://crates.io/crates/resolver_types