From a36e029d95e481bcb6bcc1644443b4a85b3138cc Mon Sep 17 00:00:00 2001 From: Tomas Touceda Date: Mon, 29 Nov 2021 12:51:57 -0300 Subject: [PATCH] App config caching per instance (#3071) * Do caching of app config per instance instead of across all of them in redis * Add changes file * Simplify code based on review comment * Use go-cache instead of creating our own * Dont export consts * Copy app config before returning it * Fix lint * Update go sum * Update go sum --- changes/app-config-caching-per-instance | 1 + cmd/fleet/serve.go | 2 +- go.mod | 2 + go.sum | 4 ++ server/datastore/cached_mysql/cached_mysql.go | 71 +++++-------------- .../cached_mysql/cached_mysql_test.go | 66 +++++------------ server/fleet/app.go | 7 ++ 7 files changed, 50 insertions(+), 103 deletions(-) create mode 100644 changes/app-config-caching-per-instance diff --git a/changes/app-config-caching-per-instance b/changes/app-config-caching-per-instance new file mode 100644 index 0000000000..311ffb7a1a --- /dev/null +++ b/changes/app-config-caching-per-instance @@ -0,0 +1 @@ +* Cache app config per fleet instance instead of globally on redis diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index 8f5dc357fd..144d357783 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -252,7 +252,7 @@ the way that the Fleet server works. } level.Info(logger).Log("component", "redis", "mode", redisPool.Mode()) - ds = cached_mysql.New(ds, redisPool) + ds = cached_mysql.New(ds) resultStore := pubsub.NewRedisQueryResults(redisPool, config.Redis.DuplicateResults) liveQueryStore := live_query.NewRedisLiveQuery(redisPool) if err := liveQueryStore.MigrateKeys(); err != nil { diff --git a/go.mod b/go.mod index 5a2beebc4c..d0f7ff02ee 100644 --- a/go.mod +++ b/go.mod @@ -40,6 +40,7 @@ require ( github.com/gosuri/uilive v0.0.4 github.com/hectane/go-acl v0.0.0-20190604041725-da78bae5fc95 github.com/igm/sockjs-go/v3 v3.0.0 + github.com/jinzhu/copier v0.3.2 github.com/jmoiron/sqlx v1.2.1-0.20190826204134-d7d95172beb5 github.com/jonboulle/clockwork v0.2.2 // indirect github.com/kevinburke/go-bindata v3.22.0+incompatible @@ -55,6 +56,7 @@ require ( github.com/oklog/run v1.1.0 github.com/olekukonko/tablewriter v0.0.5 github.com/open-policy-agent/opa v0.24.0 + github.com/patrickmn/go-cache v2.1.0+incompatible github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v0.9.3 github.com/prometheus/client_model v0.2.0 // indirect diff --git a/go.sum b/go.sum index a0e207aaff..b7abd4c754 100644 --- a/go.sum +++ b/go.sum @@ -490,6 +490,8 @@ github.com/jgautheron/goconst v0.0.0-20201117150253-ccae5bf973f3 h1:7nkB9fLPMwtn github.com/jgautheron/goconst v0.0.0-20201117150253-ccae5bf973f3/go.mod h1:aAosetZ5zaeC/2EfMeRswtxUFBpe2Hr7HzkgX4fanO4= github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a h1:GmsqmapfzSJkm28dhRoHz2tLRbJmqhU86IPgBtN3mmk= github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a/go.mod h1:xRskid8CManxVta/ALEhJha/pweKBaVG6fWgc0yH25s= +github.com/jinzhu/copier v0.3.2 h1:QdBOCbaouLDYaIPFfi1bKv5F5tPpeTwXe4sD0jqtz5w= +github.com/jinzhu/copier v0.3.2/go.mod h1:24xnZezI2Yqac9J61UC6/dG/k76ttpq0DdJI3QmUvro= github.com/jirfag/go-printf-func-name v0.0.0-20191110105641-45db9963cdd3/go.mod h1:HEWGJkRDzjJY2sqdDwxccsGicWEf9BQOZsq2tV+xzM0= github.com/jirfag/go-printf-func-name v0.0.0-20200119135958-7558a9eaa5af h1:KA9BjwUk7KlCh6S9EAGWBt1oExIUv9WyNCiRz5amv48= github.com/jirfag/go-printf-func-name v0.0.0-20200119135958-7558a9eaa5af/go.mod h1:HEWGJkRDzjJY2sqdDwxccsGicWEf9BQOZsq2tV+xzM0= @@ -670,6 +672,8 @@ github.com/open-policy-agent/opa v0.24.0 h1:fnGOIux+TTGZsC0du1bRBtV8F+KPN55Hks12 github.com/open-policy-agent/opa v0.24.0/go.mod h1:qEyD/i8j+RQettHGp4f86yjrjvv+ZYia+JHCMv2G7wA= github.com/opencensus-integrations/ocsql v0.1.1/go.mod h1:ozPYpNVBHZsX33jfoQPO5TlI5lqh0/3R36kirEqJKAM= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= +github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= +github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k= github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= github.com/pelletier/go-toml v1.8.1/go.mod h1:T2/BmBdy8dvIRq1a/8aqjN41wvWlN4lrapLU/GW4pbc= diff --git a/server/datastore/cached_mysql/cached_mysql.go b/server/datastore/cached_mysql/cached_mysql.go index 5b14014c25..1fbd09773c 100644 --- a/server/datastore/cached_mysql/cached_mysql.go +++ b/server/datastore/cached_mysql/cached_mysql.go @@ -2,94 +2,55 @@ package cached_mysql import ( "context" - "encoding/json" - "fmt" "time" - "github.com/fleetdm/fleet/v4/server/datastore/redis" "github.com/fleetdm/fleet/v4/server/fleet" - redigo "github.com/gomodule/redigo/redis" + "github.com/patrickmn/go-cache" ) type cachedMysql struct { fleet.Datastore - redisPool fleet.RedisPool + c *cache.Cache } const ( - CacheKeyAppConfig = "cache:AppConfig" + appConfigKey = "AppConfig" + defaultAppConfigExpiration = 1 * time.Second ) -func New(ds fleet.Datastore, redisPool fleet.RedisPool) fleet.Datastore { +func New(ds fleet.Datastore) fleet.Datastore { return &cachedMysql{ Datastore: ds, - redisPool: redisPool, + c: cache.New(5*time.Minute, 10*time.Minute), } } -func (ds *cachedMysql) storeInRedis(key string, v interface{}) error { - conn := redis.ConfigureDoer(ds.redisPool, ds.redisPool.Get()) - defer conn.Close() - - b, err := json.Marshal(v) - if err != nil { - return fmt.Errorf("marshaling object to cache in redis: %w", err) - } - - if _, err := conn.Do("SET", key, b, "EX", (24 * time.Hour).Seconds()); err != nil { - return fmt.Errorf("caching object in redis: %w", err) - } - - return nil -} - -func (ds *cachedMysql) getFromRedis(key string, v interface{}) error { - conn := redis.ReadOnlyConn(ds.redisPool, - redis.ConfigureDoer(ds.redisPool, ds.redisPool.Get())) - defer conn.Close() - - data, err := redigo.Bytes(conn.Do("GET", key)) - if err != nil { - return fmt.Errorf("getting value from cache: %w", err) - } - - err = json.Unmarshal(data, v) - if err != nil { - return fmt.Errorf("unmarshaling object from cache: %w", err) - } - - return nil -} - func (ds *cachedMysql) NewAppConfig(ctx context.Context, info *fleet.AppConfig) (*fleet.AppConfig, error) { ac, err := ds.Datastore.NewAppConfig(ctx, info) if err != nil { return nil, err } - err = ds.storeInRedis(CacheKeyAppConfig, ac) + ds.c.Set(appConfigKey, ac, defaultAppConfigExpiration) - return ac, err + return ac.Clone() } func (ds *cachedMysql) AppConfig(ctx context.Context) (*fleet.AppConfig, error) { - ac := &fleet.AppConfig{} - ac.ApplyDefaults() - - err := ds.getFromRedis(CacheKeyAppConfig, ac) - if err == nil { - return ac, nil + cachedAc, found := ds.c.Get(appConfigKey) + if found { + return cachedAc.(*fleet.AppConfig).Clone() } - ac, err = ds.Datastore.AppConfig(ctx) + ac, err := ds.Datastore.AppConfig(ctx) if err != nil { return nil, err } - err = ds.storeInRedis(CacheKeyAppConfig, ac) + ds.c.Set(appConfigKey, ac, defaultAppConfigExpiration) - return ac, err + return ac.Clone() } func (ds *cachedMysql) SaveAppConfig(ctx context.Context, info *fleet.AppConfig) error { @@ -98,5 +59,7 @@ func (ds *cachedMysql) SaveAppConfig(ctx context.Context, info *fleet.AppConfig) return err } - return ds.storeInRedis(CacheKeyAppConfig, info) + ds.c.Set(appConfigKey, info, defaultAppConfigExpiration) + + return nil } diff --git a/server/datastore/cached_mysql/cached_mysql_test.go b/server/datastore/cached_mysql/cached_mysql_test.go index e2041531e3..4412ac9d6b 100644 --- a/server/datastore/cached_mysql/cached_mysql_test.go +++ b/server/datastore/cached_mysql/cached_mysql_test.go @@ -4,24 +4,18 @@ import ( "context" "encoding/json" "testing" + "time" - "github.com/fleetdm/fleet/v4/server/datastore/redis/redistest" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mock" "github.com/fleetdm/fleet/v4/server/ptr" - redigo "github.com/gomodule/redigo/redis" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestCachedAppConfig(t *testing.T) { - pool := redistest.SetupRedis(t, false, false, false) - conn := pool.Get() - _, err := conn.Do("DEL", CacheKeyAppConfig) - require.NoError(t, err) - mockedDS := new(mock.Store) - ds := New(mockedDS, pool) + ds := New(mockedDS) var appConfigSet *fleet.AppConfig mockedDS.NewAppConfigFunc = func(ctx context.Context, info *fleet.AppConfig) (*fleet.AppConfig, error) { @@ -29,13 +23,13 @@ func TestCachedAppConfig(t *testing.T) { return info, nil } mockedDS.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { - return appConfigSet, err + return appConfigSet, nil } mockedDS.SaveAppConfigFunc = func(ctx context.Context, info *fleet.AppConfig) error { appConfigSet = info return nil } - _, err = ds.NewAppConfig(context.Background(), &fleet.AppConfig{ + _, err := ds.NewAppConfig(context.Background(), &fleet.AppConfig{ HostSettings: fleet.HostSettings{ AdditionalQueries: ptr.RawMessage(json.RawMessage(`"TestCachedAppConfig"`)), }, @@ -43,14 +37,11 @@ func TestCachedAppConfig(t *testing.T) { require.NoError(t, err) t.Run("NewAppConfig", func(t *testing.T) { - data, err := redigo.Bytes(conn.Do("GET", CacheKeyAppConfig)) + data, err := ds.AppConfig(context.Background()) require.NoError(t, err) require.NotEmpty(t, data) - newAc := &fleet.AppConfig{} - require.NoError(t, json.Unmarshal(data, &newAc)) - require.NotNil(t, newAc.HostSettings.AdditionalQueries) - assert.Equal(t, json.RawMessage(`"TestCachedAppConfig"`), *newAc.HostSettings.AdditionalQueries) + assert.Equal(t, json.RawMessage(`"TestCachedAppConfig"`), *data.HostSettings.AdditionalQueries) }) t.Run("AppConfig", func(t *testing.T) { @@ -62,16 +53,6 @@ func TestCachedAppConfig(t *testing.T) { require.Equal(t, ptr.RawMessage(json.RawMessage(`"TestCachedAppConfig"`)), ac.HostSettings.AdditionalQueries) }) - t.Run("AppConfig uses DS if redis fails", func(t *testing.T) { - _, err = conn.Do("DEL", CacheKeyAppConfig) - require.NoError(t, err) - ac, err := ds.AppConfig(context.Background()) - require.NoError(t, err) - require.True(t, mockedDS.AppConfigFuncInvoked) - - require.Equal(t, ptr.RawMessage(json.RawMessage(`"TestCachedAppConfig"`)), ac.HostSettings.AdditionalQueries) - }) - t.Run("SaveAppConfig", func(t *testing.T) { require.NoError(t, ds.SaveAppConfig(context.Background(), &fleet.AppConfig{ HostSettings: fleet.HostSettings{ @@ -79,14 +60,7 @@ func TestCachedAppConfig(t *testing.T) { }, })) - data, err := redigo.Bytes(conn.Do("GET", CacheKeyAppConfig)) - require.NoError(t, err) - - require.NotEmpty(t, data) - newAc := &fleet.AppConfig{} - require.NoError(t, json.Unmarshal(data, &newAc)) - require.NotNil(t, newAc.HostSettings.AdditionalQueries) - assert.Equal(t, json.RawMessage(`"NewSAVED"`), *newAc.HostSettings.AdditionalQueries) + assert.True(t, mockedDS.SaveAppConfigFuncInvoked) ac, err := ds.AppConfig(context.Background()) require.NoError(t, err) @@ -94,24 +68,20 @@ func TestCachedAppConfig(t *testing.T) { assert.Equal(t, json.RawMessage(`"NewSAVED"`), *ac.HostSettings.AdditionalQueries) }) - t.Run("AuthenticateHost skips cache if disabled", func(t *testing.T) { - _, err = conn.Do("DEL", CacheKeyAppConfig) - require.NoError(t, err) - + t.Run("External SaveAppConfig gets caught", func(t *testing.T) { mockedDS.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { - return &fleet.AppConfig{}, nil + return &fleet.AppConfig{ + HostSettings: fleet.HostSettings{ + AdditionalQueries: ptr.RawMessage(json.RawMessage(`"SavedSomewhereElse"`)), + }, + }, nil } - mockedDS.AuthenticateHostFunc = func(ctx context.Context, nodeKey string) (*fleet.Host, error) { - return &fleet.Host{ID: 999}, nil - } - _, err = ds.AuthenticateHost(context.Background(), "1234") - require.NoError(t, err) - require.True(t, mockedDS.AuthenticateHostFuncInvoked) - mockedDS.AuthenticateHostFuncInvoked = false - _, err = ds.AuthenticateHost(context.Background(), "1234") + time.Sleep(2 * time.Second) + + ac, err := ds.AppConfig(context.Background()) require.NoError(t, err) - require.True(t, mockedDS.AuthenticateHostFuncInvoked) - mockedDS.AuthenticateHostFuncInvoked = false + require.NotNil(t, ac.HostSettings.AdditionalQueries) + assert.Equal(t, json.RawMessage(`"SavedSomewhereElse"`), *ac.HostSettings.AdditionalQueries) }) } diff --git a/server/fleet/app.go b/server/fleet/app.go index c07a64bd22..82658b214f 100644 --- a/server/fleet/app.go +++ b/server/fleet/app.go @@ -6,6 +6,7 @@ import ( "time" "github.com/fleetdm/fleet/v4/server/config" + "github.com/jinzhu/copier" ) // SMTP settings names returned from API, these map to SMTPAuthType and @@ -395,3 +396,9 @@ type KafkaRESTConfig struct { ResultTopic string `json:"result_topic"` ProxyHost string `json:"proxyhost"` } + +func (c *AppConfig) Clone() (*AppConfig, error) { + newAc := AppConfig{} + err := copier.Copy(&newAc, c) + return &newAc, err +}