From c34a6800ac06c99f2a34d5278c046ffbb5915e12 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 7 May 2024 01:00:18 -0300 Subject: [PATCH 01/19] add status tracking for resources --- rust/Cargo.lock | 1 + rust/connlib/clients/shared/src/messages.rs | 7 +++- rust/connlib/shared/Cargo.toml | 2 + rust/connlib/shared/src/messages.rs | 1 + rust/connlib/shared/src/messages/client.rs | 29 +++++++++++++ rust/connlib/shared/src/proptest.rs | 46 ++++++++++++--------- rust/connlib/tunnel/src/client.rs | 25 ++++++++++- 7 files changed, 89 insertions(+), 22 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 50bb96cc0c3..f09eee5b04a 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1183,6 +1183,7 @@ dependencies = [ "snownet", "swift-bridge", "tempfile", + "test-strategy", "thiserror", "tokio", "tracing", diff --git a/rust/connlib/clients/shared/src/messages.rs b/rust/connlib/clients/shared/src/messages.rs index f9f1339d494..b4dc70fd20f 100644 --- a/rust/connlib/clients/shared/src/messages.rs +++ b/rust/connlib/clients/shared/src/messages.rs @@ -101,8 +101,7 @@ mod test { use super::*; use chrono::DateTime; use connlib_shared::messages::{ - client::ResourceDescriptionCidr, - client::{GatewayGroup, ResourceDescriptionDns}, + client::{GatewayGroup, ResourceDescriptionCidr, ResourceDescriptionDns, Status}, DnsServer, IpDnsServer, Stun, Turn, }; use phoenix_channel::{OutboundRequestId, PhoenixMessage}; @@ -238,6 +237,7 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], + status: Status::Unknown, }), ResourceDescription::Dns(ResourceDescriptionDns { id: "03000143-e25e-45c7-aafb-144990e57dcd".parse().unwrap(), @@ -248,6 +248,7 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], + status: Status::Unknown, }), ], relays: vec![], @@ -311,6 +312,7 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], + status: Status::Unknown, }), ResourceDescription::Dns(ResourceDescriptionDns { id: "03000143-e25e-45c7-aafb-144990e57dcd".parse().unwrap(), @@ -321,6 +323,7 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], + status: Status::Unknown, }), ], relays: vec![], diff --git a/rust/connlib/shared/Cargo.toml b/rust/connlib/shared/Cargo.toml index 22ae16b9436..80407a1c929 100644 --- a/rust/connlib/shared/Cargo.toml +++ b/rust/connlib/shared/Cargo.toml @@ -7,6 +7,7 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [features] mock = [] +proptest = ["dep:test-strategy", "dep:proptest"] [dependencies] anyhow = "1.0.82" @@ -34,6 +35,7 @@ libc = "0.2" snownet = { workspace = true } phoenix-channel = { workspace = true } proptest = { version = "1.4.0", optional = true } +test-strategy = { version = "0.3.1", optional = true } # Needed for Android logging until tracing is working log = "0.4" diff --git a/rust/connlib/shared/src/messages.rs b/rust/connlib/shared/src/messages.rs index d687bedbf88..3161b74eb94 100644 --- a/rust/connlib/shared/src/messages.rs +++ b/rust/connlib/shared/src/messages.rs @@ -329,6 +329,7 @@ mod tests { name: "test".to_string(), id: "99ba0c1e-5189-4cfc-a4db-fd6cb1c937fd".parse().unwrap(), }], + status: super::client::Status::Unknown, }) } diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 6025bc604dd..3e2e4942132 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -8,6 +8,22 @@ use uuid::Uuid; use super::ResourceId; +// TODO: decide if we keep the same ResourceDescription message or we separate into a non-deserializable thing + +#[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[cfg_attr(feature = "proptest", derive(test_strategy::Arbitrary))] +pub enum Status { + Unknown, + Online, + Offline, +} + +impl Default for Status { + fn default() -> Self { + Status::Unknown + } +} + /// Description of a resource that maps to a DNS record. #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { @@ -22,6 +38,9 @@ pub struct ResourceDescriptionDns { pub address_description: String, pub gateway_groups: Vec, + + #[serde(default)] + pub status: Status, } /// Description of a resource that maps to a CIDR. @@ -38,6 +57,9 @@ pub struct ResourceDescriptionCidr { pub address_description: String, pub gateway_groups: Vec, + + #[serde(default)] + pub status: Status, } #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -106,6 +128,13 @@ impl ResourceDescription { _ => true, } } + + pub fn status(&mut self) -> &mut Status { + match self { + ResourceDescription::Dns(r) => &mut r.status, + ResourceDescription::Cidr(r) => &mut r.status, + } + } } impl PartialOrd for ResourceDescription { diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 79953e1468a..40fc185876e 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -1,6 +1,6 @@ use crate::messages::{ client::ResourceDescriptionCidr, - client::{GatewayGroup, ResourceDescriptionDns, SiteId}, + client::{GatewayGroup, ResourceDescriptionDns, SiteId, Status}, ClientId, ResourceId, }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -18,16 +18,20 @@ pub fn dns_resource() -> impl Strategy { dns_resource_address(), gateway_groups(), address_description(), + any::(), ) - .prop_map(|(id, name, address, gateway_groups, address_description)| { - ResourceDescriptionDns { - id, - address, - name, - gateway_groups, - address_description, - } - }) + .prop_map( + |(id, name, address, gateway_groups, address_description, status)| { + ResourceDescriptionDns { + id, + address, + name, + gateway_groups, + address_description, + status, + } + }, + ) } pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { @@ -37,16 +41,20 @@ pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy(), ) - .prop_map(|(id, name, address, gateway_groups, address_description)| { - ResourceDescriptionCidr { - id, - address, - name, - gateway_groups, - address_description, - } - }) + .prop_map( + |(id, name, address, gateway_groups, address_description, status)| { + ResourceDescriptionCidr { + id, + address, + name, + gateway_groups, + address_description, + status, + } + }, + ) } pub fn address_description() -> impl Strategy { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 0621686d00f..15f5c64edd0 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -2,6 +2,7 @@ use crate::peer_store::PeerStore; use crate::{dns, dns::DnsQuery}; use bimap::BiMap; use connlib_shared::error::{ConnlibError as Error, ConnlibError}; +use connlib_shared::messages::client::Status; use connlib_shared::messages::{ client::ResourceDescription, client::ResourceDescriptionCidr, client::ResourceDescriptionDns, Answer, ClientPayload, DnsServer, DomainResponse, GatewayId, Interface as InterfaceConfig, @@ -174,6 +175,13 @@ where self.role_state.on_connection_failed(id); } + pub fn offline_resource(&mut self, id: ResourceId) { + let Some(resource) = self.role_state.resource_ids.get_mut(&id) else { + return; + }; + *resource.status() = Status::Offline; + } + pub fn add_ice_candidate(&mut self, conn_id: GatewayId, ice_candidate: String) { self.role_state .node @@ -834,6 +842,7 @@ impl ClientState { match event { snownet::Event::ConnectionFailed(id) => { self.cleanup_connected_gateway(&id); + self.set_gateways_resources_status(id, Status::Unknown); } snownet::Event::NewIceCandidate { connection, @@ -853,11 +862,24 @@ impl ClientState { conn_id: connection, candidate, }), - snownet::Event::ConnectionEstablished { .. } => {} + snownet::Event::ConnectionEstablished(id) => { + self.set_gateways_resources_status(id, Status::Online); + } } } } + fn set_gateways_resources_status(&mut self, gateway_id: GatewayId, status: Status) { + self.resource_ids + .iter_mut() + .filter(|(r_id, _)| { + self.resources_gateways + .get(r_id) + .is_some_and(|g_id| *g_id == gateway_id) + }) + .for_each(|(_, r)| *r.status() = status); + } + pub(crate) fn poll_event(&mut self) -> Option { self.buffered_events.pop_front() } @@ -1491,6 +1513,7 @@ mod proptests { name: resource.name, address_description: resource.address_description, gateway_groups: resource.gateway_groups, + status: resource.status, }; client_state.add_resources(&[ResourceDescription::Cidr(dns_as_cidr_resource.clone())]); From 77906c0f197a34fb609109090b38c74b40aba461 Mon Sep 17 00:00:00 2001 From: conectado Date: Wed, 8 May 2024 21:14:41 -0300 Subject: [PATCH 02/19] feat(connlib): report resource status to clients --- rust/connlib/shared/src/messages/client.rs | 18 +++++++- rust/connlib/tunnel/src/client.rs | 52 +++++++++++++++++----- rust/connlib/tunnel/src/lib.rs | 2 + rust/headless-client/src/imp_linux.rs | 2 +- rust/headless-client/src/lib.rs | 10 ++++- 5 files changed, 69 insertions(+), 15 deletions(-) diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 3e2e4942132..ffe96141e1a 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -1,6 +1,6 @@ //! Client related messages that are needed within connlib -use std::{borrow::Cow, str::FromStr}; +use std::{borrow::Cow, collections::HashSet, str::FromStr}; use ip_network::IpNetwork; use serde::{Deserialize, Serialize}; @@ -101,6 +101,13 @@ impl ResourceDescription { } } + pub fn gateway_groups(&self) -> HashSet<&GatewayGroup> { + match self { + ResourceDescription::Dns(r) => HashSet::from_iter(r.gateway_groups.iter()), + ResourceDescription::Cidr(r) => HashSet::from_iter(r.gateway_groups.iter()), + } + } + /// What the GUI clients should show as the user-friendly display name, e.g. `Firezone GitHub` pub fn name(&self) -> &str { match self { @@ -129,12 +136,19 @@ impl ResourceDescription { } } - pub fn status(&mut self) -> &mut Status { + pub fn status_mut(&mut self) -> &mut Status { match self { ResourceDescription::Dns(r) => &mut r.status, ResourceDescription::Cidr(r) => &mut r.status, } } + + pub fn status(&self) -> Status { + match self { + ResourceDescription::Dns(r) => r.status, + ResourceDescription::Cidr(r) => r.status, + } + } } impl PartialOrd for ResourceDescription { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 15f5c64edd0..d8829632f22 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -176,10 +176,15 @@ where } pub fn offline_resource(&mut self, id: ResourceId) { - let Some(resource) = self.role_state.resource_ids.get_mut(&id) else { + let Some(resource) = self.role_state.resource_ids.get(&id).cloned() else { return; }; - *resource.status() = Status::Offline; + + self.role_state + .update_resources_status(&resource, Status::Offline); + + self.callbacks + .on_update_resources(self.role_state.resources()); } pub fn add_ice_candidate(&mut self, conn_id: GatewayId, ice_candidate: String) { @@ -317,10 +322,18 @@ impl ClientState { } } - fn resources(&self) -> Vec { + pub(crate) fn resources(&self) -> Vec { self.resource_ids.values().sorted().cloned().collect_vec() } + // This updates the status for all resources sharing the same gateway groups + fn update_resources_status(&mut self, resource: &ResourceDescription, status: Status) { + self.resource_ids + .values_mut() + .filter(|r| r.gateway_groups() == resource.gateway_groups()) + .for_each(|r| *r.status_mut() = status); + } + pub(crate) fn encapsulate<'s>( &'s mut self, packet: MutableIpPacket<'_>, @@ -870,14 +883,21 @@ impl ClientState { } fn set_gateways_resources_status(&mut self, gateway_id: GatewayId, status: Status) { - self.resource_ids - .iter_mut() - .filter(|(r_id, _)| { + let resources = self + .resource_ids + .iter() + .filter_map(|(r_id, r)| { self.resources_gateways .get(r_id) .is_some_and(|g_id| *g_id == gateway_id) + .then_some(r) }) - .for_each(|(_, r)| *r.status() = status); + .cloned() + .collect_vec(); + + for r in resources { + self.update_resources_status(&r, status); + } } pub(crate) fn poll_event(&mut self) -> Option { @@ -924,12 +944,20 @@ impl ClientState { pub(crate) fn add_resources(&mut self, resources: &[ResourceDescription]) { for resource_description in resources { + let mut resource_description = resource_description.clone(); + if let Some(resource) = self.resource_ids.get(&resource_description.id()) { - if resource.has_different_address(resource_description) { + if resource.has_different_address(&resource_description) { self.remove_resources(&[resource.id()]); } } + if let Some(status) = self.resources().iter().find_map(|r| { + (r.gateway_groups() == resource_description.gateway_groups()).then_some(r.status()) + }) { + *resource_description.status_mut() = status; + } + match &resource_description { ResourceDescription::Dns(dns) => { self.dns_resources.insert(dns.address.clone(), dns.clone()); @@ -940,7 +968,7 @@ impl ClientState { } self.resource_ids - .insert(resource_description.id(), resource_description.clone()); + .insert(resource_description.id(), resource_description); } } @@ -953,7 +981,7 @@ impl ClientState { self.cidr_resources.retain(|_, r| r.id != *id); self.deferred_dns_queries.retain(|(r, _), _| r.id != *id); - self.resource_ids.remove(id); + let resource = self.resource_ids.remove(id); let Some(gateway_id) = self.resources_gateways.remove(id) else { tracing::debug!("No gateway associated with resource"); @@ -987,6 +1015,10 @@ impl ClientState { // If there's no allowed ip left we remove the whole peer because there's no point on keeping it around if peer.allowed_ips.is_empty() { self.peers.remove(&gateway_id); + // TODO: multi-site resources would need a bit extra thought here + if let Some(resource) = resource { + self.update_resources_status(&resource, Status::Unknown); + } // TODO: should we have a Node::remove_connection? } } diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index 608e5ad2dd9..5d4451eb138 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -119,6 +119,8 @@ where )? { Poll::Ready(io::Input::Timeout(timeout)) => { self.role_state.handle_timeout(timeout); + self.callbacks + .on_update_resources(self.role_state.resources()); continue; } Poll::Ready(io::Input::Device(packet)) => { diff --git a/rust/headless-client/src/imp_linux.rs b/rust/headless-client/src/imp_linux.rs index 37502e25c79..19fbaaa0aa5 100644 --- a/rust/headless-client/src/imp_linux.rs +++ b/rust/headless-client/src/imp_linux.rs @@ -241,7 +241,7 @@ impl Callbacks for CallbackHandlerIpc { } fn on_update_resources(&self, resources: Vec) { - tracing::info!(len = resources.len(), "New resource list"); + tracing::info!(?resources, "New resource list"); self.cb_tx .try_send(IpcServerMsg::OnUpdateResources(resources)) .expect("Should be able to send OnUpdateResources"); diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index f57ec8bbed5..fd74e25f0ae 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -13,9 +13,10 @@ use clap::Parser; use connlib_client_shared::{ file_logger, keypair, Callbacks, LoginUrl, ResourceDescription, Session, Sockets, }; +use connlib_shared::messages::client::Status; use firezone_cli_utils::setup_global_subscriber; use secrecy::SecretString; -use std::{future, net::IpAddr, path::PathBuf, task::Poll}; +use std::{collections::HashMap, future, net::IpAddr, path::PathBuf, task::Poll}; use tokio::sync::mpsc; use imp::default_token_path; @@ -276,7 +277,12 @@ impl Callbacks for CallbackHandler { fn on_update_resources(&self, resources: Vec) { // See easily with `export RUST_LOG=firezone_headless_client=debug` - tracing::debug!(len = resources.len(), "Printing the resource list one time"); + + let status = resources + .iter() + .map(|r| (r.name(), r.status())) + .collect::>(); + tracing::info!(?status, "Resource Status"); for resource in &resources { tracing::debug!(?resource); } From c04331cdebd52ff72a35b1bf49a7dffc227ab805 Mon Sep 17 00:00:00 2001 From: conectado Date: Wed, 8 May 2024 22:27:23 -0300 Subject: [PATCH 03/19] fix clippy --- rust/connlib/shared/src/messages/client.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index ffe96141e1a..9a2b1b79c2f 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -10,20 +10,17 @@ use super::ResourceId; // TODO: decide if we keep the same ResourceDescription message or we separate into a non-deserializable thing -#[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive( + Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Default, +)] #[cfg_attr(feature = "proptest", derive(test_strategy::Arbitrary))] pub enum Status { + #[default] Unknown, Online, Offline, } -impl Default for Status { - fn default() -> Self { - Status::Unknown - } -} - /// Description of a resource that maps to a DNS record. #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { From 00a5aaa4368258ca9633036a8e8cd88f85b2c578 Mon Sep 17 00:00:00 2001 From: conectado Date: Thu, 9 May 2024 01:31:24 -0300 Subject: [PATCH 04/19] call offline resources on offline response --- rust/connlib/clients/shared/src/eventloop.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index 9aa2654d937..93bd18febdf 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -321,6 +321,7 @@ where tracing::debug!(resource_id = %offline_resource, "Resource is offline"); + self.tunnel.offline_resource(offline_resource); self.tunnel.cleanup_connection(offline_resource); } From 1ddd42006223cb317165ce0aa8a4969cda876c52 Mon Sep 17 00:00:00 2001 From: conectado Date: Fri, 10 May 2024 01:12:36 -0300 Subject: [PATCH 05/19] wip: checkpoint keeping track of status by group id --- rust/connlib/clients/apple/src/lib.rs | 4 +- rust/connlib/clients/shared/src/eventloop.rs | 11 +- rust/connlib/clients/shared/src/messages.rs | 12 +- rust/connlib/shared/src/callbacks.rs | 103 ++++++++++++++- rust/connlib/shared/src/lib.rs | 2 +- rust/connlib/shared/src/messages.rs | 1 - rust/connlib/shared/src/messages/client.rs | 33 +---- rust/connlib/shared/src/proptest.rs | 81 +++++++----- rust/connlib/tunnel/src/client.rs | 126 ++++++++++++------- 9 files changed, 251 insertions(+), 122 deletions(-) diff --git a/rust/connlib/clients/apple/src/lib.rs b/rust/connlib/clients/apple/src/lib.rs index 8df91c862e3..243e8a99f81 100644 --- a/rust/connlib/clients/apple/src/lib.rs +++ b/rust/connlib/clients/apple/src/lib.rs @@ -2,8 +2,8 @@ #![allow(clippy::unnecessary_cast, improper_ctypes, non_camel_case_types)] use connlib_client_shared::{ - file_logger, keypair, Callbacks, Cidrv4, Cidrv6, Error, LoginUrl, ResourceDescription, Session, - Sockets, + file_logger, keypair, Callbacks, Callbacks::ResourceDescription, Cidrv4, Cidrv6, Error, + LoginUrl, Session, Sockets, }; use secrecy::SecretString; use std::{ diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index 93bd18febdf..c0e7165a301 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -269,6 +269,7 @@ where gateway_id, resource_id, relays, + gateway_group_id, .. }) => { let should_accept = self @@ -280,10 +281,12 @@ where return; } - match self - .tunnel - .create_or_reuse_connection(resource_id, gateway_id, relays) - { + match self.tunnel.create_or_reuse_connection( + resource_id, + gateway_id, + relays, + gateway_group_id, + ) { Ok(firezone_tunnel::Request::NewConnection(connection_request)) => { // TODO: keep track for the response let _id = self.portal.send( diff --git a/rust/connlib/clients/shared/src/messages.rs b/rust/connlib/clients/shared/src/messages.rs index b4dc70fd20f..14a1612694a 100644 --- a/rust/connlib/clients/shared/src/messages.rs +++ b/rust/connlib/clients/shared/src/messages.rs @@ -1,6 +1,7 @@ use connlib_shared::messages::{ - client::ResourceDescription, GatewayId, GatewayResponse, Interface, Key, Relay, RelaysPresence, - RequestConnection, ResourceId, ReuseConnection, + client::{ResourceDescription, SiteId}, + GatewayId, GatewayResponse, Interface, Key, Relay, RelaysPresence, RequestConnection, + ResourceId, ReuseConnection, }; use serde::{Deserialize, Serialize}; use std::{collections::HashSet, net::IpAddr}; @@ -25,6 +26,7 @@ pub struct ConnectionDetails { pub resource_id: ResourceId, pub gateway_id: GatewayId, pub gateway_remote_ip: IpAddr, + pub gateway_group_id: SiteId, } #[derive(Debug, Deserialize, Clone, PartialEq)] @@ -237,7 +239,6 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], - status: Status::Unknown, }), ResourceDescription::Dns(ResourceDescriptionDns { id: "03000143-e25e-45c7-aafb-144990e57dcd".parse().unwrap(), @@ -248,7 +249,6 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], - status: Status::Unknown, }), ], relays: vec![], @@ -312,7 +312,6 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], - status: Status::Unknown, }), ResourceDescription::Dns(ResourceDescriptionDns { id: "03000143-e25e-45c7-aafb-144990e57dcd".parse().unwrap(), @@ -323,7 +322,6 @@ mod test { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], - status: Status::Unknown, }), ], relays: vec![], @@ -539,6 +537,7 @@ mod test { gateway_id: "73037362-715d-4a83-a749-f18eadd970e6".parse().unwrap(), gateway_remote_ip: "172.28.0.1".parse().unwrap(), resource_id: "f16ecfa0-a94f-4bfd-a2ef-1cc1f2ef3da3".parse().unwrap(), + gateway_group_id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), relays: vec![ Relay::Stun(Stun { id: "c9cb8892-e355-41e6-a882-b6d6c38beb66".parse().unwrap(), @@ -576,6 +575,7 @@ mod test { "resource_id": "f16ecfa0-a94f-4bfd-a2ef-1cc1f2ef3da3", "gateway_id": "73037362-715d-4a83-a749-f18eadd970e6", "gateway_remote_ip": "172.28.0.1", + "gateway_group_id": "bf56f32d-7b2c-4f5d-a784-788977d014a4", "relays": [ { "id": "c9cb8892-e355-41e6-a882-b6d6c38beb66", diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index 084537a133c..bd18530c7cb 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -1,9 +1,11 @@ -use crate::messages::client::ResourceDescription; -use ip_network::{Ipv4Network, Ipv6Network}; +use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use serde::Serialize; use std::fmt::Debug; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; +use crate::messages::client::GatewayGroup; +use crate::messages::ResourceId; + // Avoids having to map types for Windows type RawFd = i32; @@ -39,6 +41,103 @@ impl From for Cidrv6 { } } +#[derive(Debug, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub enum Status { + Unknown, + Online, + Offline, +} + +#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum ResourceDescription { + Dns(ResourceDescriptionDns), + Cidr(ResourceDescriptionCidr), +} + +impl ResourceDescription { + pub fn with_status( + r: crate::messages::client::ResourceDescription, + status: Status, + ) -> ResourceDescription { + match r { + crate::messages::client::ResourceDescription::Dns(r) => { + ResourceDescription::Dns(ResourceDescriptionDns::with_status(r, status)) + } + crate::messages::client::ResourceDescription::Cidr(r) => { + ResourceDescription::Cidr(ResourceDescriptionCidr::with_status(r, status)) + } + } + } +} + +#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash)] +pub struct ResourceDescriptionDns { + /// Resource's id. + pub id: ResourceId, + /// Internal resource's domain name. + pub address: String, + /// Name of the resource. + /// + /// Used only for display. + pub name: String, + + pub address_description: String, + pub gateway_groups: Vec, + + pub status: Status, +} + +impl ResourceDescriptionDns { + fn with_status( + r: crate::messages::client::ResourceDescriptionDns, + status: Status, + ) -> ResourceDescriptionDns { + ResourceDescriptionDns { + id: r.id, + address: r.address, + name: r.name, + address_description: r.address_description, + gateway_groups: r.gateway_groups, + status, + } + } +} + +/// Description of a resource that maps to a CIDR. +#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct ResourceDescriptionCidr { + /// Resource's id. + pub id: ResourceId, + /// CIDR that this resource points to. + pub address: IpNetwork, + /// Name of the resource. + /// + /// Used only for display. + pub name: String, + + pub address_description: String, + pub gateway_groups: Vec, + + pub status: Status, +} + +impl ResourceDescriptionCidr { + fn with_status( + r: crate::messages::client::ResourceDescriptionCidr, + status: Status, + ) -> ResourceDescriptionCidr { + ResourceDescriptionCidr { + id: r.id, + address: r.address, + name: r.name, + address_description: r.address_description, + gateway_groups: r.gateway_groups, + status, + } + } +} + /// Traits that will be used by connlib to callback the client upper layers. pub trait Callbacks: Clone + Send + Sync { /// Called when the tunnel address is set. diff --git a/rust/connlib/shared/src/lib.rs b/rust/connlib/shared/src/lib.rs index 87185dd2e20..9f7130d7bf7 100644 --- a/rust/connlib/shared/src/lib.rs +++ b/rust/connlib/shared/src/lib.rs @@ -3,7 +3,7 @@ //! This includes types provided by external crates, i.e. [boringtun] to make sure that //! we are using the same version across our own crates. -mod callbacks; +pub mod callbacks; pub mod error; pub mod messages; diff --git a/rust/connlib/shared/src/messages.rs b/rust/connlib/shared/src/messages.rs index 3161b74eb94..d687bedbf88 100644 --- a/rust/connlib/shared/src/messages.rs +++ b/rust/connlib/shared/src/messages.rs @@ -329,7 +329,6 @@ mod tests { name: "test".to_string(), id: "99ba0c1e-5189-4cfc-a4db-fd6cb1c937fd".parse().unwrap(), }], - status: super::client::Status::Unknown, }) } diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 9a2b1b79c2f..4d672a28d08 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -10,17 +10,6 @@ use super::ResourceId; // TODO: decide if we keep the same ResourceDescription message or we separate into a non-deserializable thing -#[derive( - Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Default, -)] -#[cfg_attr(feature = "proptest", derive(test_strategy::Arbitrary))] -pub enum Status { - #[default] - Unknown, - Online, - Offline, -} - /// Description of a resource that maps to a DNS record. #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { @@ -35,9 +24,6 @@ pub struct ResourceDescriptionDns { pub address_description: String, pub gateway_groups: Vec, - - #[serde(default)] - pub status: Status, } /// Description of a resource that maps to a CIDR. @@ -54,9 +40,6 @@ pub struct ResourceDescriptionCidr { pub address_description: String, pub gateway_groups: Vec, - - #[serde(default)] - pub status: Status, } #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -65,7 +48,7 @@ pub struct GatewayGroup { pub id: SiteId, } -#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct SiteId(Uuid); impl FromStr for SiteId { @@ -132,20 +115,6 @@ impl ResourceDescription { _ => true, } } - - pub fn status_mut(&mut self) -> &mut Status { - match self { - ResourceDescription::Dns(r) => &mut r.status, - ResourceDescription::Cidr(r) => &mut r.status, - } - } - - pub fn status(&self) -> Status { - match self { - ResourceDescription::Dns(r) => r.status, - ResourceDescription::Cidr(r) => r.status, - } - } } impl PartialOrd for ResourceDescription { diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 40fc185876e..f4957352eca 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -1,6 +1,6 @@ use crate::messages::{ client::ResourceDescriptionCidr, - client::{GatewayGroup, ResourceDescriptionDns, SiteId, Status}, + client::{GatewayGroup, ResourceDescription, ResourceDescriptionDns, SiteId, Status}, ClientId, ResourceId, }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -11,50 +11,73 @@ use proptest::{ }; use std::net::{Ipv4Addr, Ipv6Addr}; -pub fn dns_resource() -> impl Strategy { +pub fn resource(gateway_groups: Vec) -> impl Strategy { + any::().prop_flat_map(move |is_dns| { + if is_dns { + dns_resource_with_groups(gateway_groups.clone()) + .prop_map(|r| ResourceDescription::Dns(r)) + .boxed() + } else { + cidr_resource_with_groups(8, gateway_groups.clone()) + .prop_map(|r| ResourceDescription::Cidr(r)) + .boxed() + } + }) +} + +pub fn dns_resource_with_groups( + gateway_groups: Vec, +) -> impl Strategy { ( resource_id(), resource_name(), dns_resource_address(), - gateway_groups(), address_description(), any::(), ) - .prop_map( - |(id, name, address, gateway_groups, address_description, status)| { - ResourceDescriptionDns { - id, - address, - name, - gateway_groups, - address_description, - status, - } - }, - ) + .prop_map(move |(id, name, address, address_description, status)| { + ResourceDescriptionDns { + id, + address, + name, + gateway_groups: gateway_groups.clone(), + address_description, + status, + } + }) } -pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { +pub fn cidr_resource_with_groups( + host_mask_bits: usize, + gateway_groups: Vec, +) -> impl Strategy { ( resource_id(), resource_name(), ip_network(host_mask_bits), - gateway_groups(), address_description(), any::(), ) - .prop_map( - |(id, name, address, gateway_groups, address_description, status)| { - ResourceDescriptionCidr { - id, - address, - name, - gateway_groups, - address_description, - status, - } - }, - ) + .prop_map(move |(id, name, address, address_description, status)| { + ResourceDescriptionCidr { + id, + address, + name, + gateway_groups: gateway_groups.clone(), + address_description, + status, + } + }) +} + +pub fn dns_resource() -> impl Strategy { + gateway_groups().prop_flat_map(|gateway_groups| dns_resource_with_groups(gateway_groups)) +} + +pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { + gateway_groups().prop_flat_map(move |gateway_groups| { + cidr_resource_with_groups(host_mask_bits, gateway_groups) + }) } pub fn address_description() -> impl Strategy { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index d8829632f22..a83386e1d2f 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1,14 +1,15 @@ use crate::peer_store::PeerStore; use crate::{dns, dns::DnsQuery}; use bimap::BiMap; +use connlib_shared::callbacks::Status; use connlib_shared::error::{ConnlibError as Error, ConnlibError}; -use connlib_shared::messages::client::Status; +use connlib_shared::messages::client::{GatewayGroup, SiteId}; use connlib_shared::messages::{ client::ResourceDescription, client::ResourceDescriptionCidr, client::ResourceDescriptionDns, Answer, ClientPayload, DnsServer, DomainResponse, GatewayId, Interface as InterfaceConfig, IpDnsServer, Key, Offer, Relay, RelayId, RequestConnection, ResourceId, ReuseConnection, }; -use connlib_shared::{Callbacks, Dname, PublicKey, StaticSecret}; +use connlib_shared::{callbacks, Callbacks, Dname, PublicKey, StaticSecret}; use domain::base::Rtype; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; @@ -180,8 +181,9 @@ where return; }; - self.role_state - .update_resources_status(&resource, Status::Offline); + for GatewayGroup { id, .. } in resource.gateway_groups() { + self.role_state.sites_status.insert(*id, Status::Offline); + } self.callbacks .on_update_resources(self.role_state.resources()); @@ -204,7 +206,9 @@ where resource_id: ResourceId, gateway_id: GatewayId, relays: Vec, + site_id: SiteId, ) -> connlib_shared::Result { + self.role_state.gateways_site.insert(gateway_id, site_id); self.role_state.create_or_reuse_connection( resource_id, gateway_id, @@ -290,6 +294,9 @@ pub struct ClientState { next_dns_refresh: Option, system_resolvers: Vec, + + gateways_site: HashMap, + sites_status: HashMap, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -319,19 +326,41 @@ impl ClientState { next_dns_refresh: Default::default(), node: ClientNode::new(private_key), system_resolvers: Default::default(), + sites_status: Default::default(), + gateways_site: Default::default(), } } - pub(crate) fn resources(&self) -> Vec { - self.resource_ids.values().sorted().cloned().collect_vec() + pub(crate) fn resources(&self) -> Vec { + self.resource_ids + .values() + .sorted() + .cloned() + .map(|r| { + let status = self.status(&r); + callbacks::ResourceDescription::with_status(r, status) + }) + .collect_vec() } - // This updates the status for all resources sharing the same gateway groups - fn update_resources_status(&mut self, resource: &ResourceDescription, status: Status) { - self.resource_ids - .values_mut() - .filter(|r| r.gateway_groups() == resource.gateway_groups()) - .for_each(|r| *r.status_mut() = status); + fn status(&self, resource: &ResourceDescription) -> Status { + if resource.gateway_groups().iter().any(|s| { + self.sites_status + .get(&s.id) + .is_some_and(|s| *s == Status::Online) + }) { + return Status::Online; + } + + if resource.gateway_groups().iter().all(|s| { + self.sites_status + .get(&s.id) + .is_some_and(|s| *s == Status::Offline) + }) { + return Status::Offline; + } + + Status::Unknown } pub(crate) fn encapsulate<'s>( @@ -855,7 +884,7 @@ impl ClientState { match event { snownet::Event::ConnectionFailed(id) => { self.cleanup_connected_gateway(&id); - self.set_gateways_resources_status(id, Status::Unknown); + self.update_site_status_by_gateway(&id, Status::Unknown) } snownet::Event::NewIceCandidate { connection, @@ -876,28 +905,22 @@ impl ClientState { candidate, }), snownet::Event::ConnectionEstablished(id) => { - self.set_gateways_resources_status(id, Status::Online); + self.update_site_status_by_gateway(&id, Status::Online); } } } } - fn set_gateways_resources_status(&mut self, gateway_id: GatewayId, status: Status) { - let resources = self - .resource_ids - .iter() - .filter_map(|(r_id, r)| { - self.resources_gateways - .get(r_id) - .is_some_and(|g_id| *g_id == gateway_id) - .then_some(r) - }) - .cloned() - .collect_vec(); - - for r in resources { - self.update_resources_status(&r, status); - } + fn update_site_status_by_gateway(&mut self, gateway_id: &GatewayId, status: Status) { + // Note: we can do this because in theory we shouldn't have multiple gateways for the same site + // connected at the same time. + self.sites_status.insert( + *self + .gateways_site + .get(gateway_id) + .expect("inconsistent state"), + status, + ); } pub(crate) fn poll_event(&mut self) -> Option { @@ -944,20 +967,12 @@ impl ClientState { pub(crate) fn add_resources(&mut self, resources: &[ResourceDescription]) { for resource_description in resources { - let mut resource_description = resource_description.clone(); - if let Some(resource) = self.resource_ids.get(&resource_description.id()) { if resource.has_different_address(&resource_description) { self.remove_resources(&[resource.id()]); } } - if let Some(status) = self.resources().iter().find_map(|r| { - (r.gateway_groups() == resource_description.gateway_groups()).then_some(r.status()) - }) { - *resource_description.status_mut() = status; - } - match &resource_description { ResourceDescription::Dns(dns) => { self.dns_resources.insert(dns.address.clone(), dns.clone()); @@ -968,7 +983,7 @@ impl ClientState { } self.resource_ids - .insert(resource_description.id(), resource_description); + .insert(resource_description.id(), resource_description.clone()); } } @@ -981,7 +996,7 @@ impl ClientState { self.cidr_resources.retain(|_, r| r.id != *id); self.deferred_dns_queries.retain(|(r, _), _| r.id != *id); - let resource = self.resource_ids.remove(id); + self.resource_ids.remove(id); let Some(gateway_id) = self.resources_gateways.remove(id) else { tracing::debug!("No gateway associated with resource"); @@ -1015,10 +1030,7 @@ impl ClientState { // If there's no allowed ip left we remove the whole peer because there's no point on keeping it around if peer.allowed_ips.is_empty() { self.peers.remove(&gateway_id); - // TODO: multi-site resources would need a bit extra thought here - if let Some(resource) = resource { - self.update_resources_status(&resource, Status::Unknown); - } + self.update_site_status_by_gateway(&gateway_id, Status::Unknown); // TODO: should we have a Node::remove_connection? } } @@ -1545,7 +1557,6 @@ mod proptests { name: resource.name, address_description: resource.address_description, gateway_groups: resource.gateway_groups, - status: resource.status, }; client_state.add_resources(&[ResourceDescription::Cidr(dns_as_cidr_resource.clone())]); @@ -1618,4 +1629,29 @@ mod proptests { expected_routes(vec![cidr_resource2.address]) ); } + + // #[test_strategy::proptest] + // fn setting_status_for_a_resource_updates_any_resource_with_the_same_gateway_group( + // #[strategy(cidr_resource(8))] resource: ResourceDescriptionCidr, + // #[strategy(ip_network(8))] new_address: IpNetwork, + // ) { + // let mut client_state = ClientState::for_test(); + // client_state.add_resources(&[ResourceDescription::Cidr(resource.clone())]); + + // let updated_resource = ResourceDescriptionCidr { + // address: new_address, + // ..resource + // }; + + // client_state.add_resources(&[ResourceDescription::Cidr(updated_resource.clone())]); + + // assert_eq!( + // hashset(client_state.resources().iter()), + // hashset(&[ResourceDescription::Cidr(updated_resource),]) + // ); + // assert_eq!( + // hashset(client_state.routes()), + // expected_routes(vec![new_address]) + // ); + // } } From 843cbfa928bccbe7da7e5819e44448416525edec Mon Sep 17 00:00:00 2001 From: conectado Date: Fri, 10 May 2024 20:37:34 -0300 Subject: [PATCH 06/19] fix previous commit --- rust/connlib/clients/android/src/lib.rs | 4 +- rust/connlib/clients/apple/src/lib.rs | 2 +- rust/connlib/clients/shared/src/lib.rs | 2 +- rust/connlib/clients/shared/src/messages.rs | 2 +- rust/connlib/shared/src/callbacks.rs | 40 ++++++++++++++++--- rust/connlib/shared/src/messages/client.rs | 10 +---- rust/gui-client/src-tauri/src/client/gui.rs | 2 +- .../src/client/gui/system_tray_menu.rs | 2 +- .../src/client/tunnel-wrapper/ipc.rs | 3 +- rust/headless-client/src/imp_linux.rs | 6 +-- rust/headless-client/src/lib.rs | 13 +++--- 11 files changed, 55 insertions(+), 31 deletions(-) diff --git a/rust/connlib/clients/android/src/lib.rs b/rust/connlib/clients/android/src/lib.rs index 94cdefd0869..4855add95ec 100644 --- a/rust/connlib/clients/android/src/lib.rs +++ b/rust/connlib/clients/android/src/lib.rs @@ -4,8 +4,8 @@ // ecosystem, so it's used here for consistency. use connlib_client_shared::{ - file_logger, keypair, Callbacks, Cidrv4, Cidrv6, Error, LoginUrl, LoginUrlError, - ResourceDescription, Session, Sockets, + callbacks::ResourceDescription, file_logger, keypair, Callbacks, Cidrv4, Cidrv6, Error, + LoginUrl, LoginUrlError, Session, Sockets, }; use jni::{ objects::{GlobalRef, JClass, JObject, JString, JValue}, diff --git a/rust/connlib/clients/apple/src/lib.rs b/rust/connlib/clients/apple/src/lib.rs index 243e8a99f81..afa93c0c90d 100644 --- a/rust/connlib/clients/apple/src/lib.rs +++ b/rust/connlib/clients/apple/src/lib.rs @@ -2,7 +2,7 @@ #![allow(clippy::unnecessary_cast, improper_ctypes, non_camel_case_types)] use connlib_client_shared::{ - file_logger, keypair, Callbacks, Callbacks::ResourceDescription, Cidrv4, Cidrv6, Error, + callbacks::ResourceDescription, file_logger, keypair, Callbacks, Cidrv4, Cidrv6, Error, LoginUrl, Session, Sockets, }; use secrecy::SecretString; diff --git a/rust/connlib/clients/shared/src/lib.rs b/rust/connlib/clients/shared/src/lib.rs index 44a0935b602..7cc4ccfaac2 100644 --- a/rust/connlib/clients/shared/src/lib.rs +++ b/rust/connlib/clients/shared/src/lib.rs @@ -1,7 +1,7 @@ //! Main connlib library for clients. pub use connlib_shared::messages::client::ResourceDescription; pub use connlib_shared::{ - keypair, Callbacks, Cidrv4, Cidrv6, Error, LoginUrl, LoginUrlError, StaticSecret, + callbacks, keypair, Callbacks, Cidrv4, Cidrv6, Error, LoginUrl, LoginUrlError, StaticSecret, }; pub use firezone_tunnel::Sockets; pub use tracing_appender::non_blocking::WorkerGuard; diff --git a/rust/connlib/clients/shared/src/messages.rs b/rust/connlib/clients/shared/src/messages.rs index 14a1612694a..8938ac77790 100644 --- a/rust/connlib/clients/shared/src/messages.rs +++ b/rust/connlib/clients/shared/src/messages.rs @@ -103,7 +103,7 @@ mod test { use super::*; use chrono::DateTime; use connlib_shared::messages::{ - client::{GatewayGroup, ResourceDescriptionCidr, ResourceDescriptionDns, Status}, + client::{GatewayGroup, ResourceDescriptionCidr, ResourceDescriptionDns}, DnsServer, IpDnsServer, Stun, Turn, }; use phoenix_channel::{OutboundRequestId, PhoenixMessage}; diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index bd18530c7cb..d7b81a887d6 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -1,5 +1,6 @@ use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; -use serde::Serialize; +use serde::{Deserialize, Serialize}; +use std::borrow::Cow; use std::fmt::Debug; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; @@ -41,14 +42,14 @@ impl From for Cidrv6 { } } -#[derive(Debug, Serialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum Status { Unknown, Online, Offline, } -#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash)] #[serde(tag = "type", rename_all = "snake_case")] pub enum ResourceDescription { Dns(ResourceDescriptionDns), @@ -69,9 +70,38 @@ impl ResourceDescription { } } } + + pub fn name(&self) -> &str { + match self { + ResourceDescription::Dns(r) => &r.name, + ResourceDescription::Cidr(r) => &r.name, + } + } + + pub fn status(&self) -> Status { + match self { + ResourceDescription::Dns(r) => r.status, + ResourceDescription::Cidr(r) => r.status, + } + } + + pub fn id(&self) -> ResourceId { + match self { + ResourceDescription::Dns(r) => r.id, + ResourceDescription::Cidr(r) => r.id, + } + } + + /// What the GUI clients should paste to the clipboard, e.g. `https://github.com/firezone` + pub fn pastable(&self) -> Cow<'_, str> { + match self { + ResourceDescription::Dns(r) => Cow::from(&r.address), + ResourceDescription::Cidr(r) => Cow::from(r.address.to_string()), + } + } } -#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { /// Resource's id. pub id: ResourceId, @@ -105,7 +135,7 @@ impl ResourceDescriptionDns { } /// Description of a resource that maps to a CIDR. -#[derive(Debug, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ResourceDescriptionCidr { /// Resource's id. pub id: ResourceId, diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 4d672a28d08..558e279f8ce 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -1,6 +1,6 @@ //! Client related messages that are needed within connlib -use std::{borrow::Cow, collections::HashSet, str::FromStr}; +use std::{collections::HashSet, str::FromStr}; use ip_network::IpNetwork; use serde::{Deserialize, Serialize}; @@ -96,14 +96,6 @@ impl ResourceDescription { } } - /// What the GUI clients should paste to the clipboard, e.g. `https://github.com/firezone` - pub fn pastable(&self) -> Cow<'_, str> { - match self { - ResourceDescription::Dns(r) => Cow::from(&r.address), - ResourceDescription::Cidr(r) => Cow::from(r.address.to_string()), - } - } - pub fn has_different_address(&self, other: &ResourceDescription) -> bool { match (self, other) { (ResourceDescription::Dns(dns_a), ResourceDescription::Dns(dns_b)) => { diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 12b4ba394bb..53988e29135 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -9,7 +9,7 @@ use crate::client::{ Failure, }; use anyhow::{bail, Context, Result}; -use connlib_client_shared::ResourceDescription; +use connlib_client_shared::callbacks::ResourceDescription; use connlib_shared::messages::ResourceId; use secrecy::{ExposeSecret, SecretString}; use std::{path::PathBuf, str::FromStr, sync::Arc, time::Duration}; diff --git a/rust/gui-client/src-tauri/src/client/gui/system_tray_menu.rs b/rust/gui-client/src-tauri/src/client/gui/system_tray_menu.rs index c94daeccd32..65b7c889124 100644 --- a/rust/gui-client/src-tauri/src/client/gui/system_tray_menu.rs +++ b/rust/gui-client/src-tauri/src/client/gui/system_tray_menu.rs @@ -3,7 +3,7 @@ //! "Notification Area" is Microsoft's official name instead of "System tray": //! -use connlib_client_shared::ResourceDescription; +use connlib_client_shared::callbacks::ResourceDescription; use std::str::FromStr; use tauri::{CustomMenuItem, SystemTrayMenu, SystemTrayMenuItem, SystemTraySubmenu}; diff --git a/rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs b/rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs index 73a2f1428aa..3b7b66f30e2 100644 --- a/rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs +++ b/rust/gui-client/src-tauri/src/client/tunnel-wrapper/ipc.rs @@ -1,6 +1,7 @@ use anyhow::{Context, Result}; use arc_swap::ArcSwap; -use connlib_client_shared::{Callbacks, ResourceDescription}; +use connlib_client_shared::Callbacks; +use connlib_shared::callbacks::ResourceDescription; use firezone_headless_client::{imp::sock_path, IpcClientMsg, IpcServerMsg}; use futures::{SinkExt, StreamExt}; use secrecy::{ExposeSecret, SecretString}; diff --git a/rust/headless-client/src/imp_linux.rs b/rust/headless-client/src/imp_linux.rs index 19fbaaa0aa5..0b4f204a544 100644 --- a/rust/headless-client/src/imp_linux.rs +++ b/rust/headless-client/src/imp_linux.rs @@ -3,9 +3,9 @@ use super::{Cli, IpcClientMsg, IpcServerMsg, FIREZONE_GROUP, TOKEN_ENV_KEY}; use anyhow::{bail, Context as _, Result}; use clap::Parser; -use connlib_client_shared::{file_logger, Callbacks, ResourceDescription, Sockets}; +use connlib_client_shared::{file_logger, Callbacks, Sockets}; use connlib_shared::{ - keypair, + callbacks, keypair, linux::{etc_resolv_conf, get_dns_control_from_env, DnsControlMethod}, LoginUrl, }; @@ -240,7 +240,7 @@ impl Callbacks for CallbackHandlerIpc { None } - fn on_update_resources(&self, resources: Vec) { + fn on_update_resources(&self, resources: Vec) { tracing::info!(?resources, "New resource list"); self.cb_tx .try_send(IpcServerMsg::OnUpdateResources(resources)) diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index fd74e25f0ae..8e12a34553c 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -10,10 +10,8 @@ use anyhow::{Context, Result}; use clap::Parser; -use connlib_client_shared::{ - file_logger, keypair, Callbacks, LoginUrl, ResourceDescription, Session, Sockets, -}; -use connlib_shared::messages::client::Status; +use connlib_client_shared::{file_logger, keypair, Callbacks, LoginUrl, Session, Sockets}; +use connlib_shared::callbacks::{self, Status}; use firezone_cli_utils::setup_global_subscriber; use secrecy::SecretString; use std::{collections::HashMap, future, net::IpAddr, path::PathBuf, task::Poll}; @@ -134,7 +132,7 @@ pub enum IpcClientMsg { pub enum IpcServerMsg { Ok, OnDisconnect, - OnUpdateResources(Vec), + OnUpdateResources(Vec), TunnelReady, } @@ -275,7 +273,10 @@ impl Callbacks for CallbackHandler { .expect("should be able to tell the main thread that we disconnected"); } - fn on_update_resources(&self, resources: Vec) { + fn on_update_resources( + &self, + resources: Vec, + ) { // See easily with `export RUST_LOG=firezone_headless_client=debug` let status = resources From 78bb80a88acbcd37d285f8a6f03ef8b6f3d03818 Mon Sep 17 00:00:00 2001 From: conectado Date: Fri, 10 May 2024 22:12:13 -0300 Subject: [PATCH 07/19] fix proptests --- rust/connlib/shared/src/callbacks.rs | 37 +++++++++++++++++++ rust/connlib/shared/src/proptest.rs | 22 +++++------- rust/connlib/tunnel/src/client.rs | 54 +++++++++++++++++++++------- 3 files changed, 88 insertions(+), 25 deletions(-) diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index d7b81a887d6..94045b14585 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -101,6 +101,19 @@ impl ResourceDescription { } } +impl From for crate::messages::client::ResourceDescription { + fn from(value: ResourceDescription) -> Self { + match value { + ResourceDescription::Dns(r) => { + crate::messages::client::ResourceDescription::Dns(r.into()) + } + ResourceDescription::Cidr(r) => { + crate::messages::client::ResourceDescription::Cidr(r.into()) + } + } + } +} + #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { /// Resource's id. @@ -134,6 +147,18 @@ impl ResourceDescriptionDns { } } +impl From for crate::messages::client::ResourceDescriptionDns { + fn from(r: ResourceDescriptionDns) -> Self { + crate::messages::client::ResourceDescriptionDns { + id: r.id, + address: r.address, + address_description: r.address_description, + name: r.name, + gateway_groups: r.gateway_groups, + } + } +} + /// Description of a resource that maps to a CIDR. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ResourceDescriptionCidr { @@ -168,6 +193,18 @@ impl ResourceDescriptionCidr { } } +impl From for crate::messages::client::ResourceDescriptionCidr { + fn from(r: ResourceDescriptionCidr) -> Self { + crate::messages::client::ResourceDescriptionCidr { + id: r.id, + address: r.address, + address_description: r.address_description, + name: r.name, + gateway_groups: r.gateway_groups, + } + } +} + /// Traits that will be used by connlib to callback the client upper layers. pub trait Callbacks: Clone + Send + Sync { /// Called when the tunnel address is set. diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index f4957352eca..2bc812f7457 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -1,6 +1,6 @@ use crate::messages::{ client::ResourceDescriptionCidr, - client::{GatewayGroup, ResourceDescription, ResourceDescriptionDns, SiteId, Status}, + client::{GatewayGroup, ResourceDescription, ResourceDescriptionDns, SiteId}, ClientId, ResourceId, }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -33,18 +33,16 @@ pub fn dns_resource_with_groups( resource_name(), dns_resource_address(), address_description(), - any::(), ) - .prop_map(move |(id, name, address, address_description, status)| { - ResourceDescriptionDns { + .prop_map( + move |(id, name, address, address_description)| ResourceDescriptionDns { id, address, name, gateway_groups: gateway_groups.clone(), address_description, - status, - } - }) + }, + ) } pub fn cidr_resource_with_groups( @@ -56,18 +54,16 @@ pub fn cidr_resource_with_groups( resource_name(), ip_network(host_mask_bits), address_description(), - any::(), ) - .prop_map(move |(id, name, address, address_description, status)| { - ResourceDescriptionCidr { + .prop_map( + move |(id, name, address, address_description)| ResourceDescriptionCidr { id, address, name, gateway_groups: gateway_groups.clone(), address_description, - status, - } - }) + }, + ) } pub fn dns_resource() -> impl Strategy { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index a83386e1d2f..4d95f5ccb23 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1499,8 +1499,13 @@ mod proptests { ]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ ResourceDescription::Cidr(resource1.clone()), ResourceDescription::Dns(resource2.clone()) ]) @@ -1509,8 +1514,13 @@ mod proptests { client_state.add_resources(&[ResourceDescription::Cidr(resource3.clone())]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ ResourceDescription::Cidr(resource1), ResourceDescription::Dns(resource2), ResourceDescription::Cidr(resource3) @@ -1534,8 +1544,13 @@ mod proptests { client_state.add_resources(&[ResourceDescription::Cidr(updated_resource.clone())]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ResourceDescription::Cidr(updated_resource),]) + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ResourceDescription::Cidr(updated_resource),]) ); assert_eq!( hashset(client_state.routes()), @@ -1562,8 +1577,13 @@ mod proptests { client_state.add_resources(&[ResourceDescription::Cidr(dns_as_cidr_resource.clone())]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ResourceDescription::Cidr(dns_as_cidr_resource),]) + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ResourceDescription::Cidr(dns_as_cidr_resource),]) ); assert_eq!( hashset(client_state.routes()), @@ -1585,8 +1605,13 @@ mod proptests { client_state.remove_resources(&[dns_resource.id]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ResourceDescription::Cidr(cidr_resource.clone())]) + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ResourceDescription::Cidr(cidr_resource.clone())]) ); assert_eq!( hashset(client_state.routes()), @@ -1618,8 +1643,13 @@ mod proptests { ]); assert_eq!( - hashset(client_state.resources().iter()), - hashset(&[ + hashset( + client_state + .resources() + .into_iter() + .map_into::() + ), + hashset([ ResourceDescription::Dns(dns_resource2), ResourceDescription::Cidr(cidr_resource2.clone()), ]) From 9b8c3f7a6614b16cec0d65784ef99bf3c5144737 Mon Sep 17 00:00:00 2001 From: conectado Date: Sat, 11 May 2024 00:01:34 -0300 Subject: [PATCH 08/19] add proptest for status changes --- rust/connlib/shared/Cargo.toml | 5 +- rust/connlib/shared/src/messages.rs | 7 ++ rust/connlib/shared/src/proptest.rs | 33 ++++++- rust/connlib/tunnel/src/client.rs | 137 +++++++++++++++++++++------- 4 files changed, 147 insertions(+), 35 deletions(-) diff --git a/rust/connlib/shared/Cargo.toml b/rust/connlib/shared/Cargo.toml index 80407a1c929..f11e82ea8f4 100644 --- a/rust/connlib/shared/Cargo.toml +++ b/rust/connlib/shared/Cargo.toml @@ -7,7 +7,7 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [features] mock = [] -proptest = ["dep:test-strategy", "dep:proptest"] +proptest = ["dep:test-strategy", "dep:proptest", "dep:itertools"] [dependencies] anyhow = "1.0.82" @@ -36,13 +36,14 @@ snownet = { workspace = true } phoenix-channel = { workspace = true } proptest = { version = "1.4.0", optional = true } test-strategy = { version = "0.3.1", optional = true } +itertools = { version = "0.12", optional = true } # Needed for Android logging until tracing is working log = "0.4" [dev-dependencies] -itertools = "0.12" tempfile = "3.10.1" +itertools = "0.12" mutants = "0.0.3" # Needed to mark functions as exempt from `cargo-mutants` testing tokio = { version = "1.36", features = ["macros", "rt"] } diff --git a/rust/connlib/shared/src/messages.rs b/rust/connlib/shared/src/messages.rs index d687bedbf88..8ba77ce9df8 100644 --- a/rust/connlib/shared/src/messages.rs +++ b/rust/connlib/shared/src/messages.rs @@ -50,6 +50,13 @@ impl ClientId { } } +impl GatewayId { + #[cfg(feature = "proptest")] + pub(crate) fn from_u128(v: u128) -> Self { + Self(Uuid::from_u128(v)) + } +} + #[derive(Hash, Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq)] pub struct ClientId(Uuid); diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 2bc812f7457..819740428fd 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -1,16 +1,41 @@ use crate::messages::{ client::ResourceDescriptionCidr, client::{GatewayGroup, ResourceDescription, ResourceDescriptionDns, SiteId}, - ClientId, ResourceId, + ClientId, GatewayId, ResourceId, }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; +use itertools::Itertools; use proptest::{ arbitrary::{any, any_with}, collection, sample, - strategy::Strategy, + strategy::{Just, Strategy}, }; use std::net::{Ipv4Addr, Ipv6Addr}; +// Generate resources sharing 1 gateway group +pub fn resources_sharing_group() -> impl Strategy, GatewayGroup)> +{ + (collection::vec(gateway_groups(), 1..=100), gateway_group()).prop_flat_map(|(groups, g)| { + ( + groups + .iter() + .map(|gs| { + let mut groups = gs.clone(); + groups.push(g.clone()); + resource(groups.clone()) + }) + .collect_vec(), + Just(g), + ) + }) +} + +// Generate resources sharing all gateway groups +pub fn resources_sharing_all_groups() -> impl Strategy> { + gateway_groups() + .prop_flat_map(|gateway_groups| collection::vec(resource(gateway_groups.clone()), 1..=100)) +} + pub fn resource(gateway_groups: Vec) -> impl Strategy { any::().prop_flat_map(move |is_dns| { if is_dns { @@ -95,6 +120,10 @@ pub fn resource_id() -> impl Strategy + Clone { any::().prop_map(ResourceId::from_u128) } +pub fn gateway_id() -> impl Strategy + Clone { + any::().prop_map(GatewayId::from_u128) +} + pub fn client_id() -> impl Strategy { any::().prop_map(ClientId::from_u128) } diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 4d95f5ccb23..078268c5a7d 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -177,13 +177,7 @@ where } pub fn offline_resource(&mut self, id: ResourceId) { - let Some(resource) = self.role_state.resource_ids.get(&id).cloned() else { - return; - }; - - for GatewayGroup { id, .. } in resource.gateway_groups() { - self.role_state.sites_status.insert(*id, Status::Offline); - } + self.role_state.offline_resource(id); self.callbacks .on_update_resources(self.role_state.resources()); @@ -363,6 +357,16 @@ impl ClientState { Status::Unknown } + fn offline_resource(&mut self, id: ResourceId) { + let Some(resource) = self.resource_ids.get(&id).cloned() else { + return; + }; + + for GatewayGroup { id, .. } in resource.gateway_groups() { + self.sites_status.insert(*id, Status::Offline); + } + } + pub(crate) fn encapsulate<'s>( &'s mut self, packet: MutableIpPacket<'_>, @@ -1660,28 +1664,99 @@ mod proptests { ); } - // #[test_strategy::proptest] - // fn setting_status_for_a_resource_updates_any_resource_with_the_same_gateway_group( - // #[strategy(cidr_resource(8))] resource: ResourceDescriptionCidr, - // #[strategy(ip_network(8))] new_address: IpNetwork, - // ) { - // let mut client_state = ClientState::for_test(); - // client_state.add_resources(&[ResourceDescription::Cidr(resource.clone())]); - - // let updated_resource = ResourceDescriptionCidr { - // address: new_address, - // ..resource - // }; - - // client_state.add_resources(&[ResourceDescription::Cidr(updated_resource.clone())]); - - // assert_eq!( - // hashset(client_state.resources().iter()), - // hashset(&[ResourceDescription::Cidr(updated_resource),]) - // ); - // assert_eq!( - // hashset(client_state.routes()), - // expected_routes(vec![new_address]) - // ); - // } + #[test_strategy::proptest] + fn setting_gateway_online_sets_all_related_resources_online( + #[strategy(resources_sharing_group())] resource_config_online: ( + Vec, + GatewayGroup, + ), + #[strategy(resources_sharing_group())] resource_config_unknown: ( + Vec, + GatewayGroup, + ), + #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, + ) { + let (resources_online, gateway_group) = resource_config_online; + let (resources_unknown, _) = resource_config_unknown; + let mut client_state = ClientState::for_test(); + client_state.add_resources(&resources_online); + client_state.add_resources(&resources_unknown); + client_state.resources_gateways.insert( + resources_online.first().unwrap().id(), + first_resource_gateway_id, + ); + client_state + .gateways_site + .insert(first_resource_gateway_id, gateway_group.id); + + client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Online); + + for resource in resources_online { + assert_eq!(client_state.status(&resource), Status::Online); + } + + for resource in resources_unknown { + assert_eq!(client_state.status(&resource), Status::Unknown); + } + } + + #[test_strategy::proptest] + fn disconnecting_gateway_sets_related_resources_unknown( + #[strategy(resources_sharing_group())] resource_config: ( + Vec, + GatewayGroup, + ), + #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, + ) { + let (resources, gateway_group) = resource_config; + let mut client_state = ClientState::for_test(); + client_state.add_resources(&resources); + client_state + .resources_gateways + .insert(resources.first().unwrap().id(), first_resource_gateway_id); + client_state + .gateways_site + .insert(first_resource_gateway_id, gateway_group.id); + + client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Online); + client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Unknown); + + for resource in resources { + assert_eq!(client_state.status(&resource), Status::Unknown); + } + } + + #[test_strategy::proptest] + fn setting_resource_offline_doesnt_set_all_related_resources_offline( + #[strategy(resources_sharing_group())] resource_config_online: ( + Vec, + GatewayGroup, + ), + ) { + let (mut resources, _) = resource_config_online; + let mut client_state = ClientState::for_test(); + client_state.add_resources(&resources); + let resource_offline = resources.pop().unwrap(); + + client_state.offline_resource(resource_offline.id()); + + assert_eq!(client_state.status(&resource_offline), Status::Offline); + for resource in resources { + assert_eq!(client_state.status(&resource), Status::Unknown); + } + } + + #[test_strategy::proptest] + fn setting_resource_offline_set_all_resources_sharing_all_groups_offline( + #[strategy(resources_sharing_all_groups())] resources: Vec, + ) { + let mut client_state = ClientState::for_test(); + client_state.add_resources(&resources); + + client_state.offline_resource(resources.first().unwrap().id()); + + for resource in resources { + assert_eq!(client_state.status(&resource), Status::Offline); + } + } } From d907a48462be23988ceab1fb58c35fa6e333a129 Mon Sep 17 00:00:00 2001 From: conectado Date: Mon, 13 May 2024 20:27:14 -0300 Subject: [PATCH 09/19] fix gui compilation --- .../gui-client/src-tauri/src/client/tunnel-wrapper/in_proc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/gui-client/src-tauri/src/client/tunnel-wrapper/in_proc.rs b/rust/gui-client/src-tauri/src/client/tunnel-wrapper/in_proc.rs index c1ae0d06481..4b71dda98b4 100644 --- a/rust/gui-client/src-tauri/src/client/tunnel-wrapper/in_proc.rs +++ b/rust/gui-client/src-tauri/src/client/tunnel-wrapper/in_proc.rs @@ -10,8 +10,8 @@ use anyhow::{Context, Result}; use arc_swap::ArcSwap; -use connlib_client_shared::{ResourceDescription, Sockets}; -use connlib_shared::{keypair, LoginUrl}; +use connlib_client_shared::Sockets; +use connlib_shared::{callbacks::ResourceDescription, keypair, LoginUrl}; use secrecy::SecretString; use std::{ net::{IpAddr, Ipv4Addr, Ipv6Addr}, From b66f453853927cdb478b0fe7bab6b22e28f58371 Mon Sep 17 00:00:00 2001 From: conectado Date: Mon, 13 May 2024 22:14:26 -0300 Subject: [PATCH 10/19] fix clippy --- rust/Cargo.lock | 1 - rust/connlib/shared/Cargo.toml | 3 +-- rust/connlib/shared/src/proptest.rs | 8 ++++---- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index f09eee5b04a..50bb96cc0c3 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1183,7 +1183,6 @@ dependencies = [ "snownet", "swift-bridge", "tempfile", - "test-strategy", "thiserror", "tokio", "tracing", diff --git a/rust/connlib/shared/Cargo.toml b/rust/connlib/shared/Cargo.toml index f11e82ea8f4..b9fc3bbf03d 100644 --- a/rust/connlib/shared/Cargo.toml +++ b/rust/connlib/shared/Cargo.toml @@ -7,7 +7,7 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [features] mock = [] -proptest = ["dep:test-strategy", "dep:proptest", "dep:itertools"] +proptest = ["dep:proptest", "dep:itertools"] [dependencies] anyhow = "1.0.82" @@ -35,7 +35,6 @@ libc = "0.2" snownet = { workspace = true } phoenix-channel = { workspace = true } proptest = { version = "1.4.0", optional = true } -test-strategy = { version = "0.3.1", optional = true } itertools = { version = "0.12", optional = true } # Needed for Android logging until tracing is working diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 819740428fd..890f99341af 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -33,18 +33,18 @@ pub fn resources_sharing_group() -> impl Strategy impl Strategy> { gateway_groups() - .prop_flat_map(|gateway_groups| collection::vec(resource(gateway_groups.clone()), 1..=100)) + .prop_flat_map(|gateway_groups| collection::vec(resource(gateway_groups), 1..=100)) } pub fn resource(gateway_groups: Vec) -> impl Strategy { any::().prop_flat_map(move |is_dns| { if is_dns { dns_resource_with_groups(gateway_groups.clone()) - .prop_map(|r| ResourceDescription::Dns(r)) + .prop_map(ResourceDescription::Dns) .boxed() } else { cidr_resource_with_groups(8, gateway_groups.clone()) - .prop_map(|r| ResourceDescription::Cidr(r)) + .prop_map(ResourceDescription::Cidr) .boxed() } }) @@ -92,7 +92,7 @@ pub fn cidr_resource_with_groups( } pub fn dns_resource() -> impl Strategy { - gateway_groups().prop_flat_map(|gateway_groups| dns_resource_with_groups(gateway_groups)) + gateway_groups().prop_flat_map(dns_resource_with_groups) } pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { From 6fdafa2d5b91137ce595fcbeb742b24a6043d2c0 Mon Sep 17 00:00:00 2001 From: conectado Date: Mon, 13 May 2024 22:46:37 -0300 Subject: [PATCH 11/19] more clippy fixes --- rust/connlib/tunnel/src/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 078268c5a7d..9f471f74ae5 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -972,7 +972,7 @@ impl ClientState { pub(crate) fn add_resources(&mut self, resources: &[ResourceDescription]) { for resource_description in resources { if let Some(resource) = self.resource_ids.get(&resource_description.id()) { - if resource.has_different_address(&resource_description) { + if resource.has_different_address(resource_description) { self.remove_resources(&[resource.id()]); } } From e99671ab6fb4a6ac056c22b9466baedf020c354d Mon Sep 17 00:00:00 2001 From: conectado Date: Mon, 13 May 2024 22:59:06 -0300 Subject: [PATCH 12/19] remove stale todo --- rust/connlib/shared/src/messages/client.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 558e279f8ce..c688b77e9b4 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -8,8 +8,6 @@ use uuid::Uuid; use super::ResourceId; -// TODO: decide if we keep the same ResourceDescription message or we separate into a non-deserializable thing - /// Description of a resource that maps to a DNS record. #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash)] pub struct ResourceDescriptionDns { From b8ff9461d8b0fa60a4a2c353a2a66c3fc48b3ee7 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 17:47:54 -0300 Subject: [PATCH 13/19] apply pr feedback --- rust/connlib/clients/shared/src/eventloop.rs | 6 +-- rust/connlib/clients/shared/src/messages.rs | 15 +++--- rust/connlib/shared/src/callbacks.rs | 54 ++----------------- rust/connlib/shared/src/messages.rs | 4 +- rust/connlib/shared/src/messages/client.rs | 50 ++++++++++++++++-- rust/connlib/shared/src/proptest.rs | 19 ++++--- rust/connlib/tunnel/src/client.rs | 55 +++++++++++--------- 7 files changed, 100 insertions(+), 103 deletions(-) diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index c0e7165a301..1b8a42c2b05 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -269,7 +269,7 @@ where gateway_id, resource_id, relays, - gateway_group_id, + site_id, .. }) => { let should_accept = self @@ -285,7 +285,7 @@ where resource_id, gateway_id, relays, - gateway_group_id, + site_id, ) { Ok(firezone_tunnel::Request::NewConnection(connection_request)) => { // TODO: keep track for the response @@ -324,7 +324,7 @@ where tracing::debug!(resource_id = %offline_resource, "Resource is offline"); - self.tunnel.offline_resource(offline_resource); + self.tunnel.set_resource_offline(offline_resource); self.tunnel.cleanup_connection(offline_resource); } diff --git a/rust/connlib/clients/shared/src/messages.rs b/rust/connlib/clients/shared/src/messages.rs index 8938ac77790..58615d09d1f 100644 --- a/rust/connlib/clients/shared/src/messages.rs +++ b/rust/connlib/clients/shared/src/messages.rs @@ -26,7 +26,8 @@ pub struct ConnectionDetails { pub resource_id: ResourceId, pub gateway_id: GatewayId, pub gateway_remote_ip: IpAddr, - pub gateway_group_id: SiteId, + #[serde(rename = "gateway_group_id")] + pub site_id: SiteId, } #[derive(Debug, Deserialize, Clone, PartialEq)] @@ -103,7 +104,7 @@ mod test { use super::*; use chrono::DateTime; use connlib_shared::messages::{ - client::{GatewayGroup, ResourceDescriptionCidr, ResourceDescriptionDns}, + client::{ResourceDescriptionCidr, ResourceDescriptionDns, Site}, DnsServer, IpDnsServer, Stun, Turn, }; use phoenix_channel::{OutboundRequestId, PhoenixMessage}; @@ -235,7 +236,7 @@ mod test { address: "172.172.0.0/16".parse().unwrap(), name: "172.172.0.0/16".to_string(), address_description: "cidr resource".to_string(), - gateway_groups: vec![GatewayGroup { + gateway_groups: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], @@ -245,7 +246,7 @@ mod test { address: "gitlab.mycorp.com".to_string(), name: "gitlab.mycorp.com".to_string(), address_description: "dns resource".to_string(), - gateway_groups: vec![GatewayGroup { + sites: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], @@ -308,7 +309,7 @@ mod test { address: "172.172.0.0/16".parse().unwrap(), name: "172.172.0.0/16".to_string(), address_description: "cidr resource".to_string(), - gateway_groups: vec![GatewayGroup { + gateway_groups: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], @@ -318,7 +319,7 @@ mod test { address: "gitlab.mycorp.com".to_string(), name: "gitlab.mycorp.com".to_string(), address_description: "dns resource".to_string(), - gateway_groups: vec![GatewayGroup { + sites: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], @@ -537,7 +538,7 @@ mod test { gateway_id: "73037362-715d-4a83-a749-f18eadd970e6".parse().unwrap(), gateway_remote_ip: "172.28.0.1".parse().unwrap(), resource_id: "f16ecfa0-a94f-4bfd-a2ef-1cc1f2ef3da3".parse().unwrap(), - gateway_group_id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), + site_id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), relays: vec![ Relay::Stun(Stun { id: "c9cb8892-e355-41e6-a882-b6d6c38beb66".parse().unwrap(), diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index 94045b14585..5d12e0913dc 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; use std::fmt::Debug; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; -use crate::messages::client::GatewayGroup; +use crate::messages::client::Site; use crate::messages::ResourceId; // Avoids having to map types for Windows @@ -57,20 +57,6 @@ pub enum ResourceDescription { } impl ResourceDescription { - pub fn with_status( - r: crate::messages::client::ResourceDescription, - status: Status, - ) -> ResourceDescription { - match r { - crate::messages::client::ResourceDescription::Dns(r) => { - ResourceDescription::Dns(ResourceDescriptionDns::with_status(r, status)) - } - crate::messages::client::ResourceDescription::Cidr(r) => { - ResourceDescription::Cidr(ResourceDescriptionCidr::with_status(r, status)) - } - } - } - pub fn name(&self) -> &str { match self { ResourceDescription::Dns(r) => &r.name, @@ -126,27 +112,11 @@ pub struct ResourceDescriptionDns { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + pub gateway_groups: Vec, pub status: Status, } -impl ResourceDescriptionDns { - fn with_status( - r: crate::messages::client::ResourceDescriptionDns, - status: Status, - ) -> ResourceDescriptionDns { - ResourceDescriptionDns { - id: r.id, - address: r.address, - name: r.name, - address_description: r.address_description, - gateway_groups: r.gateway_groups, - status, - } - } -} - impl From for crate::messages::client::ResourceDescriptionDns { fn from(r: ResourceDescriptionDns) -> Self { crate::messages::client::ResourceDescriptionDns { @@ -154,7 +124,7 @@ impl From for crate::messages::client::ResourceDescripti address: r.address, address_description: r.address_description, name: r.name, - gateway_groups: r.gateway_groups, + sites: r.gateway_groups, } } } @@ -172,27 +142,11 @@ pub struct ResourceDescriptionCidr { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + pub gateway_groups: Vec, pub status: Status, } -impl ResourceDescriptionCidr { - fn with_status( - r: crate::messages::client::ResourceDescriptionCidr, - status: Status, - ) -> ResourceDescriptionCidr { - ResourceDescriptionCidr { - id: r.id, - address: r.address, - name: r.name, - address_description: r.address_description, - gateway_groups: r.gateway_groups, - status, - } - } -} - impl From for crate::messages::client::ResourceDescriptionCidr { fn from(r: ResourceDescriptionCidr) -> Self { crate::messages::client::ResourceDescriptionCidr { diff --git a/rust/connlib/shared/src/messages.rs b/rust/connlib/shared/src/messages.rs index 8ba77ce9df8..0d1b725b376 100644 --- a/rust/connlib/shared/src/messages.rs +++ b/rust/connlib/shared/src/messages.rs @@ -322,7 +322,7 @@ mod tests { use super::{ client::ResourceDescription, - client::{GatewayGroup, ResourceDescriptionDns}, + client::{ResourceDescriptionDns, Site}, ResourceId, }; @@ -332,7 +332,7 @@ mod tests { name: name.to_string(), address: "unused.example.com".to_string(), address_description: "test description".to_string(), - gateway_groups: vec![GatewayGroup { + sites: vec![Site { name: "test".to_string(), id: "99ba0c1e-5189-4cfc-a4db-fd6cb1c937fd".parse().unwrap(), }], diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index c688b77e9b4..3b5af7cc340 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -6,6 +6,8 @@ use ip_network::IpNetwork; use serde::{Deserialize, Serialize}; use uuid::Uuid; +use crate::callbacks::Status; + use super::ResourceId; /// Description of a resource that maps to a DNS record. @@ -21,7 +23,21 @@ pub struct ResourceDescriptionDns { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + #[serde(rename = "gateway_groups")] + pub sites: Vec, +} + +impl ResourceDescriptionDns { + fn with_status(self, status: Status) -> crate::callbacks::ResourceDescriptionDns { + crate::callbacks::ResourceDescriptionDns { + id: self.id, + address: self.address, + name: self.name, + address_description: self.address_description, + gateway_groups: self.sites, + status, + } + } } /// Description of a resource that maps to a CIDR. @@ -37,11 +53,24 @@ pub struct ResourceDescriptionCidr { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + pub gateway_groups: Vec, +} + +impl ResourceDescriptionCidr { + fn with_status(self, status: Status) -> crate::callbacks::ResourceDescriptionCidr { + crate::callbacks::ResourceDescriptionCidr { + id: self.id, + address: self.address, + name: self.name, + address_description: self.address_description, + gateway_groups: self.gateway_groups, + status, + } + } } #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] -pub struct GatewayGroup { +pub struct Site { pub name: String, pub id: SiteId, } @@ -79,9 +108,9 @@ impl ResourceDescription { } } - pub fn gateway_groups(&self) -> HashSet<&GatewayGroup> { + pub fn gateway_groups(&self) -> HashSet<&Site> { match self { - ResourceDescription::Dns(r) => HashSet::from_iter(r.gateway_groups.iter()), + ResourceDescription::Dns(r) => HashSet::from_iter(r.sites.iter()), ResourceDescription::Cidr(r) => HashSet::from_iter(r.gateway_groups.iter()), } } @@ -105,6 +134,17 @@ impl ResourceDescription { _ => true, } } + + pub fn with_status(self, status: Status) -> crate::callbacks::ResourceDescription { + match self { + ResourceDescription::Dns(r) => { + crate::callbacks::ResourceDescription::Dns(r.with_status(status)) + } + ResourceDescription::Cidr(r) => { + crate::callbacks::ResourceDescription::Cidr(r.with_status(status)) + } + } + } } impl PartialOrd for ResourceDescription { diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 890f99341af..50e9907edf7 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -1,6 +1,6 @@ use crate::messages::{ client::ResourceDescriptionCidr, - client::{GatewayGroup, ResourceDescription, ResourceDescriptionDns, SiteId}, + client::{ResourceDescription, ResourceDescriptionDns, Site, SiteId}, ClientId, GatewayId, ResourceId, }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -13,8 +13,7 @@ use proptest::{ use std::net::{Ipv4Addr, Ipv6Addr}; // Generate resources sharing 1 gateway group -pub fn resources_sharing_group() -> impl Strategy, GatewayGroup)> -{ +pub fn resources_sharing_group() -> impl Strategy, Site)> { (collection::vec(gateway_groups(), 1..=100), gateway_group()).prop_flat_map(|(groups, g)| { ( groups @@ -36,7 +35,7 @@ pub fn resources_sharing_all_groups() -> impl Strategy) -> impl Strategy { +pub fn resource(gateway_groups: Vec) -> impl Strategy { any::().prop_flat_map(move |is_dns| { if is_dns { dns_resource_with_groups(gateway_groups.clone()) @@ -51,7 +50,7 @@ pub fn resource(gateway_groups: Vec) -> impl Strategy, + gateway_groups: Vec, ) -> impl Strategy { ( resource_id(), @@ -64,7 +63,7 @@ pub fn dns_resource_with_groups( id, address, name, - gateway_groups: gateway_groups.clone(), + sites: gateway_groups.clone(), address_description, }, ) @@ -72,7 +71,7 @@ pub fn dns_resource_with_groups( pub fn cidr_resource_with_groups( host_mask_bits: usize, - gateway_groups: Vec, + gateway_groups: Vec, ) -> impl Strategy { ( resource_id(), @@ -105,12 +104,12 @@ pub fn address_description() -> impl Strategy { any_with::("[a-z]{4,10}".into()) } -pub fn gateway_groups() -> impl Strategy> { +pub fn gateway_groups() -> impl Strategy> { collection::vec(gateway_group(), 1..=10) } -pub fn gateway_group() -> impl Strategy { - (any_with::("[a-z]{4,10}".into()), any::()).prop_map(|(name, id)| GatewayGroup { +pub fn gateway_group() -> impl Strategy { + (any_with::("[a-z]{4,10}".into()), any::()).prop_map(|(name, id)| Site { name, id: SiteId::from_u128(id), }) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 9f471f74ae5..2adc8f1378d 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -3,7 +3,7 @@ use crate::{dns, dns::DnsQuery}; use bimap::BiMap; use connlib_shared::callbacks::Status; use connlib_shared::error::{ConnlibError as Error, ConnlibError}; -use connlib_shared::messages::client::{GatewayGroup, SiteId}; +use connlib_shared::messages::client::{Site, SiteId}; use connlib_shared::messages::{ client::ResourceDescription, client::ResourceDescriptionCidr, client::ResourceDescriptionDns, Answer, ClientPayload, DnsServer, DomainResponse, GatewayId, Interface as InterfaceConfig, @@ -176,8 +176,8 @@ where self.role_state.on_connection_failed(id); } - pub fn offline_resource(&mut self, id: ResourceId) { - self.role_state.offline_resource(id); + pub fn set_resource_offline(&mut self, id: ResourceId) { + self.role_state.set_resource_offline(id); self.callbacks .on_update_resources(self.role_state.resources()); @@ -202,10 +202,10 @@ where relays: Vec, site_id: SiteId, ) -> connlib_shared::Result { - self.role_state.gateways_site.insert(gateway_id, site_id); self.role_state.create_or_reuse_connection( resource_id, gateway_id, + site_id, stun(&relays, |addr| self.io.sockets_ref().can_handle(addr)), turn(&relays), ) @@ -331,13 +331,13 @@ impl ClientState { .sorted() .cloned() .map(|r| { - let status = self.status(&r); - callbacks::ResourceDescription::with_status(r, status) + let status = self.resource_status(&r); + r.with_status(status) }) .collect_vec() } - fn status(&self, resource: &ResourceDescription) -> Status { + fn resource_status(&self, resource: &ResourceDescription) -> Status { if resource.gateway_groups().iter().any(|s| { self.sites_status .get(&s.id) @@ -357,12 +357,12 @@ impl ClientState { Status::Unknown } - fn offline_resource(&mut self, id: ResourceId) { + fn set_resource_offline(&mut self, id: ResourceId) { let Some(resource) = self.resource_ids.get(&id).cloned() else { return; }; - for GatewayGroup { id, .. } in resource.gateway_groups() { + for Site { id, .. } in resource.gateway_groups() { self.sites_status.insert(*id, Status::Offline); } } @@ -488,11 +488,14 @@ impl ClientState { &mut self, resource_id: ResourceId, gateway_id: GatewayId, + site_id: SiteId, allowed_stun_servers: HashSet, allowed_turn_servers: HashSet<(RelayId, RelaySocket, String, String, String)>, ) -> connlib_shared::Result { tracing::trace!("create_or_reuse_connection"); + self.gateways_site.insert(gateway_id, site_id); + let desc = self .resource_ids .get(&resource_id) @@ -800,6 +803,7 @@ impl ClientState { } pub fn cleanup_connected_gateway(&mut self, gateway_id: &GatewayId) { + self.update_site_status_by_gateway(&gateway_id, Status::Unknown); self.peers.remove(gateway_id); self.dns_resources_internal_ips.retain(|resource, _| { !self @@ -888,7 +892,6 @@ impl ClientState { match event { snownet::Event::ConnectionFailed(id) => { self.cleanup_connected_gateway(&id); - self.update_site_status_by_gateway(&id, Status::Unknown) } snownet::Event::NewIceCandidate { connection, @@ -1575,7 +1578,7 @@ mod proptests { id: resource.id, name: resource.name, address_description: resource.address_description, - gateway_groups: resource.gateway_groups, + gateway_groups: resource.sites, }; client_state.add_resources(&[ResourceDescription::Cidr(dns_as_cidr_resource.clone())]); @@ -1668,11 +1671,11 @@ mod proptests { fn setting_gateway_online_sets_all_related_resources_online( #[strategy(resources_sharing_group())] resource_config_online: ( Vec, - GatewayGroup, + Site, ), #[strategy(resources_sharing_group())] resource_config_unknown: ( Vec, - GatewayGroup, + Site, ), #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, ) { @@ -1692,20 +1695,17 @@ mod proptests { client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Online); for resource in resources_online { - assert_eq!(client_state.status(&resource), Status::Online); + assert_eq!(client_state.resource_status(&resource), Status::Online); } for resource in resources_unknown { - assert_eq!(client_state.status(&resource), Status::Unknown); + assert_eq!(client_state.resource_status(&resource), Status::Unknown); } } #[test_strategy::proptest] fn disconnecting_gateway_sets_related_resources_unknown( - #[strategy(resources_sharing_group())] resource_config: ( - Vec, - GatewayGroup, - ), + #[strategy(resources_sharing_group())] resource_config: (Vec, Site), #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, ) { let (resources, gateway_group) = resource_config; @@ -1722,7 +1722,7 @@ mod proptests { client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Unknown); for resource in resources { - assert_eq!(client_state.status(&resource), Status::Unknown); + assert_eq!(client_state.resource_status(&resource), Status::Unknown); } } @@ -1730,7 +1730,7 @@ mod proptests { fn setting_resource_offline_doesnt_set_all_related_resources_offline( #[strategy(resources_sharing_group())] resource_config_online: ( Vec, - GatewayGroup, + Site, ), ) { let (mut resources, _) = resource_config_online; @@ -1738,11 +1738,14 @@ mod proptests { client_state.add_resources(&resources); let resource_offline = resources.pop().unwrap(); - client_state.offline_resource(resource_offline.id()); + client_state.set_resource_offline(resource_offline.id()); - assert_eq!(client_state.status(&resource_offline), Status::Offline); + assert_eq!( + client_state.resource_status(&resource_offline), + Status::Offline + ); for resource in resources { - assert_eq!(client_state.status(&resource), Status::Unknown); + assert_eq!(client_state.resource_status(&resource), Status::Unknown); } } @@ -1753,10 +1756,10 @@ mod proptests { let mut client_state = ClientState::for_test(); client_state.add_resources(&resources); - client_state.offline_resource(resources.first().unwrap().id()); + client_state.set_resource_offline(resources.first().unwrap().id()); for resource in resources { - assert_eq!(client_state.status(&resource), Status::Offline); + assert_eq!(client_state.resource_status(&resource), Status::Offline); } } } From 0c913bb45bbad1bc3a37ead462c3ca34fe0f8291 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 18:04:27 -0300 Subject: [PATCH 14/19] rename gateway_group to site --- rust/connlib/clients/shared/src/messages.rs | 4 +-- rust/connlib/shared/src/callbacks.rs | 8 ++--- rust/connlib/shared/src/messages/client.rs | 11 +++---- rust/connlib/shared/src/proptest.rs | 33 +++++++++------------ rust/connlib/tunnel/src/client.rs | 23 +++++++------- 5 files changed, 37 insertions(+), 42 deletions(-) diff --git a/rust/connlib/clients/shared/src/messages.rs b/rust/connlib/clients/shared/src/messages.rs index 58615d09d1f..c6fe3a102a7 100644 --- a/rust/connlib/clients/shared/src/messages.rs +++ b/rust/connlib/clients/shared/src/messages.rs @@ -236,7 +236,7 @@ mod test { address: "172.172.0.0/16".parse().unwrap(), name: "172.172.0.0/16".to_string(), address_description: "cidr resource".to_string(), - gateway_groups: vec![Site { + sites: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], @@ -309,7 +309,7 @@ mod test { address: "172.172.0.0/16".parse().unwrap(), name: "172.172.0.0/16".to_string(), address_description: "cidr resource".to_string(), - gateway_groups: vec![Site { + sites: vec![Site { name: "test".to_string(), id: "bf56f32d-7b2c-4f5d-a784-788977d014a4".parse().unwrap(), }], diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index 5d12e0913dc..19a493b3482 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -112,7 +112,7 @@ pub struct ResourceDescriptionDns { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + pub sites: Vec, pub status: Status, } @@ -124,7 +124,7 @@ impl From for crate::messages::client::ResourceDescripti address: r.address, address_description: r.address_description, name: r.name, - sites: r.gateway_groups, + sites: r.sites, } } } @@ -142,7 +142,7 @@ pub struct ResourceDescriptionCidr { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + pub sites: Vec, pub status: Status, } @@ -154,7 +154,7 @@ impl From for crate::messages::client::ResourceDescript address: r.address, address_description: r.address_description, name: r.name, - gateway_groups: r.gateway_groups, + sites: r.sites, } } } diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 3b5af7cc340..0b3f4139ed6 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -34,7 +34,7 @@ impl ResourceDescriptionDns { address: self.address, name: self.name, address_description: self.address_description, - gateway_groups: self.sites, + sites: self.sites, status, } } @@ -53,7 +53,8 @@ pub struct ResourceDescriptionCidr { pub name: String, pub address_description: String, - pub gateway_groups: Vec, + #[serde(rename = "gateway_groups")] + pub sites: Vec, } impl ResourceDescriptionCidr { @@ -63,7 +64,7 @@ impl ResourceDescriptionCidr { address: self.address, name: self.name, address_description: self.address_description, - gateway_groups: self.gateway_groups, + sites: self.sites, status, } } @@ -108,10 +109,10 @@ impl ResourceDescription { } } - pub fn gateway_groups(&self) -> HashSet<&Site> { + pub fn sites(&self) -> HashSet<&Site> { match self { ResourceDescription::Dns(r) => HashSet::from_iter(r.sites.iter()), - ResourceDescription::Cidr(r) => HashSet::from_iter(r.gateway_groups.iter()), + ResourceDescription::Cidr(r) => HashSet::from_iter(r.sites.iter()), } } diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 50e9907edf7..97d79da8896 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -14,7 +14,7 @@ use std::net::{Ipv4Addr, Ipv6Addr}; // Generate resources sharing 1 gateway group pub fn resources_sharing_group() -> impl Strategy, Site)> { - (collection::vec(gateway_groups(), 1..=100), gateway_group()).prop_flat_map(|(groups, g)| { + (collection::vec(sites(), 1..=100), site()).prop_flat_map(|(groups, g)| { ( groups .iter() @@ -31,27 +31,24 @@ pub fn resources_sharing_group() -> impl Strategy impl Strategy> { - gateway_groups() - .prop_flat_map(|gateway_groups| collection::vec(resource(gateway_groups), 1..=100)) + sites().prop_flat_map(|sites| collection::vec(resource(sites), 1..=100)) } -pub fn resource(gateway_groups: Vec) -> impl Strategy { +pub fn resource(sites: Vec) -> impl Strategy { any::().prop_flat_map(move |is_dns| { if is_dns { - dns_resource_with_groups(gateway_groups.clone()) + dns_resource_with_groups(sites.clone()) .prop_map(ResourceDescription::Dns) .boxed() } else { - cidr_resource_with_groups(8, gateway_groups.clone()) + cidr_resource_with_groups(8, sites.clone()) .prop_map(ResourceDescription::Cidr) .boxed() } }) } -pub fn dns_resource_with_groups( - gateway_groups: Vec, -) -> impl Strategy { +pub fn dns_resource_with_groups(sites: Vec) -> impl Strategy { ( resource_id(), resource_name(), @@ -63,7 +60,7 @@ pub fn dns_resource_with_groups( id, address, name, - sites: gateway_groups.clone(), + sites: sites.clone(), address_description, }, ) @@ -71,7 +68,7 @@ pub fn dns_resource_with_groups( pub fn cidr_resource_with_groups( host_mask_bits: usize, - gateway_groups: Vec, + sites: Vec, ) -> impl Strategy { ( resource_id(), @@ -84,31 +81,29 @@ pub fn cidr_resource_with_groups( id, address, name, - gateway_groups: gateway_groups.clone(), + sites: sites.clone(), address_description, }, ) } pub fn dns_resource() -> impl Strategy { - gateway_groups().prop_flat_map(dns_resource_with_groups) + sites().prop_flat_map(dns_resource_with_groups) } pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { - gateway_groups().prop_flat_map(move |gateway_groups| { - cidr_resource_with_groups(host_mask_bits, gateway_groups) - }) + sites().prop_flat_map(move |sites| cidr_resource_with_groups(host_mask_bits, sites)) } pub fn address_description() -> impl Strategy { any_with::("[a-z]{4,10}".into()) } -pub fn gateway_groups() -> impl Strategy> { - collection::vec(gateway_group(), 1..=10) +pub fn sites() -> impl Strategy> { + collection::vec(site(), 1..=10) } -pub fn gateway_group() -> impl Strategy { +pub fn site() -> impl Strategy { (any_with::("[a-z]{4,10}".into()), any::()).prop_map(|(name, id)| Site { name, id: SiteId::from_u128(id), diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 2adc8f1378d..529e7f194b3 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -338,7 +338,7 @@ impl ClientState { } fn resource_status(&self, resource: &ResourceDescription) -> Status { - if resource.gateway_groups().iter().any(|s| { + if resource.sites().iter().any(|s| { self.sites_status .get(&s.id) .is_some_and(|s| *s == Status::Online) @@ -346,7 +346,7 @@ impl ClientState { return Status::Online; } - if resource.gateway_groups().iter().all(|s| { + if resource.sites().iter().all(|s| { self.sites_status .get(&s.id) .is_some_and(|s| *s == Status::Offline) @@ -362,7 +362,7 @@ impl ClientState { return; }; - for Site { id, .. } in resource.gateway_groups() { + for Site { id, .. } in resource.sites() { self.sites_status.insert(*id, Status::Offline); } } @@ -922,10 +922,9 @@ impl ClientState { // Note: we can do this because in theory we shouldn't have multiple gateways for the same site // connected at the same time. self.sites_status.insert( - *self - .gateways_site - .get(gateway_id) - .expect("inconsistent state"), + *self.gateways_site.get(gateway_id).expect( + "if we're updating a site status there should be an associated site to a gateway", + ), status, ); } @@ -1578,7 +1577,7 @@ mod proptests { id: resource.id, name: resource.name, address_description: resource.address_description, - gateway_groups: resource.sites, + sites: resource.sites, }; client_state.add_resources(&[ResourceDescription::Cidr(dns_as_cidr_resource.clone())]); @@ -1679,7 +1678,7 @@ mod proptests { ), #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, ) { - let (resources_online, gateway_group) = resource_config_online; + let (resources_online, site) = resource_config_online; let (resources_unknown, _) = resource_config_unknown; let mut client_state = ClientState::for_test(); client_state.add_resources(&resources_online); @@ -1690,7 +1689,7 @@ mod proptests { ); client_state .gateways_site - .insert(first_resource_gateway_id, gateway_group.id); + .insert(first_resource_gateway_id, site.id); client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Online); @@ -1708,7 +1707,7 @@ mod proptests { #[strategy(resources_sharing_group())] resource_config: (Vec, Site), #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, ) { - let (resources, gateway_group) = resource_config; + let (resources, site) = resource_config; let mut client_state = ClientState::for_test(); client_state.add_resources(&resources); client_state @@ -1716,7 +1715,7 @@ mod proptests { .insert(resources.first().unwrap().id(), first_resource_gateway_id); client_state .gateways_site - .insert(first_resource_gateway_id, gateway_group.id); + .insert(first_resource_gateway_id, site.id); client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Online); client_state.update_site_status_by_gateway(&first_resource_gateway_id, Status::Unknown); From 13dd296c46a44afdadd0a4f929257594eaf3ab9c Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 18:13:36 -0300 Subject: [PATCH 15/19] update names in proptests from groups to sites --- rust/connlib/shared/src/proptest.rs | 34 ++++++++++++++--------------- rust/connlib/tunnel/src/client.rs | 10 ++++----- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/rust/connlib/shared/src/proptest.rs b/rust/connlib/shared/src/proptest.rs index 97d79da8896..df6a6c9e3ab 100644 --- a/rust/connlib/shared/src/proptest.rs +++ b/rust/connlib/shared/src/proptest.rs @@ -12,43 +12,43 @@ use proptest::{ }; use std::net::{Ipv4Addr, Ipv6Addr}; -// Generate resources sharing 1 gateway group -pub fn resources_sharing_group() -> impl Strategy, Site)> { - (collection::vec(sites(), 1..=100), site()).prop_flat_map(|(groups, g)| { +// Generate resources sharing 1 site +pub fn resources_sharing_site() -> impl Strategy, Site)> { + (collection::vec(sites(), 1..=100), site()).prop_flat_map(|(sites, site)| { ( - groups + sites .iter() - .map(|gs| { - let mut groups = gs.clone(); - groups.push(g.clone()); - resource(groups.clone()) + .map(|sites| { + let mut sites = sites.clone(); + sites.push(site.clone()); + resource(sites.clone()) }) .collect_vec(), - Just(g), + Just(site), ) }) } -// Generate resources sharing all gateway groups -pub fn resources_sharing_all_groups() -> impl Strategy> { +// Generate resources sharing all sites +pub fn resources_sharing_all_sites() -> impl Strategy> { sites().prop_flat_map(|sites| collection::vec(resource(sites), 1..=100)) } pub fn resource(sites: Vec) -> impl Strategy { any::().prop_flat_map(move |is_dns| { if is_dns { - dns_resource_with_groups(sites.clone()) + dns_resource_with_sites(sites.clone()) .prop_map(ResourceDescription::Dns) .boxed() } else { - cidr_resource_with_groups(8, sites.clone()) + cidr_resource_with_sites(8, sites.clone()) .prop_map(ResourceDescription::Cidr) .boxed() } }) } -pub fn dns_resource_with_groups(sites: Vec) -> impl Strategy { +pub fn dns_resource_with_sites(sites: Vec) -> impl Strategy { ( resource_id(), resource_name(), @@ -66,7 +66,7 @@ pub fn dns_resource_with_groups(sites: Vec) -> impl Strategy, ) -> impl Strategy { @@ -88,11 +88,11 @@ pub fn cidr_resource_with_groups( } pub fn dns_resource() -> impl Strategy { - sites().prop_flat_map(dns_resource_with_groups) + sites().prop_flat_map(dns_resource_with_sites) } pub fn cidr_resource(host_mask_bits: usize) -> impl Strategy { - sites().prop_flat_map(move |sites| cidr_resource_with_groups(host_mask_bits, sites)) + sites().prop_flat_map(move |sites| cidr_resource_with_sites(host_mask_bits, sites)) } pub fn address_description() -> impl Strategy { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 529e7f194b3..62ca93a57e4 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1668,11 +1668,11 @@ mod proptests { #[test_strategy::proptest] fn setting_gateway_online_sets_all_related_resources_online( - #[strategy(resources_sharing_group())] resource_config_online: ( + #[strategy(resources_sharing_site())] resource_config_online: ( Vec, Site, ), - #[strategy(resources_sharing_group())] resource_config_unknown: ( + #[strategy(resources_sharing_site())] resource_config_unknown: ( Vec, Site, ), @@ -1704,7 +1704,7 @@ mod proptests { #[test_strategy::proptest] fn disconnecting_gateway_sets_related_resources_unknown( - #[strategy(resources_sharing_group())] resource_config: (Vec, Site), + #[strategy(resources_sharing_site())] resource_config: (Vec, Site), #[strategy(gateway_id())] first_resource_gateway_id: GatewayId, ) { let (resources, site) = resource_config; @@ -1727,7 +1727,7 @@ mod proptests { #[test_strategy::proptest] fn setting_resource_offline_doesnt_set_all_related_resources_offline( - #[strategy(resources_sharing_group())] resource_config_online: ( + #[strategy(resources_sharing_site())] resource_config_online: ( Vec, Site, ), @@ -1750,7 +1750,7 @@ mod proptests { #[test_strategy::proptest] fn setting_resource_offline_set_all_resources_sharing_all_groups_offline( - #[strategy(resources_sharing_all_groups())] resources: Vec, + #[strategy(resources_sharing_all_sites())] resources: Vec, ) { let mut client_state = ClientState::for_test(); client_state.add_resources(&resources); From 1c0b7498ff4aed771ae72f6426b60ac51028c72d Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 18:19:46 -0300 Subject: [PATCH 16/19] fix log levels --- rust/headless-client/src/imp_linux.rs | 2 +- rust/headless-client/src/lib.rs | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/rust/headless-client/src/imp_linux.rs b/rust/headless-client/src/imp_linux.rs index 0b4f204a544..9fa6de8582e 100644 --- a/rust/headless-client/src/imp_linux.rs +++ b/rust/headless-client/src/imp_linux.rs @@ -241,7 +241,7 @@ impl Callbacks for CallbackHandlerIpc { } fn on_update_resources(&self, resources: Vec) { - tracing::info!(?resources, "New resource list"); + tracing::debug!(len = resources.len(), "New resource list"); self.cb_tx .try_send(IpcServerMsg::OnUpdateResources(resources)) .expect("Should be able to send OnUpdateResources"); diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index 8e12a34553c..48515acb8f5 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -278,12 +278,6 @@ impl Callbacks for CallbackHandler { resources: Vec, ) { // See easily with `export RUST_LOG=firezone_headless_client=debug` - - let status = resources - .iter() - .map(|r| (r.name(), r.status())) - .collect::>(); - tracing::info!(?status, "Resource Status"); for resource in &resources { tracing::debug!(?resource); } From 31305a00ce10b82171133982239eb1ae9bc27507 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 18:20:34 -0300 Subject: [PATCH 17/19] remove unnecesary namespacing --- rust/headless-client/src/lib.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index 48515acb8f5..786f10c3988 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -273,10 +273,7 @@ impl Callbacks for CallbackHandler { .expect("should be able to tell the main thread that we disconnected"); } - fn on_update_resources( - &self, - resources: Vec, - ) { + fn on_update_resources(&self, resources: Vec) { // See easily with `export RUST_LOG=firezone_headless_client=debug` for resource in &resources { tracing::debug!(?resource); From eb3f439a87e841de15bb2db3e93a168f85aeaea7 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 18:22:45 -0300 Subject: [PATCH 18/19] remove no longer used imports --- rust/connlib/tunnel/src/client.rs | 2 +- rust/headless-client/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 62ca93a57e4..b764443b614 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -803,7 +803,7 @@ impl ClientState { } pub fn cleanup_connected_gateway(&mut self, gateway_id: &GatewayId) { - self.update_site_status_by_gateway(&gateway_id, Status::Unknown); + self.update_site_status_by_gateway(gateway_id, Status::Unknown); self.peers.remove(gateway_id); self.dns_resources_internal_ips.retain(|resource, _| { !self diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index 786f10c3988..2379d647a5c 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -11,10 +11,10 @@ use anyhow::{Context, Result}; use clap::Parser; use connlib_client_shared::{file_logger, keypair, Callbacks, LoginUrl, Session, Sockets}; -use connlib_shared::callbacks::{self, Status}; +use connlib_shared::callbacks; use firezone_cli_utils::setup_global_subscriber; use secrecy::SecretString; -use std::{collections::HashMap, future, net::IpAddr, path::PathBuf, task::Poll}; +use std::{future, net::IpAddr, path::PathBuf, task::Poll}; use tokio::sync::mpsc; use imp::default_token_path; From 5a3dd36960994ed096db5cf64e9e31bfac396111 Mon Sep 17 00:00:00 2001 From: conectado Date: Tue, 14 May 2024 20:47:53 -0300 Subject: [PATCH 19/19] move cleanup connection within resource_offline --- rust/connlib/clients/shared/src/eventloop.rs | 1 - rust/connlib/tunnel/src/client.rs | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index 1b8a42c2b05..09a40a40a72 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -325,7 +325,6 @@ where tracing::debug!(resource_id = %offline_resource, "Resource is offline"); self.tunnel.set_resource_offline(offline_resource); - self.tunnel.cleanup_connection(offline_resource); } ErrorReply::Disabled => { diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index b764443b614..2ae8eed67c6 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -179,6 +179,8 @@ where pub fn set_resource_offline(&mut self, id: ResourceId) { self.role_state.set_resource_offline(id); + self.role_state.on_connection_failed(id); + self.callbacks .on_update_resources(self.role_state.resources()); }