Make cache not actually cache values under core/ (#2439)

physical/cache.go

@@ -96,7 +96,7 @@ func (c *Cache) Put(entry *Entry) error {
 	defer lock.Unlock()
 
 	err := c.backend.Put(entry)
-	if err == nil {
+	if err == nil && !strings.HasPrefix(entry.Key, "core/") {
 		c.lru.Add(entry.Key, entry)
 	}
 	return err
@@ -107,6 +107,14 @@ func (c *Cache) Get(key string) (*Entry, error) {
 	lock.RLock()
 	defer lock.RUnlock()
 
+	// We do NOT cache negative results for keys in the 'core/' prefix
+	// otherwise we risk certain race conditions upstream. The primary issue is
+	// with the HA mode, we could potentially negatively cache the leader entry
+	// and cause leader discovery to fail.
+	if strings.HasPrefix(key, "core/") {
+		return c.backend.Get(key)
+	}
+
 	// Check the LRU first
 	if raw, ok := c.lru.Get(key); ok {
 		if raw == nil {
@@ -122,15 +130,12 @@ func (c *Cache) Get(key string) (*Entry, error) {
 		return nil, err
 	}
 
-	// Cache the result. We do NOT cache negative results
+	// Cache the result
-	// for keys in the 'core/' prefix otherwise we risk certain
+	if ent != nil {
-	// race conditions upstream. The primary issue is with the HA mode,
-	// we could potentially negatively cache the leader entry and cause
-	// leader discovery to fail.
-	if ent != nil || !strings.HasPrefix(key, "core/") {
 		c.lru.Add(key, ent)
 	}
-	return ent, err
+
+	return ent, nil
 }
 
 func (c *Cache) Delete(key string) error {
@@ -139,7 +144,7 @@ func (c *Cache) Delete(key string) error {
 	defer lock.Unlock()
 
 	err := c.backend.Delete(key)
-	if err == nil {
+	if err == nil && !strings.HasPrefix(key, "core/") {
 		c.lru.Remove(key)
 	}
 	return err

physical/cache_test.go

@@ -55,3 +55,89 @@ func TestCache_Purge(t *testing.T) {
 		t.Fatalf("should not have key")
 	}
 }
+
+func TestCache_IgnoreCore(t *testing.T) {
+	logger := logformat.NewVaultLogger(log.LevelTrace)
+
+	inm := NewInmem(logger)
+	cache := NewCache(inm, 0, logger)
+
+	var ent *Entry
+	var err error
+
+	// First try normal handling
+	ent = &Entry{
+		Key:   "foo",
+		Value: []byte("bar"),
+	}
+	if err := cache.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent = &Entry{
+		Key:   "foo",
+		Value: []byte("foobar"),
+	}
+	if err := inm.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent, err = cache.Get("foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(ent.Value) != "bar" {
+		t.Fatal("expected cached value")
+	}
+
+	// Now try core path
+	ent = &Entry{
+		Key:   "core/foo",
+		Value: []byte("bar"),
+	}
+	if err := cache.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent = &Entry{
+		Key:   "core/foo",
+		Value: []byte("foobar"),
+	}
+	if err := inm.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent, err = cache.Get("core/foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(ent.Value) != "foobar" {
+		t.Fatal("expected cached value")
+	}
+
+	// Now make sure looked-up values aren't added
+	ent = &Entry{
+		Key:   "core/zip",
+		Value: []byte("zap"),
+	}
+	if err := inm.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent, err = cache.Get("core/zip")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(ent.Value) != "zap" {
+		t.Fatal("expected non-cached value")
+	}
+	ent = &Entry{
+		Key:   "core/zip",
+		Value: []byte("zipzap"),
+	}
+	if err := inm.Put(ent); err != nil {
+		t.Fatal(err)
+	}
+	ent, err = cache.Get("core/zip")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(ent.Value) != "zipzap" {
+		t.Fatal("expected non-cached value")
+	}
+}