diff --git a/rust/connlib/snownet/src/lib.rs b/rust/connlib/snownet/src/lib.rs index f277a22a1..b944a89b8 100644 --- a/rust/connlib/snownet/src/lib.rs +++ b/rust/connlib/snownet/src/lib.rs @@ -15,4 +15,3 @@ pub use node::{ Transmit, HANDSHAKE_TIMEOUT, }; pub use stats::{ConnectionStats, NodeStats}; -pub use str0m::Candidate; diff --git a/rust/connlib/snownet/src/node.rs b/rust/connlib/snownet/src/node.rs index 7ec84b061..b7a088dc9 100644 --- a/rust/connlib/snownet/src/node.rs +++ b/rust/connlib/snownet/src/node.rs @@ -1144,7 +1144,7 @@ fn add_local_candidate( if candidate.kind() == CandidateKind::ServerReflexive { pending_events.push_back(Event::NewIceCandidate { connection: id, - candidate, + candidate: candidate.to_sdp_string(), }); return; } @@ -1154,7 +1154,7 @@ fn add_local_candidate( if is_new { pending_events.push_back(Event::NewIceCandidate { connection: id, - candidate, + candidate: candidate.to_sdp_string(), }) } } @@ -1170,7 +1170,7 @@ fn remove_local_candidate( if candidate.kind() == CandidateKind::ServerReflexive { pending_events.push_back(Event::NewIceCandidate { connection: id, - candidate: candidate.clone(), + candidate: candidate.to_sdp_string(), }); return; } @@ -1180,7 +1180,7 @@ fn remove_local_candidate( if was_present { pending_events.push_back(Event::InvalidateIceCandidate { connection: id, - candidate: candidate.clone(), + candidate: candidate.to_sdp_string(), }) } } @@ -1209,7 +1209,7 @@ pub enum Event { /// Candidates are in SDP format although this may change and should be considered an implementation detail of the application. NewIceCandidate { connection: TId, - candidate: Candidate, + candidate: String, }, /// We invalidated a candidate for this connection and ask to signal that to the remote party. @@ -1217,7 +1217,7 @@ pub enum Event { /// Candidates are in SDP format although this may change and should be considered an implementation detail of the application. InvalidateIceCandidate { connection: TId, - candidate: Candidate, + candidate: String, }, ConnectionEstablished(TId), diff --git a/rust/connlib/snownet/tests/lib.rs b/rust/connlib/snownet/tests/lib.rs index 9cf1093a7..343ad6a3b 100644 --- a/rust/connlib/snownet/tests/lib.rs +++ b/rust/connlib/snownet/tests/lib.rs @@ -99,7 +99,9 @@ fn only_generate_candidate_event_after_answer() { assert!(iter::from_fn(|| alice.poll_event()).any(|ev| ev == Event::NewIceCandidate { connection: 1, - candidate: Candidate::host(local_candidate, Protocol::Udp).unwrap() + candidate: Candidate::host(local_candidate, Protocol::Udp) + .unwrap() + .to_sdp_string() })); } diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index bce137f78..fd00bfdf1 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -18,7 +18,7 @@ use ip_packet::{IpPacket, MutableIpPacket, Packet as _}; use itertools::Itertools; use crate::peer::GatewayOnClient; -use crate::utils::{self, earliest, turn, Candidates}; +use crate::utils::{self, earliest, turn}; use crate::{ClientEvent, ClientTunnel, Tun}; use domain::base::Message; use secrecy::{ExposeSecret as _, Secret}; @@ -868,8 +868,8 @@ impl ClientState { fn drain_node_events(&mut self) { let mut resources_changed = false; // Track this separately to batch together `ResourcesChanged` events. - let mut added_ice_candidates = BTreeMap::::default(); - let mut removed_ice_candidates = BTreeMap::::default(); + let mut added_ice_candidates = BTreeMap::>::default(); + let mut removed_ice_candidates = BTreeMap::>::default(); while let Some(event) = self.node.poll_event() { match event { @@ -884,7 +884,7 @@ impl ClientState { added_ice_candidates .entry(connection) .or_default() - .push(candidate); + .insert(candidate); } snownet::Event::InvalidateIceCandidate { connection, @@ -893,7 +893,7 @@ impl ClientState { removed_ice_candidates .entry(connection) .or_default() - .push(candidate); + .insert(candidate); } snownet::Event::ConnectionEstablished(id) => { self.update_site_status_by_gateway(&id, Status::Online); @@ -913,7 +913,7 @@ impl ClientState { self.buffered_events .push_back(ClientEvent::AddedIceCandidates { conn_id, - candidates: candidates.serialize(), + candidates, }) } @@ -921,7 +921,7 @@ impl ClientState { self.buffered_events .push_back(ClientEvent::RemovedIceCandidates { conn_id, - candidates: candidates.serialize(), + candidates, }) } } diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index 349455fc1..22d21c2b8 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -1,6 +1,6 @@ use crate::peer::ClientOnGateway; use crate::peer_store::PeerStore; -use crate::utils::{earliest, Candidates}; +use crate::utils::earliest; use crate::{GatewayEvent, GatewayTunnel}; use anyhow::{bail, Context}; use boringtun::x25519::PublicKey; @@ -355,8 +355,8 @@ impl GatewayState { Some(_) => {} } - let mut added_ice_candidates = BTreeMap::::default(); - let mut removed_ice_candidates = BTreeMap::::default(); + let mut added_ice_candidates = BTreeMap::>::default(); + let mut removed_ice_candidates = BTreeMap::>::default(); while let Some(event) = self.node.poll_event() { match event { @@ -370,7 +370,7 @@ impl GatewayState { added_ice_candidates .entry(connection) .or_default() - .push(candidate); + .insert(candidate); } snownet::Event::InvalidateIceCandidate { connection, @@ -379,7 +379,7 @@ impl GatewayState { removed_ice_candidates .entry(connection) .or_default() - .push(candidate); + .insert(candidate); } snownet::Event::ConnectionEstablished(_) => {} } @@ -389,7 +389,7 @@ impl GatewayState { self.buffered_events .push_back(GatewayEvent::AddedIceCandidates { conn_id, - candidates: candidates.serialize(), + candidates, }) } @@ -397,7 +397,7 @@ impl GatewayState { self.buffered_events .push_back(GatewayEvent::RemovedIceCandidates { conn_id, - candidates: candidates.serialize(), + candidates, }) } } diff --git a/rust/connlib/tunnel/src/utils.rs b/rust/connlib/tunnel/src/utils.rs index 88b7551d2..72bce5139 100644 --- a/rust/connlib/tunnel/src/utils.rs +++ b/rust/connlib/tunnel/src/utils.rs @@ -3,7 +3,7 @@ use connlib_shared::messages::{Relay, RelayId}; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use itertools::Itertools; use snownet::RelaySocket; -use std::{cmp::Ordering, collections::BTreeSet, net::SocketAddr, time::Instant}; +use std::{collections::BTreeSet, net::SocketAddr, time::Instant}; pub fn turn(relays: &[Relay]) -> BTreeSet<(RelayId, RelaySocket, String, String, String)> { relays @@ -85,55 +85,3 @@ pub(crate) fn ipv6(ip: IpNetwork) -> Option { IpNetwork::V6(v6) => Some(v6), } } - -/// Helper container to aggregate candidates and emit them in sorted order, starting with the highest-priority one (host candidates). -/// -/// Whilst no fatal in theory, emitting candidates in the wrong order can cause temporary connectivity problems. -/// `str0m` needs to "replace" candidates when it receives better ones which can cancel in-flight STUN requests. -/// By sorting the candidates by priority, we try the best ones first, being symphatic to the ICE algorithm. -#[derive(Default)] -pub(crate) struct Candidates(Vec); - -impl Candidates { - pub(crate) fn push(&mut self, candidate: snownet::Candidate) { - self.0.push(candidate) - } - - pub(crate) fn serialize(mut self) -> BTreeSet { - self.0.sort_by(priority_desc); - - self.0.into_iter().map(|c| c.to_sdp_string()).collect() - } -} - -fn priority_desc(c1: &snownet::Candidate, c2: &snownet::Candidate) -> Ordering { - c1.prio().cmp(&c2.prio()).reverse() -} - -#[cfg(test)] -mod tests { - use super::*; - use std::net::{Ipv4Addr, SocketAddrV4}; - - #[test] - fn sorts_candidates_by_priority() { - let mut candidates = vec![ - snownet::Candidate::host( - SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1)), - "udp", - ) - .unwrap(), - snownet::Candidate::server_reflexive( - SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 2)), - SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1)), - "udp", - ) - .unwrap(), - ]; - - candidates.sort_by(priority_desc); - - assert_eq!(candidates[0].kind().to_string(), "host"); - assert_eq!(candidates[1].kind().to_string(), "srflx"); - } -} diff --git a/rust/tests/snownet-tests/src/main.rs b/rust/tests/snownet-tests/src/main.rs index edb6e9564..bbb2526e2 100644 --- a/rust/tests/snownet-tests/src/main.rs +++ b/rust/tests/snownet-tests/src/main.rs @@ -387,7 +387,7 @@ impl Eventloop { }) => { return Poll::Ready(Ok(Event::SignalIceCandidate { conn: connection, - candidate: candidate.to_sdp_string(), + candidate, })) } Some(snownet::Event::ConnectionEstablished(conn)) => {