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

Remove dependency on relay selector from tunnel actor #5261

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

pronebird
Copy link
Contributor

@pronebird pronebird commented Oct 10, 2023

Currently RelaySelectorResult (a part of RelaySelector module) is used across different parts of tunnel manager <-> tunnel actor.

RelaySelectorResult itself is produced by RelaySelector and passed as is from tunnel manager into actor via IPC to initiate connection, here is how the data layout looks like:

struct RelaySelectorResult {
    var endpoint: MullvadEndpoint // endpoints, gateways, pubkey
    var relay: REST.ServerRelay // Lotta info about relay returned from API
    var location: Location // Geo location
}

Then over time the tunnel manager receives a slice of that data via PacketTunnelRelay as it polls tunnel status:

struct PacketTunnelRelay { // PacketTunnelCore
    let ipv4Relay: IPv4Endpoint
    let ipv6Relay: IPv6Endpoint?
    let hostname: String
    let location: Location
}

In reality however, actor does not need the entire RelaySelectorResult, it would be enough to receive a type holding the following info instead:

struct SelectedRelay {
    var endpoint: MullvadEndpoint
    var hostname: String
    var location: Location
}

Changes

  • Replace RelaySelectorResult with SelectedRelay in IPC/Actor interaction and subsequently removes dependency on RelaySelector in the actor.
  • Pass NextRelay type instead of RelaySelectorResult? via reconnectTunnel() IPC method.

This change is Reviewable

@pronebird pronebird added the iOS Issues related to iOS label Oct 10, 2023
@linear
Copy link

linear bot commented Oct 10, 2023

IOS-339 Remove dependency on relay selector from tunnel actor

RelaySelectorProtocol – a requirement of PacketTunnelActor relies on RelaySelectorResult type exported from RelaySelector target.

We don't use all of the information provided byRelaySelectorResult but also we'd like to keep actor isolated from concrete implementation of relay selector.

So introducing a type with stricter requirements and exposing it from PacketTunnelCore could help remove the extra dependency and tighten up the architecture. For example:

struct SelectedRelay {
    var endpoint: MullvadEndpoint
    var hostname: String
    var location: Location
}

@pronebird pronebird force-pushed the selected-relay-struct-ios-339 branch 3 times, most recently from 4003ad6 to 8e259a2 Compare October 10, 2023 12:55
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 12 of 19 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @pronebird)


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

			isa = PBXGroup;
			children = (
				5819ABC22A8CF02C007B59A6 /* TunnelAdapterProtocol.swift */,

Nit
Can we reorder those files to be in alphabetical order ?


ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 18 at r2 (raw file):

        case selectedRelay = "selected-relay"

        /// Option key that holds the `NSNumber` value, which is when set to `1` indicates that

nit

        /// Option key that holds an `NSNumber` value, which, when set to `1` indicates that
        /// the tunnel was started by the system.

@pronebird pronebird force-pushed the selected-relay-struct-ios-339 branch from 8e259a2 to e8b4974 Compare October 11, 2023 08:29
Copy link
Contributor Author

@pronebird pronebird 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 7 files at r2.
Reviewable status: 17 of 20 files reviewed, all discussions resolved (waiting on @buggmagnet)


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

Previously, buggmagnet wrote…

Nit
Can we reorder those files to be in alphabetical order ?

Done but keep in mind that this change might lead likely to merge conflicts in outstanding PRs.


ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 18 at r2 (raw file):

Previously, buggmagnet wrote…

nit

        /// Option key that holds an `NSNumber` value, which, when set to `1` indicates that
        /// the tunnel was started by the system.

Done.

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 1 of 19 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pronebird pronebird force-pushed the selected-relay-struct-ios-339 branch from c3b337b to 26e7810 Compare October 11, 2023 10:09
Copy link
Contributor

@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.

Reviewed 13 of 19 files at r1, 5 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pronebird pronebird force-pushed the selected-relay-struct-ios-339 branch 3 times, most recently from ce157f6 to cedef94 Compare October 12, 2023 12: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 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Andrej Mihajlov added 2 commits October 13, 2023 09:08
…laySelector module

Also:
- IPC: accept NextRelay in "reconnect" message
- Fix dependency on MullvadREST (instead of MullvadTransport)
@buggmagnet buggmagnet merged commit c56ccac into main Oct 13, 2023
5 checks passed
@buggmagnet buggmagnet deleted the selected-relay-struct-ios-339 branch October 13, 2023 07:12
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.

3 participants