-
Notifications
You must be signed in to change notification settings - Fork 373
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
Conversation
There was a problem hiding this 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
122b2b4
to
8bfb248
Compare
There was a problem hiding this 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.
d91aa61
to
879b0ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 10 files at r1, 4 of 6 files at r2.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion
e86df23
to
d7c7eaa
Compare
d7c7eaa
to
e90f94d
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
This depends on the IAN changes to
wireguard-apple
, and implements a Pinger namedTunnelPinger
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](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)