From 71f8b86b785853cbe9316b19fed19ebede37d0d7 Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Fri, 12 Jul 2024 10:30:18 +1000
Subject: [PATCH] test(connlib): don't update resources as part of adding new
 ones (#5834)

Currently, `tunnel_test` has some old code that attempted to handle
resource _updates_ as part of adding new ones. That is outdated and
wrong. The test is easier to reason about if we disallow updates to
resources as part of _adding_ a new one.

In production, resources IDs are unique so this shouldn't actually
happen. At a later point, we can add explicit transitions for updating
an existing resource.
---
 rust/connlib/tunnel/src/tests/reference.rs | 71 +++++++++-------------
 1 file changed, 28 insertions(+), 43 deletions(-)

diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs
index acf249269..e725b1807 100644
--- a/rust/connlib/tunnel/src/tests/reference.rs
+++ b/rust/connlib/tunnel/src/tests/reference.rs
@@ -268,44 +268,18 @@ impl ReferenceStateMachine for ReferenceState {
                     .exec_mut(|client| client.dns_resources.remove(id));
             }
             Transition::AddDnsResource {
-                resource: new_resource,
+                resource,
                 records,
                 gateway,
             } => {
-                let existing_resource = state.client.exec_mut(|client| {
-                    client
-                        .gateways_by_resource
-                        .insert(new_resource.id, *gateway);
-                    client
-                        .dns_resources
-                        .insert(new_resource.id, new_resource.clone())
+                state.client.exec_mut(|client| {
+                    client.gateways_by_resource.insert(resource.id, *gateway);
+                    client.dns_resources.insert(resource.id, resource.clone());
                 });
 
                 // For the client, there is no difference between a DNS resource and a truly global DNS name.
                 // We store all records in the same map to follow the same model.
                 state.global_dns_records.extend(records.clone());
-
-                // If a resource is updated (i.e. same ID but different address) and we are currently connected, we disconnect from it.
-                if let Some(resource) = existing_resource {
-                    if new_resource.address != resource.address {
-                        state.client.exec_mut(|client| {
-                            client.connected_cidr_resources.remove(&resource.id)
-                        });
-
-                        state
-                            .global_dns_records
-                            .retain(|name, _| !matches_domain(&resource.address, name));
-
-                        // TODO: IN PRODUCTION, WE CANNOT DO THIS.
-                        // CHANGING A DNS RESOURCE BREAKS CLIENT UNTIL THEY DECIDE TO RE-QUERY THE RESOURCE.
-                        // WE DO THIS HERE TO ENSURE THE TEST DOESN'T RUN INTO THIS.
-                        state.client.exec_mut(|client| {
-                            client
-                                .dns_records
-                                .retain(|name, _| !matches_domain(&resource.address, name))
-                        });
-                    }
-                }
             }
             Transition::SendDnsQuery {
                 domain,
@@ -402,6 +376,11 @@ impl ReferenceStateMachine for ReferenceState {
                     return false;
                 };
 
+                // Resource IDs must be unique.
+                if state.client.inner().all_resources().contains(&resource.id) {
+                    return false;
+                }
+
                 // TODO: PRODUCTION CODE DOES NOT HANDLE THIS!
 
                 if resource.address.is_ipv6() && gateway.ip6.is_none() {
@@ -423,7 +402,9 @@ impl ReferenceStateMachine for ReferenceState {
                 true
             }
             Transition::AddDnsResource {
-                records, gateway, ..
+                records,
+                gateway,
+                resource,
             } => {
                 if !state.gateways.contains_key(gateway) {
                     return false;
@@ -454,6 +435,22 @@ impl ReferenceStateMachine for ReferenceState {
                     }
                 }
 
+                // Resource IDs must be unique.
+                if state.client.inner().all_resources().contains(&resource.id) {
+                    return false;
+                }
+
+                // Resource addresses must be unique.
+                if state
+                    .client
+                    .inner()
+                    .dns_resources
+                    .values()
+                    .any(|r| r.address == resource.address)
+                {
+                    return false;
+                }
+
                 true
             }
             Transition::Tick { .. } => true,
@@ -573,18 +570,6 @@ impl ReferenceState {
     }
 }
 
-fn matches_domain(resource_address: &str, domain: &DomainName) -> bool {
-    let name = domain.to_string();
-
-    if resource_address.starts_with('*') || resource_address.starts_with('?') {
-        let (_, base) = resource_address.split_once('.').unwrap();
-
-        return name.ends_with(base);
-    }
-
-    name == resource_address
-}
-
 pub(crate) fn private_key() -> impl Strategy<Value = PrivateKey> {
     any::<[u8; 32]>().prop_map(PrivateKey)
 }