adding multi-tenancy to config api and master key related changes #3439

dhrubo-os · 2025-01-27T22:21:32Z

Description

[adding multi-tenancy to config api and master key related changes]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

codecov · 2025-01-28T04:35:44Z

Codecov Report

Attention: Patch coverage is 69.67213% with 37 lines in your changes missing coverage. Please review.

Project coverage is 80.45%. Comparing base (f28bb74) to head (6e94702).
Report is 181 commits behind head on main.

Files with missing lines	Patch %	Lines
...ml/action/tasks/CancelBatchJobTransportAction.java	40.00%	6 Missing ⚠️
...va/org/opensearch/ml/common/utils/StringUtils.java	0.00%	5 Missing ⚠️
.../opensearch/ml/engine/encryptor/EncryptorImpl.java	85.71%	4 Missing and 1 partial ⚠️
...ction/deploy/TransportDeployModelOnNodeAction.java	0.00%	4 Missing ⚠️
...org/opensearch/ml/rest/RestMLPredictionAction.java	0.00%	4 Missing ⚠️
...va/org/opensearch/ml/task/MLPredictTaskRunner.java	69.23%	4 Missing ⚠️
...rch/ml/action/config/GetConfigTransportAction.java	57.14%	2 Missing and 1 partial ⚠️
...n/java/org/opensearch/ml/model/MLModelManager.java	0.00%	3 Missing ⚠️
.../opensearch/ml/common/connector/HttpConnector.java	66.66%	1 Missing ⚠️
...search/ml/action/tasks/GetTaskTransportAction.java	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3439      +/-   ##
============================================
- Coverage     81.31%   80.45%   -0.87%     
- Complexity     6094     6723     +629     
============================================
  Files           573      599      +26     
  Lines         25268    29186    +3918     
  Branches       2666     3232     +566     
============================================
+ Hits          20547    23481    +2934     
- Misses         3601     4304     +703     
- Partials       1120     1401     +281

Flag	Coverage Δ
ml-commons	`80.45% <69.67%> (-0.87%)`	⬇️

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.

arjunkumargiri

Looks good. Please add missing unit test for lines missing code coverage.

arjunkumargiri · 2025-01-28T06:46:56Z

