Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow relay selector to select an entry peer #6413

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Jun 26, 2024

The relay selector should be able to select a WireGuard relay for an entry peer. It should make precautions to not select the same relay for both entry and exit peers.

When either the entry constraints or the exit constraints are changed and the app is connected, the tunnel should reconnect and both entry and exit locations should be re-evaluated.

When selecting entry and exit relays, and either of the location constraints match only a single relay, which ever position has just a single matching relay gets to pick that one, and the single matching relay is excluded from the other position's possible set of relays. If both entry and exit positions match the same single relay, the relay selector should return an error and the app should go into the blocked state.

If both positions have multiple candidate relays, first an exit relay is picked and the selected exit relay is removed from the candidate set for the entry position.

When selecting a bridge for API communication, the relay selector should honor if multi hop is enabled or not - if it is, the entry constraints should be used to pick a bridge for API access, otherwise the exit constraints should be used to pick a bridge for API access.

When passing the relay selector result to the packet tunnel actor, the packet tunnel actor may ignore the entry relay for now.


This change is Reviewable

Copy link

linear bot commented Jun 26, 2024

@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 9dfd1ec to c9fd809 Compare June 26, 2024 12:17
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR goes the whole distance in terms of relay selector, but there's still work to do with integrating wireguardkit and the coming updates to it. As such there are some todos left and places where exit/entry endpoints and the like might not be completely correct yet.

Reviewable status: 0 of 52 files reviewed, all discussions resolved

@rablador rablador added the iOS Issues related to iOS label Jun 26, 2024
@rablador rablador marked this pull request as ready for review June 26, 2024 12:28
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 35 of 50 files at r1, 17 of 17 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 180 at r2 (raw file):

                    connectionAttemptCount: 0,
                    transportLayer: .udp,
                    remotePort: selectedRelays.exit.endpoint.ipv4Relay.port, // TODO: Multihop

Do we want to fix this for an upcoming PR ?


ios/MullvadVPN.xcodeproj/project.pbxproj line 2848 at r2 (raw file):

			isa = PBXGroup;
			children = (
				F01DAE322C2B032A00521E46 /* RelaySelection.swift */,

Order by name

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 180 at r2 (raw file):

Previously, buggmagnet wrote…

Do we want to fix this for an upcoming PR ?

Yep

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)


ios/MullvadREST/Relay/RelayPicking.swift line 75 at r2 (raw file):

        )

        let decisionFlow = OneToOne(

nit
It could be useful to have a comment here explaining why we do first 1 to 1, then 1 to many and finally many to many.

@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch 5 times, most recently from c85fefa to c282fd7 Compare June 27, 2024 14:46
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/MullvadREST/Relay/RelayPicking.swift line 75 at r2 (raw file):

Previously, buggmagnet wrote…

nit
It could be useful to have a comment here explaining why we do first 1 to 1, then 1 to many and finally many to many.

Done.


ios/MullvadVPN.xcodeproj/project.pbxproj line 2848 at r2 (raw file):

Previously, buggmagnet wrote…

Order by name

Done.

@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch 3 times, most recently from 392d7b0 to 42f9904 Compare July 1, 2024 07:53
@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 42f9904 to 7dd93ac Compare July 1, 2024 11:57
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 18 of 20 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 54 of 55 files reviewed, 1 unresolved discussion

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 54 of 55 files reviewed, 1 unresolved discussion

Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 20 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 7dd93ac to 217bd4b Compare July 3, 2024 14:29
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 8 files at r5, all commit messages.
Reviewable status: 59 of 63 files reviewed, 4 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/SelectLocation/AllLocationDataSource.swift line 105 at r5 (raw file):

            )

            if hostCode == "al-tia-wg-001" {

Did you forget to remove debug code ?


ios/MullvadVPN/View controllers/SelectLocation/AllLocationDataSource.swift line 116 at r5 (raw file):

                cityNode.children.sort()

//                hostNode.isExcluded = selectedRelays.excluded?.locations.contains { excludedLocation in

Please delete those comments


ios/MullvadVPN/View controllers/SelectLocation/CustomListLocationNodeBuilder.swift line 52 at r5 (raw file):

        listNode.isActive = !listNode.children.isEmpty
//        listNode.isExcluded = listNode.children.filter { !$0.isExcluded }.isEmpty

Please delete

@rablador rablador force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 217bd4b to 7dd93ac Compare July 4, 2024 07:55
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @buggmagnet from 4 discussions.
Reviewable status: 55 of 63 files reviewed, all discussions resolved (waiting on @buggmagnet)

@pinkisemils pinkisemils force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 7dd93ac to 1eb113d Compare July 11, 2024 11:17
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 52 of 63 files reviewed, all discussions resolved (waiting on @buggmagnet)

@pinkisemils pinkisemils force-pushed the ios-598-allow-relay-selector-to-select-an-entry-peer branch from 1eb113d to e6f0fcc Compare July 11, 2024 14:53
@pinkisemils pinkisemils merged commit 0c22831 into main Jul 12, 2024
8 of 9 checks passed
@pinkisemils pinkisemils deleted the ios-598-allow-relay-selector-to-select-an-entry-peer branch July 12, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants