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.

Sign up for GitHub

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

Jump to bottom

changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal #1897

Merged
merged 4 commits into from
Jan 23, 2024
Merged

changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal #1897

merged 4 commits into from
Jan 23, 2024

Conversation

rbhavna
Copy link
Collaborator

@rbhavna rbhavna commented Jan 22, 2024

Description

changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal

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.

@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 18:54 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:54 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:54 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 18:54 — with GitHub Actions Failure
@rbhavna
remove debugging logs
3e5281b 
Signed-off-by: Bhavana Ramaram <[email protected]>
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 18:58 — with GitHub Actions Failure
ylwu-amzn
ylwu-amzn previously approved these changes Jan 22, 2024
View reviewed changes
arjunkumargiri
arjunkumargiri reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@arjunkumargiri arjunkumargiri left a comment

Choose a reason for hiding this comment

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

Add a unit test with non-LDAP super admin name and check if it validates using adminUserNames

plugin/src/main/java/org/opensearch/ml/utils/RestActionUtils.java Outdated
Object userObject = client.threadPool().getThreadContext().getTransient(OPENDISTRO_SECURITY_USER);
if (userObject == null)
return false;
ObjectMapper objectMapper = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than creating an ObjectMapper for every execution can this be part of class variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense

plugin/src/main/java/org/opensearch/ml/utils/RestActionUtils.java Outdated
Comment on lines 264 to 265
boolean isAdmin = adminUsernames.contains(dn);
return isAdmin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can directly adminUsernames.contains(dn); rather than first assigning to a variable?

plugin/src/main/java/org/opensearch/ml/utils/RestActionUtils.java Outdated
@@ -73,7 +77,10 @@ public class RestActionUtils {
public static final String OPENDISTRO_SECURITY_CONFIG_PREFIX = "_opendistro_security_";
public static final String OPENDISTRO_SECURITY_SSL_PRINCIPAL = OPENDISTRO_SECURITY_CONFIG_PREFIX + "ssl_principal";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed, this is no longer used.

@codecov Codecov
Copy link

codecov bot commented Jan 22, 2024 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (54c788a) 82.61% compared to head (8100c18) 82.62%.

Files Patch % Lines
.../java/org/opensearch/ml/utils/RestActionUtils.java 75.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #1897   +/-   ##
=========================================
  Coverage     82.61%   82.62%           
- Complexity     5383     5387    +4     
=========================================
  Files           521      521           
  Lines         21715    21719    +4     
  Branches       2210     2211    +1     
=========================================
+ Hits          17940    17945    +5     
+ Misses         2878     2877    -1     
  Partials        897      897
Flag Coverage Δ
ml-commons 82.62% <75.00%> (+<0.01%) ⬆️

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.

@rbhavna
addressing comments
fa58183 
Signed-off-by: Bhavana Ramaram <[email protected]>
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 19:30 — with GitHub Actions Failure
@rbhavna
modify unit test
8100c18 
Signed-off-by: Bhavana Ramaram <[email protected]>
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 21:30 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 21:30 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 21:30 — with GitHub Actions Failure
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 21:30 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 22:52 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 22:52 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 22:52 — with GitHub Actions Failure
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 22:52 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 23:38 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 23:38 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env January 22, 2024 23:38 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env January 22, 2024 23:38 — with GitHub Actions Inactive
@rbhavna rbhavna merged commit de59efc into opensearch-project:main Jan 23, 2024
8 of 12 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 23, 2024
@rbhavna @github-actions
changes to hidden model code to use OPENDISTRO_SECURITY_USER instad o…
cfb5381 
…f ssl principal (#1897)

* changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal

Signed-off-by: Bhavana Ramaram <[email protected]>
(cherry picked from commit de59efc)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 23, 2024
Closed
rbhavna added a commit to rbhavna/ml-commons that referenced this pull request Jan 23, 2024
@rbhavna
changes to hidden model code to use OPENDISTRO_SECURITY_USER instad o…
502830c 
…f ssl principal (opensearch-project#1897)

* changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal

Signed-off-by: Bhavana Ramaram <[email protected]>
(cherry picked from commit de59efc)
@rbhavna rbhavna mentioned this pull request Jan 23, 2024
Merged
5 tasks
ylwu-amzn pushed a commit that referenced this pull request Jan 23, 2024
@rbhavna
changes to hidden model code to use OPENDISTRO_SECURITY_USER instad o…
25cf122 
…f ssl principal (#1897) (#1900)

* changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal

Signed-off-by: Bhavana Ramaram <[email protected]>
(cherry picked from commit de59efc)
austintlee pushed a commit to austintlee/ml-commons that referenced this pull request Mar 19, 2024
@rbhavna @austintlee
changes to hidden model code to use OPENDISTRO_SECURITY_USER instad o…
744082a 
…f ssl principal (opensearch-project#1897)

* changes to hidden model code to use OPENDISTRO_SECURITY_USER instad of ssl principal

Signed-off-by: Bhavana Ramaram <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjunkumargiri arjunkumargiri arjunkumargiri approved these changes

@b4sjoo b4sjoo b4sjoo approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

Assignees
No one assigned
Labels
backport 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rbhavna @ylwu-amzn @b4sjoo @arjunkumargiri