From 7d77b9eaeb3fe6ec8535182d63013328871f9ac6 Mon Sep 17 00:00:00 2001 From: Xinrui Bai-amazon <139305463+xinruiba@users.noreply.github.com> Date: Tue, 27 Feb 2024 10:58:00 -0800 Subject: [PATCH] [MD] Improved error handling for the search API when a null value is passed for the dataSourceId. (#5882) * [MD] Using legacy client cofig instead of globle config Signed-off-by: Xinrui Bai * [UT] add test cases to test different hostconfig and datasourceId combinations of scenario Signed-off-by: Xinrui Bai * Update changelog file Signed-off-by: Xinrui Bai * Resolve comments Signed-off-by: Xinrui Bai * [UT] update test cases and resolve comments Signed-off-by: Xinrui Bai * [UT] Update unit test Signed-off-by: Xinrui Bai --------- Signed-off-by: Xinrui Bai --- CHANGELOG.md | 1 + .../search/opensearch_search/decide_client.ts | 15 +- .../opensearch_search_strategy.test.ts | 177 +++++++++++++++++- .../opensearch_search_strategy.ts | 12 +- .../data/server/search/search_service.ts | 4 +- src/plugins/data_source/server/plugin.ts | 1 + src/plugins/data_source/server/types.ts | 1 + 7 files changed, 191 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b47a399d60c..ceeb039f981f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Multiple Datasource] Add api registry and allow it to be added into client config in data source plugin ([#5895](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5895)) - [Multiple Datasource] Concatenate data source name with index pattern name and change delimiter to double colon ([#5907](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5907)) - [Multiple Datasource] Refactor client and legacy client to use authentication registry ([#5881](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5881)) +- [Multiple Datasource] Improved error handling for the search API when a null value is passed for the dataSourceId ([#5882](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5882)) ### 🐛 Bug Fixes 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..41e0d5c16277 100644 --- a/src/plugins/data/server/search/opensearch_search/decide_client.ts +++ b/src/plugins/data/server/search/opensearch_search/decide_client.ts @@ -11,12 +11,11 @@ export const decideClient = async ( request: IOpenSearchSearchRequest, 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 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 39c367a04a41..ae6e1746dab6 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 @@ -29,15 +29,47 @@ */ import { RequestHandlerContext } from '../../../../../core/server'; -import { pluginInitializerContextConfigMock } from '../../../../../core/server/mocks'; +import { + opensearchServiceMock, + 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 mockDataSourcePluginSetupWithDataSourceEnabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => true), + registerCredentialProvider: jest.fn(), + registerCustomApiSchema(schema: any): void { + throw new Error('Function not implemented.'); + }, + }; + const mockDataSourcePluginSetupWithDataSourceDisabled: DataSourcePluginSetup = { + createDataSourceError(err: any): DataSourceError { + return new DataSourceError({}); + }, + dataSourceEnabled: jest.fn(() => false), + registerCredentialProvider: jest.fn(), + registerCustomApiSchema(schema: any): void { + throw new Error('Function not implemented.'); + }, + }; const body = { body: { _shards: { @@ -50,6 +82,7 @@ describe('OpenSearch search strategy', () => { }; const mockOpenSearchApiCaller = jest.fn().mockResolvedValue(body); const mockDataSourceApiCaller = jest.fn().mockResolvedValue(body); + const mockOpenSearchApiCallerWithLongNumeralsSupport = jest.fn().mockResolvedValue(body); const dataSourceId = 'test-data-source-id'; const mockDataSourceContext = { dataSource: { @@ -67,7 +100,14 @@ describe('OpenSearch search strategy', () => { get: () => {}, }, }, - opensearch: { client: { asCurrentUser: { search: mockOpenSearchApiCaller } } }, + opensearch: { + client: { + asCurrentUser: { search: mockOpenSearchApiCaller }, + asCurrentUserWithLongNumeralsSupport: { + search: mockOpenSearchApiCallerWithLongNumeralsSupport, + }, + }, + }, }, }; const mockDataSourceEnabledContext = { @@ -131,8 +171,23 @@ 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, config host exist, send request with dataSourceId should get data source client', async () => { + const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup(); + mockOpenSearchServiceSetup.legacy.client = { + callAsInternalUser: jest.fn(), + asScoped: jest.fn(), + config: { + hosts: ['some host'], + }, + }; + + const opensearchSearch = opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled, + mockOpenSearchServiceSetup + ); await opensearchSearch.search( (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, @@ -140,11 +195,86 @@ describe('OpenSearch search strategy', () => { dataSourceId, } ); + expect(mockDataSourceApiCaller).toBeCalled(); expect(mockOpenSearchApiCaller).not.toBeCalled(); }); - it('dataSource disabled, send request with dataSourceId get default client', async () => { + it('dataSource enabled, config host exist, send request without dataSourceId should get default client', async () => { + const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup(); + mockOpenSearchServiceSetup.legacy.client = { + callAsInternalUser: jest.fn(), + asScoped: jest.fn(), + config: { + hosts: ['some host'], + }, + }; + + const opensearchSearch = opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled, + mockOpenSearchServiceSetup + ); + + const dataSourceIdToBeTested = [undefined, '']; + + dataSourceIdToBeTested.forEach(async (id) => { + const testRequest = id === undefined ? {} : { dataSourceId: id }; + + await opensearchSearch.search( + (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, + testRequest + ); + expect(mockOpenSearchApiCaller).toBeCalled(); + expect(mockDataSourceApiCaller).not.toBeCalled(); + }); + }); + + it('dataSource enabled, config host is empty / undefined, send request with / without dataSourceId should both throw DataSourceError exception', async () => { + const hostsTobeTested = [undefined, []]; + const dataSourceIdToBeTested = [undefined, '', dataSourceId]; + + hostsTobeTested.forEach((host) => { + const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup(); + + if (host !== undefined) { + mockOpenSearchServiceSetup.legacy.client = { + callAsInternalUser: jest.fn(), + asScoped: jest.fn(), + config: { + hosts: [], + }, + }; + } + + dataSourceIdToBeTested.forEach(async (id) => { + const testRequest = id === undefined ? {} : { dataSourceId: id }; + + try { + const opensearchSearch = opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceEnabled, + mockOpenSearchServiceSetup + ); + + await opensearchSearch.search( + (mockDataSourceEnabledContext as unknown) as RequestHandlerContext, + testRequest + ); + } catch (e) { + expect(e).toBeTruthy(); + expect(e).toBeInstanceOf(DataSourceError); + expect(e.statusCode).toEqual(400); + } + }); + }); + }); + + it('dataSource disabled, send request with dataSourceId should get default client', async () => { const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger); await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, { @@ -154,11 +284,40 @@ describe('OpenSearch search strategy', () => { expect(mockDataSourceApiCaller).not.toBeCalled(); }); - it('dataSource enabled, send request without dataSourceId get default client', async () => { + it('dataSource disabled, send request without dataSourceId should get default client', async () => { const opensearchSearch = await opensearchSearchStrategyProvider(mockConfig$, mockLogger); - await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, {}); - expect(mockOpenSearchApiCaller).toBeCalled(); - expect(mockDataSourceApiCaller).not.toBeCalled(); + const dataSourceIdToBeTested = [undefined, '']; + + for (const testDataSourceId of dataSourceIdToBeTested) { + await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, { + dataSourceId: testDataSourceId, + }); + expect(mockOpenSearchApiCaller).toBeCalled(); + expect(mockDataSourceApiCaller).not.toBeCalled(); + } + }); + + it('dataSource disabled and longNumeralsSupported, send request without dataSourceId should get longNumeralsSupport client', async () => { + const mockOpenSearchServiceSetup = opensearchServiceMock.createSetup(); + const opensearchSearch = await opensearchSearchStrategyProvider( + mockConfig$, + mockLogger, + mockSearchUsage, + mockDataSourcePluginSetupWithDataSourceDisabled, + mockOpenSearchServiceSetup, + true + ); + + const dataSourceIdToBeTested = [undefined, '']; + + for (const testDataSourceId of dataSourceIdToBeTested) { + await opensearchSearch.search((mockContext as unknown) as RequestHandlerContext, { + dataSourceId: testDataSourceId, + }); + expect(mockOpenSearchApiCallerWithLongNumeralsSupport).toBeCalled(); + expect(mockOpenSearchApiCaller).not.toBeCalled(); + expect(mockDataSourceApiCaller).not.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..fa1b3e4da94c 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 @@ -29,7 +29,7 @@ */ import { first } from 'rxjs/operators'; -import { SharedGlobalConfig, Logger } from 'opensearch-dashboards/server'; +import { SharedGlobalConfig, Logger, OpenSearchServiceSetup } from 'opensearch-dashboards/server'; import { SearchResponse } from 'elasticsearch'; import { Observable } from 'rxjs'; import { ApiResponse } from '@opensearch-project/opensearch'; @@ -50,6 +50,7 @@ export const opensearchSearchStrategyProvider = ( logger: Logger, usage?: SearchUsage, dataSource?: DataSourcePluginSetup, + openSearchServiceSetup?: OpenSearchServiceSetup, withLongNumeralsSupport?: boolean ): ISearchStrategy => { return { @@ -73,6 +74,13 @@ export const opensearchSearchStrategyProvider = ( }); try { + const isOpenSearchHostsEmpty = + openSearchServiceSetup?.legacy?.client?.config?.hosts?.length === 0; + + if (dataSource?.dataSourceEnabled() && isOpenSearchHostsEmpty && !request.dataSourceId) { + throw new Error(`Data source id is required when no openseach hosts config provided`); + } + const client = await decideClient(context, request, withLongNumeralsSupport); const promise = shimAbortSignal(client.search(params), options?.abortSignal); @@ -92,7 +100,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/server/search/search_service.ts b/src/plugins/data/server/search/search_service.ts index feb1a3157794..b955596922a0 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -130,7 +130,8 @@ export class SearchService implements Plugin { this.initializerContext.config.legacy.globalConfig$, this.logger, usage, - dataSource + dataSource, + core.opensearch ) ); @@ -141,6 +142,7 @@ export class SearchService implements Plugin { this.logger, usage, dataSource, + core.opensearch, true ) ); diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts index 77ca0dd7b8a7..56b5f5caf2e8 100644 --- a/src/plugins/data_source/server/plugin.ts +++ b/src/plugins/data_source/server/plugin.ts @@ -145,6 +145,7 @@ export class DataSourcePlugin implements Plugin createDataSourceError(e), registerCredentialProvider, registerCustomApiSchema: (schema: any) => this.customApiSchemaRegistry.register(schema), + dataSourceEnabled: () => config.enabled, }; } diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts index ede0194ed3ef..12b975881e7e 100644 --- a/src/plugins/data_source/server/types.ts +++ b/src/plugins/data_source/server/types.ts @@ -85,6 +85,7 @@ export interface DataSourcePluginSetup { createDataSourceError: (err: any) => DataSourceError; registerCredentialProvider: (method: AuthenticationMethod) => void; registerCustomApiSchema: (schema: any) => void; + dataSourceEnabled: () => boolean; } export interface DataSourcePluginStart {