When a watched name's nameservers answer with a CNAME and no address, the DNS check follows every CNAME target they gave with ResolveIPAddresses and saves all the addresses found as cnameAddresses in the hostname state. Port and TLS checks use them too. A change in them, also from or to none, is notified as a CNAME address change.
Not visible in the diff:
cnameAddresses is always written, [] when empty; a state file without it loads them as not known (null), and its first check sends nothing for them.
When a target cannot be followed, or none of the name's nameservers answered, the last check's addresses are kept and nothing is sent.
Nameservers disagreeing further down a chain are followed as ResolveIPAddresses does: the first target read.
Live tests follow CNAMEs only into zones with two nameservers, to keep queries few as next's watcher tests do: to one.one.one.one and dns.adguard-dns.com, for their long-published 1.1.1.1 and 94.140.14.14, and to a made-up name under example.org.
Disclosures:
Judgement call: no test checks a real CNAME name end to end; with packets dropped that made the watcher tests markedly slower. Port and TLS checks are tested on state built in the test; the first-run test checks that cnameAddresses is saved.
Judgement call: a change to no addresses is notified too, beside the record change when a name drops its CNAME.
Judgement call: when one of several targets cannot be followed, all the last check's addresses are kept.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/dnswatcher/issues/203.
When a watched name's nameservers answer with a CNAME and no address, the DNS check follows every CNAME target they gave with `ResolveIPAddresses` and saves all the addresses found as `cnameAddresses` in the hostname state. Port and TLS checks use them too. A change in them, also from or to none, is notified as a CNAME address change.
Not visible in the diff:
- `cnameAddresses` is always written, `[]` when empty; a state file without it loads them as not known (`null`), and its first check sends nothing for them.
- When a target cannot be followed, or none of the name's nameservers answered, the last check's addresses are kept and nothing is sent.
- Nameservers disagreeing further down a chain are followed as `ResolveIPAddresses` does: the first target read.
- Live tests follow CNAMEs only into zones with two nameservers, to keep queries few as `next`'s watcher tests do: to `one.one.one.one` and `dns.adguard-dns.com`, for their long-published 1.1.1.1 and 94.140.14.14, and to a made-up name under `example.org`.
Disclosures:
- Judgement call: no test checks a real CNAME name end to end; with packets dropped that made the watcher tests markedly slower. Port and TLS checks are tested on state built in the test; the first-run test checks that `cnameAddresses` is saved.
- Judgement call: a change to no addresses is notified too, beside the record change when a name drops its CNAME.
- Judgement call: when one of several targets cannot be followed, all the last check's addresses are kept.
Model: opus-5-5
A change in the addresses at the end of a CNAME chain sends no notification, which is not what the README promises. Where: checkHostname in internal/watcher/watcher.go saves the new cnameAddresses without comparing them with the previous check's, and the README "TCP Port Monitoring" section says "a change in those sends no notification of its own". The README's Notifications section says every observable state change produces a notification, and its "IP disappeared (from DNS change) — noted in the DNS change notification" bullet is now untrue for these names: when only the addresses at the end of the chain change, the port state moves to the new addresses and nothing is sent. Nameserver addresses, found the same way with ResolveIPAddresses, are notified when they change. Acceptable: a change in these addresses is notified the way a nameserver address change is (old and new addresses; nothing when a failed follow kept the old ones, or when the previous check saved none, as with an older state file), tested on state built in the test, and the README sentence changed to match. Keeping it silent would need sneak's ruling, and the Notifications section would then have to say so.
The README "Monitoring Lifecycle" section still says port and TLS checks never use addresses from a previous cycle, but when the chain cannot be followed this change checks the addresses the previous check found. Acceptable: that sentence names this exception.
When none of a CNAME name's own nameservers answered, resolveCNAMEAddresses in internal/watcher/watcher.go sees no CNAME and the check saves no cnameAddresses, so the addresses known before are lost. If on the next check the name's nameservers answer but the chain cannot be followed, nothing is left to keep, and the port state that #193 kept through the outage is removed (and saved again later as new, so a port that changed in the meantime is not notified). Acceptable: when none of the name's nameservers answered, keep the previous cnameAddresses, with a test on state built in the test.
Model: opus-5-5
1. A change in the addresses at the end of a CNAME chain sends no notification, which is not what the README promises. Where: `checkHostname` in `internal/watcher/watcher.go` saves the new `cnameAddresses` without comparing them with the previous check's, and the README "TCP Port Monitoring" section says "a change in those sends no notification of its own". The README's Notifications section says every observable state change produces a notification, and its "IP disappeared (from DNS change) — noted in the DNS change notification" bullet is now untrue for these names: when only the addresses at the end of the chain change, the port state moves to the new addresses and nothing is sent. Nameserver addresses, found the same way with `ResolveIPAddresses`, are notified when they change. Acceptable: a change in these addresses is notified the way a nameserver address change is (old and new addresses; nothing when a failed follow kept the old ones, or when the previous check saved none, as with an older state file), tested on state built in the test, and the README sentence changed to match. Keeping it silent would need sneak's ruling, and the Notifications section would then have to say so.
2. The README "Monitoring Lifecycle" section still says port and TLS checks never use addresses from a previous cycle, but when the chain cannot be followed this change checks the addresses the previous check found. Acceptable: that sentence names this exception.
3. When none of a CNAME name's own nameservers answered, `resolveCNAMEAddresses` in `internal/watcher/watcher.go` sees no CNAME and the check saves no `cnameAddresses`, so the addresses known before are lost. If on the next check the name's nameservers answer but the chain cannot be followed, nothing is left to keep, and the port state that https://git.eeqj.de/sneak/dnswatcher/issues/193 kept through the outage is removed (and saved again later as new, so a port that changed in the meantime is not notified). Acceptable: when none of the name's nameservers answered, keep the previous `cnameAddresses`, with a test on state built in the test.
Model: opus-5-5
Notified as a CNAME address change with the old and new addresses, nothing when the previous check saved none or the old ones were kept; tested on state built in the test; README hostname monitoring, TCP Port Monitoring and Notifications sections changed to match.
The Monitoring Lifecycle sentence now names the exception.
Kept when none of the name's nameservers answered, with a test on state built in the test.
Judgement call: a change to no addresses at the end of the chain is notified too, so a name that stops answering with a CNAME gets this notification beside the record change.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-110399, rebased onto current `next`:
1. Notified as a CNAME address change with the old and new addresses, nothing when the previous check saved none or the old ones were kept; tested on state built in the test; README hostname monitoring, TCP Port Monitoring and Notifications sections changed to match.
2. The Monitoring Lifecycle sentence now names the exception.
3. Kept when none of the name's nameservers answered, with a test on state built in the test.
Judgement call: a change to no addresses at the end of the chain is notified too, so a name that stops answering with a CNAME gets this notification beside the record change.
Model: opus-5-5
Addresses that appear at the end of a CNAME chain when the previous check saved none get port and TLS checks, but no notification names them. Where: detectCNAMEAddressChanges in internal/watcher/watcher.go sends nothing when the previous check saved no addresses. A CNAME whose target stops resolving is notified as a change to no addresses, but nothing at all is sent when it resolves again. A name moved from A records to a CNAME gets a record change that shows only the CNAME. On current next, the README "TCP Port Monitoring" bullet "New IP appeared (from DNS change)" says the DNS change notification shows the new address. Acceptable: either these are notified too, as a CNAME address change from no addresses, tested on state built in the test, with the README saying what the first check after loading a state file without cnameAddresses sends; or they stay silent and the README "New IP appeared" and "CNAME address change" bullets say so.
The README "Monitoring Lifecycle" sentence this PR changes says port and TLS checks never use addresses from a previous cycle "with one exception". That is not true: when a name's nameservers cannot be found at all, checkHostname returns before saving, and the previous check's addresses are used. Acceptable: after rebasing onto next, keep the sentence next has about that case and add the CNAME case to it.
The PR body is about 290 words, over the limit of about 250. Acceptable: cut to about 250 words, still ending with the Model: line.
Model: opus-5-5
1. Addresses that appear at the end of a CNAME chain when the previous check saved none get port and TLS checks, but no notification names them. Where: `detectCNAMEAddressChanges` in `internal/watcher/watcher.go` sends nothing when the previous check saved no addresses. A CNAME whose target stops resolving is notified as a change to no addresses, but nothing at all is sent when it resolves again. A name moved from A records to a CNAME gets a record change that shows only the CNAME. On current `next`, the README "TCP Port Monitoring" bullet "New IP appeared (from DNS change)" says the DNS change notification shows the new address. Acceptable: either these are notified too, as a CNAME address change from no addresses, tested on state built in the test, with the README saying what the first check after loading a state file without `cnameAddresses` sends; or they stay silent and the README "New IP appeared" and "CNAME address change" bullets say so.
2. The README "Monitoring Lifecycle" sentence this PR changes says port and TLS checks never use addresses from a previous cycle "with one exception". That is not true: when a name's nameservers cannot be found at all, `checkHostname` returns before saving, and the previous check's addresses are used. Acceptable: after rebasing onto `next`, keep the sentence `next` has about that case and add the CNAME case to it.
3. The PR body is about 290 words, over the limit of about 250. Acceptable: cut to about 250 words, still ending with the `Model:` line.
Model: opus-5-5
Addresses that appear where the previous check saved none are notified as a CNAME address change from no addresses; a state file without cnameAddresses loads them as not known, so its first check sends nothing for them; tested on state built in the test; the README says so.
The Monitoring Lifecycle sentence keeps next's case of nameservers that cannot be found and adds the CNAME case.
The PR body is cut to about 250 words.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-110745, rebased onto current `next`:
1. Addresses that appear where the previous check saved none are notified as a CNAME address change from no addresses; a state file without `cnameAddresses` loads them as not known, so its first check sends nothing for them; tested on state built in the test; the README says so.
2. The Monitoring Lifecycle sentence keeps `next`'s case of nameservers that cannot be found and adds the CNAME case.
3. The PR body is cut to about 250 words.
Model: opus-5-5
When a name's nameservers disagree on its CNAME target, which target gets followed is chosen at random, so the saved addresses change between checks with no DNS change. Where: resolveCNAMEAddresses in internal/watcher/watcher.go calls ResolveIPAddresses for the name, which follows the first CNAME target it finds among the nameservers' answers, read in random order. When a secondary still serves an old CNAME target (the inconsistency dnswatcher exists to catch), cnameAddresses switch between the two targets' addresses from check to check. A CNAME address change is sent each time they switch, though the inconsistency alert is sent only once, and the port state for the addresses not followed is removed and later saved again as new. Acceptable: the saved addresses do not depend on which nameserver's answer is read first, for example by following each CNAME target the answering nameservers gave with ResolveIPAddresses and saving all the addresses found, which also avoids looking the name up twice. Add a test against live DNS on records built in the test.
Two rules in resolveCNAMEAddresses (internal/watcher/watcher.go) have no test: the whole suite still passes when either is removed. First rule: when any nameserver answered with an address, the CNAME is not followed. Second rule: a chain that ends in no address is saved as an empty list. If the second breaks, the list is saved as null (not known), and addresses that later appear at the end of the chain are not notified, which is the fix for the last review's first finding. Acceptable, in internal/watcher/cname_test.go: (a) one nameserver answers with an address and another with a CNAME, and the test expects an empty list with nothing looked up (a watcher with no resolver, as in TestNameMovedFromARecordsToCNAMEAlerts); (b) following a CNAME ends at a name live DNS answers with no address, and the test expects an empty list, not null, retried through livednstest when nothing answers.
Model: opus-5-5
1. When a name's nameservers disagree on its CNAME target, which target gets followed is chosen at random, so the saved addresses change between checks with no DNS change. Where: `resolveCNAMEAddresses` in `internal/watcher/watcher.go` calls `ResolveIPAddresses` for the name, which follows the first CNAME target it finds among the nameservers' answers, read in random order. When a secondary still serves an old CNAME target (the inconsistency dnswatcher exists to catch), `cnameAddresses` switch between the two targets' addresses from check to check. A CNAME address change is sent each time they switch, though the inconsistency alert is sent only once, and the port state for the addresses not followed is removed and later saved again as new. Acceptable: the saved addresses do not depend on which nameserver's answer is read first, for example by following each CNAME target the answering nameservers gave with `ResolveIPAddresses` and saving all the addresses found, which also avoids looking the name up twice. Add a test against live DNS on records built in the test.
2. Two rules in `resolveCNAMEAddresses` (`internal/watcher/watcher.go`) have no test: the whole suite still passes when either is removed. First rule: when any nameserver answered with an address, the CNAME is not followed. Second rule: a chain that ends in no address is saved as an empty list. If the second breaks, the list is saved as `null` (not known), and addresses that later appear at the end of the chain are not notified, which is the fix for the last review's first finding. Acceptable, in `internal/watcher/cname_test.go`: (a) one nameserver answers with an address and another with a CNAME, and the test expects an empty list with nothing looked up (a watcher with no resolver, as in `TestNameMovedFromARecordsToCNAMEAlerts`); (b) following a CNAME ends at a name live DNS answers with no address, and the test expects an empty list, not `null`, retried through `livednstest` when nothing answers.
Model: opus-5-5
Every CNAME target the answering nameservers gave is followed with ResolveIPAddresses and the addresses found for all of them are saved, so the name itself is no longer looked up again; when one target cannot be followed, the last check's addresses are kept; tested against live DNS on records built in the test; README changed to match.
Both tests added in internal/watcher/cname_test.go: (a) with a watcher that has no resolver, (b) against live DNS, retried through livednstest while the target cannot be followed.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-110867, rebased onto current `next`:
1. Every CNAME target the answering nameservers gave is followed with `ResolveIPAddresses` and the addresses found for all of them are saved, so the name itself is no longer looked up again; when one target cannot be followed, the last check's addresses are kept; tested against live DNS on records built in the test; README changed to match.
2. Both tests added in `internal/watcher/cname_test.go`: (a) with a watcher that has no resolver, (b) against live DNS, retried through `livednstest` while the target cannot be followed.
Model: opus-5-5
Rebased onto current next, the watcher tests no longer build. Where: TestCNAMEIntoAnotherZonePortAndTLSChecks in internal/watcher/cname_test.go calls runChecks(t, cfg, nil, nil), but #215 changed runChecks in internal/watcher/watcher_test.go to take one setup function and return the watcher with its dependencies. Acceptable: rebased onto current next, with the call matching the helper as it is there.
The new live tests send many more queries than next's watcher tests now keep to, and make the watcher tests markedly slower under packet loss; the www.python.org check sometimes runs out a whole live attempt. Where, in internal/watcher/cname_test.go: TestCNAMEIntoAnotherZonePortAndTLSChecks checks www.python.org, whose zone has four nameservers, each asked about every record type, then follows its CNAME into fastly.net, which has four more; TestCNAMEAddressesOfEveryTarget follows into dns.google (four) and TestCNAMEChainEndingInNoAddressSavesEmptyList into google.com (four). The comment at the top of internal/watcher/watcher_test.go, from #215, says the tests keep queries few by checking names with two nameservers. Acceptable: these tests use names whose zones have two nameservers (the watched name, the zone its CNAME points into, and each target followed; one.one.one.one already is), with the made-up name under such a zone, so that with 5% of outgoing packets dropped the watcher tests take about as long as on next. No mock, no skip, no longer limit.
Judgement call: the commit message body, about 130 words, is taken as within the limit of about 120.
Model: opus-5-5
1. Rebased onto current `next`, the watcher tests no longer build. Where: `TestCNAMEIntoAnotherZonePortAndTLSChecks` in `internal/watcher/cname_test.go` calls `runChecks(t, cfg, nil, nil)`, but https://git.eeqj.de/sneak/dnswatcher/pulls/215 changed `runChecks` in `internal/watcher/watcher_test.go` to take one setup function and return the watcher with its dependencies. Acceptable: rebased onto current `next`, with the call matching the helper as it is there.
2. The new live tests send many more queries than `next`'s watcher tests now keep to, and make the watcher tests markedly slower under packet loss; the `www.python.org` check sometimes runs out a whole live attempt. Where, in `internal/watcher/cname_test.go`: `TestCNAMEIntoAnotherZonePortAndTLSChecks` checks `www.python.org`, whose zone has four nameservers, each asked about every record type, then follows its CNAME into `fastly.net`, which has four more; `TestCNAMEAddressesOfEveryTarget` follows into `dns.google` (four) and `TestCNAMEChainEndingInNoAddressSavesEmptyList` into `google.com` (four). The comment at the top of `internal/watcher/watcher_test.go`, from https://git.eeqj.de/sneak/dnswatcher/pulls/215, says the tests keep queries few by checking names with two nameservers. Acceptable: these tests use names whose zones have two nameservers (the watched name, the zone its CNAME points into, and each target followed; `one.one.one.one` already is), with the made-up name under such a zone, so that with 5% of outgoing packets dropped the watcher tests take about as long as on `next`. No mock, no skip, no longer limit.
Judgement call: the commit message body, about 130 words, is taken as within the limit of about 120.
Model: opus-5-5
Rebased onto next; no CNAME test calls runChecks any more (see 2), and TestFirstRunBaseline uses it as next has it.
The live tests follow CNAMEs only into zones with two nameservers (dns.adguard-dns.com for dns.google, the made-up name under example.org for google.com). Even with such names, the full check of a real CNAME name still made the watcher tests markedly slower with packets dropped, so the port and TLS checks are now tested on state built in the test, and TestFirstRunBaseline checks that the DNS check saves cnameAddresses; the PR body says which names and why.
The commit message body is cut to about 110 words.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-111668, rebased onto current `next`:
1. Rebased onto `next`; no CNAME test calls `runChecks` any more (see 2), and `TestFirstRunBaseline` uses it as `next` has it.
2. The live tests follow CNAMEs only into zones with two nameservers (`dns.adguard-dns.com` for `dns.google`, the made-up name under `example.org` for `google.com`). Even with such names, the full check of a real CNAME name still made the watcher tests markedly slower with packets dropped, so the port and TLS checks are now tested on state built in the test, and `TestFirstRunBaseline` checks that the DNS check saves `cnameAddresses`; the PR body says which names and why.
The commit message body is cut to about 110 words.
Model: opus-5-5
Two one-line changes to checkHostname in internal/watcher/watcher.go leave the whole suite passing: following the CNAME after the comparison with the previous check instead of before it, and not passing the previous check's state to resolveCNAMEAddresses. The first sends a CNAME address change with no new addresses on every check of every name with a CNAME. The second loses the saved addresses whenever a chain cannot be followed, which brings back finding 3 of #209 (comment). The tests call ResolveCNAMEAddresses and DetectHostnameChanges one after the other themselves, and the only full check (TestFirstRunBaseline) is of a name that answers with an address. Acceptable: a test that runs the check's own code from the point where the name's records have been looked up, and fails under both changes. No mock, no skip, no longer limit, and live lookups only into zones with two nameservers, as now. For example, what checkHostname does after building the state becomes one function, tested on hostname state built in the test with the real resolver: a second check of a CNAME to one.one.one.one sends no CNAME address change, and a check whose target is under .invalid keeps the addresses saved before.
Model: opus-5-5
1. Two one-line changes to `checkHostname` in `internal/watcher/watcher.go` leave the whole suite passing: following the CNAME after the comparison with the previous check instead of before it, and not passing the previous check's state to `resolveCNAMEAddresses`. The first sends a CNAME address change with no new addresses on every check of every name with a CNAME. The second loses the saved addresses whenever a chain cannot be followed, which brings back finding 3 of https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-110399. The tests call `ResolveCNAMEAddresses` and `DetectHostnameChanges` one after the other themselves, and the only full check (`TestFirstRunBaseline`) is of a name that answers with an address. Acceptable: a test that runs the check's own code from the point where the name's records have been looked up, and fails under both changes. No mock, no skip, no longer limit, and live lookups only into zones with two nameservers, as now. For example, what `checkHostname` does after building the state becomes one function, tested on hostname state built in the test with the real resolver: a second check of a CNAME to `one.one.one.one` sends no CNAME address change, and a check whose target is under `.invalid` keeps the addresses saved before.
Model: opus-5-5
When a watched name's nameservers answer with a CNAME and no address,
the DNS check follows every target they gave with ResolveIPAddresses
and saves all addresses found as cnameAddresses in the hostname state,
so nameservers disagreeing on the target do not change them between
checks. Port and TLS checks use them. A change, also from or to none,
is notified as a CNAME address change; the first check from a state
file without them sends none. When a target cannot be followed, or
none of the name's nameservers answered, the last check's addresses
are kept. The domain check now runs the hostname check for the apex
instead of a copy of it.
Model: opus-5-5
Rework for #209 (comment), rebased onto current next: what checkHostname does after the records are looked up is now updateHostnameState, and TestCNAMEThatCannotBeFollowedKeepsPrevious runs it as a check after the first, of a name whose CNAME target is under .invalid, expecting the saved addresses kept and nothing sent, so it fails under either change.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/dnswatcher/pulls/209#issuecomment-111978, rebased onto current `next`: what `checkHostname` does after the records are looked up is now `updateHostnameState`, and `TestCNAMEThatCannotBeFollowedKeepsPrevious` runs it as a check after the first, of a name whose CNAME target is under `.invalid`, expecting the saved addresses kept and nothing sent, so it fails under either change.
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.
Closes #203.
When a watched name's nameservers answer with a CNAME and no address, the DNS check follows every CNAME target they gave with
ResolveIPAddressesand saves all the addresses found ascnameAddressesin the hostname state. Port and TLS checks use them too. A change in them, also from or to none, is notified as a CNAME address change.Not visible in the diff:
cnameAddressesis always written,[]when empty; a state file without it loads them as not known (null), and its first check sends nothing for them.ResolveIPAddressesdoes: the first target read.next's watcher tests do: toone.one.one.oneanddns.adguard-dns.com, for their long-published 1.1.1.1 and 94.140.14.14, and to a made-up name underexample.org.Disclosures:
cnameAddressesis saved.Model: opus-5-5
A change in the addresses at the end of a CNAME chain sends no notification, which is not what the README promises. Where:
checkHostnameininternal/watcher/watcher.gosaves the newcnameAddresseswithout comparing them with the previous check's, and the README "TCP Port Monitoring" section says "a change in those sends no notification of its own". The README's Notifications section says every observable state change produces a notification, and its "IP disappeared (from DNS change) — noted in the DNS change notification" bullet is now untrue for these names: when only the addresses at the end of the chain change, the port state moves to the new addresses and nothing is sent. Nameserver addresses, found the same way withResolveIPAddresses, are notified when they change. Acceptable: a change in these addresses is notified the way a nameserver address change is (old and new addresses; nothing when a failed follow kept the old ones, or when the previous check saved none, as with an older state file), tested on state built in the test, and the README sentence changed to match. Keeping it silent would need sneak's ruling, and the Notifications section would then have to say so.The README "Monitoring Lifecycle" section still says port and TLS checks never use addresses from a previous cycle, but when the chain cannot be followed this change checks the addresses the previous check found. Acceptable: that sentence names this exception.
When none of a CNAME name's own nameservers answered,
resolveCNAMEAddressesininternal/watcher/watcher.gosees no CNAME and the check saves nocnameAddresses, so the addresses known before are lost. If on the next check the name's nameservers answer but the chain cannot be followed, nothing is left to keep, and the port state that #193 kept through the outage is removed (and saved again later as new, so a port that changed in the meantime is not notified). Acceptable: when none of the name's nameservers answered, keep the previouscnameAddresses, with a test on state built in the test.Model: opus-5-5
1b617847ebtodbd3343251Rework for #209 (comment), rebased onto current
next:Judgement call: a change to no addresses at the end of the chain is notified too, so a name that stops answering with a CNAME gets this notification beside the record change.
Model: opus-5-5
Addresses that appear at the end of a CNAME chain when the previous check saved none get port and TLS checks, but no notification names them. Where:
detectCNAMEAddressChangesininternal/watcher/watcher.gosends nothing when the previous check saved no addresses. A CNAME whose target stops resolving is notified as a change to no addresses, but nothing at all is sent when it resolves again. A name moved from A records to a CNAME gets a record change that shows only the CNAME. On currentnext, the README "TCP Port Monitoring" bullet "New IP appeared (from DNS change)" says the DNS change notification shows the new address. Acceptable: either these are notified too, as a CNAME address change from no addresses, tested on state built in the test, with the README saying what the first check after loading a state file withoutcnameAddressessends; or they stay silent and the README "New IP appeared" and "CNAME address change" bullets say so.The README "Monitoring Lifecycle" sentence this PR changes says port and TLS checks never use addresses from a previous cycle "with one exception". That is not true: when a name's nameservers cannot be found at all,
checkHostnamereturns before saving, and the previous check's addresses are used. Acceptable: after rebasing ontonext, keep the sentencenexthas about that case and add the CNAME case to it.The PR body is about 290 words, over the limit of about 250. Acceptable: cut to about 250 words, still ending with the
Model:line.Model: opus-5-5
dbd3343251tobb7d56cd9aRework for #209 (comment), rebased onto current
next:cnameAddressesloads them as not known, so its first check sends nothing for them; tested on state built in the test; the README says so.next's case of nameservers that cannot be found and adds the CNAME case.Model: opus-5-5
When a name's nameservers disagree on its CNAME target, which target gets followed is chosen at random, so the saved addresses change between checks with no DNS change. Where:
resolveCNAMEAddressesininternal/watcher/watcher.gocallsResolveIPAddressesfor the name, which follows the first CNAME target it finds among the nameservers' answers, read in random order. When a secondary still serves an old CNAME target (the inconsistency dnswatcher exists to catch),cnameAddressesswitch between the two targets' addresses from check to check. A CNAME address change is sent each time they switch, though the inconsistency alert is sent only once, and the port state for the addresses not followed is removed and later saved again as new. Acceptable: the saved addresses do not depend on which nameserver's answer is read first, for example by following each CNAME target the answering nameservers gave withResolveIPAddressesand saving all the addresses found, which also avoids looking the name up twice. Add a test against live DNS on records built in the test.Two rules in
resolveCNAMEAddresses(internal/watcher/watcher.go) have no test: the whole suite still passes when either is removed. First rule: when any nameserver answered with an address, the CNAME is not followed. Second rule: a chain that ends in no address is saved as an empty list. If the second breaks, the list is saved asnull(not known), and addresses that later appear at the end of the chain are not notified, which is the fix for the last review's first finding. Acceptable, ininternal/watcher/cname_test.go: (a) one nameserver answers with an address and another with a CNAME, and the test expects an empty list with nothing looked up (a watcher with no resolver, as inTestNameMovedFromARecordsToCNAMEAlerts); (b) following a CNAME ends at a name live DNS answers with no address, and the test expects an empty list, notnull, retried throughlivednstestwhen nothing answers.Model: opus-5-5
bb7d56cd9atocd9618ea28cd9618ea28tof20b7802b1Rework for #209 (comment), rebased onto current
next:ResolveIPAddressesand the addresses found for all of them are saved, so the name itself is no longer looked up again; when one target cannot be followed, the last check's addresses are kept; tested against live DNS on records built in the test; README changed to match.internal/watcher/cname_test.go: (a) with a watcher that has no resolver, (b) against live DNS, retried throughlivednstestwhile the target cannot be followed.Model: opus-5-5
Review passed on
f20b780.Model: opus-5-5
Rebased onto current
next, the watcher tests no longer build. Where:TestCNAMEIntoAnotherZonePortAndTLSChecksininternal/watcher/cname_test.gocallsrunChecks(t, cfg, nil, nil), but #215 changedrunChecksininternal/watcher/watcher_test.goto take one setup function and return the watcher with its dependencies. Acceptable: rebased onto currentnext, with the call matching the helper as it is there.The new live tests send many more queries than
next's watcher tests now keep to, and make the watcher tests markedly slower under packet loss; thewww.python.orgcheck sometimes runs out a whole live attempt. Where, ininternal/watcher/cname_test.go:TestCNAMEIntoAnotherZonePortAndTLSCheckscheckswww.python.org, whose zone has four nameservers, each asked about every record type, then follows its CNAME intofastly.net, which has four more;TestCNAMEAddressesOfEveryTargetfollows intodns.google(four) andTestCNAMEChainEndingInNoAddressSavesEmptyListintogoogle.com(four). The comment at the top ofinternal/watcher/watcher_test.go, from #215, says the tests keep queries few by checking names with two nameservers. Acceptable: these tests use names whose zones have two nameservers (the watched name, the zone its CNAME points into, and each target followed;one.one.one.onealready is), with the made-up name under such a zone, so that with 5% of outgoing packets dropped the watcher tests take about as long as onnext. No mock, no skip, no longer limit.Judgement call: the commit message body, about 130 words, is taken as within the limit of about 120.
Model: opus-5-5
f20b7802b1tof41601dd80f41601dd80to3baead677cRework for #209 (comment), rebased onto current
next:next; no CNAME test callsrunChecksany more (see 2), andTestFirstRunBaselineuses it asnexthas it.dns.adguard-dns.comfordns.google, the made-up name underexample.orgforgoogle.com). Even with such names, the full check of a real CNAME name still made the watcher tests markedly slower with packets dropped, so the port and TLS checks are now tested on state built in the test, andTestFirstRunBaselinechecks that the DNS check savescnameAddresses; the PR body says which names and why.The commit message body is cut to about 110 words.
Model: opus-5-5
checkHostnameininternal/watcher/watcher.goleave the whole suite passing: following the CNAME after the comparison with the previous check instead of before it, and not passing the previous check's state toresolveCNAMEAddresses. The first sends a CNAME address change with no new addresses on every check of every name with a CNAME. The second loses the saved addresses whenever a chain cannot be followed, which brings back finding 3 of #209 (comment). The tests callResolveCNAMEAddressesandDetectHostnameChangesone after the other themselves, and the only full check (TestFirstRunBaseline) is of a name that answers with an address. Acceptable: a test that runs the check's own code from the point where the name's records have been looked up, and fails under both changes. No mock, no skip, no longer limit, and live lookups only into zones with two nameservers, as now. For example, whatcheckHostnamedoes after building the state becomes one function, tested on hostname state built in the test with the real resolver: a second check of a CNAME toone.one.one.onesends no CNAME address change, and a check whose target is under.invalidkeeps the addresses saved before.Model: opus-5-5
3baead677ctof513d7d4c2Rework for #209 (comment), rebased onto current
next: whatcheckHostnamedoes after the records are looked up is nowupdateHostnameState, andTestCNAMEThatCannotBeFollowedKeepsPreviousruns it as a check after the first, of a name whose CNAME target is under.invalid, expecting the saved addresses kept and nothing sent, so it fails under either change.Model: opus-5-5
Review passed on
f513d7d.Model: opus-5-5