From 790465fc3bc331a5ae46898cd690c83ccd2cacaa Mon Sep 17 00:00:00 2001 From: Brian Kassouf Date: Thu, 10 May 2018 10:29:26 -0700 Subject: [PATCH] physical/cache: Add a list of prefixes to not cache (#4515) * physical/cache: Add a list of prefixes to not cache * Rename the pathmanager * Move cache back to the beggining of postUnseal * Fix comment --- helper/pathmanager/pathmanager.go | 136 +++++++++++++++++++++++ helper/pathmanager/pathmanager_test.go | 143 +++++++++++++++++++++++++ physical/cache.go | 64 ++++++++--- vault/core.go | 1 + 4 files changed, 327 insertions(+), 17 deletions(-) create mode 100644 helper/pathmanager/pathmanager.go create mode 100644 helper/pathmanager/pathmanager_test.go diff --git a/helper/pathmanager/pathmanager.go b/helper/pathmanager/pathmanager.go new file mode 100644 index 0000000000..e0e39445b2 --- /dev/null +++ b/helper/pathmanager/pathmanager.go @@ -0,0 +1,136 @@ +package pathmanager + +import ( + "strings" + "sync" + + iradix "github.com/hashicorp/go-immutable-radix" +) + +// PathManager is a prefix searchable index of paths +type PathManager struct { + l sync.RWMutex + paths *iradix.Tree +} + +// New creates a new path manager +func New() *PathManager { + return &PathManager{ + paths: iradix.New(), + } +} + +// AddPaths adds path to the paths list +func (p *PathManager) AddPaths(paths []string) { + p.l.Lock() + defer p.l.Unlock() + + txn := p.paths.Txn() + for _, prefix := range paths { + if len(prefix) == 0 { + continue + } + + var exception bool + if strings.HasPrefix(prefix, "!") { + prefix = strings.TrimPrefix(prefix, "!") + exception = true + } + + // We trim any trailing *, but we don't touch whether it is a trailing + // slash or not since we want to be able to ignore prefixes that fully + // specify a file + txn.Insert([]byte(strings.TrimSuffix(prefix, "*")), exception) + } + p.paths = txn.Commit() +} + +// RemovePaths removes paths from the paths list +func (p *PathManager) RemovePaths(paths []string) { + p.l.Lock() + defer p.l.Unlock() + + txn := p.paths.Txn() + for _, prefix := range paths { + if len(prefix) == 0 { + continue + } + + // Exceptions aren't stored with the leading ! so strip it + if strings.HasPrefix(prefix, "!") { + prefix = strings.TrimPrefix(prefix, "!") + } + + // We trim any trailing *, but we don't touch whether it is a trailing + // slash or not since we want to be able to ignore prefixes that fully + // specify a file + txn.Delete([]byte(strings.TrimSuffix(prefix, "*"))) + } + p.paths = txn.Commit() +} + +// RemovePathPrefix removes all paths with the given prefix +func (p *PathManager) RemovePathPrefix(prefix string) { + p.l.Lock() + defer p.l.Unlock() + + // We trim any trailing *, but we don't touch whether it is a trailing + // slash or not since we want to be able to ignore prefixes that fully + // specify a file + p.paths, _ = p.paths.DeletePrefix([]byte(strings.TrimSuffix(prefix, "*"))) +} + +// Len returns the number of paths +func (p *PathManager) Len() int { + return p.paths.Len() +} + +// Paths returns the path list +func (p *PathManager) Paths() []string { + p.l.RLock() + defer p.l.RUnlock() + + paths := make([]string, 0, p.paths.Len()) + walkFn := func(k []byte, v interface{}) bool { + paths = append(paths, string(k)) + return false + } + p.paths.Root().Walk(walkFn) + return paths +} + +// HasPath returns if the prefix for the path exists regardless if it is a path +// (ending with /) or a prefix for a leaf node +func (p *PathManager) HasPath(path string) bool { + p.l.RLock() + defer p.l.RUnlock() + + if _, exceptionRaw, ok := p.paths.Root().LongestPrefix([]byte(path)); ok { + var exception bool + if exceptionRaw != nil { + exception = exceptionRaw.(bool) + } + return !exception + } + return false +} + +// HasExactPath returns if the longest match is an exact match for the +// full path +func (p *PathManager) HasExactPath(path string) bool { + p.l.RLock() + defer p.l.RUnlock() + + if val, exceptionRaw, ok := p.paths.Root().LongestPrefix([]byte(path)); ok { + var exception bool + if exceptionRaw != nil { + exception = exceptionRaw.(bool) + } + + strVal := string(val) + if strings.HasSuffix(strVal, "/") || strVal == path { + return !exception + } + } + return false +} diff --git a/helper/pathmanager/pathmanager_test.go b/helper/pathmanager/pathmanager_test.go new file mode 100644 index 0000000000..650c7de6ba --- /dev/null +++ b/helper/pathmanager/pathmanager_test.go @@ -0,0 +1,143 @@ +package pathmanager + +import ( + "reflect" + "testing" +) + +func TestPathManager(t *testing.T) { + m := New() + + if m.Len() != 0 { + t.Fatalf("bad: path length expect 0, got %d", len(m.Paths())) + } + + paths := []string{ + "path1/", + "path2/", + "path3/", + } + + for _, path := range paths { + if m.HasPath(path) { + t.Fatalf("path should not exist in filtered paths '%s'", path) + } + } + + // add paths + m.AddPaths(paths) + if m.Len() != 3 { + t.Fatalf("bad: path length expect 3, got %d", len(m.Paths())) + } + if !reflect.DeepEqual(paths, m.Paths()) { + t.Fatalf("mismatch in paths") + } + for _, path := range paths { + if !m.HasPath(path) { + t.Fatalf("path should exist in filtered paths '%s'", path) + } + } + + // remove the paths + m.RemovePaths(paths) + + for _, path := range paths { + if m.HasPath(path) { + t.Fatalf("path should not exist in filtered paths '%s'", path) + } + } +} + +func TestPathManager_RemovePrefix(t *testing.T) { + m := New() + + if m.Len() != 0 { + t.Fatalf("bad: path length expect 0, got %d", len(m.Paths())) + } + + paths := []string{ + "path1/", + "path2/", + "path3/", + } + + for _, path := range paths { + if m.HasPath(path) { + t.Fatalf("path should not exist in filtered paths '%s'", path) + } + } + + // add paths + m.AddPaths(paths) + if m.Len() != 3 { + t.Fatalf("bad: path length expect 3, got %d", len(m.Paths())) + } + if !reflect.DeepEqual(paths, m.Paths()) { + t.Fatalf("mismatch in paths") + } + for _, path := range paths { + if !m.HasPath(path) { + t.Fatalf("path should exist in filtered paths '%s'", path) + } + } + + // remove the paths + m.RemovePathPrefix("path") + + if m.Len() != 0 { + t.Fatalf("bad: path length expect 0, got %d", len(m.Paths())) + } + + for _, path := range paths { + if m.HasPath(path) { + t.Fatalf("path should not exist in filtered paths '%s'", path) + } + } +} + +func TestPathManager_HasExactPath(t *testing.T) { + m := New() + paths := []string{ + "path1/key1", + "path1/key1/subkey1", + "path1/key1/subkey2", + "path1/key1/subkey3", + "path2/*", + "path3/", + "!path4/key1", + "!path5/*", + } + m.AddPaths(paths) + if m.Len() != len(paths) { + t.Fatalf("path count does not match: expected %d, got %d", len(paths), m.Len()) + } + + type tCase struct { + key string + expect bool + } + + tcases := []tCase{ + tCase{"path1/key1", true}, + tCase{"path2/key1", true}, + tCase{"path3/key1", true}, + tCase{"path1/key1/subkey1", true}, + tCase{"path1/key1/subkey99", false}, + tCase{"path2/key1/subkey1", true}, + tCase{"path1/key1/subkey1/subkey1", false}, + tCase{"nonexistentpath/key1", false}, + tCase{"path4/key1", false}, + tCase{"path5/key1/subkey1", false}, + } + + for _, tc := range tcases { + if match := m.HasExactPath(tc.key); match != tc.expect { + t.Fatalf("incorrect match: key %q", tc.key) + } + } + + m.RemovePaths(paths) + if len(m.Paths()) != 0 { + t.Fatalf("removing all paths did not clear manager: paths %v", m.Paths()) + } +} diff --git a/physical/cache.go b/physical/cache.go index 11e40fb9aa..4e2e5a5d6a 100644 --- a/physical/cache.go +++ b/physical/cache.go @@ -7,6 +7,7 @@ import ( log "github.com/hashicorp/go-hclog" "github.com/hashicorp/golang-lru" "github.com/hashicorp/vault/helper/locksutil" + "github.com/hashicorp/vault/helper/pathmanager" ) const ( @@ -14,16 +15,26 @@ const ( DefaultCacheSize = 128 * 1024 ) +// These paths don't need to be cached by the LRU cache. This should +// particularly help memory pressure when unsealing. +var cacheExceptionsPaths = []string{ + "wal/logs/", + "index/pages/", + "index-dr/pages/", + "sys/expire/", +} + // Cache is used to wrap an underlying physical backend // and provide an LRU cache layer on top. Most of the reads done by // Vault are for policy objects so there is a large read reduction // by using a simple write-through cache. type Cache struct { - backend Backend - lru *lru.TwoQueueCache - locks []*locksutil.LockEntry - logger log.Logger - enabled *uint32 + backend Backend + lru *lru.TwoQueueCache + locks []*locksutil.LockEntry + logger log.Logger + enabled *uint32 + cacheExceptions *pathmanager.PathManager } // TransactionalCache is a Cache that wraps the physical that is transactional @@ -48,6 +59,9 @@ func NewCache(b Backend, size int, logger log.Logger) *Cache { size = DefaultCacheSize } + pm := pathmanager.New() + pm.AddPaths(cacheExceptionsPaths) + cache, _ := lru.New2Q(size) c := &Cache{ backend: b, @@ -55,7 +69,8 @@ func NewCache(b Backend, size int, logger log.Logger) *Cache { locks: locksutil.CreateLocks(), logger: logger, // This fails safe. - enabled: new(uint32), + enabled: new(uint32), + cacheExceptions: pm, } return c } @@ -68,6 +83,14 @@ func NewTransactionalCache(b Backend, size int, logger log.Logger) *Transactiona return c } +func (c *Cache) shouldCache(key string) bool { + if atomic.LoadUint32(c.enabled) == 0 { + return false + } + + return !c.cacheExceptions.HasPath(key) +} + // SetEnabled is used to toggle whether the cache is on or off. It must be // called with true to actually activate the cache after creation. func (c *Cache) SetEnabled(enabled bool) { @@ -90,7 +113,7 @@ func (c *Cache) Purge(ctx context.Context) { } func (c *Cache) Put(ctx context.Context, entry *Entry) error { - if atomic.LoadUint32(c.enabled) == 0 { + if entry != nil && !c.shouldCache(entry.Key) { return c.backend.Put(ctx, entry) } @@ -106,7 +129,7 @@ func (c *Cache) Put(ctx context.Context, entry *Entry) error { } func (c *Cache) Get(ctx context.Context, key string) (*Entry, error) { - if atomic.LoadUint32(c.enabled) == 0 { + if !c.shouldCache(key) { return c.backend.Get(ctx, key) } @@ -137,7 +160,7 @@ func (c *Cache) Get(ctx context.Context, key string) (*Entry, error) { } func (c *Cache) Delete(ctx context.Context, key string) error { - if atomic.LoadUint32(c.enabled) == 0 { + if !c.shouldCache(key) { return c.backend.Delete(ctx, key) } @@ -160,6 +183,11 @@ func (c *Cache) List(ctx context.Context, prefix string) ([]string, error) { } func (c *TransactionalCache) Transaction(ctx context.Context, txns []*TxnEntry) error { + // Bypass the locking below + if atomic.LoadUint32(c.enabled) == 0 { + return c.Transactional.Transaction(ctx, txns) + } + // Collect keys that need to be locked var keys []string for _, curr := range txns { @@ -175,14 +203,16 @@ func (c *TransactionalCache) Transaction(ctx context.Context, txns []*TxnEntry) return err } - if atomic.LoadUint32(c.enabled) == 1 { - for _, txn := range txns { - switch txn.Operation { - case PutOperation: - c.lru.Add(txn.Entry.Key, txn.Entry) - case DeleteOperation: - c.lru.Remove(txn.Entry.Key) - } + for _, txn := range txns { + if !c.shouldCache(txn.Entry.Key) { + continue + } + + switch txn.Operation { + case PutOperation: + c.lru.Add(txn.Entry.Key, txn.Entry) + case DeleteOperation: + c.lru.Remove(txn.Entry.Key) } } diff --git a/vault/core.go b/vault/core.go index a6da77dcd2..7194c7ae82 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1660,6 +1660,7 @@ func (c *Core) postUnseal() (retErr error) { c.clearForwardingClients() c.requestForwardingConnectionLock.Unlock() + // Enable the cache c.physicalCache.Purge(c.activeContext) if !c.cachingDisabled { c.physicalCache.SetEnabled(true)