Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MD] Support legacy client for data source #2204

Merged
merged 6 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

### 📈 Features/Enhancements

* [MD] Support legacy client for data source ([#2204](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2204))

### 🐛 Bug Fixes
* [Vis Builder] Fixes auto bounds for timeseries bar chart visualization ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
* [Vis Builder] Fixes visualization shift when editing agg ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
Expand Down
1 change: 0 additions & 1 deletion src/core/server/http/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@ export class Router implements IRouter {
opensearchDashboardsResponseFactory.badRequest({ body: e.message })
);
}
// TODO: add legacy data source client config error handling

return hapiResponseAdapter.toInternalError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
* under the License.
*/

import { LegacyAPICaller, OpenSearchClient } from 'opensearch-dashboards/server';
import { LegacyAPICaller } from 'opensearch-dashboards/server';

import { getFieldCapabilities, resolveTimePattern, createNoMatchingIndicesError } from './lib';

Expand All @@ -48,9 +48,9 @@ interface FieldSubType {
}

export class IndexPatternsFetcher {
private _callDataCluster: LegacyAPICaller | OpenSearchClient;
private _callDataCluster: LegacyAPICaller;

constructor(callDataCluster: LegacyAPICaller | OpenSearchClient) {
constructor(callDataCluster: LegacyAPICaller) {
this._callDataCluster = callDataCluster;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

import { defaults, keyBy, sortBy } from 'lodash';

import { LegacyAPICaller, OpenSearchClient } from 'opensearch-dashboards/server';
import { LegacyAPICaller } from 'opensearch-dashboards/server';
import { callFieldCapsApi } from '../opensearch_api';
import { FieldCapsResponse, readFieldCapsResponse } from './field_caps_response';
import { mergeOverrides } from './overrides';
Expand All @@ -47,7 +47,7 @@ import { FieldDescriptor } from '../../index_patterns_fetcher';
* @return {Promise<Array<FieldDescriptor>>}
*/
export async function getFieldCapabilities(
callCluster: LegacyAPICaller | OpenSearchClient,
callCluster: LegacyAPICaller,
indices: string | string[] = [],
metaFields: string[] = [],
fieldCapsOptions?: { allowNoIndices: boolean }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,23 +57,10 @@ export interface IndexAliasResponse {
* @return {Promise<IndexAliasResponse>}
*/
export async function callIndexAliasApi(
callCluster: LegacyAPICaller | OpenSearchClient,
callCluster: LegacyAPICaller,
indices: string[] | string
): Promise<IndicesAliasResponse> {
try {
// This approach of identify between OpenSearchClient vs LegacyAPICaller
// will be deprecated after support data client with legacy client
// https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2133
if ('transport' in callCluster) {
return (
await callCluster.indices.getAlias({
index: indices,
ignore_unavailable: true,
allow_no_indices: true,
})
).body as IndicesAliasResponse;
}

return (await callCluster('indices.getAlias', {
index: indices,
ignoreUnavailable: true,
Expand All @@ -97,25 +84,11 @@ export async function callIndexAliasApi(
* @return {Promise<FieldCapsResponse>}
*/
export async function callFieldCapsApi(
callCluster: LegacyAPICaller | OpenSearchClient,
callCluster: LegacyAPICaller,
indices: string[] | string,
fieldCapsOptions: { allowNoIndices: boolean } = { allowNoIndices: false }
) {
try {
// This approach of identify between OpenSearchClient vs LegacyAPICaller
// will be deprecated after support data client with legacy client
// https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2133
if ('transport' in callCluster) {
zhongnansu marked this conversation as resolved.
Show resolved Hide resolved
return (
await callCluster.fieldCaps({
index: indices,
fields: '*',
ignore_unavailable: true,
allow_no_indices: fieldCapsOptions.allowNoIndices,
})
).body as FieldCapsResponse;
}

return (await callCluster('fieldCaps', {
index: indices,
fields: '*',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@ import { callIndexAliasApi, IndicesAliasResponse } from './opensearch_api';
* and the indices that actually match the time
* pattern (matches);
*/
export async function resolveTimePattern(
callCluster: LegacyAPICaller | OpenSearchClient,
timePattern: string
) {
export async function resolveTimePattern(callCluster: LegacyAPICaller, timePattern: string) {
const aliases = await callIndexAliasApi(callCluster, timePatternToWildcard(timePattern));

const allIndexDetails = chain<IndicesAliasResponse>(aliases)
Expand Down
19 changes: 12 additions & 7 deletions src/plugins/data/server/index_patterns/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@
*/

import { schema } from '@osd/config-schema';
import { HttpServiceSetup, RequestHandlerContext } from 'opensearch-dashboards/server';
import {
HttpServiceSetup,
LegacyAPICaller,
RequestHandlerContext,
} from 'opensearch-dashboards/server';
import { IndexPatternsFetcher } from './fetcher';

export function registerRoutes(http: HttpServiceSetup) {
Expand Down Expand Up @@ -151,11 +155,12 @@ export function registerRoutes(http: HttpServiceSetup) {
);
}

const decideClient = async (context: RequestHandlerContext, request: any) => {
const decideClient = async (
context: RequestHandlerContext,
request: any
): Promise<LegacyAPICaller> => {
const dataSourceId = request.query.data_source;
if (dataSourceId) {
return await context.dataSource.opensearch.getClient(dataSourceId);
}

return context.core.opensearch.legacy.client.callAsCurrentUser;
return dataSourceId
? (context.dataSource.opensearch.legacy.getClient(dataSourceId).callAPI as LegacyAPICaller)
: context.core.opensearch.legacy.client.callAsCurrentUser;
};
17 changes: 9 additions & 8 deletions src/plugins/data_source/server/client/client_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,32 @@
*/

import { Client } from '@opensearch-project/opensearch';
import { Client as LegacyClient } from 'elasticsearch';
import LRUCache from 'lru-cache';
import { Logger } from 'src/core/server';
import { DataSourcePluginConfigType } from '../../config';

export interface OpenSearchClientPoolSetup {
getClientFromPool: (id: string) => Client | undefined;
addClientToPool: (endpoint: string, client: Client) => void;
getClientFromPool: (id: string) => Client | LegacyClient | undefined;
addClientToPool: (endpoint: string, client: Client | LegacyClient) => void;
}
Comment on lines 12 to 15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this pool includes a LRU of <endpoint, root_client> . since now we support Legacy client, shall we have one pool for legacy client, and another pool for new client? otherwise, it will cause runtime error when when a use case need a legacy client while getting a new client root client from the pool, and verse versa.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are in fact 2 pool instances being initiated in the constructor of DataSourceService here. One only stores new client, the other one for legacy client.

Since the only different of pool impl for legacy or new client is the "value" type of the LRU, I didn't make a copy of client_pool to avoid having too much duplicate code. Instead I defined the LRU type as multiple type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have 2 cache in the pool to avoid misuse the pool. but the current implementation makes sense to me.


/**
* OpenSearch client pool.
* OpenSearch client pool for data source.
*
* This client pool uses an LRU cache to manage OpenSearch Js client objects.
* It reuse TPC connections for each OpenSearch endpoint.
*/
export class OpenSearchClientPool {
// LRU cache
// key: data source endpoint url
// value: OpenSearch client object
private cache?: LRUCache<string, Client>;
// key: data source endpoint
// value: OpenSearch client object | Legacy client object
private cache?: LRUCache<string, Client | LegacyClient>;
private isClosed = false;

constructor(private logger: Logger) {}

public async setup(config: DataSourcePluginConfigType) {
public async setup(config: DataSourcePluginConfigType): Promise<OpenSearchClientPoolSetup> {
const logger = this.logger;
const { size } = config.clientPool;

Expand All @@ -53,7 +54,7 @@ export class OpenSearchClientPool {
return this.cache!.get(endpoint);
};

const addClientToPool = (endpoint: string, client: Client) => {
const addClientToPool = (endpoint: string, client: Client | LegacyClient) => {
this.cache!.set(endpoint, client);
};

Expand Down
30 changes: 11 additions & 19 deletions src/plugins/data_source/server/client/configure_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { ClientOptions } from '@opensearch-project/opensearch';
// eslint-disable-next-line @osd/eslint/no-restricted-paths
import { opensearchClientMock } from '../../../../core/server/opensearch/client/mocks';
import { CryptographyClient } from '../cryptography';
import { DataSourceClientParams } from '../types';

const DATA_SOURCE_ID = 'a54b76ec86771ee865a0f74a305dfff8';
const cryptoClient = new CryptographyClient('test', 'test', new Array(32).fill(0));
Expand All @@ -28,6 +29,7 @@ describe('configureClient', () => {
let clientOptions: ClientOptions;
let dataSourceAttr: DataSourceAttributes;
let dsClient: ReturnType<typeof opensearchClientMock.createInternalClient>;
let dataSourceClientParams: DataSourceClientParams;

beforeEach(() => {
dsClient = opensearchClientMock.createInternalClient();
Expand Down Expand Up @@ -70,9 +72,13 @@ describe('configureClient', () => {
references: [],
});

ClientMock.mockImplementation(() => {
return dsClient;
});
dataSourceClientParams = {
dataSourceId: DATA_SOURCE_ID,
savedObjects: savedObjectsMock,
cryptographyClient: cryptoClient,
};

ClientMock.mockImplementation(() => dsClient);
});

afterEach(() => {
Expand All @@ -94,14 +100,7 @@ describe('configureClient', () => {

parseClientOptionsMock.mockReturnValue(clientOptions);

const client = await configureClient(
DATA_SOURCE_ID,
savedObjectsMock,
cryptoClient,
clientPoolSetup,
config,
logger
);
const client = await configureClient(dataSourceClientParams, clientPoolSetup, config, logger);

expect(parseClientOptionsMock).toHaveBeenCalled();
expect(ClientMock).toHaveBeenCalledTimes(1);
Expand All @@ -113,14 +112,7 @@ describe('configureClient', () => {
test('configure client with auth.type == username_password, will first call decrypt()', async () => {
const spy = jest.spyOn(cryptoClient, 'decodeAndDecrypt').mockResolvedValue('password');

const client = await configureClient(
DATA_SOURCE_ID,
savedObjectsMock,
cryptoClient,
clientPoolSetup,
config,
logger
);
const client = await configureClient(dataSourceClientParams, clientPoolSetup, config, logger);

expect(ClientMock).toHaveBeenCalledTimes(1);
expect(savedObjectsMock.get).toHaveBeenCalledTimes(1);
Expand Down
25 changes: 15 additions & 10 deletions src/plugins/data_source/server/client/configure_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,12 @@ import {
import { DataSourcePluginConfigType } from '../../config';
import { CryptographyClient } from '../cryptography';
import { DataSourceConfigError } from '../lib/error';
import { DataSourceClientParams } from '../types';
import { parseClientOptions } from './client_config';
import { OpenSearchClientPoolSetup } from './client_pool';

export const configureClient = async (
dataSourceId: string,
savedObjects: SavedObjectsClientContract,
cryptographyClient: CryptographyClient,
{ dataSourceId, savedObjects, cryptographyClient }: DataSourceClientParams,
openSearchClientPoolSetup: OpenSearchClientPoolSetup,
config: DataSourcePluginConfigType,
logger: Logger
Expand Down Expand Up @@ -68,20 +67,26 @@ export const getCredential = async (
*
* @param rootClient root client for the connection with given data source endpoint.
* @param dataSource data source saved object
* @param cryptographyClient cryptography client for password encryption / decrpytion
* @param cryptographyClient cryptography client for password encryption / decryption
* @returns child client.
*/
const getQueryClient = async (
rootClient: Client,
dataSource: SavedObject<DataSourceAttributes>,
cryptographyClient: CryptographyClient
): Promise<Client> => {
if (AuthType.NoAuth === dataSource.attributes.auth.type) {
return rootClient.child();
} else {
const credential = await getCredential(dataSource, cryptographyClient);
const authType = dataSource.attributes.auth.type;

switch (authType) {
case AuthType.NoAuth:
return rootClient.child();

case AuthType.UsernamePasswordType:
const credential = await getCredential(dataSource, cryptographyClient);
return getBasicAuthClient(rootClient, credential);

return getBasicAuthClient(rootClient, credential);
default:
throw Error(`${authType} is not a supported auth type for data source`);
}
};

Expand All @@ -101,7 +106,7 @@ const getRootClient = (
const endpoint = dataSourceAttr.endpoint;
const cachedClient = getClientFromPool(endpoint);
if (cachedClient) {
return cachedClient;
return cachedClient as Client;
zhongnansu marked this conversation as resolved.
Show resolved Hide resolved
} else {
const clientOptions = parseClientOptions(config, endpoint);

Expand Down
4 changes: 2 additions & 2 deletions src/plugins/data_source/server/client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
* SPDX-License-Identifier: Apache-2.0
*/

export { OpenSearchClientPool } from './client_pool';
export { configureClient } from './configure_client';
export { OpenSearchClientPool, OpenSearchClientPoolSetup } from './client_pool';
export { configureClient, getDataSource, getCredential } from './configure_client';
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe('Data Source Service', () => {
test('exposes proper contract', async () => {
const setup = await service.setup(config);
expect(setup).toHaveProperty('getDataSourceClient');
expect(setup).toHaveProperty('getDataSourceLegacyClient');
});
});
});
Loading