dnsdist: Do not keep the parsed EDNS options around

The idea to keep the EDNS options around to avoid parsing them
a second time was a nice one, but invalidation is error-prone and
this is rarely useful in practice.

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>

diff --git a/pdns/dnsdistdist/dnsdist-actions-factory.cc b/pdns/dnsdistdist/dnsdist-actions-factory.cc
index 1eb69b11f1717ccb8c584c9e33a1dc2b34909183..d6b40f8c0c9b55fb6e35c950a3152e474e99d358 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-actions-factory.cc
+++ b/pdns/dnsdistdist/dnsdist-actions-factory.cc
@@ -1780,21 +1780,16 @@ public:
     }
 
     // We need to check if EDNS Options exist
-    if (dnsquestion->ednsOptions == nullptr && !parseEDNSOptions(*dnsquestion)) {
+    auto ednsOptions = parseEDNSOptions(*dnsquestion);
+    if (!ednsOptions) {
       // Maybe parsed, but no EDNS found
       return Action::None;
     }
-    if (dnsquestion->ednsOptions == nullptr) {
-      // Parsing failed, log a warning and return
-      VERBOSESLOG(infolog("parsing EDNS options failed while looking for OpenTelemetry Trace ID"),
-                  dnsquestion->getLogger()->info(Logr::Info, "Parsing EDNS options failed while looking for OpenTelemetry Trace ID"));
-      return Action::None;
-    }
 
     if (d_useIncomingTraceparent) {
       pdns::trace::TraceID traceID;
       pdns::trace::SpanID spanID;
-      if (pdns::trace::extractOTraceIDs(*(dnsquestion->ednsOptions), EDNSOptionCode::EDNSOptionCodeEnum(d_traceparentOptionCode), traceID, spanID)) {
+      if (pdns::trace::extractOTraceIDs(*(ednsOptions), EDNSOptionCode::EDNSOptionCodeEnum(d_traceparentOptionCode), traceID, spanID)) {
         tracer->setTraceID(traceID);
         if (spanID != pdns::trace::s_emptySpanID) {
           tracer->setRootSpanID(spanID);
@@ -1821,8 +1816,6 @@ public:
       size_t existingOptLen = optLen;
       removeEDNSOptionFromOPT(reinterpret_cast<char*>(&dnsquestion->getMutableData().at(optStart)), &optLen, d_traceparentOptionCode);
       dnsquestion->getMutableData().resize(dnsquestion->getData().size() - (existingOptLen - optLen));
-      // Ensure the EDNS Option View is not out of date
-      dnsquestion->ednsOptions.reset();
     }
 
     return Action::None;

diff --git a/pdns/dnsdistdist/dnsdist-ecs.cc b/pdns/dnsdistdist/dnsdist-ecs.cc
index 17f121e75275fa265e4a483208d8b576d178361b..e94e6503abcf0a1929db6fab4f14a6959851f001 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-ecs.cc
+++ b/pdns/dnsdistdist/dnsdist-ecs.cc
@@ -466,23 +466,20 @@ static bool replaceEDNSClientSubnetOption(PacketBuffer& packet, size_t maximumSi
 
 /* This function looks for an OPT RR, return true if a valid one was found (even if there was no options)
    and false otherwise. */
-bool parseEDNSOptions(const DNSQuestion& dnsQuestion)
+std::optional<EDNSOptionViewMap> parseEDNSOptions(const DNSQuestion& dnsQuestion)
 {
+  EDNSOptionViewMap ednsOptions{};
   const auto dnsHeader = dnsQuestion.getHeader();
-  if (dnsQuestion.ednsOptions != nullptr) {
-    return true;
-  }
-
-  // dnsQuestion.ednsOptions is mutable
-  dnsQuestion.ednsOptions = std::make_unique<EDNSOptionViewMap>();
-
   if (ntohs(dnsHeader->arcount) == 0) {
     /* nothing in additional so no EDNS */
-    return false;
+    return std::nullopt;
   }
 
   if (ntohs(dnsHeader->ancount) != 0 || ntohs(dnsHeader->nscount) != 0 || ntohs(dnsHeader->arcount) > 1) {
-    return slowParseEDNSOptions(dnsQuestion.getData(), *dnsQuestion.ednsOptions);
+    if (slowParseEDNSOptions(dnsQuestion.getData(), ednsOptions)) {
+      return ednsOptions;
+    }
+    return std::nullopt;
   }
 
   size_t remaining = 0;
@@ -491,11 +488,14 @@ bool parseEDNSOptions(const DNSQuestion& dnsQuestion)
 
   if (res == 0) {
     // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
-    res = getEDNSOptions(reinterpret_cast<const char*>(&dnsQuestion.getData().at(optRDPosition)), remaining, *dnsQuestion.ednsOptions);
-    return (res == 0);
+    res = getEDNSOptions(reinterpret_cast<const char*>(&dnsQuestion.getData().at(optRDPosition)), remaining, ednsOptions);
+    if (res != 0) {
+      return std::nullopt;
+    }
+    return ednsOptions;
   }
 
-  return false;
+  return std::nullopt;
 }
 
 static bool addECSToExistingOPT(PacketBuffer& packet, size_t maximumSize, const string& newECSOption, size_t optRDLenPosition, bool& ecsAdded)

diff --git a/pdns/dnsdistdist/dnsdist-ecs.hh b/pdns/dnsdistdist/dnsdist-ecs.hh
index 300195a110ba3dac1b0a9bd348de29f3d4b0a5cd..3692ffec9fd688688c39bb75d6cf80c158b10737 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-ecs.hh
+++ b/pdns/dnsdistdist/dnsdist-ecs.hh
@@ -23,6 +23,7 @@
 
 #include <string>
 
+#include "ednsoptions.hh"
 #include "iputils.hh"
 #include "noinitvector.hh"
 
@@ -46,7 +47,7 @@ bool setNegativeAndAdditionalSOA(DNSQuestion& dnsQuestion, bool nxd, const DNSNa
 bool handleEDNSClientSubnet(DNSQuestion& dnsQuestion, bool& ednsAdded, bool& ecsAdded);
 bool handleEDNSClientSubnet(PacketBuffer& packet, size_t maximumSize, size_t qnameWireLength, bool& ednsAdded, bool& ecsAdded, bool overrideExisting, const string& newECSOption);
 
-bool parseEDNSOptions(const DNSQuestion& dnsQuestion);
+std::optional<EDNSOptionViewMap> parseEDNSOptions(const DNSQuestion& dnsQuestion);
 
 bool queryHasEDNS(const DNSQuestion& dnsQuestion);
 bool getEDNS0Record(const PacketBuffer& packet, EDNS0Record& edns0);

diff --git a/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc b/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc
index 1b2162940c41157babbf57a3e3ddd7885865cfca..f6e36155e25e6b9288949c4ba2cab9c0c4664ca6 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc
@@ -164,15 +164,13 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx)
       return true;
     });
   });
-  luaCtx.registerFunction<std::map<uint16_t, EDNSOptionView> (DNSQuestion::*)() const>("getEDNSOptions", [](const DNSQuestion& dnsQuestion) {
-    if (dnsQuestion.ednsOptions == nullptr) {
-      parseEDNSOptions(dnsQuestion);
-      if (dnsQuestion.ednsOptions == nullptr) {
-        throw std::runtime_error("parseEDNSOptions should have populated the EDNS options");
-      }
+  luaCtx.registerFunction<std::map<uint16_t, EDNSOptionView> (DNSQuestion::*)() const>("getEDNSOptions", [](const DNSQuestion& dnsQuestion) -> std::map<uint16_t, EDNSOptionView> {
+    auto ednsOptions = parseEDNSOptions(dnsQuestion);
+    if (!ednsOptions) {
+      return {};
     }
 
-    return *dnsQuestion.ednsOptions;
+    return *ednsOptions;
   });
   luaCtx.registerFunction<std::string (DNSQuestion::*)(void) const>("getTrailingData", [](const DNSQuestion& dnsQuestion) {
     return dnsQuestion.getTrailingData();
@@ -516,15 +514,13 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx)
     });
   });
 
-  luaCtx.registerFunction<std::map<uint16_t, EDNSOptionView> (DNSResponse::*)() const>("getEDNSOptions", [](const DNSResponse& dnsQuestion) {
-    if (dnsQuestion.ednsOptions == nullptr) {
-      parseEDNSOptions(dnsQuestion);
-      if (dnsQuestion.ednsOptions == nullptr) {
-        throw std::runtime_error("parseEDNSOptions should have populated the EDNS options");
-      }
+  luaCtx.registerFunction<std::map<uint16_t, EDNSOptionView> (DNSResponse::*)() const>("getEDNSOptions", [](const DNSResponse& dnsQuestion) -> std::map<uint16_t, EDNSOptionView> {
+    auto ednsOptions = parseEDNSOptions(dnsQuestion);
+    if (!ednsOptions) {
+      return {};
     }
 
-    return *dnsQuestion.ednsOptions;
+    return *ednsOptions;
   });
   luaCtx.registerFunction<std::string (DNSResponse::*)(void) const>("getTrailingData", [](const DNSResponse& dnsQuestion) {
     return dnsQuestion.getTrailingData();

diff --git a/pdns/dnsdistdist/dnsdist-lua-ffi.cc b/pdns/dnsdistdist/dnsdist-lua-ffi.cc
index c764b63677817a5d6c2e32c275a70b467c4a0167..4c953d7e2eb73e772aef6b0d331ce9fd84161f35 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-lua-ffi.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-ffi.cc
@@ -399,16 +399,13 @@ static void fill_edns_option(const EDNSOptionViewValue& value, dnsdist_ffi_ednso
 // returns the length of the resulting 'out' array. 'out' is not set if the length is 0
 size_t dnsdist_ffi_dnsquestion_get_edns_options(dnsdist_ffi_dnsquestion_t* dq, const dnsdist_ffi_ednsoption_t** out)
 {
-  if (dq->dq->ednsOptions == nullptr) {
-    parseEDNSOptions(*(dq->dq));
-
-    if (dq->dq->ednsOptions == nullptr) {
-      return 0;
-    }
+  auto ednsOptions = parseEDNSOptions(*(dq->dq));
+  if (!ednsOptions) {
+    return 0;
   }
 
   size_t totalCount = 0;
-  for (const auto& option : *dq->dq->ednsOptions) {
+  for (const auto& option : *ednsOptions) {
     totalCount += option.second.values.size();
   }
 
@@ -418,7 +415,7 @@ size_t dnsdist_ffi_dnsquestion_get_edns_options(dnsdist_ffi_dnsquestion_t* dq, c
   dq->ednsOptionsVect->clear();
   dq->ednsOptionsVect->resize(totalCount);
   size_t pos = 0;
-  for (const auto& option : *dq->dq->ednsOptions) {
+  for (const auto& option : *ednsOptions) {
     for (const auto& entry : option.second.values) {
       fill_edns_option(entry, dq->ednsOptionsVect->at(pos));
       dq->ednsOptionsVect->at(pos).optionCode = option.first;

diff --git a/pdns/dnsdistdist/dnsdist.hh b/pdns/dnsdistdist/dnsdist.hh
index 1810227b0d32a84884655fa60a281394fe938c36..de74e31e7f68f18b9a8cc292f81a232c510e04aa 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.hh
+++ b/pdns/dnsdistdist/dnsdist.hh
@@ -43,7 +43,6 @@
 #include "dnsdist-doh-common.hh"
 #include "doq.hh"
 #include "doh3.hh"
-#include "ednsoptions.hh"
 #include "iputils.hh"
 #include "misc.hh"
 #include "mplexer.hh"
@@ -78,7 +77,6 @@ struct DNSQuestion
   }
   PacketBuffer& getMutableData()
   {
-    ednsOptions.reset();
     return data;
   }
 
@@ -201,7 +199,6 @@ public:
   InternalQueryState& ids;
   std::unique_ptr<Netmask> ecs{nullptr};
   std::string sni; /* Server Name Indication, if any (DoT or DoH) */
-  mutable std::unique_ptr<EDNSOptionViewMap> ednsOptions; /* this needs to be mutable because it is parsed just in time, when DNSQuestion is read-only */
   std::shared_ptr<IncomingTCPConnectionState> d_incomingTCPState{nullptr};
   std::unique_ptr<std::vector<ProxyProtocolValue>> proxyProtocolValues{nullptr};
   uint16_t ecsPrefixLength;

diff --git a/pdns/dnsdistdist/test-dnsdist_cc.cc b/pdns/dnsdistdist/test-dnsdist_cc.cc
index 91440d50bce9c71f7a0d45bd05b21510270dbeab..9e3cdb6e40bc34cc4940aa3a8179b9a0d88e9b8b 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdist_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdist_cc.cc
@@ -164,11 +164,11 @@ static void validateECS(const PacketBuffer& packet, const ComboAddress& expected
   ids.qname = DNSName(reinterpret_cast<const char*>(packet.data()), packet.size(), sizeof(dnsheader), false, &ids.qtype, &ids.qclass);
   // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
   DNSQuestion dnsQuestion(ids, const_cast<PacketBuffer&>(packet));
-  BOOST_CHECK(parseEDNSOptions(dnsQuestion));
-  BOOST_REQUIRE(dnsQuestion.ednsOptions != nullptr);
-  BOOST_CHECK_EQUAL(dnsQuestion.ednsOptions->size(), 1U);
-  const auto& ecsOption = dnsQuestion.ednsOptions->find(EDNSOptionCode::ECS);
-  BOOST_REQUIRE(ecsOption != dnsQuestion.ednsOptions->cend());
+  auto ednsOptions = parseEDNSOptions(dnsQuestion);
+  BOOST_REQUIRE(ednsOptions);
+  BOOST_CHECK_EQUAL(ednsOptions->size(), 1U);
+  const auto& ecsOption = ednsOptions->find(EDNSOptionCode::ECS);
+  BOOST_REQUIRE(ecsOption != ednsOptions->cend());
 
   string expectedOption;
   generateECSOption(expected, expectedOption, expected.sin4.sin_family == AF_INET ? ECSSourcePrefixV4 : ECSSourcePrefixV6);
@@ -2466,11 +2466,11 @@ BOOST_AUTO_TEST_CASE(test_setEDNSOption)
   BOOST_CHECK_EQUAL(edns0.extRCode, 0U);
   BOOST_CHECK_EQUAL(ntohs(edns0.extFlags), EDNS_HEADER_FLAG_DO);
 
-  BOOST_REQUIRE(parseEDNSOptions(dnsQuestion));
-  BOOST_REQUIRE(dnsQuestion.ednsOptions != nullptr);
-  BOOST_CHECK_EQUAL(dnsQuestion.ednsOptions->size(), 1U);
-  const auto& ecsOption = dnsQuestion.ednsOptions->find(EDNSOptionCode::COOKIE);
-  BOOST_REQUIRE(ecsOption != dnsQuestion.ednsOptions->cend());
+  auto ednsOptions = parseEDNSOptions(dnsQuestion);
+  BOOST_REQUIRE(ednsOptions);
+  BOOST_CHECK_EQUAL(ednsOptions->size(), 1U);
+  const auto& ecsOption = ednsOptions->find(EDNSOptionCode::COOKIE);
+  BOOST_REQUIRE(ecsOption != ednsOptions->cend());
 
   BOOST_REQUIRE_EQUAL(ecsOption->second.values.size(), 1U);
   BOOST_CHECK_EQUAL(cookiesOptionStr, std::string(ecsOption->second.values.at(0).content, ecsOption->second.values.at(0).size));