fix(rust/gui-client/windows): read DNS servers before starting connlib#6455
fix(rust/gui-client/windows): read DNS servers before starting connlib#6455ReactorScram merged 8 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
jamilbk
left a comment
jamilbk
left a comment
There was a problem hiding this comment.
Just a couple comment suggestions otherwise LGTM
| ) | ||
| .map_err(|e| Error::PortalConnection(e.to_string()))?; | ||
|
|
||
| let dns = self.dns_controller.system_resolvers(); |
There was a problem hiding this comment.
| let dns = self.dns_controller.system_resolvers(); | |
| // Reading system_resolvers immediately after / during initializing the tun interface can result in an empty read, so read them here before. | |
| let dns = self.dns_controller.system_resolvers(); |
There was a problem hiding this comment.
Actually, did we prove that? Did we see a log where we sent an empty resolver list to connlib?
There was a problem hiding this comment.
Actually, yeah maybe that's not the root bug. I think this is it:
- We call set_dns before the interface is configured
update_dns_mappingis effectively a no-op if the interface isn't configured, leaving DNS unset- We never call set_dns after the tun interface is configured
There was a problem hiding this comment.
The code kinda looks like calling set_dns first just buffers the resolvers for later. Also in the PR I call set_dns first and that appears to be fine? Although we didn't test ifconfig.net on that build.
Maybe we should have written down our hypotheses while we were testing
There was a problem hiding this comment.
Hm yeah. But if we never call set_dns after the tun interface is configured, they will be left unset, update_dns_mapping returns immediately and doesn't save them to its internal state.
See here.
There was a problem hiding this comment.
Nah, the caller of that function does save it. I don't have a stack trace but I assume it flows through here:
firezone/rust/connlib/tunnel/src/client.rs
Lines 864 to 868 in a0ee092
There was a problem hiding this comment.
Ah, kk. Hmm maybe setting them in this order then just triggers a side effect combined with the path monitoring that results in empty resolvers somehow.
Co-authored-by: Jamil <jamilbk@users.noreply.github.com> Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
jamilbk
changed the title
jamilbk
changed the title
2 participants
Closes #6453