From d71fdbf2690af633f8063839b735a24a1ad4f9fa Mon Sep 17 00:00:00 2001
From: Jamil <jamilbk@users.noreply.github.com>
Date: Fri, 28 Feb 2025 20:50:12 +0000
Subject: [PATCH] fix(connlib): Always emit_resources_changed (#8297)

When adding a new Resource that has the same address as a previous
Resource, we would fail to call `emit_resources_changed`, and the
Resource would fail to show up in the client's resource list.

This happened because we essentially didn't consider "activating" the
resource if the resource address didn't change.

With this PR, we always do the following:

- DNS Resource: Add address to the stub resolver -> no-op if address
exists
- CIDR Resource: `maybe_update_cidr_resources` -> no-op if duplicate
CIDR is added
- Internet Resource: No-op if resource ID doesn't change (it shouldn't
ever)

Since we remove the early-exit logic, the `maybe_update_tun_routes` and
`emit_resources_changed` is always called.

`maybe_update_tun_routes` is a no-op if the address hasn't changed, so
the early-exit logic to avoid calling that seems to be redundant.

## Tested:

- [x] Adding / removing a resource
- [x] Updating a resource's fields individually, observing the client
resource updates properly
- [x] Adding two CIDR resources with the same address, observing that
the routing table _was not updated_ (thus no disruption to packet
flows).


Fixes #8100
---
 rust/connlib/tunnel/src/client.rs          | 28 +++++++---------------
 rust/connlib/tunnel/src/client/resource.rs |  6 -----
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs
index a070e8c4b..dc7119ca4 100644
--- a/rust/connlib/tunnel/src/client.rs
+++ b/rust/connlib/tunnel/src/client.rs
@@ -1438,16 +1438,8 @@ impl ClientState {
         };
 
         if let Some(resource) = self.resources_by_id.get(&new_resource.id()) {
-            let resource_id = resource.id();
-            let display_fields_changed = resource.display_fields_changed(&new_resource);
-            let address_changed = resource.has_different_address(&new_resource);
-
-            if display_fields_changed {
-                self.emit_resources_changed();
-            }
-
-            if address_changed {
-                self.remove_resource(resource_id);
+            if resource.has_different_address(&new_resource) {
+                self.remove_resource(resource.id());
             }
         }
 
@@ -1459,7 +1451,7 @@ impl ClientState {
             return;
         }
 
-        let added = match &new_resource {
+        let activated = match &new_resource {
             Resource::Dns(dns) => self.stub_resolver.add_resource(dns.id, dns.address.clone()),
             Resource::Cidr(cidr) => {
                 let existing = self.active_cidr_resources.exact_match(cidr.address);
@@ -1474,16 +1466,14 @@ impl ClientState {
             }
         };
 
-        if !added {
-            return;
+        if activated {
+            let name = new_resource.name();
+            let address = new_resource.address_string().map(tracing::field::display);
+            let sites = new_resource.sites_string();
+
+            tracing::info!(%name, address, %sites, "Activating resource");
         }
 
-        let name = new_resource.name();
-        let address = new_resource.address_string().map(tracing::field::display);
-        let sites = new_resource.sites_string();
-
-        tracing::info!(%name, address, %sites, "Activating resource");
-
         if matches!(new_resource, Resource::Cidr(_)) {
             self.maybe_update_cidr_resources();
         }
diff --git a/rust/connlib/tunnel/src/client/resource.rs b/rust/connlib/tunnel/src/client/resource.rs
index 140b50cad..429a40e37 100644
--- a/rust/connlib/tunnel/src/client/resource.rs
+++ b/rust/connlib/tunnel/src/client/resource.rs
@@ -167,12 +167,6 @@ impl Resource {
         }
     }
 
-    pub fn display_fields_changed(&self, other: &Resource) -> bool {
-        self.name() != other.name()
-            || self.address_description() != other.address_description()
-            || self.sites() != other.sites()
-    }
-
     pub fn addresses(&self) -> Vec<IpNetwork> {
         match self {
             Resource::Dns(_) => vec![],