VPN-6408 and VPN-6678: Turn daemon server switches into round robin of servers, and fix DNS bug #9980
Conversation
…ust-daemon-switches
mcleinman
requested review from
strseb,
lesleyjanenorton and
oskirby
and removed request for
lesleyjanenorton
|
With #9972 merged into |
| @@ -588,6 +589,16 @@ class VPNService : android.net.VpnService() { | |||
| return confBuilder.build() | |||
| } | |||
|
|
|||
| private fun getServerConfig(obj: JSONObject, useFallbackServer: Boolean): JSONObject { | |||
There was a problem hiding this comment.
Just a minor nitpick, do we really need the useFallbackServer parameter anymore? That seems like it should be covered by the state of the currentServerConfig variable.
Also, this function is not idempotent, it actually does the rotation between servers, so a better name might be getNextServerConfig() to imply that it's iterating.
There was a problem hiding this comment.
Updated to getNextServerConfig
useFallbackServer now just controls whether we use the first server in the config (when false) or the next server in line (when true). I think using the client-selected main server on initial connection seems helpful - and we thus would need to keep useFallBackServer in order to reset currentServerConfig. (If the daemon is maintained between VPN activations - I'm not sure if it is - we'd otherwise never reset currentServerConfig back to 0, and potentially take a non-first server on initial connection to a new geo, for instance.)
| @@ -295,12 +295,18 @@ void ServerData::setExitServerPublicKey(const QString& publicKey) { | |||
| m_exitServerPublicKey = publicKey; | |||
| } | |||
|
|
|||
| const Server ServerData::backupServer(const QString& currentPublicKey) const { | |||
There was a problem hiding this comment.
Ideally, the backup servers should be selected according to the weighting as well.... but that is hard to do so I accept that this should be fine for fallback and recovery purposes. (See the mess of an algorithm in Server::weightChooser() for how it's done for the primary server) but a comment would be nice for our future selves.
There was a problem hiding this comment.
Just philosophizing: It would be a cleaner approach if we combined the concepts of primary and backup servers and instead we just provided a list of servers down to the controller to handle. This would require changing the controllers APIs a bunch though, and that's also scary.
There was a problem hiding this comment.
Philosophizing - I almost went that way, but this idea of additional backups isn't relevant to desktop. (We pass just the main server to the platform-specific controller, and then the mobile controller requests additional backup servers. And for desktop controllers, they get the main server and that's all they ever need - any server switches will come from the client.)
| @@ -161,27 +161,30 @@ public class IOSControllerImpl: NSObject { | |||
| } | |||
| } | |||
|
|
|||
| @objc func connect(serverData: VPNServerData, fallbackServer: VPNServerData?, excludeLocalNetworks: Bool, allowedIPAddressRanges: [VPNIPAddressRange], reason: Int, gleanDebugTag: String, isSuperDooperFeatureActive: Bool, installationId: String, disconnectOnErrorCallback: @escaping () -> Void, onboardingCompletedCallback: @escaping () -> Void, vpnConfigPermissionResponseCallback: @escaping (Bool) -> Void) { | |||
| @objc func connect(serverData: [VPNServerData], excludeLocalNetworks: Bool, allowedIPAddressRanges: [VPNIPAddressRange], reason: Int, gleanDebugTag: String, isSuperDooperFeatureActive: Bool, installationId: String, disconnectOnErrorCallback: @escaping () -> Void, onboardingCompletedCallback: @escaping () -> Void, vpnConfigPermissionResponseCallback: @escaping (Bool) -> Void) { | |||
There was a problem hiding this comment.
nit: Does all of this really have to be on one line? It's kind of hard to read.
There was a problem hiding this comment.
It doesn't have to be. I had just learned to live with it, but you're right, it's ugly and unclear. I've updated this to a few lines (both here and in configureTunnel) - thanks for saying something.
Description
There are a few things in this PR, apologies. If you'd like them broken out into separate PRs, please let me know and I'll happily restructure.
Reference
VPN-6408 and VPN-6678
Checklist