Skip to content

fix(client/windows): set MTU even if IPv6 is disabled#6681

Merged
ReactorScram merged 5 commits intomainfrom
fix/windows-ipv4-only
Sep 13, 2024
Merged

fix(client/windows): set MTU even if IPv6 is disabled#6681
ReactorScram merged 5 commits intomainfrom
fix/windows-ipv4-only

Conversation

@ReactorScram
Copy link
Contributor

@ReactorScram ReactorScram commented Sep 13, 2024

Refs #6547, this fixes a similar error message but it's not the same exact issue.

When IPv6 is disabled on a system, our call to set the MTU was failing with error code 0x80070490. This patch allows some of the MTU-related syscalls to fail with a warning log.

To replicate the issue, run this command to set a registry value to disable IPv6, then reboot the system:

reg add "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters" /v DisabledComponents /t REG_DWORD /d 255 /f

- [x] Update changelog
- [x] Apply PR feedback

Refs #6547, this fixes a similar error message but it's not the same exact
issue.

When IPv6 is disabled on a system, our call to set the MTU was failing.
This patch allows some of the MTU-related syscalls to fail with a warning log.

To replicate the issue, run this command to set a registry value to disable
IPv6, then reboot the system:

`reg add "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters" /v DisabledComponents /t REG_DWORD /d 255 /f`
@vercel
Copy link

vercel bot commented Sep 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 Sep 13, 2024 5:15pm

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.

Can we add an "ignored" regression test that modifies the registry entry and then tries to bring up a tunnel? Ideally with disabled IPv4 or IPv6?

unsafe { SetIpInterfaceEntry(&mut row) }.ok()?;
// SAFETY: TODO
if unsafe { GetIpInterfaceEntry(&mut row) }.ok().is_err() {
tracing::warn!(?family, "Couldn't set MTU");
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow check an error code that is the equivalent of "not found" and not log a warning on that?

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram
Copy link
Contributor Author

ReactorScram commented Sep 13, 2024

@thomaseizinger I asked ChatGPT if we can disable IPv6 without rebooting (I assume rebooting the CI runners is a non-starter) and it suggested this command netsh interface ipv6 set interface "Ethernet" disable

We'd just have to discover what the name of the runners' network interface is, and substitute it for "Ethernet".

Edit: It gave me a solution to that, too. I'll look into it

@thomaseizinger
Copy link
Member

thomaseizinger commented Sep 13, 2024

@thomaseizinger I asked ChatGPT if we can disable IPv6 without rebooting (I assume rebooting the CI runners is a non-starter) and it suggested this command netsh interface ipv6 set interface "Ethernet" disable

We'd just have to discover what the name of the runners' network interface is, and substitute it for "Ethernet".

Edit: It gave me a solution to that, too. I'll look into it

Oh, I didn't read your description well enough and missed the reboot part 🤦‍♂️

That obviously makes this a lot more complicated. Not sure if it is worth the effort!

@thomaseizinger
Copy link
Member

@thomaseizinger I asked ChatGPT if we can disable IPv6 without rebooting (I assume rebooting the CI runners is a non-starter) and it suggested this command netsh interface ipv6 set interface "Ethernet" disable

Does that make sense? We are talking about IPv6 on our tunnel interface here, right? That one won't exist before we create it and we try to set the MTU when we create it.

I think this bug happens because the entire IPv6 stack is disabled, right? So disabling IPv6 on another interface won't help. If we can't do that without rebooting, then I think we might be out of luck here in terms of a regression test.

@ReactorScram
Copy link
Contributor Author

You're right, disabling IPv6 on another interface doesn't do anything. I'll have to skip over the regression test for now.

@ReactorScram ReactorScram marked this pull request as ready for review September 13, 2024 17:15
@ReactorScram ReactorScram added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 13, 2024
@ReactorScram ReactorScram added this pull request to the merge queue Sep 13, 2024
Merged via the queue into main with commit 54b6222 Sep 13, 2024
@ReactorScram ReactorScram deleted the fix/windows-ipv4-only branch September 13, 2024 17:56
github-merge-queue bot pushed a commit that referenced this pull request Sep 13, 2024
Bumps the GUI client to 1.3.3 to publish #6681
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.

2 participants