From ed5285268dc17dc5209f539c834e937ef831a1ff Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 8 Jan 2025 19:26:40 +0100 Subject: [PATCH] refactor: merge `on_update_routes` and `on_set_interface_config` (#7699) For a while now, `connlib` has been calling these two callbacks right after each other because the internal event already bundles all the information about the TUN device. With this PR, we merge the two callback functions also in layers above `connlib` itself. Resolves: #6182. --- .../firezone/android/tunnel/TunnelService.kt | 14 +++----- .../tunnel/callback/ConnlibCallback.kt | 3 -- rust/connlib/clients/android/src/lib.rs | 36 +++++++------------ rust/connlib/clients/apple/src/lib.rs | 35 +++++++++--------- rust/connlib/clients/shared/src/callbacks.rs | 13 ++++--- rust/connlib/clients/shared/src/eventloop.rs | 7 ++-- rust/headless-client/src/ipc_service.rs | 15 +++++--- rust/headless-client/src/lib.rs | 31 +++++++++------- rust/headless-client/src/main.rs | 14 +++++--- .../FirezoneNetworkExtension/Adapter.swift | 33 +++++------------ .../CallbackHandler.swift | 20 ++++++----- 11 files changed, 104 insertions(+), 117 deletions(-) diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt index b463b73be..8e5555770 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt @@ -118,22 +118,16 @@ class TunnelService : VpnService() { addressIPv4: String, addressIPv6: String, dnsAddresses: String, - ) { - // init tunnel config - tunnelDnsAddresses = moshi.adapter>().fromJson(dnsAddresses)!! - tunnelIpv4Address = addressIPv4 - tunnelIpv6Address = addressIPv6 - - buildVpnService() - } - - override fun onUpdateRoutes( routes4JSON: String, routes6JSON: String, ) { + // init tunnel config + tunnelDnsAddresses = moshi.adapter>().fromJson(dnsAddresses)!! val routes4 = moshi.adapter>().fromJson(routes4JSON)!! val routes6 = moshi.adapter>().fromJson(routes6JSON)!! + tunnelIpv4Address = addressIPv4 + tunnelIpv6Address = addressIPv6 tunnelRoutes.clear() tunnelRoutes.addAll(routes4) tunnelRoutes.addAll(routes6) diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/callback/ConnlibCallback.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/callback/ConnlibCallback.kt index eaca6bcf8..1a3be84f7 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/callback/ConnlibCallback.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/callback/ConnlibCallback.kt @@ -6,9 +6,6 @@ interface ConnlibCallback { addressIPv4: String, addressIPv6: String, dnsAddresses: String, - ) - - fun onUpdateRoutes( routes4JSON: String, routes6JSON: String, ) diff --git a/rust/connlib/clients/android/src/lib.rs b/rust/connlib/clients/android/src/lib.rs index 67d749c1f..1c2a7a370 100644 --- a/rust/connlib/clients/android/src/lib.rs +++ b/rust/connlib/clients/android/src/lib.rs @@ -170,6 +170,8 @@ impl Callbacks for CallbackHandler { tunnel_address_v4: Ipv4Addr, tunnel_address_v6: Ipv6Addr, dns_addresses: Vec, + route_list_4: Vec, + route_list_6: Vec, ) { self.env(|mut env| { let tunnel_address_v4 = @@ -190,26 +192,6 @@ impl Callbacks for CallbackHandler { name: "dns_addresses", source, })?; - let name = "onSetInterfaceConfig"; - env.call_method( - &self.callback_handler, - name, - "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V", - &[ - JValue::from(&tunnel_address_v4), - JValue::from(&tunnel_address_v6), - JValue::from(&dns_addresses), - ], - ) - .map_err(|source| CallbackError::CallMethodFailed { name, source })?; - - Ok(()) - }) - .expect("onSetInterfaceConfig callback failed"); - } - - fn on_update_routes(&self, route_list_4: Vec, route_list_6: Vec) { - self.env(|mut env| { let route_list_4 = env .new_string(serde_json::to_string(&V4RouteList::new(route_list_4))?) .map_err(|source| CallbackError::NewStringFailed { @@ -223,18 +205,24 @@ impl Callbacks for CallbackHandler { source, })?; - let name = "onUpdateRoutes"; + let name = "onSetInterfaceConfig"; env.call_method( &self.callback_handler, name, - "(Ljava/lang/String;Ljava/lang/String;)V", - &[JValue::from(&route_list_4), JValue::from(&route_list_6)], + "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V", + &[ + JValue::from(&tunnel_address_v4), + JValue::from(&tunnel_address_v6), + JValue::from(&dns_addresses), + JValue::from(&route_list_4), + JValue::from(&route_list_6), + ], ) .map_err(|source| CallbackError::CallMethodFailed { name, source })?; Ok(()) }) - .expect("onUpdateRoutes callback failed"); + .expect("onSetInterfaceConfig callback failed"); } fn on_update_resources(&self, resource_list: Vec) { diff --git a/rust/connlib/clients/apple/src/lib.rs b/rust/connlib/clients/apple/src/lib.rs index 65963510e..9439e9899 100644 --- a/rust/connlib/clients/apple/src/lib.rs +++ b/rust/connlib/clients/apple/src/lib.rs @@ -89,11 +89,10 @@ mod ffi { tunnelAddressIPv4: String, tunnelAddressIPv6: String, dnsAddresses: String, + routeListv4: String, + routeListv6: String, ); - #[swift_bridge(swift_name = "onUpdateRoutes")] - fn on_update_routes(&self, routeList4: String, routeList6: String); - #[swift_bridge(swift_name = "onUpdateResources")] fn on_update_resources(&self, resourceList: String); @@ -129,6 +128,8 @@ impl Callbacks for CallbackHandler { tunnel_address_v4: Ipv4Addr, tunnel_address_v6: Ipv6Addr, dns_addresses: Vec, + route_list_v4: Vec, + route_list_v6: Vec, ) { let dns_addresses = match serde_json::to_string(&dns_addresses) { Ok(dns_addresses) => dns_addresses, @@ -137,24 +138,22 @@ impl Callbacks for CallbackHandler { return; } }; - - self.inner.on_set_interface_config( - tunnel_address_v4.to_string(), - tunnel_address_v6.to_string(), - dns_addresses, - ); - } - - fn on_update_routes(&self, route_list_4: Vec, route_list_6: Vec) { match ( - serde_json::to_string(&V4RouteList::new(route_list_4)), - serde_json::to_string(&V6RouteList::new(route_list_6)), + serde_json::to_string(&dns_addresses), + serde_json::to_string(&V4RouteList::new(route_list_v4)), + serde_json::to_string(&V6RouteList::new(route_list_v6)), ) { - (Ok(route_list_4), Ok(route_list_6)) => { - self.inner.on_update_routes(route_list_4, route_list_6); + (Ok(dns_addresses), Ok(route_list_4), Ok(route_list_6)) => { + self.inner.on_set_interface_config( + tunnel_address_v4.to_string(), + tunnel_address_v6.to_string(), + dns_addresses, + route_list_4, + route_list_6, + ); } - (Err(e), _) | (_, Err(e)) => { - tracing::error!(error = std_dyn_err(&e), "Failed to serialize route list"); + (Err(e), _, _) | (_, Err(e), _) | (_, _, Err(e)) => { + tracing::error!(error = std_dyn_err(&e), "Failed to serialize to JSON"); } } } diff --git a/rust/connlib/clients/shared/src/callbacks.rs b/rust/connlib/clients/shared/src/callbacks.rs index d2c47779b..b8f96a1fd 100644 --- a/rust/connlib/clients/shared/src/callbacks.rs +++ b/rust/connlib/clients/shared/src/callbacks.rs @@ -9,10 +9,15 @@ pub trait Callbacks: Clone + Send + Sync { /// The first time this is called, the Resources list is also ready, /// the routes are also ready, and the Client can consider the tunnel /// to be ready for incoming traffic. - fn on_set_interface_config(&self, _: Ipv4Addr, _: Ipv6Addr, _: Vec) {} - - /// Called when the route list changes. - fn on_update_routes(&self, _: Vec, _: Vec) {} + fn on_set_interface_config( + &self, + _: Ipv4Addr, + _: Ipv6Addr, + _: Vec, + _: Vec, + _: Vec, + ) { + } /// Called when the resource list changes. /// diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index 378c660fd..baf94ec21 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -168,9 +168,10 @@ where firezone_tunnel::ClientEvent::TunInterfaceUpdated(config) => { let dns_servers = config.dns_by_sentinel.left_values().copied().collect(); - self.callbacks - .on_set_interface_config(config.ip4, config.ip6, dns_servers); - self.callbacks.on_update_routes( + self.callbacks.on_set_interface_config( + config.ip4, + config.ip6, + dns_servers, Vec::from_iter(config.ipv4_routes), Vec::from_iter(config.ipv6_routes), ); diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index 82c7f8aa5..f07acdff3 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -456,12 +456,21 @@ impl<'a> Handler<'a> { .await .context("Error while sending IPC message `OnDisconnect`")? } - ConnlibMsg::OnSetInterfaceConfig { ipv4, ipv6, dns } => { + ConnlibMsg::OnSetInterfaceConfig { + ipv4, + ipv6, + dns, + ipv4_routes, + ipv6_routes, + } => { self.tun_device.set_ips(ipv4, ipv6).await?; self.dns_controller.set_dns(dns).await?; if let Some(instant) = self.last_connlib_start_instant.take() { tracing::info!(elapsed = ?instant.elapsed(), "Tunnel ready"); } + self.tun_device.set_routes(ipv4_routes, ipv6_routes).await?; + self.dns_controller.flush()?; + self.ipc_tx .send(&ServerMsg::TunnelReady) .await @@ -475,10 +484,6 @@ impl<'a> Handler<'a> { .await .context("Error while sending IPC message `OnUpdateResources`")?; } - ConnlibMsg::OnUpdateRoutes { ipv4, ipv6 } => { - self.tun_device.set_routes(ipv4, ipv6).await?; - self.dns_controller.flush()?; - } } Ok(()) } diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index 3b390d87d..07962cc04 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -85,12 +85,10 @@ pub enum ConnlibMsg { ipv4: Ipv4Addr, ipv6: Ipv6Addr, dns: Vec, + ipv4_routes: Vec, + ipv6_routes: Vec, }, OnUpdateResources(Vec), - OnUpdateRoutes { - ipv4: Vec, - ipv6: Vec, - }, } #[derive(Clone)] @@ -108,9 +106,22 @@ impl Callbacks for CallbackHandler { .expect("should be able to send OnDisconnect"); } - fn on_set_interface_config(&self, ipv4: Ipv4Addr, ipv6: Ipv6Addr, dns: Vec) { + fn on_set_interface_config( + &self, + ipv4: Ipv4Addr, + ipv6: Ipv6Addr, + dns: Vec, + ipv4_routes: Vec, + ipv6_routes: Vec, + ) { self.cb_tx - .try_send(ConnlibMsg::OnSetInterfaceConfig { ipv4, ipv6, dns }) + .try_send(ConnlibMsg::OnSetInterfaceConfig { + ipv4, + ipv6, + dns, + ipv4_routes, + ipv6_routes, + }) .expect("Should be able to send OnSetInterfaceConfig"); } @@ -120,12 +131,6 @@ impl Callbacks for CallbackHandler { .try_send(ConnlibMsg::OnUpdateResources(resources)) .expect("Should be able to send OnUpdateResources"); } - - fn on_update_routes(&self, ipv4: Vec, ipv6: Vec) { - self.cb_tx - .try_send(ConnlibMsg::OnUpdateRoutes { ipv4, ipv6 }) - .expect("Should be able to send messages"); - } } /// Sets up logging for stdout only, with INFO level by default @@ -148,6 +153,6 @@ mod tests { // Make sure it's okay to store a bunch of these to mitigate #5880 #[test] fn callback_msg_size() { - assert_eq!(std::mem::size_of::(), 56) + assert_eq!(std::mem::size_of::(), 96) } } diff --git a/rust/headless-client/src/main.rs b/rust/headless-client/src/main.rs index b0459516e..988747e84 100644 --- a/rust/headless-client/src/main.rs +++ b/rust/headless-client/src/main.rs @@ -298,9 +298,18 @@ fn main() -> Result<()> { // On every Resources update, flush DNS to mitigate dns_controller.flush()?; } - ConnlibMsg::OnSetInterfaceConfig { ipv4, ipv6, dns } => { + ConnlibMsg::OnSetInterfaceConfig { + ipv4, + ipv6, + dns, + ipv4_routes, + ipv6_routes, + } => { tun_device.set_ips(ipv4, ipv6).await?; + tun_device.set_routes(ipv4_routes, ipv6_routes).await?; + dns_controller.set_dns(dns).await?; + // `on_set_interface_config` is guaranteed to be called when the tunnel is completely ready // if let Some(instant) = last_connlib_start_instant.take() { @@ -313,9 +322,6 @@ fn main() -> Result<()> { break Ok(()); } } - ConnlibMsg::OnUpdateRoutes { ipv4, ipv6 } => { - tun_device.set_routes(ipv4, ipv6).await?; - } } }; diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index 3247a2c1f..4d0baa3d7 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -356,7 +356,7 @@ extension Adapter { extension Adapter: CallbackHandlerDelegate { public func onSetInterfaceConfig( - tunnelAddressIPv4: String, tunnelAddressIPv6: String, dnsAddresses: [String] + tunnelAddressIPv4: String, tunnelAddressIPv6: String, dnsAddresses: [String], routeListv4: String, routeListv6: String ) { // This is a queued callback to ensure ordering workQueue.async { [weak self] in @@ -368,7 +368,7 @@ extension Adapter: CallbackHandlerDelegate { } Log.log( - "\(#function): \(tunnelAddressIPv4) \(tunnelAddressIPv6) \(dnsAddresses)") + "\(#function): \(tunnelAddressIPv4) \(tunnelAddressIPv6) \(dnsAddresses) \(routeListv4) \(routeListv6)") switch state { case .tunnelStarted(session: _): @@ -376,6 +376,13 @@ extension Adapter: CallbackHandlerDelegate { networkSettings.tunnelAddressIPv4 = tunnelAddressIPv4 networkSettings.tunnelAddressIPv6 = tunnelAddressIPv6 networkSettings.dnsAddresses = dnsAddresses + networkSettings.routes4 = try! JSONDecoder().decode( + [NetworkSettings.Cidr].self, from: routeListv4.data(using: .utf8)! + ).compactMap { $0.asNEIPv4Route } + networkSettings.routes6 = try! JSONDecoder().decode( + [NetworkSettings.Cidr].self, from: routeListv6.data(using: .utf8)! + ).compactMap { $0.asNEIPv6Route } + networkSettings.apply() case .tunnelStopped: Log.error(AdapterError.invalidState(self.state)) @@ -383,28 +390,6 @@ extension Adapter: CallbackHandlerDelegate { } } - public func onUpdateRoutes(routeList4: String, routeList6: String) { - // This is a queued callback to ensure ordering - workQueue.async { [weak self] in - guard let self = self else { return } - guard let networkSettings = networkSettings - else { - fatalError("onUpdateRoutes called before network settings was initialized!") - } - - Log.log("\(#function): \(routeList4) \(routeList6)") - - networkSettings.routes4 = try! JSONDecoder().decode( - [NetworkSettings.Cidr].self, from: routeList4.data(using: .utf8)! - ).compactMap { $0.asNEIPv4Route } - networkSettings.routes6 = try! JSONDecoder().decode( - [NetworkSettings.Cidr].self, from: routeList6.data(using: .utf8)! - ).compactMap { $0.asNEIPv6Route } - - networkSettings.apply() - } - } - public func onUpdateResources(resourceList: String) { // This is a queued callback to ensure ordering workQueue.async { [weak self] in diff --git a/swift/apple/FirezoneNetworkExtension/CallbackHandler.swift b/swift/apple/FirezoneNetworkExtension/CallbackHandler.swift index dde0113a3..c3a8f91cc 100644 --- a/swift/apple/FirezoneNetworkExtension/CallbackHandler.swift +++ b/swift/apple/FirezoneNetworkExtension/CallbackHandler.swift @@ -20,9 +20,10 @@ public protocol CallbackHandlerDelegate: AnyObject { func onSetInterfaceConfig( tunnelAddressIPv4: String, tunnelAddressIPv6: String, - dnsAddresses: [String] + dnsAddresses: [String], + routeListv4: String, + routeListv6: String ) - func onUpdateRoutes(routeList4: String, routeList6: String) func onUpdateResources(resourceList: String) func onDisconnect(error: String) } @@ -33,7 +34,9 @@ public class CallbackHandler { func onSetInterfaceConfig( tunnelAddressIPv4: RustString, tunnelAddressIPv6: RustString, - dnsAddresses: RustString + dnsAddresses: RustString, + routeListv4: RustString, + routeListv6: RustString ) { Log.log( """ @@ -41,6 +44,8 @@ public class CallbackHandler { IPv4: \(tunnelAddressIPv4.toString()) IPv6: \(tunnelAddressIPv6.toString()) DNS: \(dnsAddresses.toString()) + IPv4 routes: \(routeListv4.toString()) + IPv6 routes: \(routeListv6.toString()) """) let dnsData = dnsAddresses.toString().data(using: .utf8)! @@ -49,15 +54,12 @@ public class CallbackHandler { delegate?.onSetInterfaceConfig( tunnelAddressIPv4: tunnelAddressIPv4.toString(), tunnelAddressIPv6: tunnelAddressIPv6.toString(), - dnsAddresses: dnsArray + dnsAddresses: dnsArray, + routeListv4: routeListv4.toString(), + routeListv6: routeListv6.toString() ) } - func onUpdateRoutes(routeList4: RustString, routeList6: RustString) { - Log.log("CallbackHandler.onUpdateRoutes: \(routeList4) \(routeList6)") - delegate?.onUpdateRoutes(routeList4: routeList4.toString(), routeList6: routeList6.toString()) - } - func onUpdateResources(resourceList: RustString) { Log.log("CallbackHandler.onUpdateResources: \(resourceList.toString())") delegate?.onUpdateResources(resourceList: resourceList.toString())