github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 18:18:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

revert: emit candidates in reverse-priority order (#6246)

Browse Source 
This ended up not fixing anything and the order is now guaranteed to be
deterministic due to an upstream change.

Related: https://github.com/algesten/str0m/pull/557.
Reverts: #6200.
This commit is contained in:
Thomas Eizinger
2024-08-13 00:55:31 +01:00
committed by GitHub
parent cd84c634ff
commit a272776171
7 changed files with 25 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
rust/connlib/snownet/src/lib.rs
View File

@@ -15,4 +15,3 @@ pub use node::{
Transmit, HANDSHAKE_TIMEOUT,
};
pub use stats::{ConnectionStats, NodeStats};
pub use str0m::Candidate;

12
rust/connlib/snownet/src/node.rs
View File

@@ -1144,7 +1144,7 @@ fn add_local_candidate<TId>(
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<TId>(
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<TId>(
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<TId>(
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<TId> {
/// 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<TId> {
/// 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),

4
rust/connlib/snownet/tests/lib.rs
View File

@@ -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()
}));
}

14
rust/connlib/tunnel/src/client.rs
View File

@@ -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::<GatewayId, Candidates>::default();
let mut removed_ice_candidates = BTreeMap::<GatewayId, Candidates>::default();
let mut added_ice_candidates = BTreeMap::<GatewayId, BTreeSet<String>>::default();
let mut removed_ice_candidates = BTreeMap::<GatewayId, BTreeSet<String>>::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,
})
}
}

14
rust/connlib/tunnel/src/gateway.rs
View File

@@ -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::<ClientId, Candidates>::default();
let mut removed_ice_candidates = BTreeMap::<ClientId, Candidates>::default();
let mut added_ice_candidates = BTreeMap::<ClientId, BTreeSet<String>>::default();
let mut removed_ice_candidates = BTreeMap::<ClientId, BTreeSet<String>>::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,
})
}
}

54
rust/connlib/tunnel/src/utils.rs
View File

@@ -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<Ipv6Network> {
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<snownet::Candidate>);
impl Candidates {
pub(crate) fn push(&mut self, candidate: snownet::Candidate) {
self.0.push(candidate)
}
pub(crate) fn serialize(mut self) -> BTreeSet<String> {
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");
}
}

2
rust/tests/snownet-tests/src/main.rs
View File

@@ -387,7 +387,7 @@ impl<T> Eventloop<T> {
}) => {
return Poll::Ready(Ok(Event::SignalIceCandidate {
conn: connection,
candidate: candidate.to_sdp_string(),
candidate,
}))
}
Some(snownet::Event::ConnectionEstablished(conn)) => {