fix(snownet): don't allow duplicate server-reflexive candidates#7334
Merged
thomaseizinger merged 3 commits intomainfrom Nov 14, 2024
Merged
fix(snownet): don't allow duplicate server-reflexive candidates#7334thomaseizinger merged 3 commits intomainfrom
thomaseizinger merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Member
Author
|
@jamilbk Let me know what you think about this approach. I am open to other ideas as well. Let me know if you need me to provide more details. |
Member
Author
|
Want to add a changelog entry here. |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Nov 19, 2024
In the latest version, we added a warning log to str0m when the maximum number of candidate pairs is exceeded: algesten/str0m#587. We only ever add the candidates of a single relay to an agent (2 candidates), plus at most 2 server-reflexive candidates and at most 2 host candidates. Unless there is a bug like what we fixed in #7334, exceeding the default number of candidate _pairs_ (100) should never happen. In case it does, the newly added `warn` log in `str0m` will trigger a Sentry alert.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #7163, we introduced a shared cache of server-reflexive candidates within a
snownet::Node. What we unfortunately overlooked is that if a node (i.e. a client or a gateway) is behind symmetric NAT, then we will repeatedly create "new" server-reflexive candiates, thereby filling up this cache.This cache is used to initialise the agents with local candidates, which manifests in us sending dozens if not hundreds of candidates to the other party. Whilst not harmful in itself, it does create quite a lot of spam. To fix this, we introduce a limit of only keeping around 1 server-reflexive candidate per IP version, i.e. only 1 IPv4 and IPv6 address.
At present,
connlibonly supports a single egress interface meaning for now, we are fine with making this assumption.In case we encounter a new candidate of the same kind and same IP version, we evict the old one and replace it with the new one. Thus, for subsequent connections, only the new candidate is used.