feat(connlib): report resource status to client #4931

conectado · 2024-05-09T00:15:57Z

This PR introduces site's Status. That's used to report to the client the status, either, unknown, online or offline, mostly as a hint to users as what's wrong with a connection.

This are the criteria for an online or offline resource

If all sites related to a resource are offline the resource is considered offline, since there's no gateway that can respond to that resource's connection
If any site is online the resource is online, since that same peer can be used to reach that resource
Any other case is unknown

Right now resources are single site so it doesn't matter too much but tracking online/offline per-site instead of per-gateway or resource seems like the better long-term solution.

The way to "find out" the site's status is:

If a response to a connection details is offline, all sites related to that resource must be offline otherwise there would've been a gateway in the response
At the point we connect to a gateway, the site that corresponds to that gateway must be online
When a connection to a peer stops it's considered unknown again

Fixes #4738

vercel · 2024-05-09T00:16:01Z

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		May 14, 2024 11:48pm

github-actions · 2024-05-09T00:17:59Z

Terraform Cloud Plan Output

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

Terraform Cloud Plan

github-actions · 2024-05-09T00:25:52Z

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	235.0 MiB (-3%)	236.1 MiB (-3%)	392 (+22%)
direct-tcp-server2client	247.1 MiB (+2%)	249.0 MiB (+2%)	98 (+1%)
relayed-tcp-client2server	227.9 MiB (-0%)	228.8 MiB (-1%)	253 (+13%)
relayed-tcp-server2client	196.0 MiB (-17%)	196.9 MiB (-17%)	427 (-13%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	500.0 MiB (-0%)	0.04ms (+76%)	45.03% (+10%)
direct-udp-server2client	500.0 MiB (-0%)	0.02ms (-69%)	21.21% (+8%)
relayed-udp-client2server	499.9 MiB (-0%)	0.03ms (+10%)	55.55% (+1%)
relayed-udp-server2client	500.0 MiB (+0%)	0.03ms (+68%)	40.18% (-5%)

jamilbk

Can't comment on the Rust stuff but the rest LGTM. Do we currently send the Site name in the on_update_resources list as well? If so I can start working on displaying that, along with the Resource offline/online status in #3514

jamilbk · 2024-05-14T05:22:38Z

rust/connlib/shared/src/callbacks.rs

+        }
+    }
+
+    /// What the GUI clients should paste to the clipboard, e.g. `https://github.com/firezone`


Just a quick note -- this will be updated to use the address_description in #3514

👍 this is just copy-pasted though

thomaseizinger

Great PR description, thank you for the summary. Left some comments!

rust/connlib/clients/shared/src/eventloop.rs

rust/connlib/clients/shared/src/messages.rs

rust/connlib/shared/src/callbacks.rs

rust/connlib/shared/src/proptest.rs

rust/connlib/tunnel/src/client.rs

thomaseizinger · 2024-05-14T06:21:29Z

rust/connlib/tunnel/src/client.rs

        }
    }

