fix(connlib): only update the interface when setting dns if the effective dns changed #4327
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
There was a problem hiding this comment.
If the unit tests can prove that it does everything we need (Ignores sentinels, doesn't update if the upstream resolvers are set and stay the same, correctly updates when the upstream is enabled and disabled, etc.) then it's fine.
But for some reason I find it really hard to follow this part of the code, so I asked a lot of questions to make sure I'm understanding it.
| @@ -167,12 +167,11 @@ where | |||
|
|
|||
| /// Updates the system's dns | |||
| pub fn set_dns(&mut self, new_dns: Vec<IpAddr>) -> connlib_shared::Result<()> { | |||
| let dns_changed = self.role_state.update_system_resolvers(new_dns); | |||
|
|
|||
| self.role_state.update_system_resolvers(new_dns); | |||
There was a problem hiding this comment.
I don't know this code well so I'll just write what I think is happening, like I did for the macOS network PR
So this function has changed to always set the DNS, the logic is moving up from RoleState to whatever this struct is. (ClientTunnel?)
There was a problem hiding this comment.
well, the function called just below this, which is still in role_state checks for changes.
| let dns_changed = self.role_state.update_system_resolvers(new_dns); | ||
|
|
||
| self.role_state.update_system_resolvers(new_dns); | ||
| let dns_changed = self.update_dns_mapping(); |
There was a problem hiding this comment.
The change check moved over to this function, so far so good..
rust/connlib/tunnel/src/client.rs
Outdated
| let effective_dns_servers = effective_dns_servers( | ||
| config.upstream_dns.clone(), | ||
| self.role_state.system_resolvers.clone(), | ||
| ); |
There was a problem hiding this comment.
These are the current new effective DNS servers
This function is called after the DNS servers are updated in role_state? So the role state is out of sync until its owner calls this?
And this is where the filtering happens, in effective_dns_servers, and that also means we won't spuriously update anything if the system DNS changes while an upstream DNS is set.
There was a problem hiding this comment.
well, the system's resolvers are right before this in role_state but we haven't updated the dns mappings considering the newest one.
And this is where the filtering happens, in effective_dns_servers, and that also means we won't spuriously update anything if the system DNS changes while an upstream DNS is set.
yup!
|
@ReactorScram fixed a bug where the effective dns would contain sentinels if they were in the upstream and added the unit tests. I think those should cover everything |
conectado
force-pushed
the
fix/only-update-interface-if-dns-changed
branch
from
2101d9a
to
5334184
Compare
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com> Signed-off-by: Gabi <gabrielalejandro7@gmail.com>
Supersedes #4320, closes #4318
Updates the interface if effective dns have changed.
Fixes a bug where we could set upstream_dns to have sentinel dns
Adds corresponding unit tests.