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
This commit is contained in:
Jamil
2025-02-28 20:50:12 +00:00
committed by GitHub
parent ab7e805fdd
commit d71fdbf269
2 changed files with 9 additions and 25 deletions

View File

@@ -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();
}

View File

@@ -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![],