feat(client): make dns resolver pluggable

[srijs]

Making the dns resolver pluggable via a trait.

This caters for the use-case of switching out the DNS implementation, for instance to influence caching behaviour, to share the dns cpu pool between multiple clients (which could be on different threads), or even to exchange it for a truly asynchronous, tokio-based resolver.

Sorry about not discussing this in an issue first, but it's blocking me on side project of mine, which is why I already started with the implementation. In any case, I'm not in a rush, so I'm happy to discuss further as part of this PR. Let me know what you think!

• The commit messages match the guidelines in https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md#git-commit-guidelines

[GitCop]

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

• Commit: 7652b19
• Your commit message body contains a line that is longer than 100 characters

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

---

This message was auto-generated by https://gitcop.com

[srijs]

Fixed up the line length in the commit message.

[seanmonstar]

Thanks for starting the discussion!

I realize a pool using getaddrinfo is not that great. I had originally thought that perhaps the Connect trait was sufficient. Someone can hook up any connecting they desire. Is there value in the http connector being generic over DNS? It'd seem like such a thin type at that point...

[srijs]

Thanks for the swift response!

Personally I think there is value in having a robust and reusable piece of code that handles connecting to a tcp server, and the fact that hyper-tls reuses HttpConnector seems like an affirmation of this.

An alternative I could think of is to split the concerns and handle dns lookup outside of the Connect trait, but that would be a breaking API change, something that I've been trying to avoid when making these changes.

Fundamentally what I would like to achieve is to be able to customize the way that hostnames are resolved when using hyper, without having to write and maintain my own connector.

I'm happy to explore/implement another solution if you can give me a hint towards how you would like this feature to be implemented.

[srijs]

@seanmonstar Have you had time to give this some thought? Would love to get your guidance as to where this PR should go. Apologies for bothering you if you are busy.

[seanmonstar]

Sorry, I was on vacation, and now I've been trying to catch up on my flood of email.

I know some others make use of the client of heavy loads already, and have heard of success plugging in a connector that used c-ares. I'm curious if they have thoughts as well (cc @jwilm).

Since, as you said, adding this isn't a breaking change, I'd probably not want to block the imminent 0.11 release, and make sure we get an API that doesn't get removed or changed significantly shortly after...

[srijs]

Just a quick note here: I've stumbled upon the abstract-ns crate by @tailhook, which already provides the trait we need here (link to doco).

On top of that, there are two crates ns-std-threaded and ns-dns-tokio that provide a futures-cpupool and a truly async (via the domain crate) implementation of this trait, respectively.

Unfortunately all three crates are labelled as "Proof of Concept" at this point, so there would probably need to be some effort to stabilise them in order to introduce a dependency.

[tailhook]

@srijs, while we use the crates in production, basically we need more users with more use cases before we declare them stable. And at least Address structure needs more accessor metrics.

The plan of abstract-ns is to make more than just plain DNS, first thing is to have notifications when address change (even if it's just polling DNS system) and to be able to plug not just DNS but also other name discovery mechanisms like consul or zookeeper. While this might be not useful in a browser, it's useful for a server-side usage of hyper.

Also, we have tk-pool which has connection pooling and reacts on a name changes.

Let me know if I can be of any help.

[srijs]

Thanks for taking the time to pitch in, @tailhook!

What you describe sounds like an awesome idea, and from experience I can confirm that a connection pool which is able to react to changes in dns records (or subscribe to other discovery feeds, such as Kubernetes endpoints) is an extremely valuable thing to have in a production environment.

We might want to pull on a slightly different thread here, which is: Is there a way we can integrate some of this into hyper, so that it would become easy to hook up an implementation of abstract_ns::Resolver to the hyper thread pool?

I mentioned in an earlier comment that we could also decouple the dns resolving mechanism from the Connect trait, which starts to make sense if service discovery needs to interact directly with the pooling mechanism in order to invalidate "stale" connections.

Of course this would be a bigger change than what I originally proposed, which is why we should consider it carefully, and I'm especially interested in your opinion, @seanmonstar, whether you see this as an option or feel it would complicate hyper too much.

Personally, there's a few changes I would suggest to the current Resolver trait, but I think if we managed to agree on a trait that could be shared between hyper and abstract_ns that would be a great thing.

Looking forward to hearing from the both of you!

[tailhook]

Personally, there's a few changes I would suggest to the current Resolver trait

Sure, open an issue. It's not carved in stone yet. I'm not satisfied with Resolver trait but I have to admit all my attempts to enhance it are just a little bit better. So your ideas might be helpful.

On the other hand, I tend to accept Stream<Address> (or just Address) in most APIs (as opposed to Resolver) so the trait might not be too critical and also easy to upgrade. Not sure if Stream API is enough for hyper though.

[srijs]

@seanmonstar penny for your thoughts?

[seanmonstar]

I kind of feel like the correct place to do all this is with the tokio's ClientProto. The ServerProto onion seems to work well, but I think the ClientProto one is forgotten about or something. Such a client onion could look like:

• The start of the onion, maybe some GaiDns thing, but it could also be skipped, if you already knew an IP address, or were using something like Unix sockets, that doesn't need DNS. This would give an T: AsyncRead + AsyncWrite.
• Optional next steps are ClientProto, building on that T. This could be something like tokios_tls::proto::Client.
• The end, taking some T: AsyncRead + AsyncWrite, and creating a thing that is a Service over it. This could be something like hyper::client::Http (or is just hyper::client::Client?)

The Connect trait I feel is more of a bandaid to get hyper clients working nicely, while the tokio side is better determined.

[seanmonstar]

Sorry I forgot about this! I think that discussing a change like this would be valuable! I'd actually prefer more discussion or exploration in this area before we were to merge something, and since this has grown stale, I'm going to close. Thank you again for starting the discussion!