chore(gui-client): proof of concept for process splitting #4788

ReactorScram · 2024-04-25T19:20:34Z

Closes #4270

Refs #3713
Refs #3782

It sort-of works, but many features are missing and it needs a refactor.

Tasks

Give feedback

Break `imp_linux.rs` into modules

Break imp_linux.rs into modules
Options
Successfully updated the issue's project

There was an error updating the issue's project
Get rid of `try_send` and panics where possible

Get rid of try_send and panics where possible
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

vercel · 2024-04-25T19:20:39Z

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 26, 2024 7:30pm

github-actions · 2024-04-25T19:22:19Z

Terraform Cloud Plan Output

Plan: 15 to add, 14 to change, 15 to destroy.

Terraform Cloud Plan

github-actions · 2024-04-25T19:40:48Z

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	243.2 MiB (+2%)	245.1 MiB (+1%)	327 (+5%)
direct-tcp-server2client	244.0 MiB (+0%)	245.5 MiB (+0%)	206 (-39%)
relayed-tcp-client2server	225.4 MiB (-1%)	226.5 MiB (-0%)	213 (-25%)
relayed-tcp-server2client	236.0 MiB (-0%)	236.9 MiB (+0%)	481 (+60%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	50.0 MiB (-0%)	0.31ms (+15%)	0.00% (NaN%)
direct-udp-server2client	50.0 MiB (-0%)	0.01ms (-18%)	0.00% (NaN%)
relayed-udp-client2server	50.0 MiB (-0%)	0.07ms (+39%)	0.00% (NaN%)
relayed-udp-server2client	50.0 MiB (+0%)	0.01ms (-36%)	0.00% (NaN%)

ReactorScram · 2024-04-25T21:58:21Z

rust/connlib/tunnel/src/device_channel.rs

-        self.tun = Some(Tun::new(config, dns_config, callbacks)?);
+        self.tun = Some(Tun::new(config, dns_config.clone(), callbacks)?);
+
+        callbacks.on_set_interface_config(config.ipv4, config.ipv6, dns_config);


This appeared to be missing from the Linux impl, I need it to act as on_tunnel_ready

ReactorScram · 2024-04-26T16:10:50Z

rust/headless-client/src/lib.rs

+    // TODO: Should this default to 30 days?
+    let max_partition_time = cli.max_partition_time.map(|d| d.into());


What happens if I pass None for max partition time?

ReactorScram · 2024-04-26T16:11:44Z

rust/gui-client/src-tauri/src/client/gui.rs

@@ -513,7 +521,7 @@ struct Session {
 impl Controller {
    // TODO: Figure out how re-starting sessions automatically will work


The Clients don't need to automatically restart connlib sessions, do they? This comment might be out of date.

ReactorScram · 2024-04-26T16:12:59Z

rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs

+    // TODO: Make sure this joins / drops somewhere
+    let recv_task = tokio_handle.spawn(async move {
+        while let Some(msg) = rx.next().await {
+            let msg = msg?;
+            let msg: IpcServerMsg = serde_json::from_slice(&msg)?;
+            match msg {
+                IpcServerMsg::Ok => {}
+                IpcServerMsg::OnDisconnect => callback_handler.on_disconnect(
+                    &connlib_client_shared::Error::Other("errors can't be serialized"),
+                ),
+                IpcServerMsg::OnUpdateResources(v) => callback_handler.on_update_resources(v),
+                IpcServerMsg::TunnelReady => callback_handler.on_tunnel_ready(),
+            }
+        }
+        Ok(())
+    });


The IPC itself could probably use improvement, maybe one of those poll functions like I use for signals and disconnect somewhere else in the PR.

This is working enough for a demo

ReactorScram · 2024-04-26T16:14:38Z

rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs

+}
+
+// Callbacks must all be non-blocking
+// TODO: DRY


There are 4 of these CallbackHandler structs now, but they're all subtly different, so I didn't try to de-dupe them yet.

I considered trying to fit them all into a "wake-and-poll" pattern but this would probably make the code longer, not shorter, and it would make the IPC protocol a little more chatty. (e.g. instead of the service just sending new resources to the Client, it would send a wake, then the Client would make a request, then the service would respond. I think this is more robust if the system is CPU-starved but in practice it probably doesn't matter at all.)

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

jamilbk

This makes sense. Just verifying -- by merging this we aren't blocking any bugfix updates that the Windows client might need, correct?

ReactorScram · 2024-04-26T19:42:13Z

Blocking as in, causing merge conflicts, or taking time away from them? I don't think there's anything important on Windows, there's a couple issues which are either not replicating or aren't critical https://github.com/firezone/firezone/issues?q=is%3Aopen+is%3Aissue+label%3Aarea%2Fwindows_client+

ReactorScram · 2024-04-26T19:51:52Z

I smoke-tested the MSI built from bbba458 and it passed. Signs in, controls DNS, ifconfig.net goes through the tunnel

jamilbk · 2024-04-26T19:55:07Z

SGTM

I accidentally modified a function that's used in both Android and Linux, this PR splits it up into two functions

Closes #5086 I accidentally modified a function that's used in both Android and Linux, this PR splits it up into two functions ```[tasklist] ### Before merging - [x] Check aarch64 deb still signs in (716320) ```

the GUI and the IPC service talk, but they don't do anything useful yet

a71d118

ReactorScram added area/linux_client Linux client area/tauri_client The Windows and Linux Tauri GUI clients labels Apr 25, 2024

ReactorScram self-assigned this Apr 25, 2024

github-actions bot added the kind/chore Issues related to repository cleanup or maintenance label Apr 25, 2024

fix Windows (in_proc) build

555d301

ReactorScram added 2 commits April 25, 2024 20:58

The split process Client can sign in and get its list of resources

6fd911b

fix Windows build

abf277f

ReactorScram commented Apr 25, 2024

View reviewed changes

ReactorScram added 9 commits April 25, 2024 22:09

de-dupe SOCK_PATH

4bd1b4f

remove completed TODO

9d37c5a

refactor, headless client now runs on Windows

031c509

fix Windows warning

3d11577

remove unused test code, refactor

51287d8

Merge remote-tracking branch 'origin/main' into chore/process-split-poc

e1ca541

Fix Windows

d4759c8

fix unused deps

0b082f7

fix sock_path

26843d2

ReactorScram commented Apr 26, 2024

View reviewed changes

clarify comments

d749662

ReactorScram marked this pull request as ready for review April 26, 2024 16:16

ReactorScram requested a review from jamilbk April 26, 2024 16:16

Merge branch 'main' into chore/process-split-poc

bbba458

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

vercel bot deployed to Preview April 26, 2024 19:30 View deployment

jamilbk approved these changes Apr 26, 2024

View reviewed changes

ReactorScram added this pull request to the merge queue Apr 26, 2024

Merged via the queue into main with commit c8d989a Apr 26, 2024
134 checks passed

ReactorScram deleted the chore/process-split-poc branch April 26, 2024 20:11

ReactorScram added a commit that referenced this pull request May 22, 2024

chore(firezone-tunnel/android): revert regression from #4788

716320d

I accidentally modified a function that's used in both Android and Linux, this PR splits it up into two functions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gui-client): proof of concept for process splitting #4788

chore(gui-client): proof of concept for process splitting #4788

ReactorScram commented Apr 25, 2024 •

edited

Tasks

vercel bot commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited

ReactorScram Apr 25, 2024

ReactorScram Apr 26, 2024

ReactorScram Apr 26, 2024

ReactorScram Apr 26, 2024

ReactorScram Apr 26, 2024

jamilbk left a comment

ReactorScram commented Apr 26, 2024

ReactorScram commented Apr 26, 2024

jamilbk commented Apr 26, 2024

		// TODO: Should this default to 30 days?
		let max_partition_time = cli.max_partition_time.map(\|d\| d.into());

		@@ -513,7 +521,7 @@ struct Session {
		impl Controller {
		// TODO: Figure out how re-starting sessions automatically will work

chore(gui-client): proof of concept for process splitting #4788

chore(gui-client): proof of concept for process splitting #4788

Conversation

ReactorScram commented Apr 25, 2024 • edited

Tasks

vercel bot commented Apr 25, 2024 • edited

github-actions bot commented Apr 25, 2024 • edited

Terraform Cloud Plan Output

github-actions bot commented Apr 25, 2024 • edited

Performance Test Results

TCP

UDP

ReactorScram Apr 25, 2024

Choose a reason for hiding this comment

ReactorScram Apr 26, 2024

Choose a reason for hiding this comment

ReactorScram Apr 26, 2024

Choose a reason for hiding this comment

ReactorScram Apr 26, 2024

Choose a reason for hiding this comment

ReactorScram Apr 26, 2024

Choose a reason for hiding this comment

jamilbk left a comment

Choose a reason for hiding this comment

ReactorScram commented Apr 26, 2024

ReactorScram commented Apr 26, 2024

jamilbk commented Apr 26, 2024

ReactorScram commented Apr 25, 2024 •

edited

vercel bot commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited