From 87ccb6f90e9e46a15c434888f690f4ce51cff926 Mon Sep 17 00:00:00 2001 From: Nitin Nizhawan Date: Fri, 14 Aug 2026 13:48:39 +0530 Subject: [PATCH] plugin/cache: add prefer_positive stale policy (#8378) * plugin/cache: add prefer_positive stale policy Add an opt-in serve_stale_policy that prefers an eligible success-cache answer over denial-cache entries while serve_stale is enabled. Preserve the existing ncache-first behavior when the policy is absent. Also classify SOA-backed CNAME NODATA responses in the cache so incomplete answers cannot be selected as positive stale responses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan * plugin/cache: retain last-known-good positive answers Keep an answering success-cache item reachable when a later NOERROR or referral response overwrites the visible cache key without answering the question. This lets prefer_positive survive empty responses, referrals, and additional-only data while leaving policy-off lookup behavior unchanged. Return the exact accepted verify refresh item instead of re-reading an ambiguous cache key, avoiding expired TTL wraparound for uncacheable replies. Add regression coverage for non-answer refreshes, NODATA, SERVFAIL, NOTIMP, stale-window expiry, and bounded verify reply shaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan * plugin/cache: validate preferred stale answers Reject truncated, DNSSEC-expired, mismatched-class, unrelated ANY, and ambiguous CNAME refreshes before replacing a stale last-known-good answer. Precompute answer eligibility when cache items are created so prefer_positive hits avoid repeated CNAME walks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan --------- Signed-off-by: Nitin Nizhawan Co-authored-by: Nitin Nizhawan Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 --- plugin/cache/README.md | 11 + plugin/cache/cache.go | 160 ++++++++++---- plugin/cache/cache_test.go | 428 +++++++++++++++++++++++++++++++++++++ plugin/cache/handler.go | 38 ++-- plugin/cache/item.go | 17 ++ plugin/cache/setup.go | 21 ++ plugin/cache/setup_test.go | 35 +++ 7 files changed, 658 insertions(+), 52 deletions(-) diff --git a/plugin/cache/README.md b/plugin/cache/README.md index 487b5392c..78f2fc941 100644 --- a/plugin/cache/README.md +++ b/plugin/cache/README.md @@ -41,6 +41,7 @@ cache [TTL] [ZONES...] { denial CAPACITY [TTL] [MINTTL] prefetch AMOUNT [[DURATION] [PERCENTAGE%]] serve_stale [DURATION] [immediate [RESPONSE_TTL [FAILURE_RECHECK]] | verify [VERIFY_TIMEOUT [RESPONSE_TTL [FAILURE_RECHECK]]]] + serve_stale_policy prefer_positive servfail DURATION disable success|denial [ZONES...] keepttl @@ -89,6 +90,16 @@ cache [TTL] [ZONES...] { intact. The default of `0` preserves the existing retry behavior. RFC 8767 recommends `30s` and says this value should not exceed 5 minutes. Examples: `serve_stale 1h immediate 30s 30s` and `serve_stale 1h verify 100ms 30s 30s`. +* `serve_stale_policy` controls cache selection while `serve_stale` is enabled. The only supported policy is + `prefer_positive`. It checks the success cache before the denial cache and returns an eligible positive response + when it actually answers the question, even when a cached NXDOMAIN, NODATA, SERVFAIL, or NOTIMP response also + exists. The positive response must be unexpired or within the configured `serve_stale` duration. + The positive response is retained independently when a later NOERROR response does not answer the question (for example, an empty response + without SOA, a referral, or a response carrying data only in the additional section), so such a refresh cannot + destroy the last-known-good answer. A usable positive refresh replaces the retained answer. + The policy is disabled by default because it can mask legitimate record deletion or removal until the positive response exceeds + the stale duration or is evicted. In `immediate` mode, the stale positive response is returned first and the cache + refreshes in the background. In `verify` mode, only a refreshed positive answer replaces the stale response. * `servfail` cache SERVFAIL responses for **DURATION**. Setting **DURATION** to 0 will disable caching of SERVFAIL responses. If this option is not set, SERVFAIL responses will be cached for 5 seconds. **DURATION** may not be greater than 5 minutes. diff --git a/plugin/cache/cache.go b/plugin/cache/cache.go index 19bb13d44..a8255843c 100644 --- a/plugin/cache/cache.go +++ b/plugin/cache/cache.go @@ -46,6 +46,7 @@ type Cache struct { staleUpTo time.Duration verifyStale bool verifyStaleTimeout time.Duration // 0 means wait for upstream until its own timeout (current default). + preferPositive bool staleTTL time.Duration // TTL returned with stale responses; 0 preserves the legacy behavior. staleRecheck time.Duration // Delay after a failed refresh before another attempt; 0 preserves the legacy behavior. @@ -124,6 +125,74 @@ func hasSOA(m *dns.Msg) bool { return false } +// cacheResponseType returns the response type used by the cache. Typify treats +// any NOERROR response with a non-empty answer section as NoError, but RFC 2308 +// NODATA responses may contain a CNAME chain. Reclassify those responses when +// an SOA provides the negative cache TTL. +func cacheResponseType(m *dns.Msg, now time.Time) response.Type { + t, _ := response.Typify(m, now) + if t == response.NoError && hasSOA(m) && isNODATA(m) { + return response.NoData + } + return t +} + +// answersQuestion reports whether a NOERROR response contains an answer to its +// question. For types other than CNAME and ANY, the queried type must exist at +// the terminal owner reached by following the CNAME chain from QNAME. +func answersQuestion(m *dns.Msg) bool { + if m == nil || m.Rcode != dns.RcodeSuccess || len(m.Question) == 0 || len(m.Answer) == 0 { + return false + } + q := m.Question[0] + return answerHasType(m.Answer, q.Name, q.Qtype, q.Qclass) +} + +func answerHasType(answer []dns.RR, name string, qtype, qclass uint16) bool { + if len(answer) == 0 { + return false + } + if qtype == dns.TypeANY { + for _, r := range answer { + h := r.Header() + if classMatches(h.Class, qclass) && strings.EqualFold(h.Name, name) { + return true + } + } + return false + } + if qtype == dns.TypeCNAME { + target, ok := uniqueCNAMETarget(answer, name, qclass) + return ok && target != "" + } + terminal, ok := canonicalName(answer, name, qclass) + if !ok { + return false + } + name = terminal + for _, r := range answer { + h := r.Header() + if h.Rrtype == qtype && classMatches(h.Class, qclass) && strings.EqualFold(h.Name, name) { + return true + } + } + return false +} + +func classMatches(rrClass, qclass uint16) bool { + return qclass == dns.ClassANY || rrClass == qclass +} + +// usableAnswer reports whether m is a complete, cache-valid positive response +// that answers its question. It intentionally permits TTL-zero responses: +// they are usable for the current client even though they are not retained. +func usableAnswer(m *dns.Msg, now time.Time) bool { + if m == nil || m.Truncated || cacheResponseType(m, now) != response.NoError { + return false + } + return answersQuestion(m) +} + // isNODATA reports whether a NOERROR response with a non-empty answer section // does not answer the question. Following RFC 1034 section 3.6.2 and RFC 2308 // sections 1 and 2.2, a query of any type other than CNAME (and ANY) is @@ -139,39 +208,15 @@ func hasSOA(m *dns.Msg) bool { // toward re-querying upstream rather than caching a non-answer. An empty answer // section returns false so that legitimate positive responses carrying data // outside the answer section (for example the whoami plugin) remain cacheable. -// ANY queries are excluded because any record answers them. Note: a bare DNAME -// (RFC 6672) without its synthesized CNAME is treated as NODATA; standard -// responses include the synthesized CNAME, which the chain walk follows. +// An ANY query is answered only by a record at the queried owner and in the +// requested class. Note: a bare DNAME (RFC 6672) without its synthesized CNAME +// is treated as NODATA; standard responses include the synthesized CNAME, which +// the chain walk follows. func isNODATA(m *dns.Msg) bool { if len(m.Answer) == 0 { return false } - qtype := m.Question[0].Qtype - if qtype == dns.TypeANY { - return false - } - // A CNAME query is answered by the CNAME itself, so the chain is not - // followed; otherwise resolve it to the terminal owner name. - name := m.Question[0].Name - if qtype != dns.TypeCNAME { - terminal, ok := canonicalName(m.Answer, name) - if !ok { - // The CNAME chain is malformed (an owner with more than one - // distinct target, or a loop) and therefore has no well-defined - // QNAME per RFC 2181 section 10.1 and RFC 1034 section 3.6.2. Such - // a response cannot be shown to answer the question, so treat it as - // NODATA and (being SOA-less) leave it uncacheable. - return true - } - name = terminal - } - for _, r := range m.Answer { - h := r.Header() - if h.Rrtype == qtype && strings.EqualFold(h.Name, name) { - return false - } - } - return true + return !answersQuestion(m) } // canonicalName follows the owner-linked CNAME chain in answer starting at name @@ -185,7 +230,7 @@ func isNODATA(m *dns.Msg) bool { // fail-closed: callers treat a malformed chain as a non-answer. Duplicate CNAME // records that name the same target are tolerated, since they still describe a // single canonical name. -func canonicalName(answer []dns.RR, name string) (string, bool) { +func canonicalName(answer []dns.RR, name string, qclass uint16) (string, bool) { visited := nameSet{} for { if visited.contains(name) { @@ -194,7 +239,7 @@ func canonicalName(answer []dns.RR, name string) (string, bool) { } visited.add(name) - target, ok := uniqueCNAMETarget(answer, name) + target, ok := uniqueCNAMETarget(answer, name, qclass) if !ok { // Owner has more than one distinct canonical name. return name, false @@ -212,10 +257,10 @@ func canonicalName(answer []dns.RR, name string) (string, bool) { // CNAME target, which violates RFC 2181 section 10.1. When owner has no CNAME the // returned target is empty and ok is true, marking a terminal owner. Duplicate // CNAME records naming the same target are tolerated. -func uniqueCNAMETarget(answer []dns.RR, owner string) (target string, ok bool) { +func uniqueCNAMETarget(answer []dns.RR, owner string, qclass uint16) (target string, ok bool) { for _, r := range answer { c, isCNAME := r.(*dns.CNAME) - if !isCNAME || !strings.EqualFold(c.Header().Name, owner) { + if !isCNAME || !classMatches(c.Header().Class, qclass) || !strings.EqualFold(c.Header().Name, owner) { continue } if target != "" && !strings.EqualFold(target, c.Target) { @@ -286,6 +331,8 @@ type ResponseWriter struct { remoteAddr net.Addr wildcardFunc func() string // function to retrieve wildcard name that synthesized the result. + lastResponse *dns.Msg // last response after cache TTL and DNSSEC adjustments. + lastItem *item // cache item written by the last response, if cacheable. pexcept []string // positive zone exceptions nexcept []string // negative zone exceptions @@ -364,7 +411,8 @@ func (w *ResponseWriter) Hijack() { // WriteMsg implements the dns.ResponseWriter interface. func (w *ResponseWriter) WriteMsg(res *dns.Msg) error { res = res.Copy() - mt, _ := response.Typify(res, w.now().UTC()) + w.lastItem = nil + mt := cacheResponseType(res, w.now().UTC()) // key returns empty string for anything we don't want to cache. hasKey, key := key(w.state.Name(), res, mt, w.do, w.cd) @@ -392,6 +440,7 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error { // But retain AD bit if requester set the AD bit in the request, per RFC6840 5.7-5.8 res.AuthenticatedData = false } + w.lastResponse = res.Copy() if hasKey && duration > 0 { if w.state.Match(res) { @@ -424,11 +473,19 @@ func (w *ResponseWriter) set(m *dns.Msg, key uint64, mt response.Type, duration if w.wildcardFunc != nil { i.wildcard = w.wildcardFunc() } + if w.preferPositive && !i.answering { + if previous, ok := w.pcache.Get(key); ok { + i.lastKnownGood = previous.answeringItem(w.state) + } + } if w.pcache.Add(key, i) { evictions.WithLabelValues(w.server, Success, w.zonesMetricLabel, w.viewMetricLabel).Inc() } - // when pre-fetching, remove the negative cache entry if it exists - if w.prefetch { + w.lastItem = i + // A positive refresh is the newest state for this key. Under the + // prefer_positive policy, only remove the denial when this response + // actually answers the question. + if (!w.preferPositive && w.prefetch) || (w.preferPositive && i.answering) { w.ncache.Remove(key) } @@ -444,6 +501,7 @@ func (w *ResponseWriter) set(m *dns.Msg, key uint64, mt response.Type, duration if w.ncache.Add(key, i) { evictions.WithLabelValues(w.server, Denial, w.zonesMetricLabel, w.viewMetricLabel).Inc() } + w.lastItem = i case response.OtherError: // don't cache these @@ -467,24 +525,43 @@ func (w *ResponseWriter) Write(buf []byte) (int, error) { type verifyStaleResponseWriter struct { *ResponseWriter refreshed bool // set to true if the last WriteMsg wrote to ResponseWriter, false otherwise. + response *dns.Msg + item *item } // newVerifyStaleResponseWriter returns a ResponseWriter to be used when verifying stale cache // entries. It only forwards matching, complete responses that successfully refresh the data -// according to RFC8767, section 4 (response is NoError or NXDomain), and ignores other responses. +// according to RFC8767, section 4 (response is NoError or NXDomain). With prefer_positive, only +// a usable positive answer is forwarded; other matching responses are cached without being sent +// to the client. func newVerifyStaleResponseWriter(w *ResponseWriter) *verifyStaleResponseWriter { return &verifyStaleResponseWriter{ - w, - false, + ResponseWriter: w, } } // WriteMsg implements the dns.ResponseWriter interface. func (w *verifyStaleResponseWriter) WriteMsg(res *dns.Msg) error { w.refreshed = false + w.response = nil + w.item = nil if res == nil || res.Truncated || !w.state.Match(res) { return nil } + if w.preferPositive { + if usableAnswer(res, w.now().UTC()) { + w.refreshed = true + err := w.ResponseWriter.WriteMsg(res) + w.response = w.lastResponse + w.item = w.lastItem + return err + } + prefetch := w.prefetch + w.prefetch = true + err := w.ResponseWriter.WriteMsg(res) + w.prefetch = prefetch + return err + } responseType, _ := response.Typify(res, w.now().UTC()) if responseType == response.OtherError || responseType == response.Meta || responseType == response.Update { return nil @@ -493,7 +570,10 @@ func (w *verifyStaleResponseWriter) WriteMsg(res *dns.Msg) error { return nil } w.refreshed = true - return w.ResponseWriter.WriteMsg(res) // stores to the cache and sends to the client + err := w.ResponseWriter.WriteMsg(res) // stores to the cache and sends to the client + w.response = w.lastResponse + w.item = w.lastItem + return err } const ( diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go index 7be23a46b..f4bfa8160 100644 --- a/plugin/cache/cache_test.go +++ b/plugin/cache/cache_test.go @@ -980,6 +980,9 @@ func TestServeFromStaleCacheFetchVerifyTimeoutFastUpstream(t *testing.T) { if got := rec.Msg.Answer[0].Header().Ttl; got != 200 { t.Errorf("expected fresh TTL=200, got %d", got) } + if !rec.Msg.Authoritative { + t.Error("expected cached fresh response to preserve authoritative cache reply shaping") + } } func TestNegativeStaleMaskingPositiveCache(t *testing.T) { @@ -1451,6 +1454,431 @@ func TestServfailDoesNotShadowPositiveCache(t *testing.T) { } } +func TestPreferPositiveCachePolicy(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + now := time.Now() + c.now = func() time.Time { return now } + + req := new(dns.Msg) + req.SetQuestion("example.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: req} + k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled) + + positive := new(dns.Msg) + positive.SetReply(req) + positive.Answer = []dns.RR{test.A("example.org. 60 IN A 192.0.2.1")} + c.pcache.Add(k, newItem(positive, now.Add(-2*time.Minute), time.Minute)) + + negative := new(dns.Msg) + negative.SetRcode(req, dns.RcodeNameError) + negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")} + c.ncache.Add(k, newItem(negative, now, 5*time.Minute)) + + if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError { + t.Fatalf("default policy should prefer ncache NXDOMAIN, got %+v", got) + } + + c.preferPositive = true + if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeSuccess { + t.Fatalf("prefer_positive should prefer eligible pcache answer, got %+v", got) + } +} + +func TestPreferPositiveRejectsNonAnswer(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.preferPositive = true + now := time.Now() + + req := new(dns.Msg) + req.SetQuestion("alias.example.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: req} + k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled) + + incomplete := new(dns.Msg) + incomplete.SetReply(req) + incomplete.Answer = []dns.RR{test.CNAME("alias.example.org. 60 IN CNAME missing.example.org.")} + c.pcache.Add(k, newItem(incomplete, now, time.Minute)) + + negative := new(dns.Msg) + negative.SetRcode(req, dns.RcodeNameError) + negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")} + c.ncache.Add(k, newItem(negative, now, 5*time.Minute)) + + if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError { + t.Fatalf("non-answer pcache item must not shadow ncache, got %+v", got) + } +} + +func TestAnswersQuestionStrictEligibility(t *testing.T) { + chAddress, err := dns.NewRR("cached.org. 60 CH A 192.0.2.20") + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + qtype uint16 + answer []dns.RR + want bool + }{ + { + name: "direct A", + qtype: dns.TypeA, + answer: []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")}, + want: true, + }, + { + name: "ANY matching owner", + qtype: dns.TypeANY, + answer: []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")}, + want: true, + }, + { + name: "ANY unrelated owner", + qtype: dns.TypeANY, + answer: []dns.RR{test.A("unrelated.org. 60 IN A 192.0.2.10")}, + want: false, + }, + { + name: "duplicate equivalent CNAME targets", + qtype: dns.TypeCNAME, + answer: []dns.RR{ + test.CNAME("cached.org. 60 IN CNAME target.org."), + test.CNAME("cached.org. 60 IN CNAME target.org."), + }, + want: true, + }, + { + name: "multiple CNAME targets", + qtype: dns.TypeCNAME, + answer: []dns.RR{ + test.CNAME("cached.org. 60 IN CNAME first.org."), + test.CNAME("cached.org. 60 IN CNAME second.org."), + }, + want: false, + }, + { + name: "wrong RR class", + qtype: dns.TypeA, + answer: []dns.RR{chAddress}, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := new(dns.Msg) + req.SetQuestion("cached.org.", tc.qtype) + res := new(dns.Msg) + res.SetReply(req) + res.Answer = tc.answer + + if got := answersQuestion(res); got != tc.want { + t.Fatalf("answersQuestion() = %t, want %t", got, tc.want) + } + }) + } +} + +func TestPreferPositiveRetainsLKGAcrossNonAnswerSuccessRefreshes(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.preferPositive = true + now := time.Now() + c.now = func() time.Time { return now } + + req := new(dns.Msg) + req.SetQuestion("cached.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: req} + writer := &ResponseWriter{Cache: c, state: state, prefetch: true} + + positive := new(dns.Msg) + positive.SetReply(req) + positive.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")} + if err := writer.WriteMsg(positive); err != nil { + t.Fatal(err) + } + + now = now.Add(2 * time.Minute) + refreshes := []*dns.Msg{ + func() *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + return m + }(), + func() *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + m.Ns = []dns.RR{test.NS("example.org. 60 IN NS ns.example.org.")} + return m + }(), + func() *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + m.Ns = []dns.RR{test.NS("example.org. 60 IN NS ns.example.org.")} + m.Extra = []dns.RR{test.A("ns.example.org. 60 IN A 192.0.2.53")} + return m + }(), + func() *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + m.Extra = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.54")} + return m + }(), + } + + for i, refresh := range refreshes { + if err := writer.WriteMsg(refresh); err != nil { + t.Fatal(err) + } + got := c.getIfNotStale(now, state, "test") + if got == nil || !got.answersQuestion(state) { + t.Fatalf("refresh %d lost last-known-good answer: %+v", i, got) + } + if address := got.Answer[0].(*dns.A).A.String(); address != "192.0.2.10" { + t.Fatalf("refresh %d returned %s, want 192.0.2.10", i, address) + } + } +} + +func TestPreferPositiveDoesNotServeLKGOutsideStaleWindow(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.preferPositive = true + now := time.Now() + + req := new(dns.Msg) + req.SetQuestion("cached.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: req} + k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled) + + positive := new(dns.Msg) + positive.SetReply(req) + positive.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")} + lastKnownGood := newItem(positive, now.Add(-2*time.Hour), time.Minute) + + empty := new(dns.Msg) + empty.SetReply(req) + current := newItem(empty, now, time.Minute) + current.lastKnownGood = lastKnownGood + c.pcache.Add(k, current) + + negative := new(dns.Msg) + negative.SetRcode(req, dns.RcodeNameError) + negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")} + c.ncache.Add(k, newItem(negative, now, 5*time.Minute)) + + if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError { + t.Fatalf("expected current NXDOMAIN after LKG stale window, got %+v", got) + } +} + +func TestPreferPositiveVerifyKeepsStaleOnNonAnswers(t *testing.T) { + tests := []struct { + name string + backend plugin.Handler + }{ + {name: "NXDOMAIN", backend: nxDomainBackend(300)}, + {name: "NODATA", backend: plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + m := new(dns.Msg) + m.SetReply(r) + m.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")} + return dns.RcodeSuccess, w.WriteMsg(m) + })}, + {name: "SERVFAIL", backend: servFailBackend(300)}, + {name: "NOTIMP", backend: plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + m := new(dns.Msg) + m.SetRcode(r, dns.RcodeNotImplemented) + return dns.RcodeNotImplemented, w.WriteMsg(m) + })}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.verifyStale = true + c.preferPositive = true + c.Next = ttlBackend(60) + + req := new(dns.Msg) + req.SetQuestion("cached.org.", dns.TypeA) + ctx := context.Background() + c.ServeDNS(ctx, &test.ResponseWriter{}, req) + + c.now = func() time.Time { return time.Now().Add(2 * time.Minute) } + c.Next = tc.backend + + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + ret, err := c.ServeDNS(ctx, rec, req.Copy()) + if err != nil { + t.Fatal(err) + } + if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeSuccess { + t.Fatalf("expected stale positive response, got ret=%d msg=%+v", ret, rec.Msg) + } + if got := rec.Msg.Answer[0].Header().Ttl; got != 0 { + t.Fatalf("expected stale TTL 0, got %d", got) + } + if c.ncache.Len() != 1 { + t.Fatalf("expected verified %s to be retained in ncache, got %d entries", tc.name, c.ncache.Len()) + } + }) + } +} + +func TestPreferPositiveVerifyRejectsInvalidFreshAnswers(t *testing.T) { + modes := []struct { + name string + timeout time.Duration + }{ + {name: "blocking"}, + {name: "bounded", timeout: time.Second}, + } + invalidResponses := []struct { + name string + build func(*dns.Msg) *dns.Msg + }{ + { + name: "expired RRSIG", + build: func(req *dns.Msg) *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + m.SetEdns0(4096, true) + m.Answer = []dns.RR{ + test.A("cached.org. 60 IN A 192.0.2.20"), + test.RRSIG("cached.org. 60 IN RRSIG A 8 2 60 20160521031301 20160421031301 12051 cached.org. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="), + } + return m + }, + }, + { + name: "truncated", + build: func(req *dns.Msg) *dns.Msg { + m := new(dns.Msg) + m.SetReply(req) + m.Truncated = true + m.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.20")} + return m + }, + }, + } + + for _, mode := range modes { + for _, invalid := range invalidResponses { + t.Run(mode.name+"/"+invalid.name, func(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.verifyStale = true + c.verifyStaleTimeout = mode.timeout + c.preferPositive = true + + now := time.Now().UTC() + c.now = func() time.Time { return now } + c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + m := new(dns.Msg) + m.SetReply(r) + m.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")} + return dns.RcodeSuccess, w.WriteMsg(m) + }) + + req := new(dns.Msg) + req.SetQuestion("cached.org.", dns.TypeA) + req.SetEdns0(4096, true) + if _, err := c.ServeDNS(context.Background(), &test.ResponseWriter{}, req.Copy()); err != nil { + t.Fatal(err) + } + + now = now.Add(2 * time.Minute) + c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + return dns.RcodeSuccess, w.WriteMsg(invalid.build(r)) + }) + + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + ret, err := c.ServeDNS(context.Background(), rec, req.Copy()) + if err != nil { + t.Fatal(err) + } + if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeSuccess { + t.Fatalf("expected stale positive response, got ret=%d msg=%+v", ret, rec.Msg) + } + if len(rec.Msg.Answer) == 0 { + t.Fatal("expected retained stale answer") + } + a, ok := rec.Msg.Answer[0].(*dns.A) + if !ok || a.A.String() != "192.0.2.10" { + t.Fatalf("expected retained 192.0.2.10, got %v", rec.Msg.Answer) + } + if got := a.Hdr.Ttl; got != 0 { + t.Fatalf("expected stale TTL 0, got %d", got) + } + }) + } + } +} + +func TestServeFromStaleCacheFetchVerifyTimeoutUncacheableResponse(t *testing.T) { + c := New() + c.staleUpTo = time.Hour + c.verifyStale = true + c.verifyStaleTimeout = time.Second + c.Next = ttlBackend(60) + + req := new(dns.Msg) + req.SetQuestion("cached.org.", dns.TypeA) + c.ServeDNS(context.Background(), &test.ResponseWriter{}, req) + c.now = func() time.Time { return time.Now().Add(2 * time.Minute) } + c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + m := new(dns.Msg) + m.SetRcode(r, dns.RcodeNameError) + return dns.RcodeNameError, w.WriteMsg(m) + }) + + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + ret, err := c.ServeDNS(context.Background(), rec, req.Copy()) + if err != nil { + t.Fatal(err) + } + if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeNameError { + t.Fatalf("expected direct uncacheable NXDOMAIN, got ret=%d msg=%+v", ret, rec.Msg) + } + for _, section := range [][]dns.RR{rec.Msg.Answer, rec.Msg.Ns, rec.Msg.Extra} { + for _, rr := range section { + if rr.Header().Ttl > uint32(maxTTL.Seconds()) { + t.Fatalf("unexpected wrapped TTL %d", rr.Header().Ttl) + } + } + } +} + +func TestCNAMEWithSOAStoredAsNODATA(t *testing.T) { + c := New() + req := new(dns.Msg) + req.SetQuestion("alias.example.org.", dns.TypeA) + crr := &ResponseWriter{ + Cache: c, + state: request.Request{Req: req}, + prefetch: true, + } + + res := new(dns.Msg) + res.SetReply(req) + res.Answer = []dns.RR{test.CNAME("alias.example.org. 300 IN CNAME missing.example.net.")} + res.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")} + + if err := crr.WriteMsg(res); err != nil { + t.Fatal(err) + } + if c.ncache.Len() != 1 { + t.Fatalf("expected NODATA in ncache, got %d entries", c.ncache.Len()) + } + if c.pcache.Len() != 0 { + t.Fatalf("expected no positive cache entry, got %d", c.pcache.Len()) + } +} + func TestServeFromStaleCacheFetchVerifyTimeoutMetadataIsolation(t *testing.T) { c := New() c.staleUpTo = time.Hour diff --git a/plugin/cache/handler.go b/plugin/cache/handler.go index 7d1b6e64b..a3171cb1a 100644 --- a/plugin/cache/handler.go +++ b/plugin/cache/handler.go @@ -163,10 +163,10 @@ func (c *Cache) doRefresh(ctx context.Context, state request.Request, cw dns.Res // verifyWithTimeout runs the upstream verify in a background goroutine and races it // against verifyStaleTimeout. If the verify completes within the timeout and the -// response is cacheable (NoError or NXDomain), the freshly cached entry is served -// to the client and served is true. Otherwise served is false and the caller falls -// through to serve stale; the goroutine continues to run and any successful response -// will update the cache without writing to the (now-detached) client connection. +// response is accepted by verifyStaleResponseWriter, the freshly cached entry is +// served to the client and served is true. Otherwise served is false and the caller +// falls through to serve stale; the goroutine continues to run and any cacheable +// response updates the cache without writing to the detached client connection. func (c *Cache) verifyWithTimeout(ctx context.Context, state request.Request, w dns.ResponseWriter, cw *verifyStaleResponseWriter, r *dns.Msg, do, ad bool, i *item, failureRecheck time.Duration, now func() time.Time) (served bool, code int, err error) { type result struct { code int @@ -191,17 +191,18 @@ func (c *Cache) verifyWithTimeout(ctx context.Context, state request.Request, w if !cw.refreshed { return false, 0, nil } - fresh := c.exists(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled) - if fresh == nil { - // Should not happen: refreshed=true means the upstream response was cacheable. + if cw.response == nil { return true, res.code, res.err } - now := c.now() - if c.keepttl { - now = fresh.stored + response := cw.response + if cw.item != nil { + now := c.now() + if c.keepttl { + now = cw.item.stored + } + response = cw.item.toMsg(r, now, do, ad) } - resp := fresh.toMsg(r, now, do, ad) - if err := w.WriteMsg(resp); err != nil { + if err := w.WriteMsg(response); err != nil { return true, dns.RcodeServerFailure, err } return true, dns.RcodeSuccess, nil @@ -227,6 +228,19 @@ func (c *Cache) getIfNotStale(now time.Time, state request.Request, server strin k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled) cacheRequests.WithLabelValues(server, c.zonesMetricLabel, c.viewMetricLabel).Inc() + if c.preferPositive && c.staleUpTo > 0 { + if i, ok := c.pcache.Get(k); ok { + i = i.answeringItem(state) + if i != nil { + ttl := i.ttl(now) + if ttl > 0 || -ttl < int(c.staleUpTo.Seconds()) { + cacheHits.WithLabelValues(server, Success, c.zonesMetricLabel, c.viewMetricLabel).Inc() + return i + } + } + } + } + if i, ok := c.ncache.Get(k); ok { ttl := i.ttl(now) if i.matches(state) && (ttl > 0 || (c.staleUpTo > 0 && -ttl < int(c.staleUpTo.Seconds()))) { diff --git a/plugin/cache/item.go b/plugin/cache/item.go index cf135fc38..18ec9fdc2 100644 --- a/plugin/cache/item.go +++ b/plugin/cache/item.go @@ -22,6 +22,8 @@ type item struct { Ns []dns.RR Extra []dns.RR wildcard string + answering bool // immutable result of validating that this item answers its question. + lastKnownGood *item // answering item retained when a non-answer overwrites this success-cache key. origTTL uint32 stored time.Time @@ -59,6 +61,7 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item { j++ } i.Extra = i.Extra[:j] + i.answering = answersQuestion(m) i.origTTL = uint32(d.Seconds()) // Keep the monotonic clock reading so TTL expiry is unaffected by wall @@ -119,6 +122,20 @@ func (i *item) matches(state request.Request) bool { return false } +func (i *item) answersQuestion(state request.Request) bool { + return i.answering && i.matches(state) +} + +func (i *item) answeringItem(state request.Request) *item { + if i.answersQuestion(state) { + return i + } + if i.lastKnownGood != nil && i.lastKnownGood.answersQuestion(state) { + return i.lastKnownGood + } + return nil +} + func (i *item) beginRefresh(now time.Time, failureRecheck time.Duration) bool { if failureRecheck > 0 { if retryAfter := i.retryAfter.Load(); retryAfter != nil && now.Before(*retryAfter) { diff --git a/plugin/cache/setup.go b/plugin/cache/setup.go index 1a33183f4..5763ed37c 100644 --- a/plugin/cache/setup.go +++ b/plugin/cache/setup.go @@ -63,6 +63,8 @@ func cacheParse(c *caddy.Controller) (*Cache, error) { } } origins := plugin.OriginsFromArgsOrServerBlock(args, c.ServerBlockKeys) + serveStaleConfigured := false + serveStalePolicyConfigured := false // Refinements? In an extra block. for c.NextBlock() { @@ -171,6 +173,7 @@ func cacheParse(c *caddy.Controller) (*Cache, error) { } case "serve_stale": + serveStaleConfigured = true args := c.RemainingArgs() if len(args) > 5 { return nil, c.ArgErr() @@ -236,6 +239,21 @@ func cacheParse(c *caddy.Controller) (*Cache, error) { } } } + case "serve_stale_policy": + if serveStalePolicyConfigured { + return nil, errors.New("serve_stale_policy can only be specified once") + } + serveStalePolicyConfigured = true + args := c.RemainingArgs() + if len(args) != 1 { + return nil, c.ArgErr() + } + switch strings.ToLower(args[0]) { + case "prefer_positive": + ca.preferPositive = true + default: + return nil, fmt.Errorf("invalid serve_stale_policy: %s", args[0]) + } case "servfail": args := c.RemainingArgs() if len(args) != 1 { @@ -292,6 +310,9 @@ func cacheParse(c *caddy.Controller) (*Cache, error) { return nil, c.ArgErr() } } + if serveStalePolicyConfigured && !serveStaleConfigured { + return nil, errors.New("serve_stale_policy requires serve_stale") + } ca.Zones = origins ca.zonesMetricLabel = strings.Join(origins, ",") diff --git a/plugin/cache/setup_test.go b/plugin/cache/setup_test.go index 7ac411d1f..168d0d4dc 100644 --- a/plugin/cache/setup_test.go +++ b/plugin/cache/setup_test.go @@ -226,6 +226,41 @@ func TestServeStaleFailureRecheck(t *testing.T) { } } +func TestServeStalePolicy(t *testing.T) { + tests := []struct { + input string + shouldErr bool + preferPositive bool + }{ + {"serve_stale\nserve_stale_policy prefer_positive", false, true}, + {"serve_stale_policy PREFER_POSITIVE\nserve_stale", false, true}, + {"serve_stale", false, false}, + // fails + {"serve_stale_policy prefer_positive", true, false}, + {"serve_stale\nserve_stale_policy", true, false}, + {"serve_stale\nserve_stale_policy prefer_positive extra", true, false}, + {"serve_stale\nserve_stale_policy invalid", true, false}, + {"serve_stale\nserve_stale_policy prefer_positive\nserve_stale_policy prefer_positive", true, false}, + } + for i, test := range tests { + c := caddy.NewTestController("dns", fmt.Sprintf("cache {\n%s\n}", test.input)) + ca, err := cacheParse(c) + if test.shouldErr && err == nil { + t.Errorf("Test %v: Expected error but found nil", i) + continue + } else if !test.shouldErr && err != nil { + t.Errorf("Test %v: Expected no error but found error: %v", i, err) + continue + } + if test.shouldErr { + continue + } + if ca.preferPositive != test.preferPositive { + t.Errorf("Test %v: Expected preferPositive %v but found %v", i, test.preferPositive, ca.preferPositive) + } + } +} + func TestServfail(t *testing.T) { tests := []struct { input string