seanmonstar / reqwest

An easy and powerful Rust HTTP Client
https://docs.rs/reqwest
Apache License 2.0
9.73k stars 1.09k forks source link

Consider letting a custom DNS resolver customize the port? #1884

Open smklein opened 1 year ago

smklein commented 1 year ago

I've been making use of https://docs.rs/reqwest/0.11.18/reqwest/dns/trait.Resolve.html and https://docs.rs/reqwest/0.11.18/reqwest/struct.ClientBuilder.html#method.dns_resolver to construct reqwest clients that can use custom DNS servers.

This is really useful in an environment where I have redundancy requirements for services and DNS servers, and I'm happy those methods exist.

However, there's a quirk with them that makes usage a little difficult:

This even seems somewhat documented:

Warning

Since the DNS protocol has no notion of ports, if you wish to send traffic to a particular
port you must include this port in the URL itself, any port in the overridden addresses
will be ignored and traffic sent to the conventional port for the given scheme (e.g. 80 for http).

This is a pain-in-the-butt for systems where a DNS server uses SRV records to store a dynamic port. For example, my company has a project where we use SRV records and AAAA records to store service information, and both IPs and ports are dynamic. Allowing the ports to be dynamic helps us significantly with testing, where we often sit on "localhost, port zero" and let the OS pick a port for new servers. We know the ports once those servers start, but they're dynamic, and we'd like to record them in DNS via SRV records.

So here's my question

Would it be possible to have a variant of the https://docs.rs/reqwest/0.11.18/reqwest/struct.ClientBuilder.html#method.dns_resolver method that actually does respect the port value?

For example, I'd like the following code to work:

struct CustomResolver { ... }

// This is the trait which already exists...
impl reqwest::dns::Resolve for CustomResolver {
  // Returns an iterator of SocketAddrs.
  fn resolve(&self, name: Name) -> Resolving { ... }

  // XXX: This would be new
  // Tells reqwest to actually use the port returned by `Self::resolve`.
  //
  // A default implementation of this method could return `false` for backwards compatibility.
  fn use_port(&self) -> bool {
    true
  }
}

async fn get(custom_resolver: Arc<CustomResolver>) {
  let client = ClientBuilder::new()
    .with_resolver(custom_resolver)
    .build();

  // Should query the custom_resolver, and use the port which is returned. 
  client.get("http://mydomain.com").await.unwrap();
}
seanmonstar commented 1 year ago

Interesting use case! I was trying to find some prior art, but was struggling. Do you know of some other HTTP clients (probably in other languages) that allow such configuration, and what that looks like? It could help make a better design here.

smklein commented 1 year ago

I know it's not exactly a client library, but https://datatracker.ietf.org/doc/html/rfc2782 defines the SRV record's port value:

The format of the SRV RR

   Here is the format of the SRV RR, whose DNS type code is 33:

        _Service._Proto.Name TTL Class SRV Priority Weight Port Target

...

   Port
        The port on this target host of this service.  The range is 0-
        65535.  This is a 16 bit unsigned integer in network byte order.
        This is often as specified in Assigned Numbers but need not be.
...

The Port number

   Currently, the translation from service name to port number happens
   at the client, often using a file such as /etc/services.

   Moving this information to the DNS makes it less necessary to update
   these files on every single computer of the net every time a new
   service is added, and makes it possible to move standard services out
   of the "root-only" port range on unix.

Also, from an API point-of-view, it's just kinda weird to require a client to return a SocketAddr, and then to ignore the port value. For example, in our codebase, figuring out the right implementation of reqwest::dns::Resolve has required some trial-and-error:

https://github.com/oxidecomputer/omicron/blob/b1baf9cf6f0c2e6ed83b2105a917cc996dd193ed/oxide-client/src/lib.rs#L71-L88

seanmonstar commented 1 year ago

I agree that it's currently weird. Here's the reason: hyper used to want an IpAddr, but there was desire to access the scope ID.

To be clear, I'm not against it. Just wanted to make sure we made it make the most sense.