From 3fba5745ee3940e40ca20289d47900abd8a397c6 Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Tue, 21 May 2024 07:20:45 +1000
Subject: [PATCH] refactor(connlib): use events for pushing updated resource
 list (#5035)

The API of connlib is designed around a uni-directional dataflow where
commands flow one way and events flow the other way. By design, this
creates a system of eventual consistency: We don't exactly know when
connlib will emit an event. This is important because it gives us
flexibility in what the internals of connlib look like. It also forces
the downstream apps to be able to handle any event at any point which
avoids bugs where clients rely on a certain order that may just be an
implementation detail.

To achieve all of this, it is important that we don't introduce APIs
with return values. As soon as a function returns a value, it commits to
being able to compute this return value _synchronously_. Any refactoring
that may make the computation of the return value asynchronous is then a
breaking change.

Consequently, APIs like `handle_timeout` should never return a value.
Instead, they should queue an event that the layer above reacts to
accordingly.
---
 rust/connlib/clients/shared/src/eventloop.rs | 11 +++++++++++
 rust/connlib/tunnel/src/client.rs            | 16 +++++++++-------
 rust/connlib/tunnel/src/lib.rs               | 13 ++++++-------
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs
index 09a40a40a..cfb14bd48 100644
--- a/rust/connlib/clients/shared/src/eventloop.rs
+++ b/rust/connlib/clients/shared/src/eventloop.rs
@@ -147,6 +147,17 @@ where
                         .send(PHOENIX_TOPIC, EgressMessages::ReuseConnection(connection));
                 }
             }
+            firezone_tunnel::ClientEvent::ResourcesChanged { resources } => {
+                // Note: This may look a bit weird: We are reading an event from the tunnel and yet delegate back to the tunnel here.
+                // Couldn't the tunnel just do this internally?
+                // Technically, yes.
+                // But, we are only accessing the callbacks here which _eventually_ will be removed from `Tunnel`.
+                // At that point, the tunnel has to emit this event and we need to handle it without delegating back to the tunnel.
+                // We only access the callbacks here because `Tunnel` already has them and the callbacks are the current way of talking to the UI.
+                // At a later point, we will probably map to another event here that gets pushed further up.
+
+                self.tunnel.callbacks.on_update_resources(resources)
+            }
         }
     }
 
diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs
index 726678266..e13d7dd3f 100644
--- a/rust/connlib/tunnel/src/client.rs
+++ b/rust/connlib/tunnel/src/client.rs
@@ -859,9 +859,7 @@ impl ClientState {
         earliest(self.next_dns_refresh, self.node.poll_timeout())
     }
 
-    /// Returns whether resources statuses have updated
-    pub fn handle_timeout(&mut self, now: Instant) -> bool {
-        let mut resources_updated = false;
+    pub fn handle_timeout(&mut self, now: Instant) {
         self.node.handle_timeout(now);
 
         match self.next_dns_refresh {
@@ -899,7 +897,10 @@ impl ClientState {
             match event {
                 snownet::Event::ConnectionFailed(id) => {
                     self.cleanup_connected_gateway(&id);
-                    resources_updated = true;
+                    self.buffered_events
+                        .push_back(ClientEvent::ResourcesChanged {
+                            resources: self.resources(),
+                        });
                 }
                 snownet::Event::NewIceCandidate {
                     connection,
@@ -921,12 +922,13 @@ impl ClientState {
                     }),
                 snownet::Event::ConnectionEstablished(id) => {
                     self.update_site_status_by_gateway(&id, Status::Online);
-                    resources_updated = true;
+                    self.buffered_events
+                        .push_back(ClientEvent::ResourcesChanged {
+                            resources: self.resources(),
+                        });
                 }
             }
         }
-
-        resources_updated
     }
 
     fn update_site_status_by_gateway(&mut self, gateway_id: &GatewayId, status: Status) {
diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs
index d540cf5a0..220011e50 100644
--- a/rust/connlib/tunnel/src/lib.rs
+++ b/rust/connlib/tunnel/src/lib.rs
@@ -6,6 +6,7 @@
 use boringtun::x25519::StaticSecret;
 use chrono::Utc;
 use connlib_shared::{
+    callbacks,
     messages::{ClientId, GatewayId, Relay, RelayId, ResourceId, ReuseConnection},
     Callbacks, Result,
 };
@@ -118,13 +119,7 @@ where
                 self.device_read_buf.as_mut(),
             )? {
                 Poll::Ready(io::Input::Timeout(timeout)) => {
-                    let resources_updated = self.role_state.handle_timeout(timeout);
-
-                    if resources_updated {
-                        self.callbacks
-                            .on_update_resources(self.role_state.resources());
-                    }
-
+                    self.role_state.handle_timeout(timeout);
                     continue;
                 }
                 Poll::Ready(io::Input::Device(packet)) => {
@@ -262,6 +257,10 @@ pub enum ClientEvent {
     RefreshResources {
         connections: Vec<ReuseConnection>,
     },
+    /// The list of resources has changed and UI clients may have to be updated.
+    ResourcesChanged {
+        resources: Vec<callbacks::ResourceDescription>,
+    },
 }
 
 pub enum GatewayEvent {