Read either cache record format in the redis backend

cache-blob.go

@@ -38,16 +38,15 @@ import (
 // blob be matched, or hashed, without decoding it back into an lruKey.
 type cacheBlob []byte
 
-// blobVersion is the version of the layout above. A blob is freshly allocated
-// and so carries 0 without being written, but it is read back: a blob read
-// from the cache file is rejected unless it carries this version, so a later
-// change to the layout keeps old records out of new accessors. A writer that
-// bumps it has to start setting it explicitly.
+// blobVersion is the version of the layout above. It is written into every
+// blob and checked wherever one is read back from storage, so a later change
+// to the layout keeps old records out of accessors that no longer match them.
 //
-// Note that 1 is already taken: it is binaryFormatVersion, the Redis record
-// format, which is a different layout in the same first byte. A stored form
-// meant to be read by either backend has to start at 2.
-const blobVersion = 0
+// 0 was this layout while it lived only in memory and in the cache file, and 1
+// is binaryFormatVersion, the Redis record format from before the backends
+// shared a layout, a different shape in the same first byte. A record that
+// either backend may read therefore starts at 2.
+const blobVersion = 2
 
 const (
 	blobOffVersion   = 0
@@ -103,6 +102,7 @@ func newCacheBlobFromWire(key lruKey, meta *cacheAnswer, wire []byte) (cacheBlob
 	}
 
 	blob := make(cacheBlob, blobHdrLen+len(key.Net)+len(key.Question.Name)+len(wire))
+	blob[blobOffVersion] = blobVersion
 	if meta.PrefetchEligible {
 		blob[blobOffMetaFlags] |= blobMetaPrefetchEligible
 	}
@@ -206,3 +206,23 @@ func (b cacheBlob) key() lruKey {
 		ECSMask: b[blobOffECSMask],
 	}
 }
+
+// cacheAnswer decodes a blob back into the form the cache layer works with.
+// Used by the Redis backend, which hands its records to callers as a
+// cacheAnswer; the memory backend serves a hit straight off the blob and needs
+// none of this.
+func (b cacheBlob) cacheAnswer() (*cacheAnswer, error) {
+	if len(b) < blobHdrLen || blobHdrLen+b.netLen()+b.nameLen() > len(b) {
+		return nil, fmt.Errorf("cache record too short: %d bytes", len(b))
+	}
+	msg := new(dns.Msg)
+	if err := msg.Unpack(b.message()); err != nil {
+		return nil, fmt.Errorf("failed to unpack DNS message: %w", err)
+	}
+	return &cacheAnswer{
+		Timestamp:        nanoTime(b.timestamp()),
+		Expiry:           nanoTime(b.expiry()),
+		PrefetchEligible: b.prefetchEligible(),
+		Msg:              msg,
+	}, nil
+}

cache-file-raw.go

@@ -21,11 +21,18 @@ import (
 // at once that it is binary.
 //
 // The version belongs to the file rather than to each record: it is checked
-// once, and a file cannot hold a mix. The version byte inside a blob stays
-// reserved for a record that travels on its own, without a file around it.
+// once, and a file cannot hold a mix. The version byte inside a blob covers a
+// record that travels on its own, without a file around it, which is how the
+// Redis backend stores one.
+//
+// Version 2 carries blobVersion 2, the layout the memory and Redis backends
+// share. Version 1 files hold the same layout under blobVersion 0 and are
+// rejected here rather than record by record, so an operator who tried the raw
+// format before the two were unified gets one clear line about a cold start
+// instead of a cache that silently comes up empty.
 const (
 	rawCacheMagic     = "\x00RDC"
-	rawCacheVersion   = 1
+	rawCacheVersion   = 2
 	rawCacheHeaderLen = len(rawCacheMagic) + 1
 
 	// An upper bound on a stored record, so a corrupt length can't ask for an

cache-file-raw_test.go

@@ -210,3 +210,33 @@ func TestRawCacheFileRejectsUnknownBlobVersion(t *testing.T) {
 	require.NoError(t, dst.deserializeRaw(bytes.NewReader(damaged)))
 	require.Equal(t, 2, dst.size(), "the record with an unknown layout is skipped, the rest load")
 }
+
+// A raw file holds blobs, so its version has to move whenever blobVersion
+// does. If it doesn't, the header check passes and every record is then
+// rejected one at a time by the per-record version check, which is the silent
+// empty cache the header check exists to prevent. Nothing in the types ties
+// the two constants together, so pin both: changing either should mean coming
+// here and deciding about the other.
+func TestRawCacheFileVersionTracksBlobVersion(t *testing.T) {
+	require.Equal(t, 2, rawCacheVersion, "if this moved, blobVersion likely has to move with it")
+	require.Equal(t, 2, blobVersion, "if this moved, rawCacheVersion has to move with it")
+}
+
+// A file written before the backends shared a record layout is refused at the
+// header, so it reports a cold start rather than being accepted and then
+// losing every record to the per-record check without a word.
+func TestRawCacheFileRejectsOlderFileVersion(t *testing.T) {
+	src := newLRUCache(0)
+	fillCache(t, src, 3)
+	var buf bytes.Buffer
+	require.NoError(t, src.serializeRaw(&buf))
+
+	older := bytes.Clone(buf.Bytes())
+	older[len(rawCacheMagic)] = rawCacheVersion - 1
+
+	dst := newLRUCache(0)
+	err := dst.deserializeRaw(bytes.NewReader(older))
+	require.Error(t, err, "an older file version must be refused, not read")
+	require.Contains(t, err.Error(), "unsupported raw cache file version")
+	require.Zero(t, dst.size())
+}

cache-redis.go

@@ -34,12 +34,37 @@ type RedisBackendOptions struct {
 
 var _ CacheBackend = (*redisBackend)(nil)
 
+// The record format this backend writes. Version 2, the layout it shares with
+// the memory backend, is read but not yet written; see decodeRecord.
 const (
 	binaryFormatVersion = 1
 	headerSize          = 10
 	flagPrefetchBit     = 1 << 0
 )
 
+// decodeRecord decodes a stored record, whichever format it is in. Version 1
+// is the format above, which this backend still writes. Version 2 is the blob
+// layout shared with the memory backend, which a later release will write.
+//
+// Reading it first is deliberate. A redis cache is shared between instances,
+// so a record written in a format an instance does not know is a miss and an
+// error log line on every lookup that finds it. Shipping the reader a release
+// ahead of the writer means that by the time anything emits version 2, the
+// instances sharing the database can already read it, and a rollback lands on
+// a build that can too.
+func decodeRecord(b []byte) (*cacheAnswer, error) {
+	if len(b) == 0 {
+		return nil, errors.New("empty cache record")
+	}
+	switch b[0] {
+	case binaryFormatVersion:
+		return decodeCacheAnswer(b)
+	case blobVersion:
+		return cacheBlob(b).cacheAnswer()
+	}
+	return nil, fmt.Errorf("unsupported cache record version: %d", b[0])
+}
+
 // encodeCacheAnswer encodes a cacheAnswer into a compact binary format:
 // - byte 0: version (1)
 // - byte 1: flags (bit0: prefetchEligible)
@@ -197,7 +222,7 @@ func (b *redisBackend) Lookup(q *dns.Msg) (*dns.Msg, bool, bool) {
 		return nil, false, false
 	}
 
-	a, err := decodeCacheAnswer(valueBytes)
+	a, err := decodeRecord(valueBytes)
 	if err != nil {
 		Log.Error("failed to decode cache record from redis", "error", err)
 		return nil, false, false

cache-redis_test.go

@@ -299,3 +299,65 @@ func BenchmarkKeyFromQuery(b *testing.B) {
 		_ = backend.keyFromQuery(q)
 	}
 }
+
+// The reader ships a release ahead of the writer, so the version 2 path has to
+// work before anything in this backend produces one. Build the record the way
+// the memory backend does and read it back through the dispatch.
+func TestRedisReadsVersion2Records(t *testing.T) {
+	msg := new(dns.Msg)
+	msg.SetQuestion("shared.example.", dns.TypeA)
+	msg.Response = true
+	rr, err := dns.NewRR("shared.example. 300 IN A 192.0.2.9")
+	require.NoError(t, err)
+	msg.Answer = append(msg.Answer, rr)
+
+	now := time.Now()
+	for _, eligible := range []bool{false, true} {
+		// The empty key is what the writer will store: the redis key already
+		// encodes the question, so the record does not repeat it.
+		encoded, err := newCacheBlob(lruKey{}, &cacheAnswer{
+			Timestamp:        now,
+			Expiry:           now.Add(5 * time.Minute),
+			PrefetchEligible: eligible,
+			Msg:              msg,
+		})
+		require.NoError(t, err)
+		require.Equal(t, byte(blobVersion), encoded.version())
+
+		decoded, err := decodeRecord(encoded)
+		require.NoError(t, err)
+		require.Equal(t, eligible, decoded.PrefetchEligible)
+		require.Equal(t, now.UnixNano(), decoded.Timestamp.UnixNano())
+		require.Equal(t, "shared.example.", decoded.Msg.Question[0].Name)
+	}
+}
+
+// Records this backend writes today keep being read, which is the direction
+// that matters while a version 1 writer is still running somewhere.
+func TestRedisReadsVersion1Records(t *testing.T) {
+	msg := new(dns.Msg)
+	msg.SetQuestion("legacy.example.", dns.TypeA)
+	msg.Response = true
+
+	encoded, err := encodeCacheAnswer(nil, &cacheAnswer{
+		Timestamp:        time.Unix(1234567890, 0),
+		PrefetchEligible: true,
+		Msg:              msg,
+	})
+	require.NoError(t, err)
+	require.Equal(t, byte(binaryFormatVersion), encoded[0])
+
+	decoded, err := decodeRecord(encoded)
+	require.NoError(t, err)
+	require.True(t, decoded.PrefetchEligible)
+	require.Equal(t, "legacy.example.", decoded.Msg.Question[0].Name)
+}
+
+func TestDecodeRecordRejectsUnknownVersion(t *testing.T) {
+	_, err := decodeRecord(nil)
+	require.Error(t, err, "an empty record has no version to dispatch on")
+
+	_, err = decodeRecord([]byte{0x99, 0, 0, 0, 0, 0, 0, 0, 0, 0})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "unsupported cache record version")
+}

lru-cache-blob_test.go

@@ -275,12 +275,16 @@ func TestBlobKeyRegionDependsOnlyOnTheKey(t *testing.T) {
 	require.NotEqual(t, first.keyRegion(), third.keyRegion())
 }
 
-// The version byte is reserved, not yet used: blobs are freshly allocated so
-// it reads as 0 without being written. Pinning it means a later layout change
-// bumps it deliberately, and notices it has to start setting it.
-func TestBlobVersionIsReserved(t *testing.T) {
+// The version byte is written, not left at whatever the allocation carried.
+// A blob now travels on its own into Redis and into the cache file, where the
+// byte is what tells a reader the layout matches its accessors, so a blob that
+// went out carrying an implicit 0 would be indistinguishable from a record
+// written before the backends shared a layout.
+func TestBlobVersionIsWritten(t *testing.T) {
 	blob, err := newCacheBlob(blobTestKey(), &cacheAnswer{Msg: blobTestAnswer(t)})
 	require.NoError(t, err)
 	require.Equal(t, byte(blobVersion), blob.version())
-	require.Zero(t, blobVersion, "still reserved; bumping it means writing it too")
+	require.NotZero(t, blobVersion, "must be written, so it cannot be the zero value")
+	require.NotEqual(t, byte(binaryFormatVersion), blob.version(),
+		"must not collide with the pre-unification Redis record format")
 }