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

model access control documentation #966

Merged

Conversation

rbhavna
Copy link
Collaborator

@rbhavna rbhavna commented Jun 8, 2023

Description

model access control documentation

Issues Resolved

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.

Signed-off-by: Bhavana Ramaram <[email protected]>
@@ -0,0 +1,652 @@
# Model Access Control

In the present setting, on a security enabled OpenSearch cluster, we can limit users to certain actions by mapping them to relevant permissions and permission roles. For example, we may want some users to only search and view ml models while allow others to register and deploy them.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"we can limit users..." -> "admin can limit user..."

"we may want some users... " -> "admin can grant some user access to .."

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #966 (06d58f2) into 2.x (e6dabc0) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                2.x     #966      +/-   ##
============================================
- Coverage     82.32%   82.26%   -0.06%     
+ Complexity     1918     1916       -2     
============================================
  Files           149      149              
  Lines          7506     7506              
  Branches        750      750              
============================================
- Hits           6179     6175       -4     
- Misses          987      990       +3     
- Partials        340      341       +1     
Flag Coverage Δ
ml-commons 82.26% <ø> (-0.06%) ⬇️

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

see 1 file with indirect coverage changes


- `public`: All users who have access to the cluster can access this model group.
- `private`: Only the model owner or an admin user can access this model group.
- `restricted`: The owner, an admin user, or any user who shares one of the model group's backend roles can access any model in this model group. When creating a `restricted` model group, the owner must attach one or more of the owner's backend roles to the model. For a user to be able to access a model group, one of the user's backend roles must match one of the model group's backend roles. This grants the user permissions associated with the user role to which that backend role is mapped. For example, if the backend role is mapped to the `ml_full_access` user role, users with this role can use the Register, Deploy, Predict, Delete, Search and Get Model APIs on any model in this model group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"This grants the user permissions associated with the user role to which that backend role is mapped. For example, if the backend role is mapped to the ml_full_access user role, users with this role can use the Register, Deploy, Predict, Delete, Search and Get Model APIs on any model in this model group."

Seems not related with restricted. Remove these or move to other part?


If a user tries to register/deploy/undeploy/predict/delete/search/get model versions, first we check if the user has access to the model. Otherwise, we throw exception saying user does not have access to the model specified.

A user is considered a model _owner_ when he/she creates a new model group and register its first version. When a model owner creates a model group, the owner can specify one of the following _access modes_ for this model group:
Copy link
Collaborator

Choose a reason for hiding this comment

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

A user is considered a model owner when he/she creates a new model group and register its first version.

This sentence says user need to create new group and register first version. What if user register model version directly to a model group? Will that user be model owner of the model version ?

Copy link
Collaborator

@ylwu-amzn ylwu-amzn Jun 8, 2023

Choose a reason for hiding this comment

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

If I understand correctly model owner is something related with model group. If a user register a model group , the user will be the owner of the model group.

Let's user "model group" and "model version" to avoid confusion?

If I understand correctly, we can change to
"When user create a model group, they can specify one of the following access modes. The user who create the model group will be the "model owner" to the model group and all model versions in the model group."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah so ideally we expect the owner to create the first version. But maybe its possible, the owner might not create the first version. Will rephrase it

We have implemented backend role-based access in which members of the same back-end roles will be able to access each other’s resources. This is similar to alerting and anomaly detection plugin [backend role based access](https://opensearch.org/docs/latest/observing-your-data/alerting/security/#advanced-limit-access-by-backend-role). Additionally we also let users define an ml resource as public or private. This advanced security access is available only on a cluster with [Security plugin](https://opensearch.org/docs/latest/security/index/).

Note:
- Model access control is experimental feature. If you see any bug or have any suggestion, feel free to cut Github issue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be add a link for the github issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Model access control is an experimental feature


## Setup

For Backend Role based security, it is not enabled by default. To enable/disable it, customers can toggle elasticsearch cluster setting shown below. If this setting is not enabled, a resource can be accessed by anyone.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, to create a model group, do we need to enable this setting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not necessary for creating model group. This setting is only to enable access control

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I just tried to create a model group:

POST /_plugins/_ml/model_groups/_register
{
    "name": "test_model_group_public",
    "description": "This is a public model group",
    "model_access_mode": "public"
}

And received error :

{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body"
  },
  "status": 400
}

But after enabling the plugins.ml_commons.model_access_control_enabled then I was able to create the model group:

{
  "model_group_id": "rJTkoYgBU4BPBnjEiV4f",
  "status": "CREATED"
}

Copy link
Collaborator Author

@rbhavna rbhavna Jun 9, 2023

Choose a reason for hiding this comment

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

So here, in this case, all model groups are public by default. We do not want user to specify the model_access_mode when the setting/security is disabled. Try this without the setting enabled. It would work

POST /_plugins/_ml/model_groups/_register
{
    "name": "test_model_group_public",
    "description": "This is a public model group"
}


If a user tries to register/deploy/undeploy/predict/delete/search/get model versions, first we check if the user has access to the model. Otherwise, we throw exception saying user does not have access to the model specified.

A user is considered a model _owner_ when he/she creates a new model group and register its first version. When a model owner creates a model group, the owner can specify one of the following _access modes_ for this model group:
Copy link
Collaborator

Choose a reason for hiding this comment

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

