Skip to content

Only update PrivilegesEvaluator configuration when the base config objects have actually changed.#6037

Merged
cwperks merged 7 commits intoopensearch-project:mainfrom
nibix:selective-role-updates
Mar 31, 2026
Merged

Only update PrivilegesEvaluator configuration when the base config objects have actually changed.#6037
cwperks merged 7 commits intoopensearch-project:mainfrom
nibix:selective-role-updates

Conversation

@nibix
Copy link
Copy Markdown
Collaborator

@nibix nibix commented Mar 26, 2026

Description

This changes PrivilegesConfiguration config change listening logic to only update the PrivilegesEvaluator and DLS/FLS configuration if the underlying configuration objects actually have been changed since the last time.

This can provide a small performance benefit, as for clusters with many roles and indices, the config updates can be costly (a few seconds for very large environments).

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

Issues Resolved

Fixes #5999

Testing

  • Existing int tests

Check List

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

@nibix
Only update PrivilegesEvaluator configuration when the base config ob…
df9aaf3 
…jects have actually changed.

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix
Changelog
004a9ca 
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix nibix mentioned this pull request Mar 26, 2026
Open
3 tasks
@cwperks
Copy link
Copy Markdown
Member

cwperks commented Mar 26, 2026

@nibix FYI test failures on the PR. Are you pushing a fix for these?


Suite: Test class org.opensearch.security.configuration.ConfigurationRepositoryTest
  2> 2026-03-26T13:14:58.773251697Z Test worker INFO Starting configuration org.apache.logging.log4j.core.config.properties.PropertiesConfiguration@402d45c6...
  2> 2026-03-26T13:14:58.779861023Z Test worker INFO Configuration org.apache.logging.log4j.core.config.properties.PropertiesConfiguration@402d45c6 started.
  2> 2026-03-26T13:14:58.783452249Z Test worker INFO Stopping configuration org.apache.logging.log4j.core.config.DefaultConfiguration@1d86b636...
  2> 2026-03-26T13:14:58.784409161Z Test worker INFO Configuration org.apache.logging.log4j.core.config.DefaultConfiguration@1d86b636 stopped.
  2> java.lang.AssertionError: 
    Expected: is not <SecurityDynamicConfiguration [seqNo=-1, primaryTerm=-1, ctype=CONFIG, version=2, centries={}, getImplementingClass()=class org.opensearch.security.securityconf.impl.v7.ConfigV7]>
         but: was <SecurityDynamicConfiguration [seqNo=-1, primaryTerm=-1, ctype=CONFIG, version=2, centries={}, getImplementingClass()=class org.opensearch.security.securityconf.impl.v7.ConfigV7]>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.configuration.ConfigurationRepositoryTest.getConfiguration_withInvalidConfigurationShouldReturnNewEmptyConfigurationObject(ConfigurationRepositoryTest.java:343)

Tests with failures:
 - org.opensearch.security.securityconf.impl.SecurityDynamicConfigurationTest.deepClone_shouldReturnNewObject
 - org.opensearch.security.configuration.ConfigurationRepositoryTest.getConfiguration_withInvalidConfigurationShouldReturnNewEmptyConfigurationObject

@nibix
Copy link
Copy Markdown
Collaborator Author

nibix commented Mar 26, 2026

This are easy to fix ... i am debugging at the moment these:

