Skip to content

Commit

Permalink
feat(iroh-net): Use the local endpoints info when closing derps (#2082)
Browse files Browse the repository at this point in the history
## Description

When the network has changed we used to close all the derp connections
unconditionally.  This passes in the local endpoints information so
that the derp http actor will only close those derp connections for
which we no longer have the local endpoint.  That's a lot less
disruptive than always closing them.

## Notes & open questions

The relevant rebinding test is still marked flaky.  I've only seen it
pass but will monitor in the flaky tests before considering enabling.

## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
- [x] Tests if relevant.
  • Loading branch information
flub authored Mar 15, 2024
1 parent 29065fd commit 8d86ffc
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 8 deletions.
2 changes: 2 additions & 0 deletions iroh-net/src/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ mod tests {
use super::*;

#[tokio::test]
#[cfg_attr(target_os = "windows", ignore = "flaky")]
async fn test_dns_lookup_basic() {
let _logging = iroh_test::logging::setup();
let res = DNS_RESOLVER.lookup_ip(NA_DERP_HOSTNAME).await.unwrap();
Expand All @@ -140,6 +141,7 @@ mod tests {
}

#[tokio::test]
#[cfg_attr(target_os = "windows", ignore = "flaky")]
async fn test_dns_lookup_ipv4_ipv6() {
let _logging = iroh_test::logging::setup();
let res = lookup_ipv4_ipv6(NA_DERP_HOSTNAME, Duration::from_secs(5))
Expand Down
18 changes: 14 additions & 4 deletions iroh-net/src/magicsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use crate::{
dns::DNS_RESOLVER,
key::{PublicKey, SecretKey, SharedSecret},
magic_endpoint::NodeAddr,
net::{ip::LocalAddresses, netmon, IpFamily},
net::{interfaces, ip::LocalAddresses, netmon, IpFamily},
netcheck, portmapper, stun, AddrInfo,
};

Expand Down Expand Up @@ -1025,7 +1025,7 @@ impl DiscoMessageSource {
}
}

/// Manages currently running endpoint updates.
/// Manages currently running endpoint updates, aka netcheck runs.
///
/// Invariants:
/// - only one endpoint update must be running at a time
Expand Down Expand Up @@ -1861,6 +1861,9 @@ impl Actor {
out
}

/// Refreshes knowledge about our local endpoints.
///
/// In other words, this triggers a netcheck run.
#[instrument(level = "debug", skip_all)]
async fn update_endpoints(&mut self, why: &'static str) {
inc!(MagicsockMetrics, update_endpoints);
Expand Down Expand Up @@ -2235,8 +2238,14 @@ impl Actor {
return;
}

let ifs = Default::default(); // TODO: load actual interfaces from the monitor
self.send_derp_actor(DerpActorMessage::MaybeCloseDerpsOnRebind(ifs));
let ifs = interfaces::State::new().await;
let local_ips = ifs
.interfaces
.values()
.flat_map(|netif| netif.addrs())
.map(|ipnet| ipnet.addr())
.collect();
self.send_derp_actor(DerpActorMessage::MaybeCloseDerpsOnRebind(local_ips));
self.reset_endpoint_states();
}

Expand All @@ -2248,6 +2257,7 @@ impl Actor {
}

/// Closes and re-binds the UDP sockets.
///
/// We consider it successful if we manage to bind the IPv4 socket.
#[instrument(skip_all, fields(me = %self.inner.me))]
async fn rebind(
Expand Down
7 changes: 5 additions & 2 deletions iroh-net/src/magicsock/derp_actor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,8 +514,11 @@ impl DerpActor {
dc
}

/// Called in response to a rebind, closes all DERP connections that don't have a local address in okay_local_ips
/// and pings all those that do.
/// Closes the DERP connections not originating from a local IP address.
///
/// Called in response to a rebind, any DERP connection originating from an address
/// that's not known to be currently a local IP address should be closed. All the other
/// DERP connections are pinged.
async fn maybe_close_derps_on_rebind(&mut self, okay_local_ips: &[IpAddr]) {
let mut tasks = Vec::new();
for url in self
Expand Down
4 changes: 4 additions & 0 deletions iroh-net/src/netcheck.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
//! Checks the network conditions from the current host.
//!
//! Netcheck is responsible for finding out the network conditions of the current host, like
//! whether it is connected to the internet via IPv4 and/or IPv6, what the NAT situation is
//! etc.
//!
//! Based on <https://github.com/tailscale/tailscale/blob/main/net/netcheck/netcheck.go>
use std::collections::{BTreeMap, HashMap};
Expand Down
8 changes: 6 additions & 2 deletions iroh-net/src/netcheck/reportgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1308,12 +1308,16 @@ mod tests {
// /etc/sysctl.conf or /etc/sysctl.d/* to persist this accross reboots.
//
// TODO: Not sure what about IPv6 pings using sysctl.
#[cfg_attr(target_os = "windows", ignore = "flaky")]
#[tokio::test]
async fn test_icmp_probe_eu_derper() {
async fn test_icmpk_probe_eu_derper() {
let _logging_guard = iroh_test::logging::setup();
let pinger = Pinger::new();
let derper = default_eu_derp_node();
let addr = get_derp_addr(&derper, ProbeProto::IcmpV4).await.unwrap();
let addr = get_derp_addr(&derper, ProbeProto::IcmpV4)
.await
.map_err(|err| format!("{err:#}"))
.unwrap();
let probe = Probe::IcmpV4 {
delay: Duration::from_secs(0),
node: Arc::new(derper),
Expand Down

0 comments on commit 8d86ffc

Please sign in to comment.