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

Consolidate retry attempts in a single location #6474

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Jul 17, 2024

The relay selector is now smart enough to choose relays for entry and exit peers. It is time to apply multihop on the tunnel. This PR introduces the ability to establish multihop connections without PQ. Implementing multihop with PQ will be addressed in another PR.


This change is Reviewable

Copy link

linear bot commented Jul 17, 2024

@mojganii mojganii self-assigned this Jul 17, 2024
@mojganii mojganii added iOS Issues related to iOS feature request For issues asking for new features labels Jul 17, 2024
@mojganii mojganii changed the title consolidate-retry-attempts-in-a-single-location Consolidate retry attempts in a single location Jul 17, 2024
@mojganii mojganii requested review from acb-mv and buggmagnet July 17, 2024 14:51
@pinkisemils
Copy link
Collaborator

I think you should rebase this on main, and drop the commit that updates wireguard-apple, then maybe the unit tests fill pass.

@pinkisemils
Copy link
Collaborator

The same goes for changing ios/build-wireguard-go.sh.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 447 at r1 (raw file):

            connectionState.connectedEndpoint,
            settings: settings,
            retryAttempts: connectionState.selectedRelays.retryAttempt

Should the retryAttempt field not be better suited to stay in connectionState?

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 28 of 28 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)

@mojganii mojganii force-pushed the apply-multihop-for-normal-connection-ios-736 branch from e64d571 to b3138d9 Compare July 19, 2024 13:32
Copy link
Collaborator Author

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

I dropped the last commit as we discussed before.

Reviewable status: 26 of 28 files reviewed, 1 unresolved discussion (waiting on @acb-mv and @pinkisemils)


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 447 at r1 (raw file):

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

Should the retryAttempt field not be better suited to stay in connectionState?

it's using in relay selector. it's a common filed between them.

Copy link
Collaborator

@pinkisemils pinkisemils 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: 26 of 28 files reviewed, all discussions resolved (waiting on @acb-mv)


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 447 at r1 (raw file):

Previously, mojganii wrote…

it's using in relay selector. it's a common filed between them.

Is it passed to the relay selector or is it written to by the relay selector?

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 25 of 28 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mojganii)


ios/MullvadVPN/TunnelManager/Tunnel+Settings.swift line 20 at r2 (raw file):

    init(logger: Logger? = nil) {
        self.logger = logger

This is confusing, what was the problem being solved by dependency injecting a logger object here ?
Loggers should not be shared around, they are bound to a specific object and it will just make the logs more confusing if they are starting to log for the wrong instance.


ios/MullvadVPN/TunnelManager/Tunnel+Settings.swift line 23 at r2 (raw file):

    }

    func shouldReconnectToNewRelay(oldSettings: LatestTunnelSettings, newSettings: LatestTunnelSettings) -> Bool {

nit
I'm wondering right now where are the places in the code base that trigger a reconnection to the VPN without going through this logic.
Ideally, it should be done in one place only, and this seems like a good candidate.
It might be worth discussing this as a team.


ios/MullvadVPN/TunnelManager/TunnelState.swift line 81 at r2 (raw file):

    var description: String {
        return switch self {

nit
We can omit return here


ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelSettingsStrategyTests.swift line 13 at r2 (raw file):

final class TunnelSettingsStrategyTests: XCTestCase {
    func connectToNewRelayOnMultihopChangesTest() {

None of those tests are running because the function name does not start with test.
Please prefix all those with test


ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 61 at r2 (raw file):

        }

        do {

If we are not making use of the async wrapper we wrote for this, might as well go all non async way and do the following instead

        adapter.stop { _ in
            adapter.startMultihop(exitConfiguration: exitConfiguration, entryConfiguration: entryConfiguration) {
                logger.error("\($0?.localizedDescription ?? "Error starting the adapter")")
            }
        }

Otherwise, let's add an async wrapper to this API in WireGuardAdapter+Async.swift

@mojganii mojganii force-pushed the apply-multihop-for-normal-connection-ios-736 branch from b3138d9 to 840c1d8 Compare July 23, 2024 09:33
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 28 files at r1, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii)


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 297 at r3 (raw file):

                interfaceAddresses: settings.interfaceAddresses,
                dns: settings.dnsServers,
                endpoint: entry.endpoint,

This should be the connectionState.connectedEndpoint otherwise, UDP2TCP obfuscation won't be used.

@mojganii mojganii force-pushed the apply-multihop-for-normal-connection-ios-736 branch from 840c1d8 to 805a8c2 Compare July 25, 2024 08:51
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:

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

@buggmagnet buggmagnet force-pushed the apply-multihop-for-normal-connection-ios-736 branch from 805a8c2 to c5acfb9 Compare July 25, 2024 13:26
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii)

Copy link
Collaborator

@pinkisemils pinkisemils 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, 6 unresolved discussions (waiting on @mojganii)

@buggmagnet buggmagnet merged commit cdf2654 into main Jul 25, 2024
7 of 9 checks passed
@buggmagnet buggmagnet deleted the apply-multihop-for-normal-connection-ios-736 branch July 25, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request For issues asking for new features iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants