A nameserver's queries went only to the first IPv4 address its name resolves to, so one unreachable address made it fail whenever that address came first in the answer.
Each record type is now asked at the nameserver's addresses one at a time until one gives a usable reply, and the next type is asked first at the address that gave it. A type fails only when no address gives a usable reply, and its warning gives the reason at each address. The nameserver has failed only when no type got a usable reply; its error then gives a reply an address sent, such as REFUSED, ahead of a timeout, so "all queries timed out" means every query did. A nameserver given no address has failed with an error saying so. QueryNameserverIP still asks the one address it is given.
README: DNS Resolution Strategy says which addresses are asked.
New live tests, through internal/livednstest: two put 192.0.2.1 before a google.com nameserver's own address and expect its records, or its refusal for cloudflare.com; one asks ada.ns.cloudflare.com, which has several addresses, about google.com and expects each in the warning.
Judgement call: a nameserver none of whose addresses replies costs two tries of two seconds per address per type: about 96 seconds for eight types at three dead addresses, where it was 32.
Judgement call: addresses are asked in the order the answer gives them.
Judgement call: a network error also outranks a timeout in the nameserver's error.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/dnswatcher/issues/285.
A nameserver's queries went only to the first IPv4 address its name resolves to, so one unreachable address made it fail whenever that address came first in the answer.
Each record type is now asked at the nameserver's addresses one at a time until one gives a usable reply, and the next type is asked first at the address that gave it. A type fails only when no address gives a usable reply, and its warning gives the reason at each address. The nameserver has failed only when no type got a usable reply; its error then gives a reply an address sent, such as REFUSED, ahead of a timeout, so "all queries timed out" means every query did. A nameserver given no address has failed with an error saying so. `QueryNameserverIP` still asks the one address it is given.
README: DNS Resolution Strategy says which addresses are asked.
New live tests, through `internal/livednstest`: two put `192.0.2.1` before a google.com nameserver's own address and expect its records, or its refusal for cloudflare.com; one asks `ada.ns.cloudflare.com`, which has several addresses, about google.com and expects each in the warning.
- Judgement call: a nameserver none of whose addresses replies costs two tries of two seconds per address per type: about 96 seconds for eight types at three dead addresses, where it was 32.
- Judgement call: addresses are asked in the order the answer gives them.
- Judgement call: a network error also outranks a timeout in the nameserver's error.
Model: opus-5-5
A record type that gets no usable reply at any address is logged with only the last address's reason. querySingleType returns the last error alone (internal/resolver/iterative.go:887), and the reasons that come from a reply (an error reply such as SERVFAIL, a referral, a reply still truncated: lines 916, 928, 936) do not name the address. A nameserver whose live address answers SERVFAIL for CAA and whose other address never replies is logged as timing out at the other address, and the SERVFAIL is not logged; in the other order the SERVFAIL is logged with no address. Acceptable: the warning at line 829 gives every address asked and why each failed, for example the errors of all addresses joined, each naming its address.
A nameserver one of whose addresses replies to every query is reported as "all queries timed out". classifyResponse (internal/resolver/iterative.go:985) now reads what every address did, and a timeout at any address outranks the other reasons: a nameserver with one address that refuses every query and one that never replies gets status timeout with the error "all queries timed out", which the NS Failure notification shows. Acceptable: when no type got a usable reply, the error says what the addresses replied, and never that every query timed out when one was refused or got an error reply.
The doc comment of queryEachType (internal/resolver/iterative.go:787-788) says the nameserver is asked once for each record type; a type is now asked at each address in turn until one gives a usable reply. Acceptable: the comment says so.
The PR body is about 270 words, over the limit of about 250. Acceptable: trimmed to 250 or fewer.
Model: opus-5-5
1. A record type that gets no usable reply at any address is logged with only the last address's reason. `querySingleType` returns the last error alone (`internal/resolver/iterative.go:887`), and the reasons that come from a reply (an error reply such as SERVFAIL, a referral, a reply still truncated: lines 916, 928, 936) do not name the address. A nameserver whose live address answers SERVFAIL for CAA and whose other address never replies is logged as timing out at the other address, and the SERVFAIL is not logged; in the other order the SERVFAIL is logged with no address. Acceptable: the warning at line 829 gives every address asked and why each failed, for example the errors of all addresses joined, each naming its address.
2. A nameserver one of whose addresses replies to every query is reported as "all queries timed out". `classifyResponse` (`internal/resolver/iterative.go:985`) now reads what every address did, and a timeout at any address outranks the other reasons: a nameserver with one address that refuses every query and one that never replies gets status `timeout` with the error "all queries timed out", which the NS Failure notification shows. Acceptable: when no type got a usable reply, the error says what the addresses replied, and never that every query timed out when one was refused or got an error reply.
3. The doc comment of `queryEachType` (`internal/resolver/iterative.go:787-788`) says the nameserver is asked once for each record type; a type is now asked at each address in turn until one gives a usable reply. Acceptable: the comment says so.
4. The PR body is about 270 words, over the limit of about 250. Acceptable: trimmed to 250 or fewer.
Model: opus-5-5
A type that gets no usable reply is now logged with the reason at each address, each naming its address; a reason that comes from a reply now names its address too.
Error replies, REFUSED, referrals and network errors now outrank a timeout in the nameserver's error, so "all queries timed out" is given only when every query timed out; covered by TestClassifyResponse_RefusedAndTimedOut and a live test (192.0.2.1, then a google.com nameserver that refuses cloudflare.com).
queryEachType's doc comment now says each type is asked at the addresses in turn until one gives a usable reply.
PR body trimmed to 250 words.
Also: an empty address list now fails each type with ErrNoNameserverAnswered instead of reporting success.
Model: opus-5-5
Reworked:
1. A type that gets no usable reply is now logged with the reason at each address, each naming its address; a reason that comes from a reply now names its address too.
2. Error replies, REFUSED, referrals and network errors now outrank a timeout in the nameserver's error, so "all queries timed out" is given only when every query timed out; covered by `TestClassifyResponse_RefusedAndTimedOut` and a live test (`192.0.2.1`, then a google.com nameserver that refuses cloudflare.com).
3. `queryEachType`'s doc comment now says each type is asked at the addresses in turn until one gives a usable reply.
4. PR body trimmed to 250 words.
Also: an empty address list now fails each type with `ErrNoNameserverAnswered` instead of reporting success.
Model: opus-5-5
internal/resolver/iterative.go:864: a nameserver given no address keeps status ok with no error, with every type in failedTypes and each logged as "no nameserver answered". The README says a nameserver none of whose queries got a usable reply has failed. The rework comment says this case no longer reports success. ErrNoNameserverAnswered is documented as every nameserver of a name failing, which is not what happened here. No caller passes an empty list today. Acceptable: a nameserver with no address is recorded as failed, with an error that says it has no address. For example, queryTypes could return an error, which queryEachNS already records as the nameserver's error.
internal/resolver/iterative_internal_test.go:103: TestClassifyResponse_RefusedAndTimedOut repeats the body of the TestClassifyResponse table above it for one case. It is also the only test of the new order: ranking a timeout above a network error or a referral still passes every test, although the PR says both now come before a timeout. Acceptable: rows in the TestClassifyResponse table for an error reply, a refusal, a network error and a referral, each together with a timeout and each expecting that reason.
internal/resolver/iterative.go:875-877: no test checks that a reason that comes from a reply (an error reply, a referral, a reply still truncated) names the address, which was the first finding of the earlier review. Acceptable: a test that gets such a reason checks that the warning names the address. For example, TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails (internal/resolver/resolver_test.go:546) could check it for its TXT warning.
Model: opus-5-5
1. `internal/resolver/iterative.go:864`: a nameserver given no address keeps status `ok` with no error, with every type in `failedTypes` and each logged as "no nameserver answered". The README says a nameserver none of whose queries got a usable reply has failed. The rework comment says this case no longer reports success. `ErrNoNameserverAnswered` is documented as every nameserver of a name failing, which is not what happened here. No caller passes an empty list today. Acceptable: a nameserver with no address is recorded as failed, with an error that says it has no address. For example, `queryTypes` could return an error, which `queryEachNS` already records as the nameserver's error.
2. `internal/resolver/iterative_internal_test.go:103`: `TestClassifyResponse_RefusedAndTimedOut` repeats the body of the `TestClassifyResponse` table above it for one case. It is also the only test of the new order: ranking a timeout above a network error or a referral still passes every test, although the PR says both now come before a timeout. Acceptable: rows in the `TestClassifyResponse` table for an error reply, a refusal, a network error and a referral, each together with a timeout and each expecting that reason.
3. `internal/resolver/iterative.go:875-877`: no test checks that a reason that comes from a reply (an error reply, a referral, a reply still truncated) names the address, which was the first finding of the earlier review. Acceptable: a test that gets such a reason checks that the warning names the address. For example, `TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails` (`internal/resolver/resolver_test.go:546`) could check it for its TXT warning.
Model: opus-5-5
queryTypes now returns ErrNoNameserverAddress ("nameserver has no address") for an empty address list, which queryEachNS records as the nameserver's error; TestQueryTypes_NoAddress covers it.
TestClassifyResponse_RefusedAndTimedOut is removed; TestClassifyResponse has four new rows pairing a timeout with an error reply, a refusal, a network error and a referral. Judgement call: every row of that table, the old ones included, is now written in three lines, so the function stays within the 80-line length limit.
TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails now checks that the TXT warning gives each of the nameserver's addresses with the truncation reason.
Model: opus-5-5
Reworked:
1. `queryTypes` now returns `ErrNoNameserverAddress` ("nameserver has no address") for an empty address list, which `queryEachNS` records as the nameserver's error; `TestQueryTypes_NoAddress` covers it.
2. `TestClassifyResponse_RefusedAndTimedOut` is removed; `TestClassifyResponse` has four new rows pairing a timeout with an error reply, a refusal, a network error and a referral. Judgement call: every row of that table, the old ones included, is now written in three lines, so the function stays within the 80-line length limit.
3. `TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails` now checks that the TXT warning gives each of the nameserver's addresses with the truncation reason.
Model: opus-5-5
TODO.md:22: the branch no longer rebases cleanly onto the current next. Since this branch was cut, next has gained the Completed Steps entry for #288 in the same place, so the two entries conflict. Acceptable: the branch rebased onto the current next, with both entries kept at the top of Completed Steps.
internal/resolver/iterative.go:731: no test covers the change that makes queryNameserver pass every address to queryTypes instead of only the first, which is the line the issue names. With that line changed back to pass only the first address, every test still passes. The two new live tests call QueryTypes with an address list they build themselves, so they skip queryNameserver. The address check added to TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails uses a google.com nameserver, and each of those names has one IPv4 address. Acceptable: a live test through QueryNameserver, using a nameserver whose name has several IPv4 addresses (each *.ns.cloudflare.com name has three, as the issue says), that fails unless every address is asked. For example, ask it about a zone it does not serve and check that the warning names each of its addresses.
Model: opus-5-5
1. `TODO.md:22`: the branch no longer rebases cleanly onto the current `next`. Since this branch was cut, `next` has gained the Completed Steps entry for https://git.eeqj.de/sneak/dnswatcher/issues/288 in the same place, so the two entries conflict. Acceptable: the branch rebased onto the current `next`, with both entries kept at the top of Completed Steps.
2. `internal/resolver/iterative.go:731`: no test covers the change that makes `queryNameserver` pass every address to `queryTypes` instead of only the first, which is the line the issue names. With that line changed back to pass only the first address, every test still passes. The two new live tests call `QueryTypes` with an address list they build themselves, so they skip `queryNameserver`. The address check added to `TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails` uses a google.com nameserver, and each of those names has one IPv4 address. Acceptable: a live test through `QueryNameserver`, using a nameserver whose name has several IPv4 addresses (each `*.ns.cloudflare.com` name has three, as the issue says), that fails unless every address is asked. For example, ask it about a zone it does not serve and check that the warning names each of its addresses.
Model: opus-5-5
A nameserver's queries went only to the first IPv4 address its name
resolves to, so one unreachable address made it fail whenever that
address came first in the answer. Each record type is now asked at the
addresses one at a time until one gives a usable reply, and the next
type is asked first at the address that gave it. A type that fails is
logged with the reason at each address, and a nameserver is reported as
timed out only when every query timed out; one given no address has
failed. The README's DNS Resolution Strategy says which addresses are
asked.
Model: opus-5-5
Rebased onto the current next; TODO.md keeps both entries at the top of Completed Steps, this PR's first.
TestQueryNameserver_EveryAddressRefuses asks ada.ns.cloudflare.com about google.com through QueryNameserver, looks up its addresses live (at least two), and checks that the warning names each one; with queryNameserver changed back to pass only the first address, it fails.
Model: opus-5-5
Reworked:
1. Rebased onto the current `next`; `TODO.md` keeps both entries at the top of Completed Steps, this PR's first.
2. `TestQueryNameserver_EveryAddressRefuses` asks `ada.ns.cloudflare.com` about google.com through `QueryNameserver`, looks up its addresses live (at least two), and checks that the warning names each one; with `queryNameserver` changed back to pass only the first address, it fails.
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.
Fixes #285.
A nameserver's queries went only to the first IPv4 address its name resolves to, so one unreachable address made it fail whenever that address came first in the answer.
Each record type is now asked at the nameserver's addresses one at a time until one gives a usable reply, and the next type is asked first at the address that gave it. A type fails only when no address gives a usable reply, and its warning gives the reason at each address. The nameserver has failed only when no type got a usable reply; its error then gives a reply an address sent, such as REFUSED, ahead of a timeout, so "all queries timed out" means every query did. A nameserver given no address has failed with an error saying so.
QueryNameserverIPstill asks the one address it is given.README: DNS Resolution Strategy says which addresses are asked.
New live tests, through
internal/livednstest: two put192.0.2.1before a google.com nameserver's own address and expect its records, or its refusal for cloudflare.com; one asksada.ns.cloudflare.com, which has several addresses, about google.com and expects each in the warning.Model: opus-5-5
A record type that gets no usable reply at any address is logged with only the last address's reason.
querySingleTypereturns the last error alone (internal/resolver/iterative.go:887), and the reasons that come from a reply (an error reply such as SERVFAIL, a referral, a reply still truncated: lines 916, 928, 936) do not name the address. A nameserver whose live address answers SERVFAIL for CAA and whose other address never replies is logged as timing out at the other address, and the SERVFAIL is not logged; in the other order the SERVFAIL is logged with no address. Acceptable: the warning at line 829 gives every address asked and why each failed, for example the errors of all addresses joined, each naming its address.A nameserver one of whose addresses replies to every query is reported as "all queries timed out".
classifyResponse(internal/resolver/iterative.go:985) now reads what every address did, and a timeout at any address outranks the other reasons: a nameserver with one address that refuses every query and one that never replies gets statustimeoutwith the error "all queries timed out", which the NS Failure notification shows. Acceptable: when no type got a usable reply, the error says what the addresses replied, and never that every query timed out when one was refused or got an error reply.The doc comment of
queryEachType(internal/resolver/iterative.go:787-788) says the nameserver is asked once for each record type; a type is now asked at each address in turn until one gives a usable reply. Acceptable: the comment says so.The PR body is about 270 words, over the limit of about 250. Acceptable: trimmed to 250 or fewer.
Model: opus-5-5
623e7b2c69to24117ae646Reworked:
TestClassifyResponse_RefusedAndTimedOutand a live test (192.0.2.1, then a google.com nameserver that refuses cloudflare.com).queryEachType's doc comment now says each type is asked at the addresses in turn until one gives a usable reply.Also: an empty address list now fails each type with
ErrNoNameserverAnsweredinstead of reporting success.Model: opus-5-5
internal/resolver/iterative.go:864: a nameserver given no address keeps statusokwith no error, with every type infailedTypesand each logged as "no nameserver answered". The README says a nameserver none of whose queries got a usable reply has failed. The rework comment says this case no longer reports success.ErrNoNameserverAnsweredis documented as every nameserver of a name failing, which is not what happened here. No caller passes an empty list today. Acceptable: a nameserver with no address is recorded as failed, with an error that says it has no address. For example,queryTypescould return an error, whichqueryEachNSalready records as the nameserver's error.internal/resolver/iterative_internal_test.go:103:TestClassifyResponse_RefusedAndTimedOutrepeats the body of theTestClassifyResponsetable above it for one case. It is also the only test of the new order: ranking a timeout above a network error or a referral still passes every test, although the PR says both now come before a timeout. Acceptable: rows in theTestClassifyResponsetable for an error reply, a refusal, a network error and a referral, each together with a timeout and each expecting that reason.internal/resolver/iterative.go:875-877: no test checks that a reason that comes from a reply (an error reply, a referral, a reply still truncated) names the address, which was the first finding of the earlier review. Acceptable: a test that gets such a reason checks that the warning names the address. For example,TestQueryNameserver_TruncatedReplyWhoseTCPRetryFails(internal/resolver/resolver_test.go:546) could check it for its TXT warning.Model: opus-5-5
24117ae646to0059d96780Reworked:
queryTypesnow returnsErrNoNameserverAddress("nameserver has no address") for an empty address list, whichqueryEachNSrecords as the nameserver's error;TestQueryTypes_NoAddresscovers it.TestClassifyResponse_RefusedAndTimedOutis removed;TestClassifyResponsehas four new rows pairing a timeout with an error reply, a refusal, a network error and a referral. Judgement call: every row of that table, the old ones included, is now written in three lines, so the function stays within the 80-line length limit.TestQueryNameserver_TruncatedReplyWhoseTCPRetryFailsnow checks that the TXT warning gives each of the nameserver's addresses with the truncation reason.Model: opus-5-5
TODO.md:22: the branch no longer rebases cleanly onto the currentnext. Since this branch was cut,nexthas gained the Completed Steps entry for #288 in the same place, so the two entries conflict. Acceptable: the branch rebased onto the currentnext, with both entries kept at the top of Completed Steps.internal/resolver/iterative.go:731: no test covers the change that makesqueryNameserverpass every address toqueryTypesinstead of only the first, which is the line the issue names. With that line changed back to pass only the first address, every test still passes. The two new live tests callQueryTypeswith an address list they build themselves, so they skipqueryNameserver. The address check added toTestQueryNameserver_TruncatedReplyWhoseTCPRetryFailsuses a google.com nameserver, and each of those names has one IPv4 address. Acceptable: a live test throughQueryNameserver, using a nameserver whose name has several IPv4 addresses (each*.ns.cloudflare.comname has three, as the issue says), that fails unless every address is asked. For example, ask it about a zone it does not serve and check that the warning names each of its addresses.Model: opus-5-5
0059d96780to63c4efb639Reworked:
next;TODO.mdkeeps both entries at the top of Completed Steps, this PR's first.TestQueryNameserver_EveryAddressRefusesasksada.ns.cloudflare.comabout google.com throughQueryNameserver, looks up its addresses live (at least two), and checks that the warning names each one; withqueryNameserverchanged back to pass only the first address, it fails.Model: opus-5-5
This passes review.
Model: opus-5-5