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

Remove special handling for do_not_fail_on_forbidden on cluster actions #4486

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Jun 24, 2024

The special handling for do_not_fail_on_forbidden on cluster actions does not provide any benefits - rather it just causes some inconsistent behavior. See #4485 for the details.

This is part of the work done for #3870

Description

  • Category: Enhancement and bug fix
  • Why these changes are required? - The present behavior is inconsistent and confusing. Additionally, it does unnecessary computations on index sets, which can potentially decrease action throughput
  • What is the old behavior before changes and new behavior after changes? - Both clusters with do_not_fail_on_forbidden set to true and to false will need the same set of privileges for multi actions and scroll actions. The need to specify indices:data/read/mget, indices:data/read/msearch, indices:data/read/mtv, indices:data/read/scroll and indices:data/read/search/template/render as index permissions in the roles config will go away.

Issues Resolved

Testing

  • Tests in DoNotFailOnForbiddenTests have been adapted.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR - not necessary
  • 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.

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.24%. Comparing base (faa8bf9) to head (fba55ac).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4486      +/-   ##
==========================================
- Coverage   65.27%   65.24%   -0.03%     
==========================================
  Files         313      313              
  Lines       22058    22045      -13     
  Branches     3563     3559       -4     
==========================================
- Hits        14398    14384      -14     
+ Misses       5889     5887       -2     
- Partials     1771     1774       +3     
Files Coverage Δ
...earch/security/privileges/PrivilegesEvaluator.java 72.29% <ø> (+0.12%) ⬆️

... and 5 files with indirect coverage changes

@nibix nibix force-pushed the priv-eval-cluster-vs-index-actions branch 2 times, most recently from 8aaf278 to 09bb8c5 Compare June 24, 2024 15:14
This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485

Signed-off-by: Nils Bandener <[email protected]>
@nibix
Copy link
Collaborator Author

nibix commented Jun 24, 2024

To avoid any confusion: Yes, this is already complete. The only code-change not related to tests is the removal of the do_not_fail_on_forbidden special handling for cluster actions.

Copy link
Member

@DarshitChanpura DarshitChanpura 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
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

LGTM, re-running CI, cuz that seems like fail due to flaky

@willyborankin willyborankin merged commit f37399e into opensearch-project:main Jun 25, 2024
41 of 42 checks passed
@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Jun 25, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 25, 2024
…ns (#4486)

Signed-off-by: Nils Bandener <[email protected]>
(cherry picked from commit f37399e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do_not_fail_on_forbidden mode introduces inconsistencies for mget, msearch and similar actions
4 participants