From b8f5fb9e251a9e4efe0e04cbee62cb67de5b0cb6 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Sat, 19 Oct 2024 11:02:00 +1100 Subject: [PATCH] chore(connlib): fix proptest failure with TCP DNS (#7093) When we query upstream DNS servers through the tunnel via TCP DNS, we will always be successful in establishing a tunnel, regardless of how many concurrent queries we send because the TCP stack will keep re-trying. Thus, tracking, which resources we are connected to after sending a bunch of DNS queries needs to be split by UDP and TCP. For UDP, only the "first" resource will be connected, however, with concurrent TCP and UDP DNS queries, "first" isn't necessarily the order in which we send the queries because with TCP DNS, one packet doesn't equate to one query anymore. This is quite hacky but it will get completely deleted once we buffer packets during the connection setup. --- .../tunnel/proptest-regressions/tests.txt | 1 + rust/connlib/tunnel/src/tests/reference.rs | 36 +++++++++++++------ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index e00bf7fbe..4a24be945 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -124,3 +124,4 @@ cc 33cd1cba9c6ecf15d6ff86c3114752f2437e432c77f671f67b08116d2b507131 cc d9793b201ec425bd77f9849ea48e63677014aeb4a91a55be9371b81e644b7a24 cc 8fcbd19c41f0483d9b81aac2ab7440bb23d7796ef9f6bf346f73f0d633f65baa cc 4494e475d22ff9a318d676f10c79f545982b7787d145925c3719fe47e9868acc +cc bafb7db795d394d1771ef07f4dd36db8ac1333dd852653900480d7ed03307853 diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index 4d5124104..65851710a 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -299,7 +299,10 @@ impl ReferenceState { } }), Transition::SendDnsQueries(queries) => { - let mut new_connections_via_gateways = BTreeMap::<_, BTreeSet>::new(); + let mut new_connections_via_gateways_udp_triggered = + BTreeMap::<_, BTreeSet>::new(); + let mut new_connections_via_gateways_tcp_triggered = + BTreeMap::<_, BTreeSet>::new(); for query in queries { // Some queries get answered locally. @@ -335,19 +338,27 @@ impl ReferenceState { { tracing::debug!(%resource, %gateway, "Not connected yet, dropping packet"); - let connected_resources = - new_connections_via_gateways.entry(gateway).or_default(); + let connected_resources = match query.transport { + DnsTransport::Udp => &mut new_connections_via_gateways_udp_triggered, + DnsTransport::Tcp => &mut new_connections_via_gateways_tcp_triggered, + } + .entry(gateway) + .or_default(); if state.client.inner().is_connected_gateway(gateway) { connected_resources.insert(resource); } else { - // As part of batch-processing DNS queries, only the first resource per gateway will be connected / authorized. - if connected_resources.is_empty() { - connected_resources.insert(resource); - } - // TCP has retries so we will also be connected to those for sure. - if query.transport == DnsTransport::Tcp { - connected_resources.insert(resource); + match query.transport { + DnsTransport::Udp => { + // As part of batch-processing DNS queries, only the first resource per gateway will be connected / authorized. + if connected_resources.is_empty() { + connected_resources.insert(resource); + } + } + DnsTransport::Tcp => { + // TCP has retries, so those will always connect. + connected_resources.insert(resource); + } } } @@ -357,7 +368,10 @@ impl ReferenceState { state.client.exec_mut(|client| client.on_dns_query(query)); } - for (gateway, resources) in new_connections_via_gateways { + for (gateway, resources) in new_connections_via_gateways_udp_triggered + .into_iter() + .chain(new_connections_via_gateways_tcp_triggered) + { for resource in resources { state.client.exec_mut(|client| { client.connect_to_internet_or_cidr_resource(resource, gateway)