Skip to content

feat(connlib): prefer relay candidates of same IP version - #8798

Merged
thomaseizinger merged 2 commits into
mainfrom
feat/prefer-relay-same-ip-version
Apr 20, 2025
Merged

feat(connlib): prefer relay candidates of same IP version#8798
thomaseizinger merged 2 commits into
mainfrom
feat/prefer-relay-same-ip-version

Conversation

@thomaseizinger

@thomaseizinger thomaseizinger commented Apr 17, 2025

Copy link
Copy Markdown
Member

When calculating preferences for candidates, str0m currently always prefer IPv6 over IPv4. This is as per the ICE spec. Howver, this can lead to sub-optimal situations when a connection ends up using a TURN server.

TURN allows a client to allocate an IPv4 and an IPv6 address in the same allocation. This makes it possible for e.g. an IPv4-only client to connect to an IPv6-only peer as long as the TURN server runs in dual-stack AND the client requests an IPv6 address in addition to an IPv4 address with the ADDITIONAL-ADDRESS-FAMILY attribute.

Assume that a client sits behind symmetric NAT and therefore needs to rely on a TURN server to communicate with its peers. The TURN server as well as all the peers operate in dual-stack mode.

The current priority calculation will yield a communication path that uses IPv4 to talk to the TURN server (as that is the only one available) but due to the preference ordering of IPv6 over IPv4, will use an IPv6 path to the peer, despite the peer also supporting IPv4.

This isn't a problem per-se but makes our life unnecessarily difficult. Our TURN servers use eBPF to efficiently deal with TURN's channel-data messages. This however is at present only implemented for the IPv4 <> IPv4 and IPv6 <> IPv6 path. Implementing the other paths is possible but complicates the eBPF code because we need to also translate IP headers between versions and not just update the source and destination IPs.

We have since patched str0m to extend the Candidate::relayed constructor to also take a base address which is - similar to the other candidate types - the address the client is sending from in order to use this candidate. In the context of relayed candidates, this is the address the client is using to talk to the TURN server. We can use this information in the candidate's priority calculation to prefer candidates that allow traffic to remain within one IP version, i.e. if the client talks to the TURN server over IPv4, the candidate with an allocated IPv4 address will have a higher priority than the one with the IPv6 address because we are applying a "punishment" factor as part of the local-preference component in the priority formula.

Staying within the same IP version whilst relaying traffic allows our TURN servers to use their eBPF kernel which results in a better UX due to lower latency and higher throughput.

The final candidate ordering is ultimately decided by the controlling ICE agent which in our case is the Firezone Client. Thus, we don't necessarily need to update Gateways in order to test / benefit from this. Building a Client with this patch included should be enough to benefit from this change.

Related: algesten/str0m#640
Related: algesten/str0m#644

Copilot AI review requested due to automatic review settings April 17, 2025 02:07
@vercel

vercel Bot commented Apr 17, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 20, 2025 10:01pm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the relay candidate selection process to prefer candidates that match the IP version used when communicating with the TURN server, reducing complexity in the TURN server's eBPF code.

  • Pass the local SocketAddr to the relay_candidate function and update Candidate::relayed calls accordingly
  • Update tests and Cargo.toml to align with the new API for relayed candidates

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
rust/connlib/snownet/src/allocation.rs Updated candidate creation to pass a local SocketAddr and revised tests
rust/Cargo.toml Updated dependency for str0m to use the branch with the new relay candidate API
Comments suppressed due to low confidence (1)

rust/connlib/snownet/src/allocation.rs:2539

  • [nitpick] Consider using a consistent conversion for the 'local' parameter in Candidate::relayed calls. For example, if other tests use 'PEER1.into()', update this call to use 'PEER1.into()' for consistency.
Event::Invalid(Candidate::relayed(RELAY_ADDR_IP4, Protocol::Udp, PEER1).unwrap()),

@thomaseizinger

Copy link
Copy Markdown
Member Author

I triggered a build for the MacOS client on this branch here: https://github.com/firezone/firezone/actions/runs/14506538180/job/40696898156

jamilbk
jamilbk previously approved these changes Apr 17, 2025

@jamilbk jamilbk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@thomaseizinger

Copy link
Copy Markdown
Member Author

I've tested this locally by disabling either IPv4 or IPv6 completely on my machine and capturing a tcpdump on the Gateway that I connected to.

With this patch, we are now reliably connecting exclusively over IPv4 to the Gateway, even if the Gateway has an IPv6 address and we have an IPv6 allocation. The tcpdump also showed that - apart from STUN requests which keep other paths alive - all the important traffic came from the relay over IPv4.

@thomaseizinger
thomaseizinger force-pushed the feat/prefer-relay-same-ip-version branch from 488474d to 20ce13c Compare April 20, 2025 21:43
@thomaseizinger
thomaseizinger marked this pull request as ready for review April 20, 2025 21:44
@thomaseizinger
thomaseizinger force-pushed the feat/prefer-relay-same-ip-version branch from 20ce13c to 6df8389 Compare April 20, 2025 21:51
@thomaseizinger

Copy link
Copy Markdown
Member Author

@jamilbk I added a changelog entry here, let me know if you agree.

@thomaseizinger

Copy link
Copy Markdown
Member Author

I still see a fair amount of user-space relaying with customers in Europe which I am hoping to fix with this.

@thomaseizinger
thomaseizinger added this pull request to the merge queue Apr 20, 2025
Merged via the queue into main with commit 4c5fd9b Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants