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.
This commit is contained in:
Thomas Eizinger
2025-01-08 19:26:40 +01:00
committed by GitHub
parent 78d92036b4
commit ed5285268d
11 changed files with 104 additions and 117 deletions

View File

@@ -118,22 +118,16 @@ class TunnelService : VpnService() {
addressIPv4: String,
addressIPv6: String,
dnsAddresses: String,
) {
// init tunnel config
tunnelDnsAddresses = moshi.adapter<MutableList<String>>().fromJson(dnsAddresses)!!
tunnelIpv4Address = addressIPv4
tunnelIpv6Address = addressIPv6
buildVpnService()
}
override fun onUpdateRoutes(
routes4JSON: String,
routes6JSON: String,
) {
// init tunnel config
tunnelDnsAddresses = moshi.adapter<MutableList<String>>().fromJson(dnsAddresses)!!
val routes4 = moshi.adapter<MutableList<Cidr>>().fromJson(routes4JSON)!!
val routes6 = moshi.adapter<MutableList<Cidr>>().fromJson(routes6JSON)!!
tunnelIpv4Address = addressIPv4
tunnelIpv6Address = addressIPv6
tunnelRoutes.clear()
tunnelRoutes.addAll(routes4)
tunnelRoutes.addAll(routes6)

View File

@@ -6,9 +6,6 @@ interface ConnlibCallback {
addressIPv4: String,
addressIPv6: String,
dnsAddresses: String,
)
fun onUpdateRoutes(
routes4JSON: String,
routes6JSON: String,
)

View File

@@ -170,6 +170,8 @@ impl Callbacks for CallbackHandler {
tunnel_address_v4: Ipv4Addr,
tunnel_address_v6: Ipv6Addr,
dns_addresses: Vec<IpAddr>,
route_list_4: Vec<Ipv4Network>,
route_list_6: Vec<Ipv6Network>,
) {
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<Ipv4Network>, route_list_6: Vec<Ipv6Network>) {
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<ResourceView>) {

View File

@@ -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<IpAddr>,
route_list_v4: Vec<Ipv4Network>,
route_list_v6: Vec<Ipv6Network>,
) {
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<Ipv4Network>, route_list_6: Vec<Ipv6Network>) {
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");
}
}
}

View File

@@ -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<IpAddr>) {}
/// Called when the route list changes.
fn on_update_routes(&self, _: Vec<Ipv4Network>, _: Vec<Ipv6Network>) {}
fn on_set_interface_config(
&self,
_: Ipv4Addr,
_: Ipv6Addr,
_: Vec<IpAddr>,
_: Vec<Ipv4Network>,
_: Vec<Ipv6Network>,
) {
}
/// Called when the resource list changes.
///

View File

@@ -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),
);

View File

@@ -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(())
}

View File

@@ -85,12 +85,10 @@ pub enum ConnlibMsg {
ipv4: Ipv4Addr,
ipv6: Ipv6Addr,
dns: Vec<IpAddr>,
ipv4_routes: Vec<Ipv4Network>,
ipv6_routes: Vec<Ipv6Network>,
},
OnUpdateResources(Vec<ResourceView>),
OnUpdateRoutes {
ipv4: Vec<Ipv4Network>,
ipv6: Vec<Ipv6Network>,
},
}
#[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<IpAddr>) {
fn on_set_interface_config(
&self,
ipv4: Ipv4Addr,
ipv6: Ipv6Addr,
dns: Vec<IpAddr>,
ipv4_routes: Vec<Ipv4Network>,
ipv6_routes: Vec<Ipv6Network>,
) {
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<Ipv4Network>, ipv6: Vec<Ipv6Network>) {
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::<ConnlibMsg>(), 56)
assert_eq!(std::mem::size_of::<ConnlibMsg>(), 96)
}
}

View File

@@ -298,9 +298,18 @@ fn main() -> Result<()> {
// On every Resources update, flush DNS to mitigate <https://github.com/firezone/firezone/issues/5052>
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
// <https://github.com/firezone/firezone/pull/6026#discussion_r1692297438>
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?;
}
}
};

View File

@@ -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

View File

@@ -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())