Skip to content

Commit 7f2cdd1

Browse files
authored
refactor(iroh)!: Add IntoDiscovery trait (#3327)
## Description Discovery services may want to access the endpoint they are attached to for various reasons. * Currently, we pass a reference to the `&Endpoint` in `Discovery::resolve`, but not in `Discovery::publish`. * All pkarr-based publishers need access to the endpoint's secret key for signing packets. For this reason, we allow passing a closure to `endpoint::Builder::add_discovery` that gets the secret key as an argument * The pkarr publisher *should* get hold of the endpoint's DNS resolver to resolve the pkarr relay URL (but doesn't atm, which is a bug - it uses reqwest's default resolver, always) So this PR changes the construction by adding an `IntoDiscovery` trait with a single method `into_discovery(self, context: &DiscoveryContext) -> impl Discovery`. And `endpoint::Builder::add_discovery` now takes any `IntoDiscovery`. Through this, discovery service construction is a two-step process where users create a builder and set any options available, and then during endpoint building we call `into_discovery` and pass a discovery context. We can't pass a full endpoint to discovery services, because if they'd clone it, the Endpoint could never be dropped due to a reference cycle: `Endpoint -> Magicsock -> Box<dyn Discovery> -> Endpoint`. Therefore, this PR adds a `DiscoveryContext` struct that contains the endpoint's secret key and DNS resolver. We can add more things later, if needed. There's also a blanket impl of `IntoDiscovery` for any `T: Discovery`, so discovery services that do not need an endpoint can skip the builder and don't have to implement `IntoDiscovery` manually at all, users can just pass the constructed discovery struct to `Endpoint::add_discovery`. This means *writing* discovery services is a little bit more involved, as you have to create a builder and implement a second trait. However, writing discovery services is a rather rare task, and with good docs the additional work is very minor. On the upside, this creates a clean way for discovery services to access the endpoint they are mounted on, which also opens the door for discovery services using iroh itself. ## Breaking Changes Changed: * `iroh::endpoint::Builder::add_discovery` now takes an `impl iroh::discovery::IntoDiscovery` argument instead of a closure that returns a `Discovery`. You can implement that on a builder struct, and any `T: Discovery` has an auto-impl of `IntoDiscovery`. * `iroh::discovery::Discovery::resolve` no longer gets a `&Endpoint` argument. If you need an endpoint in your discovery service, add a builder struct and implement `IntoDiscovery` for it, which gives you an endpoint that you can clone into your service * Removed `iroh::discovery::dns::DnsDiscovery::new`, use `DnsDiscovery::builder` instead * Removed `iroh::discovery::pkarr::PkarrPublisher::new`, use `PkarrPublisher::builder` instead * Removed `iroh::discovery::pkarr::PkarrPublisher::with_options`, use `PkarrPublisher::builder` instead * Removed `iroh::discovery::pkarr::PkarrResolver::new`, use `PkarrResolver::builder` instead * `iroh::discovery::pkarr::PkarrPublisher::n0_dns` now takes no arguments and returns a `PkarrPublisherBuilder`. The secret key is set on `PkarrPublisherBuilder::build` instead. ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist <!-- Remove any that are not relevant. --> - [x] Self-review. - [x] 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. - [x] Tests if relevant. - [x] All breaking changes documented. - [x] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)
1 parent 19323e6 commit 7f2cdd1

File tree

12 files changed

+546
-238
lines changed

12 files changed

+546
-238
lines changed

iroh-relay/src/dns.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ use snafu::{Backtrace, GenerateImplicitData, OptionExt, Snafu};
1818
use tokio::sync::RwLock;
1919
use url::Url;
2020

21-
use crate::node_info::{LookupError, NodeInfo};
21+
use crate::{
22+
defaults::timeouts::DNS_TIMEOUT,
23+
node_info::{LookupError, NodeInfo},
24+
};
2225

2326
/// The n0 testing DNS node origin, for production.
2427
pub const N0_DNS_NODE_ORIGIN_PROD: &str = "dns.iroh.link";
@@ -392,6 +395,27 @@ impl From<TokioResolver> for DnsResolver {
392395
}
393396
}
394397

