Skip to content

Commit

Permalink
refactor(iroh-net): remove cli ping (#1764)
Browse files Browse the repository at this point in the history 
## Description

Closes #1485 

## Notes & open questions

na

## Change checklist

- [x] Self-review.
- [ ] Documentation updates if relevant.
- [ ] Tests if relevant.
  • Loading branch information
@divagant-martian
divagant-martian authored Nov 1, 2023
1 parent eba3c33 commit 2b70426
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 191 deletions.
28 changes: 0 additions & 28 deletions iroh-net/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,31 +121,3 @@ pub enum LinkType {
/// LTE, 4G, 3G, etc.
Mobile,
}

/// Contains response information for the "tailscale ping" subcommand,
/// saying how Tailscale can reach a Tailscale IP or subnet-routed IP.
/// See tailcfg.PingResponse for a related response that is sent back to control
/// for remote diagnostic pings.
// Based on tailscale/ipnstate
#[derive(Debug, Clone, PartialEq, Default)]
pub struct PingResult {
/// ping destination
pub ip: Option<IpAddr>,
/// Tailscale IP of node handling IP (different for subnet routers)
pub node_ip: Option<IpAddr>,
/// DNS name base or (possibly not unique) hostname
pub node_name: Option<String>,
/// Perceived latency in seconds.
pub latency_seconds: Option<f64>,
/// The ip:port if direct UDP was used. It is not currently set for TSMP pings.
pub endpoint: Option<SocketAddr>,
/// Non-zero DERP region ID if DERP was used. It is not currently set for TSMP pings.
pub derp_region_id: Option<u16>,
/// The three-letter region code corresponding to derp_region_id. It is not currently set for TSMP pings.
pub derp_region_code: Option<String>,
/// Whether the ping request error is due to it being a ping to the local node.
pub is_local_ip: Option<bool>,

/// Did any error occur?
pub err: Option<String>,
}
35 changes: 0 additions & 35 deletions iroh-net/src/magicsock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1315,41 +1315,6 @@ impl MagicSock {
None
}

// TODO
// /// Handles a "ping" CLI query.
// #[instrument(skip_all, fields(me = %self.inner.me))]
// pub async fn ping<F>(&self, peer: config::Node, mut res: config::PingResult, cb: F)
// where
// F: Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static,
// {
// res.node_ip = peer.addresses.get(0).copied();
// res.node_name = match peer.name.as_ref().and_then(|n| n.split('.').next()) {
// Some(name) => {
// // prefer DNS name
// Some(name.to_string())
// }
// None => {
// // else hostname
// Some(peer.hostinfo.hostname.clone())
// }
// };
// let ep = self
// .peer_map
// .read()
// .await
// .endpoint_for_node_key(&peer.key)
// .cloned();
// match ep {
// Some(ep) => {
// ep.cli_ping(res, cb).await;
// }
// None => {
// res.err = Some("unknown peer".to_string());
// cb(res);
// }
// }
// }

/// Sets the connection's preferred local port.
#[instrument(skip_all, fields(me = %self.inner.me))]
pub async fn set_preferred_port(&self, port: u16) {
Expand Down
2 changes: 1 addition & 1 deletion iroh-net/src/magicsock/peer_map.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl PeerMap {
pub fn notify_shutdown(&self) {
let mut inner = self.inner.lock();
for (_, ep) in inner.endpoints_mut() {
ep.stop_and_reset();
ep.reset();
}
}

Expand Down
146 changes: 19 additions & 127 deletions iroh-net/src/magicsock/peer_map/endpoint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ use std::{
time::{Duration, Instant},
};

use futures::future::BoxFuture;
use iroh_metrics::inc;
use rand::seq::IteratorRandom;
use serde::{Deserialize, Serialize};
use tokio::sync::mpsc;
use tracing::{debug, info, instrument, trace, warn};

use crate::{
config, disco, key::PublicKey, magic_endpoint::AddrInfo, magicsock::Timer,
disco, key::PublicKey, magic_endpoint::AddrInfo, magicsock::Timer,
net::ip::is_unicast_link_local, stun, util::derp_only_mode, PeerAddr,
};

Expand Down Expand Up @@ -93,8 +92,6 @@ pub(super) struct Endpoint {
best_addr: BestAddr,
/// [`EndpointState`] for this peer's direct addresses.
direct_addr_state: HashMap<IpPort, EndpointState>,
/// Any outstanding "tailscale ping" commands running
pending_cli_pings: Vec<PendingCliPing>,
sent_ping: HashMap<stun::TransactionId, SentPing>,
/// Last time this peer was used.
///
Expand All @@ -103,13 +100,6 @@ pub(super) struct Endpoint {
last_used: Option<Instant>,
}

#[derive(derive_more::Debug)]
pub struct PendingCliPing {
pub res: config::PingResult,
#[debug("cb: Box<..>")]
pub cb: Box<dyn Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static>,
}

#[derive(Debug)]
pub(super) struct Options {
pub(super) public_key: PublicKey,
Expand Down Expand Up @@ -138,7 +128,6 @@ impl Endpoint {
best_addr: Default::default(),
sent_ping: HashMap::new(),
direct_addr_state: HashMap::new(),
pending_cli_pings: Vec::new(),
last_used: options.active.then(Instant::now),
}
}
Expand Down Expand Up @@ -310,64 +299,6 @@ impl Endpoint {
}
}

/// Starts a ping for the "ping" command.
/// `res` is value to call cb with, already partially filled.
#[allow(unused, clippy::unused_async)]
pub(super) async fn cli_ping<F>(
&mut self,
mut res: config::PingResult,
cb: F,
) -> Vec<PingAction>
where
F: Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static,
{
self.pending_cli_pings.push(PendingCliPing {
res,
cb: Box::new(cb),
});

let now = Instant::now();
let mut msgs = Vec::new();
let (udp_addr, derp_region, _should_ping) = self.addr_for_send(&now);
if let Some(derp_region) = derp_region {
if let Some(msg) = self.start_ping(SendAddr::Derp(derp_region), DiscoPingPurpose::Cli) {
msgs.push(PingAction::SendPing(msg));
}
}
if let Some(udp_addr) = udp_addr {
if let best_addr::State::Valid(_) = self.best_addr.state(now) {
// Already have an active session, so just ping the address we're using.
// Otherwise "tailscale ping" results to a node on the local network
// can look like they're bouncing between, say 10.0.0.0/9 and the peer's
// IPv6 address, both 1ms away, and it's random who replies first.
if let Some(msg) = self.start_ping(SendAddr::Udp(udp_addr), DiscoPingPurpose::Cli) {
msgs.push(PingAction::SendPing(msg));
}
} else {
let eps: Vec<_> = self.direct_addr_state.keys().cloned().collect();
for ep in eps {
if let Some(msg) =
self.start_ping(SendAddr::Udp(ep.into()), DiscoPingPurpose::Cli)
{
msgs.push(PingAction::SendPing(msg));
}
}
}
}
// NOTE: this should be checked for before dialing
// In our current set up, there is no way to report an error.
// TODO(ramfox): figure out method of reporting dial errors this far down into the stack
if udp_addr.is_none() && derp_region.is_none() {
tracing::error!(
"unable to ping endpoint {} {:?}, no UDP or DERP addresses known.",
self.id,
self.public_key
);
}

msgs
}

/// Cleanup the expired ping for the passed in txid.
#[instrument("disco", skip_all, fields(peer = %self.public_key.fmt_short()))]
pub(super) fn ping_timeout(&mut self, txid: stun::TransactionId) {
Expand Down Expand Up @@ -428,29 +359,27 @@ impl Endpoint {
trace!(%to, tx = %hex::encode(tx_id), ?purpose, "record ping sent");

let now = Instant::now();
if purpose != DiscoPingPurpose::Cli {
let mut ep_found = false;
match to {
SendAddr::Udp(addr) => {
if let Some(st) = self.direct_addr_state.get_mut(&addr.into()) {
st.last_ping.replace(now);
ep_found = true
}
let mut ep_found = false;
match to {
SendAddr::Udp(addr) => {
if let Some(st) = self.direct_addr_state.get_mut(&addr.into()) {
st.last_ping.replace(now);
ep_found = true
}
SendAddr::Derp(region) => {
if let Some((home_derp, relay_state)) = self.derp_region.as_mut() {
if *home_derp == region {
relay_state.last_ping.replace(now);
ep_found = true
}
}
SendAddr::Derp(region) => {
if let Some((home_derp, relay_state)) = self.derp_region.as_mut() {
if *home_derp == region {
relay_state.last_ping.replace(now);
ep_found = true
}
}
}
if !ep_found {
// Shouldn't happen. But don't ping an endpoint that's not active for us.
warn!(%to, ?purpose, "unexpected attempt to ping no longer live endpoint");
return;
}
}
if !ep_found {
// Shouldn't happen. But don't ping an endpoint that's not active for us.
warn!(%to, ?purpose, "unexpected attempt to ping no longer live endpoint");
return;
}

let id = self.id;
Expand Down Expand Up @@ -582,7 +511,7 @@ impl Endpoint {

/// Clears all the endpoint's p2p state, reverting it to a DERP-only endpoint.
#[instrument(skip_all, fields(peer = %self.public_key.fmt_short()))]
fn reset(&mut self) {
pub(super) fn reset(&mut self) {
self.last_full_ping = None;
self.best_addr
.clear(ClearReason::Reset, self.derp_region.is_some());
Expand Down Expand Up @@ -764,28 +693,6 @@ impl Endpoint {
},
}

if !self.pending_cli_pings.is_empty() {
let ep = sp.to;
// FIXME: this creates a deadlock as it needs to interact with the run loop in the conn::Actor
// let region_code = self.get_derp_region(region_id).await.map(|r| r.region_code);

for PendingCliPing { mut res, cb } in self.pending_cli_pings.drain(..) {
res.latency_seconds = Some(latency.as_secs_f64());
match ep {
SendAddr::Udp(addr) => {
res.endpoint = Some(addr);
}
SendAddr::Derp(region) => {
res.derp_region_id = Some(region);
// res.derp_region_code = region_code.clone();
}
}
tokio::task::spawn(async move {
cb(res).await;
});
}
}

// Promote this pong response to our current best address if it's lower latency.
// TODO(bradfitz): decide how latency vs. preference order affects decision
if let SendAddr::Udp(to) = sp.to {
Expand Down Expand Up @@ -888,15 +795,6 @@ impl Endpoint {
self.last_used = Some(now);
}

/// Stops timers associated with de and resets its state back to zero.
/// It's called when a discovery endpoint is no longer present in the
/// NetworkMap, or when magicsock is transitioning from running to
/// stopped state (via `set_secret_key(None)`).
pub(super) fn stop_and_reset(&mut self) {
self.reset();
self.pending_cli_pings.clear();
}

pub(super) fn last_ping(&self, addr: &SendAddr) -> Option<Instant> {
match addr {
SendAddr::Udp(addr) => self
Expand Down Expand Up @@ -1194,8 +1092,6 @@ pub(super) struct SentPing {
pub enum DiscoPingPurpose {
/// The purpose of a ping was to see if a path was valid.
Discovery,
/// The user is running "tailscale ping" from the CLI. These types of pings can go over DERP.
Cli,
/// Ping to ensure the current route is still valid.
StayinAlive,
}
Expand Down Expand Up @@ -1319,7 +1215,6 @@ mod tests {
now + Duration::from_secs(100),
),
direct_addr_state: endpoint_state,
pending_cli_pings: Vec::new(),
sent_ping: HashMap::new(),
last_used: Some(now),
},
Expand All @@ -1344,7 +1239,6 @@ mod tests {
derp_region: Some((0, relay_state)),
best_addr: BestAddr::default(),
direct_addr_state: HashMap::default(),
pending_cli_pings: Vec::new(),
sent_ping: HashMap::new(),
last_used: Some(now),
}
Expand All @@ -1363,7 +1257,6 @@ mod tests {
derp_region: new_relay_and_state(Some(0)),
best_addr: BestAddr::default(),
direct_addr_state: endpoint_state,
pending_cli_pings: Vec::new(),
sent_ping: HashMap::new(),
last_used: Some(now),
}
Expand Down Expand Up @@ -1403,7 +1296,6 @@ mod tests {
expired,
),
direct_addr_state: endpoint_state,
pending_cli_pings: Vec::new(),
sent_ping: HashMap::new(),
last_used: Some(now),
},
Expand Down

0 comments on commit 2b70426

Please sign in to comment.