From 920eef25f14be38b29acc716e5d2cb0e71a414f2 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Thu, 21 Nov 2024 11:16:59 +0100 Subject: [PATCH] Process comments from review: - rename needECS to needEDNSParse - remove lookForECS in getQNameAndSubnet - extend test to include packet cache hit case - take new dnsmessage.proto form dnsmessage repo --- pdns/dnsmessage.proto | 10 ++++----- pdns/recursordist/pdns_recursor.cc | 11 +++++----- pdns/recursordist/rec-tcp.cc | 6 +++--- .../test_Protobuf.py | 21 +++++++++++++++++++ 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/pdns/dnsmessage.proto b/pdns/dnsmessage.proto index ed97a22d6d6c..f03c633be7de 100644 --- a/pdns/dnsmessage.proto +++ b/pdns/dnsmessage.proto @@ -2,19 +2,19 @@ * This file describes the message format used by the protobuf logging feature in PowerDNS and dnsdist. * * MIT License - * + * * Copyright (c) 2016-now PowerDNS.COM B.V. and its contributors. - * + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell * copies of the Software, and to permit persons to whom the Software is * furnished to do so, subject to the following conditions: - * + * * The above copyright notice and this permission notice shall be included in all * copies or substantial portions of the Software. - * + * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE @@ -188,7 +188,7 @@ message PBDNSMessage { optional uint64 workerId = 25; // Thread id optional bool packetCacheHit = 26; // Was it a packet cache hit? optional uint32 outgoingQueries = 27; // Number of outgoing queries used to answer the query - optional uint32 headerFlags = 28; // Flags field in wire format + optional uint32 headerFlags = 28; // Flags field in wire format, 16 bits used optional uint32 ednsVersion = 29; // EDNS version and flags in wire format, see https://www.rfc-editor.org/rfc/rfc6891.html#section-6.1.3 } diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 24d3541429be..c2c5bd56ce73 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -2016,7 +2016,6 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi void getQNameAndSubnet(const std::string& question, DNSName* dnsname, uint16_t* qtype, uint16_t* qclass, bool& foundECS, EDNSSubnetOpts* ednssubnet, EDNSOptionViewMap* options, boost::optional& ednsVersion) { - const bool lookForECS = ednssubnet != nullptr; const dnsheader_aligned dnshead(question.data()); const dnsheader* dhPointer = dnshead.get(); size_t questionLen = question.length(); @@ -2027,7 +2026,7 @@ void getQNameAndSubnet(const std::string& question, DNSName* dnsname, uint16_t* const size_t headerSize = /* root */ 1 + sizeof(dnsrecordheader); const uint16_t arcount = ntohs(dhPointer->arcount); - for (uint16_t arpos = 0; arpos < arcount && questionLen >= (pos + headerSize) && (lookForECS && !foundECS); arpos++) { + for (uint16_t arpos = 0; arpos < arcount && questionLen >= (pos + headerSize) && !foundECS; arpos++) { if (question.at(pos) != 0) { /* not an OPT, bye. */ return; @@ -2047,7 +2046,7 @@ void getQNameAndSubnet(const std::string& question, DNSName* dnsname, uint16_t* } /* OPT root label (1) followed by type (2) */ - if (lookForECS && ntohs(drh->d_type) == QType::OPT) { + if (ntohs(drh->d_type) == QType::OPT) { if (options == nullptr) { size_t ecsStartPosition = 0; size_t ecsLen = 0; @@ -2201,7 +2200,7 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr const dnsheader* dnsheader = headerdata.get(); unsigned int ctag = 0; uint32_t qhash = 0; - bool needECS = false; + bool needEDNSParse = false; std::unordered_set policyTags; std::map meta; LuaContext::LuaObject data; @@ -2217,7 +2216,7 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr const auto outgoingbExport = checkOutgoingProtobufExport(luaconfsLocal); if (pbExport || outgoingbExport) { if (pbExport) { - needECS = true; + needEDNSParse = true; } uniqueId = getUniqueID(); } @@ -2256,7 +2255,7 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr #endif // We do not have a SyncRes specific Lua context at this point yet, so ok to use t_pdl - if (needECS || (t_pdl && (t_pdl->hasGettagFunc() || t_pdl->hasGettagFFIFunc())) || dnsheader->opcode == static_cast(Opcode::Notify)) { + if (needEDNSParse || (t_pdl && (t_pdl->hasGettagFunc() || t_pdl->hasGettagFFIFunc())) || dnsheader->opcode == static_cast(Opcode::Notify)) { try { EDNSOptionViewMap ednsOptions; diff --git a/pdns/recursordist/rec-tcp.cc b/pdns/recursordist/rec-tcp.cc index fe94e1ed8f9a..a5794035dc86 100644 --- a/pdns/recursordist/rec-tcp.cc +++ b/pdns/recursordist/rec-tcp.cc @@ -299,7 +299,7 @@ static void doProcessTCPQuestion(std::unique_ptr& comboWriter, s DNSName qname; uint16_t qtype = 0; uint16_t qclass = 0; - bool needECS = false; + bool needEDNSParse = false; string requestorId; string deviceId; string deviceName; @@ -311,12 +311,12 @@ static void doProcessTCPQuestion(std::unique_ptr& comboWriter, s comboWriter->d_eventTrace.add(RecEventTrace::ReqRecv); auto luaconfsLocal = g_luaconfs.getLocal(); if (checkProtobufExport(luaconfsLocal)) { - needECS = true; + needEDNSParse = true; } logQuery = t_protobufServers.servers && luaconfsLocal->protobufExportConfig.logQueries; comboWriter->d_logResponse = t_protobufServers.servers && luaconfsLocal->protobufExportConfig.logResponses; - if (needECS || (t_pdl && (t_pdl->hasGettagFFIFunc() || t_pdl->hasGettagFunc())) || comboWriter->d_mdp.d_header.opcode == static_cast(Opcode::Notify)) { + if (needEDNSParse || (t_pdl && (t_pdl->hasGettagFFIFunc() || t_pdl->hasGettagFunc())) || comboWriter->d_mdp.d_header.opcode == static_cast(Opcode::Notify)) { try { EDNSOptionViewMap ednsOptions; diff --git a/regression-tests.recursor-dnssec/test_Protobuf.py b/regression-tests.recursor-dnssec/test_Protobuf.py index 432859301e04..47936bb6400d 100644 --- a/regression-tests.recursor-dnssec/test_Protobuf.py +++ b/regression-tests.recursor-dnssec/test_Protobuf.py @@ -360,6 +360,27 @@ def testA(self): self.checkProtobufResponseRecord(rr, dns.rdataclass.IN, dns.rdatatype.A, name, 15) self.assertEqual(socket.inet_ntop(socket.AF_INET, rr.rdata), '192.0.2.42') self.checkNoRemainingMessage() + # + # again, for a PC cache hit + # + res = self.sendUDPQuery(query) + + self.assertRRsetInAnswer(res, expected) + + # check the protobuf messages corresponding to the UDP query and answer + msg = self.getFirstProtobufMessage() + self.checkProtobufQuery(msg, dnsmessage_pb2.PBDNSMessage.UDP, query, dns.rdataclass.IN, dns.rdatatype.A, name) + # wire format, RD and CD set in headerflags, plus DO bit in flags part of EDNS Version + self.checkProtobufHeaderFlagsAndEDNSVersion(msg, 0x0110, 0x00008000) + # then the response + msg = self.getFirstProtobufMessage() + self.checkProtobufResponse(msg, dnsmessage_pb2.PBDNSMessage.UDP, res, '127.0.0.1') + self.assertEqual(len(msg.response.rrs), 1) + rr = msg.response.rrs[0] + # we have max-cache-ttl set to 15 + self.checkProtobufResponseRecord(rr, dns.rdataclass.IN, dns.rdatatype.A, name, 15) + self.assertEqual(socket.inet_ntop(socket.AF_INET, rr.rdata), '192.0.2.42') + self.checkNoRemainingMessage() def testCNAME(self): name = 'cname.example.'