Conversation
|
🚨 End to end tests failed. Please check the failed workflow run. |
|
To test the changes if your home/office network does not support IPv6, you can use a mobile connection - most mobile network operators have a working IPv6 stack. |
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 62 files and all commit messages, and made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pinkisemils).
ios/MullvadMockData/MullvadREST/SelectedRelaysStub+Stubs.swift line 2 at r1 (raw file):
// // SelectedRelaysStub+Stubs.swift
Not a fault of this PR, but why do we have this data duplicated in its own file. Can we remove this and use what's in the RelaySelectorStub.swift file instead?
ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 52 at r1 (raw file):
public func makeConfiguration() throws -> TunnelAdapterConfiguration { let config = TunnelAdapterConfiguration(
Nit: Unnecessary change.
ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 104 at r1 (raw file):
private func resolveObfuscationMethod(features: REST.ServerRelay.Features?) -> ObfuscationMethod { switch obfuscation.method { case .off, .automatic:
If I remember correctly we'll never have .automatic here.
ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 153 at r1 (raw file):
return match.relay.features?.quic?.addrIn .compactMap({ IPv6Address($0) }) .randomElement() ?? defaultIpv6Address
Will defaultIpv6Address work here if it's not an extra address?
ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 173 at r1 (raw file):
} private func applyShadowsocksIpv6Address(in match: RelaySelectorMatch) -> IPv6Address? {
Shouldn't this follow the same logic as applyShadowsocksIpAddress? (or perhaps the other way around)
ios/MullvadVPNTests/MullvadREST/Relay/RelayObfuscatorTests.swift line 563 at r1 (raw file):
func testQuicWithIPv6ThrowsErrorWhenNoIPv6RelaysAvailable() throws { // Create relays with only IPv4 QUIC addresses let ipv4OnlyQuicRelay = REST.ServerRelay(
ipv4OnlyRelay above could be used instead, unless you don't want to associate that relay with needing quic.
ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 53 at r1 (raw file):
) async throws { if exitConfiguration.peer?.endpoint.ip is IPv6Address, entryConfiguration != nil {
Is this something we want to check here or should we do it earlier? When we're at this point, shouldn't we be all set to just start things?
ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 188 at r1 (raw file):
tunnelMonitor.handleNetworkPathUpdate(networkPath) let newReachability = networkPath.networkReachability
Is this from your other PR?
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 3 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @rablador).
ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 173 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Shouldn't this follow the same logic as
applyShadowsocksIpAddress? (or perhaps the other way around)
It should, I was figuring out if IPv6 worked properly with Shadowsocks or not - will adjust this later.
ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 53 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Is this something we want to check here or should we do it earlier? When we're at this point, shouldn't we be all set to just start things?
This is a backstop - using IPv6 for reaching the exit in multihop configurations will just end in a crash otherwise. This will go away with GotaTun.
ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 188 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Is this from your other PR?
Yes, will remove.
rablador
left a comment
There was a problem hiding this comment.
@rablador resolved 1 discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pinkisemils).
This PR introduces many things, a migration, a new settings section, and most importantly, working IPv6 connections to our relays. There's also a small change to the relay selector to not return a whole
MullvadEndpointand instead return a more focused type such that the correct IP is selected in the relay selector.This is just a draft PR, I want to see if I'm doing terribly bad things here. I'll split the PR up into smaller chunks for final reviewing soon.
I believe even the setting will change - it should be Device IP version, unless we're going to be different from the other platforms.
This change is