From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 16 Nov 2023 09:53:42 +0000 (+0100)
Subject: dnsdist: Require a minimum cache-hit ratio in `DynBlockRulesGroup:setCacheMissRatio()`
X-Git-Tag: dnsdist-1.9.0-alpha4~10^2~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=85e3d5104423d17d12589c384b679db4511d1ffc;p=thirdparty%2Fpdns.git

dnsdist: Require a minimum cache-hit ratio in `DynBlockRulesGroup:setCacheMissRatio()`
---

diff --git a/pdns/dnsdist-dynblocks.hh b/pdns/dnsdist-dynblocks.hh
index 62c58c0528..09b67a8ae2 100644
--- a/pdns/dnsdist-dynblocks.hh
+++ b/pdns/dnsdist-dynblocks.hh
@@ -85,81 +85,21 @@ private:
 
   struct DynBlockRule
   {
-    DynBlockRule() :
-      d_enabled(false)
+    DynBlockRule() = default;
+    DynBlockRule(const std::string& blockReason, unsigned int blockDuration, unsigned int rate, unsigned int warningRate, unsigned int seconds, DNSAction::Action action): d_blockReason(blockReason), d_blockDuration(blockDuration), d_rate(rate), d_warningRate(warningRate), d_seconds(seconds), d_action(action), d_enabled(true)
     {
     }
 
-    DynBlockRule(const std::string& blockReason, unsigned int blockDuration, unsigned int rate, unsigned int warningRate, unsigned int seconds, DNSAction::Action action) :
-      d_blockReason(blockReason), d_blockDuration(blockDuration), d_rate(rate), d_warningRate(warningRate), d_seconds(seconds), d_action(action), d_enabled(true)
-    {
-    }
-
-    bool matches(const struct timespec& when)
-    {
-      if (!d_enabled) {
-        return false;
-      }
-
-      if (d_seconds && when < d_cutOff) {
-        return false;
-      }
-
-      if (when < d_minTime) {
-        d_minTime = when;
-      }
-
-      return true;
-    }
-
-    bool rateExceeded(unsigned int count, const struct timespec& now) const
-    {
-      if (!d_enabled) {
-        return false;
-      }
-
-      double delta = d_seconds ? d_seconds : DiffTime(now, d_minTime);
-      double limit = delta * d_rate;
-      return (count > limit);
-    }
-
-    bool warningRateExceeded(unsigned int count, const struct timespec& now) const
-    {
-      if (!d_enabled) {
-        return false;
-      }
-
-      if (d_warningRate == 0) {
-        return false;
-      }
-
-      double delta = d_seconds ? d_seconds : DiffTime(now, d_minTime);
-      double limit = delta * d_warningRate;
-      return (count > limit);
-    }
+    bool matches(const struct timespec& when);
+    bool rateExceeded(unsigned int count, const struct timespec& now) const;
+    bool warningRateExceeded(unsigned int count, const struct timespec& now) const;
 
     bool isEnabled() const
     {
       return d_enabled;
     }
 
-    std::string toString() const
-    {
-      if (!isEnabled()) {
-        return "";
-      }
-
-      std::stringstream result;
-      if (d_action != DNSAction::Action::None) {
-        result << DNSAction::typeToString(d_action) << " ";
-      }
-      else {
-        result << "Apply the global DynBlock action ";
-      }
-      result << "for " << std::to_string(d_blockDuration) << " seconds when over " << std::to_string(d_rate) << " during the last " << d_seconds << " seconds, reason: '" << d_blockReason << "'";
-
-      return result.str();
-    }
+    std::string toString() const;
 
     std::string d_blockReason;
     struct timespec d_cutOff;
@@ -174,69 +114,33 @@ private:
 
   struct DynBlockRatioRule : DynBlockRule
   {
-    DynBlockRatioRule() :
-      DynBlockRule()
+    DynBlockRatioRule() = default;
+    DynBlockRatioRule(const std::string& blockReason, unsigned int blockDuration, double ratio, double warningRatio, unsigned int seconds, DNSAction::Action action, size_t minimumNumberOfResponses): DynBlockRule(blockReason, blockDuration, 0, 0, seconds, action), d_minimumNumberOfResponses(minimumNumberOfResponses), d_ratio(ratio), d_warningRatio(warningRatio)
     {
     }
 
-    DynBlockRatioRule(const std::string& blockReason, unsigned int blockDuration, double ratio, double warningRatio, unsigned int seconds, DNSAction::Action action, size_t minimumNumberOfResponses) :
-      DynBlockRule(blockReason, blockDuration, 0, 0, seconds, action), d_minimumNumberOfResponses(minimumNumberOfResponses), d_ratio(ratio), d_warningRatio(warningRatio)
-    {
-    }
-
-    bool ratioExceeded(unsigned int total, unsigned int count) const
-    {
-      if (!d_enabled) {
-        return false;
-      }
-
-      if (total < d_minimumNumberOfResponses) {
-        return false;
-      }
+    bool ratioExceeded(unsigned int total, unsigned int count) const;
+    bool warningRatioExceeded(unsigned int total, unsigned int count) const;
+    std::string toString() const;
 
-      double allowed = d_ratio * static_cast<double>(total);
-      return (count > allowed);
-    }
+    size_t d_minimumNumberOfResponses{0};
+    double d_ratio{0.0};
+    double d_warningRatio{0.0};
+  };
 
-    bool warningRatioExceeded(unsigned int total, unsigned int count) const
+  struct DynBlockCacheMissRatioRule : public DynBlockRatioRule
+  {
+    DynBlockCacheMissRatioRule() = default;
+    DynBlockCacheMissRatioRule(const std::string& blockReason, unsigned int blockDuration, double ratio, double warningRatio, unsigned int seconds, DNSAction::Action action, size_t minimumNumberOfResponses, double minimumGlobalCacheHitRatio): DynBlockRatioRule(blockReason, blockDuration, ratio, warningRatio, seconds, action, minimumNumberOfResponses), d_minimumGlobalCacheHitRatio(minimumGlobalCacheHitRatio)
     {
-      if (!d_enabled) {
-        return false;
-      }
-
-      if (d_warningRatio == 0.0) {
-        return false;
-      }
-
-      if (total < d_minimumNumberOfResponses) {
-        return false;
-      }
-
-      double allowed = d_warningRatio * static_cast<double>(total);
-      return (count > allowed);
     }
 
-    std::string toString() const
-    {
-      if (!isEnabled()) {
-        return "";
-      }
-
-      std::stringstream result;
-      if (d_action != DNSAction::Action::None) {
-        result << DNSAction::typeToString(d_action) << " ";
-      }
-      else {
-        result << "Apply the global DynBlock action ";
-      }
-      result << "for " << std::to_string(d_blockDuration) << " seconds when over " << std::to_string(d_ratio) << " ratio during the last " << d_seconds << " seconds, reason: '" << d_blockReason << "'";
-
-      return result.str();
-    }
+    bool checkGlobalCacheHitRatio() const;
+    bool ratioExceeded(unsigned int total, unsigned int count) const;
+    bool warningRatioExceeded(unsigned int total, unsigned int count) const;
+    std::string toString() const;
 
-    size_t d_minimumNumberOfResponses{0};
-    double d_ratio{0.0};
-    double d_warningRatio{0.0};
+    double d_minimumGlobalCacheHitRatio{0.0};
   };
 
   using counts_t = std::unordered_map<AddressAndPortRange, Counts, AddressAndPortRange::hash>;
@@ -275,9 +179,9 @@ public:
     entry = DynBlockRule(reason, blockDuration, rate, warningRate, seconds, action);
   }
 
-  void setCacheMissRatio(double ratio, double warningRatio, unsigned int seconds, const std::string& reason, unsigned int blockDuration, DNSAction::Action action, size_t minimumNumberOfResponses)
+  void setCacheMissRatio(double ratio, double warningRatio, unsigned int seconds, const std::string& reason, unsigned int blockDuration, DNSAction::Action action, size_t minimumNumberOfResponses, double minimumGlobalCacheHitRatio)
   {
-    d_respCacheMissRatioRule = DynBlockRatioRule(reason, blockDuration, ratio, warningRatio, seconds, action, minimumNumberOfResponses);
+    d_respCacheMissRatioRule = DynBlockCacheMissRatioRule(reason, blockDuration, ratio, warningRatio, seconds, action, minimumNumberOfResponses, minimumGlobalCacheHitRatio);
   }
 
   using smtVisitor_t = std::function<std::tuple<bool, boost::optional<std::string>, boost::optional<int>>(const StatNode&, const StatNode::Stat&, const StatNode::Stat&)>;
@@ -358,6 +262,7 @@ public:
     result << "Query rate rule: " << d_queryRateRule.toString() << std::endl;
     result << "Response rate rule: " << d_respRateRule.toString() << std::endl;
     result << "SuffixMatch rule: " << d_suffixMatchRule.toString() << std::endl;
