Update profile parameter "resolver.defaults.retry"

[tgreenx]

Purpose

This PR updates resolver.defaults.retry profile parameter by making it more proper to its name. See "Changes" section below.

Context

Fixes #1070

Changes

• Make it exclude the initial query. Note that the actual value of resolver.defaults.retry is silently incremented before being passed to Zonemaster-LDNS. This is needed because LDNS does count the initial query as a try with this parameter.
• Update default values
• Update documentation
• Update unitary tests

How to test this PR

Unit tests should pass.

Also you can change the value of resolver.defaults.retry in share/profile.json and test manually with :

$ git diff
diff --git a/lib/Zonemaster/Engine/Nameserver.pm b/lib/Zonemaster/Engine/Nameserver.pm
index f5962f8d..71ae62cf 100644
--- a/lib/Zonemaster/Engine/Nameserver.pm
+++ b/lib/Zonemaster/Engine/Nameserver.pm
@@ -462,6 +462,8 @@ sub _query {
     $flags{q{timeout}}   = $href->{q{timeout}}   // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.timeout} );
     $flags{q{edns_size}} = $href->{q{edns_size}} // $UDP_EDNS_QUERY_DEFAULT;

+    say "query: ", $flags{q{retry}};
+
     if ( exists $href->{edns_details} ) {
         $flags{q{dnssec}}    = $href->{edns_details}{do} // $flags{q{dnssec}};
         $flags{q{edns_size}} = $href->{edns_details}{size} // $flags{q{edns_size}};


$ perl -MZonemaster::Engine -E 'say "default: ", Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.retry} ); my
 $ns = Zonemaster::Engine->ns( "ns2.nic.fr", "192.93.0.4" ); say "ns: ", $ns->dns->retry; my $p = $ns->query( "zonemaster.net", "A", { retry => 5 } ); say "final: ", $ns
->dns->retry;'
default: 1
ns: 2
query: 6
final: 2

[mattias-p]

This is a breaking change. In previous versions you could set retry=1 to disable retrying, but with this change such installations will in fact start retrying unless the configuration is updated.

[matsduf]

I am not against the change, but I think its value is marginal since the parameter is probably almost never changed. Unless there is another change that triggers a V-Major I do not think we should do it. I do not really see the gain.

[marc-vanderwal]

It is indeed a breaking change, but I welcome it for the following reasons:

1. it was necessary to fix a problem emerging from a poorly-worded and misleading variable name. As a design flaw, the longer we wait to fix it, the harder it becomes to carry out the change, so we might as well do it now;
2. it will only affect a minority of existing installations whose administrators tweaked resolvers.defaults.retry;
3. it’s a low-risk change: the worst that can happen to an administrator who forgets to read the upcoming version’s release notes is that their Zonemaster installation retries DNS queries once too many with respect to what the administrator wanted. It might cause some slightly increased network or server load in degraded conditions, but it won’t cause name servers to be falsely diagnosed as unresponsive.

[matsduf]

Another option would be to add resolvers.defaults.tries defined as resolvers.defaults.retry is defined today, and then deprecate resolvers.defaults.retry and remove it after a year or so when we have another breaking change.

[marc-vanderwal]

Thing is, the more I look at it, the more I doubt about whose responsibility it is to fix the underlying issue. I see that the value of resolver.defaults.retry is directly fed to ldns_resolver_set_retry() in LDNS, so this means that they, too, have made a poor naming choice. And if they decide to remedy it, it’s going to be a breaking change for us.

Such a change needs more thought.

[mattias-p]

Thing is, the more I look at it, the more I doubt about whose responsibility it is to fix the underlying issue. I see that the value of resolver.defaults.retry is directly fed to ldns_resolver_set_retry() in LDNS, so this means that they, too, have made a poor naming choice. And if they decide to remedy it, it’s going to be a breaking change for us.

I agree that this is a problem we inherited from LDNS. Redefining the semantics of ldns_resolver_set_retry() in LDNS would be a breaking change and callers would have to perform version number checks to know which semantics they'd be getting. Maybe we should raise this problem in their issue tracker. A more reasonable fix in LDNS would be to introduce a new method (with semantics that are in line with its name). Or maybe they'll just leave it as is.

Right now some of our profile properties mirror interfaces provided by LDNS. But we are the ones responsible for interfaces we export. If LDNS would change its semantics, it is our responsibility to adapt so that our interfaces remain stable.

We all seem to agree that our current resolver.defaults.retry property is problematic. Two solutions have been proposed: 1) changing the semantics of resolver.defaults.retry, and 2) introducing resolver.defaults.tries, deprecating resolver.defaults.retry and adding a validation to make sure they're not both present at the same time.

With the first solution we export an interface that looks like it mirrors LDNS but really doesn't when you look closer. This would be a (small) source of confusion. And as @marc-vanderwal pointed out we risk changing the behavior of a few existing installations.

With the second solution there is a bit more work to be done for us, and the few users who actually use this property would have to update their configs before we remove the old property.

Personally I'm leaning more towards the second solution.