-    fn resources(&self) -> Vec<ResourceDescription> {
-        self.resource_ids.values().sorted().cloned().collect_vec()
+    pub(crate) fn resources(&self) -> Vec<callbacks::ResourceDescription> {


I think this creates a weird coupling to the callbacks. What if we just make this a tuple for now instead of duplicating the type?

Long-term, I'd like connlib to have a proper domain model and not (ab)use the DTO types with the portal. The new ResourceDescription is yet another DTO and not a proper domain model.

If we use a tuple here, it means having to update all clients as part of this PR since it breaks de-serialization and I don't see much benefit of using a tuple here vs adding a field, which is easier to understand the meaning of specially for the de-serializing code.

the resources function is only used after all for the callbacks.

We could also return a tuple here and create the callbacks::ResourceDescription on the top function but that just seems like a bit more convoluted that simply returning the callbacks::ResourceDescription here.

rust/connlib/tunnel/src/client.rs

ReactorScram

Looks okay other than the changes Thomas and I requested. I skimmed through the tests without looking at them in depth.

rust/connlib/tunnel/src/lib.rs

ReactorScram · 2024-05-14T14:17:51Z

rust/headless-client/src/imp_linux.rs

-    fn on_update_resources(&self, resources: Vec<ResourceDescription>) {
-        tracing::info!(len = resources.len(), "New resource list");
+    fn on_update_resources(&self, resources: Vec<callbacks::ResourceDescription>) {
+        tracing::info!(?resources, "New resource list");


This can't be logged as INFO because it has domain names and IPs and stuff, right? That's why I only had the length before in some places in the Clients.

I should have caught these too. We need to keep logs like this to debug because otherwise it'll just consume too much disk space for all users.

maybe it's a bit too often but I don't think there's any sensitive info here. I think it's safe to log users' resources

This will be called pretty often now, so I'll leave at debug with only the length, otherwise I think it will be too spammy.

rust/headless-client/src/lib.rs

rust/connlib/shared/src/callbacks.rs

ReactorScram · 2024-05-14T14:32:00Z

rust/connlib/shared/src/callbacks.rs

+    pub fn name(&self) -> &str {
+        match self {
+            ResourceDescription::Dns(r) => &r.name,
+            ResourceDescription::Cidr(r) => &r.name,
+        }
+    }
+
+    pub fn status(&self) -> Status {
+        match self {
+            ResourceDescription::Dns(r) => r.status,
+            ResourceDescription::Cidr(r) => r.status,
+        }
+    }
+
+    pub fn id(&self) -> ResourceId {
+        match self {
+            ResourceDescription::Dns(r) => r.id,
+            ResourceDescription::Cidr(r) => r.id,
+        }
+    }


Hm we can't flip ResourceDescription into a struct that shares these fields, because it would break compatibility with older code, right? Or if this type of ResourceDescription is only used within the Client, between connlib and the GUI, it could be done?

e.g.

struct ResourceDescription { id: ResourceId, name: String, status: Status, other: ResourceType, } enum ResourceType { Cidr, Dns, }

address is also part of a ResourceDescription and it has a different type depending on whether it's DNS or CIDR

But the other ones look like they have the same type, since there's no into

yeah, but some clients uses the address, maybe we can convert it to a String but I'm not sure having a single type will simplify things, specially since these are used only for serialization and we don't need to access the fields.

rust/connlib/tunnel/src/client.rs

ReactorScram

LGTM

This PR introduces site's `Status`. That's used to report to the client the status, either, unknown, online or offline, mostly as a hint to users as what's wrong with a connection. This are the criteria for an online or offline resource * If all sites related to a resource are offline the resource is considered offline, since there's no gateway that can respond to that resource's connection * If any site is online the resource is online, since that same peer can be used to reach that resource * Any other case is unknown Right now resources are single site so it doesn't matter too much but tracking online/offline per-site instead of per-gateway or resource seems like the better long-term solution. The way to "find out" the site's status is: * If a response to a connection details is offline, all sites related to that resource must be offline otherwise there would've been a gateway in the response * At the point we connect to a gateway, the site that corresponds to that gateway must be online * When a connection to a peer stops it's considered unknown again Fixes #4738

conectado added 2 commits May 8, 2024 18:56

add status tracking for resources

c34a680

feat(connlib): report resource status to clients

77906c0

github-actions bot added the kind/feature New feature or request label May 9, 2024

conectado added 3 commits May 8, 2024 22:27

fix clippy

c04331c

call offline resources on offline response

00a5aaa

wip: checkpoint keeping track of status by group id

1ddd420

conectado force-pushed the feat/pass-resource-status-to-client branch from cc817de to 1ddd420 Compare May 10, 2024 04:49

conectado added 6 commits May 10, 2024 20:37

fix previous commit

843cbfa

fix proptests

78bb80a

add proptest for status changes

9b8c3f7

fix gui compilation

d907a48

fix clippy

b66f453

more clippy fixes

6fdafa2

conectado marked this pull request as ready for review May 14, 2024 01:58

conectado requested review from thomaseizinger and ReactorScram May 14, 2024 01:58

remove stale todo

e99671a

jamilbk reviewed May 14, 2024

View reviewed changes

thomaseizinger reviewed May 14, 2024

View reviewed changes

ReactorScram requested changes May 14, 2024

View reviewed changes

conectado added 6 commits May 14, 2024 17:47

apply pr feedback

b8ff946

rename gateway_group to site

0c913bb

update names in proptests from groups to sites

13dd296

fix log levels

1c0b749

remove unnecesary namespacing

31305a0

remove no longer used imports

eb3f439

conectado requested a review from ReactorScram May 14, 2024 21:22

conectado requested a review from thomaseizinger May 14, 2024 21:22

ReactorScram approved these changes May 14, 2024

View reviewed changes

move cleanup connection within resource_offline

5a3dd36

conectado added this pull request to the merge queue May 15, 2024

Merged via the queue into main with commit a7d35cd May 15, 2024
135 checks passed

conectado deleted the feat/pass-resource-status-to-client branch May 15, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connlib): report resource status to client #4931

feat(connlib): report resource status to client #4931

conectado commented May 9, 2024 •

edited

vercel bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

jamilbk left a comment •

edited

jamilbk May 14, 2024

conectado May 14, 2024

thomaseizinger left a comment

thomaseizinger May 14, 2024

conectado May 14, 2024

conectado May 14, 2024

ReactorScram left a comment

ReactorScram May 14, 2024

jamilbk May 14, 2024

conectado May 14, 2024

conectado May 14, 2024

ReactorScram May 14, 2024

conectado May 14, 2024

ReactorScram May 14, 2024

conectado May 14, 2024

ReactorScram left a comment

feat(connlib): report resource status to client #4931

feat(connlib): report resource status to client #4931

Conversation

conectado commented May 9, 2024 • edited

vercel bot commented May 9, 2024 • edited

github-actions bot commented May 9, 2024 • edited

Terraform Cloud Plan Output

github-actions bot commented May 9, 2024 • edited

Performance Test Results

TCP

UDP

jamilbk left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a 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

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

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

conectado commented May 9, 2024 •

edited

vercel bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

jamilbk left a comment •

edited