From 7c55dbf37d15a647525ecad0f705dcade51e005f Mon Sep 17 00:00:00 2001 From: Carl Bergquist Date: Wed, 8 Mar 2023 17:08:57 +0100 Subject: [PATCH] Remotecache: Migrates get/set calls to use bytearrays and remove get/set functions (#63525) Signed-off-by: bergquist --- pkg/infra/remotecache/remotecache.go | 24 +-------------- pkg/infra/remotecache/remotecache_test.go | 37 +++++++++++------------ pkg/services/anonymous/anonimpl/impl.go | 2 +- pkg/services/authn/clients/proxy.go | 19 +++++++----- pkg/services/authn/clients/proxy_test.go | 6 ++-- 5 files changed, 34 insertions(+), 54 deletions(-) diff --git a/pkg/infra/remotecache/remotecache.go b/pkg/infra/remotecache/remotecache.go index a5193283641..0d9aa6f04fa 100644 --- a/pkg/infra/remotecache/remotecache.go +++ b/pkg/infra/remotecache/remotecache.go @@ -55,12 +55,6 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, secretsService secrets.Ser // so any struct added to the cache needs to be registered with `remotecache.Register` // ex `remotecache.Register(CacheableStruct{})` type CacheStorage interface { - // Get reads object from Cache - Get(ctx context.Context, key string) (interface{}, error) - - // Set sets an object into the cache. if `expire` is set to zero it will default to 24h - Set(ctx context.Context, key string, value interface{}, expire time.Duration) error - // GetByteArray gets the cache value as an byte array GetByteArray(ctx context.Context, key string) ([]byte, error) @@ -83,11 +77,6 @@ type RemoteCache struct { Cfg *setting.Cfg } -// Get reads object from Cache -func (ds *RemoteCache) Get(ctx context.Context, key string) (interface{}, error) { - return ds.client.Get(ctx, key) -} - // GetByteArray returns the cached value as an byte array func (ds *RemoteCache) GetByteArray(ctx context.Context, key string) ([]byte, error) { return ds.client.GetByteArray(ctx, key) @@ -95,16 +84,11 @@ func (ds *RemoteCache) GetByteArray(ctx context.Context, key string) ([]byte, er // SetByteArray stored the byte array in the cache func (ds *RemoteCache) SetByteArray(ctx context.Context, key string, value []byte, expire time.Duration) error { - return ds.client.SetByteArray(ctx, key, value, expire) -} - -// Set sets an object into the cache. if `expire` is set to zero it will default to 24h -func (ds *RemoteCache) Set(ctx context.Context, key string, value interface{}, expire time.Duration) error { if expire == 0 { expire = defaultMaxCacheExpiration } - return ds.client.Set(ctx, key, value, expire) + return ds.client.SetByteArray(ctx, key, value, expire) } // Delete object from cache @@ -208,15 +192,9 @@ type prefixCacheStorage struct { prefix string } -func (pcs *prefixCacheStorage) Get(ctx context.Context, key string) (interface{}, error) { - return pcs.cache.Get(ctx, pcs.prefix+key) -} func (pcs *prefixCacheStorage) GetByteArray(ctx context.Context, key string) ([]byte, error) { return pcs.cache.GetByteArray(ctx, pcs.prefix+key) } -func (pcs *prefixCacheStorage) Set(ctx context.Context, key string, value interface{}, expire time.Duration) error { - return pcs.cache.Set(ctx, pcs.prefix+key, value, expire) -} func (pcs *prefixCacheStorage) SetByteArray(ctx context.Context, key string, value []byte, expire time.Duration) error { return pcs.cache.SetByteArray(ctx, pcs.prefix+key, value, expire) } diff --git a/pkg/infra/remotecache/remotecache_test.go b/pkg/infra/remotecache/remotecache_test.go index 502f08946c4..1c6bb6281f0 100644 --- a/pkg/infra/remotecache/remotecache_test.go +++ b/pkg/infra/remotecache/remotecache_test.go @@ -65,15 +65,15 @@ func runCountTestsForClient(t *testing.T, opts *setting.RemoteCacheOptions, sqls } t.Run("can count items", func(t *testing.T) { - cacheableStruct := CacheableStruct{String: "hej", Int64: 2000} + cacheableValue := []byte("hej hej") - err := client.Set(context.Background(), "pref-key1", cacheableStruct, 0) + err := client.SetByteArray(context.Background(), "pref-key1", cacheableValue, 0) require.NoError(t, err) - err = client.Set(context.Background(), "pref-key2", cacheableStruct, 0) + err = client.SetByteArray(context.Background(), "pref-key2", cacheableValue, 0) require.NoError(t, err) - err = client.Set(context.Background(), "key3-not-pref", cacheableStruct, 0) + err = client.SetByteArray(context.Background(), "key3-not-pref", cacheableValue, 0) require.NoError(t, err) n, errC := client.Count(context.Background(), "pref-") @@ -89,37 +89,34 @@ func runCountTestsForClient(t *testing.T, opts *setting.RemoteCacheOptions, sqls } func canPutGetAndDeleteCachedObjects(t *testing.T, client CacheStorage) { - cacheableStruct := CacheableStruct{String: "hej", Int64: 2000} + dataToCache := []byte("some bytes") - err := client.Set(context.Background(), "key1", cacheableStruct, 0) + err := client.SetByteArray(context.Background(), "key1", dataToCache, 0) assert.Equal(t, err, nil, "expected nil. got: ", err) - data, err := client.Get(context.Background(), "key1") + data, err := client.GetByteArray(context.Background(), "key1") assert.Equal(t, err, nil) - s, ok := data.(CacheableStruct) - assert.Equal(t, ok, true) - assert.Equal(t, s.String, "hej") - assert.Equal(t, s.Int64, int64(2000)) + assert.Equal(t, string(data), "some bytes") err = client.Delete(context.Background(), "key1") assert.Equal(t, err, nil) - _, err = client.Get(context.Background(), "key1") + _, err = client.GetByteArray(context.Background(), "key1") assert.Equal(t, err, ErrCacheItemNotFound) } func canNotFetchExpiredItems(t *testing.T, client CacheStorage) { - cacheableStruct := CacheableStruct{String: "hej", Int64: 2000} + dataToCache := []byte("some bytes") - err := client.Set(context.Background(), "key1", cacheableStruct, time.Second) + err := client.SetByteArray(context.Background(), "key1", dataToCache, time.Second) assert.Equal(t, err, nil) // not sure how this can be avoided when testing redis/memcached :/ <-time.After(time.Second + time.Millisecond) // should not be able to read that value since its expired - _, err = client.Get(context.Background(), "key1") + _, err = client.GetByteArray(context.Background(), "key1") assert.Equal(t, err, ErrCacheItemNotFound) } @@ -133,16 +130,16 @@ func TestCachePrefix(t *testing.T) { prefixCache := &prefixCacheStorage{cache: cache, prefix: "test/"} // Set a value (with a prefix) - err := prefixCache.Set(context.Background(), "foo", "bar", time.Hour) + err := prefixCache.SetByteArray(context.Background(), "foo", []byte("bar"), time.Hour) require.NoError(t, err) // Get a value (with a prefix) - v, err := prefixCache.Get(context.Background(), "foo") + v, err := prefixCache.GetByteArray(context.Background(), "foo") require.NoError(t, err) - require.Equal(t, "bar", v) + require.Equal(t, "bar", string(v)) // Get a value directly from the underlying cache, ensure the prefix is in the key - v, err = cache.Get(context.Background(), "test/foo") + v, err = cache.GetByteArray(context.Background(), "test/foo") require.NoError(t, err) - require.Equal(t, "bar", v) + require.Equal(t, "bar", string(v)) // Get a value directly from the underlying cache without a prefix, should not be there _, err = cache.Get(context.Background(), "foo") require.Error(t, err) diff --git a/pkg/services/anonymous/anonimpl/impl.go b/pkg/services/anonymous/anonimpl/impl.go index 3c40f38d4d8..c6677086e49 100644 --- a/pkg/services/anonymous/anonimpl/impl.go +++ b/pkg/services/anonymous/anonimpl/impl.go @@ -96,5 +96,5 @@ func (a *AnonSessionService) TagSession(ctx context.Context, httpReq *http.Reque a.localCache.SetDefault(key, struct{}{}) - return a.remoteCache.Set(ctx, key, key, thirtyDays) + return a.remoteCache.SetByteArray(ctx, key, []byte(key), thirtyDays) } diff --git a/pkg/services/authn/clients/proxy.go b/pkg/services/authn/clients/proxy.go index d4683b4fc81..a53fb754edb 100644 --- a/pkg/services/authn/clients/proxy.go +++ b/pkg/services/authn/clients/proxy.go @@ -2,6 +2,7 @@ package clients import ( "context" + "encoding/binary" "encoding/hex" "fmt" "hash/fnv" @@ -49,8 +50,8 @@ func ProvideProxy(cfg *setting.Cfg, cache proxyCache, userSrv user.Service, clie } type proxyCache interface { - Get(ctx context.Context, key string) (interface{}, error) - Set(ctx context.Context, key string, value interface{}, expire time.Duration) error + GetByteArray(ctx context.Context, key string) ([]byte, error) + SetByteArray(ctx context.Context, key string, value []byte, expire time.Duration) error } type Proxy struct { @@ -82,14 +83,16 @@ func (c *Proxy) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden if ok { // See if we have cached the user id, in that case we can fetch the signed-in user and skip sync. // Error here means that we could not find anything in cache, so we can proceed as usual - if entry, err := c.cache.Get(ctx, cacheKey); err == nil { + if entry, err := c.cache.GetByteArray(ctx, cacheKey); err == nil { + uid := int64(binary.LittleEndian.Uint64(entry)) + usr, err := c.userSrv.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{ - UserID: entry.(int64), + UserID: uid, OrgID: r.OrgID, }) if err != nil { - c.log.FromContext(ctx).Warn("Could not resolved cached user", "error", err, "userId", entry.(int64)) + c.log.FromContext(ctx).Warn("Could not resolved cached user", "error", err, "userId", string(entry)) } // if we for some reason cannot find the user we proceed with the normal flow, authenticate with ProxyClient @@ -133,8 +136,10 @@ func (c *Proxy) Hook(ctx context.Context, identity *authn.Identity, r *authn.Req } c.log.FromContext(ctx).Debug("Cache proxy user", "userId", id) - if err := c.cache.Set(ctx, identity.ClientParams.CacheAuthProxyKey, id, time.Duration(c.cfg.AuthProxySyncTTL)*time.Minute); err != nil { - c.log.FromContext(ctx).Warn("Failed to cache proxy user", "error", err, "userId", id) + bytes := make([]byte, 8) + binary.LittleEndian.PutUint64(bytes, uint64(id)) + if err := c.cache.SetByteArray(ctx, identity.ClientParams.CacheAuthProxyKey, bytes, time.Duration(c.cfg.AuthProxySyncTTL)*time.Minute); err != nil { + c.log.Warn("failed to cache proxy user", "error", err, "userId", id) } return nil diff --git a/pkg/services/authn/clients/proxy_test.go b/pkg/services/authn/clients/proxy_test.go index bb1be4d6ee5..a0f39ec0425 100644 --- a/pkg/services/authn/clients/proxy_test.go +++ b/pkg/services/authn/clients/proxy_test.go @@ -178,13 +178,13 @@ var _ proxyCache = new(fakeCache) type fakeCache struct { expectedErr error - expectedItem interface{} + expectedItem []byte } -func (f fakeCache) Get(ctx context.Context, key string) (interface{}, error) { +func (f fakeCache) GetByteArray(ctx context.Context, key string) ([]byte, error) { return f.expectedItem, f.expectedErr } -func (f fakeCache) Set(ctx context.Context, key string, value interface{}, expire time.Duration) error { +func (f fakeCache) SetByteArray(ctx context.Context, key string, value []byte, expire time.Duration) error { return f.expectedErr }