refactor(connlib): delay initialization of `Sockets` until we have a tokio runtime #4286

jamilbk · 2024-03-24T00:35:19Z

Our sockets need to be initialized within a tokio runtime context. To achieve this, we don't actually initialize anything on Sockets::new. Instead, we call rebind within the constructor of Tunnel which already runs in a tokio context.

Fixes: #4282

vercel · 2024-03-24T00:35:26Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
firezone	⬜️ Ignored (Inspect)	Visit Preview		Mar 25, 2024 10:41pm

github-actions · 2024-03-24T00:37:03Z

Terraform Cloud Plan Output

Plan: 9 to add, 8 to change, 9 to destroy.

Terraform Cloud Plan

github-actions · 2024-03-24T00:45:44Z

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	221.0 MiB (-2%)	222.4 MiB (-2%)	169 (-18%)
direct-tcp-server2client	227.4 MiB (+0%)	229.2 MiB (+0%)	216 (-49%)
relayed-tcp-client2server	145.0 MiB (-4%)	145.6 MiB (-4%)	122 (-36%)
relayed-tcp-server2client	154.0 MiB (+2%)	154.4 MiB (+2%)	175 (+1%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	50.0 MiB (-0%)	0.05ms (-85%)	0.00% (NaN%)
direct-udp-server2client	50.0 MiB (+0%)	0.01ms (-6%)	0.00% (NaN%)
relayed-udp-client2server	50.0 MiB (-0%)	0.17ms (+92%)	0.00% (NaN%)
relayed-udp-server2client	50.0 MiB (-0%)	0.05ms (-2%)	0.00% (NaN%)

jamilbk · 2024-03-24T01:08:22Z

@thomaseizinger I think you'd be much faster at wrapping this up, so I'll assign it to you. I went down the road of adding a new with_runtime closure to Session like you have with with_protect but got stuck. Not sure if that's the right approach.

I think Gabi pulled 2c2c617 out because it was causing CI failures.

We need to call this within a tokio runtime so we can't call it as part of setting up the `Session` but need to delay it to `Tunnel::new`.

thomaseizinger · 2024-03-24T09:36:50Z

@jamilbk I hope the patch I pushed fixes it! It also removes some duplication from the previous approach which is nice :)

thomaseizinger · 2024-03-24T12:28:09Z

rust/connlib/tunnel/src/sockets.rs

