Changed secretmap.Values() to return an array of Secret objects rathe…

…r than SecretTime.

None of the callers of Values() needed the other elements of SecretTime, just the Secret
object inside, and in the most common case it meant the Secret inside had to be copied
out to another array after retrieval, creating unncessary code/work.

cache.go

@@ -135,8 +135,6 @@ func (c *Cache) Secret(name string) (*Secret, bool) {
 //  * If backend returns fast: update cache, return.
 //  * If timeout backend deadline: return cache entries, background update cache.
 //  * If timeout max wait: return cache version.
-// TODO: why do we prefer the backend to the cache? is cache intended only to be local backup?
-// TODO: could we track list freshness and prefer a fresh cache list to avoid pulling from backend?
 func (c *Cache) SecretList() []Secret {
 	backendDeadline := time.After(c.timeouts.BackendDeadline)
 	backendDone := c.backendSecretList()
@@ -179,14 +177,7 @@ func (c *Cache) cacheSecret(name string) *SecretTime {
 
 // cacheSecretList retrieves a secret listing from the cache.
 func (c *Cache) cacheSecretList() []Secret {
-	values := c.secretMap.Values()
-	secrets := make([]Secret, len(values))
-	// TODO: is it necessary to gather the list in secretmap.Values() and then copy it one by one to another
-	// TODO: list with no change/processing/etc? seems inefficient, can it be improved?
-	for i, v := range values {
-		secrets[i] = v.Secret
-	}
-	return secrets
+	return c.secretMap.Values()
 }
 
 // backendSecret retrieves a secret from the backend and updates the cache.
@@ -234,13 +225,7 @@ func (c *Cache) backendSecretList() chan []Secret {
 		}
 		c.secretMap.Replace(newMap)
 
-		// TODO: copy-pasta from cacheSecretList(), should refactor.
-		values := c.secretMap.Values()
-		secrets = make([]Secret, len(values))
-		for i, v := range values {
-			secrets[i] = v.Secret
-		}
-		secretsc <- secrets
+		secretsc <- c.cacheSecretList()
 		close(secretsc)
 	}()
 	return secretsc

main.go

@@ -19,14 +19,14 @@ import (
 	"log"
 	"os"
 	"os/signal"
-	"square/up/service/metrics"
 	"strings"
 	"time"
 
 	"gopkg.in/alecthomas/kingpin.v2"
 
 	"github.com/hanwen/go-fuse/fuse"
 	"github.com/hanwen/go-fuse/fuse/nodefs"
+	"github.com/rcrowley/go-metrics"
 	"github.com/square/go-sq-metrics"
 	klog "github.com/square/keywhiz-fs/log"
 	"golang.org/x/sys/unix"

secretmap.go

@@ -131,18 +131,18 @@ func (m *SecretMap) Replace(m2 *SecretMap) {
 }
 
 // Values returns a slice of stored secrets in no particular order.
-func (m *SecretMap) Values() []SecretTime {
+func (m *SecretMap) Values() []Secret {
 	m.lock.Lock()
 	defer m.lock.Unlock()
 
-	values := make([]SecretTime, len(m.m))
+	values := make([]Secret, len(m.m))
 	i := 0
 	now := m.getNow()
 	for key, value := range m.m {
 		if isExpired(value, now) {
 			delete(m.m, key)
 		} else {
-			values[i] = value
+			values[i] = value.Secret
 			i++
 		}
 	}

secretmap_test.go

@@ -41,7 +41,7 @@ func TestSecretMapOperations(t *testing.T) {
 
 	values := secretMap.Values()
 	assert.Len(values, 1)
-	assert.Equal(*s, values[0].Secret)
+	assert.Equal(*s, values[0])
 
 	lookup, ok = secretMap.Get("foo")
 	assert.True(ok)
@@ -59,7 +59,7 @@ func TestSecretMapOperations(t *testing.T) {
 	// Secret should still exist for a short amount of time
 	values = secretMap.Values()
 	assert.Len(values, 1)
-	assert.Equal(*s, values[0].Secret)
+	assert.Equal(*s, values[0])
 	// Advance current time by more than an hour, secret should now be gone
 	fake_now = fake_now.Add(2 * time.Hour)
 	values = secretMap.Values()