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

joshuarrrr · 2023-02-21T02:38:50Z

Description

Draft PR to validate that #3469 is sufficient to enable the 2.x backporting of #3058

Issues Resolved

Check List

Enable usage of `@opensearch-project/opensearch@^2.x` features without breaking plugins that directly depend on `1.x` Signed-off-by: Josh Romero <[email protected]>

Signed-off-by: Josh Romero <[email protected]>

…ct#3058) * [Multiple DataSource] Add support for SigV4 authentication Signed-off-by: Su <[email protected]>

…h-project/opensearch-next` alias Revert change to `osd-opensearch` Signed-off-by: Josh Romero <[email protected]>

AMoo-Miki · 2023-02-21T02:49:14Z

src/plugins/data_source/server/client/client_pool.ts

+          logger.warn(
+            `Error closing OpenSearch client when removing from aws client pool: ${error.message}`


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?

AMoo-Miki · 2023-02-21T02:49:43Z

src/plugins/data_source/server/client/client_pool.ts

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


This message needs rewording.

zhongnansu · 2023-02-21T02:49:32Z

src/plugins/data_source/server/legacy/configure_legacy_client.ts

@@ -3,27 +3,38 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-import { Client } from 'elasticsearch';
+import { Client } from '@opensearch-project/opensearch-next';


This dependency update is not needed. We are using elasticsearch everywhere in data_source/server/legacy, that's to support MD in legacy client use case

We may want a follow-up PR for main then - that's in the original PR: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3058/files#diff-442cda9a371f9aa2ce417631ddf7a8b80f9ec6ade15676b1cfe390f1c25b3fe7R6

Oh my bad, I didn't see the new lines below. No need to change. elasticsearch is being added in line 7.

AMoo-Miki · 2023-02-21T02:53:07Z

src/plugins/data_source/server/client/configure_client.test.ts

@@ -137,7 +137,7 @@ describe('configureClient', () => {
      configureClient(dataSourceClientParams, clientPoolSetup, config, logger)
    ).rejects.toThrowError();

-    expect(ClientMock).toHaveBeenCalledTimes(1);
+    expect(ClientMock).not.toHaveBeenCalled();


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

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

AMoo-Miki · 2023-02-21T02:59:42Z

src/plugins/data_source/server/legacy/configure_legacy_client.ts

@@ -113,7 +142,7 @@ const getRootClient = (
 * make wrap401Errors default to false, because we don't want login pop-up from browser
 */
 const callAPI = async (
-  client: Client,
+  client: LegacyClient,


Does this mean we are changing something that used the newer client to use the legacy client?

codecov-commenter · 2023-02-21T03:27:20Z

Codecov Report

Merging #3470 (19d7d9c) into 2.x (17ca299) will decrease coverage by 0.18%.
The diff coverage is 40.90%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              2.x    #3470      +/-   ##
==========================================
- Coverage   66.54%   66.37%   -0.18%     
==========================================
  Files        3203     3205       +2     
  Lines       61397    61551     +154     
  Branches     9453     9492      +39     
==========================================
- Hits        40859    40854       -5     
- Misses      18284    18424     +140     
- Partials     2254     2273      +19

Flag	Coverage Δ
Linux	`66.37% <40.90%> (-0.12%)`	⬇️
Windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/data_source/server/lib/error.ts	`100.00% <ø> (ø)`
...omponents/validation/datasource_form_validation.ts	`75.86% <0.00%> (-24.14%)`	⬇️
src/plugins/data_source_management/public/types.ts	`100.00% <ø> (ø)`
...s_credential_modal/update_aws_credential_modal.tsx	`5.26% <5.26%> (ø)`
...gins/data_source/server/client/configure_client.ts	`46.66% <35.71%> (-19.38%)`	⬇️
...rce/components/edit_form/edit_data_source_form.tsx	`70.74% <38.35%> (-20.03%)`	⬇️
...components/create_form/create_data_source_form.tsx	`75.26% <41.66%> (-21.71%)`	⬇️
...c/plugins/data_source/server/client/client_pool.ts	`46.66% <45.00%> (-12.16%)`	⬇️
...ta_source/server/legacy/configure_legacy_client.ts	`59.61% <50.00%> (-11.22%)`	⬇️
...ata_source/server/client/configure_client_utils.ts	`64.70% <64.70%> (ø)`
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zhongnansu

Re-posted another PR against main #3471, to address the below issues

The aliasing solution should be posted against main, and then backport to 2.x. Because main has merged in the fix to update yarn version in package.json to unblock Plugin CI. If we post PR against 2.x, the same CI issue will happen again for plugins on 2.x branch. yarn version incompatibility issue blocks Dashboards plugins CI, docker-image, windows build opensearch-build#3210

error @opensearch-project/[email protected]: The engine "yarn" is incompatible with this module. Expected version "^1.22.10". Got "1.21.1"
error Found incompatible module.

all files under data_source component should be re-imported to use aliases, this PR is missing changes in few files. client_config.ts and error.test.ts
typo

zhongnansu · 2023-02-21T05:30:29Z

src/plugins/data_source/server/lib/error.test.ts

@@ -3,7 +3,7 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-import { ApiResponse } from '@opensearch-project/opensearch';
+import { ApiResponse } from '@opensearch-project/opensearc-next';


zhongnansu · 2023-02-21T19:50:02Z

Thanks Josh. We have this PR opened to backport MD sigv4 feature #3477

joshuarrrr and others added 5 commits February 21, 2023 01:58

Chore: Add aliased opensearch js client as dependency

837cd21

Enable usage of `@opensearch-project/opensearch@^2.x` features without breaking plugins that directly depend on `1.x` Signed-off-by: Josh Romero <[email protected]>

[Multiple Data Sources] Chore: update opensearch client usage to 2.x

af8815f

Signed-off-by: Josh Romero <[email protected]>

Chore: Update CHANGELOG

bcdc5c8

Signed-off-by: Josh Romero <[email protected]>

[MD] Support SigV4 as a new auth type of datasource (opensearch-proje…

7989b81

…ct#3058) * [Multiple DataSource] Add support for SigV4 authentication Signed-off-by: Su <[email protected]>

[Multiple Data Source] Update data_source plugin to use `@opensearc…

