Only apply "do not cache" if both limits are set and satisfied. Doc t…

…weaks.
PowerDNS · Apr 1, 2019 · 42f4180 · 42f4180
1 parent e7861cc
commit 42f4180
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 13 deletions.
diff --git a/pdns/recursordist/docs/settings.rst b/pdns/recursordist/docs/settings.rst
@@ -386,7 +386,8 @@ Number of bits of client IPv4 address to pass when sending EDNS Client Subnet ad
 -  Integer
 -  Default: 24
 
-Maximum number of bits of client IPv4 address used by the authoritative server (as indicated by the EDNS Client Subnet scope in the answer) for an answer to be inserted into the query cache.
+Maximum number of bits of client IPv4 address used by the authoritative server (as indicated by the EDNS Client Subnet scope in the answer) for an answer to be inserted into the query cache. This condition applies in conjunction with ``ecs-cache-limit-ttl``.
+That is, only if both the limits apply, the record will not be cached.
 
 .. _setting-ecs-ipv6-bits:
 
@@ -408,7 +409,8 @@ Number of bits of client IPv6 address to pass when sending EDNS Client Subnet ad
 -  Integer
 -  Default: 56
 
-Maximum number of bits of client IPv6 address used by the authoritative server (as indicated by the EDNS Client Subnet scope in the answer) for an answer to be inserted into the query cache.
+Maximum number of bits of client IPv6 address used by the authoritative server (as indicated by the EDNS Client Subnet scope in the answer) for an answer to be inserted into the query cache. This condition applies in conjunction with ``ecs-cache-limit-ttl``.
+That is, only if both the limits apply, the record will not be cached.
 
 .. _setting-ecs-minimum-ttl-override:
 
@@ -430,7 +432,8 @@ Can be set at runtime using ``rec_control set-ecs-minimum-ttl 3600``.
 -  Integer
 -  Default: 0 (disabled)
 
-The minumum TTL for an ECS-specific answer to be inserted into the query cache.
+The minumum TTL for an ECS-specific answer to be inserted into the query cache. This condition applies in conjunction with ``ecs-ipv4-cache-bits`` or ``ecs-ipv6-cache-bits``.
+That is, only if both the limits apply, the record will not be cached.
 
 .. _setting-ecs-scope-zero-address:
 

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
@@ -2143,7 +2143,7 @@ BOOST_AUTO_TEST_CASE(test_ecs_cache_limit_allowed) {
   BOOST_REQUIRE_EQUAL(cached.size(), 1);
 }
 
-BOOST_AUTO_TEST_CASE(test_ecs_cache_limit_denied) {
+BOOST_AUTO_TEST_CASE(test_ecs_cache_limit_no_ttl_limit_allowed) {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
 
@@ -2175,11 +2175,11 @@ BOOST_AUTO_TEST_CASE(test_ecs_cache_limit_denied) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(ret.size(), 1);
 
-  /* should have NOT been cached because /24 is more specific than /16 */
+  /* should have been cached because /24 is more specific than /16 but TTL limit is nof efective */
   const ComboAddress who("192.0.2.128");
   vector<DNSRecord> cached;
-  BOOST_REQUIRE_LT(t_RC->get(now, target, QType(QType::A), true, &cached, who), 0);
-  BOOST_REQUIRE_EQUAL(cached.size(), 0);
+  BOOST_REQUIRE_GT(t_RC->get(now, target, QType(QType::A), true, &cached, who), 0);
+  BOOST_REQUIRE_EQUAL(cached.size(), 1);
 }
 
 BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_allowed) {

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
@@ -2426,6 +2426,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
       if (!doCache && ednsmask) {
         bool manyMaskBits = (ednsmask->isIpv4() && ednsmask->getBits() > SyncRes::s_ecsipv4cachelimit) ||
             (ednsmask->isIpv6() && ednsmask->getBits() > SyncRes::s_ecsipv6cachelimit);
+        doCache = true;
 
         if (SyncRes::s_ecscachelimitttl > 0) {
           if (manyMaskBits) {
@@ -2439,13 +2440,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
               // Case: many bits and ttlIsSmall
               doCache = false;
             }
-          } else {
-              // Case: few mask bits
-              doCache = true;
           }
-        } else {
-          // no applicable TTL limit, scope determines cacheability
-          doCache = !manyMaskBits;
         }
       }
       if (doCache) {