+    result << "Response cache-miss ratio rule: " << d_respCacheMissRatioRule.toString() << std::endl;
     result << "RCode rules: " << std::endl;
     for (const auto& rule : d_rcodeRules) {
       result << "- " << RCode::to_s(rule.first) << ": " << rule.second.toString() << std::endl;
@@ -426,7 +331,7 @@ private:
   DynBlockRule d_queryRateRule;
   DynBlockRule d_respRateRule;
   DynBlockRule d_suffixMatchRule;
-  DynBlockRatioRule d_respCacheMissRatioRule;
+  DynBlockCacheMissRatioRule d_respCacheMissRatioRule;
   NetmaskGroup d_excludedSubnets;
   SuffixMatchNode d_excludedDomains;
   smtVisitor_t d_smtVisitor;
diff --git a/pdns/dnsdist-lua-inspection.cc b/pdns/dnsdist-lua-inspection.cc
index dcd5496e99..35b5c8b9b3 100644
--- a/pdns/dnsdist-lua-inspection.cc
+++ b/pdns/dnsdist-lua-inspection.cc
@@ -876,9 +876,9 @@ void setupLuaInspection(LuaContext& luaCtx)
         group->setQTypeRate(qtype, rate, warningRate ? *warningRate : 0, seconds, reason, blockDuration, action ? *action : DNSAction::Action::None);
       }
     });
