Skip to content

fix(gui-client/windows): delete IPC service logs when user clicks "clear logs"#6280

Merged
ReactorScram merged 15 commits intomainfrom
refactor/clear-ipc-logs
Aug 14, 2024
Merged

fix(gui-client/windows): delete IPC service logs when user clicks "clear logs"#6280
ReactorScram merged 15 commits intomainfrom
refactor/clear-ipc-logs

Conversation

@ReactorScram
Copy link
Contributor

@ReactorScram ReactorScram commented Aug 13, 2024

Closes #5453

Tested once on the Windows aarch64 VM. Should always leave 4 files behind, a .log and a .jsonl for the GUI and for the IPC service. The "log directory" is a bit of a lie since it's consistently 2 directories on both platforms now.

- [x] Update changelog
- [x] Make a note to remove the known issue from the website when the next release is cut after this PR merges

The headless Client doesn't have a corresponding IPC service, so the
`TerminatingGracefully` message for it was unused. This PR shuffles the
messages around into better categories to get rid of unused cases like that.

This is also a yak shave towards fixing #5453
@ReactorScram ReactorScram self-assigned this Aug 13, 2024
@vercel
Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 2:48pm

@ReactorScram ReactorScram changed the title refactor(gui-client): make a place for the IPC service to clear its logs fix(gui-client): delete IPC service logs when user clicks "clear logs" Aug 13, 2024
@ReactorScram ReactorScram marked this pull request as ready for review August 13, 2024 18:08
@ReactorScram ReactorScram changed the base branch from refactor/clean-up-ipc-types to main August 13, 2024 19:54
@ReactorScram ReactorScram changed the base branch from main to fix/delete-crash-dumps August 13, 2024 20:04
@ReactorScram ReactorScram changed the title fix(gui-client): delete IPC service logs when user clicks "clear logs" fix(gui-client/windows): delete IPC service logs when user clicks "clear logs" Aug 13, 2024
Copy link
Member

Choose a reason for hiding this comment

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

So this got added here because both the GUI and the IPC service need it, right?

Could this live in the IPC service and it simply also clears the logs for the GUI? The gui can send its path in the request to delete logs, then this can be a private function in the service module.

Copy link
Member

Choose a reason for hiding this comment

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

Like, call it with two different paths in the IPC service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IPC service shouldn't even be able to read files inside /home and I like that sandbox quite a lot, since we have to run as root:

I could move it into headless-client since I think gui-client still depends on that?

Copy link
Member

Choose a reason for hiding this comment

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

I see and I guess the other way round too, we can't modify the files of the IPC service, right?

On Linux, should we even be writing log files? Isn't writing to journald strictly better? (Different topic, I know)

I could move it into headless-client since I think gui-client still depends on that?

Nah I'd rather not expose utilities like that, esp. since the headless client itself doesn't even use that.

Copy link
Member

@thomaseizinger thomaseizinger Aug 13, 2024

Choose a reason for hiding this comment

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

On Linux, should we even be writing log files? Isn't writing to journald strictly better? (Different topic, I know)

There is https://crates.io/crates/tracing-journald at least.

We could experiment with that for the GUI and let the IPC service just write to stdout.

Not sure what Windows offers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it would be more idiomatic for a systemd service to write to stdout.

But then we'd have to do something special to export zips, since it's no longer just files on disk.

I did move it into headless-client since that's where the IPC service lives for now. We could split it up, but then there's more crates. We could not split it up, but then the "headless client" pound for pound is mostly IPC and IPC service code.

Yes, the GUI can't modify the logs of the IPC service by default. On Windows I didn't find any easy solution, that's why I went with this. On Linux I made an exception by making the logs owned by root:firezone-client, so the GUI actually can modify the logs, since it must run as a member of firezone-client

Copy link
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Approving to unblock but I am wondering if we need to move it into the shared crate.

@ReactorScram
Copy link
Contributor Author

I'll merge this as-is and we can split up the IPC service from firezone-headless-client later. Maybe it can just live in firezone-gui-client and any small shared stuff (I think CallbackHandler is shared?) can either go in bin-shared or be duplicated.

@ReactorScram ReactorScram added this pull request to the merge queue Aug 14, 2024
Merged via the queue into main with commit 79c9811 Aug 14, 2024
@ReactorScram ReactorScram deleted the refactor/clear-ipc-logs branch August 14, 2024 15:22
@thomaseizinger
Copy link
Member

I'll merge this as-is and we can split up the IPC service from firezone-headless-client later. Maybe it can just live in firezone-gui-client and any small shared stuff (I think CallbackHandler is shared?) can either go in bin-shared or be duplicated.

I'd like to get to a point (see #4470) where the API of firezone-tunnel is ergonomic enough to use that there shouldn't need to be any "shared" crate for clients that want to use it. Until then, connlib-client-shared is the place where this stuff should probably go.

bin-shared I'd like to be tunnel-agnostic stuff (not sure how I feel about the TunDeviceManager in there).

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.

bug(gui-client/windows): Clear Logs button isn't working on main

2 participants