Model access control dev rebase #928

b4sjoo · 2023-05-30T21:34:28Z

Description

Rebase the model-access-control-dev branch to 2.x

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

* Backport changes to model-access-control feature branch (opensearch-project#837) * rename model meta/chunk API (opensearch-project#827) Signed-off-by: Yaliang Wu <[email protected]> * Change the ziputil dependency to fix a potential security concern (opensearch-project#824) Signed-off-by: Yaliang Wu <[email protected]> * add text docs ML input (opensearch-project#830) Signed-off-by: Yaliang Wu <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> Co-authored-by: Sicheng Song <[email protected]> * add model group (opensearch-project#840) * add model group Signed-off-by: Yaliang Wu <[email protected]> * rename create model group as register model group Signed-off-by: Yaliang Wu <[email protected]> * fix class name in build.gradle Signed-off-by: Yaliang Wu <[email protected]> * remove unused code; stash thread context Signed-off-by: Yaliang Wu <[email protected]> * exclude class for low coverage Signed-off-by: Yaliang Wu <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> * access validation for register/update model-group, register/get/delete/deploy/predict model Signed-off-by: Bhavana Ramaram <[email protected]> * changes to security utils and register/search model group Signed-off-by: Bhavana Ramaram <[email protected]> * update last_updated_time in model group when new version added Signed-off-by: Bhavana Ramaram <[email protected]> * fix undeploy model action Signed-off-by: Bhavana Ramaram <[email protected]> * fix format violations Signed-off-by: Bhavana Ramaram <[email protected]> * rebased to 2.x and prediction/search API fix Signed-off-by: Bhavana Ramaram <[email protected]> * fix rebase conflicts Signed-off-by: Bhavana Ramaram <[email protected]> * new delete model group API Signed-off-by: Bhavana Ramaram <[email protected]> * fix formal violations Signed-off-by: Bhavana Ramaram <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Bhavana Ramaram <[email protected]> Co-authored-by: Yaliang Wu <[email protected]> Co-authored-by: Sicheng Song <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* Add model group & model search access control Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* fix undeploy model API Signed-off-by: Yaliang Wu <[email protected]> * add model group id when upload pre-trained model; support undeploy one model when filter by backend role enabled Signed-off-by: Yaliang Wu <[email protected]> * refactor code Signed-off-by: Yaliang Wu <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* fix some bugs Signed-off-by: Yaliang Wu <[email protected]> * add stash context try block Signed-off-by: Yaliang Wu <[email protected]> * fix anyone can update description of private model group Signed-off-by: Yaliang Wu <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…earch-project#909) * Fix no corresponding backend roles user can update model issue Signed-off-by: Zan Niu <[email protected]> * Remove private model check and merge this case into has backend role check Signed-off-by: Zan Niu <[email protected]> * Add admin check for access control change to make admin pass Signed-off-by: Zan Niu <[email protected]> * Change model access mode not mandatory in registering, rename model access identifier to model access mode Signed-off-by: Zan Niu <[email protected]> --------- Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…n issue; 3.fix failure UTs (opensearch-project#910) Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…roject#912) * remove tags and add unit tests for register model group Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…opensearch-project#913) Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…ch-project#914) Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…bugs (opensearch-project#916) * Minor refactor for search model group transport action Signed-off-by: Zan Niu <[email protected]> * Add ITs and UTs and refactor model group search code and fixed minor bugs Signed-off-by: Zan Niu <[email protected]> --------- Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* fix metric correlation Signed-off-by: Yaliang Wu <[email protected]> * fix upload model via local file bug Signed-off-by: Yaliang Wu <[email protected]> --------- Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

…#918) * fix model meta API and add update model group UTs Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* Fix model search IT bugs Signed-off-by: Zan Niu <[email protected]> * Remove Ignore flag on test method Signed-off-by: Zan Niu <[email protected]> * Fix model search IT issues and removed the query type limitation Signed-off-by: Zan Niu <[email protected]> * Remove not suitable UT Signed-off-by: Zan Niu <[email protected]> * format code Signed-off-by: Zan Niu <[email protected]> --------- Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* Add undeploy models UTs Signed-off-by: Zan Niu <[email protected]> * format code Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

* bump 2.8 Signed-off-by: Jing Zhang <[email protected]> * disable codecov patch Signed-off-by: Jing Zhang <[email protected]> * address comments Signed-off-by: Jing Zhang <[email protected]> --------- Signed-off-by: Jing Zhang <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Signed-off-by: Sicheng Song <[email protected]>

codecov · 2023-05-30T22:02:51Z

Codecov Report

Merging #928 (df49851) into 2.x (6763941) will decrease coverage by 2.69%.
The diff coverage is 81.37%.

@@             Coverage Diff              @@
##                2.x     #928      +/-   ##
============================================
- Coverage     84.11%   81.42%   -2.69%     
- Complexity     1762     1894     +132     
============================================
  Files           139      149      +10     
  Lines          6779     7490     +711     
  Branches        676      742      +66     
============================================
+ Hits           5702     6099     +397     
- Misses          778     1059     +281     
- Partials        299      332      +33

Flag	Coverage Δ
ml-commons	`81.42% <81.37%> (-2.69%)`	⬇️

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

Impacted Files	Coverage Δ
...rg/opensearch/ml/client/MachineLearningClient.java	`100.00% <ø> (ø)`
...pensearch/ml/client/MachineLearningNodeClient.java	`96.07% <ø> (ø)`
...rithms/metrics_correlation/MetricsCorrelation.java	`36.07% <6.00%> (-46.45%)`	⬇️
.../action/undeploy/TransportUndeployModelAction.java	`10.20% <40.00%> (-68.75%)`	⬇️
...rch/ml/action/tasks/SearchTaskTransportAction.java	`72.72% <62.50%> (-27.28%)`	⬇️
...tion/prediction/TransportPredictionTaskAction.java	`76.74% <70.58%> (-23.26%)`	⬇️
...n/java/org/opensearch/ml/model/MLModelManager.java	`76.28% <71.54%> (-1.01%)`	⬇️
...h/ml/action/deploy/TransportDeployModelAction.java	`80.88% <71.95%> (-3.68%)`	⬇️
...h/ml/action/models/DeleteModelTransportAction.java	`83.52% <80.64%> (+2.70%)`	⬆️
...opensearch/ml/helper/ModelAccessControlHelper.java	`81.13% <81.13%> (ø)`
... and 24 more

... and 7 files with indirect coverage changes

ylwu-amzn · 2023-05-30T22:34:23Z

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

+            "    \"schema_version\": "+ML_MODEL_GROUP_INDEX_SCHEMA_VERSION+"\n" +
+            "  },\n" +
+            "  \"properties\": {\n" +
+            "    \""+MLModelGroup.MODEL_GROUP_NAME_FIELD+"\": {\n" +


Missed backend_roles field

"backend_roles": { "type": "text", "fields": { "keyword": { "type": "keyword", "ignore_above": 256 } } },

Thanks for the catch. I forgot adding it, I guess

ylwu-amzn · 2023-05-30T22:51:35Z

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

+@Getter
+public class MLModelGroup implements ToXContentObject {
+    public static final String MODEL_GROUP_NAME_FIELD = "name"; //name of the model group
+    // We use int type for version in first release 1.3. In 2.4, we changed to


Seems this comment is not for model group, model group is something new in 2.8, not released in 1.3

ylwu-amzn · 2023-05-30T22:53:48Z

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

+    public static final String BACKEND_ROLES_FIELD = "backend_roles"; //back_end roles as specified by the owner/admin
+    public static final String OWNER = "owner"; //user who creates/owns the model group
+
+    public static final String ACCESS = "access"; //assigned to public, private, or null when model group created


It's ok to use String here, but better to use the enum class ModelAccessMode.

yeah makes sense. Will change it later

ylwu-amzn · 2023-05-30T22:55:37Z

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

+        }
+        access = input.readOptionalString();
+        modelGroupId = input.readOptionalString();
+        createdTime = input.readOptionalInstant();


Not a bug, but is access, createdTime and lastUpdatedTime optional?

access was optional previously. Its mandatory now. And the times are optional in model. So just used a similar type here. We can actually make them all mandatory later

…arch-project#929) Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

ylwu-amzn · 2023-05-30T23:31:13Z

plugin/src/main/java/org/opensearch/ml/action/handler/MLSearchHandler.java

@@ -75,7 +153,7 @@ public static <T> ActionListener<T> wrapRestActionListener(ActionListener<T> act
                String errorMessage = generalErrorMessage;
                if (isBadRequest(e) || e instanceof MLException) {
                    errorMessage = e.getMessage();
-                } else if (cause != null && (isBadRequest(cause) || cause instanceof MLException)) {
+                } else if (isBadRequest(cause) || cause instanceof MLException) {


Why remove cause != null && ?

@zane-neo can help here

Throwables.getRootCause(e) never returns null, removing this won't cause any issue.

ylwu-amzn · 2023-05-30T23:35:23Z

plugin/src/main/java/org/opensearch/ml/action/prediction/TransportPredictionTaskAction.java

+        final User userInfo = user;
+
+        try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
+            mlModelManager.getModel(modelId, ActionListener.wrap(mlModel -> {


Not all users prefer to enable backend role filtering. If we always check access querying model and model group index, that may add overhead to the request.

User may concern about the latency overhead, especially for predict API. How about we skip access checking if user don't enable backend role filtering, at least for predict API?

This is some performance enhancement. We can do this later.

Yeah agreed, this can improve performance

dhrubo-os · 2023-05-30T23:02:00Z

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

+    public static final String MODEL_GROUP_ID_FIELD = "model_group_id"; //unique ID assigned to each model group
+    public static final String CREATED_TIME_FIELD = "created_time"; //model group created time stamp
+    public static final String LAST_UPDATED_TIME_FIELD = "last_updated_time"; //updated whenever a new model version is created
+    //SHA256 hash value of model content.


This comment seems oddly placed here?

Good catch. This should be removed

dhrubo-os · 2023-05-30T23:41:59Z

common/src/main/java/org/opensearch/ml/common/transport/undeploy/MLUndeployModelsRequest.java

+@Getter
+@FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE)
+@ToString
+public class MLUndeployModelsRequest extends MLTaskRequest {


Don't we need user information as input to see if user has permission to unload this model?

No actually, we get user information from the current context. Not required as input.

Signed-off-by: Sicheng Song <[email protected]>

(cherry picked from commit 6e1541a)

(cherry picked from commit 6e1541a) Signed-off-by: Bhavana Ramaram <[email protected]>

(cherry picked from commit 6e1541a) Signed-off-by: Bhavana Ramaram <[email protected]> Co-authored-by: Sicheng Song <[email protected]>

* Model access control dev rebase (#928) * Fix breaking changes from opensearch core Signed-off-by: zane-neo <[email protected]> * Decrease code coverage to pass github build Signed-off-by: zane-neo <[email protected]> * fix format issue Signed-off-by: zane-neo <[email protected]> --------- Signed-off-by: zane-neo <[email protected]> Co-authored-by: Sicheng Song <[email protected]>

rbhavna and others added 17 commits May 30, 2023 21:36

Model access control dev1 (opensearch-project#905)

8c9285e

* Add model group & model search access control Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

get 10k model group ids when search model (opensearch-project#908)

ca2737c

Signed-off-by: Yaliang Wu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

1. revert model content hash change; 2.fix task search with permissio…

e24e754

…n issue; 3.fix failure UTs (opensearch-project#910) Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

remove tags and add unit tests for register model group (opensearch-p…

d296fff

…roject#912) * remove tags and add unit tests for register model group Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

add access control to register version via local file and minor fixes (…

5ebb436

…opensearch-project#913) Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Change model_access_control to optional when register model (opensear…

d5b7d66

…ch-project#914) Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

fix model meta API and add update model group UTs (opensearch-project…

45631a3

…#918) * fix model meta API and add update model group UTs Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Add undeploy models UTs (opensearch-project#926)

1cbc374

* Add undeploy models UTs Signed-off-by: Zan Niu <[email protected]> * format code Signed-off-by: Zan Niu <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

Unit tests for model access control new classes (opensearch-project#927)

a6ec9b9

Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

b4sjoo force-pushed the model-access-control-dev-rebase branch from 8560347 to 4cbd86f Compare May 30, 2023 21:37

spotlessApply styling

6210255

Signed-off-by: Sicheng Song <[email protected]>

b4sjoo requested review from rbhavna, ylwu-amzn and jngz-es May 30, 2023 21:42

rbhavna previously approved these changes May 30, 2023

View reviewed changes

ylwu-amzn reviewed May 30, 2023

View reviewed changes

Add Search Model Group Rest Action unit tests and minor fixes (opense…

2115f54

…arch-project#929) Signed-off-by: Bhavana Ramaram <[email protected]> Signed-off-by: Sicheng Song <[email protected]>

b4sjoo dismissed rbhavna’s stale review via 2115f54 May 30, 2023 23:24

ylwu-amzn reviewed May 30, 2023

View reviewed changes

dhrubo-os reviewed May 30, 2023

View reviewed changes

b4sjoo requested review from zane-neo and rbhavna May 31, 2023 01:09

rbhavna previously approved these changes May 31, 2023

View reviewed changes

Add unit tests for new classes

df49851

Signed-off-by: Sicheng Song <[email protected]>

b4sjoo dismissed rbhavna’s stale review via df49851 May 31, 2023 01:42

rbhavna approved these changes May 31, 2023

View reviewed changes

ylwu-amzn approved these changes May 31, 2023

View reviewed changes

b4sjoo merged commit 6e1541a into opensearch-project:2.x May 31, 2023

rbhavna added the backport 2.8 label May 31, 2023

opensearch-trigger-bot bot pushed a commit that referenced this pull request May 31, 2023

Model access control dev rebase (#928)

aadb649

(cherry picked from commit 6e1541a)

opensearch-trigger-bot bot mentioned this pull request May 31, 2023

[Backport 2.8] Model access control dev rebase #931

Closed

rbhavna pushed a commit to rbhavna/ml-commons that referenced this pull request May 31, 2023

Model access control dev rebase (opensearch-project#928)

1708ad7

(cherry picked from commit 6e1541a) Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna added a commit that referenced this pull request May 31, 2023

Model access control dev rebase (#928) (#932)

b825ef0

(cherry picked from commit 6e1541a) Signed-off-by: Bhavana Ramaram <[email protected]> Co-authored-by: Sicheng Song <[email protected]>

martin-gaievski mentioned this pull request Jun 1, 2023

Adding model group id to integ tests opensearch-project/neural-search#189

Merged

2 tasks

zane-neo pushed a commit to zane-neo/ml-commons that referenced this pull request Aug 24, 2023

Model access control dev rebase (opensearch-project#928)

5a47d3f

zane-neo mentioned this pull request Aug 24, 2023

[Backport to main] Model access control dev rebase (#928) #1243

Merged

5 tasks

zane-neo pushed a commit to zane-neo/ml-commons that referenced this pull request Aug 24, 2023

Model access control dev rebase (opensearch-project#928)

8c6b8c3

pyek-bot mentioned this pull request Dec 6, 2024

[Refactor] Remove bloat due to unnecessary setup in test and add retry for potential flaky behavior due to timeout #3259

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model access control dev rebase #928

Model access control dev rebase #928

b4sjoo commented May 30, 2023

codecov bot commented May 30, 2023 •

edited

Loading

ylwu-amzn May 30, 2023

rbhavna May 30, 2023

ylwu-amzn May 30, 2023

rbhavna May 30, 2023

ylwu-amzn May 30, 2023

rbhavna May 30, 2023

ylwu-amzn May 30, 2023 •

edited

Loading

rbhavna May 30, 2023

ylwu-amzn May 30, 2023

rbhavna May 30, 2023

zane-neo May 31, 2023

ylwu-amzn May 30, 2023 •

edited

Loading

ylwu-amzn May 30, 2023

rbhavna May 30, 2023

dhrubo-os May 30, 2023

rbhavna May 30, 2023

dhrubo-os May 30, 2023

rbhavna May 31, 2023

Model access control dev rebase #928

Model access control dev rebase #928

Conversation

b4sjoo commented May 30, 2023

Description

Check List

codecov bot commented May 30, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn May 30, 2023 • 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

ylwu-amzn May 30, 2023 • 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

codecov bot commented May 30, 2023 •

edited

Loading

ylwu-amzn May 30, 2023 •

edited

Loading

ylwu-amzn May 30, 2023 •

edited

Loading