fix: NXDomain should continue to next server if available #7
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the logic around how NXDomain responses are handled by the DNS client so that the resolver continues to query other servers instead of immediately returning NXDomain. Additionally, the project version has been incremented to reflect the changes.
- Updated conditional check in DnsClientBase.cs to treat NXDomain similarly to ServerFailure.
- Bumped the project version in the csproj file.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ARSoft.Tools.Net/Dns/DnsClientBase.cs | Updated condition to check for NXDomain as well as ServerFailure. |
| ARSoft.Tools.Net/ARSoft.Tools.Net.csproj | Incremented version from 3.6.1 to 3.6.2. |
Comments suppressed due to low confidence (1)
ARSoft.Tools.Net/Dns/DnsClientBase.cs:160
- Consider adding a comment explaining why NXDomain is handled the same as ServerFailure to improve clarity for future maintainers.
if (receivedMessage.Message.ReturnCode is ReturnCode.ServerFailure or ReturnCode.NxDomain)
| { | ||
| connection.RestartIdleTimeout(receivedMessage.Message.GetEDnsKeepAliveTimeout()); | ||
|
|
||
| if (receivedMessage.Message.ReturnCode == ReturnCode.ServerFailure) |
There was a problem hiding this comment.
(question) I might be misunderstanding but if this is in the ARSoft library, it means that anything going over a DnsClient for ARSoft will be subjected to the same logic. If that's the case, would this also mean that this could affect our critical path of DNS resolution beyond local domain resolution only? We've had lots of issues with timeouts on the Zorus side so just making sure I'm not missing something.
There was a problem hiding this comment.
Correct. But the expected behavior at least from our clients is that we try all the DNS servers they have configured before deciding NXDomain is the only answer.
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
The current ARSoft code conisders NXDomain a valid response and does not try the remaining DNS resolvers. This is different than how the winRC currently works with the old library so this change makes the library continue to try all the servers and only return NXDomain if no other response is available