From 99624a4302afffe7b23d406101d334ed66801fa3 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 17 Mar 2025 01:59:32 +1100 Subject: [PATCH] fix(connlib): always update `TunConfig` on any changes (#8453) Currently, we are only emitting updates to the `TunConfig` when the routes or the DNS servers change. This isn't correct, we should also emit updates for it when the IPs or the search-domain changes. In order to achieve that, we create a new `TunConfig` based on the existing one every time we receive an `InterfaceConfig` update. Depending on our current state, we may create an entirely new `TunConfig` or create a new one where we copy the fields in from the new `InterfaceConfig`. We then unconditionally call `maybe_update_tun_config` which does the necessary work to only emit updates when things actually changed. To ensure this works in all cases and the latest update is always reflected on the TUN device, we also extend the proptests to assert the latest search domain. Fixes: #8451 --- .../tunnel/proptest-regressions/tests.txt | 1 + rust/connlib/tunnel/src/client.rs | 43 +++++++++++-------- rust/connlib/tunnel/src/tests/assertions.rs | 9 ++++ rust/connlib/tunnel/src/tests/reference.rs | 13 ++++++ rust/connlib/tunnel/src/tests/sim_client.rs | 16 +++++++ rust/connlib/tunnel/src/tests/stub_portal.rs | 18 +++++--- rust/connlib/tunnel/src/tests/sut.rs | 15 ++++++- rust/connlib/tunnel/src/tests/transition.rs | 2 + 8 files changed, 91 insertions(+), 26 deletions(-) diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index 179b13f60..9c13d8f65 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -164,3 +164,4 @@ cc 4b8aab1f09422751b66d7e46a968bb29fb9b11c8fff9bceb67cd5c8ddeab0a3d cc c48e5d18ae2cc7533bbe1d0cd155a1ec7bcaf00e8d029b0345c241ec3371dcca cc f2de44e6762e9a681d624467fd19ac9fc00f000dfc1c2a3bda05c905b01674c2 cc 36a7bb4eff285399b9c431675d4337712e7edf016a3a02b05cba5115c8bf8fe4 +cc 235333b8c818e464ba339e8c73b2467894d68d594ac896c4f6a36b25ac6b823d diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 32fffa058..8380d67e4 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1045,21 +1045,27 @@ impl ClientState { pub fn update_interface_config(&mut self, config: InterfaceConfig) { tracing::trace!(upstream_dns = ?config.upstream_dns, search_domain = ?config.search_domain, ipv4 = %config.ipv4, ipv6 = %config.ipv6, "Received interface configuration from portal"); - match self.tun_config.as_mut() { - Some(existing) => { - // We don't really expect these to change but let's update them anyway. - existing.ip.v4 = config.ipv4; - existing.ip.v6 = config.ipv6; - - // These are safe to always update - existing.search_domain = config.search_domain.clone(); - } - None => { + // Create a new `TunConfig` by patching the corresponding fields of the existing one. + let new_tun_config = self + .tun_config + .as_ref() + .map(|existing| TunConfig { + ip: IpConfig { + v4: config.ipv4, + v6: config.ipv6, + }, + dns_by_sentinel: existing.dns_by_sentinel.clone(), + search_domain: config.search_domain.clone(), + ipv4_routes: existing.ipv4_routes.clone(), + ipv6_routes: existing.ipv6_routes.clone(), + }) + .unwrap_or_else(|| { let (ipv4_routes, ipv6_routes) = self.routes().partition_map(|route| match route { IpNetwork::V4(v4) => itertools::Either::Left(v4), IpNetwork::V6(v6) => itertools::Either::Right(v6), }); - let new_tun_config = TunConfig { + + TunConfig { ip: IpConfig { v4: config.ipv4, v6: config.ipv6, @@ -1068,16 +1074,14 @@ impl ClientState { search_domain: config.search_domain.clone(), ipv4_routes, ipv6_routes, - }; + } + }); - self.maybe_update_tun_config(new_tun_config); - } - } + // Apply the new `TunConfig` if it differs from the existing one. + self.maybe_update_tun_config(new_tun_config); - self.stub_resolver.set_search_domain(config.search_domain); self.upstream_dns = config.upstream_dns; - - self.update_dns_mapping() + self.update_dns_mapping(); } pub fn poll_packets(&mut self) -> Option { @@ -1487,6 +1491,9 @@ impl ClientState { tracing::info!(config = ?new_tun_config, "Updating TUN device"); + self.stub_resolver + .set_search_domain(new_tun_config.search_domain.clone()); + // Ensure we don't emit multiple interface updates in a row. self.buffered_events .retain(|e| !matches!(e, ClientEvent::TunInterfaceUpdated(_))); diff --git a/rust/connlib/tunnel/src/tests/assertions.rs b/rust/connlib/tunnel/src/tests/assertions.rs index 33382e466..aa86f4944 100644 --- a/rust/connlib/tunnel/src/tests/assertions.rs +++ b/rust/connlib/tunnel/src/tests/assertions.rs @@ -232,6 +232,15 @@ pub(crate) fn assert_dns_servers_are_valid(ref_client: &RefClient, sim_client: & } } +pub(crate) fn assert_search_domain_is_valid(ref_client: &RefClient, sim_client: &SimClient) { + let expected = ref_client.expected_search_domain(); + let actual = sim_client.effective_search_domain(); + + if actual != expected { + tracing::error!(target: "assertions", ?actual, ?expected, "❌ Search domain is incorrect"); + } +} + pub(crate) fn assert_routes_are_valid(ref_client: &RefClient, sim_client: &SimClient) { let (expected_ipv4, expected_ipv6) = ref_client.expected_routes(); let (actual_ipv4, actual_ipv6) = ( diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index 1458d5c15..5caa85d7b 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -187,6 +187,13 @@ impl ReferenceState { 1, upstream_dns_servers().prop_map(Transition::UpdateUpstreamDnsServers), ) + .with( + 1, + state + .portal + .search_domain() + .prop_map(Transition::UpdateUpstreamSearchDomain), + ) .with_if_not_empty( 5, state.all_resources_not_known_to_client(), @@ -451,6 +458,11 @@ impl ReferenceState { .client .exec_mut(|client| client.set_upstream_dns_resolvers(servers)); } + Transition::UpdateUpstreamSearchDomain(domain) => { + state + .client + .exec_mut(|client| client.set_upstream_search_domain(domain.as_ref())); + } Transition::RoamClient { ip4, ip6, .. } => { state.network.remove_host(&state.client); state.client.ip4.clone_from(ip4); @@ -591,6 +603,7 @@ impl ReferenceState { .iter() .any(|dns_server| state.client.sending_socket_for(dns_server.ip()).is_some()) } + Transition::UpdateUpstreamSearchDomain(_) => true, Transition::SendDnsQueries(queries) => queries.iter().all(|query| { let has_socket_for_server = state .client diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index 1075f71ba..b30edb847 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -45,6 +45,9 @@ pub(crate) struct SimClient { pub(crate) ipv4_routes: BTreeSet, pub(crate) ipv6_routes: BTreeSet, + /// The search-domain emitted by connlib. + pub(crate) search_domain: Option, + pub(crate) resource_status: BTreeMap, pub(crate) sent_udp_dns_queries: HashMap<(SocketAddr, QueryId), IpPacket>, @@ -87,6 +90,7 @@ impl SimClient { received_udp_replies: Default::default(), ipv4_routes: Default::default(), ipv6_routes: Default::default(), + search_domain: Default::default(), resource_status: Default::default(), tcp_dns_client, } @@ -97,6 +101,10 @@ impl SimClient { self.dns_by_sentinel.right_values().copied().collect() } + pub(crate) fn effective_search_domain(&self) -> Option { + self.search_domain.clone() + } + pub(crate) fn set_new_dns_servers(&mut self, mapping: BiMap) { self.dns_by_sentinel = mapping; self.tcp_dns_client.reset(); @@ -925,6 +933,10 @@ impl RefClient { .collect() } + pub(crate) fn expected_search_domain(&self) -> Option { + self.search_domain.clone() + } + pub(crate) fn expected_routes(&self) -> (BTreeSet, BTreeSet) { ( self.ipv4_routes @@ -1043,6 +1055,10 @@ impl RefClient { self.upstream_dns_resolvers.clone_from(servers); } + pub(crate) fn set_upstream_search_domain(&mut self, domain: Option<&DomainName>) { + self.search_domain = domain.cloned() + } + pub(crate) fn upstream_dns_resolvers(&self) -> Vec { self.upstream_dns_resolvers.clone() } diff --git a/rust/connlib/tunnel/src/tests/stub_portal.rs b/rust/connlib/tunnel/src/tests/stub_portal.rs index 4829d7bf2..1a6013a48 100644 --- a/rust/connlib/tunnel/src/tests/stub_portal.rs +++ b/rust/connlib/tunnel/src/tests/stub_portal.rs @@ -253,6 +253,16 @@ impl StubPortal { system_dns: impl Strategy>, upstream_dns: impl Strategy>, ) -> impl Strategy> { + ref_client_host( + Just(self.client_tunnel_ipv4), + Just(self.client_tunnel_ipv6), + system_dns, + upstream_dns, + self.search_domain(), + ) + } + + pub(crate) fn search_domain(&self) -> impl Strategy> { let possible_search_domains = self .dns_resources .values() @@ -267,13 +277,7 @@ impl StubPortal { }) .collect::>(); - ref_client_host( - Just(self.client_tunnel_ipv4), - Just(self.client_tunnel_ipv6), - system_dns, - upstream_dns, - proptest::option::of(sample::select(possible_search_domains)), - ) + proptest::option::of(sample::select(possible_search_domains)) } pub(crate) fn dns_resource_records(&self) -> impl Strategy { diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index 5be744712..f83028875 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -240,6 +240,16 @@ impl TunnelTest { }) }); } + Transition::UpdateUpstreamSearchDomain(search_domain) => { + state.client.exec_mut(|c| { + c.sut.update_interface_config(Interface { + ipv4: c.sut.tunnel_ip_config().unwrap().v4, + ipv6: c.sut.tunnel_ip_config().unwrap().v6, + upstream_dns: ref_state.client.inner().upstream_dns_resolvers(), + search_domain, + }) + }); + } Transition::RoamClient { ip4, ip6, port } => { state.network.remove_host(&state.client); state.client.update_interface(ip4, ip6, port); @@ -357,6 +367,7 @@ impl TunnelTest { assert_udp_dns_packets_properties(ref_client, sim_client); assert_tcp_dns(ref_client, sim_client); assert_dns_servers_are_valid(ref_client, sim_client); + assert_search_domain_is_valid(ref_client, sim_client); assert_routes_are_valid(ref_client, sim_client); assert_resource_status(ref_client, sim_client); } @@ -751,9 +762,10 @@ impl TunnelTest { if self.client.inner().dns_mapping() == &config.dns_by_sentinel && self.client.inner().ipv4_routes == config.ipv4_routes && self.client.inner().ipv6_routes == config.ipv6_routes + && self.client.inner().search_domain == config.search_domain { tracing::error!( - "Emitted `TunInterfaceUpdated` without changing DNS servers or routes" + "Emitted `TunInterfaceUpdated` without changing DNS servers, routes or search domain" ); } @@ -770,6 +782,7 @@ impl TunnelTest { c.set_new_dns_servers(config.dns_by_sentinel); c.ipv4_routes = config.ipv4_routes; c.ipv6_routes = config.ipv6_routes; + c.search_domain = config.search_domain }); } } diff --git a/rust/connlib/tunnel/src/tests/transition.rs b/rust/connlib/tunnel/src/tests/transition.rs index d52fa52a6..41e174b69 100644 --- a/rust/connlib/tunnel/src/tests/transition.rs +++ b/rust/connlib/tunnel/src/tests/transition.rs @@ -55,6 +55,8 @@ pub(crate) enum Transition { UpdateSystemDnsServers(Vec), /// The upstream DNS servers changed. UpdateUpstreamDnsServers(Vec), + /// The upstream search domain changed. + UpdateUpstreamSearchDomain(Option), /// Roam the client to a new pair of sockets. RoamClient {