Stop resolver module faults from failing DNS queries - #9
Merged
Conversation
added 4 commits
September 3, 2026 21:22
With LOGGING on, every call raised OverflowError from strmodulevent: its
binding rejects a value outside 'enum module_ev', and the value pythonmod
passes is evidently outside it. A log line must not be able to fail, so the
event is named against the MODULE_EVENT_* constants pythonmod injects, and an
unrecognised value is printed as itself rather than converted.
pythonmod: operate, id: 1, MODULE_EVENT_PASS
pythonmod: operate, id: 1, event 7
Reviewing the callback and transmit paths turned up faults that each end in a served query being lost, all reachable from a configuration an operator can write. Every one gets a test that fails without its fix. A FIREWALLS key present but empty parses as None, which setdefault cannot replace, so transmit_all iterated None and raised on every answer carrying an address. The shipped config ships commented-out entries in exactly that shape. It is normalised at load, along with a check that it is a list. inplace_cache_callback was the one callback with no guard and no return value. An exception there discards the cache hit, and the callback's result is read as a boolean, so returning nothing leaves an error pending for the next query to fail on. It now returns True and logs what it caught. operate's handler set MODULE_WAIT_MODULE whatever the event, which at MODDONE tells the mesh to advance to the next module and re-enter one that has already run. A fault there now finishes the query. The circuit breaker could not open against a firewall that accepts and never answers: a cache report succeeds at the socket, and recording that a success cleared the failures the blocking path had counted, so with any cache hit between two resolutions the count never reached the threshold and every query kept paying SOCKET_TIMEOUT. A non-blocking send now records nothing. logger's guard against a reply with no records sat five lines after the dereference it guarded, and the MODDONE call site tested qinfo where it meant rep. Every numeric config value is coerced once at load, and PORT is checked there. A quoted or emptied number previously reached a socket call as a string or None and raised there, where the failure reads as a resolver fault rather than the configuration error it is. UDP no longer waits for an acknowledgement a failed send cannot produce, treats ACKUPDATE as proof of delivery rather than retransmitting to a firewall that has already updated PF, and skips the wait entirely for a cache report, which is fire-and-forget by design.
A single recv() took whatever the first segment carried, so an ACKUPDATE delivered in two pieces compared unequal, was logged as a refusal and was charged to the circuit breaker as a failure against a firewall that had just confirmed the update. The reply is read until the firewall closes.
Updating the PFUI module is the common case and needs no rebuild, but the prompt said the build was required, and the source-tree question came first, so a module-only upgrade was asked to decide about replacing /usr/src before it had said whether it was building at all. The build is now a choice that reports what is installed and defaults to the sensible answer: keep an Unbound that already has the Python module, build one when there is none or it cannot load a module. Skipping says what it is doing and warns when the installed resolver cannot run PFUI. The source-tree choice moved inside the build path, which is the only thing that needs sources.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of
client-unbound/pfui_unbound.pyafter anOverflowErrorappeared ona live resolver. Nine defects, each ending in a served DNS query being lost, and
each reachable from a configuration an operator can write. Every one has a test
that fails without its fix.
The event name could not be logged
With
LOGGINGon, every call tooperateraisedOverflowErrorfromstrmodulevent: its binding rejects a value outsideenum module_ev, and thevalue pythonmod passes is evidently outside it. A log line must not be able to
fail, so the event is named against the
MODULE_EVENT_*constants pythonmodinjects, and an unrecognised value is printed as itself rather than converted —
which will also show what is actually arriving.
An empty FIREWALLS key raised on every answer
FIREWALLS:with its entries commented out parses asNone.setdefaultcannot replace a key that exists, so
transmit_alliteratedNoneand raisedfor every answer carrying an address. The shipped config ships commented-out
entries in exactly that shape. It is normalised at load, with a type check.
The cache callback could poison the interpreter
inplace_cache_callbackwas the only callback with neither a guard nor a returnvalue. An exception there discards the cache hit, and the result is read as a
boolean, so returning nothing leaves an error pending for the next query to fail
on. It now returns True and logs what it caught.
A fault at MODDONE looped the query
operate's handler setMODULE_WAIT_MODULEwhatever the event. At MODDONE thattells the mesh to advance and re-enter a module that has already run. A fault
there now finishes the query, as the documented contract requires.
The circuit breaker could never open
Against a firewall that accepts connections and never answers: a cache report
succeeds at the socket, and recording that a success cleared the failures the
blocking path had counted. With any cache hit between two resolutions — the
normal ratio for a resolver — the count never reached the threshold, and every
query kept paying
SOCKET_TIMEOUT. Measured: three failures alone open thebreaker, ten interleaved leave it closed with a count of zero. This is the
failure the code's own comment claimed to have fixed. A non-blocking send now
records nothing, because nothing is observable yet.
A reply split in transit was read as a refusal
The acknowledgement was one
recv, so anACKUPDATEdelivered in two piecescompared unequal, logged as a refusal, and was charged to the breaker against a
firewall that had just confirmed the update. It is read until the firewall
closes.
Smaller, same class
logger's guard against a reply with no records sat five lines after thedereference it guarded, and the MODDONE call site tested
qinfowhere itmeant
rep.PORTis validated there.A quoted
'3'reachedsettimeout()as a string and raisedTypeErrorinside the unguarded cache path.
init_standardnow logs a failed callback registration. pythonmod discardswhat
initreturns, so a resolver would otherwise start with no cachecallback and whitelist nothing, silently.
accepts
ACKUPDATEas proof of delivery rather than retransmitting to afirewall that has already updated PF, and skips the wait entirely for a cache
report, which DECISIONS.md defines as fire-and-forget.
Testing
states. Both are fixed the way Unbound's own examples do it, so they are right
regardless, but their severity comes from reading upstream source rather than
from observing it on a resolver.