Fix broken retry strategy tests ios 938

[buggmagnet]

This PR does the following things:

• Amend the documentation about the algorithm governing the automatic port selection
  • In particular, it adds an iOS specific section as it's quite different from other platforms
• Changes how local IP discovery is done in UITests (it used to be based on the interface name, which is not a reliable method on iOS)
• Adds a UITest for Shadowsocks obfuscation
• Updates the automatic reconnection algorithm for iOS based on the official documentation

---

This change is 

[linear]

IOS-938 Fix broken retry strategy tests

[pinkisemils]

Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r2 (raw file):

        switch tunnelSettings.wireGuardObfuscation.udpOverTcpPort {
        case .automatic:
            return (connectionAttemptCount % 2 == 0) ? .only(80) : .only(5001)

Would this have ever worked? Or was the connection attempt count specific to each obfuscation method?

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

                ) == 0 {
                    ipAddress = String(cString: hostname)
                    if ipAddress.starts(with: "192.168") {

This is wrong.

[pinkisemils]

Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This is wrong.

We can never know which local subnet will be used in tests.

[MarkusPettersson98]

Reviewed all commit messages.
Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

docs/relay-selector.md line 77 at r2 (raw file):

- The UDP2TCP random port is **either** 80 **or** 5001
- The Shadowsocks port is random within a certain range of ports defined by the relays configuration

⛏️ relay's configuration* (?)

Code quote:

relays configuration

---

docs/relay-selector.md line 135 at r2 (raw file):

There are two type of obfuscators - _udp2tcp_, and _shadowsocks_.
They are used if the obfuscation mode is set to _On_ or _Auto_ and the user has selected WireGuard to be the only tunnel protocol to be used.

I think this is a bit outdated, the On option has not existed for a while. In this case, I think it is fine to just mention the Auto option 😊

Code quote:

There are two type of obfuscators - _udp2tcp_, and _shadowsocks_.
They are used if the obfuscation mode is set to _On_ or _Auto_ and the user has selected WireGuard to be the only tunnel protocol to be used.

[pinkisemils]

Reviewed 1 of 9 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 135 at r2 (raw file):

        if connectionAttempts.count == 4 { // Expected retries flow
            for (attemptIndex, attempt) in connectionAttempts.enumerated() {
                if attemptIndex == 0 || attemptIndex == 1 || attemptIndex == 2 {

Would attemptIndex < 3 not be more legible?

Or a set operation, like attemptIndex in { 0, 1, 2 }.

[buggmagnet]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @pinkisemils)

---

docs/relay-selector.md line 77 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ relay's configuration* (?)

The configuration for all shadowsocks ranges is not on a single relay, and is shared by all of them (AFAICT), which is why I've used the plural stance here.
I was tempted to link to the current API, but I thought that was a bit too far reaching.

---

docs/relay-selector.md line 135 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think this is a bit outdated, the On option has not existed for a while. In this case, I think it is fine to just mention the Auto option 😊

Indeed !

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Would this have ever worked? Or was the connection attempt count specific to each obfuscation method?

The connection count was specific to UDP Over TCP only.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We can never know which local subnet will be used in tests.

The previous version was equally wrong, it was assuming en0 was the default Wi-Fi interface.
This is not a correct assumption on iOS

And we completely control the subnet to use in tests, so I'm okay with baking that assumption in.
What would you suggest we do instead ?

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 135 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Would attemptIndex < 3 not be more legible?

Or a set operation, like attemptIndex in { 0, 1, 2 }.

Yes it would, I quickly wrote this to fix it locally and didn't come back to it, good catch !

[pinkisemils]

Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @MarkusPettersson98)

---

docs/relay-selector.md line 77 at r2 (raw file):

Previously, buggmagnet wrote…

The configuration for all shadowsocks ranges is not on a single relay, and is shared by all of them (AFAICT), which is why I've used the plural stance here.
I was tempted to link to the current API, but I thought that was a bit too far reaching.

There's 2 sets of ports, the ones that all relays support and, for relays with an shadowsocks_extra_addr_in, these addresses support all ports.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, buggmagnet wrote…

The previous version was equally wrong, it was assuming en0 was the default Wi-Fi interface.
This is not a correct assumption on iOS

And we completely control the subnet to use in tests, so I'm okay with baking that assumption in.
What would you suggest we do instead ?

Has this ever failed us? If I was to run my test suite at home, with these new changes, it'd be broken.

The better solution would be to specify the subnet in which the device is expected to run in the test config and then use that to deduce which is the correct IP address to use.

[faern]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)

---

docs/relay-selector.md line 77 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

There's 2 sets of ports, the ones that all relays support and, for relays with an shadowsocks_extra_addr_in, these addresses support all ports.

I think we should call it "relay list". Since that's what we refer to the content of this API endpoint as everywhere else(?). I interpret "relay configuration" as an internal config we have generated for talking to a relay. While the "relay list" is the response from the API, which do contain the valid port ranges.

[faern]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)

---

docs/relay-selector.md line 65 at r3 (raw file):

  - The eighth attempt will connect to an OpenVPN relay over a bridge on a random port

### Default constraints for tunnel endpoints on iOS

We'll see what happens in ~a month when we remove OpenVPN from the automatic retry order. But great that you want to update the docs so they are better for your team/platform. Let's do this.

[buggmagnet]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98 and @pinkisemils)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Has this ever failed us?

