chore: support operating the VPN without the app #36
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ethanndickson
force-pushed
the
ethan/vpn-no-app
branch
2 times, most recently
from
dd9870f to
3d407c4
Compare
| // If we moved from disabled to connected, then the NE was already | ||
| // running, and we need to request the current peer state | ||
| if tunnelState == .disabled { | ||
| xpc.getPeerState() |
There was a problem hiding this comment.
I feel like this kind of logic doesn't belong at this layer, where we're using history to infer what needs to happen next. Very path dependent.
Instead when we make an XPC connection we should explicitly (or implicitly on new connection) request the peer state. It's always the case that we need peer state at start of day.
As the tunnel goes up and down that also needs to be communicated (e.g. the user could stop the VPN via the System Settings).
There was a problem hiding this comment.
As the tunnel goes up and down that also needs to be communicated (e.g. the user could stop the VPN via the System Settings).
This function vpnDidUpdate gets called by the NotificationCentre, syncing it up with the VPN in System Settings. (Subscribing to events retrieves the current state too, so that works on app startup)
Instead when we make an XPC connection we should explicitly (or implicitly on new connection) request the peer state.
An XPC connection is only made when the app attempts to communicate with the NE, so we can't rely on XPC to tell us the NE is already running. (As seen in the NE & XPC example)
It's always the case that we need peer state at start of day.
However, always attempting to retrieve the peer state on CoderVPNService.init works just as well. Though if the NE isn't running, we do create a spurious error log. WDYT?
There was a problem hiding this comment.
I think it's fine for us to wait until we know whether the NE is running before we attempt to query the peer state. What I'm worried about is path dependence in the state machine: if we later add more states, or more transitions, you could end up in the .connected state in a way that does require us to query the peer state, but we haven't transitioned directly from the disabled state.
Could we just always request a peer state update when we get to .connected? That way we have some belief that the NE is running, so if we can't get it, it's a legit error. I realize in the case where we are starting the VPN up we'll request a spurious update, but are the updates idempotent?
There was a problem hiding this comment.
Yeah, the first update sent when the VPN starts will be identical to the one we manually request - but the one requested will overwrite the other if it arrives after. I'll have it always request on .connected.
There was a problem hiding this comment.
Actually, this is causing the NE to crash. vpnDidUpdate gets called when other parts of the VPN configuration change, not just the status, so the .connected branch was running multiple times on startup. The XPC was going through to the NE fine, but It looks like the speaker or the dylib can't keep up with multiple peer state requests in quick succession - We'll need to investigate that at some point, but not now.
| clearPeers() | ||
| applyPeerUpdate(with: msg) |
There was a problem hiding this comment.
Doesn't this clear call make handling the deletedAgents and deletedWorkspaces in applyPeerUpdate unnecessary since there won't be any workspaces or agents left?
There was a problem hiding this comment.
update.deletedAgents and update.deletedWorkspaces will always be empty on a getPeerState response, yeah. The Go code already keeps track of all known agents & workspaces, so as Dean suggested I think it'd be good to just send the full current state to the NE on any update.
ethanndickson
force-pushed
the
ethan/vpn-no-app
branch
from
1413f5a to
95bf6b5
Compare
Currently, CoderVPN disables it's system-level configuration when it's turned off. This means it can't be started and stopped from System Preferences. Since the network extension works fine without the app, we can remove this restriction.
However, it then becomes necessary to retrieve the state of the peers (agents) when starting the app whilst the VPN is running. We already have support for this on the dylib side, so this PR wires that functionality up over XPC.
Demo (I launch the app once the VPN reports itself as connected):
vpn-no-app-demo.mov
This behaviour is only present in a Release build of the app. In a Debug build, launching the app will trigger a replacement of the network extension installed on the system, but not before disconnecting the VPN.