Honor DNS RR TTLs larger than negative_dns_ttl (#1380)

Since 2017 commit fd9c47d, Squid was effectively ignoring DNS RR TTLs
that exceeded negative_dns_ttl (i.e. 60 seconds by default) because the
"find the smallest TTL across the DNS records seen so far" code in
ipcache_entry::updateTtl() mistook the "default" ipcache_entry::expires
value as the one based on an earlier seen DNS record.

In most cases, this bug decreased IP cache hit ratio.

Existing fqdncache code does not suffer from the same bug because
fqdncacheParse() always resets fqdncache_entry::expires instead of
updating it incrementally. ipcacheParse() has to update incrementally
because it is called twice per entry, once with an A answer and once
with an AAAA answer.

Ideally, ipcache_entry::expires should be made optional to eliminate
awkward "first updateTtl() call" detection, but doing so well requires
significant code changes, so that entries without a known expiration
value are not cached forever _unless_ they were loaded from /etc/hosts.
And those changes should probably be propagated to fqdncache.cc.

diff --git a/src/ipcache.cc b/src/ipcache.cc
index 37e3cd4f94fb10152f200a78d7191687e6f66b78..a88b53b432cc1ade205d33cf5c736658b54b72d3 100644 (file)
--- a/src/ipcache.cc
+++ b/src/ipcache.cc
@@ -543,8 +543,15 @@ ipcache_entry::updateTtl(const unsigned int rrTtl)
                                 Config.positiveDnsTtl); // largest value allowed
 
     const time_t rrExpires = squid_curtime + ttl;
-    if (rrExpires < expires)
+    if (addrs.size() <= 1) {
+        debugs(14, 5, "use first " << ttl << " from RR TTL " << rrTtl);
         expires = rrExpires;
+    } else if (rrExpires < expires) {
+        debugs(14, 5, "use smaller " << ttl << " from RR TTL " << rrTtl << "; was: " << (expires - squid_curtime));
+        expires = rrExpires;
+    } else {
+        debugs(14, 7, "ignore " << ttl << " from RR TTL " << rrTtl << "; keep: " << (expires - squid_curtime));
+    }
 }
 
 /// \ingroup IPCacheInternal