feat(windows-client): run the GUI and tunnel in separate processes #4978

ReactorScram · 2024-05-13T16:58:30Z

Ready for review.

Closes #3712.
Supersedes #4940.
Refs #4963.

I haven't figured out if it needs any new automated tests (unit, integration, etc.) but the code itself is ready for review. There is more refactoring that could be done, or could be left for later.

Tasks

Give feedback

Move wintun setup from GUI to IPC service / headless client

Move wintun setup from GUI to IPC service / headless client
Options
Successfully updated the issue's project

There was an error updating the issue's project
Make sure the device ID is in a sensible place

Make sure the device ID is in a sensible place
Options
Successfully updated the issue's project

There was an error updating the issue's project
Export IPC service logs in the zips

Export IPC service logs in the zips
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test GUI + SC IPC service on Windows (f4db808919a passed)

Test GUI + SC IPC service on Windows (f4db808919a passed)
Options
Successfully updated the issue's project

There was an error updating the issue's project
Make sure IPC service does not busy-loop

Make sure IPC service does not busy-loop
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test un-install checklist for Windows

Test un-install checklist for Windows
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test upgrade checklist for Windows

Test upgrade checklist for Windows
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test GUI + systemd IPC service on Linux (c4ab7e7 passed)

Test GUI + systemd IPC service on Linux (c4ab7e7 passed)
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test upgrade checklist for Linux

Test upgrade checklist for Linux
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test un-install checklist for Linux

Test un-install checklist for Linux
Options
Successfully updated the issue's project

There was an error updating the issue's project
Make sure the IPC service logs out and deactivates DNS control if the GUI crashes

Make sure the IPC service logs out and deactivates DNS control if the GUI crashes
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test network changing

Test network changing
Options
Successfully updated the issue's project

