Skip to content

fix(vpn): handle sending nil router config #16267

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2025
Merged

fix(vpn): handle sending nil router config #16267

merged 1 commit into from
Jan 28, 2025

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Jan 27, 2025 •
edited
Loading

Previously, a nil Router config would cause a panic in the dylib. Normally, a nil Router config would indicate a shutdown of the service, and that settings should be reset. However, for Coder Desktop macOS the network configuration will be reset by the disconnecting of the system VPN, so we'll instead do nothing.

@ethanndickson Graphite App
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ethanndickson ethanndickson force-pushed the ethan/nil-router-cfg branch 2 times, most recently from 42434e7 to 1e05884 Compare January 27, 2025 09:14
ethanndickson
ethanndickson commented Jan 27, 2025
View reviewed changes
@ethanndickson ethanndickson marked this pull request as ready for review January 27, 2025 09:17
johnstcn
johnstcn reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

This change looks OK to me, but I'm by no means an expert on this part of the code. Deferring approval to @spikecurtis .

@ethanndickson ethanndickson requested a review from coadler January 27, 2025 10:34
@spikecurtis
Copy link
Contributor

I think I'm missing the higher level context: why do we need to send a nil router config in the first place? E.g. why is that preferable to not sending anything?

@ethanndickson
Copy link
Member Author

I think I'm missing the higher level context: why do we need to send a nil router config in the first place? E.g. why is that preferable to not sending anything?

Oh, I should've mentioned a nil router Config is supplied as a way of sending a shutdown - that the config should be reset.
https://github.com/coder/tailscale/blob/c7962497b482239cc37bfe3fec025cfc02458493/wgengine/router/router_openbsd.go#L68-L71

@ethanndickson
Copy link
Member Author

ethanndickson commented Jan 27, 2025 •
edited
Loading

Truth be told, I don't think our macOS implementation needs to actually do any cleanup (shutting down the VPN should be sufficient), whilst the Tailscale one does - so maybe we just don't send anything...

@spikecurtis
Copy link
Contributor

Truth be told, I don't think our macOS implementation needs to actually do any cleanup (shutting down the VPN should be sufficient), whilst the Tailscale one does - so maybe we just don't send anything...

Yeah, that makes sense to me.

@ethanndickson ethanndickson merged commit ab92306 into main Jan 28, 2025
37 checks passed
@ethanndickson ethanndickson deleted the ethan/nil-router-cfg branch January 28, 2025 09:47
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn left review comments

@coadler coadler coadler approved these changes

@spikecurtis spikecurtis Awaiting requested review from spikecurtis

Assignees

@ethanndickson ethanndickson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ethanndickson @spikecurtis @johnstcn @coadler