refactor!: use a single DNS resolver #3167

Frando · 2025-02-04T16:00:50Z

Description

We used to have more-or-less identical code in various places to set a static DNS resolver and add a few extension methods on the hickory_resolver::TokioResolver. This PR cleans this up and makes all of iroh use the same resolver, defined in a single place, as a newtype struct around the TokioResolver from hickory_resolver:

Adds a newtype struct DnsResolver around hickory_resolver::TokioResolver in iroh_relay::dns. It lives (for now) in iroh_relay, because this is a the bottom of our dependency chain: both iroh and iroh-net-report need to use it as well, and both depend on iroh_relay.
Adds most methods from the previous extension traits to the DnsResolver struct
Changes all imports to use this DnsResolver now instead of the previous type alias to TokioResolver
Removes all now-obsolete duplicate extension traits and type aliases and duplicated tests
Removes the global static DnsResolver that was accessible via iroh::dns::default_resolver. This means that by default multiple endpoints in the same app would not share the DnsResolver. However, I'd say a) it is not too common to use multiple endpoints, and b) if you do and want to share the DnsResolver, you can still do so by creating the DnsResolver yourself and cloning it into the endpoint builder.
Cleans up and aligns the various methods on DnsResolver, and newtype the types from hickory so that this public iroh API does not depend on hickory types (apart from a From<TokioResolver> for DnsResolver, which is deferred to a follow-up to investigate)

The only place outside of tests and examples where a new DnsResolver is constructed is in endpoint builder if no custom DnsResolver is set. This means: As long as an app has a single endpoint only, there will only be one DnsResolver.

Breaking Changes

iroh::dns::resolver and iroh::dns::default_resolver are removed. There is no static, global DNS resolver anymore. If you want to share a DNS resolver between endpoints, create the resolver yourself with iroh::dns::DnsResolver::new and clone it into the endpoint builders (in EndpointBuilder::dns_resolver). If you want to reuse the DNS resolver from an endpoint, you can access it with Endpoint::dns_resolver and clone it to wherever you need it.
iroh::dns::DnsResolver used to be a type alias and now is a reexport of iroh_relay::dns::DnsResolver struct
iroh::dns::node_info module is now a reexport of iroh_relay::dns::node_info
- iroh::dns::node_info::{IrohAttr, TxtAttrs, node_id_from_hickory_name} are no longer public. Use iroh::dns::DnsResolver::lookup_node_by_id or iroh::dns::DnsResolver::lookup_node_by_domain_name to lookup node info from DNS.
- iroh::dns::node_info::{to_z32, from_z32}are removed. Use the methods on iroh::dns::node_info::NodeIdExt trait instead.
iroh::discovery::dns::{N0_DNS_NODE_ORIGIN_PROD, N0_DNS_NODE_ORIGIN_STAGING} are now reexports of iroh_relay::dns::{N0_DNS_NODE_ORIGIN_PROD, N0_DNS_NODE_ORIGIN_STAGING}
iroh::dns::ResolverExt is removed. Use the methods on iroh::dns::DnsResolver instead.
The methods in iroh::dns::DnsResolver now take an impl ToString instead of impl hickory_proto::rr::domain::IntoName for their host argument
iroh::test_utils::create_dns_resolver is removed, use iroh::dns::DnsResolver::new instead

Notes & open questions

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
All breaking changes documented.

github-actions · 2025-02-04T16:02:53Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3167/docs/iroh/

Last updated: 2025-02-06T13:15:56Z

github-actions · 2025-02-04T16:12:57Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 2ce5eab

iroh-relay/src/dns.rs

dignifiedquire · 2025-02-04T18:01:10Z

iroh/src/dns.rs

-        host: N,
-        timeout: Duration,
-    ) -> impl Future<Output = Result<impl Iterator<Item = IpAddr>>>;
-
    /// Looks up node info by DNS name.
    fn lookup_by_name(&self, name: &str) -> impl Future<Output = Result<NodeAddr>>;


why not move the others as well?

Went forward with this and moved the node_info module and the lookup methods for nodes to iroh_relay as well.

This means iroh_relay now has a dependency on pkarr (because the functions to parse node info from pkarr packets live on the same structs as parsing from hickory dns records) - without default features though, so should be fine (includes neither the relay nor the dht clients).

Frando · 2025-02-05T08:23:35Z

I pushed another round of commits:

