Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(headless-client/linux)!: make FIREZONE_DNS_CONTROL mandatory #5068

Closed
wants to merge 78 commits into from

Conversation

ReactorScram
Copy link
Collaborator

@ReactorScram ReactorScram commented May 21, 2024

Closes #5063

  • On Windows it's not mandatory because there is only one DNS control method and it seems to work fine
  • For Gateways it must not be used because Gateways don't do DNS control
  • The IPC service always uses systemd-resolved on Linux

Blocked on failing integration tests, because the Gateway thinks it needs FIREZONE_DNS_CONTROL, which is being yak shaved in #5075

Almost all users are likely to have DNS resources, and it will be confusing if we silently don't allow those resources. Defaulting to etc-resolv-conf will break some systems, and defaulting to systemd-resolved means that it becomes part of the CLI contract. For now, make it explicit

BREAKING CHANGE: The Headless Client will now bail out on Linux if FIREZONE_DNS_CONTROL is not set to etc-resolv-conf or systemd-resolved

Tasks

Edit tasklist title
Beta Give feedback Tasklist Tasks, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Remove FIREZONE_DNS_CONTROL from systemd service unit if it's redundant
    Options
  2. Smoke test Linux
    Options
  3. Smoke test Windows
    Options

Closes #5063

Almost all users are likely to have DNS resources, and it will be confusing
if we silently don't allow those resources. Defaulting to `etc-resolv-conf`
will break some systems, and defaulting to `systemd-resolved` means that
it becomes part of the CLI contract. For now, make it explicit
@ReactorScram ReactorScram self-assigned this May 21, 2024
Copy link

vercel bot commented May 21, 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 May 31, 2024 4:12pm

@github-actions github-actions bot added the kind/feature New feature or request label May 21, 2024
Copy link

github-actions bot commented May 21, 2024

Terraform Cloud Plan Output

Plan: 15 to add, 15 to change, 15 to destroy.

Terraform Cloud Plan

@ReactorScram ReactorScram changed the title feat(headless-client): make FIREZONE_DNS_CONTROL mandatory feat(headless-client)!: make FIREZONE_DNS_CONTROL mandatory May 21, 2024
@ReactorScram
Copy link
Collaborator Author

The Gateway also uses code that runs through there, so this is blocked until I can remove that DNS control code from the Gateway's path https://github.com/firezone/firezone/actions/runs/9180192206/job/25244235294#step:10:30

I'm thinking if we did #2986, then the Linux Headless Client can pass the DNS control method to connlib there, other platforms (Windows) will keep their default DNS control, and the Gateway will not do any DNS control. But it's a sorta big change to connlib's API.

@ReactorScram ReactorScram changed the base branch from main to refactor/move-linux-dns-control May 29, 2024 17:59
@ReactorScram ReactorScram changed the title feat(headless-client)!: make FIREZONE_DNS_CONTROL mandatory feat(headless-client/linux)!: make FIREZONE_DNS_CONTROL mandatory May 29, 2024
Copy link

github-actions bot commented May 29, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 239.4 MiB (+0%) 240.4 MiB (+0%) 405 (+21%)
direct-tcp-server2client 242.9 MiB (+3%) 244.2 MiB (+3%) 480 (+4%)
relayed-tcp-client2server 223.7 MiB (-2%) 224.3 MiB (-2%) 237 (-13%)
relayed-tcp-server2client 199.4 MiB (-16%) 195.9 MiB (-18%) 270 (-25%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 499.8 MiB (-0%) 0.15ms (+306%) 44.39% (-0%)
direct-udp-server2client 500.0 MiB (-0%) 0.02ms (-44%) 22.94% (+7%)
relayed-udp-client2server 500.0 MiB (+0%) 0.03ms (+6%) 57.91% (+9%)
relayed-udp-server2client 500.0 MiB (+0%) 0.02ms (-90%) 43.16% (+1%)

github-merge-queue bot pushed a commit that referenced this pull request Jun 3, 2024
… and up to the Client (#5111)

Refs #3636 (This pays down some of the technical debt from Linux DNS)
Refs #4473 (This partially fulfills it)
Refs #5068 (This is needed to make `FIREZONE_DNS_CONTROL` mandatory)

As of dd6421:

- On both Linux and Windows, DNS control and IP setting (i.e.
`on_set_interface_config`) both move to the Client
- On Windows, route setting stays in `tun_windows.rs`. Route setting in
Windows requires us to know the interface index, which we don't know in
the Client code. If we could pass opaque platform-specific data between
the tunnel and the Client it would be easy.
- On Linux, route setting moves to the Client and Gateway, which
completely removes the `worker` task in `tun_linux.rs`
- Notifying systemd that we're ready moves up to the headless Client /
IPC service

```[tasklist]
### Before merging / notes
- [x] Does DNS roaming work on Linux on `main`? I don't see where it hooks up. I think I only set up DNS in `Tun::new` (Yes, the `Tun` gets recreated every time we reconfigure the device)
- [x] Fix Windows Clients
- [x] Fix Gateway
- [x] Make sure connlib doesn't get the DNS control method from the env var (will be fixed in #5068)
- [x] De-dupe consts
- [ ] ~~Add DNS control test~~ (failed)
- [ ] Smoke test Linux
- [ ] Smoke test Windows
```
Base automatically changed from refactor/move-linux-dns-control to main June 3, 2024 14:53
@ReactorScram
Copy link
Collaborator Author

Out of date, breaks the CLI API, closing

Just make systemd-resolved the default, it's fine. It'll be around for a while anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ux(headless-client/linux): don't default to null DNS control
1 participant