-    pub fn new() -> io::Result<Self> {
+    #[cfg(unix)]
+    pub fn with_protect(
+        protect: impl Fn(std::os::fd::RawFd) -> io::Result<()> + Send + 'static,


@conectado As part of #4159, I want to move the initialization of the TUN device to the upper layers, i.e. outside of Tunnel. At that point, the use of FIREZONE_MARK can be entirely in the clients and the linux client can also use this callback to call set_mark on the socket which unifies this behaviour across all platforms.

I'm in favor of this, in fact, I think making connlib completely platform-independent would be very nice.

As long as it doesn't make the FFI / callbacks more complex just to do something that connlib already knows it needs to do.

jamilbk

Thanks! I'll slot this in my PR #4133 and let the others review this week.

jamilbk · 2024-03-24T15:09:48Z

Tested on Android (emulator 10), macOS, and iOS and this seems to fix the issue.

conectado · 2024-03-25T15:06:32Z

rust/connlib/tunnel/src/sockets.rs

@@ -13,10 +13,47 @@ use crate::Result;
 pub struct Sockets {
    socket_v4: Option<Socket>,
    socket_v6: Option<Socket>,
+
+    #[cfg(unix)]
+    protect: Box<dyn Fn(std::os::fd::RawFd) -> io::Result<()> + Send + 'static>,


I'm not that sure about this platform-dependent callback.

What if instead of having a protect callback, we have a create_socket callback, that returns the socket that each platform can create as it want.

Another idea, that perhaps works better, is getting a socket2::UdpSocket instead of a tokio::Socketand have the client just send the socket and we create the Sockets and Socket when it is received.

@conectado Would it make sense to save this refactor for another PR just to get these fixes into main? I believe @thomaseizinger is out this week and you next week so maybe sometime after that.

yeah, I'll be in favor of this (sorry, I missed this comment) fixing main asap seems more important :)

I think that is a great idea @conectado ! Despite some duplication across the clients, I think it is much cleaner to let them choose, how to initialize they sockets. There is actually no reason why connlib should dictate the use of a random port. it should be up to the client to pick the port. That would make it trivial to e.g. implement a PORT env variable on the gateway.

Very cool 😎

ReactorScram

Makes sense. Some of the last month of refactors have definitely been fighting each other though, I just had to add that Sockets::new() to session.reconnect() in the GUI client on another PR branch, and now it's already going away again lol

rust/connlib/tunnel/src/io.rs

Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com> Signed-off-by: Jamil <jamilbk@users.noreply.github.com>

jamilbk · 2024-03-25T22:32:13Z

@ReactorScram I think this is failing to get merged because #4198 got merged before.

jamilbk · 2024-03-25T22:33:11Z

Updating from main, just need to fix the semantic errors.

@AndrewDryga We finally hit a good example of where the merge queue prevented two PRs in flight prevent from landing in main and causing a semantic error

ReactorScram · 2024-03-25T22:36:38Z

Yeah 4198 is the one I was thinking of, where I had to do session.reconnect(Sockets::new())

thomaseizinger · 2024-03-26T00:01:06Z

Makes sense. Some of the last month of refactors have definitely been fighting each other though, I just had to add that Sockets::new() to session.reconnect() in the GUI client on another PR branch, and now it's already going away again lol

Yeah sorry about that. What was merged in #4263 was actually an in-between design, see #4263 (comment).

Hide protect callback inside Sockets

9fafc5f

jamilbk requested a review from thomaseizinger March 24, 2024 01:08

jamilbk assigned thomaseizinger Mar 24, 2024

Initialize Sockets within tokio runtime

36682b3

We need to call this within a tokio runtime so we can't call it as part of setting up the `Session` but need to delay it to `Tunnel::new`.

thomaseizinger changed the title ~~refactor(connlib): Hide protect callback inside Sockets and ensure Sockets::new() is called within a tokio runtime~~ refactor(connlib): delay initialization of Sockets until we have a tokio runtime Mar 24, 2024

Fix compilation on non-unix

b82e5af

thomaseizinger reviewed Mar 24, 2024

View reviewed changes

Add Default impl for Sockets

9d15e03

thomaseizinger marked this pull request as ready for review March 24, 2024 12:38

jamilbk commented Mar 24, 2024

View reviewed changes

jamilbk requested review from conectado and ReactorScram March 24, 2024 14:22

jamilbk mentioned this pull request Mar 25, 2024

feat(apple): Handle network changes reliably on macOS and iOS #4133

Merged

conectado reviewed Mar 25, 2024

View reviewed changes

ReactorScram approved these changes Mar 25, 2024

View reviewed changes

rust/connlib/tunnel/src/io.rs Show resolved Hide resolved

Update rust/connlib/tunnel/src/io.rs

a1d3910

Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com> Signed-off-by: Jamil <jamilbk@users.noreply.github.com>

jamilbk mentioned this pull request Mar 25, 2024

connlib: initialize Sockets with factory-fn for making a socket instead of protect #4314

Open

conectado approved these changes Mar 25, 2024

View reviewed changes

jamilbk added this pull request to the merge queue Mar 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024

conectado added this pull request to the merge queue Mar 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024

Merge branch 'main' into fix/sockets-inside-tokio-runtime

a05c059

Fix semantic error from main

68d05c6

jamilbk enabled auto-merge March 25, 2024 22:46

jamilbk added this pull request to the merge queue Mar 25, 2024

Merged via the queue into main with commit 2283898 Mar 25, 2024
138 checks passed

jamilbk deleted the fix/sockets-inside-tokio-runtime branch March 25, 2024 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(connlib): delay initialization of `Sockets` until we have a tokio runtime #4286

refactor(connlib): delay initialization of `Sockets` until we have a tokio runtime #4286

jamilbk commented Mar 24, 2024 •

edited by thomaseizinger

Loading

vercel bot commented Mar 24, 2024 •

edited

Loading

github-actions bot commented Mar 24, 2024 •

edited

Loading

github-actions bot commented Mar 24, 2024 •

edited

Loading

jamilbk commented Mar 24, 2024

thomaseizinger commented Mar 24, 2024

thomaseizinger Mar 24, 2024

conectado Mar 25, 2024

ReactorScram Mar 25, 2024

jamilbk left a comment

jamilbk commented Mar 24, 2024

conectado Mar 25, 2024 •

edited

Loading

jamilbk Mar 25, 2024

jamilbk Mar 25, 2024

conectado Mar 25, 2024

thomaseizinger Mar 26, 2024

jamilbk Mar 26, 2024

ReactorScram left a comment

jamilbk commented Mar 25, 2024

jamilbk commented Mar 25, 2024 •

edited

Loading

ReactorScram commented Mar 25, 2024

thomaseizinger commented Mar 26, 2024

refactor(connlib): delay initialization of Sockets until we have a tokio runtime #4286

refactor(connlib): delay initialization of Sockets until we have a tokio runtime #4286

Conversation

jamilbk commented Mar 24, 2024 • edited by thomaseizinger Loading

vercel bot commented Mar 24, 2024 • edited Loading

github-actions bot commented Mar 24, 2024 • edited Loading

Terraform Cloud Plan Output

github-actions bot commented Mar 24, 2024 • edited Loading

Performance Test Results

TCP

UDP

jamilbk commented Mar 24, 2024

thomaseizinger commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamilbk left a comment

Choose a reason for hiding this comment

jamilbk commented Mar 24, 2024

conectado Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReactorScram left a comment

Choose a reason for hiding this comment

jamilbk commented Mar 25, 2024

jamilbk commented Mar 25, 2024 • edited Loading

ReactorScram commented Mar 25, 2024

thomaseizinger commented Mar 26, 2024

refactor(connlib): delay initialization of `Sockets` until we have a tokio runtime #4286

refactor(connlib): delay initialization of `Sockets` until we have a tokio runtime #4286

jamilbk commented Mar 24, 2024 •

edited by thomaseizinger

Loading

vercel bot commented Mar 24, 2024 •

edited

Loading

github-actions bot commented Mar 24, 2024 •

edited

Loading

github-actions bot commented Mar 24, 2024 •

edited

Loading

conectado Mar 25, 2024 •

edited

Loading

jamilbk commented Mar 25, 2024 •

edited

Loading