From 1b47f29172c02b94dfdafc5624b0007de65632ac Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 22 Feb 2022 11:19:51 +0100 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Remi Gacogne Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Peter van Dijk --- pdns/dnsrecords.hh | 4 +-- pdns/rec-lua-conf.cc | 21 ++++++------- .../docs/lua-config/additionals.rst | 30 +++++++++++-------- pdns/recursordist/docs/lua-config/index.rst | 2 +- pdns/syncres.cc | 26 +++++++++------- .../test_Additionals.py | 8 ++--- 6 files changed, 52 insertions(+), 39 deletions(-) diff --git a/pdns/dnsrecords.hh b/pdns/dnsrecords.hh index 3d1e348011..e587edf68a 100644 --- a/pdns/dnsrecords.hh +++ b/pdns/dnsrecords.hh @@ -51,11 +51,11 @@ public: includeboilerplate(NAPTR) template void xfrRecordContent(Convertor& conv); - string getFlags() const + const string& getFlags() const { return d_flags; } - DNSName getReplacement() const + const DNSName& getReplacement() const { return d_replacement; } diff --git a/pdns/rec-lua-conf.cc b/pdns/rec-lua-conf.cc index c5c71b2e96..8f730e4642 100644 --- a/pdns/rec-lua-conf.cc +++ b/pdns/rec-lua-conf.cc @@ -332,6 +332,14 @@ public: } void postPrepareContext() override { + // clang-format off + d_pd.push_back({"AdditionalMode", in_t{ + {"Ignore", static_cast(AdditionalMode::Ignore)}, + {"CacheOnly", static_cast(AdditionalMode::CacheOnly)}, + {"CacheOnlyRequireAuth", static_cast(AdditionalMode::CacheOnlyRequireAuth)}, + {"ResolveImmediately", static_cast(AdditionalMode::ResolveImmediately)}, + {"ResolveDeferred", static_cast(AdditionalMode::ResolveDeferred)} + }}); } void postLoad() override { @@ -682,7 +690,7 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de }); #endif /* HAVE_FSTRM */ - Lua->writeFunction("addAllowedAdditionalQType", [&lci](int qtype, std::unordered_map targetqtypes, boost::optional> options) { + Lua->writeFunction("addAllowedAdditionalQType", [&lci](int qtype, std::unordered_map targetqtypes, boost::optional> options) { switch (qtype) { case QType::MX: case QType::SRV: @@ -704,17 +712,10 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de if (options) { if (const auto it = options->find("mode"); it != options->end()) { - const map modeMap = { - {"Ignore", AdditionalMode::Ignore}, - {"CacheOnly", AdditionalMode::CacheOnly}, - {"CacheOnlyRequireAuth", AdditionalMode::CacheOnlyRequireAuth}, - {"ResolveImmediately", AdditionalMode::ResolveImmediately}, - {"ResolveDeferred", AdditionalMode::ResolveDeferred}}; - if (modeMap.find(it->second) == modeMap.end()) { + mode = static_cast(it->second); + if (mode > AdditionalMode::ResolveDeferred) { g_log << Logger::Error << "addAllowedAdditionalQType: unknown mode " << it->second << endl; - return; } - mode = modeMap.at(it->second); } } lci.allowAdditionalQTypes.insert_or_assign(qtype, pair(targets, mode)); diff --git a/pdns/recursordist/docs/lua-config/additionals.rst b/pdns/recursordist/docs/lua-config/additionals.rst index 56a043803f..7fb4042559 100644 --- a/pdns/recursordist/docs/lua-config/additionals.rst +++ b/pdns/recursordist/docs/lua-config/additionals.rst @@ -27,27 +27,33 @@ An example of a configuration: .. code-block:: Lua addAllowedAdditionalQType(pdns.MX, {pdns.A, pdns.AAAA}) - addAllowedAdditionalQType(pdns.NAPTR, {pdns.A, pdns.AAAA, pdns.SRV}, {mode="ResolveImmediately"}) + addAllowedAdditionalQType(pdns.NAPTR, {pdns.A, pdns.AAAA, pdns.SRV}, {mode=pdns.AdditionalMode.ResolveImmediately}) The first line specifies that additional records should be added to the results of ``MX`` queries using the default mode. The qtype of the records to be added are ``A`` and ``AAAA``. -The default mode is ``CacheOnlyRequireAuth``, this mode will only look in the record cache. +The default mode is ``pdns.AdditionalMode.CacheOnlyRequireAuth``, this mode will only look in the record cache. The second line specifies that three record types should be added to ``NAPTR`` answers. If needed, the Recursor will do an active resolve to retrieve these records. The modes available are: - * ``Ignore`` Do not do any additional processing for this qtype. This is equivalent to not calling :func:`addAllowedAdditionalQType` for the qtype. - * ``CacheOnly`` Look in the record cache for available records. Allow non-authoritative (learned from additional sections received from authoritative servers) records to be added. - * ``CacheOnlyRequireAuth`` Look in the record cache for available records. Only authoritative records will be added. These are records received from the nameservers for the specific domain. - * ``ResolveImmediately`` Add records from the record cache (including DNSSEC records if relevant). If no record is found in the record cache, actively try to resolve the target name/qtype. This will delay the answer to the client. - * ``ResolveDeferred`` Add records from the record cache (including DNSSEC records if relevant). If no record is found in the record cache and the negative cache also has no entry, schedule a background task to resolve the target name/qtype. The next time the query is processed, the cache might hold the relevant information. This mode is not implemented yet. -If an additional record is not available at that time the query is stored into the packet cache the answer packet stored in the packer cache will not contain the additional record. +``pdns.AdditionalMode.Ignore`` + Do not do any additional processing for this qtype. This is equivalent to not calling :func:`addAllowedAdditionalQType` for the qtype. +``pdns.AdditionalMode.CacheOnly`` + Look in the record cache for available records. Allow non-authoritative (learned from additional sections received from authoritative servers) records to be added. +``pdns.AdditionalMode.CacheOnlyRequireAuth`` + Look in the record cache for available records. Only authoritative records will be added. These are records received from the nameservers for the specific domain. +``pdns.AdditionalMode.ResolveImmediately`` + Add records from the record cache (including DNSSEC records if relevant). If no record is found in the record cache, actively try to resolve the target name/qtype. This will delay the answer to the client. +``pdns.AdditionalMode.ResolveDeferred`` + Add records from the record cache (including DNSSEC records if relevant). If no record is found in the record cache and the negative cache also has no entry, schedule a background task to resolve the target name/qtype. The next time the query is processed, the cache might hold the relevant information. This mode is not implemented yet. + +If an additional record is not available at that time the query is stored into the packet cache the answer packet stored in the packet cache will not contain the additional record. Clients repeating the same question will get an answer from the packet cache if the question is still in the packet cache. -These answers do not have the additional record, even if in the meantime the record cache has learned it. -Clients wil only see the additional record once the packet cache entry expires and the record cache is consulted again. -The ``ResolveImmediately`` will not have this issue, at the cost of delaying the first query to resolve the additional records needed. +These answers do not have the additional record, even if the record cache has learned it in the meantime . +Clients will only see the additional record once the packet cache entry expires and the record cache is consulted again. +The ``pdns.AdditionalMode.ResolveImmediately`` mode will not have this issue, at the cost of delaying the first query to resolve the additional records needed. Configuring additional record processing ---------------------------------------- @@ -65,6 +71,6 @@ Calling :func:`addAllowedAdditionalQType` multiple times with a specific qtype :param int qtype: the qtype number to enable additional record processing for. Supported are: ``pdns.MX``, ``pdns.SRV``, ``pdns.SVCB``, ``pdns.HTTPS`` and ``pdns.NAPTR``. :param targets: the target qtypes to look for when adding the additionals. For example ``{pdns.A, pdns.AAAA}``. :type targets: list of qtype numbers - :param table options: a table of options. Currently the only option is ``mode`` having a string value. For the available modes, see above. If no mode is specified, the default ``"CacheOnlyRequireAuth"`` mode is used. + :param table options: a table of options. Currently the only option is ``mode`` having an integer value. For the available modes, see above. If no mode is specified, the default ``pdns.AdditionalMode.CacheOnlyRequireAuth`` mode is used. diff --git a/pdns/recursordist/docs/lua-config/index.rst b/pdns/recursordist/docs/lua-config/index.rst index 64543abbca..a602f22d67 100644 --- a/pdns/recursordist/docs/lua-config/index.rst +++ b/pdns/recursordist/docs/lua-config/index.rst @@ -1,4 +1,4 @@ -Lua Configuration: Trustanchors, Query Logging, RPZs, Sortlist and Zone to Cache +Advanced Configuration Using Lua ================================================================================ Since version 4.0.0, the PowerDNS Recursor supports additional configuration options that have to be loaded through :ref:`setting-lua-config-file`. diff --git a/pdns/syncres.cc b/pdns/syncres.cc index f78513420d..3eb784bd74 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -162,9 +162,9 @@ void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AdditionalMo if (shouldValidate() && state != vState::Secure && state != vState::Insecure) { return; } - for (const auto& rec : addRecords) { + for (auto& rec : addRecords) { if (rec.d_place == DNSResourceRecord::ANSWER) { - additionals.push_back(rec); + additionals.push_back(std::move(rec)); } } break; @@ -185,13 +185,13 @@ void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AdditionalMo for (auto& rec : addRecords) { if (rec.d_place == DNSResourceRecord::ANSWER) { rec.d_ttl -= d_now.tv_sec ; - additionals.push_back(rec); + additionals.push_back(std::move(rec)); } } break; } case AdditionalMode::ResolveDeferred: - // Not yet implemented + // FIXME: Not yet implemented // Look in cache for authoritative answer, if available return it // If not, look in nergache and submit if not there as well. The logic should be the same as // #11294, which is in review atm. @@ -201,10 +201,10 @@ void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AdditionalMo } } -// The main (recursive) function to add additonals +// The main (recursive) function to add additionals // qtype: the original query type to expand // start: records to start from -// This function uses to state sets to avoid infinite recursion +// This function uses to state sets to avoid infinite recursion and allow depulication // depth is the main recursion depth // additionaldepth is the depth for addAdditionals itself void SyncRes::addAdditionals(QType qtype, const vector&start, vector&additionals, std::set>& uniqueCalls, std::set>& uniqueResults, unsigned int depth, unsigned additionaldepth) @@ -228,12 +228,17 @@ void SyncRes::addAdditionals(QType qtype, const vector&start, vector< } } + // We maintain two sets for deduplication: + // - uniqueCalls makes sure we never resolve a qname/qtype twice + // - uniqueResults makes sure we never add the same qname/qytype RRSet to the result twice, + // but note that that set might contain multiple elements. + auto mode = it->second.second; for (const auto& targettype : it->second.first) { for (const auto& addname : addnames) { std::vector records; - if (uniqueCalls.count(std::pair(addname, targettype)) == 0) { - uniqueCalls.emplace(addname, targettype); + bool inserted = uniqueCalls.emplace(addname, targettype).second; + if (inserted) { resolveAdditionals(addname, targettype, mode, records, depth); } if (!records.empty()) { @@ -284,7 +289,7 @@ void SyncRes::addAdditionals(QType qtype, vector&ret, unsigned int de for (auto& rec : additionals) { rec.d_place = DNSResourceRecord::ADDITIONAL; - ret.push_back(rec); + ret.push_back(std::move(rec)); } } @@ -337,6 +342,7 @@ int SyncRes::beginResolve(const DNSName &qname, const QType qtype, QClass qclass } } + // Avoid calling addAdditionals() if we know we won't find anything auto luaLocal = g_luaconfs.getLocal(); if (res == 0 && qclass == QClass::IN && luaLocal->allowAdditionalQTypes.find(qtype) != luaLocal->allowAdditionalQTypes.end()) { addAdditionals(qtype, ret, depth); @@ -3225,7 +3231,7 @@ static void allowAdditionalEntry(std::unordered_set& allowedAdditionals allowedAdditionals.insert(target); } else { - // Alias mode not implemented yet + // FIXME: Alias mode not implemented yet } } break; diff --git a/regression-tests.recursor-dnssec/test_Additionals.py b/regression-tests.recursor-dnssec/test_Additionals.py index 637b61a35a..49a14d081b 100644 --- a/regression-tests.recursor-dnssec/test_Additionals.py +++ b/regression-tests.recursor-dnssec/test_Additionals.py @@ -47,9 +47,9 @@ class testAdditionalsResolveImmediately(RecursorTest): disable-packetcache """ _lua_config_file = """ - addAllowedAdditionalQType(pdns.MX, {pdns.A, pdns.AAAA}, { mode = "ResolveImmediately"}) - addAllowedAdditionalQType(pdns.NAPTR, {pdns.A, pdns.AAAA, pdns.SRV}, { mode = "ResolveImmediately"}) - addAllowedAdditionalQType(pdns.SRV, {pdns.A, pdns.AAAA}, { mode = "ResolveImmediately"}) + addAllowedAdditionalQType(pdns.MX, {pdns.A, pdns.AAAA}, { mode = pdns.AdditionalMode.ResolveImmediately}) + addAllowedAdditionalQType(pdns.NAPTR, {pdns.A, pdns.AAAA, pdns.SRV}, { mode = pdns.AdditionalMode.ResolveImmediately}) + addAllowedAdditionalQType(pdns.SRV, {pdns.A, pdns.AAAA}, { mode = pdns.AdditionalMode.ResolveImmediately}) """ def testMX(self): @@ -109,7 +109,7 @@ class testAdditionalsResolveCacheOnly(RecursorTest): disable-packetcache """ _lua_config_file = """ - addAllowedAdditionalQType(pdns.MX, {pdns.A, pdns.AAAA}, { mode = "ResolveImmediately"}) + addAllowedAdditionalQType(pdns.MX, {pdns.A, pdns.AAAA}, { mode = pdns.AdditionalMode.ResolveImmediately}) """ def testMX(self): -- 2.47.2