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.
This commit is contained in:
Thomas Eizinger
2025-01-03 15:35:32 +01:00
committed by GitHub
parent 309914a45d
commit d1eb1961dc
2 changed files with 29 additions and 3 deletions

View File

@@ -139,3 +139,5 @@ cc c50c783f60cd7126453f38d2ae37bea3120ebb588c68f99ad5ad4a659f331338
cc 606dacf44d60870087c7c65fb404f090c52762167e01e534dee1df0557d70304
cc e6fcc44d59815c5d62b58f9f12ceb4ea67be9de8421bae651c3d5ef8cecea8aa
cc eaa345674d5ae8799ae3c739184177b52e5912e6ff2b0925e07fd95f4e992c2a
cc 7bbfaee281b4701f9abecdee0e338ced1b23fee523b4bdacb658b5fe095cba9a
cc 505e3808d25131624567d942c29790d7eabea9b41a7e208d5f21b302c96f3960

View File

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