Skip to content

Commit

Permalink
refactor(iroh-net): remove unused expired field from Endpoint (#1484
Browse files Browse the repository at this point in the history
)

## Description

This field is unused and models a concept from tailscale that we don't
have. This doesn't mean the peer's info is stale, or that it was last
seen long ago, it means that the peer's public key expired based on an
explicit `KeyExpiry *time.Time`.

From their docs

>```
>// Expired is whether this node's key has expired. Control may send
>// this; clients are only allowed to set this from false to true. On
>// the client, this is calculated client-side based on a timestamp sent
>// from control, to avoid clock skew issues.
>```

In tailscale the `Expired` is only set by a `ExpiryManager` based on
that timestamp, from local computation; and only when the `NetworkMap`
has not updated it based on a `MapResponse` requested to the server. We
have none of this concepts.

If we want to define that a peer/endpoint expired, we should do it on
our terms later on if the need arises.

## Notes & open questions

none

## Change checklist

- [x] Self-review.
- [ ] Documentation updates if relevant.
- [ ] Tests if relevant.
  • Loading branch information
divagant-martian committed Sep 17, 2023
1 parent a89078f commit f2f3ead
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 31 deletions.
14 changes: 4 additions & 10 deletions iroh-net/src/magicsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1312,31 +1312,25 @@ impl Actor {
);

match ep.get_send_addrs().await {
Ok((Some(udp_addr), Some(derp_region))) => {
(Some(udp_addr), Some(derp_region)) => {
let res = self.send_raw(udp_addr, transmits.clone()).await;
self.send_derp(derp_region, public_key, Self::split_packets(transmits));

if let Err(err) = res {
warn!("failed to send UDP: {:?}", err);
}
}
Ok((None, Some(derp_region))) => {
(None, Some(derp_region)) => {
self.send_derp(derp_region, public_key, Self::split_packets(transmits));
}
Ok((Some(udp_addr), None)) => {
(Some(udp_addr), None) => {
if let Err(err) = self.send_raw(udp_addr, transmits).await {
warn!("failed to send UDP: {:?}", err);
}
}
Ok((None, None)) => {
(None, None) => {
warn!("no UDP or DERP addr")
}
Err(err) => {
warn!(
"failed to send messages to {}: {:?}",
current_destination, err
);
}
}
}
None => {
Expand Down
23 changes: 2 additions & 21 deletions iroh-net/src/magicsock/endpoint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::{
collections::HashMap,
hash::Hash,
io,
net::{IpAddr, SocketAddr},
time::Duration,
};
Expand Down Expand Up @@ -72,9 +71,6 @@ pub(super) struct Endpoint {
/// Any outstanding "tailscale ping" commands running
pending_cli_pings: Vec<PendingCliPing>,

/// Whether the node has expired.
expired: bool,

sent_ping: HashMap<stun::TransactionId, SentPing>,

/// Last time this endpoint was used.
Expand Down Expand Up @@ -118,7 +114,6 @@ impl Endpoint {
endpoint_state: HashMap::new(),
is_call_me_maybe_ep: HashMap::new(),
pending_cli_pings: Vec::new(),
expired: false,
last_active: Instant::now(),
}
}
Expand Down Expand Up @@ -304,12 +299,6 @@ impl Endpoint {
where
F: Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static,
{
if self.expired {
res.err = Some("endpoint expired".to_string());
cb(res);
return;
}

self.pending_cli_pings.push(PendingCliPing {
res,
cb: Box::new(cb),
Expand Down Expand Up @@ -963,11 +952,7 @@ impl Endpoint {
}
}

pub(crate) async fn get_send_addrs(&mut self) -> io::Result<(Option<SocketAddr>, Option<u16>)> {
if self.expired {
return Err(io::Error::new(io::ErrorKind::Other, "endpoint expired"));
}

pub(crate) async fn get_send_addrs(&mut self) -> (Option<SocketAddr>, Option<u16>) {
let now = Instant::now();
self.last_active = now;
let (udp_addr, derp_region, should_ping) = self.addr_for_send(&now);
Expand All @@ -979,7 +964,7 @@ impl Endpoint {

debug!("sending UDP: {:?}, DERP: {:?}", udp_addr, derp_region);

Ok((udp_addr, derp_region))
(udp_addr, derp_region)
}

fn is_best_addr_valid(&self, instant: Instant) -> bool {
Expand Down Expand Up @@ -1355,7 +1340,6 @@ mod tests {
endpoint_state,
is_call_me_maybe_ep: HashMap::new(),
pending_cli_pings: Vec::new(),
expired: false,
sent_ping: HashMap::new(),
last_active: now,
},
Expand Down Expand Up @@ -1398,7 +1382,6 @@ mod tests {
endpoint_state,
is_call_me_maybe_ep: HashMap::new(),
pending_cli_pings: Vec::new(),
expired: false,
sent_ping: HashMap::new(),
last_active: now,
}
Expand All @@ -1424,7 +1407,6 @@ mod tests {
endpoint_state,
is_call_me_maybe_ep: HashMap::new(),
pending_cli_pings: Vec::new(),
expired: false,
sent_ping: HashMap::new(),
last_active: now,
}
Expand Down Expand Up @@ -1490,7 +1472,6 @@ mod tests {
endpoint_state,
is_call_me_maybe_ep: HashMap::new(),
pending_cli_pings: Vec::new(),
expired: false,
sent_ping: HashMap::new(),
last_active: now,
},
Expand Down

0 comments on commit f2f3ead

Please sign in to comment.