[MD] Add endpoint validation to create data source API #6631

zhongnansu · 2024-04-24T22:59:11Z

Description

Add endpoint validation to create data source API, by testing if endpoint is reachable, using the same logic as what's behind "test connection" feature

Issues Resolved

#6517

Screenshot

Testing the changes

go to create datasource page, and create one with invalid OpenSearch endpoint, e.g. google.com
click test connection, it should fail as expected
click create data source, it should fail too, as the endpoint is not a valid endpoint
switch to use a valid opensearch domain
both test connection and create data source should pass

Changelog

feat: Add endpoint validation to create data source API

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

github-actions · 2024-04-24T22:59:43Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

github-actions · 2024-04-24T23:00:11Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

codecov · 2024-04-24T23:14:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.80%. Comparing base (d2d410b) to head (6121267).
Report is 35 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6631       +/-   ##
===========================================
+ Coverage   32.93%   67.80%   +34.87%     
===========================================
  Files        2260     3413     +1153     
  Lines       45769    66755    +20986     
  Branches     7200    10861     +3661     
===========================================
+ Hits        15075    45266    +30191     
+ Misses      29984    18845    -11139     
- Partials      710     2644     +1934

Flag	Coverage Δ
Linux_1	`33.18% <ø> (+0.25%)`	⬆️
Linux_2	`55.63% <ø> (?)`
Linux_3	`45.24% <100.00%> (?)`
Linux_4	`34.91% <ø> (?)`
Windows_1	`33.23% <ø> (?)`
Windows_2	`55.59% <ø> (?)`
Windows_3	`45.25% <100.00%> (?)`
Windows_4	`34.91% <ø> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-04-25T20:37:15Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

BionIT · 2024-04-25T22:00:37Z

Probably better to remove the credentials or modify the recording to hide it

huyaboo

Left nonblocking comment

huyaboo · 2024-04-25T22:12:36Z

src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts

      throw SavedObjectsErrorHelpers.createBadRequestError(
        '"title" attribute must be a non-empty string'
      );
    }

+    if (title.length > 32) {


nit: could we make this a constant with a meaningful name?

updated to use constant

zhyuanqi · 2024-04-25T22:15:00Z

src/plugins/data_source/server/plugin.ts

@@ -61,16 +61,24 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
    const cryptographyServiceSetup: CryptographyServiceSetup = this.cryptographyService.setup(
      config
    );
+    const dataSourceService: DataSourceServiceSetup = await this.dataSourceService.setup(config);


Should we name it dataSourceServiceSetUp as cryptographyServiceSetup

zhyuanqi · 2024-04-25T22:17:30Z

src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts

+    if (!auth) {
+      throw SavedObjectsErrorHelpers.createBadRequestError('"auth" attribute is required');
+    }
+


So from now on, we cannot choose no auth?

creating data source with no auth auth type still has an an attribute in the request payload

zhyuanqi · 2024-04-25T22:22:12Z

Just a general thought. I see the error is 'Creation of the Data Source failed with some errors.'. Not sure if we need to give more specifc error in different error cases such as no auth found, please provide authentication

Signed-off-by: Zhongnan Su <szhongna@amazon.com>

zhongnansu · 2024-04-25T22:52:36Z

Just a general thought. I see the error is 'Creation of the Data Source failed with some errors.'. Not sure if we need to give more specifc error in different error cases such as no auth found, please provide authentication

The error message is the existing behavior when failing to create data source , with/without my change, I agree we should improve by displaying the error with details to user. It could be a separate task to work on, and it should take care of both test connection and create data source

xinruiba · 2024-04-25T22:54:05Z

I remember datasource is able to be created even test connection failed used to be the expected behavior.
We decide to change it?

zhongnansu · 2024-04-25T23:01:15Z

I remember datasource is able to be created even test connection failed used to be the expected behavior. We decide to change it?

AFAIK, yes, is there any concern? @xinruiba

xinruiba · 2024-04-25T23:02:38Z

I remember datasource is able to be created even test connection failed used to be the expected behavior. We decide to change it?

AFAIK, yes, is there any concern? @xinruiba

No concerns if that's by design~

xinruiba · 2024-04-25T23:22:11Z

src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts

+        testClientDataSourceAttr: attributes as DataSourceAttributes,
+        authRegistry: await this.authRegistryPromise,
+        customApiSchemaRegistryPromise: this.customApiSchemaRegistryPromise,
+      });


Thanks for this change.

Are there any chances we can reuse this function: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source_management/public/components/utils.ts#L206?

Since datasource and datasource_management plugin will both exist in the same time, so the validation path in datasource_management plugin should always been routed.

Then we depend on sending request to /internal/data-source-management/validate to test connection instead of creating a new datasource client here.

What do you think?

Do you mean when user click create data source button, we first call /internal/data-source-management/validate to validate and then call create data source API? @xinruiba

Yes

Pros:

We rely on the URL for data source validation.

Cons:

If customers directly create a data source by calling the URL (using curl), the data source validation will be bypassed.

I'll leave the decision to you~

Yes, the cons you mentioned is exactly the reason I choose to add validation to the saved object create data source API, this will almost make sure no invalid endpoint can be created as a data source. I use "almost", cuz user can still directly write data into .kibana index

Sounds good to me. Approving~

Non blocking comment.

Maybe a question to designer. Since we already do validation as a part of datasource creation. Do we sill need a separate test connection before clicking on datasource creation?

good point, @kgcreative @xeniatup, I wonder what's your thought on this

Given the updated validation, i don't think we need it in the create page.

Signed-off-by: Zhongnan Su <szhongna@amazon.com> (cherry picked from commit 0c114ce) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…#6631) Signed-off-by: Zhongnan Su <szhongna@amazon.com>

github-actions bot added the distinguished-contributor label Apr 24, 2024

github-actions bot added the failed changeset label Apr 24, 2024

zhongnansu force-pushed the md-validate-endpoint branch from abd7ffd to ec2f7c8 Compare April 25, 2024 20:35

zhongnansu force-pushed the md-validate-endpoint branch from ec2f7c8 to 27f9ad5 Compare April 25, 2024 21:14

opensearch-changeset-bot bot added a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Apr 25, 2024

Changeset file for PR opensearch-project#6631 created/updated

809af74

github-actions bot removed the failed changeset label Apr 25, 2024

zhongnansu marked this pull request as ready for review April 25, 2024 21:18

zhongnansu requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT and xinruiba as code owners April 25, 2024 21:18

zhongnansu added the v2.14.0 label Apr 25, 2024

BionIT previously approved these changes Apr 25, 2024

View reviewed changes

huyaboo approved these changes Apr 25, 2024

View reviewed changes

zhyuanqi reviewed Apr 25, 2024

View reviewed changes

add endpoint validation to create data source API

6121267

Signed-off-by: Zhongnan Su <szhongna@amazon.com>

zhongnansu dismissed BionIT’s stale review via 6121267 April 25, 2024 22:48

zhongnansu force-pushed the md-validate-endpoint branch from 809af74 to 6121267 Compare April 25, 2024 22:48

zhongnansu requested review from BionIT, zhyuanqi and huyaboo April 25, 2024 22:52

BionIT approved these changes Apr 25, 2024

View reviewed changes

xinruiba reviewed Apr 25, 2024

View reviewed changes

xinruiba approved these changes Apr 26, 2024

View reviewed changes

zhongnansu merged commit 0c114ce into opensearch-project:main Apr 26, 2024
57 of 58 checks passed

zhongnansu added the backport 2.x label Apr 26, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 26, 2024

[Backport 2.x] [MD] Add endpoint validation to create data source API #6646

Merged

zhongnansu pushed a commit that referenced this pull request Apr 26, 2024

add endpoint validation to create data source API (#6631) (#6646)

7dbb5a0

LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024

add endpoint validation to create data source API (opensearch-project…

72cea1b

…#6631) Signed-off-by: Zhongnan Su <szhongna@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MD] Add endpoint validation to create data source API #6631

[MD] Add endpoint validation to create data source API #6631

zhongnansu commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

codecov bot commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024

BionIT commented Apr 25, 2024

huyaboo left a comment

huyaboo Apr 25, 2024

zhongnansu Apr 25, 2024

zhyuanqi Apr 25, 2024

zhongnansu Apr 25, 2024

zhyuanqi Apr 25, 2024

zhongnansu Apr 25, 2024 •

edited

Loading

zhyuanqi commented Apr 25, 2024

zhongnansu commented Apr 25, 2024

xinruiba commented Apr 25, 2024 •

edited

Loading

zhongnansu commented Apr 25, 2024

xinruiba commented Apr 25, 2024

xinruiba Apr 25, 2024 •

edited

Loading

zhongnansu Apr 25, 2024

xinruiba Apr 25, 2024 •

edited

Loading

zhongnansu Apr 26, 2024 •

edited

Loading

xinruiba Apr 26, 2024

xinruiba Apr 26, 2024 •

edited

Loading

zhongnansu Apr 26, 2024

kgcreative Apr 26, 2024

[MD] Add endpoint validation to create data source API #6631

[MD] Add endpoint validation to create data source API #6631

Conversation

zhongnansu commented Apr 24, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Apr 24, 2024

❌ Empty Changelog Section

github-actions bot commented Apr 24, 2024

❌ Empty Changelog Section

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

github-actions bot commented Apr 25, 2024

❌ Empty Changelog Section

BionIT commented Apr 25, 2024

huyaboo left a 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

zhongnansu Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

zhyuanqi commented Apr 25, 2024

zhongnansu commented Apr 25, 2024

xinruiba commented Apr 25, 2024 • edited Loading

zhongnansu commented Apr 25, 2024

xinruiba commented Apr 25, 2024

xinruiba Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinruiba Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

zhongnansu Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinruiba Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongnansu commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

zhongnansu Apr 25, 2024 •

edited

Loading

xinruiba commented Apr 25, 2024 •

edited

Loading

xinruiba Apr 25, 2024 •

edited

Loading

xinruiba Apr 25, 2024 •

edited

Loading

zhongnansu Apr 26, 2024 •

edited

Loading

xinruiba Apr 26, 2024 •

edited

Loading