Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ethanndickson merged 1 commit intomainfromethan/nil-router-cfg
Jan 28, 2025

Conversation

@ethanndickson
Copy link
Member

@ethanndicksonethanndickson commentedJan 27, 2025•
edited
Loading

Previously, anil 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.

@ethanndicksonGraphite App
Copy link
MemberAuthor

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

@ethanndicksonethanndicksonforce-pushed theethan/nil-router-cfg branch 2 times, most recently from42434e7 to1e05884CompareJanuary 27, 2025 09:14
@ethanndicksonethanndickson marked this pull request as ready for reviewJanuary 27, 2025 09:17
Copy link
Member

@johnstcnjohnstcn 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 .

@spikecurtis
Copy link
Contributor

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

@ethanndickson
Copy link
MemberAuthor

I think I'm missing the higher level context: why do we need to send anil 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
MemberAuthor

ethanndickson commentedJan 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.

@ethanndicksonethanndickson merged commitab92306 intomainJan 28, 2025
37 checks passed
@ethanndicksonethanndickson deleted the ethan/nil-router-cfg branchJanuary 28, 2025 09:47
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 28, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@johnstcnjohnstcnjohnstcn left review comments

@spikecurtisspikecurtisAwaiting requested review from spikecurtis

+1 more reviewer

@coadlercoadlercoadler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ethanndicksonethanndickson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ethanndickson@spikecurtis@johnstcn@coadler
[8]ページ先頭
©2009-2025 Movatter.jp