Skip to content

Commit a2e760a

Browse files
LostAttractorLostAttractor
committed
fix(dns): serve the real TTL instead of zeroing it, reverting the P2-5 direction
The audit treated the comment above the A/AAAA rewrite ("set TTL to 0 to prevent downstream caching while we manage it") as the contract and made every delivery path agree with it: the cache-hit packing path stopped re-stamping records whose stored TTL was zero. That was the wrong direction. Returning the real TTL is the intended design - the comment describes an older implementation and was never updated - and the hit path, which already stamped the remaining lifetime, was the part that matched the intent, while the first response carried the leftover zero. Answers now carry a real TTL on every path: the upstream value on the first response and the remaining lifetime on a cache hit, so a client no longer sees a different answer depending on whether the entry happened to be cached. The A/AAAA rewrite in NormalizeAndCacheDnsResp_ is gone; the entry lifetime is still computed from the retained records before it, and freshness continues to be tracked by the entry deadline and the stale window rather than by asking resolvers not to cache. Tests follow the behaviour they describe: the materialization cases assert a real TTL in [1,60] on both miss and hit, the shared record policy asserts that a record stored with TTL 0 is stamped with the remaining lifetime, the EDNS refresh case asserts the remaining 30s (it packs the state 30s before the deadline), and the corpus fixtures expect a real TTL range.
1 parent 60bdd67 commit a2e760a

5 files changed

Lines changed: 77 additions & 73 deletions

File tree

