From 917a9e63d5066219d5162d9ebe53b3db66985fa8 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 11 Apr 2018 10:18:24 +0100
Subject: [PATCH] agent: check cache hit count to verify CA root caching,
 background update

---
 agent/agent_endpoint_test.go | 59 +++++++++++++++++++++++++++++++-----
 agent/cache/cache.go         | 26 ++++++++++++++++
 2 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go
index 32cb6ab98f..2e583ec4f0 100644
--- a/agent/agent_endpoint_test.go
+++ b/agent/agent_endpoint_test.go
@@ -2121,32 +2121,77 @@ func TestAgentConnectCARoots_empty(t *testing.T) {
 func TestAgentConnectCARoots_list(t *testing.T) {
 	t.Parallel()
 
-	assert := assert.New(t)
+	require := require.New(t)
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
 
+	// Grab the initial cache hit count
+	cacheHits := a.cache.Hits()
+
 	// Set some CAs
 	var reply interface{}
 	ca1 := connect.TestCA(t, nil)
 	ca1.Active = false
 	ca2 := connect.TestCA(t, nil)
-	assert.Nil(a.RPC("Test.ConnectCASetRoots",
+	require.Nil(a.RPC("Test.ConnectCASetRoots",
 		[]*structs.CARoot{ca1, ca2}, &reply))
 
 	// List
 	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/roots", nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.AgentConnectCARoots(resp, req)
-	assert.Nil(err)
+	require.Nil(err)
 
 	value := obj.(structs.IndexedCARoots)
-	assert.Equal(value.ActiveRootID, ca2.ID)
-	assert.Len(value.Roots, 2)
+	require.Equal(value.ActiveRootID, ca2.ID)
+	require.Len(value.Roots, 2)
 
 	// We should never have the secret information
 	for _, r := range value.Roots {
-		assert.Equal("", r.SigningCert)
-		assert.Equal("", r.SigningKey)
+		require.Equal("", r.SigningCert)
+		require.Equal("", r.SigningKey)
+	}
+
+	// That should've been a cache miss, so not hit change
+	require.Equal(cacheHits, a.cache.Hits())
+
+	// Test caching
+	{
+		// List it again
+		obj2, err := a.srv.AgentConnectCARoots(httptest.NewRecorder(), req)
+		require.Nil(err)
+		require.Equal(obj, obj2)
+
+		// Should cache hit this time and not make request
+		require.Equal(cacheHits+1, a.cache.Hits())
+		cacheHits++
+	}
+
+	// Test that caching is updated in the background
+	{
+		// Set some new CAs
+		var reply interface{}
+		ca := connect.TestCA(t, nil)
+		require.Nil(a.RPC("Test.ConnectCASetRoots",
+			[]*structs.CARoot{ca}, &reply))
+
+		// Sleep a bit to wait for the cache to update
+		time.Sleep(100 * time.Millisecond)
+
+		// List it again
+		obj, err := a.srv.AgentConnectCARoots(httptest.NewRecorder(), req)
+		require.Nil(err)
+		require.Equal(obj, obj)
+
+		value := obj.(structs.IndexedCARoots)
+		require.Equal(value.ActiveRootID, ca.ID)
+		require.Len(value.Roots, 1)
+
+		// Should be a cache hit! The data should've updated in the cache
+		// in the background so this should've been fetched directly from
+		// the cache.
+		require.Equal(cacheHits+1, a.cache.Hits())
+		cacheHits++
 	}
 }
 
diff --git a/agent/cache/cache.go b/agent/cache/cache.go
index c512476d56..a57ab83435 100644
--- a/agent/cache/cache.go
+++ b/agent/cache/cache.go
@@ -15,6 +15,7 @@ package cache
 import (
 	"fmt"
 	"sync"
+	"sync/atomic"
 	"time"
 )
 
@@ -22,6 +23,11 @@ import (
 
 // Cache is a agent-local cache of Consul data.
 type Cache struct {
+	// Keeps track of the cache hits and misses in total. This is used by
+	// tests currently to verify cache behavior and is not meant for general
+	// analytics; for that, go-metrics emitted values are better.
+	hits, misses uint64
+
 	// types stores the list of data types that the cache knows how to service.
 	// These can be dynamically registered with RegisterType.
 	typesLock sync.RWMutex
@@ -127,6 +133,9 @@ func (c *Cache) Get(t string, r Request) (interface{}, error) {
 	// Get the actual key for our entry
 	key := c.entryKey(&info)
 
+	// First time through
+	first := true
+
 RETRY_GET:
 	// Get the current value
 	c.entriesLock.RLock()
@@ -139,10 +148,22 @@ RETRY_GET:
 	// we have.
 	if ok && entry.Valid {
 		if info.MinIndex == 0 || info.MinIndex < entry.Index {
+			if first {
+				atomic.AddUint64(&c.hits, 1)
+			}
+
 			return entry.Value, nil
 		}
 	}
 
+	if first {
+		// Record the miss if its our first time through
+		atomic.AddUint64(&c.misses, 1)
+	}
+
+	// No longer our first time through
+	first = false
+
 	// At this point, we know we either don't have a value at all or the
 	// value we have is too old. We need to wait for new data.
 	waiter, err := c.fetch(t, key, r)
@@ -263,3 +284,8 @@ func (c *Cache) refresh(opts *RegisterOptions, t string, key string, r Request)
 	// Trigger
 	c.fetch(t, key, r)
 }
+
+// Returns the number of cache hits. Safe to call concurrently.
+func (c *Cache) Hits() uint64 {
+	return atomic.LoadUint64(&c.hits)
+}