Repository navigation
feat(connlib): persistent DNS resource records across sessions - #10104
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces persistent DNS resource records to prevent IP reassignment issues when Firezone restarts or users sign out and back in. When DNS queries are made for DNS resources, Firezone assigns IPs from the CG-NAT range, but previously this state was lost on restart, causing problems for applications that don't re-query DNS frequently.
Key changes:
- Added
DnsRecordsChangedevent to notify host applications when new DNS records are assigned - Implemented file-based cache for the headless client to persist DNS records across restarts
- Added
RestartClienttransition to proptests to verify packet routing works after client restart with re-seeded DNS records
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/headless-client/src/main.rs | Implements file-based DNS record caching with JSON serialization |
| rust/gui-client/src-tauri/src/service.rs | Adds TODO placeholders for DNS record persistence |
| rust/connlib/tunnel/src/dns.rs | Core DNS resolver logic with record tracking and event emission |
| rust/connlib/tunnel/src/client.rs | Updates ClientState to accept initial DNS records and poll DNS events |
| rust/connlib/tunnel/src/lib.rs | Adds DnsResourceRecord type and DnsRecordsChanged event |
| rust/client-shared/src/lib.rs | Updates Session::connect to accept DNS records parameter |
| rust/client-shared/src/serde_dns_resource_records.rs | Serialization utilities for DNS record persistence |
| rust/connlib/tunnel/src/tests/ | Test infrastructure updates for restart scenario validation |
| // Advance IP provider to make sure future addresses are unique. | ||
| let _ = ip_provider.get_n_ipv4(num_ip4_records); |
There was a problem hiding this comment.
Using let _ = to ignore the return value makes the intent unclear. Consider adding a comment explaining why the returned IPs are discarded, or use a more explicit method name like advance_by() if this is just advancing the provider state.
| // Advance IP provider to make sure future addresses are unique. | |
| let _ = ip_provider.get_n_ipv4(num_ip4_records); | |
| // Advance IP provider to make sure future addresses are unique. | |
| // The returned IPs are intentionally discarded; we only want to advance the provider's state. | |
| let _ = ip_provider.get_n_ipv4(num_ip4_records); | |
| // The returned IPs are intentionally discarded; we only want to advance the provider's state. |
f059165 to
481aff5
Compare
|
Oh, I thought this mapping is just an implementation detail to the stub resolver and the upper layers wouldn't be concerned with it. Curious why this approach was used - aren't we essentially going to have to maintain this cache across three languages now? I think some of the decisions on Apple for example to have different implementations for the favorite resource IDs and enabled / disabled resources could probably be simplified to one implementation in Rust since we don't really need anything but file read/write for that. Obviously not in this PR. But the overall theme would be to reduce complexity in the host app and maintain it in one, tested place. Wdyt? |
|
Where do you want me to maintain the cache then? I definitely want to keep the core of connlib side-effect free otherwise we get into all sorts of trouble with testing and understanding the code. |
Is it actually going to be one implementation though? Or just more complexity pushed down to connlib? Even if it is all file-based, I think iOS and Android are gonna throw a wrench in the works again because they'll need special handling. |
So at the very least, it would have to be in connlib's IO layer, next to the UDP sockets. We currently don't do any file IO there but I guess we could. Or maybe the event-loop would be a more natural way to tie it in. That is where all the other events are handled. |
|
Hmm maybe similar to file logger, where it's outside of connlib proper but still implemented once effectively? Zooming out a bit, maybe there is a dir we can pass for logs/settings/dns-cache and all of those just use the same interface to use from the host app. Just a thought since we'll have more horsepower to do things in Rust instead of Swift/Android |
|
It needs a bit of shuffling around in our crate structure but I think we can implement just one layer up from connlib's core. We just need to make sure that the "runtime" directory detection where we can put caches definitely works on all platforms. |
81bccfb to
bd1309d
Compare
bd1309d to
933a99a
Compare
10d0760 to
dd35421
Compare
jamilbk
left a comment
There was a problem hiding this comment.
Are we worried this DNS cache could grow to be very large for a client that has many DNS resources churn over its lifetime? The upper-bound is 2m entries for both IPv4 and IPv6, correct?
Yeah I am not very happy with this solution tbh. I think in practice, deterministic IPs should solve 90% of customer's cases? They'd come without this state baggage that we now need to worry about. How many cases are there where actually need to resolve an IP after a restart? |
No need to speculate here. The reason this ticket was opened was due to a customer's end users hitting exactly this, leading to a few wasted hours digging through logs and such. This happens more than you might think - any time a user disconnects/reconnects Firezone (such as when debugging a suspected Firezone issue) and they have some kind of "setup" going - like SSH in a bunch of terminals and such. The scenario this ticket was opened for was a VScode setup that involved a Mongo client. If this issue turns out not to be the cause for https://firezonehq.slack.com/archives/C06L41XN05T/p1755803516599929 then we can think more about the proper solution here. Would hate to have to revisit this later on. |
|
Two approaches we could use on session start to prune this cache:
I feel your sentiment about state baggage here. I think it's what we signed up for unfortunately building essentially a DNS resolver as part of our Split DNS architecture. Related: https://firezonehq.slack.com/archives/C090QGKJ45D/p1753731484870329?thread_ts=1753721737.313819&cid=C090QGKJ45D |
That is not a good idea I think otherwise we might break determinism as a result of portal config changes. |
dd35421 to
74ca90d
Compare
74ca90d to
7633ab6
Compare
7633ab6 to
4ac24c2
Compare
jamilbk
left a comment
There was a problem hiding this comment.
Looks pretty clean. Just a reminder to update the changelog.
831c192 to
162841c
Compare
162841c to
37112f5
Compare
54c3881 to
69896f3
Compare
In #10040, we decided to persist a peer's routing state on the Gateway across ICE sessions. This routing state also includes the DNS resource NAT. Prior to #10104 (which is not released yet), when a Client signs out and back in, it resets the proxy IP mapping for DNS resources and will start numbering them again from the front, i.e. starting from 100.96.0.1. With the state still being preserved on the Gateway, this represents a problem: We keep existing mappings around if there is still a NAT session for this proxy IP. However, if the proxy IP is actually for a different domain, this NAT session is meaningless. In fact, not replacing the IP is problematic as we will now route packets for the new proxy IP to the wrong destination. The persistent DNS resource mapping from #10104 fixes this. In this PR, we add an additional check to the Gateway where we detect whether the Client has started to re-assign proxy IPs and if so, we completely reset the DNS resource NAT state including all existing NAT sessions. Fixes #10268
When we receive a DNS query for a DNS resource in Firezone, we take the next available 4 IPs from the CG-NAT range and assign them to the domain name. For example, if
example.comis a DNS resource and it is the first resource being queried in a Firezone session, we will assigned the IPs100.96.0.1-100.96.0.4to it. If the user now restarts Firezone or signs out and back in, this state is lost and we assign those same IPs to the next DNS query coming in.This creates a problem for applications that do not re-query DNS very often or never. They expect these IPs to not change. Restarting software or signing out and back in is a common approach to fixing software problems, yet in this specific case, doing so may create even more problems for the user.
To mitigate this,
ClientStateintroduce a new eventDnsRecordsChangedthat gets emitted to the event-loop every time we assign new records. The event-loop then caches this in memory and reuses it in case a new session is initiated. The records are only stored in-memory and not on disk. Most likely, the tunnel process will be alive for the entire OS session.To verify this behaviour, we add a new
RestartClienttransition to our proptests. In the proptests, we already keep a mapping of all DNS names we ever resolved, including DNS resources. When generating IP traffic, we sample from this list of IPs and then expect the packet to be routed. By replacing theClientStateas part of this transition and re-seeding it with the previously exported DNS records, we can verify that packets to IPs resolved from a previous session still get successfully routed to the resource.Resolves: #5498