RollbackVersionApiTest > testRollbackWithoutEnoughVersions_shouldFail FAILED
    java.lang.AssertionError: 
    Expected: is <404>
         but: was <200>
        at __randomizedtesting.SeedInfo.seed([E3FEB377C5119CA2:A95F4D4AA34D797E]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.dlic.rest.api.RollbackVersionApiTest.testRollbackWithoutEnoughVersions_shouldFail(RollbackVersionApiTest.java:154)

RollbackVersionApiTest > testRollbackToPreviousVersion_success FAILED
    java.lang.AssertionError: 
    Expected: a string containing "config rolled back to version v1"
         but: was "{
      "status" : "OK",
      "message" : "config rolled back to version v2"
    }"
        at __randomizedtesting.SeedInfo.seed([E3FEB377C5119CA2:C4832FC52AB61458]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.dlic.rest.api.RollbackVersionApiTest.testRollbackToPreviousVersion_success(RollbackVersionApiTest.java:96)

There I am not yet clear on the cause, taking back to draft

@nibix nibix marked this pull request as draft March 26, 2026 15:32
@nibix
Test fixes
e77834d 
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 64.44444% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.04%. Comparing base (5ad8784) to head (6f2f244).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...ensearch/security/securityconf/impl/v7/RoleV7.java 33.33% 5 Missing and 7 partials ⚠️
...h/security/privileges/PrivilegesConfiguration.java 79.06% 6 Missing and 3 partials ⚠️
...ecurityconf/impl/SecurityDynamicConfiguration.java 0.00% 2 Missing and 2 partials ⚠️
.../security/securityconf/impl/v7/ActionGroupsV7.java 0.00% 2 Missing and 2 partials ⚠️
...search/security/securityconf/impl/v7/TenantV7.java 33.33% 1 Missing and 1 partial ⚠️
...earch/security/privileges/PrivilegesEvaluator.java 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6037      +/-   ##
==========================================
- Coverage   74.04%   74.04%   -0.01%     
==========================================
  Files         441      441              
  Lines       27355    27413      +58     
  Branches     4083     4110      +27     
==========================================
+ Hits        20256    20298      +42     
- Misses       5173     5177       +4     
- Partials     1926     1938      +12
Files with missing lines Coverage Δ
...h/security/privileges/PrivilegesEvaluatorImpl.java 83.81% <100.00%> (+0.89%) ⬆️
...earch/security/privileges/PrivilegesEvaluator.java 82.85% <92.85%> (+6.66%) ⬆️
...search/security/securityconf/impl/v7/TenantV7.java 82.35% <33.33%> (-10.51%) ⬇️
...ecurityconf/impl/SecurityDynamicConfiguration.java 82.00% <0.00%> (-2.25%) ⬇️
.../security/securityconf/impl/v7/ActionGroupsV7.java 83.87% <0.00%> (-12.43%) ⬇️
...h/security/privileges/PrivilegesConfiguration.java 85.39% <79.06%> (+0.88%) ⬆️
...ensearch/security/securityconf/impl/v7/RoleV7.java 76.92% <33.33%> (-10.75%) ⬇️

... and 8 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.

@nibix
Addded a few tests
2b00abd 
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix nibix marked this pull request as ready for review March 27, 2026 15:30
@nibix
Copy link
Copy Markdown
Collaborator Author

nibix commented Mar 27, 2026

Fixed now. Regarding the codecov issues: These are because of the added equals() and hashCode() methods. TBH, I do not think it makes sense to add test coverage for every single dimension these methods could be used. Thus, I would vote for keeping it the way it is.

@cwperks
Merge branch 'main' into selective-role-updates
78c7f78 
Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks
cwperks reviewed Mar 30, 2026
View reviewed changes
src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java
generalConfiguration,
pluginIdToRolePrivileges
)
boolean privilegesChanged = oldRawConfiguration == null
Copy link
Copy Markdown
Member

@cwperks cwperks Mar 30, 2026

Choose a reason for hiding this comment

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

Should we switch the ordering of the last 2 checkis here? I imagine oldRawConfiguration.equals(rawConfiguration) is the most expensive to compute.

Copy link
Copy Markdown
Collaborator Author

@nibix nibix Mar 30, 2026

Choose a reason for hiding this comment

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

I am not sure if it relevant to consider the computation time here. comparing will certainly not take longer than constructing the data structure. and looking into SecurityDynamicConfiguration, we make very liberal use of cloning. So, I would guess that is max a few ms.

cwperks
cwperks previously approved these changes Mar 30, 2026
View reviewed changes
nibix added 2 commits March 31, 2026 00:41
@nibix
Log fix
2cce36d 
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix
Merge remote-tracking branch 'origin/selective-role-updates' into sel…
6f2f244 
…ective-role-updates
@cwperks cwperks merged commit db4eb23 into opensearch-project:main Mar 31, 2026
109 of 113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cwperks cwperks cwperks approved these changes

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

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

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

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

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

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid reconstructing RoleBasedActionPrivileges when security configs other than roles and actiongroups change

3 participants

@nibix @cwperks @DarshitChanpura