Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(iroh-net): remove cli ping #1764

Merged
merged 2 commits into from
Nov 1, 2023
Merged

refactor(iroh-net): remove cli ping #1764

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7091e7
remove cli ping
divagant-martian Nov 1, 2023
8534b4d
make reset pub(super)
divagant-martian Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions iroh-net/src/config.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 fn reset(&mut self) {
divagant-martian marked this conversation as resolved.
Show resolved Hide resolved
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
Loading