Datasource id is required if multiple datasource is enabled and no de…

…fault cluster supported (opensearch-project#5751) * datasource id is required if multiple datasource is enabled and no default cluster supported Signed-off-by: Xinrui Bai <xinruiba@amazon.com> --------- Signed-off-by: Xinrui Bai <xinruiba@amazon.com>
yujin-emma · Feb 6, 2024 · bbd40e1 · bbd40e1
1 parent d968bbe
commit bbd40e1
Show file tree

Hide file tree

Showing 8 changed files with 127 additions and 14 deletions.
diff --git 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
@@ -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
@@ -9,14 +9,14 @@ import { IOpenSearchSearchRequest } from '..';
 export const decideClient = async (
   context: RequestHandlerContext,
   request: IOpenSearchSearchRequest,
+  withDataSourceEnabled: boolean = false,
   withLongNumeralsSupport: boolean = false
 ): Promise<OpenSearchClient> => {
-  // 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
@@ -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<void> {
+      return Promise.resolve(undefined);
+    },
+    trackSuccess(duration: number): Promise<void> {
+      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
@@ -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<SearchResponse<any>>;
@@ -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
@@ -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
@@ -111,6 +111,8 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
 
     return {
       createDataSourceError: (e: any) => 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
@@ -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 {}