add version number to newly created datasource object #6178

ZilongX · 2024-03-18T07:47:43Z

Description

This change is the first step per RFC [RFC] Plugins Version Decoupling #5877 aiming at decouple the strict version constrains between OSD Core and Plugins
To make it happen this first step change is to include the data source version number in the newly created data-source object which would be consumed by plugins as part of the proof for determining version support

Issues Resolved

#6177

Screenshot

Unit Test passed

Demonstration Recording

Screen.Recording.2024-03-18.at.9.21.47.AM.mov

In the recording we demoed two scenarios:

When data source connection checks out, the data source version number is added to the data-source saved object
When data source connection doesn't work, the data source version number will be empty but will not block the creation of data-source saved object

Testing the changes

R1 - 028ae3c - update attribute key from version to dataSourceVersion

R2 - 9beabfb - add support and test coverage for custom auth type

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

Signed-off-by: Zilong Xia <[email protected]>

ZilongX · 2024-03-18T07:51:47Z

Will make a video record tomorrow morning ~

codecov · 2024-03-18T08:01:46Z

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 67.24%. Comparing base (db9806d) to head (79f59c4).

Files	Patch %	Lines
..._source/server/routes/fetch_data_source_version.ts	81.25%	3 Missing ⚠️
.../server/routes/data_source_connection_validator.ts	77.77%	1 Missing and 1 partial ⚠️
.../data_source_management/public/components/utils.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6178      +/-   ##
==========================================
- Coverage   67.26%   67.24%   -0.02%     
==========================================
  Files        3344     3345       +1     
  Lines       64796    64826      +30     
  Branches    10427    10432       +5     
==========================================
+ Hits        43584    43595      +11     
- Misses      18671    18689      +18     
- Partials     2541     2542       +1

Flag	Coverage Δ
Linux_1	`31.64% <ø> (ø)`
Linux_2	`55.45% <ø> (ø)`
Linux_3	`44.72% <76.66%> (+0.01%)`	⬆️
Linux_4	`35.05% <0.00%> (-0.01%)`	⬇️
Windows_1	`31.67% <ø> (-0.03%)`	⬇️
Windows_2	`55.41% <ø> (ø)`
Windows_3	`44.73% <76.66%> (+0.02%)`	⬆️
Windows_4	`35.05% <0.00%> (-0.01%)`	⬇️

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.

