From 9987dbc59270234252f84f768f18ff38338f222c Mon Sep 17 00:00:00 2001 From: kari-ts <135075563+kari-ts@users.noreply.github.com> Date: Tue, 17 Sep 2024 10:26:20 -0700 Subject: [PATCH] android: only update DNS configs on LinkProperties changes (#511) We were updating DNS configs when capabilities changed, without LinkProperties having been filled in. Because onAvailable always happened first, LinkProperties were created with default value, and onCapabilitiesChanged sent a DNS update using those LinkProperties. This change only updates DNS configs on LinkProperties, which is the last update sent on a network change. Updates tailscale/tailscale#13173 Signed-off-by: kari-ts --- .../tailscale/ipn/NetworkChangeCallback.kt | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt b/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt index c3d57b6..4dae7c1 100644 --- a/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt +++ b/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt @@ -9,7 +9,6 @@ import android.net.NetworkCapabilities import android.net.NetworkRequest import android.util.Log import libtailscale.Libtailscale -import java.net.InetAddress import java.util.concurrent.locks.ReentrantLock import kotlin.concurrent.withLock @@ -17,12 +16,10 @@ object NetworkChangeCallback { private const val TAG = "NetworkChangeCallback" - private data class NetworkInfo( - var caps: NetworkCapabilities, - var linkProps: LinkProperties - ) + private data class NetworkInfo(var caps: NetworkCapabilities, var linkProps: LinkProperties) private val lock = ReentrantLock() + private val activeNetworks = mutableMapOf() // keyed by Network // monitorDnsChanges sets up a network callback to monitor changes to the @@ -30,13 +27,15 @@ object NetworkChangeCallback { // become available or properties of those interfaces change. fun monitorDnsChanges(connectivityManager: ConnectivityManager, dns: DnsConfig) { val networkConnectivityRequest = - NetworkRequest.Builder() - .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) - .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) - .build() + NetworkRequest.Builder() + .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) + .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) + .build() // Use registerNetworkCallback to listen for updates from all networks, and - // then update DNS configs for the best network. + // then update DNS configs for the best network when LinkProperties are changed. + // Per + // https://developer.android.com/reference/android/net/ConnectivityManager.NetworkCallback#onAvailable(android.net.Network), this happens after all other updates. // // Note that we can't use registerDefaultNetworkCallback because the // default network used by Tailscale will always show up with capability @@ -51,23 +50,19 @@ object NetworkChangeCallback { Log.d(TAG, "onAvailable: network ${network}") lock.withLock { activeNetworks[network] = NetworkInfo(NetworkCapabilities(), LinkProperties()) - maybeUpdateDNSConfigLocked("onAvailable", dns) } } override fun onCapabilitiesChanged(network: Network, capabilities: NetworkCapabilities) { super.onCapabilitiesChanged(network, capabilities) - lock.withLock { - activeNetworks[network]?.caps = capabilities - maybeUpdateDNSConfigLocked("onCapabilitiesChanged", dns) - } + lock.withLock { activeNetworks[network]?.caps = capabilities } } override fun onLinkPropertiesChanged(network: Network, linkProperties: LinkProperties) { super.onLinkPropertiesChanged(network, linkProperties) lock.withLock { activeNetworks[network]?.linkProps = linkProperties - maybeUpdateDNSConfigLocked("onLinkPropertiesChanged", dns) + maybeUpdateDNSConfig("onLinkPropertiesChanged", dns) } } @@ -77,7 +72,7 @@ object NetworkChangeCallback { Log.d(TAG, "onLost: network ${network}") lock.withLock { activeNetworks.remove(network) - maybeUpdateDNSConfigLocked("onLost", dns) + maybeUpdateDNSConfig("onLost", dns) } } }) @@ -101,11 +96,12 @@ object NetworkChangeCallback { // Filter the list of all networks to those that have the INTERNET // capability, are not VPNs, and have a non-zero number of DNS servers // available. - val networks = activeNetworks.filter { (_, info) -> - info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) && - info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) && - info.linkProps.dnsServers.isNotEmpty() == true - } + val networks = + activeNetworks.filter { (_, info) -> + info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) && + info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) && + info.linkProps.dnsServers.isNotEmpty() == true + } // If we have one; just return it; otherwise, prefer networks that are also // not metered (i.e. cell modems). @@ -122,7 +118,9 @@ object NetworkChangeCallback { for ((network, info) in activeNetworks) { if (info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) && info.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN)) { - Log.w(TAG, "no networks available that also have DNS servers set; falling back to first network ${network}") + Log.w( + TAG, + "no networks available that also have DNS servers set; falling back to first network ${network}") return network } } @@ -136,9 +134,7 @@ object NetworkChangeCallback { // maybeUpdateDNSConfig will maybe update our DNS configuration based on the // current set of active Networks. - // - // 'lock' must be held. - private fun maybeUpdateDNSConfigLocked(why: String, dns: DnsConfig) { + private fun maybeUpdateDNSConfig(why: String, dns: DnsConfig) { val defaultNetwork = pickDefaultNetwork() if (defaultNetwork == null) { Log.d(TAG, "${why}: no default network available; not updating DNS config") @@ -146,7 +142,9 @@ object NetworkChangeCallback { } val info = activeNetworks[defaultNetwork] if (info == null) { - Log.w(TAG, "${why}: [unexpected] no info available for default network; not updating DNS config") + Log.w( + TAG, + "${why}: [unexpected] no info available for default network; not updating DNS config") return } @@ -160,7 +158,9 @@ object NetworkChangeCallback { sb.append(searchDomains) } if (dns.updateDNSFromNetwork(sb.toString())) { - Log.d(TAG, "${why}: updated DNS config for network ${defaultNetwork} (${info.linkProps.interfaceName})") + Log.d( + TAG, + "${why}: updated DNS config for network ${defaultNetwork} (${info.linkProps.interfaceName})") Libtailscale.onDNSConfigChanged(info.linkProps.interfaceName) } }