Stop serving stale responses from negative cache.

Serving stale responses from the negative cache interacts poorly with
other plugins and seems to confuse users.

Fixes #3586.

plugin/cache/cache_test.go

@@ -250,6 +250,53 @@ func TestCacheZeroTTL(t *testing.T) {
 	}
 }
 
+func TestServeFromStaleNoNCache(t *testing.T) {
+	c := New()
+	c.staleUpTo = 1 * time.Hour
+	c.Next = ttlBackend(60)
+
+	req := new(dns.Msg)
+	req.SetQuestion("cached.org.", dns.TypeA)
+	ctx := context.TODO()
+
+	// Cache cached.org in ncache.
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	c.ServeDNS(ctx, rec, req)
+	if c.pcache.Len() != 1 {
+		t.Fatalf("Msg with > 0 TTL should have been cached")
+	}
+	p := c.pcache
+	c = New()
+	c.staleUpTo = 1 * time.Hour
+	c.Next = ttlBackend(60)
+	c.ncache = p
+
+	// No more backend resolutions, just from cache if available.
+	c.Next = plugin.HandlerFunc(func(context.Context, dns.ResponseWriter, *dns.Msg) (int, error) {
+		return 255, nil // Below, a 255 means we tried querying upstream.
+	})
+
+	tests := []struct {
+		name           string
+		futureMinutes  int
+		expectedResult int
+	}{
+		{"cached.org.", 30, 255},
+		{"cached.org.", 60, 255},
+		{"cached.org.", 70, 255},
+	}
+
+	for i, tt := range tests {
+		rec := dnstest.NewRecorder(&test.ResponseWriter{})
+		c.now = func() time.Time { return time.Now().Add(time.Duration(tt.futureMinutes) * time.Minute) }
+		r := req.Copy()
+		r.SetQuestion(tt.name, dns.TypeA)
+		if ret, _ := c.ServeDNS(ctx, rec, r); ret != tt.expectedResult {
+			t.Errorf("Test %d: expecting %v; got %v", i, tt.expectedResult, ret)
+		}
+	}
+}
+
 func TestServeFromStaleCache(t *testing.T) {
 	c := New()
 	c.Next = ttlBackend(60)

plugin/cache/handler.go

@@ -27,11 +27,11 @@ func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 	server := metrics.WithServer(ctx)
 
 	ttl := 0
-	i := c.getIgnoreTTL(now, state, server)
+	i, okStale := c.getIgnoreTTL(now, state, server)
 	if i != nil {
 		ttl = i.ttl(now)
 	}
-	if i == nil || -ttl >= int(c.staleUpTo.Seconds()) {
+	if i == nil || (!okStale && ttl < 0) || -ttl >= int(c.staleUpTo.Seconds()) {
 		crr := &ResponseWriter{ResponseWriter: w, Cache: c, state: state, server: server}
 		return plugin.NextOrFailure(c.Name(), c.Next, ctx, crr, r)
 	}
@@ -97,25 +97,25 @@ func (c *Cache) get(now time.Time, state request.Request, server string) (*item,
 }
 
 // getIgnoreTTL unconditionally returns an item if it exists in the cache.
-func (c *Cache) getIgnoreTTL(now time.Time, state request.Request, server string) *item {
+func (c *Cache) getIgnoreTTL(now time.Time, state request.Request, server string) (*item, bool) {
 	k := hash(state.Name(), state.QType(), state.Do())
 
 	if i, ok := c.ncache.Get(k); ok {
 		ttl := i.(*item).ttl(now)
 		if ttl > 0 || (c.staleUpTo > 0 && -ttl < int(c.staleUpTo.Seconds())) {
 			cacheHits.WithLabelValues(server, Denial).Inc()
 		}
-		return i.(*item)
+		return i.(*item), false
 	}
 	if i, ok := c.pcache.Get(k); ok {
 		ttl := i.(*item).ttl(now)
 		if ttl > 0 || (c.staleUpTo > 0 && -ttl < int(c.staleUpTo.Seconds())) {
 			cacheHits.WithLabelValues(server, Success).Inc()
 		}
-		return i.(*item)
+		return i.(*item), true
 	}
 	cacheMisses.WithLabelValues(server).Inc()
-	return nil
+	return nil, false
 }
 
 func (c *Cache) exists(state request.Request) *item {