From ab9e0c946d3d846cf80e0a1d4f955ebfafbfeea6 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Mon, 29 Jan 2024 18:37:28 +0000 Subject: [PATCH 1/4] datasource id is required if multiple datasource is enabled and no default cluster supported Signed-off-by: Xinrui Bai --- config/opensearch_dashboards.yml | 2 + .../search/opensearch_search/decide_client.ts | 15 +-- .../opensearch_search_strategy.test.ts | 100 +++++++++++++++++- .../opensearch_search_strategy.ts | 17 ++- src/plugins/data_source/config.ts | 1 + src/plugins/data_source/server/plugin.ts | 2 + src/plugins/data_source/server/types.ts | 2 + 7 files changed, 127 insertions(+), 12 deletions(-) diff --git a/config/opensearch_dashboards.yml b/config/opensearch_dashboards.yml index 9c6c040433b4..3bf9f745bdf4 100644 --- a/config/opensearch_dashboards.yml +++ b/config/opensearch_dashboards.yml @@ -237,6 +237,8 @@ # Set the value of this setting to true to enable multiple data source feature. #data_source.enabled: false +# Set the value of this setting to true to enable default cluster in data source feature. +#data_source.defaultCluster: false # Set the value of these settings to customize crypto materials to encryption saved credentials # in data sources. #data_source.encryption.wrappingKeyName: 'changeme' diff --git a/src/plugins/data/server/search/opensearch_search/decide_client.ts b/src/plugins/data/server/search/opensearch_search/decide_client.ts index 2ff2339add44..032f671668a3 100644 --- a/src/plugins/data/server/search/opensearch_search/decide_client.ts +++ b/src/plugins/data/server/search/opensearch_search/decide_client.ts @@ -9,14 +9,17 @@ import { IOpenSearchSearchRequest } from '..'; export const decideClient = async ( context: RequestHandlerContext, request: IOpenSearchSearchRequest, + withDataSourceEnabled: boolean = false, withLongNumeralsSupport: boolean = false ): Promise => { - // if data source feature is disabled, return default opensearch client of current user - const client = + const defaultOpenSearchClient = withLongNumeralsSupport + ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport + : context.core.opensearch.client.asCurrentUser; + + const dataSourceClient = request.dataSourceId && context.dataSource ? await context.dataSource.opensearch.getClient(request.dataSourceId) - : withLongNumeralsSupport - ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport - : context.core.opensearch.client.asCurrentUser; - return client; + : defaultOpenSearchClient; + + return withDataSourceEnabled ? dataSourceClient : defaultOpenSearchClient; }; diff --git a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts index fe95e3d7d4eb..6bd4eea5d17a 100644 --- a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts +++ b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.test.ts @@ -31,11 +31,22 @@ import { RequestHandlerContext } from '../../../../../core/server'; import { pluginInitializerContextConfigMock } from '../../../../../core/server/mocks'; import { opensearchSearchStrategyProvider } from './opensearch_search_strategy'; +import { DataSourceError } from '../../../../data_source/server/lib/error'; +import { DataSourcePluginSetup } from '../../../../data_source/server'; +import { SearchUsage } from '../collectors'; describe('OpenSearch search strategy', () => { const mockLogger: any = { debug: () => {}, }; + const mockSearchUsage: SearchUsage = { + trackError(): Promise { + return Promise.resolve(undefined); + }, + trackSuccess(duration: number): Promise { + return Promise.resolve(undefined); + }, + }; const body = { body: { _shards: { @@ -129,8 +140,21 @@ describe('OpenSearch search strategy', () => { expect(response).toHaveProperty('rawResponse'); }); - it('dataSource enabled, send request with dataSourceId get data source client', async () => { - const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger); + it('dataSource enabled and default cluster disabled, send request with dataSourceId get data source client', async () => { + const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => true), + defaultClusterEnabled: jest.fn(() => false), + }; + + const opensearchSearch = await opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled + ); await opensearchSearch.search( (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, @@ -142,6 +166,35 @@ describe('OpenSearch search strategy', () => { expect(mockOpenSearchApiCaller).not.toBeCalled(); }); + it('dataSource enabled and default cluster disabled, send request with empty dataSourceId should throw exception', async () => { + const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => true), + defaultClusterEnabled: jest.fn(() => false), + }; + + try { + const opensearchSearch = opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled + ); + + await opensearchSearch.search( + (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, + { + dataSourceId: '', + } + ); + } catch (e) { + expect(e).toBeTruthy(); + expect(e).toBeInstanceOf(DataSourceError); + } + }); + it('dataSource disabled, send request with dataSourceId get default client', async () => { const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger); @@ -152,8 +205,47 @@ describe('OpenSearch search strategy', () => { expect(mockDataSourceApiCaller).not.toBeCalled(); }); - it('dataSource enabled, send request without dataSourceId get default client', async () => { - const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger); + it('dataSource enabled and default cluster enabled, send request with dataSourceId get datasource client', async () => { + const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => true), + defaultClusterEnabled: jest.fn(() => true), + }; + + const opensearchSearch = await opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled + ); + + await opensearchSearch.search( + (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, + { + dataSourceId, + } + ); + expect(mockDataSourceApiCaller).toBeCalled(); + expect(mockOpenSearchApiCaller).not.toBeCalled(); + }); + + it('dataSource enabled and default cluster enabled, send request without dataSourceId get default client', async () => { + const mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => true), + defaultClusterEnabled: jest.fn(() => true), + }; + + const opensearchSearch = await opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled + ); await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {}); expect(mockOpenSearchApiCaller).toBeCalled(); diff --git a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts index 5eb290517792..81244c0a4054 100644 --- a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts +++ b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts @@ -73,7 +73,20 @@ export const opensearchSearchStrategyProvider = ( }); try { - const client = await decideClient(context, request, withLongNumeralsSupport); + if ( + dataSource?.dataSourceEnabled() && + !dataSource?.defaultClusterEnabled() && + !request.dataSourceId + ) { + throw new Error(`Data source id is required when no openseach hosts config provided`); + } + + const client = await decideClient( + context, + request, + dataSource?.dataSourceEnabled(), + withLongNumeralsSupport + ); const promise = shimAbortSignal(client.search(params), options?.abortSignal); const { body: rawResponse } = (await promise) as ApiResponse>; @@ -92,7 +105,7 @@ export const opensearchSearchStrategyProvider = ( } catch (e) { if (usage) usage.trackError(); - if (dataSource && request.dataSourceId) { + if (dataSource) { throw dataSource.createDataSourceError(e); } throw e; diff --git a/src/plugins/data_source/config.ts b/src/plugins/data_source/config.ts index 09ce35978921..b197c4a126fa 100644 --- a/src/plugins/data_source/config.ts +++ b/src/plugins/data_source/config.ts @@ -13,6 +13,7 @@ const WRAPPING_KEY_SIZE: number = 32; export const configSchema = schema.object({ enabled: schema.boolean({ defaultValue: false }), + defaultCluster: schema.boolean({ defaultValue: false }), encryption: schema.object({ wrappingKeyName: schema.string({ minLength: KEY_NAME_MIN_LENGTH, diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts index 0f3c47be4b4c..eee9bc0b8e0e 100644 --- a/src/plugins/data_source/server/plugin.ts +++ b/src/plugins/data_source/server/plugin.ts @@ -111,6 +111,8 @@ export class DataSourcePlugin implements Plugin createDataSourceError(e), + dataSourceEnabled: () => config.enabled, + defaultClusterEnabled: () => config.defaultCluster, }; } diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts index 68a840ebbbcb..eed435b57301 100644 --- a/src/plugins/data_source/server/types.ts +++ b/src/plugins/data_source/server/types.ts @@ -53,6 +53,8 @@ declare module 'src/core/server' { export interface DataSourcePluginSetup { createDataSourceError: (err: any) => DataSourceError; + dataSourceEnabled: () => boolean; + defaultClusterEnabled: () => boolean; } // eslint-disable-next-line @typescript-eslint/no-empty-interface export interface DataSourcePluginStart {} From 89dc35cf68d43738552556ae3e78ba0f1c0f4f07 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Mon, 29 Jan 2024 19:22:00 +0000 Subject: [PATCH 2/4] Update changelog file Signed-off-by: Xinrui Bai --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1580a367da8a..c1db583b575d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Discover] Fix missing index pattern field from breaking Discover [#5626](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5626) - [BUG] Remove duplicate sample data as id 90943e30-9a47-11e8-b64d-95841ca0b247 ([5668](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5668)) - [BUG][Multiple Datasource] Fix datasource testing connection unexpectedly passed with wrong endpoint [#5663](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5663) +- [BUG][Multiple Datasource] Datasource id is required if multiple datasource is enabled and no default cluster supported [#5751](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5751) ### 🚞 Infrastructure From 42412137e01517069a0485bccb83e25a1bc5801a Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Thu, 1 Feb 2024 19:08:57 +0000 Subject: [PATCH 3/4] Update code format and resolve comments Signed-off-by: Xinrui Bai --- config/opensearch_dashboards.yml | 2 +- .../server/search/opensearch_search/decide_client.ts | 9 +++------ .../opensearch_search/opensearch_search_strategy.ts | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/config/opensearch_dashboards.yml b/config/opensearch_dashboards.yml index 3bf9f745bdf4..aa38a78eea85 100644 --- a/config/opensearch_dashboards.yml +++ b/config/opensearch_dashboards.yml @@ -238,7 +238,7 @@ # Set the value of this setting to true to enable multiple data source feature. #data_source.enabled: false # Set the value of this setting to true to enable default cluster in data source feature. -#data_source.defaultCluster: false +#data_source.defaultCluster: true # Set the value of these settings to customize crypto materials to encryption saved credentials # in data sources. #data_source.encryption.wrappingKeyName: 'changeme' diff --git a/src/plugins/data/server/search/opensearch_search/decide_client.ts b/src/plugins/data/server/search/opensearch_search/decide_client.ts index 032f671668a3..9e0306d19269 100644 --- a/src/plugins/data/server/search/opensearch_search/decide_client.ts +++ b/src/plugins/data/server/search/opensearch_search/decide_client.ts @@ -16,10 +16,7 @@ export const decideClient = async ( ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport : context.core.opensearch.client.asCurrentUser; - const dataSourceClient = - request.dataSourceId && context.dataSource - ? await context.dataSource.opensearch.getClient(request.dataSourceId) - : defaultOpenSearchClient; - - return withDataSourceEnabled ? dataSourceClient : defaultOpenSearchClient; + return withDataSourceEnabled && request.dataSourceId && context.dataSource + ? await context.dataSource.opensearch.getClient(request.dataSourceId) + : defaultOpenSearchClient; }; diff --git a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts index 81244c0a4054..00507faaf0f8 100644 --- a/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts +++ b/src/plugins/data/server/search/opensearch_search/opensearch_search_strategy.ts @@ -105,7 +105,7 @@ export const opensearchSearchStrategyProvider = ( } catch (e) { if (usage) usage.trackError(); - if (dataSource) { + if (dataSource?.dataSourceEnabled()) { throw dataSource.createDataSourceError(e); } throw e; From 2b003b41ec64e61e764a0b0d34fac589ed55be63 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Thu, 1 Feb 2024 19:27:11 +0000 Subject: [PATCH 4/4] Update feature flag description Signed-off-by: Xinrui Bai --- config/opensearch_dashboards.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/opensearch_dashboards.yml b/config/opensearch_dashboards.yml index aa38a78eea85..189344ff1481 100644 --- a/config/opensearch_dashboards.yml +++ b/config/opensearch_dashboards.yml @@ -237,7 +237,7 @@ # Set the value of this setting to true to enable multiple data source feature. #data_source.enabled: false -# Set the value of this setting to true to enable default cluster in data source feature. +# Set the value of this setting to false to disable default cluster in data source feature. #data_source.defaultCluster: true # Set the value of these settings to customize crypto materials to encryption saved credentials # in data sources.