Only switch to TCP for a DNS query if response was truncated (configurable with an option)

[peterthejohnston]

Currently, if queries to name servers over UDP result in either a
truncated response, or any kind of error, the query will be retried over
TCP. It should be possible to only use TCP for the case of oversized
messages, and avoid it in the common case. This change makes this the
default behavior and adds an option to ResolverOpts that can be enabled
to get the current behavior.

Fixes #1561

[codecov]

Codecov Report

Merging #1562 (82ba817) into main (cb73fd9) will decrease coverage by 0.14%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##             main    #1562      +/-   ##
==========================================
- Coverage   83.36%   83.22%   -0.14%     
==========================================
  Files         172      172              
  Lines       17075    17088      +13     
==========================================
- Hits        14233    14220      -13     
- Misses       2842     2868      +26     

[bluejekyll]

Overall, this looks really good. I wonder if we should make this be the default logic? There have been past complaints about slow timeouts/failures, and this is probably the reason.

See one comment I left.

[bluejekyll]

I wonder if we should change this to be a positive check for is truncated vs. a not truncated. Looking at this, I'm actually really confused why I wrote it this way.

What d you think about having the default be Ok(response) => { return Ok(response); } and then make the truncated check be Ok(response) if response.truncated() => { debug!(...); Ok(response }.

[peterthejohnston]

Yeah, I think that's much easier to follow. I've combined the default arms for both Ok and Err variants, so there are just three match arms:

• Ok && truncated → retry over TCP
• Err && try_tcp_on_error → retry over TCP
• otherwise → go with the UDP result

[djc]

Yeah, I think this makes sense to be the default (and maybe even skip the option for now?).

[bluejekyll]

and maybe even skip the option for now?

I think I'm ok with that... though there could be reasons that UDP is blocked on a network and TCP is not.

[peterthejohnston]

and maybe even skip the option for now?

I think I'm ok with that... though there could be reasons that UDP is blocked on a network and TCP is not.

Yeah, if we remove fallback to TCP on errors that also means that if a resolver is configured with only TCP name server(s), it won't use those to query, will always see a "No connections available" error, which seems undesirable. Maybe we should leave it in as an option, but default to go UDP unless we get a truncated response?

[bluejekyll]

This looks good to me. Do you mind if I update the branch to main? or do you want to do that?

[peterthejohnston]

Do you mind if I update the branch to main? or do you want to do that?

Just updated it, thanks!