From a8ba292be6a45bd71c1be6f1d5100c158b2d931a Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Thu, 5 Dec 2024 14:54:13 +0100 Subject: [PATCH 01/25] rec: mark test that depend on external servers and or are unreliable While there get rid of the special status of test_WellKnown.py and correct the way args are passed to pytest --- regression-tests.recursor-dnssec/pytest.ini | 4 ++++ regression-tests.recursor-dnssec/runtests | 8 ++++++-- regression-tests.recursor-dnssec/test_Chain.py | 2 ++ regression-tests.recursor-dnssec/test_ExtendedErrors.py | 6 ++++++ regression-tests.recursor-dnssec/test_SimpleDoT.py | 2 ++ .../test_SimpleForwardOverDoT.py | 2 ++ regression-tests.recursor-dnssec/test_WellKnown.py | 4 +++- 7 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 regression-tests.recursor-dnssec/pytest.ini diff --git a/regression-tests.recursor-dnssec/pytest.ini b/regression-tests.recursor-dnssec/pytest.ini new file mode 100644 index 0000000000000..261099a05f0e3 --- /dev/null +++ b/regression-tests.recursor-dnssec/pytest.ini @@ -0,0 +1,4 @@ +[pytest] +markers = + external: uses external web servers + unreliable_on_gh: test is fine when run locally, but shows issues on GitHub diff --git a/regression-tests.recursor-dnssec/runtests b/regression-tests.recursor-dnssec/runtests index adee05c461732..3543dd2b79c52 100755 --- a/regression-tests.recursor-dnssec/runtests +++ b/regression-tests.recursor-dnssec/runtests @@ -61,9 +61,13 @@ if ! "$PDNSRECURSOR" --version 2>&1 | grep Features | grep -q dnstap-framestream export NODNSTAPTESTS=1 fi +# Run with -m 'not external' to skip test that require external connectivity +# Run with -m 'not unreliable_on_gh' to skip tests that are unreliable on GitHUb +# Run with -m 'not (external or unreliable_on_gh)' to skip both categories + # LIBFAKETIME is only added to LD_PRELOAD by the pyton code when needed if [ "${LIBASAN}" != "" -o "${LIBAUTHBIND}" != "" ]; then -LD_PRELOAD="${LIBASAN} ${LIBAUTHBIND}" pytest --ignore=test_WellKnown.py --junitxml=pytest.xml $@ +LD_PRELOAD="${LIBASAN} ${LIBAUTHBIND}" pytest --junitxml=pytest.xml "$@" else -pytest --ignore=test_WellKnown.py --junitxml=pytest.xml $@ +pytest --junitxml=pytest.xml "$@" fi diff --git a/regression-tests.recursor-dnssec/test_Chain.py b/regression-tests.recursor-dnssec/test_Chain.py index 6c20937b31e32..3c20da0fc7589 100644 --- a/regression-tests.recursor-dnssec/test_Chain.py +++ b/regression-tests.recursor-dnssec/test_Chain.py @@ -1,3 +1,4 @@ +import pytest import dns import os import time @@ -23,6 +24,7 @@ class ChainTest(RecursorTest): api-key=%s """ % (_wsPort, _wsPassword, _apiKey) + @pytest.mark.unreliable_on_gh def testBasic(self): """ Tests the case of #14624. Sending many equal requests could lead to ServFail because of clashing diff --git a/regression-tests.recursor-dnssec/test_ExtendedErrors.py b/regression-tests.recursor-dnssec/test_ExtendedErrors.py index 635c2c579c4c4..a684665a7935d 100644 --- a/regression-tests.recursor-dnssec/test_ExtendedErrors.py +++ b/regression-tests.recursor-dnssec/test_ExtendedErrors.py @@ -83,6 +83,7 @@ def generateRecursorConfig(cls, confdir): super(ExtendedErrorsTest, cls).generateRecursorConfig(confdir) + @pytest.mark.external def testNotIncepted(self): qname = 'signotincepted.bad-dnssec.wb.sidnlabs.nl.' query = dns.message.make_query(qname, 'A', want_dnssec=True) @@ -96,6 +97,7 @@ def testNotIncepted(self): self.assertEqual(res.options[0].otype, 15) self.assertEqual(res.options[0], extendederrors.ExtendedErrorOption(8, b'')) + @pytest.mark.external def testExpired(self): qname = 'sigexpired.bad-dnssec.wb.sidnlabs.nl.' query = dns.message.make_query(qname, 'A', want_dnssec=True) @@ -109,6 +111,7 @@ def testExpired(self): self.assertEqual(res.options[0].otype, 15) self.assertEqual(res.options[0], extendederrors.ExtendedErrorOption(7, b'')) + @pytest.mark.external def testAllExpired(self): qname = 'servfail.nl.' query = dns.message.make_query(qname, 'AAAA', want_dnssec=True) @@ -122,6 +125,7 @@ def testAllExpired(self): self.assertEqual(res.options[0].otype, 15) self.assertEqual(res.options[0], extendederrors.ExtendedErrorOption(6, b'')) + @pytest.mark.external def testBogus(self): qname = 'bogussig.ok.bad-dnssec.wb.sidnlabs.nl.' query = dns.message.make_query(qname, 'A', want_dnssec=True) @@ -135,6 +139,7 @@ def testBogus(self): self.assertEqual(res.options[0].otype, 15) self.assertEqual(res.options[0], extendederrors.ExtendedErrorOption(6, b'')) + @pytest.mark.external def testMissingRRSIG(self): qname = 'brokendnssec.net.' query = dns.message.make_query(qname, 'A', want_dnssec=True) @@ -236,6 +241,7 @@ def tearDownClass(cls): def generateRecursorConfig(cls, confdir): super(NoExtendedErrorsTest, cls).generateRecursorConfig(confdir) + @pytest.mark.external def testNotIncepted(self): qname = 'signotincepted.bad-dnssec.wb.sidnlabs.nl.' query = dns.message.make_query(qname, 'A', want_dnssec=True) diff --git a/regression-tests.recursor-dnssec/test_SimpleDoT.py b/regression-tests.recursor-dnssec/test_SimpleDoT.py index 4802e4ef4c5c3..20eaea132d295 100644 --- a/regression-tests.recursor-dnssec/test_SimpleDoT.py +++ b/regression-tests.recursor-dnssec/test_SimpleDoT.py @@ -1,3 +1,4 @@ +import pytest import dns import os import subprocess @@ -30,6 +31,7 @@ def setUpClass(cls): cls.generateRecursorConfig(confdir) cls.startRecursor(confdir, cls._recursorPort) + @pytest.mark.external def testTXT(self): expected = dns.rrset.from_text('dot-test-target.powerdns.org.', 0, dns.rdataclass.IN, 'TXT', 'https://github.com/PowerDNS/pdns/pull/12825') query = dns.message.make_query('dot-test-target.powerdns.org', 'TXT', want_dnssec=True) diff --git a/regression-tests.recursor-dnssec/test_SimpleForwardOverDoT.py b/regression-tests.recursor-dnssec/test_SimpleForwardOverDoT.py index d7b85e648c21e..8de66060b5d8a 100644 --- a/regression-tests.recursor-dnssec/test_SimpleForwardOverDoT.py +++ b/regression-tests.recursor-dnssec/test_SimpleForwardOverDoT.py @@ -1,3 +1,4 @@ +import pytest import dns import os import subprocess @@ -27,6 +28,7 @@ def setUpClass(cls): cls.generateRecursorConfig(confdir) cls.startRecursor(confdir, cls._recursorPort) + @pytest.mark.external def testA(self): expected = dns.rrset.from_text('dns.google.', 0, dns.rdataclass.IN, 'A', '8.8.8.8', '8.8.4.4') query = dns.message.make_query('dns.google', 'A', want_dnssec=True) diff --git a/regression-tests.recursor-dnssec/test_WellKnown.py b/regression-tests.recursor-dnssec/test_WellKnown.py index feaaa6f29d0e5..fa04a9692c924 100644 --- a/regression-tests.recursor-dnssec/test_WellKnown.py +++ b/regression-tests.recursor-dnssec/test_WellKnown.py @@ -1,7 +1,9 @@ +import pytest import dns from recursortests import RecursorTest -class TestWellKnown(RecursorTest): +@pytest.mark.external +class WellKnownTest(RecursorTest): _auths_zones = None _confdir = 'WellKnown' _roothints = None From 863c7565211e0816f698ffbb48675a5f8e25f58f Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Wed, 4 Dec 2024 16:22:41 +0100 Subject: [PATCH 02/25] Handle empty ipv*hint values as "auto", when coming from lmdb. Fixes #12653 --- modules/lmdbbackend/lmdbbackend.cc | 2 +- pdns/dnsparser.cc | 11 ++++++++--- pdns/dnsparser.hh | 7 ++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 36841e619a683..8b6860ee5a089 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1009,7 +1009,7 @@ static std::shared_ptr deserializeContentZR(uint16_t qtype, co if (qtype == QType::A && content.size() == 4) { return std::make_shared(*((uint32_t*)content.c_str())); } - return DNSRecordContent::deserialize(qname, qtype, content); + return DNSRecordContent::deserialize(qname, qtype, content, QClass::IN, true); } /* design. If you ask a question without a zone id, we lookup the best diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc index e1f81a14d3e86..5dd2de0ce9e34 100644 --- a/pdns/dnsparser.cc +++ b/pdns/dnsparser.cc @@ -80,7 +80,7 @@ void UnknownRecordContent::toPacket(DNSPacketWriter& pw) const pw.xfrBlob(string(d_record.begin(),d_record.end())); } -shared_ptr DNSRecordContent::deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass) +shared_ptr DNSRecordContent::deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass, bool trusted) { dnsheader dnsheader; memset(&dnsheader, 0, sizeof(dnsheader)); @@ -118,7 +118,7 @@ shared_ptr DNSRecordContent::deserialize(const DNSName& qname, dr.d_type = qtype; dr.d_name = qname; dr.d_clen = serialized.size(); - PacketReader pr(std::string_view(reinterpret_cast(packet.data()), packet.size()), packet.size() - serialized.size() - sizeof(dnsrecordheader)); + PacketReader pr(std::string_view(reinterpret_cast(packet.data()), packet.size()), packet.size() - serialized.size() - sizeof(dnsrecordheader), trusted); /* needed to get the record boundaries right */ pr.getDnsrecordheader(drh); auto content = DNSRecordContent::make(dr, pr, Opcode::Query); @@ -665,7 +665,12 @@ void PacketReader::xfrSvcParamKeyVals(set &kvs) { xfrCAWithoutPort(key, addr); addresses.push_back(addr); } - kvs.insert(SvcParam(key, std::move(addresses))); + // If there were no addresses, and the input comes from a trusted source, + // we can reasonably suppose this is the serialization of "auto". + bool doAuto{d_trusted && len == 0}; + auto param = SvcParam(key, std::move(addresses)); + param.setAutoHint(doAuto); + kvs.insert(param); break; } case SvcParam::ech: { diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh index 21ad0205c14a7..ae9778a7d7fbc 100644 --- a/pdns/dnsparser.hh +++ b/pdns/dnsparser.hh @@ -68,8 +68,8 @@ class MOADNSParser; class PacketReader { public: - PacketReader(const std::string_view& content, uint16_t initialPos=sizeof(dnsheader)) - : d_pos(initialPos), d_startrecordpos(initialPos), d_content(content) + PacketReader(const std::string_view& content, uint16_t initialPos=sizeof(dnsheader), bool trusted = false) + : d_pos(initialPos), d_startrecordpos(initialPos), d_content(content), d_trusted(trusted) { if(content.size() > std::numeric_limits::max()) throw std::out_of_range("packet too large"); @@ -186,6 +186,7 @@ private: uint16_t d_recordlen; // ditto uint16_t not_used; // Aligns the whole class on 8-byte boundaries const std::string_view d_content; + bool d_trusted; }; struct DNSRecord; @@ -226,7 +227,7 @@ public: } // parse the content in wire format, possibly including compressed pointers pointing to the owner name - static shared_ptr deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass=QClass::IN); + static shared_ptr deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass=QClass::IN, bool trusted = false); void doRecordCheck(const struct DNSRecord&){} From 3332fd455ba7b256702f231dd302f8301cb9e443 Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Fri, 6 Dec 2024 11:10:41 +0100 Subject: [PATCH 03/25] Address review and clang-tidy comments. --- pdns/dnsparser.cc | 16 +++++++++------- pdns/dnsparser.hh | 12 +++++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc index 5dd2de0ce9e34..12974ae57cea7 100644 --- a/pdns/dnsparser.cc +++ b/pdns/dnsparser.cc @@ -80,7 +80,7 @@ void UnknownRecordContent::toPacket(DNSPacketWriter& pw) const pw.xfrBlob(string(d_record.begin(),d_record.end())); } -shared_ptr DNSRecordContent::deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass, bool trusted) +shared_ptr DNSRecordContent::deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass, bool internalRepresentation) { dnsheader dnsheader; memset(&dnsheader, 0, sizeof(dnsheader)); @@ -118,10 +118,11 @@ shared_ptr DNSRecordContent::deserialize(const DNSName& qname, dr.d_type = qtype; dr.d_name = qname; dr.d_clen = serialized.size(); - PacketReader pr(std::string_view(reinterpret_cast(packet.data()), packet.size()), packet.size() - serialized.size() - sizeof(dnsrecordheader), trusted); + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast): packet.data() is uint8_t * + PacketReader reader(std::string_view(reinterpret_cast(packet.data()), packet.size()), packet.size() - serialized.size() - sizeof(dnsrecordheader), internalRepresentation); /* needed to get the record boundaries right */ - pr.getDnsrecordheader(drh); - auto content = DNSRecordContent::make(dr, pr, Opcode::Query); + reader.getDnsrecordheader(drh); + auto content = DNSRecordContent::make(dr, reader, Opcode::Query); return content; } @@ -665,9 +666,10 @@ void PacketReader::xfrSvcParamKeyVals(set &kvs) { xfrCAWithoutPort(key, addr); addresses.push_back(addr); } - // If there were no addresses, and the input comes from a trusted source, - // we can reasonably suppose this is the serialization of "auto". - bool doAuto{d_trusted && len == 0}; + // If there were no addresses, and the input comes from internal + // representation, we can reasonably assume this is the serialization + // of "auto". + bool doAuto{d_internal && len == 0}; auto param = SvcParam(key, std::move(addresses)); param.setAutoHint(doAuto); kvs.insert(param); diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh index ae9778a7d7fbc..fd2f5b7112f3a 100644 --- a/pdns/dnsparser.hh +++ b/pdns/dnsparser.hh @@ -68,8 +68,8 @@ class MOADNSParser; class PacketReader { public: - PacketReader(const std::string_view& content, uint16_t initialPos=sizeof(dnsheader), bool trusted = false) - : d_pos(initialPos), d_startrecordpos(initialPos), d_content(content), d_trusted(trusted) + PacketReader(const std::string_view& content, uint16_t initialPos=sizeof(dnsheader), bool internalRepresentation = false) + : d_pos(initialPos), d_startrecordpos(initialPos), d_content(content), d_internal(internalRepresentation) { if(content.size() > std::numeric_limits::max()) throw std::out_of_range("packet too large"); @@ -186,7 +186,7 @@ private: uint16_t d_recordlen; // ditto uint16_t not_used; // Aligns the whole class on 8-byte boundaries const std::string_view d_content; - bool d_trusted; + bool d_internal; }; struct DNSRecord; @@ -226,8 +226,10 @@ public: return typeid(*this)==typeid(rhs) && this->getZoneRepresentation() == rhs.getZoneRepresentation(); } - // parse the content in wire format, possibly including compressed pointers pointing to the owner name - static shared_ptr deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass=QClass::IN, bool trusted = false); + // parse the content in wire format, possibly including compressed pointers pointing to the owner name. + // internalRepresentation is set when the data comes from an internal source, + // such as the LMDB backend. + static shared_ptr deserialize(const DNSName& qname, uint16_t qtype, const string& serialized, uint16_t qclass=QClass::IN, bool internalRepresentation = false); void doRecordCheck(const struct DNSRecord&){} From cd5b9e686657ebf9b296015f137a82b976dcfd91 Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Wed, 11 Dec 2024 15:04:07 +0100 Subject: [PATCH 04/25] Extend the SVCB tests to also test with lmdb backend. --- regression-tests.auth-py/test_SVCB.py | 156 ++++++++++++++++++++++++-- 1 file changed, 147 insertions(+), 9 deletions(-) diff --git a/regression-tests.auth-py/test_SVCB.py b/regression-tests.auth-py/test_SVCB.py index 7bb10ac4ce79f..12f35c4b67190 100644 --- a/regression-tests.auth-py/test_SVCB.py +++ b/regression-tests.auth-py/test_SVCB.py @@ -1,10 +1,23 @@ from authtests import AuthTest import dns +import os +import subprocess +class SVCBRecordsBase(AuthTest): + # Copied from AuthTest, without the bind-config and bind-dnssec fields. + _config_template_default = """ +module-dir={PDNS_MODULE_DIR} +daemon=no +socket-dir={confdir} +cache-ttl=0 +negquery-cache-ttl=0 +query-cache-ttl=0 +log-dns-queries=yes +log-dns-details=yes +loglevel=9 +distributor-threads=1""" -class TestSVCBRecords(AuthTest): _config_template = """ -launch=bind svc-autohints """ @@ -39,7 +52,7 @@ class TestSVCBRecords(AuthTest): """, } - def testWithoutAlias(self): + def impl_testWithoutAlias(self): query = dns.message.make_query('www.example.org', 'HTTPS') res = self.sendUDPQuery(query) expected_ans = dns.rrset.from_text( @@ -50,7 +63,7 @@ def testWithoutAlias(self): self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 2) - def testWithAlias(self): + def impl_testWithAlias(self): """ Ensure additional processing happens for HTTPS AliasMode """ @@ -70,7 +83,7 @@ def testWithAlias(self): self.assertRRsetInAdditional(res, expected_addl) self.assertEqual(len(res.additional), 3) - def testWithMissingA(self): + def impl_testWithMissingA(self): """ Ensure PowerDNS removes the ipv4hint if there's no A record """ @@ -84,7 +97,7 @@ def testWithMissingA(self): self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 1) - def testWithMissingAAAA(self): + def impl_testWithMissingAAAA(self): """ Ensure PowerDNS removes the ipv6hint if there's no AAAA record """ @@ -98,7 +111,7 @@ def testWithMissingAAAA(self): self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 1) - def testNoAuto(self): + def impl_testNoAuto(self): """ Ensure we send the actual hints, not generated ones """ @@ -113,7 +126,7 @@ def testNoAuto(self): self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 2) - def testAutoA(self): + def impl_testAutoA(self): """ Ensure we send a generated A hint, but keep the existing AAAA hint """ @@ -128,7 +141,7 @@ def testAutoA(self): self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 2) - def testAutoAAAA(self): + def impl_testAutoAAAA(self): """ Ensure we send a generated AAAA hint, but keep the existing A hint """ @@ -142,3 +155,128 @@ def testAutoAAAA(self): print(res) self.assertRRsetInAnswer(res, expected_ans) self.assertEqual(len(res.additional), 2) + +class TestSVCBRecordsBind(SVCBRecordsBase): + _config_template_default = ( + SVCBRecordsBase._config_template_default + + """ +bind-config={confdir}/named.conf +bind-dnssec-db={bind_dnssec_db} +""" + ) + + _config_template = ( + SVCBRecordsBase._config_template + + """ +launch=bind +""" + ) + + def testWithoutAlias(self): + self.impl_testWithoutAlias() + + def testWithAlias(self): + """ + Ensure additional processing happens for HTTPS AliasMode + """ + self.impl_testWithAlias() + + def testWithMissingA(self): + """ + Ensure PowerDNS removes the ipv4hint if there's no A record + """ + self.impl_testWithMissingA() + + def testWithMissingAAAA(self): + """ + Ensure PowerDNS removes the ipv6hint if there's no AAAA record + """ + self.impl_testWithMissingAAAA() + + def testNoAuto(self): + """ + Ensure we send the actual hints, not generated ones + """ + self.impl_testNoAuto() + + def testAutoA(self): + """ + Ensure we send a generated A hint, but keep the existing AAAA hint + """ + self.impl_testAutoA() + + def testAutoAAAA(self): + """ + Ensure we send a generated AAAA hint, but keep the existing A hint + """ + self.impl_testAutoAAAA() + +class TestSVCBRecordsLMDB(SVCBRecordsBase): + _config_template = ( + SVCBRecordsBase._config_template + + """ +launch=lmdb +""" + ) + + @classmethod + def generateAllAuthConfig(cls, confdir): + # This is very similar to AuthTest.generateAllAuthConfig, + # but for lmdb backend, we ignore auth keys but need to load-zone + # into lmdb storage. + cls.generateAuthConfig(confdir) + + for zonename, zonecontent in cls._zones.items(): + cls.generateAuthZone(confdir, + zonename, + zonecontent) + pdnsutilCmd = [os.environ['PDNSUTIL'], + '--config-dir=%s' % confdir, + 'load-zone', + zonename, + os.path.join(confdir, '%s.zone' % zonename)] + + print(' '.join(pdnsutilCmd)) + try: + subprocess.check_output(pdnsutilCmd, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: + raise AssertionError('%s failed (%d): %s' % (pdnsutilCmd, e.returncode, e.output)) + + def testWithoutAlias(self): + self.impl_testWithoutAlias() + + def testWithAlias(self): + """ + Ensure additional processing happens for HTTPS AliasMode + """ + self.impl_testWithAlias() + + def testWithMissingA(self): + """ + Ensure PowerDNS removes the ipv4hint if there's no A record + """ + self.impl_testWithMissingA() + + def testWithMissingAAAA(self): + """ + Ensure PowerDNS removes the ipv6hint if there's no AAAA record + """ + self.impl_testWithMissingAAAA() + + def testNoAuto(self): + """ + Ensure we send the actual hints, not generated ones + """ + self.impl_testNoAuto() + + def testAutoA(self): + """ + Ensure we send a generated A hint, but keep the existing AAAA hint + """ + self.impl_testAutoA() + + def testAutoAAAA(self): + """ + Ensure we send a generated AAAA hint, but keep the existing A hint + """ + self.impl_testAutoAAAA() From b51f86701152b8972cc4637cbcbcf52ec4b73811 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 09:04:56 +0100 Subject: [PATCH 05/25] dnsdist: add support for labels on custom metrics This adds support for prometheus labels on custom metrics. Changes the metrics maps to hold a map of labels to metric values for each metric. Metrics without labels have an empty string label combination value. Adds an optional options table as the last parameter of `incMetric`, `decMetric`, `setMetric` and `getMetric`, which can be used to set labels by using the `label` key or to set the previously used parameter (either `step` or `value`). Closes: #13359 --- pdns/dnsdistdist/dnsdist-lua-ffi.cc | 15 +- pdns/dnsdistdist/dnsdist-lua.cc | 52 +++++- pdns/dnsdistdist/dnsdist-metrics.cc | 267 +++++++++++++++++----------- pdns/dnsdistdist/dnsdist-metrics.hh | 14 +- pdns/dnsdistdist/dnsdist-web.cc | 4 + 5 files changed, 225 insertions(+), 127 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index 192e8f1cfd10c..c7a0c055f6825 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -1811,7 +1811,8 @@ bool dnsdist_ffi_metric_declare(const char* name, size_t nameLen, const char* ty void dnsdist_ffi_metric_inc(const char* metricName, size_t metricNameLen) { - auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), 1U); + // TODO: add labels? + auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), 1U, {}); if (std::get_if(&result) != nullptr) { return; } @@ -1819,7 +1820,8 @@ void dnsdist_ffi_metric_inc(const char* metricName, size_t metricNameLen) void dnsdist_ffi_metric_inc_by(const char* metricName, size_t metricNameLen, uint64_t value) { - auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), value); + // TODO: add labels? + auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), value, {}); if (std::get_if(&result) != nullptr) { return; } @@ -1827,7 +1829,8 @@ void dnsdist_ffi_metric_inc_by(const char* metricName, size_t metricNameLen, uin void dnsdist_ffi_metric_dec(const char* metricName, size_t metricNameLen) { - auto result = dnsdist::metrics::decrementCustomCounter(std::string_view(metricName, metricNameLen), 1U); + // TODO: add labels? + auto result = dnsdist::metrics::decrementCustomCounter(std::string_view(metricName, metricNameLen), 1U, {}); if (std::get_if(&result) != nullptr) { return; } @@ -1835,7 +1838,8 @@ void dnsdist_ffi_metric_dec(const char* metricName, size_t metricNameLen) void dnsdist_ffi_metric_set(const char* metricName, size_t metricNameLen, double value) { - auto result = dnsdist::metrics::setCustomGauge(std::string_view(metricName, metricNameLen), value); + // TODO: add labels? + auto result = dnsdist::metrics::setCustomGauge(std::string_view(metricName, metricNameLen), value, {}); if (std::get_if(&result) != nullptr) { return; } @@ -1843,7 +1847,8 @@ void dnsdist_ffi_metric_set(const char* metricName, size_t metricNameLen, double double dnsdist_ffi_metric_get(const char* metricName, size_t metricNameLen, bool isCounter) { - auto result = dnsdist::metrics::getCustomMetric(std::string_view(metricName, metricNameLen)); + // TODO: add labels? + auto result = dnsdist::metrics::getCustomMetric(std::string_view(metricName, metricNameLen), {}); if (std::get_if(&result) != nullptr) { return 0.; } diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index 09818d982107a..53e1038c135c6 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -85,6 +85,8 @@ using std::thread; +using update_metric_opts_t = LuaAssociativeTable>>; + static boost::tribool s_noLuaSideEffect; /* this is a best effort way to prevent logging calls with no side-effects in the output of delta() @@ -3419,8 +3421,20 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return true; }); - luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional step) { - auto result = dnsdist::metrics::incrementCustomCounter(name, step ? *step : 1); + luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional>> opts) { + auto incOpts = opts.get_value_or(1); + uint64_t step = 1; + std::unordered_map labels; + if (auto* custom_step = boost::get(&incOpts)) { + step = *custom_step; + } + else { + boost::optional vars = boost::get>(incOpts); + getOptionalValue(vars, "step", step); + getOptionalValue>(vars, "labels", labels); + checkAllParametersConsumed("incMetric", vars); + } + auto result = dnsdist::metrics::incrementCustomCounter(name, step, labels); if (const auto* errorStr = std::get_if(&result)) { g_outputBuffer = *errorStr + "'\n"; errlog("Error in incMetric: %s", *errorStr); @@ -3428,8 +3442,20 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional step) { - auto result = dnsdist::metrics::decrementCustomCounter(name, step ? *step : 1); + luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional>> opts) { + auto decOpts = opts.get_value_or(1); + uint64_t step = 1; + std::unordered_map labels; + if (auto custom_step = boost::get(&decOpts)) { + step = *custom_step; + } + else { + boost::optional vars = boost::get>(decOpts); + getOptionalValue(vars, "step", step); + getOptionalValue>(vars, "labels", labels); + checkAllParametersConsumed("decMetric", vars); + } + auto result = dnsdist::metrics::decrementCustomCounter(name, step, labels); if (const auto* errorStr = std::get_if(&result)) { g_outputBuffer = *errorStr + "'\n"; errlog("Error in decMetric: %s", *errorStr); @@ -3437,8 +3463,13 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("setMetric", [](const std::string& name, const double value) -> double { - auto result = dnsdist::metrics::setCustomGauge(name, value); + luaCtx.writeFunction("setMetric", [](const std::string& name, const double value, boost::optional opts) -> double { + std::unordered_map labels; + if (opts) { + getOptionalValue>(opts, "labels", labels); + } + checkAllParametersConsumed("setMetric", opts); + auto result = dnsdist::metrics::setCustomGauge(name, value, labels); if (const auto* errorStr = std::get_if(&result)) { g_outputBuffer = *errorStr + "'\n"; errlog("Error in setMetric: %s", *errorStr); @@ -3446,8 +3477,13 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("getMetric", [](const std::string& name) { - auto result = dnsdist::metrics::getCustomMetric(name); + luaCtx.writeFunction("getMetric", [](const std::string& name, boost::optional opts) { + std::unordered_map labels; + if (opts) { + getOptionalValue>(opts, "labels", labels); + } + checkAllParametersConsumed("getMetric", opts); + auto result = dnsdist::metrics::getCustomMetric(name, labels); if (const auto* errorStr = std::get_if(&result)) { g_outputBuffer = *errorStr + "'\n"; errlog("Error in getMetric: %s", *errorStr); diff --git a/pdns/dnsdistdist/dnsdist-metrics.cc b/pdns/dnsdistdist/dnsdist-metrics.cc index 434088df4e2f1..039b1ffc89249 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.cc +++ b/pdns/dnsdistdist/dnsdist-metrics.cc @@ -19,6 +19,8 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include +#include #include #include "dnsdist-metrics.hh" @@ -67,100 +69,100 @@ struct MutableGauge mutable pdns::stat_double_t d_value{0}; }; -static SharedLockGuarded>> s_customCounters; -static SharedLockGuarded>> s_customGauges; +static SharedLockGuarded, std::less<>>> s_customCounters; +static SharedLockGuarded, std::less<>>> s_customGauges; Stats::Stats() : - entries{std::vector{ - {"responses", &responses}, - {"servfail-responses", &servfailResponses}, - {"queries", &queries}, - {"frontend-nxdomain", &frontendNXDomain}, - {"frontend-servfail", &frontendServFail}, - {"frontend-noerror", &frontendNoError}, - {"acl-drops", &aclDrops}, - {"rule-drop", &ruleDrop}, - {"rule-nxdomain", &ruleNXDomain}, - {"rule-refused", &ruleRefused}, - {"rule-servfail", &ruleServFail}, - {"rule-truncated", &ruleTruncated}, - {"self-answered", &selfAnswered}, - {"downstream-timeouts", &downstreamTimeouts}, - {"downstream-send-errors", &downstreamSendErrors}, - {"trunc-failures", &truncFail}, - {"no-policy", &noPolicy}, - {"latency0-1", &latency0_1}, - {"latency1-10", &latency1_10}, - {"latency10-50", &latency10_50}, - {"latency50-100", &latency50_100}, - {"latency100-1000", &latency100_1000}, - {"latency-slow", &latencySlow}, - {"latency-avg100", &latencyAvg100}, - {"latency-avg1000", &latencyAvg1000}, - {"latency-avg10000", &latencyAvg10000}, - {"latency-avg1000000", &latencyAvg1000000}, - {"latency-tcp-avg100", &latencyTCPAvg100}, - {"latency-tcp-avg1000", &latencyTCPAvg1000}, - {"latency-tcp-avg10000", &latencyTCPAvg10000}, - {"latency-tcp-avg1000000", &latencyTCPAvg1000000}, - {"latency-dot-avg100", &latencyDoTAvg100}, - {"latency-dot-avg1000", &latencyDoTAvg1000}, - {"latency-dot-avg10000", &latencyDoTAvg10000}, - {"latency-dot-avg1000000", &latencyDoTAvg1000000}, - {"latency-doh-avg100", &latencyDoHAvg100}, - {"latency-doh-avg1000", &latencyDoHAvg1000}, - {"latency-doh-avg10000", &latencyDoHAvg10000}, - {"latency-doh-avg1000000", &latencyDoHAvg1000000}, - {"latency-doq-avg100", &latencyDoQAvg100}, - {"latency-doq-avg1000", &latencyDoQAvg1000}, - {"latency-doq-avg10000", &latencyDoQAvg10000}, - {"latency-doq-avg1000000", &latencyDoQAvg1000000}, - {"latency-doh3-avg100", &latencyDoH3Avg100}, - {"latency-doh3-avg1000", &latencyDoH3Avg1000}, - {"latency-doh3-avg10000", &latencyDoH3Avg10000}, - {"latency-doh3-avg1000000", &latencyDoH3Avg1000000}, - {"uptime", uptimeOfProcess}, - {"real-memory-usage", getRealMemoryUsage}, - {"special-memory-usage", getSpecialMemoryUsage}, - {"udp-in-errors", [](const std::string&) { return udpErrorStats("udp-in-errors"); }}, - {"udp-noport-errors", [](const std::string&) { return udpErrorStats("udp-noport-errors"); }}, - {"udp-recvbuf-errors", [](const std::string&) { return udpErrorStats("udp-recvbuf-errors"); }}, - {"udp-sndbuf-errors", [](const std::string&) { return udpErrorStats("udp-sndbuf-errors"); }}, - {"udp-in-csum-errors", [](const std::string&) { return udpErrorStats("udp-in-csum-errors"); }}, - {"udp6-in-errors", [](const std::string&) { return udp6ErrorStats("udp6-in-errors"); }}, - {"udp6-recvbuf-errors", [](const std::string&) { return udp6ErrorStats("udp6-recvbuf-errors"); }}, - {"udp6-sndbuf-errors", [](const std::string&) { return udp6ErrorStats("udp6-sndbuf-errors"); }}, - {"udp6-noport-errors", [](const std::string&) { return udp6ErrorStats("udp6-noport-errors"); }}, - {"udp6-in-csum-errors", [](const std::string&) { return udp6ErrorStats("udp6-in-csum-errors"); }}, - {"tcp-listen-overflows", [](const std::string&) { return tcpErrorStats("ListenOverflows"); }}, - {"noncompliant-queries", &nonCompliantQueries}, - {"noncompliant-responses", &nonCompliantResponses}, - {"proxy-protocol-invalid", &proxyProtocolInvalid}, - {"rdqueries", &rdQueries}, - {"empty-queries", &emptyQueries}, - {"cache-hits", &cacheHits}, - {"cache-misses", &cacheMisses}, - {"cpu-iowait", getCPUIOWait}, - {"cpu-steal", getCPUSteal}, - {"cpu-sys-msec", getCPUTimeSystem}, - {"cpu-user-msec", getCPUTimeUser}, - {"fd-usage", getOpenFileDescriptors}, - {"dyn-blocked", &dynBlocked}, + entries{std::vector{ + {"responses", "", &responses}, + {"servfail-responses", "", &servfailResponses}, + {"queries", "", &queries}, + {"frontend-nxdomain", "", &frontendNXDomain}, + {"frontend-servfail", "", &frontendServFail}, + {"frontend-noerror", "", &frontendNoError}, + {"acl-drops", "", &aclDrops}, + {"rule-drop", "", &ruleDrop}, + {"rule-nxdomain", "", &ruleNXDomain}, + {"rule-refused", "", &ruleRefused}, + {"rule-servfail", "", &ruleServFail}, + {"rule-truncated", "", &ruleTruncated}, + {"self-answered", "", &selfAnswered}, + {"downstream-timeouts", "", &downstreamTimeouts}, + {"downstream-send-errors", "", &downstreamSendErrors}, + {"trunc-failures", "", &truncFail}, + {"no-policy", "", &noPolicy}, + {"latency0-1", "", &latency0_1}, + {"latency1-10", "", &latency1_10}, + {"latency10-50", "", &latency10_50}, + {"latency50-100", "", &latency50_100}, + {"latency100-1000", "", &latency100_1000}, + {"latency-slow", "", &latencySlow}, + {"latency-avg100", "", &latencyAvg100}, + {"latency-avg1000", "", &latencyAvg1000}, + {"latency-avg10000", "", &latencyAvg10000}, + {"latency-avg1000000", "", &latencyAvg1000000}, + {"latency-tcp-avg100", "", &latencyTCPAvg100}, + {"latency-tcp-avg1000", "", &latencyTCPAvg1000}, + {"latency-tcp-avg10000", "", &latencyTCPAvg10000}, + {"latency-tcp-avg1000000", "", &latencyTCPAvg1000000}, + {"latency-dot-avg100", "", &latencyDoTAvg100}, + {"latency-dot-avg1000", "", &latencyDoTAvg1000}, + {"latency-dot-avg10000", "", &latencyDoTAvg10000}, + {"latency-dot-avg1000000", "", &latencyDoTAvg1000000}, + {"latency-doh-avg100", "", &latencyDoHAvg100}, + {"latency-doh-avg1000", "", &latencyDoHAvg1000}, + {"latency-doh-avg10000", "", &latencyDoHAvg10000}, + {"latency-doh-avg1000000", "", &latencyDoHAvg1000000}, + {"latency-doq-avg100", "", &latencyDoQAvg100}, + {"latency-doq-avg1000", "", &latencyDoQAvg1000}, + {"latency-doq-avg10000", "", &latencyDoQAvg10000}, + {"latency-doq-avg1000000", "", &latencyDoQAvg1000000}, + {"latency-doh3-avg100", "", &latencyDoH3Avg100}, + {"latency-doh3-avg1000", "", &latencyDoH3Avg1000}, + {"latency-doh3-avg10000", "", &latencyDoH3Avg10000}, + {"latency-doh3-avg1000000", "", &latencyDoH3Avg1000000}, + {"uptime", "", uptimeOfProcess}, + {"real-memory-usage", "", getRealMemoryUsage}, + {"special-memory-usage", "", getSpecialMemoryUsage}, + {"udp-in-errors", "", [](const std::string&) { return udpErrorStats("udp-in-errors"); }}, + {"udp-noport-errors", "", [](const std::string&) { return udpErrorStats("udp-noport-errors"); }}, + {"udp-recvbuf-errors", "", [](const std::string&) { return udpErrorStats("udp-recvbuf-errors"); }}, + {"udp-sndbuf-errors", "", [](const std::string&) { return udpErrorStats("udp-sndbuf-errors"); }}, + {"udp-in-csum-errors", "", [](const std::string&) { return udpErrorStats("udp-in-csum-errors"); }}, + {"udp6-in-errors", "", [](const std::string&) { return udp6ErrorStats("udp6-in-errors"); }}, + {"udp6-recvbuf-errors", "", [](const std::string&) { return udp6ErrorStats("udp6-recvbuf-errors"); }}, + {"udp6-sndbuf-errors", "", [](const std::string&) { return udp6ErrorStats("udp6-sndbuf-errors"); }}, + {"udp6-noport-errors", "", [](const std::string&) { return udp6ErrorStats("udp6-noport-errors"); }}, + {"udp6-in-csum-errors", "", [](const std::string&) { return udp6ErrorStats("udp6-in-csum-errors"); }}, + {"tcp-listen-overflows", "", [](const std::string&) { return tcpErrorStats("ListenOverflows"); }}, + {"noncompliant-queries", "", &nonCompliantQueries}, + {"noncompliant-responses", "", &nonCompliantResponses}, + {"proxy-protocol-invalid", "", &proxyProtocolInvalid}, + {"rdqueries", "", &rdQueries}, + {"empty-queries", "", &emptyQueries}, + {"cache-hits", "", &cacheHits}, + {"cache-misses", "", &cacheMisses}, + {"cpu-iowait", "", getCPUIOWait}, + {"cpu-steal", "", getCPUSteal}, + {"cpu-sys-msec", "", getCPUTimeSystem}, + {"cpu-user-msec", "", getCPUTimeUser}, + {"fd-usage", "", getOpenFileDescriptors}, + {"dyn-blocked", "", &dynBlocked}, #ifndef DISABLE_DYNBLOCKS - {"dyn-block-nmg-size", [](const std::string&) { return dnsdist::DynamicBlocks::getClientAddressDynamicRules().size(); }}, + {"dyn-block-nmg-size", "", [](const std::string&) { return dnsdist::DynamicBlocks::getClientAddressDynamicRules().size(); }}, #endif /* DISABLE_DYNBLOCKS */ - {"security-status", &securityStatus}, - {"doh-query-pipe-full", &dohQueryPipeFull}, - {"doh-response-pipe-full", &dohResponsePipeFull}, - {"doq-response-pipe-full", &doqResponsePipeFull}, - {"doh3-response-pipe-full", &doh3ResponsePipeFull}, - {"outgoing-doh-query-pipe-full", &outgoingDoHQueryPipeFull}, - {"tcp-query-pipe-full", &tcpQueryPipeFull}, - {"tcp-cross-protocol-query-pipe-full", &tcpCrossProtocolQueryPipeFull}, - {"tcp-cross-protocol-response-pipe-full", &tcpCrossProtocolResponsePipeFull}, + {"security-status", "", &securityStatus}, + {"doh-query-pipe-full", "", &dohQueryPipeFull}, + {"doh-response-pipe-full", "", &dohResponsePipeFull}, + {"doq-response-pipe-full", "", &doqResponsePipeFull}, + {"doh3-response-pipe-full", "", &doh3ResponsePipeFull}, + {"outgoing-doh-query-pipe-full", "", &outgoingDoHQueryPipeFull}, + {"tcp-query-pipe-full", "", &tcpQueryPipeFull}, + {"tcp-cross-protocol-query-pipe-full", "", &tcpCrossProtocolQueryPipeFull}, + {"tcp-cross-protocol-response-pipe-full", "", &tcpCrossProtocolResponsePipeFull}, // Latency histogram - {"latency-sum", &latencySum}, - {"latency-count", &latencyCount}, + {"latency-sum", "", &latencySum}, + {"latency-count", "", &latencyCount}, }} { } @@ -176,18 +178,16 @@ std::optional declareCustomMetric(const std::string& name, const st const std::string finalCustomName(customName ? *customName : ""); if (type == "counter") { auto customCounters = s_customCounters.write_lock(); - auto itp = customCounters->insert({name, MutableCounter()}); + auto itp = customCounters->emplace(name, std::map()); if (itp.second) { - g_stats.entries.write_lock()->emplace_back(Stats::EntryPair{name, &(*customCounters)[name].d_value}); dnsdist::prometheus::PrometheusMetricDefinition def{name, type, description, finalCustomName}; dnsdist::webserver::addMetricDefinition(def); } } else if (type == "gauge") { auto customGauges = s_customGauges.write_lock(); - auto itp = customGauges->insert({name, MutableGauge()}); + auto itp = customGauges->emplace(name, std::map()); if (itp.second) { - g_stats.entries.write_lock()->emplace_back(Stats::EntryPair{name, &(*customGauges)[name].d_value}); dnsdist::prometheus::PrometheusMetricDefinition def{name, type, description, finalCustomName}; dnsdist::webserver::addMetricDefinition(def); } @@ -198,57 +198,108 @@ std::optional declareCustomMetric(const std::string& name, const st return std::nullopt; } -std::variant incrementCustomCounter(const std::string_view& name, uint64_t step) +static string prometheusLabelValueEscape(const string& value) { - auto customCounters = s_customCounters.read_lock(); + string ret; + + for (char i : value) { + if (i == '"' || i == '\\') { + ret += '\\'; + ret += i; + } + else if (i == '\n') { + ret += '\\'; + ret += 'n'; + } + else + ret += i; + } + return ret; +} + +static std::string generateCombinationOfLabels(const std::unordered_map& labels) +{ + return std::accumulate(labels.begin(), labels.end(), std::string(), [](const std::string& acc, const std::pair& l) { + return acc + (acc.empty() ? std::string() : ",") + l.first + "=" + "\"" + prometheusLabelValueEscape(l.second) + "\""; + }); +} + +std::variant incrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels) +{ + auto customCounters = s_customCounters.write_lock(); auto metric = customCounters->find(name); if (metric != customCounters->end()) { - metric->second.d_value += step; - return metric->second.d_value.load(); + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = metric->second.find(combinationOfLabels); + if (metricEntry == metric->second.end()) { + metricEntry = metric->second.emplace(combinationOfLabels, MutableCounter()).first; + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); + } + metricEntry->second.d_value += step; + return metricEntry->second.d_value.load(); } return std::string("Unable to increment custom metric '") + std::string(name) + "': no such counter"; } -std::variant decrementCustomCounter(const std::string_view& name, uint64_t step) +std::variant decrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels) { - auto customCounters = s_customCounters.read_lock(); + auto customCounters = s_customCounters.write_lock(); auto metric = customCounters->find(name); if (metric != customCounters->end()) { - metric->second.d_value -= step; - return metric->second.d_value.load(); + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = metric->second.find(combinationOfLabels); + if (metricEntry == metric->second.end()) { + metricEntry = metric->second.emplace(combinationOfLabels, MutableCounter()).first; + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); + } + metricEntry->second.d_value -= step; + return metricEntry->second.d_value.load(); } return std::string("Unable to decrement custom metric '") + std::string(name) + "': no such counter"; } -std::variant setCustomGauge(const std::string_view& name, const double value) +std::variant setCustomGauge(const std::string_view& name, const double value, const std::unordered_map& labels) { - auto customGauges = s_customGauges.read_lock(); + auto customGauges = s_customGauges.write_lock(); auto metric = customGauges->find(name); if (metric != customGauges->end()) { - metric->second.d_value = value; + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = metric->second.find(combinationOfLabels); + if (metricEntry == metric->second.end()) { + metricEntry = metric->second.emplace(combinationOfLabels, MutableGauge()).first; + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); + } + metricEntry->second.d_value = value; return value; } return std::string("Unable to set metric '") + std::string(name) + "': no such gauge"; } -std::variant getCustomMetric(const std::string_view& name) +std::variant getCustomMetric(const std::string_view& name, const std::unordered_map& labels) { { auto customCounters = s_customCounters.read_lock(); auto counter = customCounters->find(name); if (counter != customCounters->end()) { - return static_cast(counter->second.d_value.load()); + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = counter->second.find(combinationOfLabels); + if (metricEntry != counter->second.end()) { + return static_cast(metricEntry->second.d_value.load()); + } } } { auto customGauges = s_customGauges.read_lock(); auto gauge = customGauges->find(name); if (gauge != customGauges->end()) { - return gauge->second.d_value.load(); + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = gauge->second.find(combinationOfLabels); + if (metricEntry != gauge->second.end()) { + return metricEntry->second.d_value.load(); + } } } return std::string("Unable to get metric '") + std::string(name) + "': no such metric"; } - } diff --git a/pdns/dnsdistdist/dnsdist-metrics.hh b/pdns/dnsdistdist/dnsdist-metrics.hh index 47a3fb84078bb..8afb67509d20a 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.hh +++ b/pdns/dnsdistdist/dnsdist-metrics.hh @@ -25,6 +25,7 @@ #include #include #include +#include #include #include "lock.hh" @@ -35,10 +36,10 @@ namespace dnsdist::metrics using Error = std::string; [[nodiscard]] std::optional declareCustomMetric(const std::string& name, const std::string& type, const std::string& description, std::optional customName); -[[nodiscard]] std::variant incrementCustomCounter(const std::string_view& name, uint64_t step); -[[nodiscard]] std::variant decrementCustomCounter(const std::string_view& name, uint64_t step); -[[nodiscard]] std::variant setCustomGauge(const std::string_view& name, const double value); -[[nodiscard]] std::variant getCustomMetric(const std::string_view& name); +[[nodiscard]] std::variant incrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels); +[[nodiscard]] std::variant decrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels); +[[nodiscard]] std::variant setCustomGauge(const std::string_view& name, const double value, const std::unordered_map& labels); +[[nodiscard]] std::variant getCustomMetric(const std::string_view& name, const std::unordered_map& labels); using pdns::stat_t; @@ -89,13 +90,14 @@ struct Stats pdns::stat_double_t latencyDoH3Avg100{0}, latencyDoH3Avg1000{0}, latencyDoH3Avg10000{0}, latencyDoH3Avg1000000{0}; using statfunction_t = std::function; using entry_t = std::variant; - struct EntryPair + struct EntryTriple { std::string d_name; + std::string d_labels; entry_t d_value; }; - SharedLockGuarded> entries; + SharedLockGuarded> entries; }; extern struct Stats g_stats; diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index 5cdfead09cd06..1f38516e8d631 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -500,6 +500,10 @@ static void handlePrometheus(const YaHTTP::Request& req, YaHTTP::Response& resp) prometheusMetricName = metricDetails.customName; } + if (!entry.d_labels.empty()) { + prometheusMetricName += "{" + entry.d_labels + "}"; + } + // for these we have the help and types encoded in the sources // but we need to be careful about labels in custom metrics std::string helpName = prometheusMetricName.substr(0, prometheusMetricName.find('{')); From 0dff6be4145aadd48cfd9f20f6ac48ac8b3d08df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 16:01:14 +0100 Subject: [PATCH 06/25] dnsdist: add options to `declareMetric` to skip initialization if labels are used --- pdns/dnsdistdist/dnsdist-lua-ffi.cc | 3 ++- pdns/dnsdistdist/dnsdist-lua.cc | 21 ++++++++++++++++++--- pdns/dnsdistdist/dnsdist-metrics.cc | 10 +++++++++- pdns/dnsdistdist/dnsdist-metrics.hh | 2 +- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index c7a0c055f6825..d6991323b090c 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -1805,7 +1805,8 @@ bool dnsdist_ffi_metric_declare(const char* name, size_t nameLen, const char* ty if (name == nullptr || nameLen == 0 || type == nullptr || description == nullptr) { return false; } - auto result = dnsdist::metrics::declareCustomMetric(name, type, description, customName != nullptr ? std::optional(customName) : std::nullopt); + // TODO: add labels options? + auto result = dnsdist::metrics::declareCustomMetric(name, type, description, customName != nullptr ? std::optional(customName) : std::nullopt, false); return !result; } diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index 53e1038c135c6..cf366cb69987a 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -85,7 +85,8 @@ using std::thread; -using update_metric_opts_t = LuaAssociativeTable>>; +using update_metric_opts_t = LuaAssociativeTable>>; +using declare_metric_opts_t = LuaAssociativeTable>; static boost::tribool s_noLuaSideEffect; @@ -3412,8 +3413,22 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) newThread.detach(); }); - luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional customName) { - auto result = dnsdist::metrics::declareCustomMetric(name, type, description, customName ? std::optional(*customName) : std::nullopt); + luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional>> opts) { + bool withLabels = false; + std::optional customName = std::nullopt; + if (opts) { + auto* optCustomName = boost::get(&opts.get()); + if (optCustomName) { + customName = std::optional(*optCustomName); + } + if (!customName) { + boost::optional vars = boost::get>(opts.get()); + getOptionalValue(vars, "customName", customName); + getOptionalValue(vars, "withLabels", withLabels); + checkAllParametersConsumed("declareMetric", vars); + } + } + auto result = dnsdist::metrics::declareCustomMetric(name, type, description, customName, withLabels); if (result) { g_outputBuffer += *result + "\n"; errlog("Error in declareMetric: %s", *result); diff --git a/pdns/dnsdistdist/dnsdist-metrics.cc b/pdns/dnsdistdist/dnsdist-metrics.cc index 039b1ffc89249..18ac63aab2263 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.cc +++ b/pdns/dnsdistdist/dnsdist-metrics.cc @@ -169,7 +169,7 @@ Stats::Stats() : struct Stats g_stats; -std::optional declareCustomMetric(const std::string& name, const std::string& type, const std::string& description, std::optional customName) +std::optional declareCustomMetric(const std::string& name, const std::string& type, const std::string& description, std::optional customName, bool withLabels) { if (!std::regex_match(name, std::regex("^[a-z0-9-]+$"))) { return std::string("Unable to declare metric '") + std::string(name) + std::string("': invalid name\n"); @@ -180,6 +180,10 @@ std::optional declareCustomMetric(const std::string& name, const st auto customCounters = s_customCounters.write_lock(); auto itp = customCounters->emplace(name, std::map()); if (itp.second) { + if (!withLabels) { + auto counter = itp.first->second.emplace("", MutableCounter()); + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{name, "", &counter.first->second.d_value}); + } dnsdist::prometheus::PrometheusMetricDefinition def{name, type, description, finalCustomName}; dnsdist::webserver::addMetricDefinition(def); } @@ -188,6 +192,10 @@ std::optional declareCustomMetric(const std::string& name, const st auto customGauges = s_customGauges.write_lock(); auto itp = customGauges->emplace(name, std::map()); if (itp.second) { + if (!withLabels) { + auto gauge = itp.first->second.emplace("", MutableGauge()); + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{name, "", &gauge.first->second.d_value}); + } dnsdist::prometheus::PrometheusMetricDefinition def{name, type, description, finalCustomName}; dnsdist::webserver::addMetricDefinition(def); } diff --git a/pdns/dnsdistdist/dnsdist-metrics.hh b/pdns/dnsdistdist/dnsdist-metrics.hh index 8afb67509d20a..6cab90527284c 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.hh +++ b/pdns/dnsdistdist/dnsdist-metrics.hh @@ -35,7 +35,7 @@ namespace dnsdist::metrics { using Error = std::string; -[[nodiscard]] std::optional declareCustomMetric(const std::string& name, const std::string& type, const std::string& description, std::optional customName); +[[nodiscard]] std::optional declareCustomMetric(const std::string& name, const std::string& type, const std::string& description, std::optional customName, bool withLabels); [[nodiscard]] std::variant incrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels); [[nodiscard]] std::variant decrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels); [[nodiscard]] std::variant setCustomGauge(const std::string_view& name, const double value, const std::unordered_map& labels); From 138a0fb776a3de2a9013c70cea8f459dd9889fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 16:17:01 +0100 Subject: [PATCH 07/25] chore: reformat metrics related files --- pdns/dnsdistdist/dnsdist-lua-ffi.cc | 17 ++-- pdns/dnsdistdist/dnsdist-lua-ffi.hh | 120 ++++++++++++++++------------ pdns/dnsdistdist/dnsdist-lua.cc | 71 ++++++++-------- pdns/dnsdistdist/dnsdist-metrics.cc | 3 +- pdns/dnsdistdist/dnsdist-web.cc | 2 +- 5 files changed, 115 insertions(+), 98 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index d6991323b090c..d3f9a39829983 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -260,7 +260,7 @@ void dnsdist_ffi_dnsquestion_get_sni(const dnsdist_ffi_dnsquestion_t* dq, const const char* dnsdist_ffi_dnsquestion_get_tag(const dnsdist_ffi_dnsquestion_t* dq, const char* label) { - const char * result = nullptr; + const char* result = nullptr; if (dq != nullptr && dq->dq != nullptr && dq->dq->ids.qTag != nullptr) { const auto it = dq->dq->ids.qTag->find(label); @@ -456,7 +456,6 @@ size_t dnsdist_ffi_dnsquestion_get_tag_array(dnsdist_ffi_dnsquestion_t* dq, cons ++pos; } - if (!dq->tagsVect->empty()) { *out = dq->tagsVect->data(); } @@ -1007,7 +1006,7 @@ static constexpr char s_lua_ffi_code[] = R"FFICodeContent( ffi.cdef[[ )FFICodeContent" #include "dnsdist-lua-ffi-interface.inc" -R"FFICodeContent( + R"FFICodeContent( ]] )FFICodeContent"; @@ -1057,7 +1056,7 @@ size_t dnsdist_ffi_generate_proxy_protocol_payload(const size_t addrSize, const if (valuesCount > 0) { valuesVect.reserve(valuesCount); for (size_t idx = 0; idx < valuesCount; idx++) { - valuesVect.push_back({ std::string(values[idx].value, values[idx].size), values[idx].type }); + valuesVect.push_back({std::string(values[idx].value, values[idx].size), values[idx].type}); } } @@ -1086,7 +1085,7 @@ size_t dnsdist_ffi_dnsquestion_generate_proxy_protocol_payload(const dnsdist_ffi if (valuesCount > 0) { valuesVect.reserve(valuesCount); for (size_t idx = 0; idx < valuesCount; idx++) { - valuesVect.push_back({ std::string(values[idx].value, values[idx].size), values[idx].type }); + valuesVect.push_back({std::string(values[idx].value, values[idx].size), values[idx].type}); } } @@ -1113,7 +1112,7 @@ bool dnsdist_ffi_dnsquestion_add_proxy_protocol_values(dnsdist_ffi_dnsquestion_t dnsQuestion->dq->proxyProtocolValues->reserve(dnsQuestion->dq->proxyProtocolValues->size() + valuesCount); for (size_t idx = 0; idx < valuesCount; idx++) { // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic): the Lua FFI API is a C API.. - dnsQuestion->dq->proxyProtocolValues->push_back({ std::string(values[idx].value, values[idx].size), values[idx].type }); + dnsQuestion->dq->proxyProtocolValues->push_back({std::string(values[idx].value, values[idx].size), values[idx].type}); } return true; @@ -1146,7 +1145,8 @@ struct dnsdist_ffi_domain_list_t { std::vector d_domains; }; -struct dnsdist_ffi_address_list_t { +struct dnsdist_ffi_address_list_t +{ std::vector d_addresses; }; @@ -1476,7 +1476,8 @@ void dnsdist_ffi_ring_entry_list_free(dnsdist_ffi_ring_entry_list_t* list) delete list; } -template static void addRingEntryToList(std::unique_ptr& list, const struct timespec& now, const T& entry) +template +static void addRingEntryToList(std::unique_ptr& list, const struct timespec& now, const T& entry) { auto age = DiffTime(entry.when, now); diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.hh b/pdns/dnsdistdist/dnsdist-lua-ffi.hh index 1369c2a07cd11..644114d699711 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.hh +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.hh @@ -23,27 +23,31 @@ #include "dnsdist.hh" -extern "C" { +extern "C" +{ #include "dnsdist-lua-ffi-interface.h" } #include "ext/luawrapper/include/LuaContext.hpp" // dnsdist_ffi_dnsquestion_t is a lightuserdata -template<> -struct LuaContext::Pusher { - static const int minSize = 1; - static const int maxSize = 1; - - static PushedObject push(lua_State* state, dnsdist_ffi_dnsquestion_t* ptr) noexcept { - lua_pushlightuserdata(state, ptr); - return PushedObject{state, 1}; - } +template <> +struct LuaContext::Pusher +{ + static const int minSize = 1; + static const int maxSize = 1; + + static PushedObject push(lua_State* state, dnsdist_ffi_dnsquestion_t* ptr) noexcept + { + lua_pushlightuserdata(state, ptr); + return PushedObject{state, 1}; + } }; struct dnsdist_ffi_dnsquestion_t { - dnsdist_ffi_dnsquestion_t(DNSQuestion* dq_): dq(dq_) + dnsdist_ffi_dnsquestion_t(DNSQuestion* dq_) : + dq(dq_) { } @@ -63,20 +67,23 @@ struct dnsdist_ffi_dnsquestion_t }; // dnsdist_ffi_dnsresponse_t is a lightuserdata -template<> -struct LuaContext::Pusher { - static const int minSize = 1; - static const int maxSize = 1; - - static PushedObject push(lua_State* state, dnsdist_ffi_dnsresponse_t* ptr) noexcept { - lua_pushlightuserdata(state, ptr); - return PushedObject{state, 1}; - } +template <> +struct LuaContext::Pusher +{ + static const int minSize = 1; + static const int maxSize = 1; + + static PushedObject push(lua_State* state, dnsdist_ffi_dnsresponse_t* ptr) noexcept + { + lua_pushlightuserdata(state, ptr); + return PushedObject{state, 1}; + } }; struct dnsdist_ffi_dnsresponse_t { - dnsdist_ffi_dnsresponse_t(DNSResponse* dr_): dr(dr_) + dnsdist_ffi_dnsresponse_t(DNSResponse* dr_) : + dr(dr_) { } @@ -85,20 +92,23 @@ struct dnsdist_ffi_dnsresponse_t }; // dnsdist_ffi_server_t is a lightuserdata -template<> -struct LuaContext::Pusher { - static const int minSize = 1; - static const int maxSize = 1; - - static PushedObject push(lua_State* state, dnsdist_ffi_server_t* ptr) noexcept { - lua_pushlightuserdata(state, ptr); - return PushedObject{state, 1}; - } +template <> +struct LuaContext::Pusher +{ + static const int minSize = 1; + static const int maxSize = 1; + + static PushedObject push(lua_State* state, dnsdist_ffi_server_t* ptr) noexcept + { + lua_pushlightuserdata(state, ptr); + return PushedObject{state, 1}; + } }; struct dnsdist_ffi_server_t { - dnsdist_ffi_server_t(const std::shared_ptr& server_): server(server_) + dnsdist_ffi_server_t(const std::shared_ptr& server_) : + server(server_) { } @@ -106,23 +116,26 @@ struct dnsdist_ffi_server_t }; // dnsdist_ffi_servers_list_t is a lightuserdata -template<> -struct LuaContext::Pusher { - static const int minSize = 1; - static const int maxSize = 1; - - static PushedObject push(lua_State* state, dnsdist_ffi_servers_list_t* ptr) noexcept { - lua_pushlightuserdata(state, ptr); - return PushedObject{state, 1}; - } +template <> +struct LuaContext::Pusher +{ + static const int minSize = 1; + static const int maxSize = 1; + + static PushedObject push(lua_State* state, dnsdist_ffi_servers_list_t* ptr) noexcept + { + lua_pushlightuserdata(state, ptr); + return PushedObject{state, 1}; + } }; struct dnsdist_ffi_servers_list_t { - dnsdist_ffi_servers_list_t(const ServerPolicy::NumberedServerVector& servers_): servers(servers_) + dnsdist_ffi_servers_list_t(const ServerPolicy::NumberedServerVector& servers_) : + servers(servers_) { ffiServers.reserve(servers.size()); - for (const auto& server: servers) { + for (const auto& server : servers) { ffiServers.push_back(dnsdist_ffi_server_t(server.second)); } } @@ -132,20 +145,23 @@ struct dnsdist_ffi_servers_list_t }; // dnsdist_ffi_network_message_t is a lightuserdata -template<> -struct LuaContext::Pusher { - static const int minSize = 1; - static const int maxSize = 1; - - static PushedObject push(lua_State* state, dnsdist_ffi_network_message_t* ptr) noexcept { - lua_pushlightuserdata(state, ptr); - return PushedObject{state, 1}; - } +template <> +struct LuaContext::Pusher +{ + static const int minSize = 1; + static const int maxSize = 1; + + static PushedObject push(lua_State* state, dnsdist_ffi_network_message_t* ptr) noexcept + { + lua_pushlightuserdata(state, ptr); + return PushedObject{state, 1}; + } }; struct dnsdist_ffi_network_message_t { - dnsdist_ffi_network_message_t(const std::string& payload_ ,const std::string& from_, uint16_t endpointID_): payload(payload_), from(from_), endpointID(endpointID_) + dnsdist_ffi_network_message_t(const std::string& payload_, const std::string& from_, uint16_t endpointID_) : + payload(payload_), from(from_), endpointID(endpointID_) { } diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index cf366cb69987a..7da861d9db36a 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -886,29 +886,28 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) const std::function mutator; const size_t maximumValue{std::numeric_limits::max()}; }; - static const std::vector unsignedIntegerImmutableConfigItems - { + static const std::vector unsignedIntegerImmutableConfigItems{ {"setMaxTCPQueuedConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPQueuedConnections = newValue; }, std::numeric_limits::max()}, - {"setMaxTCPClientThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPClientThreads = newValue; }, std::numeric_limits::max()}, - {"setMaxTCPConnectionsPerClient", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPConnectionsPerClient = newValue; }, std::numeric_limits::max()}, - {"setTCPInternalPipeBufferSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_tcpInternalPipeBufferSize = newValue; }, std::numeric_limits::max()}, - {"setMaxCachedTCPConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, - {"setTCPDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPCleanupInterval = newValue; }, std::numeric_limits::max()}, - {"setTCPDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdleTime = newValue; }, std::numeric_limits::max()}, + {"setMaxTCPClientThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPClientThreads = newValue; }, std::numeric_limits::max()}, + {"setMaxTCPConnectionsPerClient", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPConnectionsPerClient = newValue; }, std::numeric_limits::max()}, + {"setTCPInternalPipeBufferSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_tcpInternalPipeBufferSize = newValue; }, std::numeric_limits::max()}, + {"setMaxCachedTCPConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, + {"setTCPDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPCleanupInterval = newValue; }, std::numeric_limits::max()}, + {"setTCPDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdleTime = newValue; }, std::numeric_limits::max()}, #if defined(HAVE_DNS_OVER_HTTPS) && defined(HAVE_NGHTTP2) - {"setOutgoingDoHWorkerThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHWorkers = newValue; }, std::numeric_limits::max()}, - {"setMaxIdleDoHConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, - {"setDoHDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHCleanupInterval = newValue; }, std::numeric_limits::max()}, - {"setDoHDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdleTime = newValue; }, std::numeric_limits::max()}, + {"setOutgoingDoHWorkerThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHWorkers = newValue; }, std::numeric_limits::max()}, + {"setMaxIdleDoHConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, + {"setDoHDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHCleanupInterval = newValue; }, std::numeric_limits::max()}, + {"setDoHDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdleTime = newValue; }, std::numeric_limits::max()}, #endif /* HAVE_DNS_OVER_HTTPS && HAVE_NGHTTP2 */ - {"setMaxUDPOutstanding", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxUDPOutstanding = newValue; }, std::numeric_limits::max()}, - {"setWHashedPertubation", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_hashPerturbation = newValue; }, std::numeric_limits::max()}, + {"setMaxUDPOutstanding", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxUDPOutstanding = newValue; }, std::numeric_limits::max()}, + {"setWHashedPertubation", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_hashPerturbation = newValue; }, std::numeric_limits::max()}, #ifndef DISABLE_RECVMMSG - {"setUDPMultipleMessagesVectorSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpVectorSize = newValue; }, std::numeric_limits::max()}, + {"setUDPMultipleMessagesVectorSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpVectorSize = newValue; }, std::numeric_limits::max()}, #endif /* DISABLE_RECVMMSG */ - {"setUDPTimeout", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpTimeout = newValue; }, std::numeric_limits::max()}, - {"setConsoleMaximumConcurrentConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_consoleMaxConcurrentConnections = newValue; }, std::numeric_limits::max()}, - {"setRingBuffersLockRetries", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_ringsNbLockTries = newValue; }, std::numeric_limits::max()}, + {"setUDPTimeout", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpTimeout = newValue; }, std::numeric_limits::max()}, + {"setConsoleMaximumConcurrentConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_consoleMaxConcurrentConnections = newValue; }, std::numeric_limits::max()}, + {"setRingBuffersLockRetries", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_ringsNbLockTries = newValue; }, std::numeric_limits::max()}, }; struct DoubleImmutableConfigurationItems @@ -1267,7 +1266,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) }); luaCtx.writeFunction("getPoolServers", [](const string& pool) { - //coverity[auto_causes_copy] + // coverity[auto_causes_copy] const auto poolServers = getDownstreamCandidates(pool); return *poolServers; }); @@ -1603,7 +1602,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) g_outputBuffer = "Crypto failed..\n"; } #else - g_outputBuffer = "Crypto not available.\n"; + g_outputBuffer = "Crypto not available.\n"; #endif }); @@ -1876,9 +1875,9 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) // 1 2 3 4 ret << (fmt % "Name" % "Cache" % "ServerPolicy" % "Servers") << endl; - //coverity[auto_causes_copy] + // coverity[auto_causes_copy] const auto defaultPolicyName = dnsdist::configuration::getCurrentRuntimeConfiguration().d_lbPolicy->getName(); - //coverity[auto_causes_copy] + // coverity[auto_causes_copy] const auto pools = dnsdist::configuration::getCurrentRuntimeConfiguration().d_pools; for (const auto& entry : pools) { const string& name = entry.first; @@ -2461,7 +2460,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_NGHTTP2 frontend->d_library = "nghttp2"; #else /* HAVE_NGHTTP2 */ - frontend->d_library = "h2o"; + frontend->d_library = "h2o"; #endif /* HAVE_NGHTTP2 */ } if (frontend->d_library == "h2o") { @@ -2470,8 +2469,8 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) // we _really_ need to set it again, as we just replaced the generic frontend by a new one frontend->d_library = "h2o"; #else /* HAVE_LIBH2OEVLOOP */ - errlog("DOH bind %s is configured to use libh2o but the library is not available", addr); - return; + errlog("DOH bind %s is configured to use libh2o but the library is not available", addr); + return; #endif /* HAVE_LIBH2OEVLOOP */ } else if (frontend->d_library == "nghttp2") { @@ -2588,7 +2587,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_LIBSSL const std::string provider("openssl"); #else - const std::string provider("gnutls"); + const std::string provider("gnutls"); #endif vinfolog("Loading default TLS provider '%s'", provider); } @@ -2609,7 +2608,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else /* HAVE_DNS_OVER_HTTPS */ - throw std::runtime_error("addDOHLocal() called but DNS over HTTPS support is not present!"); + throw std::runtime_error("addDOHLocal() called but DNS over HTTPS support is not present!"); #endif /* HAVE_DNS_OVER_HTTPS */ }); @@ -2686,7 +2685,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else - throw std::runtime_error("addDOH3Local() called but DNS over HTTP/3 support is not present!"); + throw std::runtime_error("addDOH3Local() called but DNS over HTTP/3 support is not present!"); #endif }); @@ -2763,7 +2762,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else - throw std::runtime_error("addDOQLocal() called but DNS over QUIC support is not present!"); + throw std::runtime_error("addDOQLocal() called but DNS over QUIC support is not present!"); #endif }); @@ -2786,7 +2785,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over QUIC support is not present!\n"; + g_outputBuffer = "DNS over QUIC support is not present!\n"; #endif }); @@ -2845,7 +2844,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTPS support is not present!\n"; + g_outputBuffer = "DNS over HTTPS support is not present!\n"; #endif }); @@ -2868,7 +2867,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTP3 support is not present!\n"; + g_outputBuffer = "DNS over HTTP3 support is not present!\n"; #endif }); @@ -2938,7 +2937,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTPS support is not present!\n"; + g_outputBuffer = "DNS over HTTPS support is not present!\n"; #endif }); @@ -3117,7 +3116,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_LIBSSL const std::string provider("openssl"); #else - const std::string provider("gnutls"); + const std::string provider("gnutls"); #endif vinfolog("Loading default TLS provider '%s'", provider); } @@ -3143,7 +3142,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) g_outputBuffer = "Error: " + string(e.what()) + "\n"; } #else - throw std::runtime_error("addTLSLocal() called but DNS over TLS support is not present!"); + throw std::runtime_error("addTLSLocal() called but DNS over TLS support is not present!"); #endif }); @@ -3167,7 +3166,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over TLS support is not present!\n"; + g_outputBuffer = "DNS over TLS support is not present!\n"; #endif }); diff --git a/pdns/dnsdistdist/dnsdist-metrics.cc b/pdns/dnsdistdist/dnsdist-metrics.cc index 18ac63aab2263..4f9c92b94f931 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.cc +++ b/pdns/dnsdistdist/dnsdist-metrics.cc @@ -227,7 +227,8 @@ static string prometheusLabelValueEscape(const string& value) static std::string generateCombinationOfLabels(const std::unordered_map& labels) { - return std::accumulate(labels.begin(), labels.end(), std::string(), [](const std::string& acc, const std::pair& l) { + auto ordered = std::map(labels.begin(), labels.end()); + return std::accumulate(ordered.begin(), ordered.end(), std::string(), [](const std::string& acc, const std::pair& l) { return acc + (acc.empty() ? std::string() : ",") + l.first + "=" + "\"" + prometheusLabelValueEscape(l.second) + "\""; }); } diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index 1f38516e8d631..772af64902865 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -1912,7 +1912,7 @@ void setMaxConcurrentConnections(size_t max) void WebserverThread(Socket sock) { setThreadName("dnsdist/webserv"); - //coverity[auto_causes_copy] + // coverity[auto_causes_copy] const auto local = *dnsdist::configuration::getCurrentRuntimeConfiguration().d_webServerAddress; infolog("Webserver launched on %s", local.toStringWithPort()); From 7d075382ad52b6c4e784b9c0c398c164d1482655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 16:36:54 +0100 Subject: [PATCH 08/25] dnsdist: remove unnecessary optional around variant opts --- pdns/dnsdistdist/dnsdist-lua.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index 7da861d9db36a..1e432d31b0a4d 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -3412,7 +3412,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) newThread.detach(); }); - luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional>> opts) { + luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional> opts) { bool withLabels = false; std::optional customName = std::nullopt; if (opts) { @@ -3421,7 +3421,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) customName = std::optional(*optCustomName); } if (!customName) { - boost::optional vars = boost::get>(opts.get()); + boost::optional vars = {boost::get(opts.get())}; getOptionalValue(vars, "customName", customName); getOptionalValue(vars, "withLabels", withLabels); checkAllParametersConsumed("declareMetric", vars); @@ -3435,7 +3435,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return true; }); - luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional>> opts) { + luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional> opts) { auto incOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; @@ -3443,7 +3443,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) step = *custom_step; } else { - boost::optional vars = boost::get>(incOpts); + boost::optional vars = {boost::get(incOpts)}; getOptionalValue(vars, "step", step); getOptionalValue>(vars, "labels", labels); checkAllParametersConsumed("incMetric", vars); @@ -3456,7 +3456,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional>> opts) { + luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional> opts) { auto decOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; @@ -3464,7 +3464,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) step = *custom_step; } else { - boost::optional vars = boost::get>(decOpts); + boost::optional vars = {boost::get(decOpts)}; getOptionalValue(vars, "step", step); getOptionalValue>(vars, "labels", labels); checkAllParametersConsumed("decMetric", vars); From 423251d25d553d85fb87bbd388bec3b251b95d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 16:51:33 +0100 Subject: [PATCH 09/25] dnsdist: fix formatting in dnsdist-lua.cc --- pdns/dnsdistdist/dnsdist-lua.cc | 65 +++++++++++++++++---------------- 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index 1e432d31b0a4d..bf7e179ed0b37 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -886,28 +886,29 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) const std::function mutator; const size_t maximumValue{std::numeric_limits::max()}; }; - static const std::vector unsignedIntegerImmutableConfigItems{ + static const std::vector unsignedIntegerImmutableConfigItems + { {"setMaxTCPQueuedConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPQueuedConnections = newValue; }, std::numeric_limits::max()}, - {"setMaxTCPClientThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPClientThreads = newValue; }, std::numeric_limits::max()}, - {"setMaxTCPConnectionsPerClient", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPConnectionsPerClient = newValue; }, std::numeric_limits::max()}, - {"setTCPInternalPipeBufferSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_tcpInternalPipeBufferSize = newValue; }, std::numeric_limits::max()}, - {"setMaxCachedTCPConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, - {"setTCPDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPCleanupInterval = newValue; }, std::numeric_limits::max()}, - {"setTCPDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdleTime = newValue; }, std::numeric_limits::max()}, + {"setMaxTCPClientThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPClientThreads = newValue; }, std::numeric_limits::max()}, + {"setMaxTCPConnectionsPerClient", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxTCPConnectionsPerClient = newValue; }, std::numeric_limits::max()}, + {"setTCPInternalPipeBufferSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_tcpInternalPipeBufferSize = newValue; }, std::numeric_limits::max()}, + {"setMaxCachedTCPConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, + {"setTCPDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPCleanupInterval = newValue; }, std::numeric_limits::max()}, + {"setTCPDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingTCPMaxIdleTime = newValue; }, std::numeric_limits::max()}, #if defined(HAVE_DNS_OVER_HTTPS) && defined(HAVE_NGHTTP2) - {"setOutgoingDoHWorkerThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHWorkers = newValue; }, std::numeric_limits::max()}, - {"setMaxIdleDoHConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, - {"setDoHDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHCleanupInterval = newValue; }, std::numeric_limits::max()}, - {"setDoHDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdleTime = newValue; }, std::numeric_limits::max()}, + {"setOutgoingDoHWorkerThreads", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHWorkers = newValue; }, std::numeric_limits::max()}, + {"setMaxIdleDoHConnectionsPerDownstream", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdlePerBackend = newValue; }, std::numeric_limits::max()}, + {"setDoHDownstreamCleanupInterval", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHCleanupInterval = newValue; }, std::numeric_limits::max()}, + {"setDoHDownstreamMaxIdleTime", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_outgoingDoHMaxIdleTime = newValue; }, std::numeric_limits::max()}, #endif /* HAVE_DNS_OVER_HTTPS && HAVE_NGHTTP2 */ - {"setMaxUDPOutstanding", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxUDPOutstanding = newValue; }, std::numeric_limits::max()}, - {"setWHashedPertubation", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_hashPerturbation = newValue; }, std::numeric_limits::max()}, + {"setMaxUDPOutstanding", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_maxUDPOutstanding = newValue; }, std::numeric_limits::max()}, + {"setWHashedPertubation", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_hashPerturbation = newValue; }, std::numeric_limits::max()}, #ifndef DISABLE_RECVMMSG - {"setUDPMultipleMessagesVectorSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpVectorSize = newValue; }, std::numeric_limits::max()}, + {"setUDPMultipleMessagesVectorSize", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpVectorSize = newValue; }, std::numeric_limits::max()}, #endif /* DISABLE_RECVMMSG */ - {"setUDPTimeout", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpTimeout = newValue; }, std::numeric_limits::max()}, - {"setConsoleMaximumConcurrentConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_consoleMaxConcurrentConnections = newValue; }, std::numeric_limits::max()}, - {"setRingBuffersLockRetries", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_ringsNbLockTries = newValue; }, std::numeric_limits::max()}, + {"setUDPTimeout", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_udpTimeout = newValue; }, std::numeric_limits::max()}, + {"setConsoleMaximumConcurrentConnections", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_consoleMaxConcurrentConnections = newValue; }, std::numeric_limits::max()}, + {"setRingBuffersLockRetries", [](dnsdist::configuration::ImmutableConfiguration& config, uint64_t newValue) { config.d_ringsNbLockTries = newValue; }, std::numeric_limits::max()}, }; struct DoubleImmutableConfigurationItems @@ -1602,7 +1603,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) g_outputBuffer = "Crypto failed..\n"; } #else - g_outputBuffer = "Crypto not available.\n"; + g_outputBuffer = "Crypto not available.\n"; #endif }); @@ -2460,7 +2461,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_NGHTTP2 frontend->d_library = "nghttp2"; #else /* HAVE_NGHTTP2 */ - frontend->d_library = "h2o"; + frontend->d_library = "h2o"; #endif /* HAVE_NGHTTP2 */ } if (frontend->d_library == "h2o") { @@ -2469,8 +2470,8 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) // we _really_ need to set it again, as we just replaced the generic frontend by a new one frontend->d_library = "h2o"; #else /* HAVE_LIBH2OEVLOOP */ - errlog("DOH bind %s is configured to use libh2o but the library is not available", addr); - return; + errlog("DOH bind %s is configured to use libh2o but the library is not available", addr); + return; #endif /* HAVE_LIBH2OEVLOOP */ } else if (frontend->d_library == "nghttp2") { @@ -2587,7 +2588,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_LIBSSL const std::string provider("openssl"); #else - const std::string provider("gnutls"); + const std::string provider("gnutls"); #endif vinfolog("Loading default TLS provider '%s'", provider); } @@ -2608,7 +2609,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else /* HAVE_DNS_OVER_HTTPS */ - throw std::runtime_error("addDOHLocal() called but DNS over HTTPS support is not present!"); + throw std::runtime_error("addDOHLocal() called but DNS over HTTPS support is not present!"); #endif /* HAVE_DNS_OVER_HTTPS */ }); @@ -2685,7 +2686,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else - throw std::runtime_error("addDOH3Local() called but DNS over HTTP/3 support is not present!"); + throw std::runtime_error("addDOH3Local() called but DNS over HTTP/3 support is not present!"); #endif }); @@ -2762,7 +2763,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) config.d_frontends.push_back(std::move(clientState)); }); #else - throw std::runtime_error("addDOQLocal() called but DNS over QUIC support is not present!"); + throw std::runtime_error("addDOQLocal() called but DNS over QUIC support is not present!"); #endif }); @@ -2785,7 +2786,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over QUIC support is not present!\n"; + g_outputBuffer = "DNS over QUIC support is not present!\n"; #endif }); @@ -2844,7 +2845,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTPS support is not present!\n"; + g_outputBuffer = "DNS over HTTPS support is not present!\n"; #endif }); @@ -2867,7 +2868,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTP3 support is not present!\n"; + g_outputBuffer = "DNS over HTTP3 support is not present!\n"; #endif }); @@ -2937,7 +2938,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over HTTPS support is not present!\n"; + g_outputBuffer = "DNS over HTTPS support is not present!\n"; #endif }); @@ -3116,7 +3117,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) #ifdef HAVE_LIBSSL const std::string provider("openssl"); #else - const std::string provider("gnutls"); + const std::string provider("gnutls"); #endif vinfolog("Loading default TLS provider '%s'", provider); } @@ -3142,7 +3143,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) g_outputBuffer = "Error: " + string(e.what()) + "\n"; } #else - throw std::runtime_error("addTLSLocal() called but DNS over TLS support is not present!"); + throw std::runtime_error("addTLSLocal() called but DNS over TLS support is not present!"); #endif }); @@ -3166,7 +3167,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) throw; } #else - g_outputBuffer = "DNS over TLS support is not present!\n"; + g_outputBuffer = "DNS over TLS support is not present!\n"; #endif }); From 657caa55a4c3ae37a039c7de24762553b08ed8f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 17:23:05 +0100 Subject: [PATCH 10/25] dnsdist: use template function to simplify metrics mutation functions --- pdns/dnsdistdist/dnsdist-metrics.cc | 44 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-metrics.cc b/pdns/dnsdistdist/dnsdist-metrics.cc index 4f9c92b94f931..10d18dc2923ac 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.cc +++ b/pdns/dnsdistdist/dnsdist-metrics.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include "dnsdist-metrics.hh" #include "dnsdist.hh" @@ -233,19 +234,26 @@ static std::string generateCombinationOfLabels(const std::unordered_map +static T& initializeOrGetMetric(const std::string_view& name, std::map& metricEntries, const std::unordered_map& labels) +{ + auto combinationOfLabels = generateCombinationOfLabels(labels); + auto metricEntry = metricEntries.find(combinationOfLabels); + if (metricEntry == metricEntries.end()) { + metricEntry = metricEntries.emplace(std::piecewise_construct, std::forward_as_tuple(combinationOfLabels), std::forward_as_tuple()).first; + g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); + } + return metricEntry->second; +} + std::variant incrementCustomCounter(const std::string_view& name, uint64_t step, const std::unordered_map& labels) { auto customCounters = s_customCounters.write_lock(); auto metric = customCounters->find(name); if (metric != customCounters->end()) { - auto combinationOfLabels = generateCombinationOfLabels(labels); - auto metricEntry = metric->second.find(combinationOfLabels); - if (metricEntry == metric->second.end()) { - metricEntry = metric->second.emplace(combinationOfLabels, MutableCounter()).first; - g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); - } - metricEntry->second.d_value += step; - return metricEntry->second.d_value.load(); + auto& metricEntry = initializeOrGetMetric(name, metric->second, labels); + metricEntry.d_value += step; + return metricEntry.d_value.load(); } return std::string("Unable to increment custom metric '") + std::string(name) + "': no such counter"; } @@ -255,14 +263,9 @@ std::variant decrementCustomCounter(const std::string_view& nam auto customCounters = s_customCounters.write_lock(); auto metric = customCounters->find(name); if (metric != customCounters->end()) { - auto combinationOfLabels = generateCombinationOfLabels(labels); - auto metricEntry = metric->second.find(combinationOfLabels); - if (metricEntry == metric->second.end()) { - metricEntry = metric->second.emplace(combinationOfLabels, MutableCounter()).first; - g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); - } - metricEntry->second.d_value -= step; - return metricEntry->second.d_value.load(); + auto& metricEntry = initializeOrGetMetric(name, metric->second, labels); + metricEntry.d_value -= step; + return metricEntry.d_value.load(); } return std::string("Unable to decrement custom metric '") + std::string(name) + "': no such counter"; } @@ -272,13 +275,8 @@ std::variant setCustomGauge(const std::string_view& name, const d auto customGauges = s_customGauges.write_lock(); auto metric = customGauges->find(name); if (metric != customGauges->end()) { - auto combinationOfLabels = generateCombinationOfLabels(labels); - auto metricEntry = metric->second.find(combinationOfLabels); - if (metricEntry == metric->second.end()) { - metricEntry = metric->second.emplace(combinationOfLabels, MutableGauge()).first; - g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value}); - } - metricEntry->second.d_value = value; + auto& metricEntry = initializeOrGetMetric(name, metric->second, labels); + metricEntry.d_value = value; return value; } From ec216cdb5f74e3947ebefeb1effb3de767c63d96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 17:38:43 +0100 Subject: [PATCH 11/25] dnsdist: clean up clang-tidy warnings in lua and metrics --- pdns/dnsdistdist/dnsdist-lua.cc | 10 +++++----- pdns/dnsdistdist/dnsdist-metrics.cc | 17 +++++++++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index bf7e179ed0b37..ff4e05587e5c1 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -3413,12 +3413,12 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) newThread.detach(); }); - luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional> opts) { + luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, const boost::optional>& opts) { bool withLabels = false; std::optional customName = std::nullopt; if (opts) { auto* optCustomName = boost::get(&opts.get()); - if (optCustomName) { + if (optCustomName != nullptr) { customName = std::optional(*optCustomName); } if (!customName) { @@ -3436,7 +3436,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return true; }); - luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional> opts) { + luaCtx.writeFunction("incMetric", [](const std::string& name, const boost::optional>& opts) { auto incOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; @@ -3457,11 +3457,11 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional> opts) { + luaCtx.writeFunction("decMetric", [](const std::string& name, const boost::optional>& opts) { auto decOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; - if (auto custom_step = boost::get(&decOpts)) { + if (auto* custom_step = boost::get(&decOpts)) { step = *custom_step; } else { diff --git a/pdns/dnsdistdist/dnsdist-metrics.cc b/pdns/dnsdistdist/dnsdist-metrics.cc index 10d18dc2923ac..293be6d95c4d5 100644 --- a/pdns/dnsdistdist/dnsdist-metrics.cc +++ b/pdns/dnsdistdist/dnsdist-metrics.cc @@ -211,17 +211,18 @@ static string prometheusLabelValueEscape(const string& value) { string ret; - for (char i : value) { - if (i == '"' || i == '\\') { + for (char lblchar : value) { + if (lblchar == '"' || lblchar == '\\') { ret += '\\'; - ret += i; + ret += lblchar; } - else if (i == '\n') { + else if (lblchar == '\n') { ret += '\\'; ret += 'n'; } - else - ret += i; + else { + ret += lblchar; + } } return ret; } @@ -229,8 +230,8 @@ static string prometheusLabelValueEscape(const string& value) static std::string generateCombinationOfLabels(const std::unordered_map& labels) { auto ordered = std::map(labels.begin(), labels.end()); - return std::accumulate(ordered.begin(), ordered.end(), std::string(), [](const std::string& acc, const std::pair& l) { - return acc + (acc.empty() ? std::string() : ",") + l.first + "=" + "\"" + prometheusLabelValueEscape(l.second) + "\""; + return std::accumulate(ordered.begin(), ordered.end(), std::string(), [](const std::string& acc, const std::pair& label) { + return acc + (acc.empty() ? std::string() : ",") + label.first + "=" + "\"" + prometheusLabelValueEscape(label.second) + "\""; }); } From 64e93522ade211912b79ba27758254811113aaf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Thu, 12 Dec 2024 19:36:41 +0100 Subject: [PATCH 12/25] dnsdist: skip metrics with labels for carbon export and stats dump --- pdns/dnsdistdist/dnsdist-carbon.cc | 5 +++++ pdns/dnsdistdist/dnsdist-lua-inspection.cc | 14 +++++++++++--- pdns/dnsdistdist/dnsdist-lua.cc | 6 +++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-carbon.cc b/pdns/dnsdistdist/dnsdist-carbon.cc index 040783b68a956..66ec734250e4c 100644 --- a/pdns/dnsdistdist/dnsdist-carbon.cc +++ b/pdns/dnsdistdist/dnsdist-carbon.cc @@ -57,6 +57,11 @@ static bool doOneCarbonExport(const Carbon::Endpoint& endpoint) { auto entries = dnsdist::metrics::g_stats.entries.read_lock(); for (const auto& entry : *entries) { + // Skip non-empty labels, since labels are not supported in Carbon + if (!entry.d_labels.empty()) { + continue; + } + str << namespace_name << "." << hostname << "." << instance_name << "." << entry.d_name << ' '; if (const auto& val = std::get_if(&entry.d_value)) { str << (*val)->load(); diff --git a/pdns/dnsdistdist/dnsdist-lua-inspection.cc b/pdns/dnsdistdist/dnsdist-lua-inspection.cc index 2ad3999f7c2e5..26d37f712254f 100644 --- a/pdns/dnsdistdist/dnsdist-lua-inspection.cc +++ b/pdns/dnsdistdist/dnsdist-lua-inspection.cc @@ -19,7 +19,9 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #include +#include #include "dnsdist.hh" #include "dnsdist-console.hh" @@ -811,12 +813,18 @@ void setupLuaInspection(LuaContext& luaCtx) boost::format fmt("%-35s\t%+11s"); g_outputBuffer.clear(); auto entries = *dnsdist::metrics::g_stats.entries.read_lock(); - sort(entries.begin(), entries.end(), + + // Filter entries to just the ones without label, for clearer output + std::vector> unlabeledEntries; + std::copy_if(entries.begin(), entries.end(), std::back_inserter(unlabeledEntries), [](const decltype(entries)::value_type& triple) { return triple.d_labels.empty(); }); + + sort(unlabeledEntries.begin(), unlabeledEntries.end(), [](const decltype(entries)::value_type& lhs, const decltype(entries)::value_type& rhs) { return lhs.d_name < rhs.d_name; }); boost::format flt(" %9.1f"); - for (const auto& entry : entries) { + for (const auto& entryRef : unlabeledEntries) { + const auto& entry = entryRef.get(); string second; if (const auto& val = std::get_if(&entry.d_value)) { second = std::to_string((*val)->load()); @@ -828,7 +836,7 @@ void setupLuaInspection(LuaContext& luaCtx) second = std::to_string((*func)(entry.d_name)); } - if (leftcolumn.size() < entries.size() / 2) { + if (leftcolumn.size() < unlabeledEntries.size() / 2) { leftcolumn.push_back((fmt % entry.d_name % second).str()); } else { diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index ff4e05587e5c1..eec7e4dc97f41 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -3413,7 +3413,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) newThread.detach(); }); - luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, const boost::optional>& opts) { + luaCtx.writeFunction("declareMetric", [](const std::string& name, const std::string& type, const std::string& description, boost::optional> opts) { bool withLabels = false; std::optional customName = std::nullopt; if (opts) { @@ -3436,7 +3436,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return true; }); - luaCtx.writeFunction("incMetric", [](const std::string& name, const boost::optional>& opts) { + luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional> opts) { auto incOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; @@ -3457,7 +3457,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); - luaCtx.writeFunction("decMetric", [](const std::string& name, const boost::optional>& opts) { + luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional> opts) { auto decOpts = opts.get_value_or(1); uint64_t step = 1; std::unordered_map labels; From 9e9c3e54a91189a4b1434cdce3296e94a4e4dfa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Fri, 13 Dec 2024 12:16:13 +0100 Subject: [PATCH 13/25] dnsdist: remove todo comments from `dnsdist-lua-ffi` --- pdns/dnsdistdist/dnsdist-lua-ffi.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index d3f9a39829983..92e042e285066 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -1806,14 +1806,12 @@ bool dnsdist_ffi_metric_declare(const char* name, size_t nameLen, const char* ty if (name == nullptr || nameLen == 0 || type == nullptr || description == nullptr) { return false; } - // TODO: add labels options? auto result = dnsdist::metrics::declareCustomMetric(name, type, description, customName != nullptr ? std::optional(customName) : std::nullopt, false); return !result; } void dnsdist_ffi_metric_inc(const char* metricName, size_t metricNameLen) { - // TODO: add labels? auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), 1U, {}); if (std::get_if(&result) != nullptr) { return; @@ -1822,7 +1820,6 @@ void dnsdist_ffi_metric_inc(const char* metricName, size_t metricNameLen) void dnsdist_ffi_metric_inc_by(const char* metricName, size_t metricNameLen, uint64_t value) { - // TODO: add labels? auto result = dnsdist::metrics::incrementCustomCounter(std::string_view(metricName, metricNameLen), value, {}); if (std::get_if(&result) != nullptr) { return; @@ -1831,7 +1828,6 @@ void dnsdist_ffi_metric_inc_by(const char* metricName, size_t metricNameLen, uin void dnsdist_ffi_metric_dec(const char* metricName, size_t metricNameLen) { - // TODO: add labels? auto result = dnsdist::metrics::decrementCustomCounter(std::string_view(metricName, metricNameLen), 1U, {}); if (std::get_if(&result) != nullptr) { return; @@ -1840,7 +1836,6 @@ void dnsdist_ffi_metric_dec(const char* metricName, size_t metricNameLen) void dnsdist_ffi_metric_set(const char* metricName, size_t metricNameLen, double value) { - // TODO: add labels? auto result = dnsdist::metrics::setCustomGauge(std::string_view(metricName, metricNameLen), value, {}); if (std::get_if(&result) != nullptr) { return; @@ -1849,7 +1844,6 @@ void dnsdist_ffi_metric_set(const char* metricName, size_t metricNameLen, double double dnsdist_ffi_metric_get(const char* metricName, size_t metricNameLen, bool isCounter) { - // TODO: add labels? auto result = dnsdist::metrics::getCustomMetric(std::string_view(metricName, metricNameLen), {}); if (std::get_if(&result) != nullptr) { return 0.; From 12af2075a86c11ee5441defbfe6695a609cb6eb4 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 13 Dec 2024 15:45:31 +0100 Subject: [PATCH 14/25] dnsdist: Fix ECS zero-scope with incoming DoH queries The zero-scope feature involves a first cache lookup before the ECS information has been added to the query, then on a miss a second, regular lookup is done. When we get a response from the backend that contains an ECS scope set to 0, we can insert it into the cache in a way that allows using it for all clients, but we must be careful to use the key that was computed during the first lookup, and not the second one. Incoming DoH queries make that even more interesting because while they are received over TCP, they are initially forwarded to the backend over UDP but can be retried over TCP if a TC=1 answer is received. In that case we must be very careful not to insert the answer into the cache using the wrong protocol, as we don't want to serve a TC=1 answer to a client contacting us over TCP, for example. The computation of the cache key and protocol was unfortunately broken for the incoming query received over DoH, forwarded over UDP and response has a zero scope case. This commit fixes it. --- pdns/dnsdistdist/dnsdist-idstate.hh | 4 +-- pdns/dnsdistdist/dnsdist.cc | 33 ++++++++++--------- regression-tests.dnsdist/test_Caching.py | 42 ++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-idstate.hh b/pdns/dnsdistdist/dnsdist-idstate.hh index 259c55e92a15c..48fdeefc30565 100644 --- a/pdns/dnsdistdist/dnsdist-idstate.hh +++ b/pdns/dnsdistdist/dnsdist-idstate.hh @@ -157,8 +157,8 @@ struct InternalQueryState int32_t d_streamID{-1}; // 4 uint32_t cacheKey{0}; // 4 uint32_t cacheKeyNoECS{0}; // 4 - // DoH-only */ - uint32_t cacheKeyUDP{0}; // 4 + // DoH-only: if we received a TC=1 answer, we had to retry over TCP and thus we need the TCP cache key */ + uint32_t cacheKeyTCP{0}; // 4 uint32_t ttlCap{0}; // cap the TTL _after_ inserting into the packet cache // 4 int backendFD{-1}; // 4 int delayMsec{0}; diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc index 75ec0dd29a637..758e0b7dadada 100644 --- a/pdns/dnsdistdist/dnsdist.cc +++ b/pdns/dnsdistdist/dnsdist.cc @@ -501,14 +501,15 @@ bool processResponseAfterRules(PacketBuffer& response, DNSResponse& dnsResponse, zeroScope = false; } uint32_t cacheKey = dnsResponse.ids.cacheKey; - if (dnsResponse.ids.protocol == dnsdist::Protocol::DoH && dnsResponse.ids.forwardedOverUDP) { - cacheKey = dnsResponse.ids.cacheKeyUDP; + if (dnsResponse.ids.protocol == dnsdist::Protocol::DoH && !dnsResponse.ids.forwardedOverUDP) { + cacheKey = dnsResponse.ids.cacheKeyTCP; + // disable zeroScope in that case, as we only have the "no-ECS" cache key for UDP + zeroScope = false; } - else if (zeroScope) { + if (zeroScope) { // if zeroScope, pass the pre-ECS hash-key and do not pass the subnet to the cache cacheKey = dnsResponse.ids.cacheKeyNoECS; } - dnsResponse.ids.packetCache->insert(cacheKey, zeroScope ? boost::none : dnsResponse.ids.subnet, dnsResponse.ids.cacheFlags, dnsResponse.ids.dnssecOK, dnsResponse.ids.qname, dnsResponse.ids.qtype, dnsResponse.ids.qclass, response, dnsResponse.ids.forwardedOverUDP, dnsResponse.getHeader()->rcode, dnsResponse.ids.tempFailureTTL); const auto& chains = dnsdist::configuration::getCurrentRuntimeConfiguration().d_ruleChains; @@ -1418,6 +1419,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_ std::shared_ptr serverPool = getPool(dnsQuestion.ids.poolName); dnsQuestion.ids.packetCache = serverPool->packetCache; selectBackendForOutgoingQuery(dnsQuestion, serverPool, selectedBackend); + bool willBeForwardedOverUDP = !dnsQuestion.overTCP() || dnsQuestion.ids.protocol == dnsdist::Protocol::DoH; + if (selectedBackend && selectedBackend->isTCPOnly()) { + willBeForwardedOverUDP = false; + } uint32_t allowExpired = selectedBackend ? 0 : dnsdist::configuration::getCurrentRuntimeConfiguration().d_staleCacheEntriesTTL; @@ -1430,7 +1435,7 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_ // we need ECS parsing (parseECS) to be true so we can be sure that the initial incoming query did not have an existing // ECS option, which would make it unsuitable for the zero-scope feature. if (dnsQuestion.ids.packetCache && !dnsQuestion.ids.skipCache && (!selectedBackend || !selectedBackend->d_config.disableZeroScope) && dnsQuestion.ids.packetCache->isECSParsingEnabled()) { - if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyNoECS, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, !dnsQuestion.overTCP(), allowExpired, false, true, false)) { + if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyNoECS, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, willBeForwardedOverUDP, allowExpired, false, true, false)) { vinfolog("Packet cache hit for query for %s|%s from %s (%s, %d bytes)", dnsQuestion.ids.qname.toLogString(), QType(dnsQuestion.ids.qtype).toString(), dnsQuestion.ids.origRemote.toStringWithPort(), dnsQuestion.ids.protocol.toString(), dnsQuestion.getData().size()); @@ -1456,14 +1461,11 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_ } if (dnsQuestion.ids.packetCache && !dnsQuestion.ids.skipCache) { - bool forwardedOverUDP = !dnsQuestion.overTCP(); - if (selectedBackend && selectedBackend->isTCPOnly()) { - forwardedOverUDP = false; - } - - /* we do not record a miss for queries received over DoH and forwarded over TCP + /* First lookup, which takes into account how the protocol over which the query will be forwarded. + For DoH, this lookup is done with the protocol set to TCP but we will retry over UDP below, + therefore we do not record a miss for queries received over DoH and forwarded over TCP yet, as we will do a second-lookup */ - if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, forwardedOverUDP, allowExpired, false, true, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH || forwardedOverUDP)) { + if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, dnsQuestion.ids.protocol == dnsdist::Protocol::DoH ? &dnsQuestion.ids.cacheKeyTCP : &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH && willBeForwardedOverUDP, allowExpired, false, true, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH || !willBeForwardedOverUDP)) { dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [flags = dnsQuestion.ids.origFlags](dnsheader& header) { restoreFlags(&header, flags); @@ -1480,9 +1482,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_ ++dnsQuestion.ids.cs->responses; return ProcessQueryResult::SendAnswer; } - if (dnsQuestion.ids.protocol == dnsdist::Protocol::DoH && !forwardedOverUDP) { - /* do a second-lookup for UDP responses, but we do not want TC=1 answers */ - if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyUDP, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, true, allowExpired, false, false, true)) { + if (dnsQuestion.ids.protocol == dnsdist::Protocol::DoH && willBeForwardedOverUDP) { + /* do a second-lookup for responses received over UDP, but we do not want TC=1 answers */ + /* we need to be careful to keep the existing cache-key (TCP) */ + if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, true, allowExpired, false, false, true)) { if (!prepareOutgoingResponse(*dnsQuestion.ids.cs, dnsQuestion, true)) { return ProcessQueryResult::Drop; } diff --git a/regression-tests.dnsdist/test_Caching.py b/regression-tests.dnsdist/test_Caching.py index f9421812b3632..0f0f1195b4778 100644 --- a/regression-tests.dnsdist/test_Caching.py +++ b/regression-tests.dnsdist/test_Caching.py @@ -2396,6 +2396,12 @@ def testCacheCollisionWithECSParsing(self): class TestCachingScopeZero(DNSDistTest): + _serverKey = 'server.key' + _serverCert = 'server.chain' + _serverName = 'tls.tests.dnsdist.org' + _caCert = 'ca.pem' + _dohServerPort = pickAvailablePort() + _dohBaseURL = ("https://%s:%d/" % (_serverName, _dohServerPort)) _config_template = """ -- Be careful to enable ECS parsing in the packet cache, otherwise scope zero is disabled pc = newPacketCache(100, {maxTTL=86400, minTTL=1, temporaryFailureTTL=60, staleTTL=60, dontAge=false, numberOfShards=1, deferrableInsertLock=true, maxNegativeTTL=3600, parseECS=true}) @@ -2406,7 +2412,11 @@ class TestCachingScopeZero(DNSDistTest): -- to unset it using rules before the first cache lookup) addAction(RDRule(), SetECSAction("192.0.2.1/32")) addAction(RDRule(), SetNoRecurseAction()) + + -- test the DoH special case (query received over TCP, forwarded over UDP) + addDOHLocal("127.0.0.1:%d", "%s", "%s", { "/" }, {library='nghttp2'}) """ + _config_params = ['_testServerPort', '_dohServerPort', '_serverCert', '_serverKey'] def testScopeZero(self): """ @@ -2582,6 +2592,38 @@ def testNoECS(self): self.checkMessageEDNSWithECS(expectedQuery2, receivedQuery) self.checkMessageNoEDNS(receivedResponse, response) + def testScopeZeroIncomingDoH(self): + """ + Cache: Test the scope-zero feature with a query received over DoH, backend returns a scope of zero + """ + ttl = 600 + name = 'scope-zero-incoming-doh.cache.tests.powerdns.com.' + query = dns.message.make_query(name, 'AAAA', 'IN') + query.flags &= ~dns.flags.RD + ecso = clientsubnetoption.ClientSubnetOption('127.0.0.0', 24) + expectedQuery = dns.message.make_query(name, 'AAAA', 'IN', use_edns=True, options=[ecso], payload=4096) + expectedQuery.flags &= ~dns.flags.RD + ecsoResponse = clientsubnetoption.ClientSubnetOption('127.0.0.1', 24, 0) + expectedResponse = dns.message.make_response(query) + scopedResponse = dns.message.make_response(query) + scopedResponse.use_edns(edns=True, payload=4096, options=[ecsoResponse]) + rrset = dns.rrset.from_text(name, + ttl, + dns.rdataclass.IN, + dns.rdatatype.AAAA, + '::1') + scopedResponse.answer.append(rrset) + expectedResponse.answer.append(rrset) + + (receivedQuery, receivedResponse) = self.sendDOHQueryWrapper(query, scopedResponse) + receivedQuery.id = expectedQuery.id + self.checkMessageEDNSWithECS(expectedQuery, receivedQuery) + self.checkMessageNoEDNS(receivedResponse, expectedResponse) + + # next query should hit the cache + (receivedQuery, receivedResponse) = self.sendDOHQueryWrapper(query, response=None, useQueue=False) + self.checkMessageNoEDNS(receivedResponse, expectedResponse) + class TestCachingScopeZeroButNoSubnetcheck(DNSDistTest): _config_template = """ From 60872f6741955289de6f41407f2d80aece07b5bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Fri, 13 Dec 2024 16:02:24 +0100 Subject: [PATCH 15/25] dnsdist: ignore clangd-tidy warnings Co-authored-by: Remi Gacogne --- pdns/dnsdistdist/dnsdist-lua-ffi.cc | 2 ++ pdns/dnsdistdist/dnsdist-lua.cc | 2 ++ 2 files changed, 4 insertions(+) diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc index 92e042e285066..03ffb98a26022 100644 --- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc +++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc @@ -1056,6 +1056,7 @@ size_t dnsdist_ffi_generate_proxy_protocol_payload(const size_t addrSize, const if (valuesCount > 0) { valuesVect.reserve(valuesCount); for (size_t idx = 0; idx < valuesCount; idx++) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) valuesVect.push_back({std::string(values[idx].value, values[idx].size), values[idx].type}); } } @@ -1085,6 +1086,7 @@ size_t dnsdist_ffi_dnsquestion_generate_proxy_protocol_payload(const dnsdist_ffi if (valuesCount > 0) { valuesVect.reserve(valuesCount); for (size_t idx = 0; idx < valuesCount; idx++) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) valuesVect.push_back({std::string(values[idx].value, values[idx].size), values[idx].type}); } } diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index eec7e4dc97f41..d97c8329e0dec 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -3436,6 +3436,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return true; }); + // NOLINTNEXTLINE(performance-unnecessary-value-param) luaCtx.writeFunction("incMetric", [](const std::string& name, boost::optional> opts) { auto incOpts = opts.get_value_or(1); uint64_t step = 1; @@ -3457,6 +3458,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } return std::get(result); }); + // NOLINTNEXTLINE(performance-unnecessary-value-param) luaCtx.writeFunction("decMetric", [](const std::string& name, boost::optional> opts) { auto decOpts = opts.get_value_or(1); uint64_t step = 1; From ce95774ffbba6053a2208a60c758f604142aa84f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 11:10:32 +0100 Subject: [PATCH 16/25] dnsdist: update metrics related regression tests --- pdns/dnsdistdist/dnsdist-web.cc | 3 +++ regression-tests.dnsdist/test_API.py | 9 +++++++++ regression-tests.dnsdist/test_Prometheus.py | 20 ++++++++++++++++---- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index 772af64902865..55cbc1588dd38 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -931,6 +931,9 @@ static void addStatsToJSONObject(Json::object& obj) if (entry.d_name == "special-memory-usage") { continue; // Too expensive for get-all } + if (entry.d_labels != "") { + continue; // Skip labeled metrics to prevent duplicates + } if (const auto& val = std::get_if(&entry.d_value)) { obj.emplace(entry.d_name, (double)(*val)->load()); } diff --git a/regression-tests.dnsdist/test_API.py b/regression-tests.dnsdist/test_API.py index dd0e9fde85a72..dfdda817f8a33 100644 --- a/regression-tests.dnsdist/test_API.py +++ b/regression-tests.dnsdist/test_API.py @@ -878,6 +878,10 @@ class TestAPICustomStatistics(APITestsBase): declareMetric("my-custom-metric", "counter", "Number of statistics") declareMetric("my-other-metric", "counter", "Another number of statistics") declareMetric("my-gauge", "gauge", "Current memory usage") + declareMetric("my-labeled-gauge", "gauge", "Custom gauge with labels", { withLabels = true }) + setMetric("my-labeled-gauge", 123, { labels = { foo = "bar" } }) + declareMetric("my-labeled-counter", "counter", "Custom counter with labels", { withLabels = true }) + incMetric("my-labeled-counter", { labels = { foo = "bar" } }) setWebserverConfig({password="%s", apiKey="%s"}) """ @@ -899,3 +903,8 @@ def testCustomStats(self): for key in expected: self.assertIn(key, content) self.assertTrue(content[key] >= 0) + + unexpected = ['my-labeled-gauge', 'my-labeled-counter'] + + for key in unexpected: + self.assertNotIn(key, content) diff --git a/regression-tests.dnsdist/test_Prometheus.py b/regression-tests.dnsdist/test_Prometheus.py index f6d334038b0fa..c4559f5d9b4eb 100644 --- a/regression-tests.dnsdist/test_Prometheus.py +++ b/regression-tests.dnsdist/test_Prometheus.py @@ -30,8 +30,9 @@ class TestPrometheus(DNSDistTest): declareMetric('custom-metric3', 'counter', 'Custom counter', 'custom_prometheus_name') -- test prometheus labels in custom metrics - declareMetric('custom-metric-foo-x-bar-y-xyz', 'counter', 'Custom counter with labels', 'custom_metric_foo{x="bar",y="xyz"}') - declareMetric('custom-metric-foo-x-baz-y-abc', 'counter', 'Custom counter with labels', 'custom_metric_foo{x="baz",y="abc"}') + declareMetric('custom-metric-foo', 'counter', 'Custom counter with labels', { withLabels = true }) + incMetric('custom-metric-foo', { labels = { x = 'bar', y = 'xyz' } }) + incMetric('custom-metric-foo', { labels = { x = 'baz', y = 'abc' } }) """ def checkPrometheusContentBasic(self, content): @@ -47,12 +48,16 @@ def checkPrometheusContentBasic(self, content): tokens = line.split(' ') self.assertEqual(len(tokens), 2) if not line.startswith('dnsdist_') and not line.startswith('custom_'): - raise AssertionError('Expecting prometheus metric to be prefixed by \'dnsdist_\', got: "%s"' % (line)) + raise AssertionError( + 'Expecting prometheus metric to be prefixed by \'dnsdist_\', got: "%s"' % (line)) - def checkMetric(self, content, name, expectedType, expectedValue): + def checkMetric(self, content, name, expectedType, expectedValue, expectedLabels=""): typeFound = False helpFound = False valueFound = False + labelsFound = False + if expectedLabels == "": + labelsFound = True for line in content.splitlines(): if name in str(line): tokens = line.split(' ') @@ -70,10 +75,15 @@ def checkMetric(self, content, name, expectedType, expectedValue): if tokens[0] == name: valueFound = True self.assertEqual(int(tokens[1]), expectedValue) + elif tokens[0] == name + expectedLabels: + valueFound = True + labelsFound = True + self.assertEqual(int(tokens[1]), expectedValue) self.assertTrue(typeFound) self.assertTrue(helpFound) self.assertTrue(valueFound) + self.assertTrue(labelsFound) def checkPrometheusContentPromtool(self, content): output = None @@ -106,3 +116,5 @@ def testMetrics(self): self.checkMetric(r.text, 'dnsdist_custom_metric1', 'counter', 1) self.checkMetric(r.text, 'dnsdist_custom_metric2', 'gauge', 0) self.checkMetric(r.text, 'custom_prometheus_name', 'counter', 0) + self.checkMetric(r.text, 'dnsdist_custom_metric_foo', 'counter', 1, '{x="bar",y="xyz"}') + self.checkMetric(r.text, 'dnsdist_custom_metric_foo', 'counter', 1, '{x="baz",y="abc"}') From f58b74b096559655ce205cdc4d12299d96a4dd97 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 16 Dec 2024 11:19:17 +0100 Subject: [PATCH 17/25] rec: if the full CNAME chain leading to the answer is cached, indicate that Alternative approach to #14918 --- pdns/recursordist/syncres.cc | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9a318dfc46ba8..a3591e9311219 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1885,6 +1885,32 @@ unsigned int SyncRes::getAdjustedRecursionBound() const return bound; } +static bool haveFinalAnswer(const DNSName& qname, QType qtype, int res, const vector& ret) +{ + if (res != RCode::NoError) { + return false; + } + + // This loop assumes the CNAME's records are in-order + DNSName target(qname); + for (const auto& record : ret) { + if (record.d_place == DNSResourceRecord::ANSWER && record.d_name == target) { + if (record.d_type == qtype) { + return true; + } + if (record.d_type == QType::CNAME) { + if (auto ptr = getRR(record)) { + target = ptr->getTarget(); + } + else { + return false; + } + } + } + } + return false; +} + /*! This function will check the cache and go out to the internet if the answer is not in cache * * \param qname The name we need an answer for @@ -1986,6 +2012,11 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } + // This handles the case mentioned above: if the full CNAME chain leading to the answer was + // constructed from the cache, indicate that. + if (fromCache != nullptr && haveFinalAnswer(qname, qtype, res, ret)) { + *fromCache = true; + } return res; } @@ -2035,7 +2066,9 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } - + if (fromCache != nullptr && haveFinalAnswer(qname, qtype, res, ret)) { + *fromCache = true; + } return res; } } From 15bee38419a91c4247c3c46e083e7b3966e847fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 11:31:01 +0100 Subject: [PATCH 18/25] dnsdist: add docs for new arguments for custom metric functions --- .../docs/reference/custommetrics.rst | 72 +++++++++++++++++-- 1 file changed, 65 insertions(+), 7 deletions(-) diff --git a/pdns/dnsdistdist/docs/reference/custommetrics.rst b/pdns/dnsdistdist/docs/reference/custommetrics.rst index b2f5b517be3d2..1cda8f2329dcf 100644 --- a/pdns/dnsdistdist/docs/reference/custommetrics.rst +++ b/pdns/dnsdistdist/docs/reference/custommetrics.rst @@ -10,13 +10,19 @@ Then you can update those at runtime using the following functions, depending on * manipulate counters using :func:`incMetric` and :func:`decMetric` * update a gauge using :func:`setMetric` -.. function:: declareMetric(name, type, description [, prometheusName]) -> bool +.. function:: declareMetric(name, type, description [, prometheusName|options]) -> bool .. versionadded:: 1.8.0 .. versionchanged:: 1.8.1 This function can now be used at runtime, instead of only at configuration time. + .. versionchanged:: ? + This function now takes options, with ``withLabels`` option added. ``prometheusName`` can now be provided in options. + + .. note:: + Labels are only available for prometheus. Metrics with labels are otherwise ignored. + Re-declaring an existing metric with the same name and type will not reset it. Re-declaring with the same name but a different type will cause one of them to be masked. @@ -25,51 +31,103 @@ Then you can update those at runtime using the following functions, depending on :param str name: The name of the metric, lowercase alphanumerical characters and dashes (-) only :param str type: The desired type in ``gauge`` or ``counter`` :param str description: The description of the metric - :param str prometheusName: The name to use in the prometheus metrics, if supplied. Otherwise the regular name will be used, prefixed with ``dnsdist_`` and ``-`` replaced by ``_``. + :param str prometheusName: The name to use in the prometheus metrics, if supplied. Otherwise the regular name will be used, prefixed with ``dnsdist_`` and ``-`` replaced by ``_`` + :param table options: A table with key: value pairs with metric options. + + Options: -.. function:: incMetric(name [, step]) -> int + * ``name``: str - The name to use in the prometheus metrics, if supplied. Otherwise the regular name will be used, prefixed with ``dnsdist_`` and ``-`` replaced by ``_`` + * ``withLabels=false``: bool - If set to true, labels will be expected when updating this metric and it will not be automatically created without labels. Defaults to ``false``, which automatically creates this metric without labels with default value. + +.. function:: incMetric(name [, step|options]) -> int .. versionadded:: 1.8.0 .. versionchanged:: 1.8.1 Optional ``step`` parameter added. + .. versionchanged:: ? + This function now takes options, with ``labels`` option added. ``step`` can now be provided in options. + + .. note:: + Labels are only available for prometheus. Metrics with labels are otherwise ignored. + Increment counter by one (or more, see the ``step`` parameter), will issue an error if the metric is not declared or not a ``counter``. Returns the new value. :param str name: The name of the metric - :param int step: By how much the counter should be incremented, default to 1. + :param int step: By how much the counter should be incremented, default to 1 + :param table options: A table with key: value pairs with metric options. + + Options: + + * ``step``: int - By ow much the counter should be incremented, default to 1 + * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to increment the metric. Different combinations of labels have different metric values. -.. function:: decMetric(name) -> int +.. function:: decMetric(name [, step|options]) -> int .. versionadded:: 1.8.0 .. versionchanged:: 1.8.1 Optional ``step`` parameter added. + .. versionchanged:: ? + This function now takes options, with ``labels`` option added. ``step`` can now be provided in options. + + .. note:: + Labels are only available for prometheus. Metrics with labels are otherwise ignored. + Decrement counter by one (or more, see the ``step`` parameter), will issue an error if the metric is not declared or not a ``counter``. Returns the new value. :param str name: The name of the metric :param int step: By how much the counter should be decremented, default to 1. + :param table options: A table with key: value pairs with metric options. -.. function:: getMetric(name) -> double + Options: + + * ``step``: int - By ow much the counter should be decremented, default to 1 + * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to decrement the metric. Different combinations of labels have different metric values. + +.. function:: getMetric(name [, options]) -> double .. versionadded:: 1.8.0 + .. versionchanged:: ? + This function now takes options, with ``labels`` option added. + + .. note:: + Labels are only available for prometheus. Metrics with labels are otherwise ignored. + Get metric value. :param str name: The name of the metric + :param table options: A table with key: value pairs with metric options. + + Options: -.. function:: setMetric(name, value) -> double + * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to read the metric. Different combinations of labels have different metric values. + +.. function:: setMetric(name, value [, options]) -> double .. versionadded:: 1.8.0 + .. versionchanged:: ? + This function now takes options, with ``labels`` option added. + + .. note:: + Labels are only available for prometheus. Metrics with labels are otherwise ignored. + Set the new value, will issue an error if the metric is not declared or not a ``gauge``. Return the new value. :param str name: The name of the metric :param double value: The new value + :param table options: A table with key: value pairs with metric options. + + Options: + + * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to set the metric. Different combinations of labels have different metric values. From 882718e14f10fe5fac4d5b6e29ee556a8ce4b514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 11:42:44 +0100 Subject: [PATCH 19/25] dnsdist: use `empty()` instead of "" check --- pdns/dnsdistdist/dnsdist-web.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index 55cbc1588dd38..91ad9110a7f42 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -931,7 +931,7 @@ static void addStatsToJSONObject(Json::object& obj) if (entry.d_name == "special-memory-usage") { continue; // Too expensive for get-all } - if (entry.d_labels != "") { + if (entry.d_labels.empty()) { continue; // Skip labeled metrics to prevent duplicates } if (const auto& val = std::get_if(&entry.d_value)) { From fc8822a4cf3ca36978c8d0b5d8ed61582a56462c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 12:22:04 +0100 Subject: [PATCH 20/25] dnsdist: fix empty labels check in json stats endpoint --- pdns/dnsdistdist/dnsdist-web.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index 91ad9110a7f42..26c34edc4d148 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -931,7 +931,7 @@ static void addStatsToJSONObject(Json::object& obj) if (entry.d_name == "special-memory-usage") { continue; // Too expensive for get-all } - if (entry.d_labels.empty()) { + if (!entry.d_labels.empty()) { continue; // Skip labeled metrics to prevent duplicates } if (const auto& val = std::get_if(&entry.d_value)) { From c6258397632ec4de3e332d444878f642d2c1ef29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 17:10:28 +0100 Subject: [PATCH 21/25] dnsdist: set version changed for new custom metrics options in docs --- pdns/dnsdistdist/docs/reference/custommetrics.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pdns/dnsdistdist/docs/reference/custommetrics.rst b/pdns/dnsdistdist/docs/reference/custommetrics.rst index 1cda8f2329dcf..a2b91e41413cc 100644 --- a/pdns/dnsdistdist/docs/reference/custommetrics.rst +++ b/pdns/dnsdistdist/docs/reference/custommetrics.rst @@ -17,7 +17,7 @@ Then you can update those at runtime using the following functions, depending on .. versionchanged:: 1.8.1 This function can now be used at runtime, instead of only at configuration time. - .. versionchanged:: ? + .. versionchanged:: 2.0.0 This function now takes options, with ``withLabels`` option added. ``prometheusName`` can now be provided in options. .. note:: @@ -46,7 +46,7 @@ Then you can update those at runtime using the following functions, depending on .. versionchanged:: 1.8.1 Optional ``step`` parameter added. - .. versionchanged:: ? + .. versionchanged:: 2.0.0 This function now takes options, with ``labels`` option added. ``step`` can now be provided in options. .. note:: @@ -72,7 +72,7 @@ Then you can update those at runtime using the following functions, depending on .. versionchanged:: 1.8.1 Optional ``step`` parameter added. - .. versionchanged:: ? + .. versionchanged:: 2.0.0 This function now takes options, with ``labels`` option added. ``step`` can now be provided in options. .. note:: @@ -95,7 +95,7 @@ Then you can update those at runtime using the following functions, depending on .. versionadded:: 1.8.0 - .. versionchanged:: ? + .. versionchanged:: 2.0.0 This function now takes options, with ``labels`` option added. .. note:: @@ -114,7 +114,7 @@ Then you can update those at runtime using the following functions, depending on .. versionadded:: 1.8.0 - .. versionchanged:: ? + .. versionchanged:: 2.0.0 This function now takes options, with ``labels`` option added. .. note:: From b3f572fe3e421cdc1e0e3edaabd3bc4ec809914f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ensar=20Saraj=C4=8Di=C4=87?= Date: Mon, 16 Dec 2024 17:11:35 +0100 Subject: [PATCH 22/25] dnsdist: fix typos in custom metrics docs Co-authored-by: Remi Gacogne --- pdns/dnsdistdist/docs/reference/custommetrics.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pdns/dnsdistdist/docs/reference/custommetrics.rst b/pdns/dnsdistdist/docs/reference/custommetrics.rst index a2b91e41413cc..d688a0a1c16ce 100644 --- a/pdns/dnsdistdist/docs/reference/custommetrics.rst +++ b/pdns/dnsdistdist/docs/reference/custommetrics.rst @@ -62,7 +62,7 @@ Then you can update those at runtime using the following functions, depending on Options: - * ``step``: int - By ow much the counter should be incremented, default to 1 + * ``step``: int - By how much the counter should be incremented, default to 1 * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to increment the metric. Different combinations of labels have different metric values. .. function:: decMetric(name [, step|options]) -> int @@ -88,7 +88,7 @@ Then you can update those at runtime using the following functions, depending on Options: - * ``step``: int - By ow much the counter should be decremented, default to 1 + * ``step``: int - By how much the counter should be decremented, default to 1 * ``labels={}``: table - Set of key: value pairs with labels and their values that should be used to decrement the metric. Different combinations of labels have different metric values. .. function:: getMetric(name [, options]) -> double From 3455105d0de1b738156440b70f6711fd1e6128fa Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 16 Dec 2024 17:13:49 +0100 Subject: [PATCH 23/25] if *fromCache is already true, no need to check CNAME chain Co-authored-by: Remi Gacogne --- pdns/recursordist/syncres.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index a3591e9311219..20a7c3d4b4003 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -2014,7 +2014,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } // This handles the case mentioned above: if the full CNAME chain leading to the answer was // constructed from the cache, indicate that. - if (fromCache != nullptr && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; @@ -2066,7 +2066,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } - if (fromCache != nullptr && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; From f533aca00994264373a01b58c27efe25104e0cfa Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 17 Dec 2024 10:24:04 +0100 Subject: [PATCH 24/25] Add test The last step of the test shows that there is likely room for more improvement. --- pdns/recursordist/syncres.cc | 4 +- pdns/recursordist/test-syncres_cc1.cc | 111 ++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 20a7c3d4b4003..b8a24c5c8506c 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -2014,7 +2014,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } // This handles the case mentioned above: if the full CNAME chain leading to the answer was // constructed from the cache, indicate that. - if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && !*fromCache && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; @@ -2066,7 +2066,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } - if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && !*fromCache && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index d51953fd2ff71..af5fccaa28c59 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -1737,6 +1737,117 @@ BOOST_AUTO_TEST_CASE(test_cname_long_loop) } } +BOOST_AUTO_TEST_CASE(test_cname_long_step0_shortcut) +{ + // This tets the case fixed /optimizaed in #14973 + std::unique_ptr resolver; + initSR(resolver); + resolver->setQNameMinimization(); + primeHints(); + resolver->setLogMode(SyncRes::Store); + + size_t count = 0; + const DNSName target1("cname1.powerdns.com."); + const DNSName target2("cname2.powerdns.com."); + const DNSName target3("cname3.powerdns.com."); + const DNSName target4("cname4.powerdns.com."); + + resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int qtype, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) { + count++; + + if (domain == DNSName("com.")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (domain == DNSName("powerdns.com.")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, "ns.powerdns.com.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "ns.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (address == ComboAddress("192.0.2.2:53")) { + + if (domain == target1) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target2.toString()); + return LWResult::Result::Success; + } + if (domain == target2) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target3.toString()); + return LWResult::Result::Success; + } + if (domain == target3) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target4.toString()); + return LWResult::Result::Success; + } + if (domain == target4) { + setLWResult(res, 0, true, false, false); + if (qtype == QType::A) { + addRecordToLW(res, domain, QType::A, "1.2.3.4"); + } + else if (qtype == QType::AAAA) { + addRecordToLW(res, domain, QType::AAAA, "::1234"); + } + return LWResult::Result::Success; + } + + return LWResult::Result::Success; + } + + return LWResult::Result::Timeout; + }); + + // We analyze the trace to see how many cases of a failed Step0 occurs. This is a bit dirty, but + // we have no other way of telling how the resolving took place, as the number of cache lookups is + // not recorded by the record cache. Also, we like to know if something in Syncres changed that + // influences the number of failing Step0 lookups. + auto counter = [](const string& str) { + const std::string key = "Step0 Not cached"; + size_t occurences = 0; + auto pos = str.find(key); + while (pos != std::string::npos) { + occurences++; + pos = str.find(key, pos + 1); + } + return occurences; + }; + vector ret; + int res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 6U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + auto trace = resolver->getTrace(); + trace = resolver->getTrace(); + BOOST_CHECK_EQUAL(counter(trace), 4U); + + // And again to check cache, all Step0 cases should succeed + ret.clear(); + res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 6U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + trace = resolver->getTrace(); + BOOST_CHECK_EQUAL(counter(trace), 4U); + + // And again to check a name that does not fully resolve, we expect Step0 failures to increase + g_recCache->doWipeCache(target4, false, QType::AAAA); + ret.clear(); + res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 7U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + trace = resolver->getTrace(); + // XXX This number feels pretty high (15 extra, same as before #14973, there seems to be room for more improvement). + BOOST_CHECK_EQUAL(counter(trace), 19U); +} + BOOST_AUTO_TEST_CASE(test_cname_length) { std::unique_ptr sr; From 5c61dd3327d0a13ca4c8f871284941cb6fe33a30 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Mon, 16 Dec 2024 12:50:02 +0100 Subject: [PATCH 25/25] auth-4.9.3: docs&secpoll --- docs/changelog/4.9.rst | 14 ++++++++++++++ docs/secpoll.zone | 3 ++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/changelog/4.9.rst b/docs/changelog/4.9.rst index 920522f3bb86f..9621e3ed2d32f 100644 --- a/docs/changelog/4.9.rst +++ b/docs/changelog/4.9.rst @@ -1,6 +1,20 @@ Changelogs for 4.9.x ==================== +.. changelog:: + :version: 4.9.3 + :released: 17th of December 2024 + + This is release 4.9.3 of the Authoritative Server. + + Please review the :doc:`Upgrade Notes <../upgrading>` before upgrading from versions < 4.9.x. + + .. change:: + :tags: Bug Fixes + :pullreq: 14968 + + SVCB parser: allow quotes around port= + .. changelog:: :version: 4.9.2 :released: 1st of October 2024 diff --git a/docs/secpoll.zone b/docs/secpoll.zone index d1c208964e5f0..ba37b4f72ece8 100644 --- a/docs/secpoll.zone +++ b/docs/secpoll.zone @@ -1,4 +1,4 @@ -@ 86400 IN SOA pdns-public-ns1.powerdns.com. peter\.van\.dijk.powerdns.com. 2024121300 10800 3600 604800 10800 +@ 86400 IN SOA pdns-public-ns1.powerdns.com. peter\.van\.dijk.powerdns.com. 2024121700 10800 3600 604800 10800 @ 3600 IN NS pdns-public-ns1.powerdns.com. @ 3600 IN NS pdns-public-ns2.powerdns.com. @@ -130,6 +130,7 @@ auth-4.9.0-beta2.security-status 60 IN TXT "2 Unsupported auth-4.9.0.security-status 60 IN TXT "1 OK" auth-4.9.1.security-status 60 IN TXT "1 OK" auth-4.9.2.security-status 60 IN TXT "1 OK" +auth-4.9.3.security-status 60 IN TXT "1 OK" ; Auth Debian auth-3.4.1-2.debian.security-status 60 IN TXT "3 Upgrade now, see https://doc.powerdns.com/3/security/powerdns-advisory-2015-01/ and https://doc.powerdns.com/3/security/powerdns-advisory-2015-02/ and https://doc.powerdns.com/3/security/powerdns-advisory-2016-02/ and https://doc.powerdns.com/3/security/powerdns-advisory-2016-03/ and https://doc.powerdns.com/3/security/powerdns-advisory-2016-04/ and https://doc.powerdns.com/3/security/powerdns-advisory-2016-05/"