-  luaCtx.registerFunction<void(std::shared_ptr<DynBlockRulesGroup>::*)(double, unsigned int, const std::string&, unsigned int, size_t, boost::optional<DNSAction::Action>, boost::optional<double>)>("setCacheMissRatio", [](std::shared_ptr<DynBlockRulesGroup>& group, double ratio, unsigned int seconds, const std::string& reason, unsigned int blockDuration, size_t minimumNumberOfResponses, boost::optional<DNSAction::Action> action, boost::optional<double> warningRatio) {
+  luaCtx.registerFunction<void(std::shared_ptr<DynBlockRulesGroup>::*)(double, unsigned int, const std::string&, unsigned int, size_t, double, boost::optional<DNSAction::Action>, boost::optional<double>)>("setCacheMissRatio", [](std::shared_ptr<DynBlockRulesGroup>& group, double ratio, unsigned int seconds, const std::string& reason, unsigned int blockDuration, size_t minimumNumberOfResponses, double minimumGlobalCacheHitRatio, boost::optional<DNSAction::Action> action, boost::optional<double> warningRatio) {
       if (group) {
-        group->setCacheMissRatio(ratio, warningRatio ? *warningRatio : 0.0, seconds, reason, blockDuration, action ? *action : DNSAction::Action::None, minimumNumberOfResponses);
+        group->setCacheMissRatio(ratio, warningRatio ? *warningRatio : 0.0, seconds, reason, blockDuration, action ? *action : DNSAction::Action::None, minimumNumberOfResponses, minimumGlobalCacheHitRatio);
       }
     });
   luaCtx.registerFunction<void(std::shared_ptr<DynBlockRulesGroup>::*)(uint8_t, uint8_t, uint8_t)>("setMasks", [](std::shared_ptr<DynBlockRulesGroup>& group, uint8_t v4, uint8_t v6, uint8_t port) {
diff --git a/pdns/dnsdistdist/dnsdist-dynblocks.cc b/pdns/dnsdistdist/dnsdist-dynblocks.cc
index 1225f3932e..e09130a530 100644
--- a/pdns/dnsdistdist/dnsdist-dynblocks.cc
+++ b/pdns/dnsdistdist/dnsdist-dynblocks.cc
@@ -830,4 +830,163 @@ std::map<std::string, std::list<std::pair<DNSName, unsigned int>>> DynBlockMaint
 {
   return s_tops.lock()->topSMTsByReason;
 }
+
+std::string DynBlockRulesGroup::DynBlockRule::toString() const
+{
+  if (!isEnabled()) {
+    return "";
+  }
+
+  std::stringstream result;
+  if (d_action != DNSAction::Action::None) {
+    result << DNSAction::typeToString(d_action) << " ";
+  }
+  else {
+    result << "Apply the global DynBlock action ";
+  }
+  result << "for " << std::to_string(d_blockDuration) << " seconds when over " << std::to_string(d_rate) << " during the last " << d_seconds << " seconds, reason: '" << d_blockReason << "'";
+
+  return result.str();
+}
+
+bool DynBlockRulesGroup::DynBlockRule::matches(const struct timespec& when)
+{
+  if (!d_enabled) {
+    return false;
+  }
+
+  if (d_seconds && when < d_cutOff) {
+    return false;
+  }
+
+  if (when < d_minTime) {
+    d_minTime = when;
+  }
+
+  return true;
+}
+
+bool DynBlockRulesGroup::DynBlockRule::rateExceeded(unsigned int count, const struct timespec& now) const
+{
+  if (!d_enabled) {
+    return false;
+  }
+
+  double delta = d_seconds ? d_seconds : DiffTime(now, d_minTime);
+  double limit = delta * d_rate;
+  return (count > limit);
+}
+
+bool DynBlockRulesGroup::DynBlockRule::warningRateExceeded(unsigned int count, const struct timespec& now) const
+{
+  if (!d_enabled) {
+    return false;
+  }
+
+  if (d_warningRate == 0) {
+    return false;
+  }
+
+  double delta = d_seconds ? d_seconds : DiffTime(now, d_minTime);
+  double limit = delta * d_warningRate;
+  return (count > limit);
+}
+
+bool DynBlockRulesGroup::DynBlockRatioRule::ratioExceeded(unsigned int total, unsigned int count) const
+{
+  if (!d_enabled) {
+    return false;
+  }
+
+  if (total < d_minimumNumberOfResponses) {
+    return false;
+  }
+
+  double allowed = d_ratio * static_cast<double>(total);
+  return (count > allowed);
+}
+
+bool DynBlockRulesGroup::DynBlockRatioRule::warningRatioExceeded(unsigned int total, unsigned int count) const
+{
+  if (!d_enabled) {
+    return false;
+  }
+
+  if (d_warningRatio == 0.0) {
+    return false;
+  }
+
+  if (total < d_minimumNumberOfResponses) {
+    return false;
+  }
+
+  double allowed = d_warningRatio * static_cast<double>(total);
+  return (count > allowed);
+}
+
+std::string DynBlockRulesGroup::DynBlockRatioRule::toString() const
+{
+  if (!isEnabled()) {
+    return "";
+  }
+
+  std::stringstream result;
+  if (d_action != DNSAction::Action::None) {
+    result << DNSAction::typeToString(d_action) << " ";
+  }
+  else {
+    result << "Apply the global DynBlock action ";
+  }
+  result << "for " << std::to_string(d_blockDuration) << " seconds when over " << std::to_string(d_ratio) << " ratio during the last " << d_seconds << " seconds, reason: '" << d_blockReason << "'";
+
+  return result.str();
+}
+
+bool DynBlockRulesGroup::DynBlockCacheMissRatioRule::checkGlobalCacheHitRatio() const
+{
+  auto globalMisses = dnsdist::metrics::g_stats.cacheMisses.load();
+  auto globalHits = dnsdist::metrics::g_stats.cacheHits.load();
+  if (globalMisses == 0 || globalHits == 0) {
+    return false;
+  }
+  double globalCacheHitRatio = static_cast<double>(globalHits) / (globalHits + globalMisses);
+  return globalCacheHitRatio >= d_minimumGlobalCacheHitRatio;
+}
+
+bool DynBlockRulesGroup::DynBlockCacheMissRatioRule::ratioExceeded(unsigned int total, unsigned int count) const
+{
+  if (!DynBlockRulesGroup::DynBlockRatioRule::ratioExceeded(total, count)) {
+    return false;
+  }
+
+  return checkGlobalCacheHitRatio();
+}
+
+bool DynBlockRulesGroup::DynBlockCacheMissRatioRule::warningRatioExceeded(unsigned int total, unsigned int count) const
+{
+  if (!DynBlockRulesGroup::DynBlockRatioRule::warningRatioExceeded(total, count)) {
+    return false;
+  }
+
+  return checkGlobalCacheHitRatio();
+}
+
+std::string DynBlockRulesGroup::DynBlockCacheMissRatioRule::toString() const
+{
+  if (!isEnabled()) {
+    return "";
+  }
+
+  std::stringstream result;
+  if (d_action != DNSAction::Action::None) {
+    result << DNSAction::typeToString(d_action) << " ";
+  }
+  else {
+    result << "Apply the global DynBlock action ";
+  }
+  result << "for " << std::to_string(d_blockDuration) << " seconds when over " << std::to_string(d_ratio) << " ratio during the last " << d_seconds << " seconds, with a global cache-hit ratio of at least " << d_minimumGlobalCacheHitRatio << ", reason: '" << d_blockReason << "'";
+
+  return result.str();
+}
+
 #endif /* DISABLE_DYNBLOCKS */
diff --git a/pdns/dnsdistdist/docs/reference/config.rst b/pdns/dnsdistdist/docs/reference/config.rst
index 7f33304d66..b4055536cd 100644
--- a/pdns/dnsdistdist/docs/reference/config.rst
+++ b/pdns/dnsdistdist/docs/reference/config.rst
@@ -1629,19 +1629,21 @@ faster than the existing rules.
 
   Represents a group of dynamic block rules.
 
-  .. method:: DynBlockRulesGroup:setCacheMissRatio(ratio, seconds, reason, blockingTime, minimumNumberOfResponses [, action [, warningRate]])
+  .. method:: DynBlockRulesGroup:setCacheMissRatio(ratio, seconds, reason, blockingTime, minimumNumberOfResponses, minimumGlobalCacheHitRatio, [, action [, warningRate]])
 
     .. versionadded:: 1.9.0
 
     Adds a rate-limiting rule for the ratio of cache-misses responses over the total number of responses for a given client.
+    A minimum global cache-hit ratio has to specified to prevent false-positive when the cache is empty.
 
-    :param int ratio: Ratio of cache-miss responses per second over the total number of responses for this client to exceed
+    :param float ratio: Ratio of cache-miss responses per second over the total number of responses for this client to exceed
     :param int seconds: Number of seconds the ratio has been exceeded
     :param string reason: The message to show next to the blocks
     :param int blockingTime: The number of seconds this block to expire
     :param int minimumNumberOfResponses: How many total responses is required for this rule to apply
+    :param float minimumGlobalCacheHitRatio: The minimum global cache-hit ratio (over all pools, so ``cache-hits / (cache-hits + cache-misses)``) for that rule to be applied.
     :param int action: The action to take when the dynamic block matches, see :ref:`DNSAction <DNSAction>`. (default to the one set with :func:`setDynBlocksAction`)
-    :param int warningRatio: If set to a non-zero value, the ratio above which a warning message will be issued and a no-op block inserted
+    :param float warningRatio: If set to a non-zero value, the ratio above which a warning message will be issued and a no-op block inserted
 
   .. method:: DynBlockRulesGroup:setMasks(v4, v6, port)
 
@@ -1707,13 +1709,13 @@ faster than the existing rules.
     Adds a rate-limiting rule for the ratio of responses of code ``rcode`` over the total number of responses for a given client.
 
     :param int rcode: The response code
-    :param int ratio: Ratio of responses per second of the given rcode over the total number of responses for this client to exceed
+    :param float ratio: Ratio of responses per second of the given rcode over the total number of responses for this client to exceed
     :param int seconds: Number of seconds the ratio has been exceeded
     :param string reason: The message to show next to the blocks
     :param int blockingTime: The number of seconds this block to expire
     :param int minimumNumberOfResponses: How many total responses is required for this rule to apply
     :param int action: The action to take when the dynamic block matches, see :ref:`DNSAction <DNSAction>`. (default to the one set with :func:`setDynBlocksAction`)
-    :param int warningRatio: If set to a non-zero value, the ratio above which a warning message will be issued and a no-op block inserted
+    :param float warningRatio: If set to a non-zero value, the ratio above which a warning message will be issued and a no-op block inserted
 
   .. method:: DynBlockRulesGroup:setQTypeRate(qtype, rate, seconds, reason, blockingTime [, action [, warningRate]])
 
diff --git a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
index 4e01e9efc2..df481eb09a 100644
--- a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
+++ b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
@@ -9,6 +9,7 @@
 
 #include "dnsdist.hh"
 #include "dnsdist-dynblocks.hh"
+#include "dnsdist-metrics.hh"
 #include "dnsdist-rings.hh"
 
 Rings g_rings;
@@ -857,8 +858,10 @@ BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_CacheMissRatio, TestFixture) {
   DynBlockRulesGroup dbrg;
   dbrg.setQuiet(true);
 
-  /* block above 0.5 Cache-Miss/Total ratio over numberOfSeconds seconds, no warning, minimum number of queries should be at least 51 */
-  dbrg.setCacheMissRatio(0.5, 0, numberOfSeconds, reason, blockDuration, action, 51);
+  /* block above 0.5 Cache-Miss/Total ratio over numberOfSeconds seconds, no warning, minimum number of queries should be at least 51, global cache hit at least 80% */
+  dnsdist::metrics::g_stats.cacheHits.store(80);
+  dnsdist::metrics::g_stats.cacheMisses.store(20);
+  dbrg.setCacheMissRatio(0.5, 0, numberOfSeconds, reason, blockDuration, action, 51, 0.8);
 
   {
     /* insert 50 cache misses and 50 cache hits from a given client in the last 10s
@@ -927,6 +930,28 @@ BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_CacheMissRatio, TestFixture) {
     BOOST_CHECK_EQUAL(g_dynblockNMG.getLocal()->size(), 0U);
     BOOST_CHECK(g_dynblockNMG.getLocal()->lookup(requestor1) == nullptr);
   }
+
+  /* the global cache-hit rate is too low, should not trigger */
+  dnsdist::metrics::g_stats.cacheHits.store(60);
+  dnsdist::metrics::g_stats.cacheMisses.store(40);
+  {
+    /* insert 51 cache misses and 49 hits from a given client in the last 10s */
+    g_rings.clear();
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfResponseEntries(), 0U);
+    g_dynblockNMG.setState(emptyNMG);
+
+    for (size_t idx = 0; idx < 51; idx++) {
+      g_rings.insertResponse(now, requestor1, qname, qtype, responseTime, size, dnsHeader, backend, outgoingProtocol);
+    }
+    for (size_t idx = 0; idx < 49; idx++) {
+      g_rings.insertResponse(now, requestor1, qname, qtype, responseTime, size, dnsHeader, cacheHit, outgoingProtocol);
+    }
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfResponseEntries(), 100U);
+
+    dbrg.apply(now);
+    BOOST_CHECK_EQUAL(g_dynblockNMG.getLocal()->size(), 0U);
+    BOOST_REQUIRE(g_dynblockNMG.getLocal()->lookup(requestor1) == nullptr);
+  }
 }
 
 BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_Warning, TestFixture) {