build(rust): depend on our boringtun fork - #7120
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ReactorScram
approved these changes
Oct 22, 2024
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 23, 2024 07:08
4100593 to
6d4489f
Compare
thomaseizinger
commented
Oct 23, 2024
jamilbk
reviewed
Oct 23, 2024
jamilbk
left a comment
jamilbk
left a comment
Member
There was a problem hiding this comment.
Don't really have anything to add here
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 24, 2024 00:58
6d4489f to
4766ede
Compare
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 24, 2024 04:31
4766ede to
d6de041
Compare
Member
Author
|
We should have done this much sooner. Modelling time correctly in |
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 24, 2024 22:16
ffefff6 to
b75fc57
Compare
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 24, 2024 23:54
67954fa to
22187ca
Compare
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 25, 2024 01:33
22187ca to
341afb2
Compare
thomaseizinger
enabled auto-merge
October 25, 2024 01:34
thomaseizinger
disabled auto-merge
October 25, 2024 03:22
thomaseizinger
marked this pull request as draft
October 25, 2024 03:22
Member
Author
|
Need to deep-dive on these test failures and how |
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
October 25, 2024 03:27
341afb2 to
fe3f4b6
Compare
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
November 12, 2024 01:49
fe3f4b6 to
6bb6db0
Compare
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
November 12, 2024 01:52
6bb6db0 to
bc45f5d
Compare
thomaseizinger
marked this pull request as ready for review
November 12, 2024 01:53
Member
Author
|
@jamilbk I ended up adjusting our fork to - for now - only change log levels and remove the panic paths. I'll deal with the time-impurity later once I changed our test suite to be less sensitive to individual packet drops. |
thomaseizinger
force-pushed
the
chore/depend-on-boringtun-fork
branch
from
November 12, 2024 02:55
bc45f5d to
2bea6c9
Compare
thomaseizinger
commented
Nov 12, 2024
Comment on lines
-442
to
-453
| let packet_len = packet.packet().len(); | ||
| let max_len = if self.mode.is_client() { | ||
| ip_packet::PACKET_SIZE | ||
| } else { | ||
| ip_packet::PACKET_SIZE + ip_packet::NAT46_OVERHEAD | ||
| }; | ||
|
|
||
| // TODO: This is a it of a hack, we should compile-time enforce this. | ||
| if packet_len > max_len { | ||
| tracing::warn!("Packet is too large; max={max_len}, actual={packet_len}"); | ||
| return Ok(None); | ||
| } |
Member
Author
There was a problem hiding this comment.
This is no longer needed because our fork now has firezone/boringtun#26.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Nov 12, 2024
Within our test suite, we "spin" for several (simulated) seconds after each state transition to allow for packets being sent between the different nodes. The test suite simulates different latencies by delaying the delivery of some of these packets. `connlib` has several timers for sending packets, i.e. STUN bindings, WG keep-alives etc. These timers never end so we cannot simply spin "until we no longer want to send any packets". Currently, we simply hard-stop after a few seconds and drop the remaining packets and move on to the next state transition. At present, this isn't an issue because only our ICE agent adheres to the simulated time advancement. `boringtun` is still impure and thus we usually don't get to see any of the WireGuard packets like keep-alives and session timeouts etc in our tests. The STUN messages are pretty resilient to retransmissions so the current packet drop doesn't matter. In the process of adopting our boringtun fork (https://github.com/firezone/boringtun) where we will eventually fix the time impurity, dropping some of these packets caused problems. To fix this, we now drain all remaining packets that are sitting in the "yet-to-be-delivered" buffer. These packets are delivered to an "inbox" that is per-host, meaning the host (i.e. client, gateway or relay) will still perceive the incoming packet with the correct latency. We extract this functionality from #7120 because it is generally useful.
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
4 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.
This switches our dependency on
boringtunover to our fork at https://github.com/firezone/boringtun. The idea of the fork is to carefully only patch selective parts such that upstream things later is still possible. The complete diff can be seen here: cloudflare/boringtun@master...firezone:boringtun:masterSo far, the only patches in the fork are dependency bumps, linter fixes, adjustments to log levels and the removal of panics when the destination buffer is too small.