‎control/dns_cache.go‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -290,35 +290,30 @@ func setRecordTTL(rr dnsmessage.RR, ttl uint32) {
290290

291291
func setSectionTTL(rrs []dnsmessage.RR, ttl uint32, scratch []uint32) {
292292
for i, rr := range rrs {
293-
hdr := rr.Header()
294-
scratch[i] = hdr.Ttl
295-
// A stored zero TTL is the downstream "do not cache this" signal dae
296-
// sets on its own A/AAAA answers; the TTL rewrite must not resurrect
297-
// it (see clampWireRecordTtls for the served-stale counterpart).
298-
if hdr.Ttl != 0 {
299-
setRecordTTL(rr, ttl)
300-
}
293+
scratch[i] = rr.Header().Ttl
294+
setRecordTTL(rr, ttl)
301295
}
302296
}
303297

304-
// copySectionWithTTL deep-copies a record section and re-stamps the TTL of
305-
// every record whose stored TTL is non-zero. This is the single place where the
306-
// "materialize a stored section onto the wire at a given remaining TTL" policy
307-
// lives, so the pre-packed path and the in-place fallback cannot drift apart.
308-
// Records that are already zero-TTL (dae-managed A/AAAA answers) stay zero: the
309-
// zero is the signal that the downstream resolver must not cache the answer
310-
// because dae manages its lifetime itself. The EDNS OPT pseudo-record is never
311-
// treated as a TTL (see setRecordTTL).
298+
// copySectionWithTTL deep-copies a record section and stamps the remaining
299+
// lifetime onto every record. This is the single place where the "materialize a
300+
// stored section onto the wire at a given remaining TTL" policy lives, so the
301+
// pre-packed path and the in-place fallback cannot drift apart.
302+
//
303+
// Answers carry a real TTL: the first answer carries the TTL the upstream
304+
// returned, a cache hit the remaining lifetime of the entry. dae tracks
305+
// freshness of its own address answers through the entry deadline and the stale
306+
// window rather than by telling resolvers not to cache, so nothing here may
307+
// rewrite a lifetime to zero. The EDNS OPT pseudo-record is never treated as a
308+
// TTL (see setRecordTTL).
312309
func copySectionWithTTL(rrs []dnsmessage.RR, ttl uint32) []dnsmessage.RR {
313310
if rrs == nil {
314311
return nil
315312
}
316313
copied := make([]dnsmessage.RR, len(rrs))
317314
for i, rr := range rrs {
318315
record := dnsmessage.Copy(rr)
319-
if record.Header().Ttl != 0 {
320-
setRecordTTL(record, ttl)
321-
}
316+
setRecordTTL(record, ttl)
322317
copied[i] = record
323318
}
324319
return copied

‎control/dns_cache_edns_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,10 @@ func TestDNSCachePreservesEDNSMetadata(t *testing.T) {
6969
wire := cache.GetPackedResponseWithApproximateTTL(phase0NamedUpstreamScopeQName, dnsmessage.TypeA, cache.Deadline.Add(-30*time.Second))
7070
var got dnsmessage.Msg
7171
require.NoError(t, got.Unpack(wire))
72-
// dae zeroes the TTL of its own A/AAAA answers so downstream
73-
// resolvers do not cache them; the TTL refresh must preserve
74-
// that zero (P2-5) while still ageing ordinary records.
75-
require.EqualValues(t, 0, got.Answer[0].Header().Ttl, "dae-managed address answers keep their zero TTL")
72+
// The answer carries the real remaining lifetime: this pack
73+
// asks for the state 30s before the deadline, so the answer
74+
// leaves with 30 and ordinary records are aged the same way.
75+
require.EqualValues(t, 30, got.Answer[0].Header().Ttl, "an address answer leaves with its remaining lifetime, not a zero marker")
7676
require.EqualValues(t, 30, got.Extra[1].Header().Ttl, "ordinary additional records still receive the remaining TTL")
7777
require.Equal(t, opt, got.IsEdns0())
7878
require.Equal(t, opt, cache.Extra[0], "refresh must not mutate shared stored records")

‎control/dns_controller_cache.go‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1138,28 +1138,27 @@ func (c *DnsController) NormalizeAndCacheDnsResp_(msg *dnsmessage.Msg, responseC
11381138
}
11391139

11401140
// Positive response. Entry lifetime is the minimum over all retained real
1141-
// records (answers, authority and additional sections), collected before
1142-
// the A/AAAA downstream-zero rewrite below. Fixed-domain operator
1143-
// overrides still apply downstream in the deadline functions.
1141+
// records (answers, authority and additional sections). Fixed-domain
1142+
// operator overrides still apply downstream in the deadline functions.
11441143
//
1145-
// A zero minimum is intentionally still cached with a now-deadline: the
1146-
// A/AAAA downstream-zero rewrite above makes zero the *normal* stored TTL
1147-
// for dae-managed address answers, whose freshness dae tracks through the
1148-
// entry deadline and serves through the stale window. Treating every
1149-
// zero-TTL answer as uncacheable would disable that self-management.
1144+
// A zero minimum is still cached with a now-deadline: an upstream that
1145+
// answers with TTL 0 asks for exactly that, and dae keeps its own freshness
1146+
// bookkeeping through the entry deadline and the stale window instead of
1147+
// treating the record as uncacheable.
11501148
ttl := minRealRecordTtl(msg)
11511149
// Clamp TTL to 1 year max to prevent integer overflow when casting to int
11521150
// on 32-bit platforms.
11531151
if ttl > dnsMaxCacheableTtl {
11541152
ttl = dnsMaxCacheableTtl
11551153
}
11561154

1157-
// For A/AAAA records, we set TTL to 0 to prevent downstream caching while we manage it.
1158-
if q.Qtype == dnsmessage.TypeA || q.Qtype == dnsmessage.TypeAAAA {
1159-
for i := range msg.Answer {
1160-
msg.Answer[i].Header().Ttl = 0
1161-
}
1162-
}
1155+
// Answers are forwarded with their real TTL, both on the first response and
1156+
// on a cache hit (where it is the remaining lifetime). dae used to rewrite
1157+
// A/AAAA TTLs to zero to keep resolvers from caching, but that made the
1158+
// answer a client saw depend on whether the entry happened to be cached:
1159+
// the first response carried 0 and every later one the remaining lifetime.
1160+
// Freshness of dae-managed address answers is tracked by the entry deadline
1161+
// and the stale window, not by suppressing downstream caching.
11631162

11641163
// Update DnsCache.
11651164
return c.updateDnsCache(msg, responseCacheKey, ttl, &q)

‎control/dns_corpus_fixtures_test.go‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,12 @@ func udpCacheMissFixture() DnsCorpusFixture {
6161
AnswerCount: 1,
6262
AnswerIPv4: "1.1.1.1", // matches defaultUdpRequest realDst
6363
HasAnswerTTL: true,
64-
// dae zeroes the TTL of its own A/AAAA answers and serves
65-
// them with that zero (P2-5); the entry lifetime stays in
66-
// the cache deadline, not in the wire.
67-
AnswerTTLMin: 0,
68-
AnswerTTLMax: 0,
64+
// The delivered answer carries the real TTL: the upstream
65+
// value on the first response, the remaining lifetime on a
66+
// cache hit. dae tracks freshness in the entry deadline and
67+
// the stale window, not by zeroing the wire.
68+
AnswerTTLMin: 1,
69+
AnswerTTLMax: 60,
6970
WireHex: "1001818000010001000000000a63616368652d6d697373047465737400000100010a63616368652d6d6973730474657374000001000100000000000401010101",
7071
},
7172
PostAssert: func(t *testing.T, _ *DnsController, _ *dnsmessage.Msg) {
@@ -575,11 +576,12 @@ func tcpUdpFallbackFixture() DnsCorpusFixture {
575576
AnswerCount: 1,
576577
AnswerIPv4: "198.51.100.53",
577578
HasAnswerTTL: true,
578-
// dae zeroes the TTL of its own A/AAAA answers and serves
579-
// them with that zero (P2-5); the entry lifetime stays in
580-
// the cache deadline, not in the wire.
581-
AnswerTTLMin: 0,
582-
AnswerTTLMax: 0,
579+
// The delivered answer carries the real TTL: the upstream
580+
// value on the first response, the remaining lifetime on a
581+
// cache hit. dae tracks freshness in the entry deadline and
582+
// the stale window, not by zeroing the wire.
583+
AnswerTTLMin: 1,
584+
AnswerTTLMax: 60,
583585
WireHex: "1005818000010001000000000c7463702d66616c6c6261636b047465737400000100010c7463702d66616c6c6261636b04746573740000010001000000000004c6336435",
584586
},
585587
PostAssert: func(t *testing.T, _ *DnsController, _ *dnsmessage.Msg) {
@@ -668,11 +670,12 @@ func tcpUdpBlackholeFallbackFixture() DnsCorpusFixture {
668670
AnswerCount: 1,
669671
AnswerIPv4: "198.51.100.53",
670672
HasAnswerTTL: true,
671-
// dae zeroes the TTL of its own A/AAAA answers and serves
672-
// them with that zero (P2-5); the entry lifetime stays in
673-
// the cache deadline, not in the wire.
674-
AnswerTTLMin: 0,
675-
AnswerTTLMax: 0,
673+
// The delivered answer carries the real TTL: the upstream
674+
// value on the first response, the remaining lifetime on a
675+
// cache hit. dae tracks freshness in the entry deadline and
676+
// the stale window, not by zeroing the wire.
677+
AnswerTTLMin: 1,
678+
AnswerTTLMax: 60,
676679
WireHex: "1006818000010001000000000c7463702d66616c6c6261636b047465737400000100010c7463702d66616c6c6261636b04746573740000010001000000000004c6336435",
677680
},
678681
},

‎control/dns_wire_materialization_test.go‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,18 @@ import (
1919
)
2020

2121
// These tests pin the single "wire materialization" policy shared by the
22-
// pre-packed path and the in-place fallback: section TTLs are re-stamped only
23-
// for records whose stored TTL is non-zero, and the question section echoes the
22+
// pre-packed path and the in-place fallback: section TTLs are stamped with the
23+
// remaining lifetime on every delivery path, and the question section echoes the
2424
// requester's own spelling.
2525

26-
// TestServedAddressAnswersKeepZeroTtlOnMissAndHit is the P2-5 regression: dae
27-
// zeroes the TTL of its own A/AAAA answers so downstream resolvers do not cache
28-
// them, and the packing paths must not resurrect the entry lifetime. Before the
29-
// fix the served TTL was rewritten from the cache deadline (a positive value) on
30-
// both the miss and the hit delivery path.
31-
func TestServedAddressAnswersKeepZeroTtlOnMissAndHit(t *testing.T) {
26+
// TestServedAddressAnswersCarryTheRealTtl is the regression for the TTL policy:
27+
// an answer carries a real TTL on both the miss and the hit path - the upstream
28+
// TTL on the first response and the remaining lifetime on a cache hit. dae used
29+
// to rewrite A/AAAA TTLs to zero so that downstream resolvers would not cache
30+
// them, which made the answer a client saw depend on whether the entry happened
31+
// to be cached (0 first, a positive lifetime afterwards). Freshness is tracked
32+
// by the entry deadline and the stale window instead.
33+
func TestServedAddressAnswersCarryTheRealTtl(t *testing.T) {
3234
for _, qtype := range []uint16{dnsmessage.TypeA, dnsmessage.TypeAAAA} {
3335
t.Run(dnsmessage.TypeToString[qtype], func(t *testing.T) {
3436
installCorpusDnsForwarderFactory(t, func(_ *componentdns.Upstream, _ dialArgument, _ *logrus.Logger) (DnsForwarder, error) {
@@ -56,26 +58,31 @@ func TestServedAddressAnswersKeepZeroTtlOnMissAndHit(t *testing.T) {
5658
if msg == nil || len(msg.Answer) == 0 {
5759
t.Fatalf("%s: no answer captured", phase)
5860
}
59-
if ttl := msg.Answer[0].Header().Ttl; ttl != 0 {
60-
t.Fatalf("%s: answer TTL = %d, want 0 (the zero is the downstream "+
61-
"\"do not cache\" signal and the packing path must not restore the entry lifetime)", phase, ttl)
61+
// The upstream stub answers with TTL 60, so a real TTL is in
62+
// [1, 60]: the first response carries the upstream value and a
63+
// cache hit the remaining lifetime. Zero is only correct when
64+
// the upstream itself said zero.
65+
ttl := msg.Answer[0].Header().Ttl
66+
if ttl == 0 || ttl > 60 {
67+
t.Fatalf("%s: answer TTL = %d, want a real TTL in [1,60] (zero only when the upstream answered zero)", phase, ttl)
6268
}
6369
// The same value must be on the wire that was actually sent.
6470
var wire dnsmessage.Msg
6571
if err := wire.Unpack(writer.Wire()); err != nil {
6672
t.Fatalf("%s: unpack delivered wire: %v", phase, err)
6773
}
68-
if ttl := wire.Answer[0].Header().Ttl; ttl != 0 {
69-
t.Fatalf("%s: delivered wire answer TTL = %d, want 0", phase, ttl)
74+
if wireTtl := wire.Answer[0].Header().Ttl; wireTtl != ttl {
75+
t.Fatalf("%s: delivered wire answer TTL = %d, in-memory %d", phase, wireTtl, ttl)
7076
}
7177
}
7278
})
7379
}
7480
}
7581

76-
// TestCopySectionWithTTLPolicy pins the shared record-level TTL policy: zero
77-
// stays zero (dae-managed address answers), non-zero is re-stamped, and the
78-
// EDNS OPT pseudo-record is never treated as a lifetime.
82+
// TestCopySectionWithTTLPolicy pins the shared record-level TTL policy: every
83+
// real record is stamped with the requested remaining lifetime (including one
84+
// whose stored TTL was zero, so the first response and a cache hit agree), and
85+
// the EDNS OPT pseudo-record is never treated as a lifetime.
7986
func TestCopySectionWithTTLPolicy(t *testing.T) {
8087
section := []dnsmessage.RR{
8188
&dnsmessage.A{Hdr: dnsmessage.RR_Header{Name: "zero.test.", Rrtype: dnsmessage.TypeA, Class: dnsmessage.ClassINET, Ttl: 0}, A: net.ParseIP("192.0.2.1").To4()},
@@ -87,11 +94,11 @@ func TestCopySectionWithTTLPolicy(t *testing.T) {
8794
if len(copied) != len(section) {
8895
t.Fatalf("copied section length = %d, want %d", len(copied), len(section))
8996
}
90-
if got := copied[0].Header().Ttl; got != 0 {
91-
t.Fatalf("zero-TTL record = %d, want 0", got)
97+
if got := copied[0].Header().Ttl; got != 60 {
98+
t.Fatalf("record stored with TTL 0 = %d, want the requested 60 (the delivered answer carries the real remaining lifetime, not a zero marker)", got)
9299
}
93100
if got := copied[1].Header().Ttl; got != 60 {
94-
t.Fatalf("non-zero record = %d, want the requested 60", got)
101+
t.Fatalf("record stored with TTL 300 = %d, want the requested 60", got)
95102
}
96103
if got := copied[2].Header().Ttl; got != 0x00008000 {
97104
t.Fatalf("OPT flags field = %#x, want it untouched (it is not a TTL)", got)
@@ -153,8 +160,8 @@ func TestPrepackAndInPlaceShareRecordTtlPolicy(t *testing.T) {
153160
t.Fatalf("answer[%d] TTL differs between the paths: in-place %d, prepacked %d", i, got, want)
154161
}
155162
}
156-
if got := prepacked.Answer[0].Header().Ttl; got != 0 {
157-
t.Fatalf("prepacked zero-TTL answer = %d, want 0", got)
163+
if got := prepacked.Answer[0].Header().Ttl; got != 30 {
164+
t.Fatalf("prepacked answer stored with TTL 0 = %d, want the remaining 30: the delivered answer carries a real TTL, not a zero marker", got)
158165
}
159166
if got := prepacked.Answer[1].Header().Ttl; got != 30 {
160167
t.Fatalf("prepacked CNAME TTL = %d, want the remaining 30", got)

0 commit comments

Comments
 (0)