[MD]Improve datasource server side error handling (opensearch-project…

…#2236) Signed-off-by: Su <[email protected]> Signed-off-by: Kristen Tian <[email protected]>
kristenTian · Sep 14, 2022 · 4fe8da1 · 4fe8da1
1 parent 70302d9
commit 4fe8da1
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 49 deletions.
diff --git 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
@@ -4,21 +4,16 @@
  */
 
 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,
   UsernamePasswordTypedContent,
 } 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<Client> => {
-  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<SavedObject<DataSourceAttributes>> => {
-  try {
-    const dataSource = await savedObjects.get<DataSourceAttributes>(
-      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<DataSourceAttributes>(
+    DATA_SOURCE_SAVED_OBJECT_TYPE,
+    dataSourceId
+  );
+  return dataSource;
 };
 
 export const getCredential = async (
   dataSource: SavedObject<DataSourceAttributes>,
   cryptographyClient: CryptographyClient
 ): Promise<UsernamePasswordTypedContent> => {
-  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
@@ -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
@@ -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
@@ -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
@@ -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<DataSourcePluginSetup, DataSourc
       return {
         opensearch: {
           getClient: (dataSourceId: string) => {
-            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
+            );
           },
         },
       };