Literally the first device I took in the office didn't have an en0 interface, so yes, which led me to make this change.

If I was to run my test suite at home, with these new changes, it'd be broken.

The tests would likely fail anyway because they rely on the firewall API that is in the office.

The better solution would....

We should not try to deduct the correct IP address to use from the tests, it will be broken in subtle ways that we might not expect.
I thought about adding the IP address of the device running the tests in the xcconfig file but that is also error prone as you'd have to change it for every different device you use.

Realistically, 99% of networks will use an IP in the 192.168.x.x/16 range for local IP addresses, and this is the least path of resistance.

I made this change mostly with the automated tests running on the device attached to the local runner in the office.
But also this way, everyone can just run those tests without any issue, or needing to edit their configuration files.

[buggmagnet]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @faern, @MarkusPettersson98, and @pinkisemils)

---

docs/relay-selector.md line 77 at r2 (raw file):

The Shadowsocks port is random within a certain range of ports defined by the relay list

Does that sound acceptable ?

[MarkusPettersson98]

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @buggmagnet, @faern, and @pinkisemils)

---

docs/relay-selector.md line 77 at r2 (raw file):

Previously, buggmagnet wrote…

The Shadowsocks port is random within a certain range of ports defined by the relay list

Does that sound acceptable ?

Sounds good to me 👍

[pinkisemils]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

docs/relay-selector.md line 77 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Sounds good to me 👍

___ *[`ios/MullvadVPNUITests/Networking/Networking.swift` line 59 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/7213#-OCCgpKl4G1_MjWhFen4:-OCDqWAgBJ8Vw0XUjx9h:bsf5lnh) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/8cfa2dad08cb80734f5e45afd205f05ddb5d3181/ios/MullvadVPNUITests/Networking/Networking.swift#L59)):* > Realistically, 99% of networks will use an IP in the 192.168.x.x/16 range for local IP addresses, and this is the least path of resistance.

99% of networks will not use an IP in 192.168/16.

We should not try to deduct the correct IP address to use from the tests, it will be broken in subtle ways that we might not expect.
I thought about adding the IP address of the device running the tests in the xcconfig file but that is also error prone as you'd have to change it for every different device you use.

I am not talking about specifying the IP address of the device - I am talking about specifying the subnet which to match with.
You can never know what IP address will your mobile provider give to you, it could also be a 192.168/16 address, as per Eskimo's post.

Also, may I ask, which device did not have the correct interface on en0?

[rablador]

Reviewed 7 of 9 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[buggmagnet]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Realistically, 99% of networks will use an IP in the 192.168.x.x/16 range for local IP addresses, and this is the least path of resistance.

99% of networks will not use an IP in 192.168/16.

We should not try to deduct the correct IP address to use from the tests, it will be broken in subtle ways that we might not expect.
I thought about adding the IP address of the device running the tests in the xcconfig file but that is also error prone as you'd have to change it for every different device you use.

I am not talking about specifying the IP address of the device - I am talking about specifying the subnet which to match with.
You can never know what IP address will your mobile provider give to you, it could also be a 192.168/16 address, as per Eskimo's post.

Also, may I ask, which device did not have the correct interface on en0?

We discussed this offline with the conclusion that we will add an API on the endpoint where the firewall resides
that will tell us which IP address the phone has, so we don't have to do local discovery of our own IP address.

[rablador]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

[pinkisemils]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, buggmagnet wrote…

We discussed this offline with the conclusion that we will add an API on the endpoint where the firewall resides
that will tell us which IP address the phone has, so we don't have to do local discovery of our own IP address.

The router API will now return the IP address as a string (be it IPv4 or IPv6) on route /own-ip.

[rablador]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[rablador]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[rablador]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[buggmagnet]

Dismissed @pinkisemils from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

---

ios/MullvadVPNUITests/Networking/Networking.swift line 59 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The router API will now return the IP address as a string (be it IPv4 or IPv6) on route /own-ip.

Done.

[rablador]

Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[rablador]

Reviewable status: complete! all files reviewed, all discussions resolved

Dismissed due to not working this week. The feedback has been addressed.

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.