From fbe59a50b5f031e56b797ca153bd301ad3dd7c2f Mon Sep 17 00:00:00 2001 From: Zhongnan Su Date: Wed, 26 Oct 2022 22:41:29 -0700 Subject: [PATCH] [MD] Refactor data source server side error handling (#2661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename DataSourceConfigError to DataSourceError - Add helper createDataSourceError() to create data source error from， and replace usage of new DataSourceError(e) with createDataSourceError(e) - import createDataSourceError() from data source plugin setup interface, refactor to move data source error response logic from router.ts to resolve_index.ts. Signed-off-by: Su Signed-off-by: Ajay Gupta --- CHANGELOG.md | 1 + src/core/server/http/router/router.ts | 10 -- src/plugins/data/server/plugin.ts | 5 +- .../opensearch_search_strategy.ts | 8 +- .../data/server/search/search_service.ts | 7 +- .../server/client/configure_client.ts | 10 +- .../server/legacy/configure_legacy_client.ts | 7 +- .../data_source/server/lib/error.test.ts | 108 +++++++++++++++--- src/plugins/data_source/server/lib/error.ts | 48 ++++++-- src/plugins/data_source/server/plugin.ts | 5 +- src/plugins/data_source/server/types.ts | 6 +- .../server/routes/resolve_index.ts | 26 +++-- 12 files changed, 184 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11f4301d9409..f27ee2166433 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Adding @zhongnansu as maintainer. ([#2590](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2590)) ### 🪛 Refactoring +* [MD] Refactor data source error handling ([#2661](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2661)) ### 🔩 Tests diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index 047639372454..c99134e5ce8f 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -301,21 +301,11 @@ export class Router implements IRouter { return e; } - if (isDataSourceConfigError(e)) { - return hapiResponseAdapter.handle( - opensearchDashboardsResponseFactory.badRequest({ body: e.message }) - ); - } - return hapiResponseAdapter.toInternalError(); } } } -const isDataSourceConfigError = (error: any) => { - return error.constructor.name === 'DataSourceConfigError' && error.statusCode === 400; -}; - const convertOpenSearchUnauthorized = ( e: OpenSearchNotAuthorizedError ): ErrorHttpResponseOptions => { diff --git a/src/plugins/data/server/plugin.ts b/src/plugins/data/server/plugin.ts index f8e3bb5bacdb..f9159b3246ff 100644 --- a/src/plugins/data/server/plugin.ts +++ b/src/plugins/data/server/plugin.ts @@ -30,6 +30,7 @@ import { PluginInitializerContext, CoreSetup, CoreStart, Plugin, Logger } from 'src/core/server'; import { ExpressionsServerSetup } from 'src/plugins/expressions/server'; +import { DataSourcePluginSetup } from 'src/plugins/data_source/server'; import { ConfigSchema } from '../config'; import { IndexPatternsService, IndexPatternsServiceStart } from './index_patterns'; import { ISearchSetup, ISearchStart, SearchEnhancements } from './search'; @@ -64,6 +65,7 @@ export interface DataPluginStart { export interface DataPluginSetupDependencies { expressions: ExpressionsServerSetup; usageCollection?: UsageCollectionSetup; + dataSource?: DataSourcePluginSetup; } // eslint-disable-next-line @typescript-eslint/no-empty-interface @@ -96,7 +98,7 @@ export class DataServerPlugin public setup( core: CoreSetup, - { expressions, usageCollection }: DataPluginSetupDependencies + { expressions, usageCollection, dataSource }: DataPluginSetupDependencies ) { this.indexPatterns.setup(core); this.scriptsService.setup(core); @@ -109,6 +111,7 @@ export class DataServerPlugin const searchSetup = this.searchService.setup(core, { registerFunction: expressions.registerFunction, usageCollection, + dataSource, }); return { 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 4ccc8db05728..ba50740e6baf 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 @@ -33,6 +33,7 @@ import { SharedGlobalConfig, Logger } from 'opensearch-dashboards/server'; import { SearchResponse } from 'elasticsearch'; import { Observable } from 'rxjs'; import { ApiResponse } from '@opensearch-project/opensearch'; +import { DataSourcePluginSetup } from 'src/plugins/data_source/server'; import { SearchUsage } from '../collectors/usage'; import { toSnakeCase } from './to_snake_case'; import { @@ -47,7 +48,8 @@ import { decideClient } from './decide_client'; export const opensearchSearchStrategyProvider = ( config$: Observable, logger: Logger, - usage?: SearchUsage + usage?: SearchUsage, + dataSource?: DataSourcePluginSetup ): ISearchStrategy => { return { search: async (context, request, options) => { @@ -88,6 +90,10 @@ export const opensearchSearchStrategyProvider = ( }; } catch (e) { if (usage) usage.trackError(); + + if (dataSource && request.dataSourceId) { + 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 42de7fd023ed..0c33b95f4606 100644 --- a/src/plugins/data/server/search/search_service.ts +++ b/src/plugins/data/server/search/search_service.ts @@ -42,6 +42,7 @@ import { StartServicesAccessor, } from 'src/core/server'; import { first } from 'rxjs/operators'; +import { DataSourcePluginSetup } from 'src/plugins/data_source/server'; import { ISearchSetup, ISearchStart, ISearchStrategy, SearchEnhancements } from './types'; import { AggsService, AggsSetupDependencies } from './aggs'; @@ -78,6 +79,7 @@ type StrategyMap = Record>; export interface SearchServiceSetupDependencies { registerFunction: AggsSetupDependencies['registerFunction']; usageCollection?: UsageCollectionSetup; + dataSource?: DataSourcePluginSetup; } /** @internal */ @@ -105,7 +107,7 @@ export class SearchService implements Plugin { public setup( core: CoreSetup<{}, DataPluginStart>, - { registerFunction, usageCollection }: SearchServiceSetupDependencies + { registerFunction, usageCollection, dataSource }: SearchServiceSetupDependencies ): ISearchSetup { const usage = usageCollection ? usageProvider(core, this.initializerContext) : undefined; @@ -122,7 +124,8 @@ export class SearchService implements Plugin { opensearchSearchStrategyProvider( this.initializerContext.config.legacy.globalConfig$, this.logger, - usage + usage, + dataSource ) ); diff --git a/src/plugins/data_source/server/client/configure_client.ts b/src/plugins/data_source/server/client/configure_client.ts index 0e153f89b0f9..c4d1b4eef9f7 100644 --- a/src/plugins/data_source/server/client/configure_client.ts +++ b/src/plugins/data_source/server/client/configure_client.ts @@ -13,7 +13,7 @@ import { } from '../../common/data_sources'; import { DataSourcePluginConfigType } from '../../config'; import { CryptographyServiceSetup } from '../cryptography_service'; -import { DataSourceConfigError } from '../lib/error'; +import { createDataSourceError, DataSourceError } from '../lib/error'; import { DataSourceClientParams } from '../types'; import { parseClientOptions } from './client_config'; import { OpenSearchClientPoolSetup } from './client_pool'; @@ -32,8 +32,8 @@ export const configureClient = async ( } catch (error: any) { logger.error(`Failed to get data source client for dataSourceId: [${dataSourceId}]`); logger.error(error); - // Re-throw as DataSourceConfigError - throw new DataSourceConfigError('Failed to get data source client: ', error); + // Re-throw as DataSourceError + throw createDataSourceError(error); } }; @@ -59,8 +59,8 @@ export const getCredential = async ( const { decryptedText, encryptionContext } = await cryptography .decodeAndDecrypt(password) .catch((err: any) => { - // Re-throw as DataSourceConfigError - throw new DataSourceConfigError('Unable to decrypt "auth.credentials.password".', err); + // Re-throw as DataSourceError + throw createDataSourceError(err); }); if (encryptionContext!.endpoint !== endpoint) { diff --git a/src/plugins/data_source/server/legacy/configure_legacy_client.ts b/src/plugins/data_source/server/legacy/configure_legacy_client.ts index 2921e0cad48e..94e6b40a4d2e 100644 --- a/src/plugins/data_source/server/legacy/configure_legacy_client.ts +++ b/src/plugins/data_source/server/legacy/configure_legacy_client.ts @@ -23,7 +23,7 @@ import { CryptographyServiceSetup } from '../cryptography_service'; import { DataSourceClientParams, LegacyClientCallAPIParams } from '../types'; import { OpenSearchClientPoolSetup, getCredential, getDataSource } from '../client'; import { parseClientOptions } from './client_config'; -import { DataSourceConfigError } from '../lib/error'; +import { createDataSourceError, DataSourceError } from '../lib/error'; export const configureLegacyClient = async ( { dataSourceId, savedObjects, cryptography }: DataSourceClientParams, @@ -40,8 +40,8 @@ export const configureLegacyClient = async ( } catch (error: any) { logger.error(`Failed to get data source client for dataSourceId: [${dataSourceId}]`); logger.error(error); - // Re-throw as DataSourceConfigError - throw new DataSourceConfigError('Failed to get data source client: ', error); + // Re-throw as DataSourceError + throw createDataSourceError(error); } }; @@ -140,7 +140,6 @@ const callAPI = async ( if (!options.wrap401Errors || err.statusCode !== 401) { throw err; } - throw LegacyOpenSearchErrorHelpers.decorateNotAuthorizedError(err); } }; diff --git a/src/plugins/data_source/server/lib/error.test.ts b/src/plugins/data_source/server/lib/error.test.ts index fca7efb0904c..2ead6fde7345 100644 --- a/src/plugins/data_source/server/lib/error.test.ts +++ b/src/plugins/data_source/server/lib/error.test.ts @@ -3,38 +3,118 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { ApiResponse } from '@opensearch-project/opensearch'; +import { + ConnectionError, + NoLivingConnectionsError, + ResponseError, +} from '@opensearch-project/opensearch/lib/errors'; import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; -import { DataSourceConfigError } from './error'; +import { createDataSourceError, DataSourceError } from './error'; +import { errors as LegacyErrors } from 'elasticsearch'; -describe('DataSourceConfigError', () => { - it('create from savedObject bad request error should be 400 error', () => { +const createApiResponseError = ({ + statusCode = 200, + headers = {}, + body = {}, +}: { + statusCode?: number; + headers?: Record; + body?: Record; +} = {}): ApiResponse => { + return { + body, + statusCode, + headers, + warnings: [], + meta: {} as any, + }; +}; + +describe('CreateDataSourceError', () => { + it('create from savedObject bad request error should generate 400 error', () => { const error = SavedObjectsErrorHelpers.createBadRequestError('test reason message'); - expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + expect(createDataSourceError(error)).toMatchObject({ statusCode: 400, - message: 'test prefix: test reason message: Bad Request', + message: 'Data Source Error: test reason message: Bad Request', }); }); - it('create from savedObject not found error should be 400 error', () => { - const error = SavedObjectsErrorHelpers.decorateNotAuthorizedError(new Error()); - expect(new DataSourceConfigError('test prefix: ', error)).toHaveProperty('statusCode', 400); + it('create from savedObject not found error should have statusCode 404', () => { + const error = SavedObjectsErrorHelpers.createGenericNotFoundError('type', 'id'); + expect(createDataSourceError(error)).toHaveProperty('statusCode', 404); }); - it('create from savedObject service unavailable error should be a 500 error', () => { + it('create from savedObject service unavailable error should have statusCode 503', () => { const error = SavedObjectsErrorHelpers.decorateOpenSearchUnavailableError( new Error('test reason message') ); - expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ - statusCode: 500, - message: 'test prefix: test reason message', + expect(createDataSourceError(error)).toMatchObject({ + statusCode: 503, + message: 'Data Source Error: test reason message', }); }); it('create from non savedObject error should always be a 400 error', () => { const error = new Error('test reason message'); - expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + expect(createDataSourceError(error)).toMatchObject({ statusCode: 400, - message: 'test prefix: test reason message', + message: 'Data Source Error: test reason message', }); }); + + it('create from client response error 401 should be casted to a 400 DataSourceError', () => { + expect( + createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 401 }))) + ).toHaveProperty('statusCode', 400); + }); + + it('create from non 401 client response error should respect original statusCode', () => { + expect( + createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 403 }))) + ).toHaveProperty('statusCode', 403); + expect( + createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 404 }))) + ).toHaveProperty('statusCode', 404); + expect( + createDataSourceError(new ResponseError(createApiResponseError({ statusCode: 500 }))) + ).toHaveProperty('statusCode', 500); + }); + + it('create from non-response client error should be casted to a 400 DataSourceError', () => { + expect( + createDataSourceError(new ConnectionError('error', createApiResponseError())) + ).toHaveProperty('statusCode', 400); + expect( + createDataSourceError(new NoLivingConnectionsError('error', createApiResponseError())) + ).toHaveProperty('statusCode', 400); + expect(createDataSourceError(new Error('foo'))).toHaveProperty('statusCode', 400); + }); + + it('create from legacy client 401 error should be casted to a 400 DataSourceError', () => { + expect(createDataSourceError(new LegacyErrors.AuthenticationException())).toEqual( + new DataSourceError(new Error('dummy'), 'Authentication Exception', 400) + ); + }); + + it('create from legacy client non 401 error should respect original statusCode', () => { + expect(createDataSourceError(new LegacyErrors.NotFound())).toEqual( + new DataSourceError(new Error('dummy'), 'Not Found', 404) + ); + expect(createDataSourceError(new LegacyErrors.TooManyRequests())).toEqual( + new DataSourceError(new Error('dummy'), 'Too Many Requests', 429) + ); + expect(createDataSourceError(new LegacyErrors.InternalServerError())).toEqual( + new DataSourceError(new Error('dummy'), 'Internal Server Error', 400) + ); + }); + + it('create from legacy client error should be casted to a 400 DataSourceError', () => { + expect(createDataSourceError(new LegacyErrors.NoConnections())).toEqual( + new DataSourceError(new Error('dummy'), 'No Living connections', 400) + ); + expect(createDataSourceError(new LegacyErrors.ConnectionFault())).toEqual( + new DataSourceError(new Error('dummy'), 'Connection Failure', 400) + ); + }); }); diff --git a/src/plugins/data_source/server/lib/error.ts b/src/plugins/data_source/server/lib/error.ts index 6667b41992f3..9a9866a072d6 100644 --- a/src/plugins/data_source/server/lib/error.ts +++ b/src/plugins/data_source/server/lib/error.ts @@ -3,19 +3,47 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { ResponseError } from '@opensearch-project/opensearch/lib/errors'; +import { errors as LegacyErrors } from 'elasticsearch'; import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; -import { OsdError } from '../../../../../src/plugins/opensearch_dashboards_utils/common'; +import { OsdError } from '../../../opensearch_dashboards_utils/common'; -export class DataSourceConfigError extends OsdError { +export class DataSourceError extends OsdError { // must have statusCode to avoid route handler in search.ts to return 500 statusCode: number; - constructor(messagePrefix: string, error: any) { - const messageContent = SavedObjectsErrorHelpers.isSavedObjectsClientError(error) - ? error.output.payload.message - : error.message; - super(messagePrefix + messageContent); - // Cast all 5xx error returned by saveObjectClient to 500. - // Cast both savedObject client 4xx errors, and other errors to 400 - this.statusCode = SavedObjectsErrorHelpers.isOpenSearchUnavailableError(error) ? 500 : 400; + constructor(error: any, message?: string, statusCode?: number) { + message = message ? message : error.message; + super('Data Source Error: ' + message); + if (statusCode) { + this.statusCode = statusCode; + } else if (error.statusCode) { + this.statusCode = error.statusCode; + } else { + this.statusCode = 400; + } } } + +export const createDataSourceError = (error: any, message?: string): DataSourceError => { + // handle saved object client error, while retrieve data source meta info + if (SavedObjectsErrorHelpers.isSavedObjectsClientError(error)) { + return new DataSourceError(error, error.output.payload.message, error.output.statusCode); + } + + // cast OpenSearch client 401 response error to 400, due to https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2591 + if (isResponseError(error) && error.statusCode === 401) { + return new DataSourceError(error, JSON.stringify(error.meta.body), 400); + } + + // cast legacy client 401 response error to 400 + if (error instanceof LegacyErrors.AuthenticationException) { + return new DataSourceError(error, error.message, 400); + } + + // handle all other error that may or may not comes with statuscode + return new DataSourceError(error, message); +}; + +const isResponseError = (error: any): error is ResponseError => { + return Boolean(error.body && error.statusCode && error.headers); +}; diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts index 2a3d02be6838..069bf1d0f457 100644 --- a/src/plugins/data_source/server/plugin.ts +++ b/src/plugins/data_source/server/plugin.ts @@ -28,6 +28,7 @@ import { DATA_SOURCE_SAVED_OBJECT_TYPE } from '../common'; // eslint-disable-next-line @osd/eslint/no-restricted-paths import { ensureRawRequest } from '../../../../src/core/server/http/router'; +import { createDataSourceError } from './lib/error'; export class DataSourcePlugin implements Plugin { private readonly logger: Logger; private readonly cryptographyService: CryptographyService; @@ -102,7 +103,9 @@ export class DataSourcePlugin implements Plugin createDataSourceError(e), + }; } public start(core: CoreStart) { diff --git a/src/plugins/data_source/server/types.ts b/src/plugins/data_source/server/types.ts index b5cc61a772bd..cf2748d046d7 100644 --- a/src/plugins/data_source/server/types.ts +++ b/src/plugins/data_source/server/types.ts @@ -10,6 +10,7 @@ import { } from 'src/core/server'; import { CryptographyServiceSetup } from './cryptography_service'; +import { DataSourceError } from './lib/error'; export interface LegacyClientCallAPIParams { endpoint: string; @@ -46,7 +47,8 @@ declare module 'src/core/server' { } } -// eslint-disable-next-line @typescript-eslint/no-empty-interface -export interface DataSourcePluginSetup {} +export interface DataSourcePluginSetup { + createDataSourceError: (err: any) => DataSourceError; +} // eslint-disable-next-line @typescript-eslint/no-empty-interface export interface DataSourcePluginStart {} diff --git a/src/plugins/index_pattern_management/server/routes/resolve_index.ts b/src/plugins/index_pattern_management/server/routes/resolve_index.ts index baf19ca5b7d0..ed463bd54ea7 100644 --- a/src/plugins/index_pattern_management/server/routes/resolve_index.ts +++ b/src/plugins/index_pattern_management/server/routes/resolve_index.ts @@ -63,13 +63,25 @@ export function registerResolveIndexRoute(router: IRouter): void { ? context.dataSource.opensearch.legacy.getClient(dataSourceId).callAPI : context.core.opensearch.legacy.client.callAsCurrentUser; - const result = await caller('transport.request', { - method: 'GET', - path: `/_resolve/index/${encodeURIComponent(req.params.query)}${ - queryString ? '?' + new URLSearchParams(queryString).toString() : '' - }`, - }); - return res.ok({ body: result }); + try { + const result = await caller('transport.request', { + method: 'GET', + path: `/_resolve/index/${encodeURIComponent(req.params.query)}${ + queryString ? '?' + new URLSearchParams(queryString).toString() : '' + }`, + }); + return res.ok({ body: result }); + } catch (err) { + return res.customError({ + statusCode: err.statusCode || 500, + body: { + message: err.message, + attributes: { + error: err.body?.error || err.message, + }, + }, + }); + } } ); }