on REFUSED response, fall back to other nameservers

[peterthejohnston]

In using trust-dns-resolver as the DNS resolver for Fuchsia, we noticed that there are some authoritative DNS name servers that respond with a REFUSED response when they don't know the domain (e.g., it wasn't on an allowlist of hosts it'd respond to queries about).

We have a mitigation for this to our current version of trust-dns-resolver (0.19.2): https://fuchsia-review.googlesource.com/c/fuchsia/+/545423/17/third_party/rust_crates/vendor/trust-dns-resolver/src/name_server/name_server.rs

I'd like to contribute a similar fix here, if you think it makes sense. The intention of this patch is essentially to add REFUSED to the list of "retryable" errors—errors that should not lead to a terminal query failure. I looked into the precedent for this and found this issue where SERVFAIL being a terminal error led to failed queries where the resolver should have continued on to other name servers. I also saw this TODO which suggests it might be appropriate to consider continuing a query after a REFUSED response.

I have a couple of questions:

• Do you have any thoughts about this approach?
• If you think it makes sense to move forward with this PR, where can I add tests for this? Perhaps in name_server_pool_tests?

[bluejekyll]

LGTM, I think it makes sense for us to apply this change. Thanks for upstreaming the patch!

[bluejekyll]

Do you have any thoughts about this approach?

Yes, I think this is reasonable. (We may in fact want to become even more lax about this)

If you think it makes sense to move forward with this PR, where can I add tests for this? Perhaps in name_server_pool_tests?

Yes, I think it would be great to add a mocked test there, as we won't want to accidentally remove this in the future.

[codecov]

Codecov Report

Merging #1513 (cde3b31) into main (be0324a) will increase coverage by 0.00%.
The diff coverage is 81.82%.

@@           Coverage Diff           @@
##             main    #1513   +/-   ##
=======================================
  Coverage   84.43%   84.43%           
=======================================
  Files         171      171           
  Lines       16888    16876   -12     
=======================================
- Hits        14259    14249   -10     
+ Misses       2629     2627    -2     

[bluejekyll]

@peterthejohnston, do you think you'll have time to add the testcase you mentioned?

[peterthejohnston]

Definitely, sorry for the delay—I got caught up in other things but will be back at work Tuesday 6/29 and will update this PR then if that works.

[bluejekyll]

No pressure at all, I just wanted to make sure we were both on the same page. Thanks again for these changes.

[peterthejohnston]

I'd like to make this match expression exhaustive (i.e. avoid the wildcard _ match) so it's easier to tell which response codes don't lead to a ResolveError. It does require a little refactoring, though—moving the ifs inside the NXDomain and NoError match arms—so let me know what you think.

Edit: I've also added an integration test.

[bluejekyll]

These changes all look good to me. Thanks for adding the test and cleaning up the match cases.

[bluejekyll]

Sorry for the delay on the review. These changes all look good, assuming tests pass, I'll merge in.

[bluejekyll]

The beta test failure appears to be unrelated to this change. Merging.

Thanks for this PR!