Skip to content

Conversation

@thomaseizinger
Copy link
Member

As part of fixing #6777, we added a code path to explicitly set nameservers on the tunnel interface. This was necessary to make DNS resources work under WSL. Exactly this code also seems to be causing issues where this particular registry key is not available on a users machine.

DNS resources outside of WSL will also work without this, therefore we make this part optional to at least have something working on the users machine.

Related: #7962.

@vercel
Copy link

vercel bot commented Jan 31, 2025

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 Feb 3, 2025 5:19am

@thomaseizinger
Copy link
Member Author

I am a bit hesitant on this "fix" because I'd like to first understand, why the registry key doesn't exist but the adapter is supposedly up? That seems like a state we should never get ourselves into.

@thomaseizinger
Copy link
Member Author

thomaseizinger commented Jan 31, 2025

I tested this in my VM and regular DNS still works (as expected). Might be worth merging as a hotfix. I also tested with the code commented out and it also works, i.e. on machines where this is failing, not executing it should still give you access to DNS resources (at least in the browser etc).

@thomaseizinger
Copy link
Member Author

I also tested this with WSL and it seems to work so I am not even sure why we are setting these nameservers at all?

@thomaseizinger
Copy link
Member Author

I also tested this with WSL and it seems to work so I am not even sure why we are setting these nameservers at all?

To confirm: I tested with the code commented out! So something else must have changed because that would mean #6777 was never an issue?

@jamilbk
Copy link
Member

jamilbk commented Feb 1, 2025

I also tested this with WSL and it seems to work so I am not even sure why we are setting these nameservers at all?

To confirm: I tested with the code commented out! So something else must have changed because that would mean #6777 was never an issue?

Perhaps a recent Windows / WSL update changed the behavior here (or fixed it). I do remember in that issue specifically the patch fixed the issue for the particular customer(s).

Base automatically changed from fix/nrpt-rule-apply-failed to main February 1, 2025 21:29
@thomaseizinger thomaseizinger force-pushed the fix/apply-nameservers-optional branch from 81d259f to 789f1de Compare February 3, 2025 05:16
@thomaseizinger thomaseizinger marked this pull request as ready for review February 3, 2025 05:16
Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

LGTM

@thomaseizinger thomaseizinger added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit f2725df Feb 3, 2025
112 checks passed
@thomaseizinger thomaseizinger deleted the fix/apply-nameservers-optional branch February 3, 2025 06:03
github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2025
Adds changelog entries for #7972 and #8003. Bundled together to avoid
merge conflicts.

---------

Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
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