Flyingliuhub · 2024-03-18T16:45:05Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+
+        return response.ok({
+          body: {
+            dataSourceVersion,


should we use dataSourceversion in the attr? Thanks

yes, let me update the entry key in the attributes

updated entry key from version to dataSourceVersion

yujin-emma

LGTM

Signed-off-by: Zilong Xia <[email protected]>

Flyingliuhub · 2024-03-18T17:09:45Z

src/plugins/data_source/server/routes/fetch_data_source_version.test.ts

+import { CryptographyServiceSetup } from '../cryptography_service';
+import { registerFetchDataSourceVersionRoute } from './fetch_data_source_version';
+import { AuthType } from '../../common/data_sources';
+// eslint-disable-next-line @osd/eslint/no-restricted-paths


Should we remove this line 17 if we not comment out here. Thanks

need this line to pass the lint check for line 18, similar to https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source/server/routes/test_connection.test.ts#L17, we may need to find a better way for organizing the imports through

Flyingliuhub · 2024-03-18T17:14:32Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+  const authRegistry = await authRegistryPromise;
+  router.post(
+    {
+      path: '/internal/data-source-management/fetchDataSourceVersion',


If the user has enabled "compatibility mode" in their cluster, it will always be set to version 7.10.2. While this isn't a blocking issue, it's something we should probably take into consideration. Thank you.

Nice call out ! We've already included this concern in #6177 specifically as

Describe alternatives you've considered

We may need to switch to different api for example leveraging the nodes ones since the compatibility mode might be turned on on the data source domain, yet even that the version number would be locked to 7.10.2 which is supposed to be supported by plugins. We still need to further check with each plugin owners to finalized the use cases and this feature for now is more focusing on getting the version in place first.

While it would need more work including coordination with plugin owners we'll for sure keep it in mind and make the api switch if needed

BionIT · 2024-03-18T16:45:34Z

src/plugins/data_source/server/routes/data_source_connection_validator.ts

+    try {
+      // OpenSearch Serverless does not have version concept
+      if (
+        this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless


Since we will support other authentication types, I wonder how determine if the data source is serverless or on premise, or open search cluster, can we use the attributes.auth.credentials.service to determine the service type. Any suggestions @bandinib-amzn @xinruiba ?

Yeah would like to learn more on the serverless/premise side, actually we may need to add another attri entry say dataSource type maybe (OS | OS Serverless | On Premise)

We use attributes.auth.credentials.service to determine the service type for sigV4 dataSources.

This attribute, as far as I know, only has effect on how to deal with credentials in server side. But I don't think we add this server type as an attribute of our datasource object.

And for NoAuth and UserName&&PassWord auth, we don't set attributes.auth.credentials.service attribute, which means those dataSources will by default treated as server domain here.

I'm thinking if the server type is a must have, are we able to decide the service type based on the open-search endpoint format?

Like server endpoint (have es):
https://someurl.region.es.amazonaws.com/

Serverless endpoint (have aoss):
https://someurl.region.aoss.amazonaws.com/

Good call out ! Let me check further on above when adding the dataSourceType to data-source (in case it's needed)

Right now service is at authentication level. It is only being used when auth method is SigV4. We can move service parameter to data source level. As it sounds simple, it will need refactoring and testing of existing functionality. @ZilongX Can you create an issue to track?

BionIT · 2024-03-18T16:47:02Z

src/plugins/data_source/server/routes/data_source_connection_validator.ts

+      await this.callDataCluster
+        .info()
+        .then((response) => response.body)
+        .then((body) => {


since we use await already, do we still need to use then?

yes here just leverage the thenable to further trim the body keeping only required entries

It's redundant to use both await and then in the same function.

+1
nit:

clusterInfo = await this.callDataCluster.info(); if (clusterInfo?.statusCode === 200 && clusterInfo?.body) { dataSourceVersion = clusterInfo.version.number; } return dataSourceVersion;

BionIT · 2024-03-18T18:37:29Z

src/plugins/data_source/server/routes/data_source_connection_validator.ts

@@ -35,4 +35,27 @@ export class DataSourceConnectionValidator {
      throw createDataSourceError(e);
    }
  }
+
+  async fetchDataSourceVersion() {


Should this be called fetchDataSourceMetadata or do we want to create another API to fetch enabled plugins?

yes, this is just the first step to fetch the version number, next one is to fetch the installed plugins which would be consolidated into one method say fetchDataSourceMetadata with multiple api calls behind the scene

BionIT · 2024-03-18T18:40:32Z

src/plugins/data_source_management/public/types.ts

@@ -140,6 +140,7 @@ export interface DataSourceAttributes extends SavedObjectAttributes {
  title: string;
  description?: string;
  endpoint?: string;
+  dataSourceVersion?: string;


There is another DataSourceAttributes interface in data source plugin, should we update there as well?

yes but maybe later, that one seems to be leveraged for data source display page, this one is focusing on the saving part, we may need to display the version number on the datasource list page, with some UI updates as well, will need to check further.

BionIT · 2024-03-18T18:42:23Z

src/plugins/data_source_management/public/components/utils.ts

+export async function fetchDataSourceVersion(
+  http: HttpStart,
+  { endpoint, auth: { type, credentials } }: DataSourceAttributes,
+  dataSourceID?: string


should data source id be required?

I think it is optional because of test connection. Because if we do test connection before saving data source, we won't have data source id. But what will happen if it empty or undefined? Does it throw error?

just tested and it won't cause any exception :) further digging indicates a maybe checkid: schema.maybe(schema.string()), so an empty one won't cause trouble for now

BionIT · 2024-03-18T18:50:39Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+          id: schema.maybe(schema.string()),
+          dataSourceAttr: schema.object({
+            endpoint: schema.string(),
+            auth: schema.maybe(


for auth, I think we are introducing custom auth types, thus need to be similar to https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source/server/routes/test_connection.ts#L56, @xinruiba might have more context

Yes. @ZilongX can you add schema validation for custom auth type?

That is a good call out, thanks~
@ZilongX, you may also need to add the same schema validation to the "test_connect.ts" (@BionIT attached) to support registered Auth Type.

Nice call out, let me add the validation for custom auth type

BionIT · 2024-03-18T18:51:25Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+      },
+    },
+    async (context, request, response) => {
+      const { dataSourceAttr, id: dataSourceId } = request.body;


since data source id is optional, how do we handle undefined data source id here?

same as the test connection call , dataSourceId is actually needed for the OS API call but more on the MD internal side for validation to make sure the call is actually triggered by data source creation.

bandinib-amzn · 2024-03-18T19:02:57Z

src/plugins/data_source_management/public/components/utils.ts

+export async function fetchDataSourceVersion(
+  http: HttpStart,
+  { endpoint, auth: { type, credentials } }: DataSourceAttributes,
+  dataSourceID?: string


I think it is optional because of test connection. Because if we do test connection before saving data source, we won't have data source id. But what will happen if it empty or undefined? Does it throw error?

bandinib-amzn · 2024-03-18T19:07:59Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+          id: schema.maybe(schema.string()),
+          dataSourceAttr: schema.object({
+            endpoint: schema.string(),
+            auth: schema.maybe(


Yes. @ZilongX can you add schema validation for custom auth type?

bandinib-amzn · 2024-03-18T19:13:52Z

src/plugins/data_source/server/routes/fetch_data_source_version.test.ts

+      .expect(200);
+    expect(result.body).toEqual({ dataSourceVersion: '2.11.0' });
+  });
+});


Can you please add test case for cutsom auth as well?

Yes, let me add the validation and test for custom auth

Signed-off-by: Zilong Xia <[email protected]>

xinruiba · 2024-03-18T20:43:52Z

@ZilongX Thanks for the change.

It will be good if we can also add a video to test registered Auth Type.
Feel free to sync up offline~

Signed-off-by: ZilongX <[email protected]>

xinruiba · 2024-03-18T22:30:56Z

@ZilongX Thanks for the change.

It will be good if we can also add a video to test registered Auth Type. Feel free to sync up offline~

Synced up with @ZilongX offline and test registered Auth Type together, it works fine.

Signed-off-by: Zilong Xia <[email protected]>

bandinib-amzn · 2024-03-19T15:47:11Z

src/plugins/data_source/server/routes/data_source_connection_validator.ts

+    try {
+      // OpenSearch Serverless does not have version concept
+      if (
+        this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless


Right now service is at authentication level. It is only being used when auth method is SigV4. We can move service parameter to data source level. As it sounds simple, it will need refactoring and testing of existing functionality. @ZilongX Can you create an issue to track?

bandinib-amzn · 2024-03-19T15:54:11Z

src/plugins/data_source/server/routes/data_source_connection_validator.ts

+      await this.callDataCluster
+        .info()
+        .then((response) => response.body)
+        .then((body) => {


+1
nit:

clusterInfo = await this.callDataCluster.info(); if (clusterInfo?.statusCode === 200 && clusterInfo?.body) { dataSourceVersion = clusterInfo.version.number; } return dataSourceVersion;

bandinib-amzn · 2024-03-19T16:00:23Z

src/plugins/data_source/server/routes/fetch_data_source_version.ts

+    {
+      path: '/internal/data-source-management/fetchDataSourceVersion',
+      validate: {
+        body: schema.object({


nit: Now the code for request payload validation duplicate. We should be reusing the same code to remove duplicate code. I think we can define schema to separate parameter and use here and in test data connection.

bandinib-amzn · 2024-03-19T16:02:38Z

All checks are passing.

* add version number to newly created datasource object Signed-off-by: Zilong Xia <[email protected]> * update attribute key from version to dataSourceVersion Signed-off-by: Zilong Xia <[email protected]> * add support and test coverage for custom auth type Signed-off-by: Zilong Xia <[email protected]> * fix ciGroup3 test case suite Create Datasource Wizard Signed-off-by: Zilong Xia <[email protected]> --------- Signed-off-by: Zilong Xia <[email protected]> Signed-off-by: ZilongX <[email protected]> (cherry picked from commit daccae7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* add version number to newly created datasource object Signed-off-by: Zilong Xia <[email protected]> * update attribute key from version to dataSourceVersion Signed-off-by: Zilong Xia <[email protected]> * add support and test coverage for custom auth type Signed-off-by: Zilong Xia <[email protected]> * fix ciGroup3 test case suite Create Datasource Wizard Signed-off-by: Zilong Xia <[email protected]> --------- Signed-off-by: Zilong Xia <[email protected]> Signed-off-by: ZilongX <[email protected]> (cherry picked from commit daccae7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

add version number to newly created datasource object

2c900dd

Signed-off-by: Zilong Xia <[email protected]>

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

github-actions bot added the seasoned-contributor label Mar 18, 2024

ZilongX self-assigned this Mar 18, 2024

ZilongX added multiple datasource multiple datasource project v2.13.0 version decoupling labels Mar 18, 2024

ZilongX added the backport 2.x label Mar 18, 2024

Flyingliuhub reviewed Mar 18, 2024

View reviewed changes

yujin-emma approved these changes Mar 18, 2024

View reviewed changes

update attribute key from version to dataSourceVersion

028ae3c

Signed-off-by: Zilong Xia <[email protected]>

Flyingliuhub reviewed Mar 18, 2024

View reviewed changes

Flyingliuhub previously approved these changes Mar 18, 2024

View reviewed changes

xinruiba approved these changes Mar 18, 2024

View reviewed changes

BionIT reviewed Mar 18, 2024

View reviewed changes

bandinib-amzn reviewed Mar 18, 2024

View reviewed changes

add support and test coverage for custom auth type

9beabfb

Signed-off-by: Zilong Xia <[email protected]>

ZilongX dismissed Flyingliuhub’s stale review via 9beabfb March 18, 2024 20:22

Merge branch 'main' into main

febfdcb

Flyingliuhub previously approved these changes Mar 18, 2024

View reviewed changes

xinruiba self-requested a review March 18, 2024 21:15

xinruiba approved these changes Mar 18, 2024

View reviewed changes

Merge branch 'main' into main

1a4e629

Signed-off-by: ZilongX <[email protected]>

ZilongX dismissed Flyingliuhub’s stale review via 1a4e629 March 18, 2024 22:20

ZilongX and others added 3 commits March 18, 2024 22:54

fix ciGroup3 test case suite Create Datasource Wizard

154320f

Signed-off-by: Zilong Xia <[email protected]>

Merge branch 'main' into main

1da877f

Merge branch 'main' into main

79f59c4

BionIT approved these changes Mar 19, 2024

View reviewed changes

Flyingliuhub approved these changes Mar 19, 2024

View reviewed changes

bandinib-amzn approved these changes Mar 19, 2024

View reviewed changes

ZilongX merged commit daccae7 into opensearch-project:main Mar 19, 2024
68 checks passed

opensearch-trigger-bot bot mentioned this pull request Mar 19, 2024

[Backport 2.x] add version number to newly created datasource object #6198

Merged

seraphjiang mentioned this pull request Apr 13, 2024

[BUG] UI doesn't show version of distribution #731

Closed

add version number to newly created datasource object #6178

add version number to newly created datasource object #6178

Conversation

ZilongX commented Mar 18, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

ZilongX commented Mar 18, 2024

codecov bot commented Mar 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujin-emma 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

ZilongX Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinruiba Mar 18, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZilongX Mar 18, 2024 • edited Loading

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

xinruiba commented Mar 18, 2024

xinruiba commented Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bandinib-amzn commented Mar 19, 2024

ZilongX commented Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

ZilongX Mar 18, 2024 •

edited

Loading

xinruiba Mar 18, 2024 •

edited

Loading

ZilongX Mar 18, 2024 •

edited

Loading

xinruiba commented Mar 18, 2024 •

edited

Loading