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

@@ -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

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

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{})
+}

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

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{})
+}

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...)
+}

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

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)
 		}
 	}
 }