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

configure new ML plugin actions #1182

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Conversation

ylwu-amzn
Copy link
Contributor

Signed-off-by: Yaliang Wu [email protected]

Description

Configure new plugin actions for 2.4 release.

Category

Maintenance

Why these changes are required?

We need these actions for 2.4 release.

What is the old behavior before changes and new behavior after changes?

Old: no these actions
New: add these actions, so user can configure custom permission role

Issues Resolved

Resolve #1181

Testing

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

@ylwu-amzn ylwu-amzn requested a review from a team November 2, 2022 18:07
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #1182 (fe07f57) into main (24807bb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1182   +/-   ##
=======================================
  Coverage   71.78%   71.78%           
=======================================
  Files          88       88           
  Lines        2027     2027           
  Branches      269      269           
=======================================
  Hits         1455     1455           
  Misses        509      509           
  Partials       63       63           
Impacted Files Coverage Δ
public/apps/configuration/constants.tsx 85.71% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Zhangxunmt
Zhangxunmt previously approved these changes Nov 2, 2022
Copy link

@Zhangxunmt Zhangxunmt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Is there documentation around the new features that are being permissioned that would be helpful to read up on? From the new names structure is hard for me to understand the relationship between the permissions and their granularity.

e.g.

Added a new model metadata, automatically created on new models, can be manually created via an API call to create_model_meta. If the user has access to the model they can read the metadata. Only users with permissions can call the API to manually create the metadata.

From Fake Feature Documentation

public/apps/configuration/constants.tsx Show resolved Hide resolved
'cluster:admin/opensearch/ml/execute',
'cluster:admin/opensearch/ml/forward',
Copy link
Member

Choose a reason for hiding this comment

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

Both forward/syncup seem like they apply to something rather than a top level permission, is this expected?

rbhavna
rbhavna previously approved these changes Nov 2, 2022
@cliu123 cliu123 added v2.4.0 'Issues and PRs related to version v2.4.0' backport 2.x backport to 2.x branch backport 2.4 backport to 2.4 branch labels Nov 2, 2022
cliu123
cliu123 previously approved these changes Nov 2, 2022
@peternied
Copy link
Member

[Offline Recap] FYI @ylwu-amzn

More changes might be needed to the backend

This change does not change any default permissions settings or register new permissions associated with roles or action groups. It only modifies the permissions that are displayed in the UI for the security plugin for admins to add/remove permissions in a couple of flows.

If default permissions, roles, action groups, need to be changed those need to happen in the security backend's config files [1].

Permissions should only be created that impact users experience

There is a permission in the current pull request cluster:admin/opensearch/ml/forward that a cluster admin would never allow or not allow. This action name might need to be changed to start with 'internal' so it does not have any access checks applied to it - it is an internal action.

Testing should be done with the security plugin

There are a couple of comments where there is a question of functionality - I am at a disadvantage answer these questions as the security plugin has not changed its model, but it sounds like considerable work has been done inside ML for its new features. In order to know if those features will work correctly or you'll need to see how you've changed the contract of the systems.

[1] https://github.com/opensearch-project/security/tree/main/config

@peternied
Copy link
Member

To get more background on setting up for using with Security plugin, see this https://github.com/opensearch-project/security#onboarding-new-apis

@ylwu-amzn
Copy link
Contributor Author

ylwu-amzn commented Nov 3, 2022

More changes might be needed to the backend

No need, we have configured reserved roles before opensearch-project/security#1654

Permissions should only be created that impact users experience

Agree, these internal actions were removed.

Testing should be done with the security plugin

Yes , we have done test with security backend plugin (I guess @peternied knows better than me, security dashboard plugin is not necessary when do pen test as security plugin provides APIs ). This PR is just add new actions to security dashboard plugin, so user can configure action easily on UI.

@peternied
Copy link
Member

More changes might be needed to the backend

No need, we have configured reserved roles before opensearch-project/security#1654

@ylwu-amzn Thanks for the confirmation, you are good to merge. Please create an issue to document these permissions - I couldn't find any references to those names within the OpenSearch project.

@peternied peternied merged commit 1ae8e24 into opensearch-project:main Nov 3, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 3, 2022
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Chang Liu <[email protected]>
(cherry picked from commit 1ae8e24)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 3, 2022
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Chang Liu <[email protected]>
(cherry picked from commit 1ae8e24)
peternied pushed a commit that referenced this pull request Nov 3, 2022
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Chang Liu <[email protected]>
(cherry picked from commit 1ae8e24)
peternied pushed a commit that referenced this pull request Nov 3, 2022
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Chang Liu <[email protected]>
(cherry picked from commit 1ae8e24)
@ylwu-amzn
Copy link
Contributor Author

More changes might be needed to the backend

No need, we have configured reserved roles before opensearch-project/security#1654

@ylwu-amzn Thanks for the confirmation, you are good to merge. Please create an issue to document these permissions - I couldn't find any references to those names within the OpenSearch project.

Yes, tech writer is working on these documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch backport 2.4 backport to 2.4 branch v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add new ml-commons transport actions
6 participants