Removed the static DNS_RESOLVER alltogether. This means that by default multiple endpoints in the same app would not share the DnsResolver. However, I'd say a) it is not too common to use multiple endpoints, and b) if you do and want to share the DnsResolver, you can still do so by creating the DnsResolver yourself and cloning it into the endpoint builder.
- DnsResolver::new_with_defaults is only used as the default if no custom resolver is set on the endpoint builder, and in tests and examples. This means: As long as an app has a single endpoint only, there will only be one DnsResolver.
Moved iroh::dns::node_info to iroh_relay as well, so that now the functions to resolve node info over DNS are directly available on the DnsResolver.
- This means iroh_relay now has a dependency on pkarr (because the functions to parse node info from pkarr packets live on the same structs as parsing from hickory dns records) - without default features though, so should be fine (includes neither the relay nor the dht clients).

flub

I like these changes overall! Mostly some nitpicking.

I haven't looked at it, but this should take into account #2901 so that whatever it wants will still be possible with this DNS interface.

iroh-relay/src/dns.rs

flub · 2025-02-05T12:14:31Z

iroh-relay/src/dns.rs

+    }
+
+    /// Lookup a TXT record.
+    pub async fn lookup_txt(&self, query: impl IntoName) -> Result<TxtLookup, ResolveError> {


Howe come this doesn't have a timeout while the ipv4 & ipv6 looksups do?

I'm also confused by the return type, it's owned by hickory but not pub use? I'm sure I missed something because otherwise that shouldn't work.

Anyway, long way to say that we should own all our types and not re-export new types from external crates. We can newtype them.

All these methods were so far left as-is from the existing implementations in the extension traits.

I added a timeout to be in line with the others.

Also tried out adding newtypes for TxtLookup. It turned out OK. At the same time I though I realized that we still have quite a few hickory_resolver or hickory_proto types exposed, e.g. the IntoName trait is used in most of the methods on DnsResolver. Do we want to newtype everything we expose? I'll do a review of the API next, but many of these hickory types are composed or match with other hickory types, so to cover the full API surface of a Dns resolver that will be quite some newtyping, methods etc.

The remaining usage of hickory-proto is now the IntoName trait, which efficiently converts String, &str and Names into the Name struct from hickory_proto.

flub · 2025-02-05T12:15:50Z

iroh-relay/src/dns.rs

+        url: &Url,
+        prefer_ipv6: bool,
+        timeout: Duration,
+    ) -> Result<IpAddr> {


This is a bit an odd API, why not return impl Iterator<Item = IpAddr> like for the other resolve methods?

Taken over from existing usage. Likely we want to change that, yes - it is not used much so will be a simple change to align with the others.

This is a bit an odd API, why not return impl Iterator<Item = IpAddr> like for the other resolve methods?

the point of this method when I last touched it, is to handle the turning the iterator into a single address, for you

iroh-relay/src/dns/node_info.rs

iroh/src/endpoint.rs

flub

Just more nits, like before nothing serious. Overall this is good.

flub · 2025-02-05T15:14:33Z

iroh-relay/src/dns.rs

 impl From<TokioResolver> for DnsResolver {
    fn from(resolver: TokioResolver) -> Self {
        DnsResolver(resolver)
    }
 }

+/// TXT records returned from [`DnsResolver::lookup_txt`]
+#[derive(Debug, Clone)]
+pub struct TxtLookup(pub(self) hickory_resolver::lookup::TxtLookup);


TIL about pub(self) (had to look this up in https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.scoped.self). I'm confused why this even exists in rust? Seems like leaving it off is exactly the same, so why do folks want to spell it out sometimes? What is the motivation here?

Yea, dunno, removed it :)

flub · 2025-02-05T15:18:23Z

iroh-relay/src/dns/node_info.rs

+/// subsequent labels.
+///
+/// Returns a [`NodeId`] if parsed successfully, otherwise `None`.
+pub fn node_id_from_domain_name(name: &str) -> Option<NodeId> {


This seems like a pkarr helper method. Is there any reason to make all this pub? Maybe this stuff was already pub, but I guess we haven't tried to make the pub API here much smaller yet. If we don't want to reduce the amount of pub in this PR to keep concerns separate (I often prefer this in my own PRs) I'm also fine to leave this here for now.

It was only used in iroh::test_utils - moved it there.

dignifiedquire · 2025-02-05T16:25:17Z

iroh-relay/src/dns.rs

+    }
+
+    /// Create a new DNS resolver configured with a single UDP DNS nameserver.
+    pub fn with_single_nameserver(nameserver: SocketAddr) -> Self {


nit: I would probably prefer with_namesever

agree, renamed

Frando · 2025-02-06T08:59:15Z

I pushed another round of commits:

Rename some methods as suggested
Further reduce public API surface. Move z32 encoding of NodeId into an extension trait instead of public methods.
Move public methods that were only used in test_utils to test_utils

There's now three remaining uses of hickory_* types in the public API surface of the dns module:

IntoName trait - we can change this to ToString instead, I think (with a perfomance penalty if you have a hickory Name already) - see refactor: Remove hickory_protos IntoName from public API surface of iroh, replace with ToString #3173
impl From<hickory_resolver::TokioResolver> for DnsResolver - we could remove this, it is not used within the iroh repo at the moment. However, this is the current escape hatch for a custom configured DnsResolver. Removing that would mean that we'd have to newtype the full, complex builder pattern of hickory_resolver if we wanted to allow custom configuration. Without this From impl, we currently only expose the default resolver (using the system's configured nameservers) and a function to create a resolver with a single UDP nameserver.

