structs: fix cache keys

So that requests are cached properly, and the cache does not return the wrong data for a
request.

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
+		Name              string
-		EnterpriseMeta EnterpriseMeta
+		EnterpriseMeta    EnterpriseMeta
-		Upstreams      []string    `hash:"set"`
+		Upstreams         []string    `hash:"set"`
-		UpstreamIDs    []ServiceID `hash:"set"`
+		UpstreamIDs       []ServiceID `hash:"set"`
+		MeshGatewayConfig MeshGatewayConfig
+		ProxyMode         ProxyMode
+		Filter            string
 	}{
-		Name:           r.Name,
+		Name:              r.Name,
-		EnterpriseMeta: r.EnterpriseMeta,
+		EnterpriseMeta:    r.EnterpriseMeta,
-		Upstreams:      r.Upstreams,
+		Upstreams:         r.Upstreams,
-		UpstreamIDs:    r.UpstreamIDs,
+		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

@@ -2222,18 +2222,9 @@ func TestConfigEntryQuery_CacheInfoKey(t *testing.T) {
 }
 
 func TestServiceConfigRequest_CacheInfoKey(t *testing.T) {
-	ignoredFields := []string{
+	assertCacheInfoKeyIsComplete(t, &ServiceConfigRequest{})
-		// TODO: should QueryOptions.Filter be included in the key?
-		"QueryOptions",
-		// TODO: should this be included in the key?
-		"MeshGateway",
-		// TODO: should this be included in the key?
-		"Mode",
-	}
-	assertCacheInfoKeyIsComplete(t, &ServiceConfigRequest{}, ignoredFields...)
 }
 
 func TestDiscoveryChainRequest_CacheInfoKey(t *testing.T) {
-	// TODO: should QueryOptions.Filter be included in the key?
+	assertCacheInfoKeyIsComplete(t, &DiscoveryChainRequest{})
-	assertCacheInfoKeyIsComplete(t, &DiscoveryChainRequest{}, "QueryOptions")
 }

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
+	v, err := hashstructure.Hash(struct {
-	// has been configured so things like ordering of entries has no
+		IntentionID string
-	// effect (via struct tags).
+		Match       *IntentionQueryMatch
-	v, err := hashstructure.Hash(q.Match, nil)
+		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

@@ -456,7 +456,5 @@ func TestIntention_String(t *testing.T) {
 }
 
 func TestIntentionQueryRequest_CacheInfoKey(t *testing.T) {
-	// TODO: should these fields be included in the key?
+	assertCacheInfoKeyIsComplete(t, &IntentionQueryRequest{})
-	ignored := []string{"IntentionID", "Check", "Exact", "QueryOptions"}
-	assertCacheInfoKeyIsComplete(t, &IntentionQueryRequest{}, ignored...)
 }

agent/structs/structs.go

@@ -642,6 +642,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

@@ -1597,8 +1597,7 @@ func TestNodeSpecificRequest_CacheInfoKey(t *testing.T) {
 }
 
 func TestServiceSpecificRequest_CacheInfoKey(t *testing.T) {
-	// TODO: should ServiceKind filed be included in the key?
+	assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{})
-	assertCacheInfoKeyIsComplete(t, &ServiceSpecificRequest{}, "ServiceKind")
 }
 
 func TestServiceDumpRequest_CacheInfoKey(t *testing.T) {
@@ -1613,7 +1612,7 @@ 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 from  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.
@@ -1654,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)
 		}
 	}
 }