From 8d86ffcf20abf104a68f122a05158844dc896052 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 15 Mar 2024 12:20:38 +0100 Subject: [PATCH] feat(iroh-net): Use the local endpoints info when closing derps (#2082) ## 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. --- iroh-net/src/dns.rs | 2 ++ iroh-net/src/magicsock.rs | 18 ++++++++++++++---- iroh-net/src/magicsock/derp_actor.rs | 7 +++++-- iroh-net/src/netcheck.rs | 4 ++++ iroh-net/src/netcheck/reportgen.rs | 8 ++++++-- 5 files changed, 31 insertions(+), 8 deletions(-) diff --git a/iroh-net/src/dns.rs b/iroh-net/src/dns.rs index ac675a7c66..80e0abbecb 100644 --- a/iroh-net/src/dns.rs +++ b/iroh-net/src/dns.rs @@ -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(); @@ -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)) diff --git a/iroh-net/src/magicsock.rs b/iroh-net/src/magicsock.rs index 84f088b3cb..20ef8a22dc 100644 --- a/iroh-net/src/magicsock.rs +++ b/iroh-net/src/magicsock.rs @@ -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, }; @@ -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 @@ -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); @@ -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(); } @@ -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( diff --git a/iroh-net/src/magicsock/derp_actor.rs b/iroh-net/src/magicsock/derp_actor.rs index 20bc18bf28..55395b9c49 100644 --- a/iroh-net/src/magicsock/derp_actor.rs +++ b/iroh-net/src/magicsock/derp_actor.rs @@ -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 diff --git a/iroh-net/src/netcheck.rs b/iroh-net/src/netcheck.rs index 0c52251fe5..e06a932ea3 100644 --- a/iroh-net/src/netcheck.rs +++ b/iroh-net/src/netcheck.rs @@ -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 use std::collections::{BTreeMap, HashMap}; diff --git a/iroh-net/src/netcheck/reportgen.rs b/iroh-net/src/netcheck/reportgen.rs index 7695392f73..79ff30d41c 100644 --- a/iroh-net/src/netcheck/reportgen.rs +++ b/iroh-net/src/netcheck/reportgen.rs @@ -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),