LLT-6855: Separation of daemon lifecycle from VPN connection state management - #1909
LLT-6855: Separation of daemon lifecycle from VPN connection state management#1909tomasz-kumor wants to merge 4 commits into
Conversation
f57a61c to
e00827b
Compare
e00827b to
2558ede
Compare
958e427 to
0c1644b
Compare
0c1644b to
26f42ca
Compare
26f42ca to
3111f18
Compare
3111f18 to
be8d6ac
Compare
be8d6ac to
9401f81
Compare
9401f81 to
e16efbb
Compare
e16efbb to
59fd4e2
Compare
59fd4e2 to
09d954f
Compare
09d954f to
507311f
Compare
507311f to
d34ae16
Compare
d34ae16 to
fa5151f
Compare
f42f791 to
6e90760
Compare
| pub async fn daemon_event_loop( | ||
| mut config: RunningConfig, | ||
| logging_handle: &mut logging::LoggingHandle, | ||
| auto_connect: bool, |
There was a problem hiding this comment.
Perhaps this param belongs to the config? Not saying to ditch the CLI flag, both can coexist
There was a problem hiding this comment.
Not really. It would require a bigger refactoring. Right now the RunningConfig contains the parsed data from the configuration file and some information about the configuration file itself. Command line parameter does not fit to it now.
|
|
||
| if let Err(e) = execute(Command::new("ip").args([ | ||
| "-6", | ||
| "route", | ||
| "del", | ||
| "default", | ||
| "dev", | ||
| &self.interface_name, | ||
| ])) { | ||
| error!("Failed to remove IPv6 default route: {e}"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Seeing this makes me wonder: is there anything else to be cleaned for IPv6? (eg: tables, routes, fwmarks)
There was a problem hiding this comment.
This was added to make the cleanup implementation symetric to set_exit_routes(). Without that there were sometimes problems with reconnecting after the VPN connection was disconnected.
| Disconnected, | ||
| Connected, | ||
| } | ||
|
|
There was a problem hiding this comment.
This enum is needless, the exit node status should be checked instead. See examples in the code
There was a problem hiding this comment.
isn't what TelioTaskCmd::GetStatus is doing? IIUC, you have created enum VpnConnectionState which is intrinsically Daemon's object, however this is an exclusive property of telio, the protocol lib.
There was a problem hiding this comment.
That's a good point. I have changed the implementation that if exit node was in connected or connecting state after reload, the app will try to re-establish the VPN connection.
| .await | ||
| } | ||
| ClientCmd::Connect => { | ||
| trace!("Connect"); |
There was a problem hiding this comment.
Maybe we can wrap self.telio_task_tx.send() and trace!() on every call. Wdyt?
There was a problem hiding this comment.
This change won't remove too much duplications. I would prefer not try to refactor this code in scope of this PR.
|
|
||
| #[tokio::test] | ||
| async fn test_command_connect_disconnect_error() { | ||
| test_command_error_helper(ClientCmd::Connect, "simulated connect failure").await; |
There was a problem hiding this comment.
'"simulated connect failure"' message doesn't add any info, pls add more details like what was the test intention and what failed
There was a problem hiding this comment.
Slightly improved it by adding comments and documenting the helper function.
|
|
||
| let endpoint_result = ctx | ||
| .tokio_handle | ||
| .block_on(get_server_endpoints_list(&ctx.config)) |
There was a problem hiding this comment.
This API call can block the whole daemon for long time, and I think would be best to avoid moving the runtime around.
| config: NordVpnLiteConfig, | ||
| exit_node: Option<ExitNodeStatus>, | ||
| connection_state: VpnConnectionState, | ||
| /// Handle to the tokio runtime. Used to run async tasks (e.g. HTTP calls) from |
There was a problem hiding this comment.
Used to run async tasks
You're using it to run sync tasks.
There was a problem hiding this comment.
I am calling the get_server_endpoints_list() which is async.
| exit_node_config: ExitNodeConfig, | ||
| ) -> Result<(), NordVpnLiteError> { | ||
| self.interface_config_provider | ||
| .set_exit_routes(&exit_node_config.endpoint.address, &exit_node_config.dns) |
There was a problem hiding this comment.
set_exit_routes() can be called twice if handle_exit_node_connection() is still fetching the endpoints, this should be fixed by making the endpoints fetch async though.
| f"got {status['exit_node']['state']!r}" | ||
| ) | ||
|
|
||
| log.debug("Confirmed: VPN connection intact after duplicate connect") |
There was a problem hiding this comment.
You're asserting that nordvpnlite reports connected, you should (also) assert that the connection is working independently of what the app says.
There was a problem hiding this comment.
How should I do that?
| f"but exit_node={status.get('exit_node')}" | ||
| ) | ||
|
|
||
| log.debug("Confirmed: VPN disconnected") |
There was a problem hiding this comment.
I think these tests can be already split into their own categories/classes/mods, e.g.: TestLog, TestConnection.. just like libtelio.
There was a problem hiding this comment.
They could be but there are not that many of them yet. 3 tests for connect/disconnect command and 2 for reload.
6e90760 to
3d885ab
Compare
|
Rebased on top of main |
Currently, starting the nordvpnlite daemon is strictly coupled with establishing a VPN connection. This commit introduces a new flag that allows the daemon startup to be separated from establishing a VPN connection, without breaking the legacy behaviour.
Without explicitly deleting the IPv6 default route, the route persisted after the interface was brought down, causing stale routing entries and potential IPv6 traffic leaks through the old interface.
After reload, the daemon re-evaluated --do-not-connect instead of the VPN last state and as a result after the reload VPN connection state could be different than the pre-reload state.
Currently, establishing a VPN connection is tightly coupled with the NordVPN Lite daemon start/stop lifecycle. This commit decouples VPN connection management from daemon state management, establishing clear separation of concerns between the two functionalities.
3d885ab to
2549ddd
Compare
Problem
Currently the VPN connection state is tightly coupled with the daemon lifecycle and there is not way to connect/disconnect VPN exit node without changing the NordVPN Lite daemon state.
Solution
nordvpnlite start --do-not-connectflag to allow separate launching the daemon from establishing VPN connection.connect/disconnectcommands that allows to control the VPN connection stateTesting
Check that daemon is running but VPN connection is not established
Connect to the VPN exit node & check that VPN connection is established
Disconnect from the VPN exit node
☑️ Definition of Done checklist