github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(connlib): regression seed failure (#7558)

Browse Source 
In #7477, we introduced a regression in our test suite for DNS queries
that are forwarded through the tunnel.

In order to be deterministic when users configure overlapping CIDR
resources, we use the sort order of all CIDR resource IDs to pick, which
one "wins". To make sure existing connections are not interrupted, this
rule does not apply when we already have a connection to a gateway for a
resource. In other words, if a new CIDR resource (e.g. resource `A`) is
added to connlib that has an overlapping route with another resource
(e.g. resource `B`) but we already have a connection to resource `B`, we
will continue routing traffic for this CIDR range to resource `B`,
despite `A` sorting "before" `B`.

The regression that we introduced was that we did not account for
resources being "connected" after forwarding a query through the tunnel
to it. As a result, in the found failure case, the test suite was
expecting to route the packet to resource `A` because it did not know
that we are connected to resource `B` at the time of processing the ICMP
packet.
This commit is contained in:
Thomas Eizinger
2024-12-20 10:59:58 +01:00
committed by GitHub
parent 4dea4049da
commit 1dc6b46344
3 changed files with 24 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
rust/connlib/tunnel/proptest-regressions/tests.txt
View File

@@ -137,3 +137,4 @@ cc d188ee5433064634b07dff90aeb3c26bd3698310bcc4d27f15f35a6296eb8687
cc e60fe97280614a96052e1af1c8d3e4661ec2fead4d22106edf6fb5caf330b6a5
cc c50c783f60cd7126453f38d2ae37bea3120ebb588c68f99ad5ad4a659f331338
cc 606dacf44d60870087c7c65fb404f090c52762167e01e534dee1df0557d70304
cc e6fcc44d59815c5d62b58f9f12ceb4ea67be9de8421bae651c3d5ef8cecea8aa

8
rust/connlib/tunnel/src/tests/reference.rs
View File

@@ -362,7 +362,13 @@ impl ReferenceState {
}),
Transition::SendDnsQueries(queries) => {
for query in queries {
state.client.exec_mut(|client| client.on_dns_query(query));
state.client.exec_mut(|client| {
client.on_dns_query(query);
if let Some(resource) = client.dns_query_via_resource(query) {
client.connect_to_internet_or_cidr_resource(resource);
}
});
}
}
Transition::SendIcmpPacket {

20
rust/connlib/tunnel/src/tests/sim_client.rs
View File

@@ -508,12 +508,16 @@ impl RefClient {
continue;
}
if let Some(resource) = table.exact_match(resource.address) {
if self.is_connected_to_internet_or_cidr(*resource) {
if let Some(overlapping_resource) = table.exact_match(resource.address) {
if self.is_connected_to_internet_or_cidr(*overlapping_resource) {
tracing::debug!(%overlapping_resource, resource = %resource.id, address = %resource.address, "Already connected to resource with this exact address, retaining existing route");
continue;
}
}
tracing::debug!(resource = %resource.id, address = %resource.address, "Adding CIDR route");
table.insert(resource.address, resource.id);
}
@@ -644,7 +648,7 @@ impl RefClient {
);
}
#[tracing::instrument(level = "debug", skip_all, fields(dst, resource))]
#[tracing::instrument(level = "debug", skip_all, fields(dst, resource, gateway))]
fn on_packet<E>(
&mut self,
src: IpAddr,
@@ -665,12 +669,16 @@ impl RefClient {
return;
};
tracing::Span::current().record("gateway", tracing::field::display(gateway));
self.connect_to_resource(resource, dst);
if !self.is_tunnel_ip(src) {
return;
}
tracing::debug!(%payload, "Sending packet to resource");
map(self)
.entry(gateway)
.or_default()
@@ -695,7 +703,11 @@ impl RefClient {
}
if self.cidr_resources.iter().any(|(_, r)| *r == resource) {
self.connected_cidr_resources.insert(resource);
let is_new = self.connected_cidr_resources.insert(resource);
if is_new {
tracing::debug!(%resource, "Now connected to CIDR resource");
}
}
}