From 148dff2c24f02ccaff5c459800ac781cc5185052 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Fri, 3 Jan 2025 10:25:35 +0530 Subject: [PATCH] MM-62378: Initialize status and session cache before loading license (#29686) * MM-62378: Initialize status and session cache before loading license ps.LoadLicense could end up calling InvalidateAllCaches. Therefore, we need to intialize those caches before calling LoadLicense to prevent a panic from happening. While here, we also remove some unused code. https://mattermost.atlassian.net/browse/MM-62378 ```release-note NONE ``` * fix lint errors ```release-note NONE ``` * fix test ```release-note NONE ``` --- .../channels/app/platform/cluster_handlers.go | 14 ---- server/channels/app/platform/service.go | 68 +++++++++---------- server/channels/app/platform/service_test.go | 35 ++++++++++ 3 files changed, 69 insertions(+), 48 deletions(-) diff --git a/server/channels/app/platform/cluster_handlers.go b/server/channels/app/platform/cluster_handlers.go index b6b56da124..c769dbd566 100644 --- a/server/channels/app/platform/cluster_handlers.go +++ b/server/channels/app/platform/cluster_handlers.go @@ -6,7 +6,6 @@ package platform import ( "bytes" "encoding/json" - "fmt" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" @@ -31,19 +30,6 @@ func (ps *PlatformService) RegisterClusterMessageHandler(ev model.ClusterEvent, ps.additionalClusterHandlers[ev] = h } -// ClusterHandlersPreCheck checks whether the platform service is ready to handle cluster messages. -func (ps *PlatformService) ClusterHandlersPreCheck() error { - if ps.Store == nil { - return fmt.Errorf("could not find store") - } - - if ps.statusCache == nil { - return fmt.Errorf("could not find status cache") - } - - return nil -} - func (ps *PlatformService) ClusterPublishHandler(msg *model.ClusterMessage) { event, err := model.WebSocketEventFromJSON(bytes.NewReader(msg.Data)) if err != nil { diff --git a/server/channels/app/platform/service.go b/server/channels/app/platform/service.go index 2d88573875..91314d7eca 100644 --- a/server/channels/app/platform/service.go +++ b/server/channels/app/platform/service.go @@ -266,7 +266,37 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) { return nil, fmt.Errorf("cannot create store: %w", err) } - // Step 7: Init License + // Step 7: initialize status and session cache. + // We need to do this because ps.LoadLicense() called in step 8, could + // end up calling InvalidateAllCaches, so the status and session caches + // need to be initialized before that. + + // Note: we hardcode the session and status cache to LRU because they lead + // to a lot of SCAN calls in case of Redis. We could potentially have a + // reverse mapping to avoid the scan, but this needs more complicated code. + // Leaving this for now. + ps.statusCache, err = cache.NewProvider().NewCache(&cache.CacheOptions{ + Name: "Status", + Size: model.StatusCacheSize, + Striped: true, + StripedBuckets: max(runtime.NumCPU()-1, 1), + DefaultExpiry: 30 * time.Minute, + }) + if err != nil { + return nil, fmt.Errorf("unable to create status cache: %w", err) + } + + ps.sessionCache, err = cache.NewProvider().NewCache(&cache.CacheOptions{ + Name: "Session", + Size: model.SessionCacheSize, + Striped: true, + StripedBuckets: max(runtime.NumCPU()-1, 1), + }) + if err != nil { + return nil, fmt.Errorf("could not create session cache: %w", err) + } + + // Step 8: Init License if model.BuildEnterpriseReady == "true" { ps.LoadLicense() } @@ -280,7 +310,7 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) { return nil, fmt.Errorf("Redis cannot be used in an instance without a license or a license without clustering") } - // Step 8: Initialize filestore + // Step 9: Initialize filestore if ps.filestore == nil { insecure := ps.Config().ServiceSettings.EnableInsecureOutgoingConnections backend, err2 := filestore.NewFileBackend(filestore.NewFileBackendSettingsFromConfig(&ps.Config().FileSettings, license != nil && *license.Features.Compliance, insecure != nil && *insecure)) @@ -304,33 +334,7 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) { } } - // Note: we hardcode the session and status cache to LRU because they lead - // to a lot of SCAN calls in case of Redis. We could potentially have a - // reverse mapping to avoid the scan, but this needs more complicated code. - // Leaving this for now. - - ps.statusCache, err = cache.NewProvider().NewCache(&cache.CacheOptions{ - Name: "Status", - Size: model.StatusCacheSize, - Striped: true, - StripedBuckets: max(runtime.NumCPU()-1, 1), - DefaultExpiry: 30 * time.Minute, - }) - if err != nil { - return nil, fmt.Errorf("unable to create status cache: %w", err) - } - - ps.sessionCache, err = cache.NewProvider().NewCache(&cache.CacheOptions{ - Name: "Session", - Size: model.SessionCacheSize, - Striped: true, - StripedBuckets: max(runtime.NumCPU()-1, 1), - }) - if err != nil { - return nil, fmt.Errorf("could not create session cache: %w", err) - } - - // Step 9: Init Metrics Server depends on step 6 (store) and 7 (license) + // Step 10: Init Metrics Server depends on step 6 (store) and 8 (license) if ps.startMetrics { if mErr := ps.resetMetrics(); mErr != nil { return nil, mErr @@ -345,7 +349,7 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) { }) } - // Step 10: Init AsymmetricSigningKey depends on step 6 (store) + // Step 11: Init AsymmetricSigningKey depends on step 6 (store) if err = ps.EnsureAsymmetricSigningKey(); err != nil { return nil, fmt.Errorf("unable to ensure asymmetric signing key: %w", err) } @@ -499,10 +503,6 @@ func (ps *PlatformService) CacheProvider() cache.Provider { return ps.cacheProvider } -func (ps *PlatformService) StatusCache() cache.Cache { - return ps.statusCache -} - // SetSqlStore is used for plugin testing func (ps *PlatformService) SetSqlStore(s *sqlstore.SqlStore) { ps.sqlStore = s diff --git a/server/channels/app/platform/service_test.go b/server/channels/app/platform/service_test.go index a4bb740eb7..3149dc50f0 100644 --- a/server/channels/app/platform/service_test.go +++ b/server/channels/app/platform/service_test.go @@ -21,6 +21,41 @@ import ( "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" ) +// A copy of validTestLicense from channels/utils/license_test.go +var validTestLicense = []byte("eyJpZCI6InpvZ3c2NW44Z2lmajVkbHJoYThtYnUxcGl3IiwiaXNzdWVkX2F0IjoxNjg0Nzg3MzcxODY5LCJzdGFydHNfYXQiOjE2ODQ3ODczNzE4NjksImV4cGlyZXNfYXQiOjIwMDA0MDY1MzgwMDAsInNrdV9uYW1lIjoiUHJvZmVzc2lvbmFsIiwic2t1X3Nob3J0X25hbWUiOiJwcm9mZXNzaW9uYWwiLCJjdXN0b21lciI6eyJpZCI6InA5dW4zNjlhNjdnaW1qNHlkNmk2aWIzOXdoIiwibmFtZSI6Ik1hdHRlcm1vc3QiLCJlbWFpbCI6ImpvcmFtQG1hdHRlcm1vc3QuY29tIiwiY29tcGFueSI6Ik1hdHRlcm1vc3QifSwiZmVhdHVyZXMiOnsidXNlcnMiOjIwMDAwMCwibGRhcCI6dHJ1ZSwibGRhcF9ncm91cHMiOmZhbHNlLCJtZmEiOnRydWUsImdvb2dsZV9vYXV0aCI6dHJ1ZSwib2ZmaWNlMzY1X29hdXRoIjp0cnVlLCJjb21wbGlhbmNlIjpmYWxzZSwiY2x1c3RlciI6dHJ1ZSwibWV0cmljcyI6dHJ1ZSwibWhwbnMiOnRydWUsInNhbWwiOnRydWUsImVsYXN0aWNfc2VhcmNoIjp0cnVlLCJhbm5vdW5jZW1lbnQiOnRydWUsInRoZW1lX21hbmFnZW1lbnQiOmZhbHNlLCJlbWFpbF9ub3RpZmljYXRpb25fY29udGVudHMiOmZhbHNlLCJkYXRhX3JldGVudGlvbiI6ZmFsc2UsIm1lc3NhZ2VfZXhwb3J0IjpmYWxzZSwiY3VzdG9tX3Blcm1pc3Npb25zX3NjaGVtZXMiOmZhbHNlLCJjdXN0b21fdGVybXNfb2Zfc2VydmljZSI6ZmFsc2UsImd1ZXN0X2FjY291bnRzIjp0cnVlLCJndWVzdF9hY2NvdW50c19wZXJtaXNzaW9ucyI6dHJ1ZSwiaWRfbG9hZGVkIjpmYWxzZSwibG9ja190ZWFtbWF0ZV9uYW1lX2Rpc3BsYXkiOmZhbHNlLCJjbG91ZCI6ZmFsc2UsInNoYXJlZF9jaGFubmVscyI6ZmFsc2UsInJlbW90ZV9jbHVzdGVyX3NlcnZpY2UiOmZhbHNlLCJvcGVuaWQiOnRydWUsImVudGVycHJpc2VfcGx1Z2lucyI6dHJ1ZSwiYWR2YW5jZWRfbG9nZ2luZyI6dHJ1ZSwiZnV0dXJlX2ZlYXR1cmVzIjpmYWxzZX0sImlzX3RyaWFsIjp0cnVlLCJpc19nb3Zfc2t1IjpmYWxzZX0bEOVk2GdE1kSWKJ3dENWnkj0htY6QyXTtNA5hqnQ71Uc6teqXc7htHAxrnT/hV42xu+G24OMrAIsQtX4NjFSX6jvehIMRL5II3RPXYhHKUd2wruQ5ITEh1htFb5DgOJW3tvBdMmXt09nXjLRS1UYJ7ZsX3mU0uQndt7qfMriGAkk71veYuUJgztB3MsV7lRWB+8ZTp6WJ7RH+uWnuDspiA8B85mLnyuoCDokYksF2uIb+CtPGBTUB6qSOgxBBJxu5qftQXISCDAWY4O8lCrN3p5HCA/zf/rSRRNtet06QFobbjUDI4B7ZEAescKBKoHpP6nZPhg4KmhnkUi/o04ox") + +func TestSetLicenseOnStart(t *testing.T) { + oldValue := model.BuildEnterpriseReady + defer func() { model.BuildEnterpriseReady = oldValue }() + model.BuildEnterpriseReady = "true" + + cfg := model.Config{} + cfg.SetDefaults() + + f, err := os.CreateTemp("", "TestSetLicenseOnStart") + require.NoError(t, err) + defer os.Remove(f.Name()) + require.NoError(t, os.WriteFile(f.Name(), validTestLicense, 0777)) + + *cfg.ServiceSettings.LicenseFileLocation = f.Name() + + driverName := os.Getenv("MM_SQLSETTINGS_DRIVERNAME") + if driverName == "" { + driverName = model.DatabaseDriverPostgres + } + cfg.SqlSettings = *storetest.MakeSqlSettings(driverName, false) + + configStore := config.NewTestMemoryStore() + _, _, err = configStore.Set(&cfg) + require.NoError(t, err) + // It should not panic when ps.LoadLicense gets called from platform.New + _, err = New( + ServiceConfig{}, + ConfigStore(configStore), + ) + require.NoError(t, err) +} + func TestReadReplicaDisabledBasedOnLicense(t *testing.T) { cfg := model.Config{} cfg.SetDefaults()