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

[Multiple Data Source] Support SigV4 as a new auth type of datasource with aliased client #3470

Closed
Show file tree
Hide file tree
Changes from all 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 @@ -25,6 +25,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### 🛠 Maintenance

- Bumps `re2` and `supertest` ([3018](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3018))
- Introduce @opensearch-project/opensearch@^2.1.0, aliased as @opensearch-project/opensearch-next ([#3469](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3469))

### 🪛 Refactoring

Expand Down Expand Up @@ -52,6 +53,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [VisBuilder] Enable persistence for app filter and query without using state containers ([#3100](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3100))
- [Data] Make the newly created configurations get added to beginning of the `aggConfig` array when using `createAggConfig` ([#3160](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3160))
- [Optimizer] Increase the amount of time an optimizer worker is provided to exit before throwing an error ([#3193](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3193))
- [Multiple DataSource] Add support for SigV4 authentication ([#3058](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3058))

### 🐛 Bug Fixes

Expand Down
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
"@hapi/vision": "^6.1.0",
"@hapi/wreck": "^17.1.0",
"@opensearch-project/opensearch": "^1.1.0",
"@opensearch-project/opensearch-next": "npm:@opensearch-project/opensearch@^2.1.0",
"@osd/ace": "1.0.0",
"@osd/analytics": "1.0.0",
"@osd/apm-config-loader": "1.0.0",
Expand Down Expand Up @@ -166,6 +167,7 @@
"dns-sync": "^0.2.1",
"elastic-apm-node": "^3.7.0",
"elasticsearch": "^16.7.0",
"http-aws-es": "6.0.0",
"execa": "^4.0.2",
"expiry-js": "0.1.7",
"fast-deep-equal": "^3.1.1",
Expand Down Expand Up @@ -334,6 +336,7 @@
"@types/zen-observable": "^0.8.0",
"@typescript-eslint/eslint-plugin": "^3.10.0",
"@typescript-eslint/parser": "^3.10.0",
"@types/http-aws-es": "6.0.2",
"angular-aria": "^1.8.0",
"angular-mocks": "^1.8.2",
"angular-recursion": "^1.0.5",
Expand Down
2 changes: 2 additions & 0 deletions src/dev/jest/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export default {
moduleNameMapper: {
'@elastic/eui$': '<rootDir>/node_modules/@elastic/eui/test-env',
'@elastic/eui/lib/(.*)?': '<rootDir>/node_modules/@elastic/eui/test-env/$1',
'@opensearch-project/opensearch-next/aws':
'<rootDir>/node_modules/@opensearch-project/opensearch-next/lib/aws',
'^src/plugins/(.*)': '<rootDir>/src/plugins/$1',
'^test_utils/(.*)': '<rootDir>/src/test_utils/public/$1',
'^fixtures/(.*)': '<rootDir>/src/fixtures/$1',
Expand Down
15 changes: 14 additions & 1 deletion src/plugins/data_source/common/data_sources/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,20 @@ export interface DataSourceAttributes extends SavedObjectAttributes {
endpoint: string;
auth: {
type: AuthType;
credentials: UsernamePasswordTypedContent | undefined;
credentials: UsernamePasswordTypedContent | SigV4Content | undefined;
};
lastUpdatedTime?: string;
}

/**
* Multiple datasource supports authenticating as IAM user, it doesn't support IAM role.
* Because IAM role session requires temporary security credentials through assuming role,
* which makes no sense to store the credentials.
*/
export interface SigV4Content extends SavedObjectAttributes {
accessKey: string;
secretKey: string;
region: string;
}

export interface UsernamePasswordTypedContent extends SavedObjectAttributes {
Expand All @@ -23,4 +35,5 @@ export interface UsernamePasswordTypedContent extends SavedObjectAttributes {
export enum AuthType {
NoAuth = 'no_auth',
UsernamePasswordType = 'username_password',
SigV4 = 'sigv4',
}
66 changes: 51 additions & 15 deletions src/plugins/data_source/server/client/client_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@
* SPDX-License-Identifier: Apache-2.0
*/

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

export interface OpenSearchClientPoolSetup {
getClientFromPool: (id: string) => Client | LegacyClient | undefined;
addClientToPool: (endpoint: string, client: Client | LegacyClient) => void;
getClientFromPool: (endpoint: string, authType: AuthType) => Client | LegacyClient | undefined;
addClientToPool: (endpoint: string, authType: AuthType, client: Client | LegacyClient) => void;
}

/**
Expand All @@ -21,23 +22,28 @@ export interface OpenSearchClientPoolSetup {
* It reuse TPC connections for each OpenSearch endpoint.
*/
export class OpenSearchClientPool {
// LRU cache
// LRU cache of client
// key: data source endpoint
// value: OpenSearch client object | Legacy client object
private cache?: LRUCache<string, Client | LegacyClient>;
// value: OpenSearch client | Legacy client
private clientCache?: LRUCache<string, Client | LegacyClient>;
// LRU cache of aws clients
// key: endpoint + dataSourceId + lastUpdatedTime together to support update case.
// value: OpenSearch client | Legacy client
private awsClientCache?: LRUCache<string, Client | LegacyClient>;
private isClosed = false;

constructor(private logger: Logger) {}

public setup(config: DataSourcePluginConfigType): OpenSearchClientPoolSetup {
const logger = this.logger;
const { size } = config.clientPool;
const MAX_AGE = 15 * 60 * 1000; // by default, TCP connection times out in 15 minutes

this.cache = new LRUCache({
this.clientCache = new LRUCache({
max: size,
maxAge: 15 * 60 * 1000, // by default, TCP connection times out in 15 minutes
maxAge: MAX_AGE,

async dispose(endpoint, client) {
async dispose(key, client) {
try {
await client.close();
} catch (error: any) {
Expand All @@ -50,12 +56,34 @@ export class OpenSearchClientPool {
});
this.logger.info(`Created data source client pool of size ${size}`);

const getClientFromPool = (endpoint: string) => {
return this.cache!.get(endpoint);
// aws client specific pool
this.awsClientCache = new LRUCache({
max: size,
maxAge: MAX_AGE,

async dispose(key, client) {
try {
await client.close();
} catch (error: any) {
logger.warn(
`Error closing OpenSearch client when removing from aws client pool: ${error.message}`
Comment on lines +68 to +69
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it a common practice in OSD to issue warnings with the term "Error"? If not, we should change this to

Suggested change
logger.warn(
`Error closing OpenSearch client when removing from aws client pool: ${error.message}`
logger.warn(
`OpenSearch client failed to close while disposing from the client pool: ${error.message}`

Also, why "AWS", that too lowercase?

);
}
},
});
this.logger.info(`Created data source aws client pool of size ${size}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message needs rewording.


const getClientFromPool = (key: string, authType: AuthType) => {
const selectedCache = authType === AuthType.SigV4 ? this.awsClientCache : this.clientCache;

return selectedCache!.get(key);
};

const addClientToPool = (endpoint: string, client: Client | LegacyClient) => {
this.cache!.set(endpoint, client);
const addClientToPool = (key: string, authType: string, client: Client | LegacyClient) => {
const selectedCache = authType === AuthType.SigV4 ? this.awsClientCache : this.clientCache;
if (!selectedCache?.has(key)) {
return selectedCache!.set(key, client);
}
};

return {
Expand All @@ -71,7 +99,15 @@ export class OpenSearchClientPool {
if (this.isClosed) {
return;
}
await Promise.all(this.cache!.values().map((client) => client.close()));
this.isClosed = true;

try {
await Promise.all([
...this.clientCache!.values().map((client) => client.close()),
...this.awsClientCache!.values().map((client) => client.close()),
]);
this.isClosed = true;
} catch (error) {
this.logger.error(`Error closing clients in pool. ${error}`);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
*/

export const ClientMock = jest.fn();
jest.doMock('@opensearch-project/opensearch', () => {
const actual = jest.requireActual('@opensearch-project/opensearch');
jest.doMock('@opensearch-project/opensearch-next', () => {
const actual = jest.requireActual('@opensearch-project/opensearch-next');
return {
...actual,
Client: ClientMock,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { DataSourcePluginConfigType } from '../../config';
import { ClientMock, parseClientOptionsMock } from './configure_client.test.mocks';
import { OpenSearchClientPoolSetup } from './client_pool';
import { configureClient } from './configure_client';
import { ClientOptions } from '@opensearch-project/opensearch';
import { ClientOptions } from '@opensearch-project/opensearch-next';
// eslint-disable-next-line @osd/eslint/no-restricted-paths
import { opensearchClientMock } from '../../../../core/server/opensearch/client/mocks';
import { cryptographyServiceSetupMock } from '../cryptography_service.mocks';
Expand Down Expand Up @@ -137,7 +137,7 @@ describe('configureClient', () => {
configureClient(dataSourceClientParams, clientPoolSetup, config, logger)
).rejects.toThrowError();

expect(ClientMock).toHaveBeenCalledTimes(1);
expect(ClientMock).not.toHaveBeenCalled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This previously made sure it was called but also that it was called only once; is that no longer true?

Copy link
Member

Choose a reason for hiding this comment

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

no longer true. The logic was imporved. If password determined to be contaminated, there's no need to call the Client() to create client

expect(savedObjectsMock.get).toHaveBeenCalledTimes(1);
expect(decodeAndDecryptSpy).toHaveBeenCalledTimes(1);
});
Expand All @@ -152,7 +152,7 @@ describe('configureClient', () => {
configureClient(dataSourceClientParams, clientPoolSetup, config, logger)
).rejects.toThrowError();

expect(ClientMock).toHaveBeenCalledTimes(1);
expect(ClientMock).not.toHaveBeenCalled();
expect(savedObjectsMock.get).toHaveBeenCalledTimes(1);
expect(decodeAndDecryptSpy).toHaveBeenCalledTimes(1);
});
Expand Down
Loading