There was an error updating the issue's project
(it's intended behavior) ~~Look into spurious `on_update_resources` (fad86babd7)~~

(it's intended behavior) ~~Look into spurious on_update_resources (fad86babd7)~~
Options
Successfully updated the issue's project

There was an error updating the issue's project
~~Test max partition time on offline laptop~~ (I ended up just setting a 30-day default in the code)

~~Test max partition time on offline laptop~~ (I ended up just setting a 30-day default in the code)
Options
Successfully updated the issue's project

There was an error updating the issue's project
Make sure headless Client does not busy-loop

Make sure headless Client does not busy-loop
Options
Successfully updated the issue's project

There was an error updating the issue's project
Test standalone headless on Linux

Test standalone headless on Linux
Options
Successfully updated the issue's project

There was an error updating the issue's project
Add unit / integration tests

Add unit / integration tests
Options
Successfully updated the issue's project

There was an error updating the issue's project
Think about security a bit #3971

Think about security a bit gui-client: Write threat model for Windows / Linux multi-process change #3971
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

…gets confused

…-poc

I'm going to want a well-known dir that the Windows IPC service writes logs to, and that the Windows GUI can pick them up from. I don't know how I did this for Linux last week, but it should probably be in here too.

…one into chore/windows-service-poc

This has a known gap where theoretically the GUI could sign in while the service is hung in startup, and then the service would wipe out the GUI's DNS rules. The workaround for that would be to restart the GUI, but in practice I think this is almost impossible, Windows would have to give the service no CPU time while the user was signing in, then the user would have to immediately open Firezone before the service got running. Closes #4899

…-poc

thomaseizinger

Left a few ideas and questions. Feel free to defer any of the refactoring if you don't feel like it!

There are some questions that I think we should answer before merging (like exit codes on error), hence not approving yet.

rust/gui-client/src-tauri/src/client/tunnel_wrapper/windows.rs

rust/gui-client/src-tauri/src/client/ipc/windows.rs

rust/gui-client/src-tauri/src/client/elevation.rs

rust/gui-client/src-tauri/src/client.rs

rust/headless-client/src/windows.rs

jamilbk · 2024-05-17T16:21:20Z

Left a few ideas and questions. Feel free to defer any of the refactoring if you don't feel like it!

There are some questions that I think we should answer before merging (like exit codes on error), hence not approving yet.

Yeah if possible let's defer the refactoring until next week so we can ship this. Quite a big PR already

…ezone into chore/windows-ipc-service-2

ReactorScram · 2024-05-17T18:58:18Z

@thomaseizinger I fixed the exit code on error case. I'm not sure if Windows does anything with that exit code but it looks better with it fixed, anyway.

There's like 10+ refactorings I'm pushing off in #5022. I also found that it takes about 30 seconds to sign in on Windows, but I think that's due to the DNS control being slow after the last set of DNS fixes, not due to a regression related to IPC. I can test that next week.

thomaseizinger

LGTM, thank you for working in the comments. Let me know if you want to pair on any of the codec stuff! :)

Approving modulo actually setting a non-zero exit code for the Windows service.

rust/headless-client/src/windows.rs

…vice-2

```[tasklist] ### Before opening for review - [ ] ~~Wait for some other refactors to merge~~ - [x] Test Windows - [x] Test Linux ```

ReactorScram added 30 commits May 7, 2024 13:29

proof of concept for installing a system service with WiX

e742f80

this one gets the service to run, but it doesn't respond, so Windows …

c83174f

…gets confused

Merge remote-tracking branch 'origin/main' into chore/windows-service…

6d36d31

…-poc

fmt

abc9953

this one starts and stops properly

e615f93

fix Linux build

b8a8670

Merge remote-tracking branch 'origin/main' into chore/windows-service…

4011ca3

…-poc

refactor(gui-client): move known_dirs module into headless-client

618c941

I'm going to want a well-known dir that the Windows IPC service writes logs to, and that the Windows GUI can pick them up from. I don't know how I did this for Linux last week, but it should probably be in here too.

fix Linux build

442e01c

Merge branch 'refactor/move-known-dirs' into chore/windows-service-poc

7e17b56

Merge branch 'chore/windows-service-poc' of github.com:firezone/firez…

8585e2d

…one into chore/windows-service-poc

DRY log dirs

679a9da

typo

606469f

fix logging

12edbc1

fix stopping, remove debug file

0c77e34

opt in to receive shutdown signals

9bc5d5c

do a test install of the MSI

6083819

/passive wasn't working in CI

0a35424

try to fix CI

d5f58a6

you gotta escape those deadly forward slashes

900737b

fix test checklist

0f9de54

Merge branch 'chore/windows-service-poc' into fix/windows-dns-4899

c654e35

Merge remote-tracking branch 'origin/main' into chore/windows-service…

470eb1c

…-poc

Merge branch 'chore/windows-service-poc' into fix/windows-dns-4899

ed5ee6a

Merge branch 'main' into chore/windows-service-poc

91b2c10

Merge branch 'chore/windows-service-poc' into fix/windows-dns-4899

dc9eb42

remove unimplemented fn

0a7e678

feedback from review

9be70e2

Merge branch 'main' into chore/windows-service-poc

733afa0

ReactorScram requested a review from conectado May 16, 2024 14:50

ReactorScram mentioned this pull request May 16, 2024

feat: Windows headless / CLI client #3782

Open

thomaseizinger reviewed May 17, 2024

View reviewed changes

ReactorScram added 8 commits May 17, 2024 11:36

use question mark here

8c5f3a6

admit we aren't handling SHUTDOWN

9bd75d9

set error code if the Windows service fails

fc466ff

comments

063fbaf

mark known dirs as unimplemented for macos

dbd35ef

use transpose

99aabb1

add error here

2a20b37

fmt

a92e13d

ReactorScram mentioned this pull request May 17, 2024

Initial sign-in takes 30 seconds on Windows #5026

Open

ReactorScram added 3 commits May 17, 2024 12:29

typo

6bcd611

Merge branch 'chore/windows-ipc-service-2' of github.com:firezone/fir…

c9e18f6

…ezone into chore/windows-ipc-service-2

Merge branch 'main' into chore/windows-ipc-service-2

a2868cb

vercel bot deployed to Preview May 17, 2024 18:02 View deployment

ReactorScram requested a review from thomaseizinger May 17, 2024 18:56

thomaseizinger approved these changes May 18, 2024

View reviewed changes

rust/headless-client/src/windows.rs Outdated Show resolved Hide resolved

ReactorScram added 3 commits May 20, 2024 09:10

fix exit code

5f67ceb

30 day default max partition time on IPC services

638e971

Merge remote-tracking branch 'origin/main' into chore/windows-ipc-ser…

275e3cd

…vice-2

vercel bot deployed to Preview May 20, 2024 20:43 View deployment

ReactorScram added this pull request to the merge queue May 20, 2024

Merged via the queue into main with commit 1ef775d May 20, 2024
135 checks passed

ReactorScram deleted the chore/windows-ipc-service-2 branch May 20, 2024 21:49

ReactorScram mentioned this pull request May 21, 2024

Finish refactoring from #4978 #5022

Open

github-merge-queue bot pushed a commit that referenced this pull request May 31, 2024

refactor(gui-client): finish refactors from #4978 (#5158)

94cb494

```[tasklist] ### Before opening for review - [ ] ~~Wait for some other refactors to merge~~ - [x] Test Windows - [x] Test Linux ```

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(windows-client): run the GUI and tunnel in separate processes #4978

feat(windows-client): run the GUI and tunnel in separate processes #4978

ReactorScram commented May 13, 2024 •

edited

Tasks

thomaseizinger left a comment

jamilbk commented May 17, 2024

ReactorScram commented May 17, 2024

thomaseizinger left a comment •

edited

feat(windows-client): run the GUI and tunnel in separate processes #4978

feat(windows-client): run the GUI and tunnel in separate processes #4978

Conversation

ReactorScram commented May 13, 2024 • edited

Tasks

thomaseizinger left a comment

Choose a reason for hiding this comment

jamilbk commented May 17, 2024

ReactorScram commented May 17, 2024

thomaseizinger left a comment • edited

Choose a reason for hiding this comment

ReactorScram commented May 13, 2024 •

edited

thomaseizinger left a comment •

edited