From 1dc6b46344ca28ec227e72ddefb88852195470de Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Fri, 20 Dec 2024 10:59:58 +0100
Subject: [PATCH] fix(connlib): regression seed failure (#7558)

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.
---
 .../tunnel/proptest-regressions/tests.txt     |  1 +
 rust/connlib/tunnel/src/tests/reference.rs    |  8 +++++++-
 rust/connlib/tunnel/src/tests/sim_client.rs   | 20 +++++++++++++++----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt
index 38ed10ea2..9a8d29360 100644
--- a/rust/connlib/tunnel/proptest-regressions/tests.txt
+++ b/rust/connlib/tunnel/proptest-regressions/tests.txt
@@ -137,3 +137,4 @@ cc d188ee5433064634b07dff90aeb3c26bd3698310bcc4d27f15f35a6296eb8687
 cc e60fe97280614a96052e1af1c8d3e4661ec2fead4d22106edf6fb5caf330b6a5
 cc c50c783f60cd7126453f38d2ae37bea3120ebb588c68f99ad5ad4a659f331338
 cc 606dacf44d60870087c7c65fb404f090c52762167e01e534dee1df0557d70304
+cc e6fcc44d59815c5d62b58f9f12ceb4ea67be9de8421bae651c3d5ef8cecea8aa
diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs
index 35fdc680d..314055945 100644
--- a/rust/connlib/tunnel/src/tests/reference.rs
+++ b/rust/connlib/tunnel/src/tests/reference.rs
@@ -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 {
diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs
index daa5dce14..35effda9a 100644
--- a/rust/connlib/tunnel/src/tests/sim_client.rs
+++ b/rust/connlib/tunnel/src/tests/sim_client.rs
@@ -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");
+            }
         }
     }