From cab82ca5a0af50f8d2002765f5d404ec3b485ad2 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Jun 2021 11:24:02 -0400 Subject: [PATCH] Merge pull request #9556 from hashicorp/dnephin/add-more-cache-key-completness-tests structs: Add more cache key completeness tests --- agent/structs/config_entry.go | 22 +++++---- agent/structs/config_entry_discoverychain.go | 5 +- agent/structs/config_entry_test.go | 12 +++++ agent/structs/intention.go | 29 +++++++----- agent/structs/intention_test.go | 7 ++- agent/structs/prepared_query_test.go | 6 +++ agent/structs/structs.go | 1 + agent/structs/structs_test.go | 50 +++++++++++--------- 8 files changed, 86 insertions(+), 46 deletions(-) diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 44142ac8dc..f6a2b25f4a 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -639,15 +639,21 @@ func (r *ServiceConfigRequest) CacheInfo() cache.RequestInfo { // the slice would affect cache keys if we ever persist between agent restarts // and change it. v, err := hashstructure.Hash(struct { - Name string - EnterpriseMeta EnterpriseMeta - Upstreams []string `hash:"set"` - UpstreamIDs []ServiceID `hash:"set"` + Name string + EnterpriseMeta EnterpriseMeta + Upstreams []string `hash:"set"` + UpstreamIDs []ServiceID `hash:"set"` + MeshGatewayConfig MeshGatewayConfig + ProxyMode ProxyMode + Filter string }{ - Name: r.Name, - EnterpriseMeta: r.EnterpriseMeta, - Upstreams: r.Upstreams, - UpstreamIDs: r.UpstreamIDs, + Name: r.Name, + EnterpriseMeta: r.EnterpriseMeta, + Upstreams: r.Upstreams, + UpstreamIDs: r.UpstreamIDs, + ProxyMode: r.Mode, + MeshGatewayConfig: r.MeshGateway, + Filter: r.QueryOptions.Filter, }, nil) if err == nil { // If there is an error, we don't set the key. A blank key forces diff --git a/agent/structs/config_entry_discoverychain.go b/agent/structs/config_entry_discoverychain.go index b9cee72772..a0d2bed0e8 100644 --- a/agent/structs/config_entry_discoverychain.go +++ b/agent/structs/config_entry_discoverychain.go @@ -11,10 +11,11 @@ import ( "strings" "time" + "github.com/mitchellh/hashstructure" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/lib" - "github.com/mitchellh/hashstructure" ) const ( @@ -1378,6 +1379,7 @@ func (r *DiscoveryChainRequest) CacheInfo() cache.RequestInfo { OverrideMeshGateway MeshGatewayConfig OverrideProtocol string OverrideConnectTimeout time.Duration + Filter string }{ Name: r.Name, EvaluateInDatacenter: r.EvaluateInDatacenter, @@ -1385,6 +1387,7 @@ func (r *DiscoveryChainRequest) CacheInfo() cache.RequestInfo { OverrideMeshGateway: r.OverrideMeshGateway, OverrideProtocol: r.OverrideProtocol, OverrideConnectTimeout: r.OverrideConnectTimeout, + Filter: r.QueryOptions.Filter, }, nil) if err == nil { // If there is an error, we don't set the key. A blank key forces diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index e37cf49cd4..0526c43acb 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -2216,3 +2216,15 @@ func requireContainsLower(t *testing.T, haystack, needle string) { func intPointer(i int) *int { return &i } + +func TestConfigEntryQuery_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &ConfigEntryQuery{}) +} + +func TestServiceConfigRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &ServiceConfigRequest{}) +} + +func TestDiscoveryChainRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &DiscoveryChainRequest{}) +} diff --git a/agent/structs/intention.go b/agent/structs/intention.go index f5ef8ebb38..2549f8d5ec 100644 --- a/agent/structs/intention.go +++ b/agent/structs/intention.go @@ -10,11 +10,12 @@ import ( "strings" "time" + "github.com/hashicorp/go-multierror" + "github.com/mitchellh/hashstructure" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/lib" - "github.com/hashicorp/go-multierror" - "github.com/mitchellh/hashstructure" "golang.org/x/crypto/blake2b" ) @@ -596,13 +597,6 @@ func (q *IntentionQueryRequest) RequestDatacenter() string { // CacheInfo implements cache.Request func (q *IntentionQueryRequest) CacheInfo() cache.RequestInfo { - // We only support caching Match queries, so if Match isn't set, - // then return an empty info object which will cause a pass-through - // (and likely fail). - if q.Match == nil { - return cache.RequestInfo{} - } - info := cache.RequestInfo{ Token: q.Token, Datacenter: q.Datacenter, @@ -610,10 +604,19 @@ func (q *IntentionQueryRequest) CacheInfo() cache.RequestInfo { Timeout: q.MaxQueryTime, } - // Calculate the cache key via just hashing the Match struct. This - // has been configured so things like ordering of entries has no - // effect (via struct tags). - v, err := hashstructure.Hash(q.Match, nil) + v, err := hashstructure.Hash(struct { + IntentionID string + Match *IntentionQueryMatch + Check *IntentionQueryCheck + Exact *IntentionQueryExact + Filter string + }{ + IntentionID: q.IntentionID, + Check: q.Check, + Match: q.Match, + Exact: q.Exact, + Filter: q.QueryOptions.Filter, + }, nil) if err == nil { // If there is an error, we don't set the key. A blank key forces // no cache for this request so the request is forwarded directly diff --git a/agent/structs/intention_test.go b/agent/structs/intention_test.go index be63060184..7e94db3526 100644 --- a/agent/structs/intention_test.go +++ b/agent/structs/intention_test.go @@ -5,9 +5,10 @@ import ( "strings" "testing" - "github.com/hashicorp/consul/acl" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/acl" ) func TestIntention_ACLs(t *testing.T) { @@ -453,3 +454,7 @@ func TestIntention_String(t *testing.T) { }) } } + +func TestIntentionQueryRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &IntentionQueryRequest{}) +} diff --git a/agent/structs/prepared_query_test.go b/agent/structs/prepared_query_test.go index 6a629874b4..d77ba3578b 100644 --- a/agent/structs/prepared_query_test.go +++ b/agent/structs/prepared_query_test.go @@ -27,3 +27,9 @@ func TestStructs_PreparedQuery_GetACLPrefix(t *testing.T) { t.Fatalf("bad: ok=%v prefix=%#v", ok, prefix) } } + +func TestPreparedQueryExecuteRequest_CacheInfoKey(t *testing.T) { + // TODO: should these fields be included in the key? + ignored := []string{"Agent", "QueryOptions"} + assertCacheInfoKeyIsComplete(t, &PreparedQueryExecuteRequest{}, ignored...) +} diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 9f0aefa263..3855e06ff9 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -617,6 +617,7 @@ func (r *ServiceSpecificRequest) CacheInfo() cache.RequestInfo { r.Filter, r.EnterpriseMeta, r.Ingress, + r.ServiceKind, }, nil) if err == nil { // If there is an error, we don't set the key. A blank key forces diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index b527322c5e..8c36368031 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -1588,30 +1588,21 @@ func TestStructs_validateMetaPair(t *testing.T) { } } -func TestDCSpecificRequestCacheInfoKey(t *testing.T) { - assertCacheInfoKeyIsComplete(t, &DCSpecificRequest{}, nil) +func TestDCSpecificRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &DCSpecificRequest{}) } -func TestNodeSpecificRequestCacheInfoKey(t *testing.T) { - assertCacheInfoKeyIsComplete(t, &NodeSpecificRequest{}, nil) +func TestNodeSpecificRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &NodeSpecificRequest{}) } -func TestServiceSpecificRequestCacheInfoKey(t *testing.T) { - ignoredFields := map[string]bool{ - // TODO: should this filed be included? - "ServiceKind": true, - } - - assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{}, ignoredFields) +func TestServiceSpecificRequest_CacheInfoKey(t *testing.T) { + assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{}) } -func TestServiceDumpRequestCacheInfoKey(t *testing.T) { - ignoredFields := map[string]bool{ - // ServiceKind is only included when UseServiceKind=true - "ServiceKind": true, - } - - assertCacheInfoKeyIsComplete(t, &ServiceDumpRequest{}, ignoredFields) +func TestServiceDumpRequest_CacheInfoKey(t *testing.T) { + // ServiceKind is only included when UseServiceKind=true + assertCacheInfoKeyIsComplete(t, &ServiceDumpRequest{}, "ServiceKind") } // cacheInfoIgnoredFields are fields that can be ignored in all cache.Request types @@ -1621,14 +1612,26 @@ func TestServiceDumpRequestCacheInfoKey(t *testing.T) { var cacheInfoIgnoredFields = map[string]bool{ // Datacenter is part of the cache key added by the cache itself. "Datacenter": true, - // QuerySource is always the same for every request a single agent, so it + // QuerySource is always the same for every request from a single agent, so it // is excluded from the key. "Source": true, // EnterpriseMeta is an empty struct, so can not be included. enterpriseMetaField: true, } -func assertCacheInfoKeyIsComplete(t *testing.T, request cache.Request, ignoredFields map[string]bool) { +// assertCacheInfoKeyIsComplete is an assertion to verify that all fields on a request +// struct are considered as part of the cache key. It is used to prevent regressions +// when new fields are added to the struct. If a field is not included in the cache +// key it can lead to API requests or DNS requests returning the wrong value +// because a request matches the wrong entry in the agent/cache.Cache. +func assertCacheInfoKeyIsComplete(t *testing.T, request cache.Request, ignoredFields ...string) { + t.Helper() + + ignored := make(map[string]bool, len(ignoredFields)) + for _, f := range ignoredFields { + ignored[f] = true + } + fuzzer := fuzz.NewWithSeed(time.Now().UnixNano()) fuzzer.Funcs(randQueryOptions) fuzzer.Fuzz(request) @@ -1640,7 +1643,7 @@ func assertCacheInfoKeyIsComplete(t *testing.T, request cache.Request, ignoredFi fieldName := requestValue.Type().Field(i).Name originalValue := field.Interface() - if cacheInfoIgnoredFields[fieldName] || ignoredFields[fieldName] { + if cacheInfoIgnoredFields[fieldName] || ignored[fieldName] { continue } @@ -1650,10 +1653,11 @@ func assertCacheInfoKeyIsComplete(t *testing.T, request cache.Request, ignoredFi key := request.CacheInfo().Key if originalKey == key { - t.Fatalf("expected field %v to be represented in the CacheInfo.Key, %v change to %v", + t.Fatalf("expected field %v to be represented in the CacheInfo.Key, %v change to %v (key: %v)", fieldName, originalValue, - field.Interface()) + field.Interface(), + key) } } }