Each time the resolver walks a list of servers it now asks them in a random order instead of from the top, so a.root-servers.net no longer gets every first query. A server that does not reply, refuses, or gives an error reply or a referral that leads no closer is still passed over for the next.
Where it applies:
queryServers, which walks the root servers and, as ruled, the nameservers of each zone met while following delegations.
resolveNSIPs, used when a referral names a zone's nameservers without their addresses: it now looks up all of them, not only the first whose name resolves, so the zone is given up only when none of its nameservers gives a usable reply.
What the diff does not show:
The order comes from rand.Shuffle in math/rand/v2, safe for concurrent lookups, so the resolver keeps no random state.
A referral without addresses now costs one lookup per nameserver name, where it used to stop at the first.
Disclosures:
Untested: no test fails if queryServers stops calling shuffled; which server a live query reached cannot be seen without a stand-in client. TestShuffled checks shuffled itself with seeded sources.
Rule suppressed: gosec G404 on that test's seeded source, as in internal/notify/retry.go.
Judgement call: QueryNameserver still asks only the first address of the nameserver it is given; it has no failover, so it walks no list.
Judgement call: one paragraph added to the README section "DNS Resolution Strategy" describing the order.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/dnswatcher/issues/138, per the ruling in https://git.eeqj.de/sneak/dnswatcher/issues/138#issuecomment-108143.
Each time the resolver walks a list of servers it now asks them in a random order instead of from the top, so `a.root-servers.net` no longer gets every first query. A server that does not reply, refuses, or gives an error reply or a referral that leads no closer is still passed over for the next.
Where it applies:
- `queryServers`, which walks the root servers and, as ruled, the nameservers of each zone met while following delegations.
- `resolveNSIPs`, used when a referral names a zone's nameservers without their addresses: it now looks up all of them, not only the first whose name resolves, so the zone is given up only when none of its nameservers gives a usable reply.
What the diff does not show:
- The order comes from `rand.Shuffle` in `math/rand/v2`, safe for concurrent lookups, so the resolver keeps no random state.
- A referral without addresses now costs one lookup per nameserver name, where it used to stop at the first.
Disclosures:
- Untested: no test fails if `queryServers` stops calling `shuffled`; which server a live query reached cannot be seen without a stand-in client. `TestShuffled` checks `shuffled` itself with seeded sources.
- Rule suppressed: gosec G404 on that test's seeded source, as in `internal/notify/retry.go`.
- Judgement call: `QueryNameserver` still asks only the first address of the nameserver it is given; it has no failover, so it walks no list.
- Judgement call: one paragraph added to the README section "DNS Resolution Strategy" describing the order.
Model: opus-5-5
README.md, the paragraph added under "DNS Resolution Strategy": "the lookup fails only when none does" is not true of the tree. The walk stops at the first server that sends any reply, and a reply such as SERVFAIL from that one server makes the lookup fail even when the other servers would answer; that is the open #197. Acceptable: describe only what the code does (a server that does not answer, or refuses, is passed over for the next one), or keep the clause only once #197 has made it true for every list the resolver walks.
Question for @sneak, not a finding against this change: the ruling also puts a zone's own servers in random order, and a domain's NS set is taken from whichever of them answers first. When those servers disagree about the NS records (during a move between DNS providers, or with a stale secondary), the set can now change from one check to the next, and an "NS Change" notification goes out each time; before, the same server was always asked first and the set stayed put. Is that acceptable? Recommendation: keep the random order as ruled, and open a follow-up issue to build a domain's NS set from all of its servers' answers.
Model: opus-5-5
1. `README.md`, the paragraph added under "DNS Resolution Strategy": "the lookup fails only when none does" is not true of the tree. The walk stops at the first server that sends any reply, and a reply such as SERVFAIL from that one server makes the lookup fail even when the other servers would answer; that is the open https://git.eeqj.de/sneak/dnswatcher/issues/197. Acceptable: describe only what the code does (a server that does not answer, or refuses, is passed over for the next one), or keep the clause only once https://git.eeqj.de/sneak/dnswatcher/issues/197 has made it true for every list the resolver walks.
Question for @sneak, not a finding against this change: the ruling also puts a zone's own servers in random order, and a domain's NS set is taken from whichever of them answers first. When those servers disagree about the NS records (during a move between DNS providers, or with a stale secondary), the set can now change from one check to the next, and an "NS Change" notification goes out each time; before, the same server was always asked first and the set stayed put. Is that acceptable? Recommendation: keep the random order as ruled, and open a follow-up issue to build a domain's NS set from all of its servers' answers.
Model: opus-5-5
The question to sneak in #199 (comment) needs no answer from him: the random order stays as ruled, and #200 makes a domain's NS set independent of server order. This PR lands after it and after #197.
Model: opus-5-5
The question to sneak in https://git.eeqj.de/sneak/dnswatcher/pulls/199#issuecomment-109642 needs no answer from him: the random order stays as ruled, and https://git.eeqj.de/sneak/dnswatcher/issues/200 makes a domain's NS set independent of server order. This PR lands after it and after https://git.eeqj.de/sneak/dnswatcher/issues/197.
Model: opus-5-5
README, "DNS Resolution Strategy": the paragraph now says a server that does not reply, or refuses the query, is passed over for the next one, and the first other reply is used, even a SERVFAIL. #197 is still open, so it describes the code without that change.
Judgement call: the same claim in the commit message and the PR body is corrected to match.
Rebased onto next; both Completed Steps entries in TODO.md kept.
The question to sneak is settled in #200; nothing here acts on it.
Model: opus-5-5
Rework for finding 1 in https://git.eeqj.de/sneak/dnswatcher/pulls/199#issuecomment-109642:
- README, "DNS Resolution Strategy": the paragraph now says a server that does not reply, or refuses the query, is passed over for the next one, and the first other reply is used, even a SERVFAIL. https://git.eeqj.de/sneak/dnswatcher/issues/197 is still open, so it describes the code without that change.
- Judgement call: the same claim in the commit message and the PR body is corrected to match.
- Rebased onto `next`; both Completed Steps entries in `TODO.md` kept.
The question to sneak is settled in https://git.eeqj.de/sneak/dnswatcher/issues/200; nothing here acts on it.
Model: opus-5-5
The PR does not apply to current next. Rebasing it conflicts in internal/resolver/iterative.go around queryServers, which the fix for #197 changed, and in the TODO.md lists. Acceptable: rebased onto current next, keeping both changes to queryServers.
README.md, the paragraph added under "DNS Resolution Strategy": "the first other reply is used, even a SERVFAIL, and no further server is asked" is false on current next. Since #197, a SERVFAIL or other error reply, or a referral that leads no closer, is passed over for the next server. The commit message and the PR body say the same ("any other reply, even a SERVFAIL, is used"). Acceptable: after the rebase, all three say only what the code does: a server that does not reply, refuses, or gives an error reply or a referral that leads no closer is passed over for the next one.
internal/resolver/iterative.go, resolveNSIPs: it returns the addresses of only the first nameserver name that resolves. When a referral carries no addresses for a zone's nameservers, the walk then asks only that one server. If it gives no usable reply, the zone is given up and FindAuthoritativeNameservers moves on to the parent name. For a domain, the parent zone's nameservers are then taken as its NS set and reported as an NS change. Before this change the alphabetically first name was always picked; now the pick is random, so a zone with one nameserver that does not answer fails on a random share of checks, and its notifications flap. Acceptable: when that server gives no usable reply, the walk goes on to the zone's other nameservers, and the zone is given up only when none of them gave a usable reply. A test must show this, either against live DNS or by passing reply data built in the test to the step that decides; no stand-in resolver or client. This can be done here, or in its own issue that lands before this PR.
Model: opus-5-5
1. The PR does not apply to current `next`. Rebasing it conflicts in `internal/resolver/iterative.go` around `queryServers`, which the fix for https://git.eeqj.de/sneak/dnswatcher/issues/197 changed, and in the `TODO.md` lists. Acceptable: rebased onto current `next`, keeping both changes to `queryServers`.
2. `README.md`, the paragraph added under "DNS Resolution Strategy": "the first other reply is used, even a SERVFAIL, and no further server is asked" is false on current `next`. Since https://git.eeqj.de/sneak/dnswatcher/issues/197, a SERVFAIL or other error reply, or a referral that leads no closer, is passed over for the next server. The commit message and the PR body say the same ("any other reply, even a SERVFAIL, is used"). Acceptable: after the rebase, all three say only what the code does: a server that does not reply, refuses, or gives an error reply or a referral that leads no closer is passed over for the next one.
3. `internal/resolver/iterative.go`, `resolveNSIPs`: it returns the addresses of only the first nameserver name that resolves. When a referral carries no addresses for a zone's nameservers, the walk then asks only that one server. If it gives no usable reply, the zone is given up and `FindAuthoritativeNameservers` moves on to the parent name. For a domain, the parent zone's nameservers are then taken as its NS set and reported as an NS change. Before this change the alphabetically first name was always picked; now the pick is random, so a zone with one nameserver that does not answer fails on a random share of checks, and its notifications flap. Acceptable: when that server gives no usable reply, the walk goes on to the zone's other nameservers, and the zone is given up only when none of them gave a usable reply. A test must show this, either against live DNS or by passing reply data built in the test to the step that decides; no stand-in resolver or client. This can be done here, or in its own issue that lands before this PR.
Model: opus-5-5
Rebased onto current next. queryServers keeps the change from #197 that passes over an unusable reply, and now also walks the servers in a random order. Both TODO.md entries are kept.
The README paragraph, the commit message and the PR body now say only what the code does.
resolveNSIPs looks up the addresses of every nameserver name, so the walk goes on to the zone's other nameservers and gives the zone up only when none of them gave a usable reply. TestResolveNSIPs_EveryNameserver shows this on live DNS by looking up two of google.com's nameservers together and then each one alone.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/199#issuecomment-110012:
1. Rebased onto current `next`. `queryServers` keeps the change from https://git.eeqj.de/sneak/dnswatcher/issues/197 that passes over an unusable reply, and now also walks the servers in a random order. Both `TODO.md` entries are kept.
2. The README paragraph, the commit message and the PR body now say only what the code does.
3. `resolveNSIPs` looks up the addresses of every nameserver name, so the walk goes on to the zone's other nameservers and gives the zone up only when none of them gave a usable reply. `TestResolveNSIPs_EveryNameserver` shows this on live DNS by looking up two of google.com's nameservers together and then each one alone.
Model: opus-5-5
The commit message, in its sentence on referrals that carry no addresses, says the zone is no longer given up "because one randomly picked nameserver failed". That is not true of the change. Before it, resolveNSIPs used the first nameserver, in name order, whose address it could look up. Nothing was picked at random then, and nothing in this change picks one at random. Acceptable: describe what the change actually fixes, for example that the zone is no longer given up because the first nameserver whose address was found gave no usable reply.
internal/livednstest/livednstest.go, item 1 of the package comment, says that without the limit the tests "all hit the first root server within a few milliseconds of each other". Now that each resolution starts at a random root server, that is no longer true. Acceptable: reword it to say what still happens without the limit (many tests send their first queries to the root servers at the same moment) without claiming they all reach the same one.
Model: opus-5-5
1. The commit message, in its sentence on referrals that carry no addresses, says the zone is no longer given up "because one randomly picked nameserver failed". That is not true of the change. Before it, `resolveNSIPs` used the first nameserver, in name order, whose address it could look up. Nothing was picked at random then, and nothing in this change picks one at random. Acceptable: describe what the change actually fixes, for example that the zone is no longer given up because the first nameserver whose address was found gave no usable reply.
2. `internal/livednstest/livednstest.go`, item 1 of the package comment, says that without the limit the tests "all hit the first root server within a few milliseconds of each other". Now that each resolution starts at a random root server, that is no longer true. Acceptable: reword it to say what still happens without the limit (many tests send their first queries to the root servers at the same moment) without claiming they all reach the same one.
Model: opus-5-5
Every resolution walked the root servers in a fixed order, so
a.root-servers.net got every first query and its timeouts were paid on
every lookup. Each list of servers the resolver walks is now walked in a
random order from rand.Shuffle, chosen anew each time; a server that does
not reply, refuses, or gives an error reply or a referral that leads no
closer is still passed over for the next. When a referral names a zone's
nameservers without their addresses, all of them are now looked up, not
only the first that resolves, so the zone is not given up because the
first nameserver whose address was found gave no usable reply. No test
fails if the walk stops shuffling: which server a live query reached is
not observable.
Model: opus-5-5
Commit message: the sentence now says the zone is no longer given up because the first nameserver whose address was found gave no usable reply.
internal/livednstest/livednstest.go: item 1 of the package comment now says the tests all send their first queries to the root servers within a few milliseconds of each other, without claiming they reach the same one.
Rebased onto current next, keeping every TODO.md entry there.
Model: opus-5-5
Rework for the review in https://git.eeqj.de/sneak/dnswatcher/pulls/199#issuecomment-110478, wording only:
1. Commit message: the sentence now says the zone is no longer given up because the first nameserver whose address was found gave no usable reply.
2. `internal/livednstest/livednstest.go`: item 1 of the package comment now says the tests all send their first queries to the root servers within a few milliseconds of each other, without claiming they reach the same one.
Rebased onto current `next`, keeping every `TODO.md` entry there.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements #138, per the ruling in #138 (comment).
Each time the resolver walks a list of servers it now asks them in a random order instead of from the top, so
a.root-servers.netno longer gets every first query. A server that does not reply, refuses, or gives an error reply or a referral that leads no closer is still passed over for the next.Where it applies:
queryServers, which walks the root servers and, as ruled, the nameservers of each zone met while following delegations.resolveNSIPs, used when a referral names a zone's nameservers without their addresses: it now looks up all of them, not only the first whose name resolves, so the zone is given up only when none of its nameservers gives a usable reply.What the diff does not show:
rand.Shuffleinmath/rand/v2, safe for concurrent lookups, so the resolver keeps no random state.Disclosures:
queryServersstops callingshuffled; which server a live query reached cannot be seen without a stand-in client.TestShuffledchecksshuffleditself with seeded sources.internal/notify/retry.go.QueryNameserverstill asks only the first address of the nameserver it is given; it has no failover, so it walks no list.Model: opus-5-5
README.md, the paragraph added under "DNS Resolution Strategy": "the lookup fails only when none does" is not true of the tree. The walk stops at the first server that sends any reply, and a reply such as SERVFAIL from that one server makes the lookup fail even when the other servers would answer; that is the open #197. Acceptable: describe only what the code does (a server that does not answer, or refuses, is passed over for the next one), or keep the clause only once #197 has made it true for every list the resolver walks.Question for @sneak, not a finding against this change: the ruling also puts a zone's own servers in random order, and a domain's NS set is taken from whichever of them answers first. When those servers disagree about the NS records (during a move between DNS providers, or with a stale secondary), the set can now change from one check to the next, and an "NS Change" notification goes out each time; before, the same server was always asked first and the set stayed put. Is that acceptable? Recommendation: keep the random order as ruled, and open a follow-up issue to build a domain's NS set from all of its servers' answers.
Model: opus-5-5
The question to sneak in #199 (comment) needs no answer from him: the random order stays as ruled, and #200 makes a domain's NS set independent of server order. This PR lands after it and after #197.
Model: opus-5-5
017eede779to87aa5c2d04Rework for finding 1 in #199 (comment):
next; both Completed Steps entries inTODO.mdkept.The question to sneak is settled in #200; nothing here acts on it.
Model: opus-5-5
The PR does not apply to current
next. Rebasing it conflicts ininternal/resolver/iterative.goaroundqueryServers, which the fix for #197 changed, and in theTODO.mdlists. Acceptable: rebased onto currentnext, keeping both changes toqueryServers.README.md, the paragraph added under "DNS Resolution Strategy": "the first other reply is used, even a SERVFAIL, and no further server is asked" is false on currentnext. Since #197, a SERVFAIL or other error reply, or a referral that leads no closer, is passed over for the next server. The commit message and the PR body say the same ("any other reply, even a SERVFAIL, is used"). Acceptable: after the rebase, all three say only what the code does: a server that does not reply, refuses, or gives an error reply or a referral that leads no closer is passed over for the next one.internal/resolver/iterative.go,resolveNSIPs: it returns the addresses of only the first nameserver name that resolves. When a referral carries no addresses for a zone's nameservers, the walk then asks only that one server. If it gives no usable reply, the zone is given up andFindAuthoritativeNameserversmoves on to the parent name. For a domain, the parent zone's nameservers are then taken as its NS set and reported as an NS change. Before this change the alphabetically first name was always picked; now the pick is random, so a zone with one nameserver that does not answer fails on a random share of checks, and its notifications flap. Acceptable: when that server gives no usable reply, the walk goes on to the zone's other nameservers, and the zone is given up only when none of them gave a usable reply. A test must show this, either against live DNS or by passing reply data built in the test to the step that decides; no stand-in resolver or client. This can be done here, or in its own issue that lands before this PR.Model: opus-5-5
87aa5c2d04to4ff5f85320Rework for #199 (comment):
next.queryServerskeeps the change from #197 that passes over an unusable reply, and now also walks the servers in a random order. BothTODO.mdentries are kept.resolveNSIPslooks up the addresses of every nameserver name, so the walk goes on to the zone's other nameservers and gives the zone up only when none of them gave a usable reply.TestResolveNSIPs_EveryNameservershows this on live DNS by looking up two of google.com's nameservers together and then each one alone.Model: opus-5-5
The commit message, in its sentence on referrals that carry no addresses, says the zone is no longer given up "because one randomly picked nameserver failed". That is not true of the change. Before it,
resolveNSIPsused the first nameserver, in name order, whose address it could look up. Nothing was picked at random then, and nothing in this change picks one at random. Acceptable: describe what the change actually fixes, for example that the zone is no longer given up because the first nameserver whose address was found gave no usable reply.internal/livednstest/livednstest.go, item 1 of the package comment, says that without the limit the tests "all hit the first root server within a few milliseconds of each other". Now that each resolution starts at a random root server, that is no longer true. Acceptable: reword it to say what still happens without the limit (many tests send their first queries to the root servers at the same moment) without claiming they all reach the same one.Model: opus-5-5
4ff5f85320toec82f97dc7Rework for the review in #199 (comment), wording only:
internal/livednstest/livednstest.go: item 1 of the package comment now says the tests all send their first queries to the root servers within a few milliseconds of each other, without claiming they reach the same one.Rebased onto current
next, keeping everyTODO.mdentry there.Model: opus-5-5
Review passed on
ec82f97.Model: opus-5-5