19d7d9c

…h-project/opensearch-next` alias Revert change to `osd-opensearch` Signed-off-by: Josh Romero <[email protected]>

joshuarrrr requested a review from a team as a code owner February 21, 2023 02:38

joshuarrrr marked this pull request as draft February 21, 2023 02:39

AMoo-Miki reviewed Feb 21, 2023

View reviewed changes

zhongnansu reviewed Feb 21, 2023

View reviewed changes

AMoo-Miki reviewed Feb 21, 2023

View reviewed changes

zhongnansu reviewed Feb 21, 2023

View reviewed changes

zhongnansu closed this Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshuarrrr commented Feb 21, 2023

AMoo-Miki Feb 21, 2023

AMoo-Miki Feb 21, 2023

zhongnansu Feb 21, 2023

joshuarrrr Feb 21, 2023

zhongnansu Feb 21, 2023

AMoo-Miki Feb 21, 2023

zhongnansu Feb 21, 2023

AMoo-Miki Feb 21, 2023

codecov-commenter commented Feb 21, 2023

zhongnansu left a comment •

edited

Loading

zhongnansu Feb 21, 2023

zhongnansu commented Feb 21, 2023

		logger.warn(
		`Error closing OpenSearch client when removing from aws client pool: ${error.message}`

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

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

Conversation

joshuarrrr commented Feb 21, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 21, 2023

Codecov Report

zhongnansu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongnansu commented Feb 21, 2023

zhongnansu left a comment •

edited

Loading