You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
From docs/FINDINGS.md. Found by running the Go provider this replaces, and correlated
against this codebase rather than acted on. Sources: external-dns-desec-provider#26,
a year of production fixes, its review, and sshine/external-dns-desec-provider#2,
another operator's account of the same failure from a four-cluster deployment. This is not a decision.
and zone_list_ok is true whenever GET /domains/ succeeded. refresh.rs sets it before reading
a single RRset, and its own test pins that it survives a mid-tick throttle:
// The zone list itself succeeded, so nothing is removed.assert!(update.zone_list_ok);assert!(update.error.is_some());
That is right for what the flag is for: a failed list is not evidence a zone has gone away (#7).
The problem is that last_full_ok is then reused as the snapshot's age, and age is what /readyz, webhook_snapshot_age_seconds and the age_s field on the /records log line all
report.
So a webhook that lists zones happily every 180 seconds and has been throttled out of every RRset
read for six hours reports an age of a couple of minutes and answers /readyz: ready. The records
it is serving are six hours old. Per-zone truth exists — Zone::listed_at, which Zone::is_stale already uses to schedule forced re-reads — and nothing surfaces it.
The staleness question underneath
The Go provider capped staleness at 24 hours:
// maxCacheStaleness caps how old a last-known-good record set may be before it is no longer// served during a throttle window. Past it we prefer a 500 (retried next interval) over// feeding external-dns a stale zone under --policy=sync, which could delete records that in// fact still exist.
Here there is no cap at all. #7 states the position — "a stale snapshot → 200 with stale data,
because old truth beats no truth" — and it is stronger than the Go comment's fear, because #7 also
establishes that reported-but-absent records cannot cause a deletion the owner filter would not
already stop, and because plan::build suppresses deletes for RRsets the snapshot does not hold.
What a stale snapshot can do is the opposite of deletion. A record created out of band while we
were blind — cert-manager's ACME TXT, a hand-edited MX — is missing from what we report, so
external-dns takes the len(row.current) == 0 branch, plans a Create, and we write over it. That
is a real overwrite rather than a phantom one, and its likelihood grows with age.
The author of the Go patch had arrived at the same doubt from the other direction, in pr2:
I'm wondering if the 24h maxCacheStaleness should be configurable and default closer to the
throttle window.
Where we stand
Two separable things, and only the first is clearly worth doing.
Report the right age.max(zone.listed_at.elapsed()) across the snapshot, exposed alongside the zone-list age rather than instead of it — the two mean different things and
both are diagnostic. This costs nothing and makes the existing readiness bound honest, which is
a precondition for arguing about the second point at all.
Cap the age. Refusing to serve past a bound trades a known-wrong answer for a 503, and An empty /records cannot cause deletion — but a missing filter can #7's argument is that the known-wrong answer is usually the better one. If it is ever added it
should be a flag defaulting to off rather than a constant, and it should be per-zone rather
than all-or-nothing — the Go version's all-or-nothing rule exists because it had no zone-level
model, and this one does.
The mechanism
store::publish:and
zone_list_okis true wheneverGET /domains/succeeded.refresh.rssets it before readinga single RRset, and its own test pins that it survives a mid-tick throttle:
That is right for what the flag is for: a failed list is not evidence a zone has gone away (#7).
The problem is that
last_full_okis then reused as the snapshot's age, and age is what/readyz,webhook_snapshot_age_secondsand theage_sfield on the/recordslog line allreport.
So a webhook that lists zones happily every 180 seconds and has been throttled out of every RRset
read for six hours reports an age of a couple of minutes and answers
/readyz: ready. The recordsit is serving are six hours old. Per-zone truth exists —
Zone::listed_at, whichZone::is_stalealready uses to schedule forced re-reads — and nothing surfaces it.The staleness question underneath
The Go provider capped staleness at 24 hours:
Here there is no cap at all. #7 states the position — "a stale snapshot →
200with stale data,because old truth beats no truth" — and it is stronger than the Go comment's fear, because #7 also
establishes that reported-but-absent records cannot cause a deletion the owner filter would not
already stop, and because
plan::buildsuppresses deletes for RRsets the snapshot does not hold.What a stale snapshot can do is the opposite of deletion. A record created out of band while we
were blind — cert-manager's ACME TXT, a hand-edited MX — is missing from what we report, so
external-dns takes the
len(row.current) == 0branch, plans a Create, and we write over it. Thatis a real overwrite rather than a phantom one, and its likelihood grows with age.
The author of the Go patch had arrived at the same doubt from the other direction, in pr2:
Where we stand
Two separable things, and only the first is clearly worth doing.
max(zone.listed_at.elapsed())across the snapshot, exposedalongside the zone-list age rather than instead of it — the two mean different things and
both are diagnostic. This costs nothing and makes the existing readiness bound honest, which is
a precondition for arguing about the second point at all.
503, andAn empty
/recordscannot cause deletion — but a missing filter can #7's argument is that the known-wrong answer is usually the better one. If it is ever added itshould be a flag defaulting to off rather than a constant, and it should be per-zone rather
than all-or-nothing — the Go version's all-or-nothing rule exists because it had no zone-level
model, and this one does.