Closed nibix closed 2 weeks ago
codecov[bot]
commented
2 weeks ago Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.
Project coverage is 65.48%. Comparing base (
94f7ccb) to head (110426f). Report is 5 commits behind head on main.:exclamation: Current head 110426f differs from pull request most recent head 289e9dd
Please upload reports for the commit 289e9dd to get more accurate results.
nibix
commented
2 weeks ago @cwperks
Thank you @nibix! This change looks good to me. One scenario I'd like to see covered would be a change in action group and verify its reflected in the new FlattenedActionGroups data structure.
This is certainly an important thing to test. However, such configuration updates are handled by the DynamicConfigFactory class which is not changed by this PR.
There used to be an integration test which would test what you are asking for - that is, it changed action groups via API and tested the effectiveness of these changes via normal user operations. That was at https://github.com/opensearch-project/security/blob/069249656970e839302a0d3b8b87c82fbf3e56cc/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java . There was recently a refactor of this test in #4371. I quickly skimmed the replacement code and could not find any logic which tests the effectiveness of the modified configuration, but maybe I missed something?
nibix
commented
2 weeks ago @DarshitChanpura
Would the new runtime be more linear compared to the older implementation because we are using a combination of BFS + DFS?
The major gain comes from the fact that we calculate the resolved action group space only once instead of re-calculating it again and again for each action privilege configuration. Additionally, by using an iterative algorithm instead of a recursive algorithm it is easier for us to detect that resolution has finished and abort early. That give some more gain. However, all these gains only pertain to the config reload phase and not to the actual privilege evaluation.
DarshitChanpura
commented
2 weeks ago Merging this since we have approvals. Any discussions can be addressed in follow-up PRs.
Description
This code change is just in preparation for the change in #4380 as requested in https://github.com/opensearch-project/security/pull/4416#pullrequestreview-2106138499 .
This replaces the previous action group resolution algorithm done in ConfigModelV7 by a new one, which has a number of enhancements:
The new implementation forms an independent class, enhancing testability and use in other components that will be introduced in #4380.
The new implementation handles action group configurations containing loops gracefully. The old one would wind up StackOverflowErrors.
The new one pre-computes the resolved action groups, thus enhancing the performance.
Category: Enhancement
Why these changes are required? - The changes in #4380 will no longer use ConfigModelV7 and thus need an independent action group resolution mechanism.
What is the old behavior before changes and new behavior after changes? - No behavioral changes except for action group definitions containing groups, which will be handled gracefully now.
Testing
org.opensearch.security.securityconf.FlattenedActionGroupsTestCheck List
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.