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

chore: add network extension manager #18

Merged
merged 5 commits into from
Jan 14, 2025
Merged

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Jan 8, 2025

Relates to #2.

Adds the Manager abstraction that:

  • Downloads the dylib
  • Calls the SignatureValidator on a downloaded dylib
  • Passes network settings to NEPacketTunnelProvider
  • Owns the TunnelHandle
  • Reads and writes to the Speaker

Eventually, it'll act as the middleman between the tunnel Speaker and the XPC speaker.

Copy link
Member Author

ethanndickson commented Jan 8, 2025

@ethanndickson ethanndickson marked this pull request as ready for review January 8, 2025 09:57
@ethanndickson ethanndickson marked this pull request as draft January 8, 2025 09:58
@ethanndickson ethanndickson force-pushed the ethan/ne-manager branch 6 times, most recently from 617a941 to 300889e Compare January 10, 2025 04:07
@@ -4,16 +4,200 @@ import VPNLib

actor Manager {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is worth unit testing, at least right now. We'd need to mock the PTP, the TunnelHandle, the validator, and eventually the XPC speaker, all for a relatively simple abstraction that's mostly error handling.

Comment on lines +215 to +216
// This can't be checked at compile-time, as both Tasks & Continuations can only ever throw
// a type-erased `Error`
Copy link
Member Author

@ethanndickson ethanndickson Jan 10, 2025

Choose a reason for hiding this comment

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

This is a little frustrating - theTask type itself is generic on two type parameters, Success and Failure, but the only APIs you can await on a task are either value with an untyped throws or result which returns a type-erased any Error error variant. It's very similar for continuations, the type itself is generic but there's no way to return that typed error.

As an aside, I think I'm becoming more of a fan, for binaries at least, of just having a single opaque error type, propagating it with context, and then just displaying it, ala Go or Rust's anyhow - it's so rare that one or more of the error variants are recoverable, and that the caller needs to know which one.

@ethanndickson ethanndickson force-pushed the ethan/ne-manager branch 2 times, most recently from 4922fcf to 7e24349 Compare January 10, 2025 05:28
@ethanndickson ethanndickson marked this pull request as ready for review January 10, 2025 05:38
@ethanndickson ethanndickson self-assigned this Jan 10, 2025
@ethanndickson ethanndickson requested a review from coadler January 13, 2025 04:51
@ethanndickson
Copy link
Member Author

@coadler Requesting your review here since Spike is OOO, and this is pretty relevant to you. This is the portion of the network extension that communicates over XPC.

@ethanndickson ethanndickson changed the base branch from ethan/dylib-download to graphite-base/18 January 14, 2025 04:48
@ethanndickson ethanndickson changed the base branch from graphite-base/18 to main January 14, 2025 04:49
Copy link
Member Author

ethanndickson commented Jan 14, 2025

Merge activity

  • Jan 14, 12:19 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 14, 12:19 AM EST: A user merged this pull request with Graphite.

@ethanndickson ethanndickson merged commit 46c2c09 into main Jan 14, 2025
4 checks passed
@ethanndickson ethanndickson deleted the ethan/ne-manager branch January 14, 2025 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants