From 690e5c7aff8347c3885096f3962a0633d9129607 Mon Sep 17 00:00:00 2001 From: Cody Rose Date: Fri, 4 Apr 2025 15:26:31 -0400 Subject: [PATCH] Make verification cache detector type-aware (#4009) This PR modifies the verification cache to be detector type-aware by adding the detector type to the cache keys. There is also a slight change to the key generation logic to prevent it from reslicing the passed-in Raw value. (We haven't seen any trouble from this so far, but I think it's good to disarm the footgun while I'm here.) When adding tests, I also realized that the existing tests were inconsistently named, so I cleaned them up. This resolves #3984. --- pkg/verificationcache/verification_cache.go | 10 +++- .../verification_cache_test.go | 56 ++++++++++++++++--- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/pkg/verificationcache/verification_cache.go b/pkg/verificationcache/verification_cache.go index 8f2c9f609..97ea3f93a 100644 --- a/pkg/verificationcache/verification_cache.go +++ b/pkg/verificationcache/verification_cache.go @@ -1,6 +1,8 @@ package verificationcache import ( + "bytes" + "encoding/binary" "sync" "time" @@ -135,5 +137,11 @@ func (v *VerificationCache) getResultCacheKey(result detectors.Result) ([]byte, v.hashMu.Lock() defer v.hashMu.Unlock() - return v.hasher.Hash(append(result.Raw, result.RawV2...)) + keyBytes := bytes.Join([][]byte{result.Raw, result.RawV2}, nil) + keyBytes, err := binary.Append(keyBytes, binary.BigEndian, result.DetectorType) + if err != nil { + return nil, err + } + + return v.hasher.Hash(keyBytes) } diff --git a/pkg/verificationcache/verification_cache_test.go b/pkg/verificationcache/verification_cache_test.go index 69476844d..c40a30216 100644 --- a/pkg/verificationcache/verification_cache_test.go +++ b/pkg/verificationcache/verification_cache_test.go @@ -23,7 +23,7 @@ func (t *testDetector) FromData(_ context.Context, verify bool, _ []byte) ([]det t.fromDataCallCount = t.fromDataCallCount + 1 var results []detectors.Result for _, r := range t.results { - copy := detectors.Result{Redacted: r.Redacted, Raw: r.Raw, RawV2: r.RawV2} + copy := detectors.Result{Redacted: r.Redacted, Raw: r.Raw, RawV2: r.RawV2, DetectorType: r.DetectorType} if verify { copy.CopyVerificationInfo(&r) } @@ -49,7 +49,7 @@ func getResultCacheKey(t *testing.T, cache *VerificationCache, result detectors. return string(key) } -func TestVerificationCacheFromData_Passthrough(t *testing.T) { +func TestVerificationCache_FromData_Passthrough(t *testing.T) { detector := testDetector{results: []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, }} @@ -69,7 +69,7 @@ func TestVerificationCacheFromData_Passthrough(t *testing.T) { }) } -func TestVerificationCacheFromData_VerifyFalseForceCacheUpdateFalse(t *testing.T) { +func TestVerificationCache_FromData_VerifyFalseForceCacheUpdateFalse(t *testing.T) { detector := testDetector{results: []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, }} @@ -96,7 +96,7 @@ func TestVerificationCacheFromData_VerifyFalseForceCacheUpdateFalse(t *testing.T assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load()) } -func TestFromDataCached_VerifyFalseForceCacheUpdateTrue(t *testing.T) { +func TestVerificationCache_FromData_VerifyFalseForceCacheUpdateTrue(t *testing.T) { detector := testDetector{results: []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, {Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false}, @@ -129,7 +129,7 @@ func TestFromDataCached_VerifyFalseForceCacheUpdateTrue(t *testing.T) { assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load()) } -func TestFromDataCached_VerifyTrueForceCacheUpdateFalseAllCacheHits(t *testing.T) { +func TestVerificationCache_FromData_VerifyTrueForceCacheUpdateFalseAllCacheHits(t *testing.T) { remoteResults := []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, {Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false}, @@ -169,7 +169,7 @@ func TestFromDataCached_VerifyTrueForceCacheUpdateFalseAllCacheHits(t *testing.T assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load()) } -func TestFromDataCached_VerifyTrueForceCacheUpdateFalseCacheMiss(t *testing.T) { +func TestVerificationCache_FromData_VerifyTrueForceCacheUpdateFalseCacheMiss(t *testing.T) { detector := testDetector{results: []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, {Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false}, @@ -205,7 +205,7 @@ func TestFromDataCached_VerifyTrueForceCacheUpdateFalseCacheMiss(t *testing.T) { assert.Equal(t, int32(1), metrics.ResultCacheHitsWasted.Load()) } -func TestFromDataCached_VerifyTrueForceCacheUpdateTrue(t *testing.T) { +func TestVerificationCache_FromData_VerifyTrueForceCacheUpdateTrue(t *testing.T) { detector := testDetector{results: []detectors.Result{ {Redacted: "hello", Raw: []byte("hello"), RawV2: []byte("helloV2"), Verified: true}, {Redacted: "world", Raw: []byte("world"), RawV2: []byte("worldV2"), Verified: false}, @@ -238,3 +238,45 @@ func TestFromDataCached_VerifyTrueForceCacheUpdateTrue(t *testing.T) { assert.Equal(t, int32(0), metrics.ResultCacheMisses.Load()) assert.Equal(t, int32(0), metrics.ResultCacheHitsWasted.Load()) } + +func TestVerificationCache_FromData_SameRawDifferentType_CacheMiss(t *testing.T) { + detector1 := testDetector{results: []detectors.Result{ + {Redacted: "hello", Raw: []byte("hello"), Verified: true, DetectorType: -1}, + }} + detector2 := testDetector{results: []detectors.Result{ + {Redacted: "hello", Raw: []byte("hello"), Verified: true, DetectorType: -2}, + }} + cache := New(simple.NewCache[detectors.Result](), nil) + _, err := cache.FromData(logContext.Background(), &detector1, true, false, nil) + require.NoError(t, err) + + res, err := cache.FromData(logContext.Background(), &detector2, true, false, nil) + + if assert.NoError(t, err) { + if assert.Len(t, res, 1) { + assert.Equal(t, detectorspb.DetectorType(-2), res[0].DetectorType) + } + } + assert.Len(t, cache.resultCache.Values(), 2) +} + +func TestVerificationCache_FromData_SameRawV2DifferentType_CacheMiss(t *testing.T) { + detector1 := testDetector{results: []detectors.Result{ + {Redacted: "hello", RawV2: []byte("there"), Verified: true, DetectorType: -1}, + }} + detector2 := testDetector{results: []detectors.Result{ + {Redacted: "hello", RawV2: []byte("there"), Verified: true, DetectorType: -2}, + }} + cache := New(simple.NewCache[detectors.Result](), nil) + _, err := cache.FromData(logContext.Background(), &detector1, true, false, nil) + require.NoError(t, err) + + res, err := cache.FromData(logContext.Background(), &detector2, true, false, nil) + + if assert.NoError(t, err) { + if assert.Len(t, res, 1) { + assert.Equal(t, detectorspb.DetectorType(-2), res[0].DetectorType) + } + } + assert.Len(t, cache.resultCache.Values(), 2) +}