From 3bfafcd23c20c22b468c10664c729e922d643e30 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 7 Nov 2023 09:51:55 -0500 Subject: [PATCH] Implement custom cloning of Team MDM config for the cached mysql layer. (#14965) --- changes/issue-14708-fix-cached-team-mdm | 1 + .../cached_mysql/bench_cached_mysql_test.go | 72 +++++++++++++++++++ server/datastore/cached_mysql/cached_mysql.go | 9 +++ .../cached_mysql/cached_mysql_test.go | 40 ++++++++++- server/fleet/app.go | 53 +++++++++++++- server/fleet/app_test.go | 47 ++++++++++++ server/fleet/teams.go | 34 +++++++++ server/fleet/teams_test.go | 41 +++++++++++ 8 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 changes/issue-14708-fix-cached-team-mdm create mode 100644 server/datastore/cached_mysql/bench_cached_mysql_test.go diff --git a/changes/issue-14708-fix-cached-team-mdm b/changes/issue-14708-fix-cached-team-mdm new file mode 100644 index 0000000000..553cfb63de --- /dev/null +++ b/changes/issue-14708-fix-cached-team-mdm @@ -0,0 +1 @@ +* Fixed caching of a team's MDM configuration so that it implements a custom cloning, avoiding performance issues at scale. diff --git a/server/datastore/cached_mysql/bench_cached_mysql_test.go b/server/datastore/cached_mysql/bench_cached_mysql_test.go new file mode 100644 index 0000000000..68a518429f --- /dev/null +++ b/server/datastore/cached_mysql/bench_cached_mysql_test.go @@ -0,0 +1,72 @@ +package cached_mysql + +import ( + "testing" + "time" + + "github.com/fleetdm/fleet/v4/pkg/optjson" + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/ptr" + "github.com/patrickmn/go-cache" + "github.com/stretchr/testify/require" +) + +type unclonableTeamMDMConfig fleet.TeamMDM + +// This exported variable is to make sure that the compiler doesn't optimize +// away the benchmarked function call. +var Result interface{} + +// On my laptop, results are as follows. Under load, the reflection-based +// approach really adds up and is CPU intensive, resulting in drastic +// performance drops. +// +// goos: linux +// goarch: amd64 +// pkg: github.com/fleetdm/fleet/v4/server/datastore/cached_mysql +// cpu: Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz +// BenchmarkCacheGetFallbackClone-8 53228 22706 ns/op 11976 B/op 217 allocs/op +// BenchmarkCacheGetCustomClone-8 5741186 196.3 ns/op 177 B/op 3 allocs/op + +func BenchmarkCacheGetFallbackClone(b *testing.B) { + v := unclonableTeamMDMConfig(cachedValue()) + benchmarkCacheGet(b, &v) +} + +func BenchmarkCacheGetCustomClone(b *testing.B) { + v := cachedValue() + benchmarkCacheGet(b, &v) +} + +func benchmarkCacheGet(b *testing.B, v any) { + c := &cloneCache{cache.New(time.Minute, time.Minute)} + c.Set("k", v, cache.DefaultExpiration) + + b.ResetTimer() + + var ok bool + for i := 0; i < b.N; i++ { + Result, ok = c.Get("k") + if !ok { + b.Fatal("expected ok") + } + } + require.Equal(b, v, Result) +} + +func cachedValue() fleet.TeamMDM { + return fleet.TeamMDM{ + EnableDiskEncryption: true, + MacOSUpdates: fleet.MacOSUpdates{ + MinimumVersion: optjson.SetString("10.10.10"), + Deadline: optjson.SetString("1992-03-01"), + }, + MacOSSettings: fleet.MacOSSettings{ + CustomSettings: []string{"a", "b"}, + DeprecatedEnableDiskEncryption: ptr.Bool(false), + }, + MacOSSetup: fleet.MacOSSetup{ + BootstrapPackage: optjson.SetString("bootstrap"), + }, + } +} diff --git a/server/datastore/cached_mysql/cached_mysql.go b/server/datastore/cached_mysql/cached_mysql.go index 1c33a17638..c602c90108 100644 --- a/server/datastore/cached_mysql/cached_mysql.go +++ b/server/datastore/cached_mysql/cached_mysql.go @@ -41,6 +41,11 @@ func clone(v interface{}) (interface{}, error) { return cloner.Clone() } + // TODO(mna): consider making implementation of the cloner interface + // mandatory, and panic/fail loudly if not implemented. Reflection-based deep + // cloning has significant performance issues at scale (better yet - make the + // cache accept/return cloner types instead of interface{}). + if v == nil { return nil, nil } @@ -157,6 +162,7 @@ func New(ds fleet.Datastore, opts ...Option) fleet.Datastore { scheduledQueriesExp: defaultScheduledQueriesExpiration, teamAgentOptionsExp: defaultTeamAgentOptionsExpiration, teamFeaturesExp: defaultTeamFeaturesExpiration, + teamMDMConfigExp: defaultTeamMDMConfigExpiration, queryByNameExp: defaultQueryByNameExpiration, queryResultsCountExp: defaultQueryResultsCountExpiration, } @@ -292,6 +298,9 @@ func (ds *cachedMysql) TeamMDMConfig(ctx context.Context, teamID uint) (*fleet.T if err != nil { return nil, err } + + ds.c.Set(key, cfg, ds.teamMDMConfigExp) + return cfg, nil } diff --git a/server/datastore/cached_mysql/cached_mysql_test.go b/server/datastore/cached_mysql/cached_mysql_test.go index 0417987931..fb446c4697 100644 --- a/server/datastore/cached_mysql/cached_mysql_test.go +++ b/server/datastore/cached_mysql/cached_mysql_test.go @@ -400,6 +400,7 @@ func TestCachedTeamFeatures(t *testing.T) { EnableHostUsers: false, EnableSoftwareInventory: true, AdditionalQueries: &aq, + DetailQueryOverrides: map[string]*string{"a": ptr.String("A"), "b": ptr.String("B")}, } testTeam := fleet.Team{ @@ -427,9 +428,18 @@ func TestCachedTeamFeatures(t *testing.T) { return nil } + // get it the first time, it will populate the cache features, err := ds.TeamFeatures(context.Background(), 1) require.NoError(t, err) require.Equal(t, testFeatures, *features) + require.True(t, mockedDS.TeamFeaturesFuncInvoked) + mockedDS.TeamFeaturesFuncInvoked = false + + // get it again, will retrieve it from the cache + features, err = ds.TeamFeatures(context.Background(), 1) + require.NoError(t, err) + require.Equal(t, testFeatures, *features) + require.False(t, mockedDS.TeamFeaturesFuncInvoked) // saving a team updates features in cache aq = json.RawMessage(`{"bar": "baz"}`) @@ -437,6 +447,7 @@ func TestCachedTeamFeatures(t *testing.T) { EnableHostUsers: true, EnableSoftwareInventory: false, AdditionalQueries: &aq, + DetailQueryOverrides: map[string]*string{"c": ptr.String("C")}, } updateTeam := &fleet.Team{ ID: testTeam.ID, @@ -450,10 +461,12 @@ func TestCachedTeamFeatures(t *testing.T) { _, err = ds.SaveTeam(context.Background(), updateTeam) require.NoError(t, err) + require.True(t, mockedDS.SaveTeamFuncInvoked) features, err = ds.TeamFeatures(context.Background(), testTeam.ID) require.NoError(t, err) require.Equal(t, updateFeatures, *features) + require.False(t, mockedDS.TeamFeaturesFuncInvoked) // deleting a team removes the features from the cache err = ds.DeleteTeam(context.Background(), testTeam.ID) @@ -461,20 +474,29 @@ func TestCachedTeamFeatures(t *testing.T) { _, err = ds.TeamFeatures(context.Background(), testTeam.ID) require.Error(t, err) + require.True(t, mockedDS.TeamFeaturesFuncInvoked) } func TestCachedTeamMDMConfig(t *testing.T) { t.Parallel() mockedDS := new(mock.Store) - ds := New(mockedDS, WithTeamFeaturesExpiration(100*time.Millisecond)) + ds := New(mockedDS, WithTeamMDMConfigExpiration(100*time.Millisecond)) ao := json.RawMessage(`{}`) testMDMConfig := fleet.TeamMDM{ + EnableDiskEncryption: true, MacOSUpdates: fleet.MacOSUpdates{ MinimumVersion: optjson.SetString("10.10.10"), Deadline: optjson.SetString("1992-03-01"), }, + MacOSSettings: fleet.MacOSSettings{ + CustomSettings: []string{"a", "b"}, + DeprecatedEnableDiskEncryption: ptr.Bool(false), + }, + MacOSSetup: fleet.MacOSSetup{ + BootstrapPackage: optjson.SetString("bootstrap"), + }, } testTeam := fleet.Team{ @@ -502,9 +524,18 @@ func TestCachedTeamMDMConfig(t *testing.T) { return nil } + // get the team's config, will load it into cache mdmConfig, err := ds.TeamMDMConfig(context.Background(), 1) require.NoError(t, err) require.Equal(t, testMDMConfig, *mdmConfig) + require.True(t, mockedDS.TeamMDMConfigFuncInvoked) + mockedDS.TeamMDMConfigFuncInvoked = false + + // get it again, will get it from cache + mdmConfig, err = ds.TeamMDMConfig(context.Background(), 1) + require.NoError(t, err) + require.Equal(t, testMDMConfig, *mdmConfig) + require.False(t, mockedDS.TeamMDMConfigFuncInvoked) // saving a team updates config in cache updateMDMConfig := fleet.TeamMDM{ @@ -512,6 +543,10 @@ func TestCachedTeamMDMConfig(t *testing.T) { MinimumVersion: optjson.SetString("13.13.13"), Deadline: optjson.SetString("2022-03-01"), }, + MacOSSettings: fleet.MacOSSettings{ + CustomSettings: nil, + DeprecatedEnableDiskEncryption: ptr.Bool(true), + }, } updateTeam := &fleet.Team{ ID: testTeam.ID, @@ -525,10 +560,12 @@ func TestCachedTeamMDMConfig(t *testing.T) { _, err = ds.SaveTeam(context.Background(), updateTeam) require.NoError(t, err) + require.True(t, mockedDS.SaveTeamFuncInvoked) mdmConfig, err = ds.TeamMDMConfig(context.Background(), testTeam.ID) require.NoError(t, err) require.Equal(t, updateMDMConfig, *mdmConfig) + require.False(t, mockedDS.TeamMDMConfigFuncInvoked) // deleting a team removes the config from the cache err = ds.DeleteTeam(context.Background(), testTeam.ID) @@ -536,4 +573,5 @@ func TestCachedTeamMDMConfig(t *testing.T) { _, err = ds.TeamMDMConfig(context.Background(), testTeam.ID) require.Error(t, err) + require.True(t, mockedDS.TeamMDMConfigFuncInvoked) } diff --git a/server/fleet/app.go b/server/fleet/app.go index e12e04892f..6d1db0993a 100644 --- a/server/fleet/app.go +++ b/server/fleet/app.go @@ -443,7 +443,17 @@ func (c *AppConfig) Copy() *AppConfig { if c.Features.AdditionalQueries != nil { aq := make(json.RawMessage, len(*c.Features.AdditionalQueries)) copy(aq, *c.Features.AdditionalQueries) - c.Features.AdditionalQueries = &aq + clone.Features.AdditionalQueries = &aq + } + if c.Features.DetailQueryOverrides != nil { + clone.Features.DetailQueryOverrides = make(map[string]*string, len(c.Features.DetailQueryOverrides)) + for k, v := range c.Features.DetailQueryOverrides { + var s *string + if v != nil { + s = ptr.String(*v) + } + clone.Features.DetailQueryOverrides[k] = s + } } if c.AgentOptions != nil { ao := make(json.RawMessage, len(*c.AgentOptions)) @@ -774,6 +784,11 @@ type Features struct { EnableSoftwareInventory bool `json:"enable_software_inventory"` AdditionalQueries *json.RawMessage `json:"additional_queries,omitempty"` DetailQueryOverrides map[string]*string `json:"detail_query_overrides,omitempty"` + + ///////////////////////////////////////////////////////////////// + // WARNING: If you add to this struct make sure it's taken into + // account in the Features Clone implementation! + ///////////////////////////////////////////////////////////////// } func (f *Features) ApplyDefaultsForNewInstalls() { @@ -788,6 +803,42 @@ func (f *Features) ApplyDefaults() { f.EnableHostUsers = true } +// Clone implements cloner for Features. +func (f *Features) Clone() (interface{}, error) { + return f.Copy(), nil +} + +// Copy returns a deep copy of the Features. +func (f *Features) Copy() *Features { + if f == nil { + return nil + } + + // EnableHostUsers and EnableSoftwareInventory don't have fields that require + // cloning (all fields are basic value types, no pointers/slices/maps). + + var clone Features + clone = *f + + if f.AdditionalQueries != nil { + aq := make(json.RawMessage, len(*f.AdditionalQueries)) + copy(aq, *f.AdditionalQueries) + clone.AdditionalQueries = &aq + } + if f.DetailQueryOverrides != nil { + clone.DetailQueryOverrides = make(map[string]*string, len(f.DetailQueryOverrides)) + for k, v := range f.DetailQueryOverrides { + var s *string + if v != nil { + s = ptr.String(*v) + } + clone.DetailQueryOverrides[k] = s + } + } + + return &clone +} + // FleetDesktopSettings contains settings used to configure Fleet Desktop. type FleetDesktopSettings struct { // TransparencyURL is the URL used for the “Transparency” link in the Fleet Desktop menu. diff --git a/server/fleet/app_test.go b/server/fleet/app_test.go index bc7a4173d3..79ac1847fa 100644 --- a/server/fleet/app_test.go +++ b/server/fleet/app_test.go @@ -311,3 +311,50 @@ func TestAtLeastOnePlatformEnabledAndConfigured(t *testing.T) { }) } } + +func TestFeaturesCopy(t *testing.T) { + t.Run("nil receiver", func(t *testing.T) { + var f *Features + require.Nil(t, f.Copy()) + }) + + t.Run("copy value fields", func(t *testing.T) { + f := &Features{ + EnableHostUsers: true, + EnableSoftwareInventory: false, + } + clone := f.Copy() + require.NotNil(t, clone) + require.Equal(t, f.EnableHostUsers, clone.EnableHostUsers) + require.Equal(t, f.EnableSoftwareInventory, clone.EnableSoftwareInventory) + require.Nil(t, clone.AdditionalQueries) + require.Nil(t, clone.DetailQueryOverrides) + }) + + t.Run("copy AdditionalQueries", func(t *testing.T) { + rawMessage := json.RawMessage(`{"test": "data"}`) + f := &Features{ + AdditionalQueries: &rawMessage, + } + clone := f.Copy() + require.NotNil(t, clone.AdditionalQueries) + require.NotSame(t, f.AdditionalQueries, clone.AdditionalQueries) + require.Equal(t, *f.AdditionalQueries, *clone.AdditionalQueries) + }) + + t.Run("copy DetailQueryOverrides", func(t *testing.T) { + f := &Features{ + DetailQueryOverrides: map[string]*string{ + "foo": ptr.String("bar"), + "baz": nil, + }, + } + clone := f.Copy() + require.NotNil(t, clone.DetailQueryOverrides) + require.NotSame(t, f.DetailQueryOverrides, clone.DetailQueryOverrides) + // map values are pointers, check that they have been cloned + require.NotSame(t, f.DetailQueryOverrides["foo"], clone.DetailQueryOverrides["foo"]) + // the map content itself is equal + require.Equal(t, f.DetailQueryOverrides, clone.DetailQueryOverrides) + }) +} diff --git a/server/fleet/teams.go b/server/fleet/teams.go index bf33bede63..087104ad23 100644 --- a/server/fleet/teams.go +++ b/server/fleet/teams.go @@ -7,6 +7,7 @@ import ( "time" "github.com/fleetdm/fleet/v4/pkg/optjson" + "github.com/fleetdm/fleet/v4/server/ptr" ) const ( @@ -152,6 +153,39 @@ type TeamMDM struct { MacOSSettings MacOSSettings `json:"macos_settings"` MacOSSetup MacOSSetup `json:"macos_setup"` // NOTE: TeamSpecMDM must be kept in sync with TeamMDM. + + ///////////////////////////////////////////////////////////////// + // WARNING: If you add to this struct make sure it's taken into + // account in the TeamMDM Clone implementation! + ///////////////////////////////////////////////////////////////// +} + +// Clone implements cloner for TeamMDM. +func (t *TeamMDM) Clone() (interface{}, error) { + return t.Copy(), nil +} + +// Copy returns a deep copy of the TeamMDM. +func (t *TeamMDM) Copy() *TeamMDM { + if t == nil { + return nil + } + + var clone TeamMDM + clone = *t + + // EnableDiskEncryption, MacOSUpdates and MacOSSetup don't have fields that + // require cloning (all fields are basic value types, no + // pointers/slices/maps). + + if t.MacOSSettings.CustomSettings != nil { + clone.MacOSSettings.CustomSettings = make([]string, len(t.MacOSSettings.CustomSettings)) + copy(clone.MacOSSettings.CustomSettings, t.MacOSSettings.CustomSettings) + } + if t.MacOSSettings.DeprecatedEnableDiskEncryption != nil { + clone.MacOSSettings.DeprecatedEnableDiskEncryption = ptr.Bool(*t.MacOSSettings.DeprecatedEnableDiskEncryption) + } + return &clone } type TeamSpecMDM struct { diff --git a/server/fleet/teams_test.go b/server/fleet/teams_test.go index b0fd507397..10289b4e85 100644 --- a/server/fleet/teams_test.go +++ b/server/fleet/teams_test.go @@ -3,6 +3,7 @@ package fleet import ( "testing" + "github.com/fleetdm/fleet/v4/pkg/optjson" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/stretchr/testify/require" ) @@ -258,3 +259,43 @@ func TestValidateUserRoles(t *testing.T) { }) } } + +func TestTeamMDMCopy(t *testing.T) { + t.Run("nil receiver", func(t *testing.T) { + var tm *TeamMDM + require.Nil(t, tm.Copy()) + }) + + t.Run("copy value fields", func(t *testing.T) { + tm := &TeamMDM{ + EnableDiskEncryption: true, + MacOSUpdates: MacOSUpdates{ + MinimumVersion: optjson.SetString("10.15.4"), + Deadline: optjson.SetString("2020-01-01"), + }, + MacOSSetup: MacOSSetup{ + BootstrapPackage: optjson.SetString("bootstrap"), + EnableEndUserAuthentication: true, + MacOSSetupAssistant: optjson.SetString("assistant"), + }, + } + clone := tm.Copy() + require.NotNil(t, clone) + require.NotSame(t, tm, clone) + require.Equal(t, tm, clone) + }) + + t.Run("copy MacOSSettings", func(t *testing.T) { + tm := &TeamMDM{ + MacOSSettings: MacOSSettings{ + CustomSettings: []string{"a", "b"}, + DeprecatedEnableDiskEncryption: ptr.Bool(false), + }, + } + clone := tm.Copy() + require.NotSame(t, tm, clone) + require.Equal(t, tm, clone) + require.NotSame(t, tm.MacOSSettings.CustomSettings, clone.MacOSSettings.CustomSettings) + require.NotSame(t, tm.MacOSSettings.DeprecatedEnableDiskEncryption, clone.MacOSSettings.DeprecatedEnableDiskEncryption) + }) +}