plugin/src/main/java/org/opensearch/ml/action/config/GetConfigTransportAction.java

    }

    @Override
    protected void doExecute(Task task, ActionRequest request, ActionListener<MLConfigGetResponse> actionListener) {
        MLConfigGetRequest mlConfigGetRequest = MLConfigGetRequest.fromActionRequest(request);
        String configId = mlConfigGetRequest.getConfigId();
+        String tenantId = mlConfigGetRequest.getTenantId();


Currently tenantId is not being used. Please add a comment that tenantId will be used as part of SDKClient migration.

b4sjoo · 2025-01-28T10:15:10Z

I do see a lot of place did not cover by UT according to codecov report. I know the release time line is quite tense but could you please mark them and add those later?

dbwiddis

LGTM with a question

dbwiddis · 2025-01-28T15:58:22Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

+                case TENANT_ID_FIELD:
+                    tenantId = parser.textOrNull();


Do we need the tenant ID field added to the config? We use a hash of the key as part of the document ID and never actually compare vs. this.

We had is here in the multi-tenancy branch, I will deep dive later (after merging all the code) if we can remove it from here.

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

arjunkumargiri

Code changes looks good. Seems build is failing due to flaky test. Please validate that build succeeds before merging.

jngz-es · 2025-01-28T20:21:16Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/encryptor/EncryptorImpl.java

@@ -48,50 +53,52 @@ public class EncryptorImpl implements Encryptor {
        "The ML encryption master key has not been initialized yet. Please retry after waiting for 10 seconds.";
    private ClusterService clusterService;
    private Client client;
-    private volatile String masterKey;
+    private final Map<String, String> tenantMasterKeys;


I don't get why we have different key for every tenant. Currently this key is shared on the cluster level and crosses users. It's only for the node internal use, doesn't matter with user information. If we did so, we would also need to have different key for every user.

Tenant isn't an user here. So every tenant will have their own separate master key which will be used to encrypt/decrypt their connector credentials. If we have only one master key, that'll be a security concern for multi-tenancy.

) * adding multi-tenancy to config api and master key related changes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 9846e6e)

) (#3444) * adding multi-tenancy to config api and master key related changes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 9846e6e) Co-authored-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os had a problem deploying to ml-commons-cicd-env January 27, 2025 22:23 — with GitHub Actions Failure

dhrubo-os force-pushed the multi_tenancy_config branch from 6b5ad0a to 9578ff1 Compare January 28, 2025 00:27

dhrubo-os changed the title ~~adding multi-tenancy to config api and master key related changes~~ [DRAFT] adding multi-tenancy to config api and master key related changes Jan 28, 2025

dhrubo-os had a problem deploying to ml-commons-cicd-env January 28, 2025 00:28 — with GitHub Actions Failure

dhrubo-os force-pushed the multi_tenancy_config branch 2 times, most recently from f897de2 to 4f70041 Compare January 28, 2025 01:21

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 01:23 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env January 28, 2025 01:23 — with GitHub Actions Failure

dhrubo-os marked this pull request as ready for review January 28, 2025 01:32

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners January 28, 2025 01:32

dhrubo-os changed the title ~~[DRAFT] adding multi-tenancy to config api and master key related changes~~ adding multi-tenancy to config api and master key related changes Jan 28, 2025

adding multi-tenancy to config api and master key related changes

4948ea7

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os had a problem deploying to ml-commons-cicd-env January 28, 2025 02:25 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 03:37 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 04:35 — with GitHub Actions Inactive

arjunkumargiri reviewed Jan 28, 2025

View reviewed changes

dbwiddis approved these changes Jan 28, 2025

View reviewed changes

dhrubo-os force-pushed the multi_tenancy_config branch from 4f70041 to 518f7a8 Compare January 28, 2025 17:19

dhrubo-os had a problem deploying to ml-commons-cicd-env January 28, 2025 17:21 — with GitHub Actions Failure

adding more unit tests

6e94702

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os force-pushed the multi_tenancy_config branch from 518f7a8 to 6e94702 Compare January 28, 2025 18:03

dhrubo-os had a problem deploying to ml-commons-cicd-env January 28, 2025 18:04 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 19:10 — with GitHub Actions Inactive

arjunkumargiri approved these changes Jan 28, 2025

View reviewed changes

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 19:46 — with GitHub Actions Inactive

b4sjoo approved these changes Jan 28, 2025

View reviewed changes

ylwu-amzn approved these changes Jan 28, 2025

View reviewed changes

jngz-es reviewed Jan 28, 2025

View reviewed changes

dhrubo-os added the backport 2.x label Jan 28, 2025

dhrubo-os merged commit 9846e6e into opensearch-project:main Jan 28, 2025
9 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 28, 2025

[Backport 2.x] adding multi-tenancy to config api and master key related changes #3444

Merged

dhrubo-os temporarily deployed to ml-commons-cicd-env January 28, 2025 20:45 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding multi-tenancy to config api and master key related changes #3439

adding multi-tenancy to config api and master key related changes #3439

dhrubo-os commented Jan 27, 2025

codecov bot commented Jan 28, 2025 •

edited

Loading

arjunkumargiri left a comment

arjunkumargiri Jan 28, 2025

b4sjoo commented Jan 28, 2025

dbwiddis left a comment

dbwiddis Jan 28, 2025

dhrubo-os Jan 28, 2025

arjunkumargiri left a comment

jngz-es Jan 28, 2025

dhrubo-os Jan 28, 2025

adding multi-tenancy to config api and master key related changes #3439

adding multi-tenancy to config api and master key related changes #3439

Conversation

dhrubo-os commented Jan 27, 2025

Description

Related Issues

Check List

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

arjunkumargiri left a comment

Choose a reason for hiding this comment

arjunkumargiri Jan 28, 2025

Choose a reason for hiding this comment

b4sjoo commented Jan 28, 2025

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Jan 28, 2025

Choose a reason for hiding this comment

dhrubo-os Jan 28, 2025

Choose a reason for hiding this comment

arjunkumargiri left a comment

Choose a reason for hiding this comment

jngz-es Jan 28, 2025

Choose a reason for hiding this comment

dhrubo-os Jan 28, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2025 •

edited

Loading