The downcasting abilities of `anyhow` are pretty powerful.
Unfortunately, they can also be a bit tricky to get right. Whilst `is`
and `downcast` work fine for any errors that are within the `anyhow`
error chain, they don't check the chain of errors prior to that. In
other words, if we already have a nested `std::error::Error` with
several causes, `anyhow` cannot downcast to these causes directly.
In order to avoid this footgun, we create a thin-layer on top of the
`anyhow` crate with some downcasting functions that always try to do the
right thing.
In addition to the `iter` functions, `keys` and `values` also iterate
over the contents of a `HashMap` and are thus non-deterministic. This
can create problems where our test-suite is non-deterministic.
Rust 1.88 has been released and brings with it a quite exciting feature:
let-chains! It allows us to mix-and-match `if` and `let` expressions,
therefore often reducing the "right-drift" of the relevant code, making
it easier to read.
Rust.188 also comes with a new clippy lint that warns when creating a
mutable reference from an immutable pointer. Attempting to fix this
revealed that this is exactly what we are doing in the eBPF kernel.
Unfortunately, it doesn't seem to be possible to design this in a way
that is both accepted by the borrow-checker AND by the eBPF verifier.
Hence, we simply make the function `unsafe` and document for the
programmer, what needs to be upheld.
In order to implement GSO in `connlib`, we opted for an approach where
packets of the same length are being appended to a buffer. Each of these
buffers is the sent to the kernel in a single syscall, which drastically
decreases the per-packet overhead of syscalls and therefore improves
performance.
Within `connlib` itself, we prioritise control-protocol associated
packets over tunnel traffic. The idea here is that even under high-load,
we want to ensure that STUN probes between the peers and to the relays
are sent in a timely manner. Failing to send these probes results in a
false-positive detection of a lost connection because the `connlib`'s
internal state uses timeouts to detect such situations.
Despite processing the packets itself in a timely manner, it is still
possible that they get delayed depending on which order the get flushed
to the socket. This order is currently non-deterministic because
`GsoQueue` uses a `HashMap` internally and when accessing the
batched-together datagrams, we just access it via `iter_mut`.
To fix this, we use a `BTreeMap` instead and explicitly define the `Key`
to start with the `segment_size` field. As a result, entries within the
`BTreeMap` will be sorted ascending by `segment_size` (i.e. the size of
individual packets within the batch). Packets of smaller size are more
likely to be control messages like STUN binding requests or TURN
messages to the relays for managing allocations.
By sorting the map explicitly, we ensure that if the UDP socket is ready
to send, we flush out these messages first before moving on to bigger
packets such as the ones containing (more likely) WireGuard data
messages.
It was already a bit sus that we didn't receive as many errors in Sentry
from the IPC service as from the GUI client. Turns out that we forgot to
initialise our `sentry_layer` there. Additionally, we also didn't
initialise the `LogTracer`, meaning we didn't capture logs from the
`log` crate which is used by some of the dependencies, for example
`wintun`.
For quite a while now, we have been making extensive use of
property-based testing to ensure `connlib` works as intended. The idea
of proptests is that - given a certain seed - we deterministically
sample test inputs and assert properties on a given function.
If the test fails, `proptest` prints the seed which can then be added to
a regressions file to iterate on the test case and fix it. It is quite
obvious that non-determinism in how the test input gets generated is no
bueno and reduces the value we get out of these tests a fair bit.
The `HashMap` and `HashSet` data structures are known to be
non-deterministic in their iteration order. This causes non-determinism
during the input generation because we make use of a lot of maps and
sets to gradually build up the test input. We fix all uses of `HashMap`
and `HashSet` by replacing them with `BTreeMap` and `BTreeSet`.
To ensure this doesn't regress, we refactor `tunnel_test` to not make
use of proptest's macros and instead, we initialise and run the test
ourselves. This allows us to dump the sampled state and transitions into
a file per test run. In CI, we then run a 2nd iteration of all
regression tests and compare the sampled state and transitions with the
previous run. They must match byte-for-byte.
Finally, to discourage use of non-deterministic iteration, we ban the
use of the iteration functions on `HashMap` and `HashSet` across the
codebase. This doesn't catch iteration in a `for`-loop but it is better
than not linting against it at all.
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Motivated by: #4340.
I also activated
[`clippy::unnnecessary_wraps`](https://rust-lang.github.io/rust-clippy/master/#/unnecessary_wraps)
which does create some false-positives for the platform-specific code
but is IMO overall a net-positive. With the amount of Rust code and
crates increasing, it is good to have tools point out simplifications
like these as they are otherwise hard to spot, especially across crate
boundaries.
