From 46dfdb611f3cf0c5054014bd0833c761108c0bf1 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Fri, 21 May 2021 13:31:18 -0400
Subject: [PATCH] structs: improve the interface of
 assertCacheInfoKeyIsComplete

---
 agent/structs/config_entry_test.go | 19 ++++++++-------
 agent/structs/structs_test.go      | 37 ++++++++++++++++--------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go
index 83906542b3..3d46ca5d4b 100644
--- a/agent/structs/config_entry_test.go
+++ b/agent/structs/config_entry_test.go
@@ -2218,23 +2218,22 @@ func intPointer(i int) *int {
 }
 
 func TestConfigEntryQuery_CacheInfoKey(t *testing.T) {
-	assertCacheInfoKeyIsComplete(t, &ConfigEntryQuery{}, nil)
+	assertCacheInfoKeyIsComplete(t, &ConfigEntryQuery{})
 }
 
 func TestServiceConfigRequest_CacheInfoKey(t *testing.T) {
-	ignoredFields := map[string]bool{
-		"QueryOptions": true,
+	ignoredFields := []string{
+		// TODO: should QueryOptions.Filter be included in the key?
+		"QueryOptions",
 		// TODO: should this be included in the key?
-		"MeshGateway": true,
+		"MeshGateway",
 		// TODO: should this be included in the key?
-		"Mode": true,
+		"Mode",
 	}
-	assertCacheInfoKeyIsComplete(t, &ServiceConfigRequest{}, ignoredFields)
+	assertCacheInfoKeyIsComplete(t, &ServiceConfigRequest{}, ignoredFields...)
 }
 
 func TestDiscoveryChainRequest_CacheInfoKey(t *testing.T) {
-	ignoredFields := map[string]bool{
-		"QueryOptions": true,
-	}
-	assertCacheInfoKeyIsComplete(t, &DiscoveryChainRequest{}, ignoredFields)
+	// TODO: should QueryOptions.Filter be included in the key?
+	assertCacheInfoKeyIsComplete(t, &DiscoveryChainRequest{}, "QueryOptions")
 }
diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go
index 728cd63bdc..ff7f39fc88 100644
--- a/agent/structs/structs_test.go
+++ b/agent/structs/structs_test.go
@@ -1589,29 +1589,21 @@ func TestStructs_validateMetaPair(t *testing.T) {
 }
 
 func TestDCSpecificRequest_CacheInfoKey(t *testing.T) {
-	assertCacheInfoKeyIsComplete(t, &DCSpecificRequest{}, nil)
+	assertCacheInfoKeyIsComplete(t, &DCSpecificRequest{})
 }
 
 func TestNodeSpecificRequest_CacheInfoKey(t *testing.T) {
-	assertCacheInfoKeyIsComplete(t, &NodeSpecificRequest{}, nil)
+	assertCacheInfoKeyIsComplete(t, &NodeSpecificRequest{})
 }
 
 func TestServiceSpecificRequest_CacheInfoKey(t *testing.T) {
-	ignoredFields := map[string]bool{
-		// TODO: should this filed be included?
-		"ServiceKind": true,
-	}
-
-	assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{}, ignoredFields)
+	// TODO: should ServiceKind filed be included in the key?
+	assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{}, "ServiceKind")
 }
 
 func TestServiceDumpRequest_CacheInfoKey(t *testing.T) {
-	ignoredFields := map[string]bool{
-		// ServiceKind is only included when UseServiceKind=true
-		"ServiceKind": true,
-	}
-
-	assertCacheInfoKeyIsComplete(t, &ServiceDumpRequest{}, ignoredFields)
+	// ServiceKind is only included when UseServiceKind=true
+	assertCacheInfoKeyIsComplete(t, &ServiceDumpRequest{}, "ServiceKind")
 }
 
 // cacheInfoIgnoredFields are fields that can be ignored in all cache.Request types
@@ -1621,15 +1613,26 @@ func TestServiceDumpRequest_CacheInfoKey(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)
@@ -1641,7 +1644,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
 		}