From d1eb1961dc04eb478fea15c9560f68cbdfb5e04c Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Fri, 3 Jan 2025 15:35:32 +0100
Subject: [PATCH] fix(connlib): recalculate overlapping CIDR routes less often
 (#7592)

Firezone needs to deterministically handle overlapping CIDR routes. The
way we handle this is that more specific routes are preferred over less
specific one. In case of an exact overlap, the sorting of the resource
ID acts as a tie-breaker: "Smaller" resource IDs preferred over "larger"
ones. This ensures that regardless of which order the resources are
added / enabled in, Firezone behaves deterministically.

In addition to the above rules, existing connections to Gateways always
have precedence: In other words, if we are connected to resource A via
Gateway 1 and resource B exactly overlaps with A yet needs to be routed
to Gateway B and B < A, we still retain resource A in order to not
interrupt existing connections.

When a connection to a Gateway fails, these mappings are cleaned up. The
proptests seeds added in this PR identify a routing mismatch in case a
(relayed) connection is cut, followed by adding a non-CIDR resource:
`connlib` recalculated the CIDR routes as part of adding the new
resource, even though the CIDR resources didn't actually change. This
could potentially result in a connection suddenly being routed to a
different Gateway despite nothing about that resource changing.

To fix this, we add a check for updating the CIDR routes and only
perform it in case CIDR resources get changed.
---
 .../tunnel/proptest-regressions/tests.txt     |  2 ++
 rust/connlib/tunnel/src/client.rs             | 30 +++++++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt
index a3a3cf758..9a0143026 100644
--- a/rust/connlib/tunnel/proptest-regressions/tests.txt
+++ b/rust/connlib/tunnel/proptest-regressions/tests.txt
@@ -139,3 +139,5 @@ cc c50c783f60cd7126453f38d2ae37bea3120ebb588c68f99ad5ad4a659f331338
 cc 606dacf44d60870087c7c65fb404f090c52762167e01e534dee1df0557d70304
 cc e6fcc44d59815c5d62b58f9f12ceb4ea67be9de8421bae651c3d5ef8cecea8aa
 cc eaa345674d5ae8799ae3c739184177b52e5912e6ff2b0925e07fd95f4e992c2a
+cc 7bbfaee281b4701f9abecdee0e338ced1b23fee523b4bdacb658b5fe095cba9a
+cc 505e3808d25131624567d942c29790d7eabea9b41a7e208d5f21b302c96f3960
diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs
index a67eead0b..2641ba126 100644
--- a/rust/connlib/tunnel/src/client.rs
+++ b/rust/connlib/tunnel/src/client.rs
@@ -916,6 +916,7 @@ impl ClientState {
             self.disable_resource(*disabled_resource);
         }
 
+        self.maybe_update_cidr_resources();
         self.maybe_update_tun_routes()
     }
 
@@ -1229,8 +1230,6 @@ impl ClientState {
     }
 
     fn maybe_update_tun_routes(&mut self) {
-        self.active_cidr_resources = self.recalculate_active_cidr_resources();
-
         let Some(config) = self.tun_config.clone() else {
             return;
         };
@@ -1249,6 +1248,18 @@ impl ClientState {
         self.maybe_update_tun_config(new_tun_config);
     }
 
+    fn maybe_update_cidr_resources(&mut self) {
+        let new_resources = self.recalculate_active_cidr_resources();
+
+        if self.active_cidr_resources == new_resources {
+            return;
+        }
+
+        tracing::info!(?self.active_cidr_resources, ?new_resources, "Re-calculated active CIDR resources");
+
+        self.active_cidr_resources = new_resources;
+    }
+
     fn recalculate_active_cidr_resources(&self) -> IpNetworkTable<CidrResource> {
         let mut active_cidr_resources = IpNetworkTable::<CidrResource>::new();
 
@@ -1427,6 +1438,7 @@ impl ClientState {
             self.add_resource(resource)
         }
 
+        self.maybe_update_cidr_resources();
         self.maybe_update_tun_routes();
         self.emit_resources_changed();
     }
@@ -1478,6 +1490,10 @@ impl ClientState {
 
         tracing::info!(%name, address, %sites, "Activating resource");
 
+        if matches!(new_resource, Resource::Cidr(_)) {
+            self.maybe_update_cidr_resources();
+        }
+
         self.maybe_update_tun_routes();
         self.emit_resources_changed();
     }
@@ -1485,7 +1501,15 @@ impl ClientState {
     #[tracing::instrument(level = "debug", skip_all, fields(?id))]
     pub fn remove_resource(&mut self, id: ResourceId) {
         self.disable_resource(id);
-        self.resources_by_id.remove(&id);
+
+        if self
+            .resources_by_id
+            .remove(&id)
+            .is_some_and(|r| matches!(r, Resource::Cidr(_)))
+        {
+            self.maybe_update_cidr_resources();
+        };
+
         self.maybe_update_tun_routes();
         self.emit_resources_changed();
     }