398+
impl reqwest::dns::Resolve for DnsResolver {
399+
fn resolve(&self, name: reqwest::dns::Name) -> reqwest::dns::Resolving {
400+
let this = self.clone();
401+
let name = name.as_str().to_string();
402+
Box::pin(async move {
403+
let res = this.lookup_ipv4_ipv6(name, DNS_TIMEOUT).await;
404+
match res {
405+
Ok(addrs) => {
406+
let addrs: reqwest::dns::Addrs =
407+
Box::new(addrs.map(|addr| SocketAddr::new(addr, 0)));
408+
Ok(addrs)
409+
}
410+
Err(err) => {
411+
let err: Box<dyn std::error::Error + Send + Sync> = Box::new(err);
412+
Err(err)
413+
}
414+
}
415+
})
416+
}
417+
}
418+
395419
/// TXT records returned from [`DnsResolver::lookup_txt`]
396420
#[derive(Debug, Clone)]
397421
pub struct TxtLookup(pub(crate) hickory_resolver::lookup::TxtLookup);

iroh/examples/dht_discovery.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,11 @@ fn build_discovery(args: Args) -> iroh::discovery::pkarr::dht::Builder {
6464
async fn chat_server(args: Args) -> n0_snafu::Result<()> {
6565
let secret_key = iroh::SecretKey::generate(rand::rngs::OsRng);
6666
let node_id = secret_key.public();
67-
let discovery = build_discovery(args)
68-
.secret_key(secret_key.clone())
69-
.build()?;
67+
let discovery = build_discovery(args);
7068
let endpoint = Endpoint::builder()
7169
.alpns(vec![CHAT_ALPN.to_vec()])
7270
.secret_key(secret_key)
73-
.discovery(Box::new(discovery))
71+
.discovery(discovery)
7472
.bind()
7573
.await?;
7674
let zid = pkarr::PublicKey::try_from(node_id.as_bytes()).e()?.to_z32();
@@ -111,11 +109,11 @@ async fn chat_client(args: Args) -> n0_snafu::Result<()> {
111109
let secret_key = iroh::SecretKey::generate(rand::rngs::OsRng);
112110
let node_id = secret_key.public();
113111
// note: we don't pass a secret key here, because we don't need to publish our address, don't spam the DHT
114-
let discovery = build_discovery(args).build()?;
112+
let discovery = build_discovery(args).no_publish();
115113
// we do not need to specify the alpn here, because we are not going to accept connections
116114
let endpoint = Endpoint::builder()
117115
.secret_key(secret_key)
118-
.discovery(Box::new(discovery))
116+
.discovery(discovery)
119117
.bind()
120118
.await?;
121119
println!("We are {} and connecting to {}", node_id, remote_node_id);

iroh/examples/transfer.rs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -207,25 +207,19 @@ impl EndpointArgs {
207207
let url = self
208208
.pkarr_relay_url
209209
.unwrap_or_else(|| self.env.pkarr_relay_url());
210-
builder = builder
211-
.add_discovery(|secret_key| Some(PkarrPublisher::new(secret_key.clone(), url)));
210+
builder = builder.add_discovery(PkarrPublisher::builder(url));
212211
}
213212

214213
if !self.no_dns_resolve {
215214
let domain = self
216215
.dns_origin_domain
217216
.unwrap_or_else(|| self.env.dns_origin_domain());
218-
builder = builder.add_discovery(|_| Some(DnsDiscovery::new(domain)));
217+
builder = builder.add_discovery(DnsDiscovery::builder(domain));
219218
}
220219

221220
#[cfg(feature = "discovery-local-network")]
222221
if self.mdns {
223-
builder = builder.add_discovery(|secret_key| {
224-
Some(
225-
iroh::discovery::mdns::MdnsDiscovery::new(secret_key.public())
226-
.expect("Failed to create mDNS discovery"),
227-
)
228-
});
222+
builder = builder.discovery_local_network();
229223
}
230224

231225
#[cfg(feature = "test-utils")]

0 commit comments

Comments
 (0)