Make --retries global to a name and try with other name servers in a given layer (#451)

* re-work for retries

* added network timeout

* add comments

* consolidated cachedRetryingLookup and retryingLookup into single fn, remove randomRootServer fn

* better iterateOnAuth handling

* handle status error in iterateOnAuths

* remove unused status

* update README

* update retries in cli

* properly handle the NetworkTimeout

* PR cleanup

* fix defaults for network timeout

* fix todo in comment

* reset nonqueried map if all nameservers are queried

* fix merge issues

* comment theorized root cause of issue

* resolve compile issues and sanity check the retry logic

* more global retry handling

* update default timeouts and retries in cli.go

---------

Co-authored-by: Zakir Durumeric <[email protected]>

Author: phillip-stephens

README.md

@@ -291,7 +291,10 @@ routines. This architecture has several caveats:
 
   * `--timeout` The maximum amount of time ZDNS will spend on a single name
   * `--iteration-timeout` The maximum amount of time ZDNS will spend on a single iteration step (ex: resolving google.com at the .com layer)
-  * `--retries` The number of retries ZDNS will make against a single nameserver before giving up for that name
+  * `--network-timeout` The maximum amount of time ZDNS will wait for a response from a nameserver
+  * `--retries=N` If a connection to a specific nameserver fails in `--iterative`, ZDNS will retry with another un-queried name server at that layer.
+  Retries are per-name, so if `--retries=1` then ZDNS will retry a name against a new nameserver once during it's full iteration process. If all nameservers have been queried
+  then a random nameserver will be chosen.
   * `--name-servers` The list of nameservers to use for lookups, mostly useful with `--iterative=false`
 
 

src/cli/cli.go

@@ -44,16 +44,17 @@ type GeneralOptions struct {
 	LookupAllNameServers bool   `long:"all-nameservers" description:"Perform the lookup via all the nameservers for the domain."`
 	CacheSize            int    `long:"cache-size" default:"10000" description:"how many items can be stored in internal recursive cache"`
 	GoMaxProcs           int    `long:"go-processes" default:"0" description:"number of OS processes (GOMAXPROCS by default)"`
-	IterationTimeout     int    `long:"iteration-timeout" default:"4" description:"timeout for a single iterative step in an iterative query, in seconds. Only applicable with --iterative"`
+	IterationTimeout     int    `long:"iteration-timeout" default:"8" description:"timeout for a single iterative step in an iterative query, in seconds. Only applicable with --iterative"`
 	IterativeResolution  bool   `long:"iterative" description:"Perform own iteration instead of relying on recursive resolver"`
 	MaxDepth             int    `long:"max-depth" default:"10" description:"how deep should we recurse when performing iterative lookups"`
 	NameServerMode       bool   `long:"name-server-mode" description:"Treats input as nameservers to query with a static query rather than queries to send to a static name server"`
 	NameServersString    string `long:"name-servers" description:"List of DNS servers to use. Can be passed as comma-delimited string or via @/path/to/file. If no port is specified, defaults to 53."`
 	UseNanoseconds       bool   `long:"nanoseconds" description:"Use nanosecond resolution timestamps in output"`
+	NetworkTimeout       int    `long:"network-timeout" default:"2" description:"timeout for round trip network operations, in seconds"`
 	DisableFollowCNAMEs  bool   `long:"no-follow-cnames" description:"do not follow CNAMEs/DNAMEs in the lookup process"`
-	Retries              int    `long:"retries" default:"1" description:"how many times should zdns retry query if timeout or temporary failure"`
+	Retries              int    `long:"retries" default:"3" description:"how many times should zdns retry query against a new nameserver if timeout or temporary failure"`
 	Threads              int    `short:"t" long:"threads" default:"100" description:"number of lightweight go threads"`
-	Timeout              int    `long:"timeout" default:"15" description:"timeout for resolving a individual name, in seconds"`
+	Timeout              int    `long:"timeout" default:"20" description:"timeout for resolving a individual name, in seconds"`
 	Version              bool   `long:"version" short:"v" description:"Print the version of zdns and exit"`
 }
 

src/cli/worker_manager.go

@@ -191,6 +191,7 @@ func populateResolverConfig(gc *CLIConf) *zdns.ResolverConfig {
 	}
 
 	config.Timeout = time.Second * time.Duration(gc.Timeout)
+	config.NetworkTimeout = time.Second * time.Duration(gc.NetworkTimeout)
 	config.IterativeTimeout = time.Second * time.Duration(gc.IterationTimeout)
 	config.LookupAllNameServers = gc.LookupAllNameServers
 	config.FollowCNAMEs = !gc.DisableFollowCNAMEs // ZFlags only allows default-false bool flags. We'll invert here.

src/modules/bindversion/bindversion_test.go

@@ -35,8 +35,8 @@ var queries []QueryRecord
 // DoSingleDstServerLookup(r *Resolver, q Question, nameServer string, isIterative bool) (*SingleQueryResult, Trace, Status, error)
 type MockLookup struct{}
 
-func (ml MockLookup) DoSingleDstServerLookup(r *zdns.Resolver, question zdns.Question, nameServer *zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
-	queries = append(queries, QueryRecord{q: question, NameServer: nameServer})
+func (ml MockLookup) DoDstServersLookup(r *zdns.Resolver, question zdns.Question, nameServers []zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
+	queries = append(queries, QueryRecord{q: question, NameServer: &nameServers[0]})
 	if res, ok := mockResults[question.Name]; ok {
 		return res, nil, zdns.StatusNoError, nil
 	} else {

src/modules/dmarc/dmarc_test.go

@@ -35,8 +35,8 @@ var queries []QueryRecord
 
 type MockLookup struct{}
 
-func (ml MockLookup) DoSingleDstServerLookup(r *zdns.Resolver, question zdns.Question, nameServer *zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
-	queries = append(queries, QueryRecord{question, nameServer})
+func (ml MockLookup) DoDstServersLookup(r *zdns.Resolver, question zdns.Question, nameServers []zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
+	queries = append(queries, QueryRecord{question, &nameServers[0]})
 	if res, ok := mockResults[question.Name]; ok {
 		return res, nil, zdns.StatusNoError, nil
 	} else {

src/modules/spf/spf_test.go

@@ -35,8 +35,8 @@ var queries []QueryRecord
 
 type MockLookup struct{}
 
-func (ml MockLookup) DoSingleDstServerLookup(r *zdns.Resolver, question zdns.Question, nameServer *zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
-	queries = append(queries, QueryRecord{question, nameServer})
+func (ml MockLookup) DoDstServersLookup(r *zdns.Resolver, question zdns.Question, nameServers []zdns.NameServer, isIterative bool) (*zdns.SingleQueryResult, zdns.Trace, zdns.Status, error) {
+	queries = append(queries, QueryRecord{question, &nameServers[0]})
 	if res, ok := mockResults[question.Name]; ok {
 		return res, nil, zdns.StatusNoError, nil
 	} else {