From 4fe8da125525401a7268fba57e42b745a44234c3 Mon Sep 17 00:00:00 2001 From: Zhongnan Su Date: Tue, 6 Sep 2022 11:11:55 -0700 Subject: [PATCH] [MD]Improve datasource server side error handling (#2236) Signed-off-by: Su Signed-off-by: Kristen Tian --- src/core/server/http/router/router.ts | 12 ++++ .../server/client/configure_client.ts | 56 ++++++++----------- .../data_source/server/data_source_service.ts | 2 +- .../data_source/server/lib/error.test.ts | 40 +++++++++++++ src/plugins/data_source/server/lib/error.ts | 21 +++++++ src/plugins/data_source/server/plugin.ts | 24 +++----- 6 files changed, 106 insertions(+), 49 deletions(-) create mode 100644 src/plugins/data_source/server/lib/error.test.ts create mode 100644 src/plugins/data_source/server/lib/error.ts diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index a426eef40e7a..752706879d0e 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -300,11 +300,23 @@ export class Router implements IRouter { if (LegacyOpenSearchErrorHelpers.isNotAuthorizedError(e)) { return e; } + + if (isDataSourceConfigError(e)) { + return hapiResponseAdapter.handle( + opensearchDashboardsResponseFactory.badRequest({ body: e.message }) + ); + } + // TODO: add legacy data source client config error handling + 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_source/server/client/configure_client.ts b/src/plugins/data_source/server/client/configure_client.ts index 5ba18261ba74..8cfa9769de7a 100644 --- a/src/plugins/data_source/server/client/configure_client.ts +++ b/src/plugins/data_source/server/client/configure_client.ts @@ -4,14 +4,8 @@ */ import { Client } from '@opensearch-project/opensearch'; -import { - Logger, - SavedObject, - SavedObjectsClientContract, - SavedObjectsErrorHelpers, -} from '../../../../../src/core/server'; +import { Logger, SavedObject, SavedObjectsClientContract } from '../../../../../src/core/server'; import { DATA_SOURCE_SAVED_OBJECT_TYPE } from '../../common'; - import { AuthType, DataSourceAttributes, @@ -19,6 +13,7 @@ import { } from '../../common/data_sources'; import { DataSourcePluginConfigType } from '../../config'; import { CryptographyClient } from '../cryptography'; +import { DataSourceConfigError } from '../lib/error'; import { parseClientOptions } from './client_config'; import { OpenSearchClientPoolSetup } from './client_pool'; @@ -30,45 +25,42 @@ export const configureClient = async ( config: DataSourcePluginConfigType, logger: Logger ): Promise => { - const dataSource = await getDataSource(dataSourceId, savedObjects); - const rootClient = getRootClient(dataSource.attributes, config, openSearchClientPoolSetup); + try { + const dataSource = await getDataSource(dataSourceId, savedObjects); + const rootClient = getRootClient(dataSource.attributes, config, openSearchClientPoolSetup); - return getQueryClient(rootClient, dataSource, cryptographyClient); + return await getQueryClient(rootClient, dataSource, cryptographyClient); + } catch (error: any) { + logger.error(`Fail to get data source client for dataSourceId: [${dataSourceId}]`); + logger.error(error); + // Re-throw as DataSourceConfigError + throw new DataSourceConfigError('Fail to get data source client: ', error); + } }; export const getDataSource = async ( dataSourceId: string, savedObjects: SavedObjectsClientContract ): Promise> => { - try { - const dataSource = await savedObjects.get( - DATA_SOURCE_SAVED_OBJECT_TYPE, - dataSourceId - ); - return dataSource; - } catch (error: any) { - // it will cause 500 error when failed to get saved objects, need to handle such error gracefully - throw SavedObjectsErrorHelpers.createBadRequestError(error.message); - } + const dataSource = await savedObjects.get( + DATA_SOURCE_SAVED_OBJECT_TYPE, + dataSourceId + ); + return dataSource; }; export const getCredential = async ( dataSource: SavedObject, cryptographyClient: CryptographyClient ): Promise => { - try { - const { username, password } = dataSource.attributes.auth.credentials!; - const decodedPassword = await cryptographyClient.decodeAndDecrypt(password); - const credential = { - username, - password: decodedPassword, - }; + const { username, password } = dataSource.attributes.auth.credentials!; + const decodedPassword = await cryptographyClient.decodeAndDecrypt(password); + const credential = { + username, + password: decodedPassword, + }; - return credential; - } catch (error: any) { - // it will cause 500 error when failed to get saved objects, need to handle such error gracefully - throw SavedObjectsErrorHelpers.createBadRequestError(error.message); - } + return credential; }; /** diff --git a/src/plugins/data_source/server/data_source_service.ts b/src/plugins/data_source/server/data_source_service.ts index 1bab1a1d0b0b..73f61b87cb38 100644 --- a/src/plugins/data_source/server/data_source_service.ts +++ b/src/plugins/data_source/server/data_source_service.ts @@ -30,7 +30,7 @@ export class DataSourceService { async setup(config: DataSourcePluginConfigType) { const openSearchClientPoolSetup = await this.openSearchClientPool.setup(config); - const getDataSourceClient = async ( + const getDataSourceClient = ( dataSourceId: string, savedObjects: SavedObjectsClientContract, cryptographyClient: CryptographyClient diff --git a/src/plugins/data_source/server/lib/error.test.ts b/src/plugins/data_source/server/lib/error.test.ts new file mode 100644 index 000000000000..fca7efb0904c --- /dev/null +++ b/src/plugins/data_source/server/lib/error.test.ts @@ -0,0 +1,40 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; +import { DataSourceConfigError } from './error'; + +describe('DataSourceConfigError', () => { + it('create from savedObject bad request error should be 400 error', () => { + const error = SavedObjectsErrorHelpers.createBadRequestError('test reason message'); + expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + statusCode: 400, + message: 'test prefix: 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 service unavailable error should be a 500 error', () => { + const error = SavedObjectsErrorHelpers.decorateOpenSearchUnavailableError( + new Error('test reason message') + ); + expect(new DataSourceConfigError('test prefix: ', error)).toMatchObject({ + statusCode: 500, + message: 'test prefix: 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({ + statusCode: 400, + message: 'test prefix: test reason message', + }); + }); +}); diff --git a/src/plugins/data_source/server/lib/error.ts b/src/plugins/data_source/server/lib/error.ts new file mode 100644 index 000000000000..e921a8d8043f --- /dev/null +++ b/src/plugins/data_source/server/lib/error.ts @@ -0,0 +1,21 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; +import { OsdError } from '../../../../../src/plugins/opensearch_dashboards_utils/common'; + +export class DataSourceConfigError 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, 400 for both savedObject client + // 4xx errors, and other errors + this.statusCode = SavedObjectsErrorHelpers.isOpenSearchUnavailableError(error) ? 500 : 400; + } +} diff --git a/src/plugins/data_source/server/plugin.ts b/src/plugins/data_source/server/plugin.ts index e2b1275a5079..5ce3508433ce 100644 --- a/src/plugins/data_source/server/plugin.ts +++ b/src/plugins/data_source/server/plugin.ts @@ -3,7 +3,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { OpenSearchClientError } from '@opensearch-project/opensearch/lib/errors'; import { Observable } from 'rxjs'; import { first, map } from 'rxjs/operators'; import { @@ -129,21 +128,14 @@ export class DataSourcePlugin implements Plugin { - try { - const auditor = auditTrailPromise.then((auditTrail) => auditTrail.asScoped(req)); - this.logAuditMessage(auditor, dataSourceId, req); - - return dataSourceService.getDataSourceClient( - dataSourceId, - context.core.savedObjects.client, - cryptographyClient - ); - } catch (error: any) { - logger.error( - `Fail to get data source client for dataSourceId: [${dataSourceId}]. Detail: ${error.messages}` - ); - throw new OpenSearchClientError(error.message); - } + const auditor = auditTrailPromise.then((auditTrail) => auditTrail.asScoped(req)); + this.logAuditMessage(auditor, dataSourceId, req); + + return dataSourceService.getDataSourceClient( + dataSourceId, + context.core.savedObjects.client, + cryptographyClient + ); }, }, };