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

Feature: MDS client enhancement #706

Draft
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Apr 14, 2023

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@SuZhou-Joe SuZhou-Joe requested a review from a team April 14, 2023 07:43
@SuZhou-Joe SuZhou-Joe changed the title feat: update Feature: POC for MDS client. Apr 14, 2023
@SuZhou-Joe SuZhou-Joe marked this pull request as draft April 14, 2023 07:46
@SuZhou-Joe SuZhou-Joe changed the title Feature: POC for MDS client. Feature: MDS client enhancement Apr 20, 2023
@SuZhou-Joe SuZhou-Joe marked this pull request as ready for review April 20, 2023 02:29
@SuZhou-Joe SuZhou-Joe added v2.8.0 infra Changes to infrastructure, testing, CI/CD, pipelines, etc. labels Apr 20, 2023
@SuZhou-Joe SuZhou-Joe self-assigned this Apr 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #706 (6386623) into 2.x (4979092) will decrease coverage by 0.98%.
The diff coverage is n/a.

❗ Current head 6386623 differs from pull request most recent head 165b91c. Consider uploading reports for the commit 165b91c to get more accurate results

@@            Coverage Diff             @@
##              2.x     #706      +/-   ##
==========================================
- Coverage   63.37%   62.40%   -0.98%     
==========================================
  Files         341      337       -4     
  Lines       11545    11435     -110     
  Branches     2108     2211     +103     
==========================================
- Hits         7317     7136     -181     
- Misses       3654     3737      +83     
+ Partials      574      562      -12     

see 21 files with indirect coverage changes

RequestHandlerContext,
} from "opensearch-dashboards/server";

export interface IRequestHandlerContentWithDataSource extends RequestHandlerContext {
Copy link
Member

Choose a reason for hiding this comment

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

data source plugin registered client objects at https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source/server/plugin.ts#L99 , not sure if that can be reused

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point and plugin should add dataSource plugin as requiredPlugins, add this change into this PR.

* it is not a good practice to rewrite the method like this
* but JS does not provide a method to copy a class instance
*/
props.client.asScoped = function (request: DashboardRequestEnhancedWithContext): ILegacyScopedClusterClient {
Copy link
Member

@ruanyl ruanyl May 19, 2023

Choose a reason for hiding this comment

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

What do you think of using Proxy instead of overwriting the original method? Something like this:

    const proxy = new Proxy(legacyClient, {
      get(target, prop) {
        if (prop === 'asScoped') {
          return (request) => {
            if (/* data source id presents */) {
              return {
                callAsCurrentUser: async () => {/.../},
                callAsInternalUser: async () => {/.../},
              };
            }
            return target.asScoped(request);
          };
        }
        return Reflect.get(...arguments);
      },
    });
    // then use legacyClient proxy
    const indexService = new IndexService(proxy);

context: RequestHandlerContext,
request: OpenSearchDashboardsRequest
) => {
(request as DashboardRequestEnhancedWithContext)[`${props.pluginId}_context`] = context as IRequestHandlerContentWithDataSource;
Copy link
Member

Choose a reason for hiding this comment

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

I remember you mentioned the concern of putting _data_source_id_ in http headers and the challenge of moving it to url search parameters.

Since we modified the original request here anyway(though it looks a bit hacky as registerRouteHandlerContext is not intent for this). If we end up with modifying the request object, perhaps we can also do this to handle data source id in search parameters:

    core.http.registerRouteHandlerContext(`${props.pluginId}_MDS_CTX_SUPPORT`, (ctx, req) => {
      req[`${props.pluginId}_context`] = ctx;
      if (req.url.searchParams.has('__data_source_id__')) {
        // add data source id to request object so later access
        req[`${props.pluginId}_data_source_id`] = req.url.searchParams.get('__data_source_id__');
        // then we can remove it from search params so it won't bother router validators.
        req.url.searchParams.delete('__data_source_id__');
      }
      return {};
    });

@SuZhou-Joe SuZhou-Joe force-pushed the feature/mds-client branch from 9ea1a39 to 6386623 Compare May 30, 2023 02:58
@SuZhou-Joe SuZhou-Joe force-pushed the feature/mds-client branch from 6386623 to 165b91c Compare July 17, 2023 06:48
@SuZhou-Joe SuZhou-Joe marked this pull request as draft July 17, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Changes to infrastructure, testing, CI/CD, pipelines, etc. v2.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants