Skip to content

Commit

Permalink
fix(tests): For DNS discovery only use a local DNS server (#2598)
Browse files Browse the repository at this point in the history
## Description

Because the Node builder by default configures DNS discovery the tests
all do a lookup.  This went to the staging DNS server, but even so, we
hit 429 Too Many Requests on this sometimes.

Instead this configures a domain under `.test.`.  This domain is
reserved for tests and any DNS server should return an emtpy response
without going through upstream DNS servers - unless the records were
directly added to the DNS server receiving the query in which case it
will respond.

So now all our default DNS discovery just doesn't discover anything,
which is fine as all tests have other mechanisms to find the nodes.
The tests that do use DNS discovery run their own local DNS server for
this, which is also switched to `.test.`, because that's the right
domain to use for this.

## Breaking Changes

When using `iroh::node::Node` in tests, when `cfg(test)` is enabled, the
default node discovery will no longer work. You must explicitly
configure
the node discovery, options to use a test-local node discovery or a
staging node discovery are described in the API docs.

## Notes & open questions

I feel like we should also try this with the relay server.  That will
probably be a bit harder and it is not an issue for me right now.

I actually need this for the quinn11 branch, but the fix is so generic
that it makes more sense to do on main and merge it back.

## Change checklist

- [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.
  • Loading branch information
flub authored Aug 9, 2024
1 parent 7494566 commit 5eee643
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 19 deletions.
27 changes: 23 additions & 4 deletions iroh-net/src/discovery/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ use crate::{
pub const N0_DNS_NODE_ORIGIN_PROD: &str = "dns.iroh.link";
/// The n0 testing DNS node origin, for testing.
pub const N0_DNS_NODE_ORIGIN_STAGING: &str = "staging-dns.iroh.link";
/// Testing DNS node origin, must run server from [`crate::test_utils::DnsPkarrServer`].
#[cfg(any(test, feature = "test-utils"))]
pub const TEST_DNS_NODE_ORIGIN: &str = "dns.iroh.test";

const DNS_STAGGERING_MS: &[u64] = &[200, 300];

/// DNS node discovery
Expand Down Expand Up @@ -41,21 +45,36 @@ pub struct DnsDiscovery {
}

impl DnsDiscovery {
/// Create a new DNS discovery.
/// Creates a new DNS discovery.
pub fn new(origin_domain: String) -> Self {
Self { origin_domain }
}

/// Create a new DNS discovery which uses the [`N0_DNS_NODE_ORIGIN_PROD`] origin domain and in testing
/// uses [`N0_DNS_NODE_ORIGIN_STAGING`].
/// Creates a new DNS discovery using the `iroh.link` domain.
///
/// This uses the [`N0_DNS_NODE_ORIGIN_PROD`] domain.
///
/// # Usage during tests
///
/// When `cfg(test)` is enabled or when using the `test-utils` cargo feature the
/// [`TEST_DNS_NODE_ORIGIN`] is used.
///
/// Note that the `iroh.test` domain is not integrated with the global DNS network and
/// thus node discovery is effectively disabled. To use node discovery in a test use
/// the [`crate::test_utils::DnsPkarrServer`] in the test and configure it as a
/// custom discovery mechanism.
///
/// For testing it is also possible to use the [`N0_DNS_NODE_ORIGIN_STAGING`] domain
/// with [`DnsDiscovery::new`]. This would then use a hosted discovery service again,
/// but for testing purposes.
pub fn n0_dns() -> Self {
#[cfg(not(any(test, feature = "test-utils")))]
{
Self::new(N0_DNS_NODE_ORIGIN_PROD.to_string())
}
#[cfg(any(test, feature = "test-utils"))]
{
Self::new(N0_DNS_NODE_ORIGIN_STAGING.to_string())
Self::new(TEST_DNS_NODE_ORIGIN.to_string())
}
}
}
Expand Down
13 changes: 9 additions & 4 deletions iroh-net/src/discovery/pkarr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,17 @@ impl PublisherService {
loop {
if let Some(info) = self.watcher.get() {
if let Err(err) = self.publish_current(info).await {
warn!(?err, url = %self.pkarr_client.pkarr_relay_url , "Failed to publish to pkarr");
failed_attempts += 1;
// Retry after increasing timeout
republish
.as_mut()
.reset(Instant::now() + Duration::from_secs(failed_attempts));
let retry_after = Duration::from_secs(failed_attempts);
republish.as_mut().reset(Instant::now() + retry_after);
warn!(
err = %format!("{err:#}"),
url = %self.pkarr_client.pkarr_relay_url ,
?retry_after,
%failed_attempts,
"Failed to publish to pkarr",
);
} else {
failed_attempts = 0;
// Republish after fixed interval
Expand Down
6 changes: 5 additions & 1 deletion iroh-net/src/relay/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@ use crate::defaults::DEFAULT_STUN_PORT;

use super::RelayUrl;

/// Configuration options for the relay servers of the magic endpoint.
/// Configuration of the relay servers for an [`Endpoint`].
///
/// [`Endpoint`]: crate::endpoint::Endpoint
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum RelayMode {
/// Disable relay servers completely.
Disabled,
/// Use the default relay map, with production relay servers from n0.
///
/// See [`crate::defaults::prod`] for the severs used.
Default,
/// Use the staging relay servers from n0.
Staging,
Expand Down
2 changes: 1 addition & 1 deletion iroh-net/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub(crate) mod dns_and_pkarr_servers {
impl DnsPkarrServer {
/// Run DNS and Pkarr servers on localhost.
pub async fn run() -> anyhow::Result<Self> {
Self::run_with_origin("dns.example".to_string()).await
Self::run_with_origin("dns.iroh.test".to_string()).await
}

/// Run DNS and Pkarr servers on localhost with the specified `node_origin` domain.
Expand Down
65 changes: 56 additions & 9 deletions iroh/src/node/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,23 @@ pub enum DocsStorage {
/// Blob store implementations are available in [`iroh_blobs::store`].
/// Document store implementations are available in [`iroh_docs::store`].
///
/// Everything else is optional.
/// Everything else is optional, with some sensible defaults.
///
/// The default **relay servers** are hosted by [number 0] on the `iroh.network` domain. To
/// customise this use the [`Builder::relay_mode`] function.
///
/// For **node discovery** the default is to use the [number 0] hosted DNS server hosted on
/// `iroh.link`. To customise this use the [`Builder::node_discovery`] function.
///
/// Note that some defaults change when running using `cfg(test)`, see the individual
/// methods for details.
///
/// Finally you can create and run the node by calling [`Builder::spawn`].
///
/// The returned [`Node`] is awaitable to know when it finishes. It can be terminated
/// using [`Node::shutdown`].
///
/// [number 0]: https://n0.computer
#[derive(derive_more::Debug)]
pub struct Builder<D>
where
Expand Down Expand Up @@ -125,13 +136,37 @@ impl StorageConfig {
}

/// Configuration for node discovery.
///
/// Node discovery enables connecting to other peers by only the [`NodeId`]. This usually
/// works by the nodes publishing their [`RelayUrl`] and/or their direct addresses to some
/// publicly available service.
///
/// [`NodeId`]: crate::base::key::NodeId
/// [`RelayUrl`]: crate::base::node_addr::RelayUrl
#[derive(Debug, Default)]
pub enum DiscoveryConfig {
/// Use no node discovery mechanism.
None,
/// Use the default discovery mechanism.
///
/// This enables the [`DnsDiscovery`] service.
/// This uses two discovery services concurrently:
///
/// - It publishes to a pkarr service operated by [number 0] which makes the information
/// available via DNS in the `iroh.link` domain.
///
/// - It uses an mDNS-like system to announce itself on the local network.
///
/// # Usage during tests
///
/// Note that the default changes when compiling with `cfg(test)` or the `test-utils`
/// cargo feature from [iroh-net] is enabled. In this case only the Pkarr/DNS service
/// is used, but on the `iroh.test` domain. This domain is not integrated with the
/// global DNS network and thus node discovery is effectively disabled. To use node
/// discovery in a test use the [`iroh_net::test_utils::DnsPkarrServer`] in the test and
/// configure it here as a custom discovery mechanism ([`DiscoveryConfig::Custom`]).
///
/// [number 0]: https://n0.computer
/// [iroh-net]: crate::net
#[default]
Default,
/// Use a custom discovery mechanism.
Expand Down Expand Up @@ -354,18 +389,30 @@ where
/// establish connections between peers by being an initial relay for traffic while
/// assisting in holepunching to establish a direct connection between peers.
///
/// When using [RelayMode::Custom], the provided `relay_map` must contain at least one
/// configured relay node. If an invalid [`iroh_net::relay::RelayMode`]
/// is provided [`Self::spawn`] will result in an error.
pub fn relay_mode(mut self, dm: RelayMode) -> Self {
self.relay_mode = dm;
/// When using [`RelayMode::Custom`], the provided `relay_map` must contain at least one
/// configured relay node. If an invalid [`iroh_net::relay::RelayMode`] is provided
/// [`Self::spawn`] will result in an error.
///
/// # Usage during tests
///
/// Note that while the default is [`RelayMode::Default`], when using `cfg(test)` or
/// when the `test-utils` cargo feature [`RelayMode::Staging`] is the default.
pub fn relay_mode(mut self, relay_mode: RelayMode) -> Self {
self.relay_mode = relay_mode;
self
}

/// Sets the node discovery mechanism.
///
/// The default is [`DiscoveryConfig::Default`]. Use [`DiscoveryConfig::Custom`] to pass a
/// custom [`Discovery`].
/// Node discovery enables connecting to other peers by only the [`NodeId`]. This
/// usually works by the nodes publishing their [`RelayUrl`] and/or their direct
/// addresses to some publicly available service.
///
/// See [`DiscoveryConfig::default`] for the defaults, note that the defaults change
/// when using `cfg(test)`.
///
/// [`NodeId`]: crate::base::key::NodeId
/// [`RelayUrl`]: crate::base::node_addr::RelayUrl
pub fn node_discovery(mut self, config: DiscoveryConfig) -> Self {
self.node_discovery = config;
self
Expand Down

0 comments on commit 5eee643

Please sign in to comment.