register its first version : how about register the first version of the model?


## Registering a model group:

User should use the `_register` endpoint to register a model group. User can register a model group with a `public`, `private`, or `restricted` access mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

_register api endpoint

| `name` | String | The model group name. Required. |
| `description` | String | The model group description. Optional. |
| `model_access_mode` | String | The access mode for this model. Valid values are `public`, `private`, and `restricted`. When this parameter is set to `restricted`, user must specify either `backend_roles` or `add_all_backend_roles`, but not both. Optional. Default is `restricted`. |
| `backend_roles` | Array | A list of the model owner's backend roles to add to the model. Can be specified only if the `model_access_mode` is `restricted`. Cannot be specified at the same time as `add_all_backend_roles`. Optional. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be specified only if the model_access_mode is restricted. --> Required only if the model_access_mode is restricted ?


### Response fields

The following table lists the available request fields.
Copy link
Collaborator

Choose a reason for hiding this comment

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

available response fields.


- To create a restrcited Model Group: User should set `model_access_mode` field to `restricted`

When registering a restricted model group, user must attach one or more of their backend roles to the model group using one but not both of the following methods:
Copy link
Collaborator

Choose a reason for hiding this comment

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

using any of the following methods?

"backend_roles" : ["IT"]
}
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

May be add a comment here: "Or we can specify add_all_backend_roles instead of backend_roles?

### Path and HTTP method

```
PUT /_plugins/_ml/model_groups/<model_group_id>_update
Copy link
Collaborator

Choose a reason for hiding this comment

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

UT /_plugins/_ml/model_groups/<model_group_id>/_update

"framework_type": "sentence_transformers",
"all_config": "{\"_name_or_path\":\"nreimers/MiniLM-L6-H384-uncased\",\"architectures\":[\"BertModel\"],\"attention_probs_dropout_prob\":0.1,\"gradient_checkpointing\":false,\"hidden_act\":\"gelu\",\"hidden_dropout_prob\":0.1,\"hidden_size\":384,\"initializer_range\":0.02,\"intermediate_size\":1536,\"layer_norm_eps\":1e-12,\"max_position_embeddings\":512,\"model_type\":\"bert\",\"num_attention_heads\":12,\"num_hidden_layers\":6,\"pad_token_id\":0,\"position_embedding_type\":\"absolute\",\"transformers_version\":\"4.8.2\",\"type_vocab_size\":2,\"use_cache\":true,\"vocab_size\":30522}"
},
"url": "https://github.com/ylwu-amzn/ml-commons/blob/2.x_custom_m_helper/ml-algorithms/src/test/resources/org/opensearch/ml/engine/algorithms/text_embedding/all-MiniLM-L6-v2_torchscript_sentence-transformer.zip?raw=true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not using the pre-trained models as we discussed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the catch. I used the old request accidentlly

"last_updated_time": 1684362571300,
"name": "model_group_test",
"description": "This is an example description",
"tags": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have tags in 2.8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Accidentally used the old response. Will change it


{
"name": "all-MiniLM-L6-v2",
"version": "1.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

version is not needed here, right? You can explain more about version part

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the auto incrementing of the version?

* A user can make updates to a model group to which he/she has access which is determined by the access mode of the model group.
* In a model_access_control enabled cluster, the model group owner or an admin user can update all fields. Whereas, any other user who shares one or more backend roles with the model group can update the `name` and `description` fields only.
* In a security/model_access_control disabled cluster, any user can make updates to any model group but cannot specify any of the access fields.
* Consider a model group created by User1 with IT backend_role set to it. Since user1 is the owner, he/she will be able to update all the fields of the model group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems line 188 already covers line 190, 191
Line 187 covers line 192?

Line 190, 191, 192 are describing an example. Remove these lines or move these lines out of bulleted list?

* If User2 tries to send update request to the same model group, only name, and description will be allowed to be updated but not other fields because user2 has access to the model group but is not the owner/admin.
* If User3 tries to send update request, exception will be thrown because user3 has no matching backend_roles of the model group and therefore has no access to it at all.

Similar to registering, there are other checks for updating a model group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These exceptions seems not the most important things for most users who are using this feature in correct way. How about we add a new section for all checkings and exceptions like register and update model group? So we can keep the doc clean and tight for most happy cases. Otherwise, user have to read through a long document to start use, they may be lost in too much details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I have removed the exception cases from this doc. As you mentioned, this might cause unnecessary confusion to the user. Just made sure that user knows which field should be specified in what case




## Search model versions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some example search request? Like how to search all model versions in a model group with model group id




## Deploy/Undeploy/Predict/Delete/Get model version
Copy link
Collaborator

@ylwu-amzn ylwu-amzn Jun 8, 2023

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah so there is basically no change to API requests here. Maybe we can add a link

Signed-off-by: Bhavana Ramaram <[email protected]>
@rbhavna rbhavna merged commit 199261b into opensearch-project:2.x Jun 9, 2023
zane-neo pushed a commit to zane-neo/ml-commons that referenced this pull request Aug 28, 2023
* model access control documentation

Signed-off-by: Bhavana Ramaram <[email protected]>
zane-neo added a commit that referenced this pull request Aug 29, 2023
* model access control documentation

Signed-off-by: Bhavana Ramaram <[email protected]>
Co-authored-by: Bhavana Ramaram <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants