From bbd40e105089f4efe75c7bd94ab9231dbbb93580 Mon Sep 17 00:00:00 2001 From: Xinrui Bai-amazon <139305463+xinruiba@users.noreply.github.com> Date: Mon, 5 Feb 2024 17:34:58 -0800 Subject: [PATCH] Datasource id is required if multiple datasource is enabled and no default cluster supported (#5751) * datasource id is required if multiple datasource is enabled and no default cluster supported Signed-off-by: Xinrui Bai --------- Signed-off-by: Xinrui Bai --- CHANGELOG.md | 1 + config/opensearch_dashboards.yml | 2 + .../search/opensearch_search/decide_client.ts | 16 +-- .../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 + 8 files changed, 127 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11401714b622..eecee0c09503 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,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 diff --git a/config/opensearch_dashboards.yml b/config/opensearch_dashboards.yml index 9c6c040433b4..189344ff1481 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 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. #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..9e0306d19269 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,14 @@ 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 = - request.dataSourceId && context.dataSource - ? await context.dataSource.opensearch.getClient(request.dataSourceId) - : withLongNumeralsSupport - ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport - : context.core.opensearch.client.asCurrentUser; - return client; + const defaultOpenSearchClient = withLongNumeralsSupport + ? context.core.opensearch.client.asCurrentUserWithLongNumeralsSupport + : context.core.opensearch.client.asCurrentUser; + + 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.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..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 @@ -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?.dataSourceEnabled()) { 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 {}