From e54eebfa9b45a63cb8f5254aa9189e70e71d4151 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 11 Apr 2024 04:19:59 +0000 Subject: [PATCH] Improve dynamic configurations by adding cache and simplifying client fetch (#6364) * Improve dynamic config Signed-off-by: Tianle Huang * reset yml Signed-off-by: Tianle Huang * bring back previous changes Signed-off-by: Tianle Huang --------- Signed-off-by: Tianle Huang (cherry picked from commit cd857cb42e507e210f5b2ae2672bf251285e196f) Signed-off-by: github-actions[bot] # Conflicts: # CHANGELOG.md --- .../server/opensearch_config_client.test.ts | 106 ++++++++++++++--- .../server/opensearch_config_client.ts | 25 +++- .../application_config/server/plugin.test.ts | 108 +++++++++++++++++- .../application_config/server/plugin.ts | 22 +++- .../application_config/server/types.ts | 4 +- .../csp_handler/server/csp_handlers.test.ts | 7 ++ .../csp_handler/server/csp_handlers.ts | 6 +- 7 files changed, 248 insertions(+), 30 deletions(-) diff --git a/src/plugins/application_config/server/opensearch_config_client.test.ts b/src/plugins/application_config/server/opensearch_config_client.test.ts index 827d309303cb..17b22dad7295 100644 --- a/src/plugins/application_config/server/opensearch_config_client.test.ts +++ b/src/plugins/application_config/server/opensearch_config_client.test.ts @@ -48,7 +48,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.getConfig(); @@ -77,7 +79,10 @@ describe('OpenSearch Configuration Client', () => { }), }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.getConfig()).rejects.toThrowError(ERROR_MESSAGE); }); @@ -99,11 +104,45 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + has: jest.fn().mockReturnValue(false), + set: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.getEntityConfig('config1'); expect(value).toBe('value1'); + expect(cache.set).toBeCalledWith('config1', 'value1'); + }); + + it('return configuration value from cache', async () => { + const opensearchClient = { + asInternalUser: { + get: jest.fn().mockImplementation(() => { + return { + body: { + _source: { + value: 'value1', + }, + }, + }; + }), + }, + }; + + const cache = { + has: jest.fn().mockReturnValue(true), + get: jest.fn().mockReturnValue('cachedValue'), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); + + const value = await client.getEntityConfig('config1'); + + expect(value).toBe('cachedValue'); + expect(cache.get).toBeCalledWith('config1'); }); it('throws error when input is empty', async () => { @@ -121,7 +160,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.getEntityConfig(EMPTY_INPUT)).rejects.toThrowError( ERROR_MESSSAGE_FOR_EMPTY_INPUT @@ -151,9 +192,16 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + has: jest.fn().mockReturnValue(false), + set: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.getEntityConfig('config1')).rejects.toThrowError(ERROR_MESSAGE); + + expect(cache.set).toBeCalledWith('config1', undefined); }); }); @@ -167,11 +215,16 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + del: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.deleteEntityConfig('config1'); expect(value).toBe('config1'); + expect(cache.del).toBeCalledWith('config1'); }); it('throws error when input entity is empty', async () => { @@ -183,7 +236,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.deleteEntityConfig(EMPTY_INPUT)).rejects.toThrowError( ERROR_MESSSAGE_FOR_EMPTY_INPUT @@ -213,11 +268,16 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + del: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.deleteEntityConfig('config1'); expect(value).toBe('config1'); + expect(cache.del).toBeCalledWith('config1'); }); it('return deleted document entity when deletion fails due to document not found', async () => { @@ -241,11 +301,16 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + del: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.deleteEntityConfig('config1'); expect(value).toBe('config1'); + expect(cache.del).toBeCalledWith('config1'); }); it('throws error when opensearch throws error', async () => { @@ -271,7 +336,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.deleteEntityConfig('config1')).rejects.toThrowError(ERROR_MESSAGE); }); @@ -287,11 +354,16 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = { + set: jest.fn(), + }; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); const value = await client.updateEntityConfig('config1', 'newValue1'); expect(value).toBe('newValue1'); + expect(cache.set).toBeCalledWith('config1', 'newValue1'); }); it('throws error when entity is empty ', async () => { @@ -303,7 +375,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.updateEntityConfig(EMPTY_INPUT, 'newValue1')).rejects.toThrowError( ERROR_MESSSAGE_FOR_EMPTY_INPUT @@ -319,7 +393,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.updateEntityConfig('config1', EMPTY_INPUT)).rejects.toThrowError( ERROR_MESSSAGE_FOR_EMPTY_INPUT @@ -349,7 +425,9 @@ describe('OpenSearch Configuration Client', () => { }, }; - const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger); + const cache = {}; + + const client = new OpenSearchConfigurationClient(opensearchClient, INDEX_NAME, logger, cache); await expect(client.updateEntityConfig('config1', 'newValue1')).rejects.toThrowError( ERROR_MESSAGE diff --git a/src/plugins/application_config/server/opensearch_config_client.ts b/src/plugins/application_config/server/opensearch_config_client.ts index 9103919c396f..3a2c90147ade 100644 --- a/src/plugins/application_config/server/opensearch_config_client.ts +++ b/src/plugins/application_config/server/opensearch_config_client.ts @@ -3,8 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import LRUCache from 'lru-cache'; import { IScopedClusterClient, Logger } from '../../../../src/core/server'; - import { ConfigurationClient } from './types'; import { validate } from './string_utils'; @@ -12,32 +12,45 @@ export class OpenSearchConfigurationClient implements ConfigurationClient { private client: IScopedClusterClient; private configurationIndexName: string; private readonly logger: Logger; + private cache: LRUCache; constructor( scopedClusterClient: IScopedClusterClient, configurationIndexName: string, - logger: Logger + logger: Logger, + cache: LRUCache ) { this.client = scopedClusterClient; this.configurationIndexName = configurationIndexName; this.logger = logger; + this.cache = cache; } async getEntityConfig(entity: string) { const entityValidated = validate(entity, this.logger); + if (this.cache.has(entityValidated)) { + return this.cache.get(entityValidated); + } + + this.logger.info(`Key ${entityValidated} is not found from cache.`); + try { const data = await this.client.asInternalUser.get({ index: this.configurationIndexName, id: entityValidated, }); + const value = data?.body?._source?.value; - return data?.body?._source?.value || ''; + this.cache.set(entityValidated, value); + + return value; } catch (e) { const errorMessage = `Failed to get entity ${entityValidated} due to error ${e}`; this.logger.error(errorMessage); + this.cache.set(entityValidated, undefined); throw e; } } @@ -55,6 +68,8 @@ export class OpenSearchConfigurationClient implements ConfigurationClient { }, }); + this.cache.set(entityValidated, newValueValidated); + return newValueValidated; } catch (e) { const errorMessage = `Failed to update entity ${entityValidated} with newValue ${newValueValidated} due to error ${e}`; @@ -74,15 +89,19 @@ export class OpenSearchConfigurationClient implements ConfigurationClient { id: entityValidated, }); + this.cache.del(entityValidated); + return entityValidated; } catch (e) { if (e?.body?.error?.type === 'index_not_found_exception') { this.logger.info('Attemp to delete a not found index.'); + this.cache.del(entityValidated); return entityValidated; } if (e?.body?.result === 'not_found') { this.logger.info('Attemp to delete a not found document.'); + this.cache.del(entityValidated); return entityValidated; } diff --git a/src/plugins/application_config/server/plugin.test.ts b/src/plugins/application_config/server/plugin.test.ts index e1ac45444c14..5390223f4d87 100644 --- a/src/plugins/application_config/server/plugin.test.ts +++ b/src/plugins/application_config/server/plugin.test.ts @@ -6,6 +6,11 @@ import { of } from 'rxjs'; import { ApplicationConfigPlugin } from './plugin'; import { ConfigurationClient } from './types'; +import LRUCache from 'lru-cache'; +import { OpenSearchConfigurationClient } from './opensearch_config_client'; + +jest.mock('lru-cache'); +jest.mock('./opensearch_config_client'); describe('application config plugin', () => { it('throws error when trying to register twice', async () => { @@ -54,8 +59,8 @@ describe('application config plugin', () => { setup.registerConfigurationClient(client1); - const scopedClient = {}; - expect(setup.getConfigurationClient(scopedClient)).toBe(client1); + const request = {}; + expect(setup.getConfigurationClient(request)).toBe(client1); const client2: ConfigurationClient = { getConfig: jest.fn(), @@ -71,6 +76,103 @@ describe('application config plugin', () => { 'Configuration client is already registered! Cannot register again!' ); - expect(setup.getConfigurationClient(scopedClient)).toBe(client1); + expect(setup.getConfigurationClient(request)).toBe(client1); + }); + + it('getConfigurationClient returns opensearch client when no external registration', async () => { + let capturedLRUCacheConstructorArgs = []; + + const cache = { + get: jest.fn(), + }; + + LRUCache.mockImplementation(function (...args) { + capturedLRUCacheConstructorArgs = args; + return cache; + }); + + let capturedConfigurationClientConstructorArgs = []; + + const client: ConfigurationClient = { + getConfig: jest.fn(), + getEntityConfig: jest.fn(), + updateEntityConfig: jest.fn(), + deleteEntityConfig: jest.fn(), + }; + + OpenSearchConfigurationClient.mockImplementation(function (...args) { + capturedConfigurationClientConstructorArgs = args; + return client; + }); + + const logger = { + info: jest.fn(), + error: jest.fn(), + }; + + const initializerContext = { + logger: { + get: jest.fn().mockReturnValue(logger), + }, + config: { + legacy: { + globalConfig$: of({ + opensearchDashboards: { + configIndex: '.osd_test', + }, + }), + }, + }, + }; + + const plugin = new ApplicationConfigPlugin(initializerContext); + + const coreSetup = { + http: { + createRouter: jest.fn().mockImplementation(() => { + return { + get: jest.fn(), + post: jest.fn(), + delete: jest.fn(), + }; + }), + }, + }; + + const setup = await plugin.setup(coreSetup); + + const scopedClient = { + asCurrentUser: jest.fn(), + }; + + const coreStart = { + opensearch: { + client: { + asScoped: jest.fn().mockReturnValue(scopedClient), + }, + }, + }; + + await plugin.start(coreStart); + + const request = {}; + + expect(setup.getConfigurationClient(request)).toBe(client); + + expect(capturedLRUCacheConstructorArgs).toEqual([ + { + max: 100, + maxAge: 600000, + }, + ]); + + expect(capturedConfigurationClientConstructorArgs).toEqual([ + scopedClient, + '.osd_test', + logger, + cache, + ]); + + expect(coreStart.opensearch.client.asScoped).toBeCalledTimes(1); }); }); diff --git a/src/plugins/application_config/server/plugin.ts b/src/plugins/application_config/server/plugin.ts index d0bd2ab42270..8536f7e134e3 100644 --- a/src/plugins/application_config/server/plugin.ts +++ b/src/plugins/application_config/server/plugin.ts @@ -6,14 +6,16 @@ import { Observable } from 'rxjs'; import { first } from 'rxjs/operators'; +import LRUCache from 'lru-cache'; import { PluginInitializerContext, CoreSetup, CoreStart, Plugin, Logger, - IScopedClusterClient, SharedGlobalConfig, + OpenSearchDashboardsRequest, + IClusterClient, } from '../../../core/server'; import { @@ -31,11 +33,20 @@ export class ApplicationConfigPlugin private configurationClient: ConfigurationClient; private configurationIndexName: string; + private clusterClient: IClusterClient; + + private cache: LRUCache; constructor(initializerContext: PluginInitializerContext) { this.logger = initializerContext.logger.get(); this.config$ = initializerContext.config.legacy.globalConfig$; this.configurationIndexName = ''; + this.clusterClient = null; + + this.cache = new LRUCache({ + max: 100, // at most 100 entries + maxAge: 10 * 60 * 1000, // 10 mins + }); } private registerConfigurationClient(configurationClient: ConfigurationClient) { @@ -50,15 +61,16 @@ export class ApplicationConfigPlugin this.configurationClient = configurationClient; } - private getConfigurationClient(scopedClusterClient: IScopedClusterClient): ConfigurationClient { + private getConfigurationClient(request?: OpenSearchDashboardsRequest): ConfigurationClient { if (this.configurationClient) { return this.configurationClient; } const openSearchConfigurationClient = new OpenSearchConfigurationClient( - scopedClusterClient, + this.clusterClient.asScoped(request), this.configurationIndexName, - this.logger + this.logger, + this.cache ); return openSearchConfigurationClient; @@ -81,6 +93,8 @@ export class ApplicationConfigPlugin } public start(core: CoreStart) { + this.clusterClient = core.opensearch.client; + return {}; } diff --git a/src/plugins/application_config/server/types.ts b/src/plugins/application_config/server/types.ts index 416d0258169e..c8039cf6cff3 100644 --- a/src/plugins/application_config/server/types.ts +++ b/src/plugins/application_config/server/types.ts @@ -3,10 +3,10 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { IScopedClusterClient, Headers } from 'src/core/server'; +import { Headers, OpenSearchDashboardsRequest } from 'src/core/server'; export interface ApplicationConfigPluginSetup { - getConfigurationClient: (inputOpenSearchClient: IScopedClusterClient) => ConfigurationClient; + getConfigurationClient: (request?: OpenSearchDashboardsRequest) => ConfigurationClient; registerConfigurationClient: (inputConfigurationClient: ConfigurationClient) => void; } // eslint-disable-next-line @typescript-eslint/no-empty-interface diff --git a/src/plugins/csp_handler/server/csp_handlers.test.ts b/src/plugins/csp_handler/server/csp_handlers.test.ts index d6c2f8a16d49..b185410f6174 100644 --- a/src/plugins/csp_handler/server/csp_handlers.test.ts +++ b/src/plugins/csp_handler/server/csp_handlers.test.ts @@ -55,6 +55,7 @@ describe('CSP handlers', () => { }); expect(configurationClient.getEntityConfig).toBeCalledTimes(1); + expect(getConfigurationClient).toBeCalledWith(request); }); it('do not add CSP headers when the client returns empty and CSP from YML already has frame-ancestors', async () => { @@ -89,6 +90,7 @@ describe('CSP handlers', () => { expect(toolkit.next).toHaveBeenCalledWith({}); expect(configurationClient.getEntityConfig).toBeCalledTimes(1); + expect(getConfigurationClient).toBeCalledWith(request); }); it('add frame-ancestors CSP headers when the client returns empty and CSP from YML has no frame-ancestors', async () => { @@ -128,6 +130,7 @@ describe('CSP handlers', () => { }); expect(configurationClient.getEntityConfig).toBeCalledTimes(1); + expect(getConfigurationClient).toBeCalledWith(request); }); it('do not add CSP headers when the configuration does not exist and CSP from YML already has frame-ancestors', async () => { @@ -164,6 +167,7 @@ describe('CSP handlers', () => { expect(toolkit.next).toBeCalledWith({}); expect(configurationClient.getEntityConfig).toBeCalledTimes(1); + expect(getConfigurationClient).toBeCalledWith(request); }); it('add frame-ancestors CSP headers when the configuration does not exist and CSP from YML has no frame-ancestors', async () => { @@ -200,6 +204,7 @@ describe('CSP handlers', () => { }); expect(configurationClient.getEntityConfig).toBeCalledTimes(1); + expect(getConfigurationClient).toBeCalledWith(request); }); it('do not add CSP headers when request dest exists and shall skip', async () => { @@ -235,6 +240,7 @@ describe('CSP handlers', () => { expect(toolkit.next).toBeCalledWith({}); expect(configurationClient.getEntityConfig).toBeCalledTimes(0); + expect(getConfigurationClient).toBeCalledTimes(0); }); it('do not add CSP headers when request dest does not exist', async () => { @@ -269,5 +275,6 @@ describe('CSP handlers', () => { expect(toolkit.next).toBeCalledWith({}); expect(configurationClient.getEntityConfig).toBeCalledTimes(0); + expect(getConfigurationClient).toBeCalledTimes(0); }); }); diff --git a/src/plugins/csp_handler/server/csp_handlers.ts b/src/plugins/csp_handler/server/csp_handlers.ts index 3bfa90115518..1a76ed942460 100644 --- a/src/plugins/csp_handler/server/csp_handlers.ts +++ b/src/plugins/csp_handler/server/csp_handlers.ts @@ -30,7 +30,7 @@ const CSP_RULES_CONFIG_KEY = 'csp.rules'; export function createCspRulesPreResponseHandler( core: CoreSetup, cspHeader: string, - getConfigurationClient: (scopedClusterClient: IScopedClusterClient) => ConfigurationClient, + getConfigurationClient: (request?: OpenSearchDashboardsRequest) => ConfigurationClient, logger: Logger ): OnPreResponseHandler { return async ( @@ -47,9 +47,7 @@ export function createCspRulesPreResponseHandler( return toolkit.next({}); } - const [coreStart] = await core.getStartServices(); - - const client = getConfigurationClient(coreStart.opensearch.client.asScoped(request)); + const client = getConfigurationClient(request); const cspRules = await client.getEntityConfig(CSP_RULES_CONFIG_KEY, { headers: request.headers,