because the cache no longer stores delegation (parent-side) NS rrsets,
and authoritative (child-side) NS rrsets don't affect recursion,
it no longer makes sense for qpcache_find() to look for NS rrsets
and return DNS_R_DELEGATION. that code has been removed.
the cache still does search for covering DNAME records. the
check_zonecut() function has been renamed to check_dname() for clarity.
related changes:
- one test case has been removed from the mirror system test, because it
tested the behavior of a cached delegation.
- query_checkrrl() and rpz_rrset_find() have been updated so they no
longer expect cache responses to have DNS_R_DELEGATION response codes.
If both dns_view_bestzonecut() and dns_deleg_fromrdataset() fail,
delegset stays NULL. Passing it to ns_query_recurse() would crash
on the REQUIRE(DNS_DELEGSET_VALID(delegset)) in createfetch().
Return ISC_R_NOTFOUND instead, which lets the caller handle the
failure gracefully.
When `query.c` finds a zonecut in the main cache (e.g. from stale NS
records), it must still use the correct delegation for recursion. Look
up the delegation DB via `dns_view_bestzonecut()` first; fall back to
`dns_deleg_fromrdataset()` only if no delegation is found.
This might also be done inside `query_lookup()` instead, with the `qctx`
holding a delegset property, but that approach needs further work to
avoid breakage and it is not clear so far if there would be other use
case of it. Current approach is simpler for now.
Previously, when answering from the cache, and when minimal-responses
was not set, we added the best known zone cut to the authority section
of the response message, using dns_db_findzonecut() to look it up in
the DNS cache. Since the DNS cache will no longer be used to store
parent-side NS RRsets, it will now be possible for an ancestor node
to be used as the zone cut, leading to the wrong NS record being
included.
There are various ways we could correct this:
1. Use dns_deleg_lookup() instead of dns_db_findzonecut() to find the
zone cut. But currently, the deleg database stores only the server
addresses for the delegation, not the full NS RRset; this would need
to be changed.
2. Look up <name>/NS whenever we cache a referral; that way we'll get
the child-side NS RRset and cache that, and we can retrieve it when
building the response.
But the solution chosen here is simply not to look up the NS record
when answering from the cache, effectively making "minimal-responses
yes;" mandatory for queries answered from the cache.
System tests have been updated as needed, so they no longer expect
NS RRsets in the authority section of recursive responses.
Function `dns_view_bestzonecut()` now uses the delegation DB instead of
the main cache when looking up at the cache.
As a result, replace `dns_rdataset_t` (representing an NS RRset) with
`dns_delegset_t` in `dns_view_bestzonecut()` and
`dns_resolver_createfetch()` APIs. The resolver and query processing now
use the delegation DB instead of the cache for zonecut lookups.
In the case of the delegation lives in the local database, the locally
found `rdataset` is internally converted into a `dns_delegset_t` object.
From caller POV, it doesn't change anything: a delegation set is a
read-only object which can be used as long as needed and must be
detached one it's done with it.
Since it is impossible to increase an isc_statsmulti counter and
retrieve the new counter atomically, and we need the output of
recursclients in order to compute ns_highwater_recursive, we change the
recursclients counter to an isc_stats one.
In the current statistics counter implementation, the statistics are
backed by an array of counters, which are updated via atomic operations.
This leads to contention, especially on high core count
machines.
This commit introduces a new isc_statsmulti_t counter that keeps a
separate array per thread. These counters are then aggregated only when
statistics are queried, shifting work off the critical path.
These changes lead to a ~2% improvement in perflab.
previously, rpz_rrset_find() behaved differently depending on whether
a cache lookup returned DNS_R_DELEGATION or ISC_R_NOTFOUND. the former
indicates the presence of a cached NS rrset, and the latter indicates
that the cache is cold or that all NS rrsets above the query name have
expired. both results indicate that the caller should recurse, but
rpz_rrset_find() only recursed in the case of DNS_R_DELEGATION.
the nsip-wait-recurse and nsdname-wait-recurse test cases in the
rpzrecurse system test were dependent on this misbehavior. the test
server was configured with a lame delegation, so that recursion always
failed, but once the lame delegation was expired due to a zero TTL, the
cache returned ISC_R_NOTFOUND, which caused the recursion not to be
attempted. the test seemed to be observing a delay before recursion
succeeded, but it was actually observing a delay before recursion was
skipped. fixing this bug caused the test to fail.
the test server has now been reconfigured so that recursion succeeds
after a delay, instead of failing. now we're able to test that
we're waiting for the successful completion of recursion.
Replace the local SAVE(), RESTORE(), and INITANDSAVE() macros in
query.c with the project-wide MOVE_OWNERSHIP() macro. The new
form is clearer about the intent: ownership of a pointer is being
transferred from source to destination, with the source set to NULL.
SAVE and RESTORE were identical macros with different names used to
indicate the direction of transfer, but this distinction was purely
cosmetic. INITANDSAVE additionally set the destination to NULL
first, which is unnecessary because the preceding memcpy already
initialized all fields from the source struct.
Currently we add an rrset-order cyclic statement to the default config.
Since the rrset-order allows matching a subset of all names, it must
be implemented with a string comparison against a wildcard, and since
the statement applies per rrset, this can result in millions of
comparisons per second on a busy authoritative server.
This commit removes rrset-order from the default config, but adds back
a code shim in query_setorder to preserve the previous behaviour.
Manually go through the code using dns_rdataset_isassociated() and
use dns_rdataset_cleanup() where appropriate in places that a simple
semantic patch is not able to find automatically.
When EDNS DO flag (`dig +dnssec`) flag is set, an rdataset is allocated
to hold the RRSIG of an RR, if present in DB. However, this allocation
is not done if the zone DB is not considered as secure
(`dns_db_issecure() == false`). Changes this behaviour by allocating the
rdataset anyway, so the RRSIG can be associated in the answer section of
the response as soon it is found from the DB.
Maintain the relationship between the parent and child fetch and when
creating a new child fetch, properly check the resolution loops that
would lead to a new fetch would join one of the parent's fetch contexts.
The qctx_destroy() only needs to be called on allocated memory and
qctx_deinit() needs to be called always. Also remove .allocated member
from the query_ctx_t structure.
Adding the query ID to the query trace message. The log is now as the
following (id is at the end):
query client=0x7f75c5017000 thread=0x7f75c6dfe680(foo.fr/A): \
client attr:0x22300, query attr:0x700, restarts:0, \
origqname:foo.fr, timer:0, authdb:0, referral:0, id:21338
This should help debugging tests, in particular to quickly get a
specific query from the logs.
Extended DNS Error 24 (Invalid Data) is returned when the server cannot
answer data for a zone it is configured for. This occurs typically when
an authoritative server does not have loaded the DB of a configured
zone, or a secondary server zone is expired.
See RFC 8914 section 4.25.
If `query_getzonedb()` finds a zone but the zone is expired it
immediately returns `DNS_R_EXPIRED` and doesn't attempt to get the zone
DB (which would be NULL in this case).
This enable caller to have a more precise reason of why getting the DB
has failed.
The field `ns_hookasync_t` was initially named `hook_actx` and wrongly
renamed `hook_aclctx` during a mass-renaming of various names for the
config acl context into a consistent `aclctx` name (see !11003). Of
course this is wrong as `ns_hookasync_t` has nothing to do with ACL but
about _async_ context. This commit fixes the mistake by renaming this
field `hookasyncctx`
When disabling algorithms, use the signer name to determine if the
algorithm is disabled or not. This allows for algorithms to be
cleanly disabled on a zone level basis. Previously, just using the
records owner name, "disable-algorithms" could impact resolution of
names that where not disabled. This does now mean that
"disable-algorithms" can not be used to disable part of a zone anymore.
ACL configuration context variables are inconsistently named as `actx`,
`ac`, or `aclconfctx`, which caused confusion during code reviews. This
commit renames all `cfg_aclconfctx_t` variables to `aclctx`, which is
short, consistent, and unambiguous.
Enabling LTO in the subsequent commit requires the file names to be
unique and having same probes.d in each of the libraries breaks this
requirement. Rename probes.d to probes-{isc,dns,ns}.d files and adjust
the includes.
query_reset() is called during query initialization, but the only
time the NS_QUERY_SETUP hook runs is when it's called from
query_cleanup(). it makes more sense to move the hook point to
there and rename it to NS_QUERY_CLEANUP.
this change caused a crash in the unit tests due to the view being
unnecessarily detached before ns__client_reset_cb() was called.
this has also been fixed.
guard the call to the NS_QUERY_RESET hook so it's called only if
the view has been set. If the view is NULL, it means the client has
been reset _before_ the query even started, and no other hook could
have been called, so it doesn't make sense to call this one.
this also enables us to avoid a NULL-check on the qctx->view in the
CALL_HOOK macros.
add a 'zhooks' member to the query_ctx structure, so that we only
need to look up the hook table for the zone once when iniitalizing
a qctx, and not once for every hook point.
Since the removal of NS_QUERY_QCTX_DESTROYED hook, there is no need for
the `qctx->detach_client` object anymore, as this was designed to tell
the plugin whether the client object is about to be, or is already,
freed from memory. This is not needed anymore, as NS_QUERY_RESET is
called _always_ when the client object is about to be freed from memory.
Remove `detach_client` and tidy up the code a bit by including the
freeing of the qctx object (when allocated) inside the qctx_destroy
function instead of requiring extra calls.
The hook NS_QUERY_QCTX_DESTROY is problematic with zone plugins because
it can be called in some contexts where `qctx->client` is invalid (the
pointer is dangling); which would lead to a use-after-free (spotted by
TSAN build) as `qctx->client` is used to get the zone hooktable, to find
out whether there is an authoritive zone which would have
NS_QUERY_QCTX_DESTROY registered.
This can't easily be fixed, because there is no easy way to know from
query.c code if `client` is still a valid object: `client->reqhandle`,
representing the request from a client, is refcounted, and the `client`
object is freed from memory once its refcounter gets to 0. While
`reqhandle` is attached from query.c code, it can be attached more than
once from asynchronous code and there is no clear path where detaching
it would lead to a client free. Hence, there is no way to know for sure
when to set `qctx->client = NULL` (this is why the pointer remains
dangling).
Back to the original problem; this is why the NS_QUERY_QCTX_DESTROY hook
is incompatible with zone plugins. `qctx->detach_client`, which is used
to tell a plugin that the `client` object is either free or about to be
free can't be use either, because in some cases the client is still
there, and should be used.
Code issue aside, the `qctx` object is really just an aggregate of
various data to pass easily in the various functions and callbacks,
initially stored on the stack, but allocated in some cases (for some
asynchronous flow, when recursion is needed), so the point it gets
created/"destroyed" is really just an implementation "detail", and
providing a higher level hook for the plugin would be beneficial. Hence,
NS_QUERY_RESET and NS_QUERY_INIT are removed, and instead, the existing
NS_QUERY_SETUP can be used as well as the newly introduced
NS_QUERY_RESET (which replaces NS_QUERY_QCTX_DESTROY). The advanage is
that NS_QUERY_RESET is called _only_ when the client object is _always_
about to be freed, which avoids usage of the extra `qctx->detach_client`
usage from the plugin.
The way NS_QUERY_RESET works is that when the `client` is freed, a
callback (from `query.c`) is called. This callback creates a transient
qctx object on the stack with a pointer to the view, and uses that
to call the hook.
Add a new query hook called `NS_QUERY_AUTHZONE_ATTACHED`. This hook is
called whenever an authoritative zone is found and attached during a
query answer.
From code level, this hook is called when `qctx->client->query->authzone`
is attached during a query. This enables zone-specific plugins to
initialize specific states whenever a local zone is found that can
answer a query.
Since the log level has been raised, busy servers can "explode" from
the amount of log messages. Use the usual practice of logging "every
once in a while".
The "RPZ not ready yet" message is logged at debug 3 level. Use the
info level instead for better visibility.
After raising the log level, the rpz_log_fail_helper() function starts
appending " failed: " the the message. Change the log message so it
makes more sense.
In order to not pollute the SERVFAIL cache with the configured
SERVFAIL answers while RPZ is loading, set the NS_CLIENTATTR_NOSETFC
attribute for the client.
The counter in ns_client_t is used to track the maximum number of
recursions in the resolver, but it is created unconditionally when
starting the client and deallocated when resetting it.
This commit defers the allocation of the counter till recursion needs to
actually happen, speeding up authoritative workloads in perflab by
1.5~2%.
By default, when named is started it may start answering to
queries before the response policy zones are completely loaded
and processed. This new feature gives an option to the users to
tell named that incoming requests should result in SERVFAIL anwser
until all the response policy zones are procesed and ready.
> Put a space before opening parentheses only after control statement
> keywords (for/if/while...) except this option doesn’t apply to ForEach
> and If macros. This is useful in projects where ForEach/If macros are
> treated as function calls instead of control statements.
Use dns_rdatatype_none instead of plain '0' for dns_rdatatype_t and
dns_typepair_t manipulation. While plain '0' is technically ok, it
doesn't carry the required semantic meaning, and using the named
dns_rdatatype_none constant makes the code more readable.
All databases in the codebase follow the same structure: a database is
an associative container from DNS names to nodes, and each node is an
associative container from RR types to RR data.
Each database implementation (qpzone, qpcache, sdlz, builtin, dyndb) has
its own corresponding node type (qpznode, qpcnode, etc). However, some
code needs to work with nodes generically regardless of their specific
type - for example, to acquire locks, manage references, or
register/unregister slabs from the heap.
Currently, these generic node operations are implemented as methods in
the database vtable, which creates problematic coupling between database
and node lifetimes. If a node outlives its parent database, the node
destructor will destroy all RR data, and each RR data destructor will
try to unregister from heaps by calling a virtual function from the
database vtable. Since the database was already freed, this causes a
crash.
This commit breaks the coupling by standardizing the layout of all
database nodes, adding a dedicated vtable for node operations, and
moving node-specific methods from the database vtable to the node
vtable.
When the zone is configured with a CNAME override policy, also add the
configured EDE code.
When the zone is contains a wildcard CNAME, also add the configured
EDE code.
A serve-stale refresh is similar to a prefetch, the only difference
is when it triggers. Where a prefetch is done when an RRset is about
to expire, a serve-stale refresh is done when the RRset is already
stale.
This means that the check for the stale-refresh window needs to
move into query_stale_refresh(). We need to clear the
DNS_DBFIND_STALEENABLED option at the same places as where we clear
DNS_DBFIND_STALETIMEOUT.
Now that serve-stale refresh acts the same as prefetch, there is no
worry that the same rdataset is added to the message twice. This makes
some code obsolete, specifically where we need to clear rdatasets from
the message.