flub

The From<HickoryResolver> is probably fine as an external type? Can this still get you into trouble if they update it?

…of `iroh`, replace with `ToString` (#3173) ## Description Removes `IntoName` trait from `hickory_proto` from the public API of `iroh::dns` module, to not commit to hickory types in our potential 1.0 API. Replaces the `impl IntoName` with `impl ToString`, which covers all our existing usages. This might incur a small perfomance penalty if you already have a hickory `Name` at the call site, but I don't think it will matter in practice. ## Breaking Changes * The methods in `iroh::dns::DnsResolver` now take an `impl ToString` instead of `impl hickory_proto::rr::domain::IntoName` for their `host` argument ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.

Frando · 2025-02-06T13:21:05Z

The From is probably fine as an external type? Can this still get you into trouble if they update it?

We should revisit this before 1.0. I think the impl is no different than a method that takes the TokioResolver, which means we cannot update hickory major version without that being a semver breaking change to iroh...

But I'd leave figuring out a solution to a followup PR.

flub · 2025-02-06T13:25:32Z

The From is probably fine as an external type? Can this still get you into trouble if they update it?

We should revisit this before 1.0. I think the impl is no different than a method that takes the TokioResolver, which means we cannot update hickory major version without that being a semver breaking change to iroh...

But I'd leave figuring out a solution to a followup PR.

Hum, yes. You're probably right. 👍

Frando added 2 commits February 4, 2025 16:48

refactor: use a single DNS resolver

4b5f56d

refactor: do not deref to TokioResolver, cleanups

1641b7d

chore: fix compilations

a90bc41

Frando changed the title ~~refactor: use a single DNS resolver~~ refactor!: use a single DNS resolver Feb 4, 2025

dignifiedquire reviewed Feb 4, 2025

View reviewed changes

iroh-relay/src/dns.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Feb 4, 2025

View reviewed changes

iroh-relay/src/dns.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Feb 4, 2025

View reviewed changes

Frando added 5 commits February 4, 2025 20:23

refactor: do not use a static DNS resolver

8d2c000

refactor: use DnsResolver::new_with_single_nameserver in more places

1f05769

refactor: move iroh::dns::node_info to iroh_relay, cleanup API

302600c

refactor: cleanups and doc fixes

acc668b

chore: cleanups

c6d092b

docs: fixes

0b6b03f

Frando marked this pull request as ready for review February 5, 2025 08:55

flub reviewed Feb 5, 2025

View reviewed changes

Frando added 4 commits February 5, 2025 14:35

refactor: impl Default for DnsResolver

1576a91

refactor: method names, and newtype some hickory_resolver types

4217e6e

refactor: add node_id_from_domain_name

6251052

fix: use hickory_proto through hickory_resolver

5a28d66

flub approved these changes Feb 5, 2025

View reviewed changes

dignifiedquire reviewed Feb 5, 2025

View reviewed changes

Frando added 3 commits February 5, 2025 22:09

refactor: rename method

ff27431

refactor: reduce public api surface, cleanup api

c37500a

chore: fixup clippy, docs, fmt

ee6894c

docs

b86021f

flub approved these changes Feb 6, 2025

View reviewed changes

Frando added this pull request to the merge queue Feb 7, 2025

Merged via the queue into main with commit c39b998 Feb 7, 2025
27 of 28 checks passed

refactor!: use a single DNS resolver #3167

refactor!: use a single DNS resolver #3167

Uh oh!

Conversation

Frando commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Breaking Changes

Notes & open questions

Change checklist

Uh oh!

github-actions bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frando commented Feb 5, 2025

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frando commented Feb 6, 2025

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Frando commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub commented Feb 6, 2025

Uh oh!

Uh oh!

Uh oh!

Frando commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

Frando commented Feb 6, 2025 •

edited

Loading