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

Add IAN-based TunnelPinger, refactoring the pinger protocol accordingly. #6802

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented Sep 17, 2024

This depends on the IAN changes to wireguard-apple, and implements a Pinger named TunnelPinger which uses the tunnel's WireGuard-based ICMP ECHO capabilities to send and receive pings. The Pinger protocol has been modified, moving the address to the setup phase in accordance with how WireGuard ICMP sockets like it.


This change is Reviewable

Copy link

linear bot commented Sep 17, 2024

@acb-mv acb-mv added the iOS Issues related to iOS label Sep 17, 2024
@acb-mv acb-mv self-assigned this Sep 17, 2024
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.

The tests don't compile, please make sure that they do and fix them

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions (waiting on @acb-mv)


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 35 at r1 (raw file):

    }

    var socketHandle: Int32?

This doesn't seem to be used, let's delete it


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 37 at r1 (raw file):

    var socketHandle: Int32?

    var pingProvider: ICMPPingProvider

private


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 47 at r1 (raw file):

        self.logger = Logger(label: "TunnelPinger")
    }
    

Please make sure to run swiftformat


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 79 at r1 (raw file):

            replyQueue.async { [weak self] in
                guard let self else { return }
                // NOTE: we cheat here by returning the destination address we were passed, rather than parsing it from the packet on the other side of the FFI boundary.

This comment doesn't seem to make too much sense here, we should probably move it to the reply = .success part


ios/PacketTunnelCoreTests/Mocks/PingerMock.swift line 99 at r1 (raw file):

        var isSocketOpen = false
        var onReply: ((PingerReply) -> Void)?
        var destAddress: IPv4Address? = nil

nil assignment is redundant here

Copy link
Contributor Author

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

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


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 35 at r1 (raw file):

Previously, buggmagnet wrote…

This doesn't seem to be used, let's delete it

Done.


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 37 at r1 (raw file):

Previously, buggmagnet wrote…

private

Done.


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 47 at r1 (raw file):

Previously, buggmagnet wrote…

Please make sure to run swiftformat

Done.


ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 79 at r1 (raw file):

Previously, buggmagnet wrote…

This comment doesn't seem to make too much sense here, we should probably move it to the reply = .success part

Done.


ios/PacketTunnelCoreTests/Mocks/PingerMock.swift line 99 at r1 (raw file):

Previously, buggmagnet wrote…

nil assignment is redundant here

Done.

@acb-mv acb-mv force-pushed the IOS-753-TunnelPinger branch 2 times, most recently from d91aa61 to 879b0ff Compare September 18, 2024 13:58
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 4 of 10 files at r1, 4 of 6 files at r2.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion

@buggmagnet buggmagnet merged commit 7d87bbb into main Sep 18, 2024
8 of 10 checks passed
@buggmagnet buggmagnet deleted the IOS-753-TunnelPinger branch September 18, 2024 14:29
Copy link

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

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.

2 participants