fix(apple): save networkSettings var - #10022
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where the networkSettings variable was not being saved to the adapter instance, causing iOS-specific system resolver calls to return connlib sentinels instead of actual system DNS resolvers. The fix ensures proper DNS resolution functionality on iOS devices.
Key changes:
- Save
networkSettingsto the adapter instance after configuration - Move path monitoring initialization to occur after network settings are applied
- Consolidate DNS resolver fetching logic into a single method that handles the complete flow
| if lastRelevantPath?.connectivityDifferentFrom(path: path) != false { | ||
| lastRelevantPath = path | ||
|
|
||
| if lastPath?.connectivityDifferentFrom(path: path) != false { |
There was a problem hiding this comment.
[nitpick] The double negative logic != false is unclear and harder to read. Consider using == true or restructuring the condition for better readability.
| if lastPath?.connectivityDifferentFrom(path: path) != false { | |
| if lastPath?.connectivityDifferentFrom(path: path) == true { |
| let networkSettings = | ||
| self.networkSettings | ||
| ?? NetworkSettings(packetTunnelProvider: packetTunnelProvider) | ||
| networkSettings ?? NetworkSettings(packetTunnelProvider: packetTunnelProvider) |
There was a problem hiding this comment.
[nitpick] The variable networkSettings is being accessed without self. prefix, which could be confusing since there's also a local variable assignment on line 355. Consider using self.networkSettings for clarity.
| networkSettings ?? NetworkSettings(packetTunnelProvider: packetTunnelProvider) | |
| self.networkSettings ?? NetworkSettings(packetTunnelProvider: packetTunnelProvider) |
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Nice catch. It may be useful to add a few debug log lines here. For one, I find natural language in those easier to read along as they are often a good replacement for comments. Two, looking at those logs will then make it easier to debug in case we ever want to know what is happening.
|
Yeah let me do one more quick pass over it to add more logging, comments, and some testing. |
| for stringAddress in resolvers { | ||
| if let ipv4Address = IPv4Address(stringAddress) { | ||
| parsedResolvers.append("\(ipv4Address)") | ||
| if ipv4Address.isWithinSentinelRange() { |
There was a problem hiding this comment.
Connlib filters these out for you already so you wouldn't need to do that :)
There was a problem hiding this comment.
Are you saying I should move the warning log to connlib instead?
There was a problem hiding this comment.
It would be helpful to know that we are (mistakenly) trying to set the sentinels as resolvers. Note this is really only a problem on iOS AFAIK.
There was a problem hiding this comment.
I didn't see that there is a warning log, yeah I guess that makes sense then.
We can add a debug log to connlib that we are filtering a sentinel DNS and we already have one that we don't have any DNS servers.
What we could react to instead is receiving an empty list of DNS resolvers to set from connlib? Really that should never happen.
There was a problem hiding this comment.
I don't want to add the warning log to connlib because we may have intermitten states where we don't have a resolver so there would be false-positives.
All of this is going to get a lot easier once we do #8263 because then there will only be one resolver IP and that is simply always set. Then, connlib definitely knows if it is in a state where it doesn't have any servers to forward the queries to.
| if lastRelevantPath?.connectivityDifferentFrom(path: path) != false { | ||
| lastRelevantPath = path | ||
|
|
||
| if lastPath?.connectivityDifferentFrom(path: path) != false { |
There was a problem hiding this comment.
Thinking about it now, does it make sense to update the path but not react to it? Wouldn't it better to store the last path we performed a reset on and always compare connectivity to that?
There was a problem hiding this comment.
It's logically the same either way. This way there's one less thing to update inside the conditional and it's a bit less cognitive overhead IMO.
In 45466e3, the
networkSettingsvariable was no longer saved on theadapterinstance, causing all calls of the iOS-specific version of getting system resolvers to return the connlib sentinels after the tunnel first came up.This PR fixes that logic bug and also cleans this area of the codebase up just a tiny bit so it's easier to follow.
Lastly, we also fix a bug where if the tunnel came up while Firezone was already running,
networkSettingswould benil, and we would read the default system resolvers, which were the connlib sentinels.Fixes #10017