Introduced setting for disabling denormalized privilege data structures

[nibix]

Description

This introduces the setting plugins.security.privileges_evaluation.precomputed_privileges.enabled which can be used to disable the creation of denormalized privilege data structures. It is enabled by default to provide the best action throughput. It can make sense to disable the setting when it is seen that the initialisation process takes so much time/resources that it negatively affects the cluster performance (like observed in #5464) . This come at the price of a reduced action throughput.

• Category: Enhancement
• Why these changes are required? Operational safety
• What is the old behavior before changes and new behavior after changes? No behavioral changes.

Testing

• Unit 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.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.62%. Comparing base (da7d66a) to head (8989ecf).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...privileges/dlsfls/AbstractRuleBasedPrivileges.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5465      +/-   ##
==========================================
+ Coverage   72.57%   72.62%   +0.05%     
==========================================
  Files         397      397              
  Lines       24562    24575      +13     
  Branches     3734     3736       +2     
==========================================
+ Hits        17825    17848      +23     
+ Misses       4904     4895       -9     
+ Partials     1833     1832       -1     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	84.98% <100.00%> (+0.01%)	⬆️
...vileges/actionlevel/RoleBasedActionPrivileges.java	96.15% <100.00%> (+0.77%)	⬆️
...privileges/dlsfls/AbstractRuleBasedPrivileges.java	97.65% <50.00%> (+0.02%)	⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cwperks]

The new settings looks ok to me. What would be the effort required to run more of the automated tests with plugins.security.privileges_evaluation.precomputed_privileges.enabled set to false?

[nibix]

What would be the effort required to run more of the automated tests with plugins.security.privileges_evaluation.precomputed_privileges.enabled set to false?

We already run all unit tests for RoleBasedActionPrivileges and the AbstractRuleBasedPrivileges family. Due to slight implementation differences, it is explicitly implemented in AbstractRuleBasedPrivileges. In RoleBasedActionPrivileges, it is just implicitly tested via the test param NON_STATEFUL:

security/src/integrationTest/java/org/opensearch/security/privileges/actionlevel/RoleBasedActionPrivilegesTest.java

Lines 792 to 796 in da7d66a

	enum Statefulness {
	STATEFUL,
	STATEFUL_LIMITED,
	NON_STATEFUL
	}

[shikharj05]

Thanks @nibix !

This come at the price of a reduced action throughput.

Do you think an async method should be implemented as a follow-up to still allow building of pre-computed privileges and keep most of the benefits from #3870 ? I see your comment here - did you see any challenges while trying an async method or is it just a pending TODO?

[nibix]

Do you think an async method should be implemented as a follow-up to still allow building of pre-computed privileges and keep most of the benefits from #3870 ? I see your comment here - did you see any challenges while trying an async method or is it just a pending TODO?

That's not a TODO, async processing is already in place. The only exception is currently the security config update process. However, from the test results so far, I can not really imagine that the CPU time is a real issue here. My tests showed so far maybe 1000 ms of processing time, which is of course not fast, but which also should not lead to timeout issues. I am still actively looking for the cause of the issue in #5464 but so far I could not really pinpoint anything.

[shikharj05]

Do you think an async method should be implemented as a follow-up to still allow building of pre-computed privileges and keep most of the benefits from #3870 ? I see your comment here - did you see any challenges while trying an async method or is it just a pending TODO?

That's not a TODO, async processing is already in place. The only exception is currently the security config update process. However, from the test results so far, I can not really imagine that the CPU time is a real issue here. My tests showed so far maybe 1000 ms of processing time, which is of course not fast, but which also should not lead to timeout issues. I am still actively looking for the cause of the issue in #5464 but so far I could not really pinpoint anything.

we can continue to discuss on the open issue #5464

[opensearch-trigger-bot]

The backport to 3.1 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-3.1 3.1
# Navigate to the new working tree
pushd ../.worktrees/security/backport-3.1
# Create a new branch
git switch --create backport/backport-5465-to-3.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8d5988014c1304210ba48ddff421c63f0306db3f
# Push it to GitHub
git push --set-upstream origin backport/backport-5465-to-3.1
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-3.1

Then, create a pull request where the base branch is 3.1 and the compare/head branch is backport/backport-5465-to-3.1.

[pranu2502]

Hey @nibix, I noticed that the backport attempt was unsuccessful. This appears to be related to PR #5374 , which hasn't yet been merged into the 3.1 branch.

Would you mind verifying if this PR was inadvertently missed? If so, can we backport it now?

cc: @cwperks

[nibix]

@pranu2502 good point, thank you for the heads up, I will take care ASAP.

[kumargu]

Do we know how PR #5374 got missed in 3.1 ?

[nibix]

See #5483

[nibix]

Do we know how PR #5374 got missed in 3.1 ?

This is only needed for support of features which are also both not part of 3.1:

• Introduce API Tokens with cluster_permissions and index_permissions directly associated with the token #5443
• Create a mechanism for plugins to explicitly declare actions they need to perform with their assigned PluginSubject #5341

Thus, I guess, there's no real point to have that in 3.1

[kumargu]

See #5483

Do you mean #5374 was not meant to be for 3.1 while the dependent #5483 was?

[kumargu]

Do we know how PR #5374 got missed in 3.1 ?

This is only needed for support of features which are also both not part of 3.1:

• Introduce API Tokens with cluster_permissions and index_permissions directly associated with the token #5443
• Create a mechanism for plugins to explicitly declare actions they need to perform with their assigned PluginSubject #5341

Thus, I guess, there's no real point to have that in 3.1

Ok. got it. Yeah, if the change is not needed for 3.1, we can skip it and accordingly fix #5483

[nibix]

Do you mean #5374 was not meant to be for 3.1 while the dependent #5483 was?

Sorry, that was a mid-air collision regarding comments ;-) This was in response to the request for backports, this is the new backport PR