Resolve ImmutableOpenMap issue from core refactor by stephen-crawford · Pull Request #2715 · opensearch-project/security

stephen-crawford · 2023-04-21T16:17:03Z

Description

Removes use of the ImmutableOpenMap from Security code to resolve issue #2714.

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.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford · 2023-04-21T16:51:58Z

Will have to wait until the next build: https://build.ci.opensearch.org/job/distribution-build-opensearch/7748/ before we can actually confirm this is fixed. Most recent build gets pulled as part of build but that does not include the Core changes.

Something needs to be fixed still

RyanL1997

Hi @scrawfor99 , thanks for this fix. Sorry I had to dismissed my previous approve, because I was trying this fix on my local machine, and I think we need to fix some of the functions. I left a comment down below.

RyanL1997 · 2023-04-21T22:44:10Z

src/integrationTest/java/org/opensearch/test/framework/matcher/AliasExistsMatcher.java

 			GetAliasesResponse response = client.admin().indices().getAliases(new GetAliasesRequest(aliasName)).get();
-			ImmutableOpenMap<String, List<AliasMetadata>> aliases = response.getAliases();
-			Set<String> actualAliasNames = StreamSupport.stream(spliteratorUnknownSize(aliases.valuesIt(), IMMUTABLE), false)
+			Map<String, List<AliasMetadata>> aliases = response.getAliases();


I think for this getAliases(), we also need to fix it, because it is also depending on the stale library of ImmutableOpenMap

Oh okay, I will take a look.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

peternied · 2023-04-24T20:10:38Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

                @Override
                public Iterator<IndexMetadata> iterator() {
-                    return clusterService.state().getMetadata().getIndices().valuesIt();
+                    return (Iterator<IndexMetadata>) clusterService.state().getMetadata().getIndices().values();


2023-04-24T19:23:06.3310228Z 1> java.lang.ClassCastException: class java.util.Collections$UnmodifiableCollection cannot be cast to class java.util.Iterator (java.util.Collections$UnmodifiableCollection and java.util.Iterator are in module java.base of loader 'bootstrap') 2023-04-24T19:23:06.3310965Z 1> at org.opensearch.security.privileges.PrivilegesEvaluator$1.iterator(PrivilegesEvaluator.java:652) ~[main/:?]

Think you might be missing a .iterator() call before attempting to return, can you do this without the unchecked suppression?

You can repro the test failure with org.opensearch.security.IndexIntegrationTests.testIndexResolveMinus for a quick turnaround

CI failure due to code issue

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

codecov-commenter · 2023-04-24T23:50:05Z

Codecov Report

Merging #2715 (876871e) into main (d168c1a) will increase coverage by 0.03%.
The diff coverage is 77.77%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #2715      +/-   ##
============================================
+ Coverage     61.45%   61.48%   +0.03%     
- Complexity     3385     3398      +13     
============================================
  Files           270      272       +2     
  Lines         18694    18743      +49     
  Branches       3279     3285       +6     
============================================
+ Hits          11488    11524      +36     
- Misses         5611     5620       +9     
- Partials       1595     1599       +4

Impacted Files	Coverage Δ
...security/dlic/rest/api/InternalUsersApiAction.java	`86.88% <57.14%> (+2.88%)`	⬆️
...java/org/opensearch/security/user/UserService.java	`78.18% <78.18%> (ø)`
.../opensearch/security/OpenSearchSecurityPlugin.java	`80.27% <100.00%> (+0.11%)`	⬆️
...security/dlic/rest/api/SecurityRestApiActions.java	`95.00% <100.00%> (ø)`
...earch/security/privileges/PrivilegesEvaluator.java	`73.53% <100.00%> (+0.24%)`	⬆️
...opensearch/security/user/UserServiceException.java	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cwperks · 2023-04-25T13:17:12Z

...st/java/org/opensearch/test/framework/matcher/GetSettingsResponseContainsIndicesMatcher.java

+		final Map<String, Settings> indexToSettings = new HashMap<>();
+		for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {
+			indexToSettings.put(cursor.key, cursor.value);
+		}


Can this be simplified to:

Suggested change

final Map<String, Settings> indexToSettings = new HashMap<>();

for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {

indexToSettings.put(cursor.key, cursor.value);

}

final Set<String> indexKeys = new HashSet<>();

for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {

indexKeys.add(cursor.key);

}

Looks like either would work, but since we are not using the values in the map and only getting a keySet it would take less memory.

Sorry Craig, I didn't see this before the PR got merged. I had left it as a map because that is what the original implementation was. I agree that a Set should work fine here as well. If you wanted to swap it you could open up a quick PR to change it in main.

cwperks

Thank you @scrawfor99, the changes look good to me. I left one suggestion so let me know what you think. It wouldn't block this PR from being approved and merged. If the suggestion doesn't work then let me know and I will approve this PR.

andrross · 2023-04-25T20:35:02Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

            final List<AliasMetadata> filteredAliases = new ArrayList<AliasMetadata>();

-            final ImmutableOpenMap<String, AliasMetadata> aliases = indexMetaData.getAliases();
+            final Map<String, AliasMetadata> aliases = new HashMap<>();


@scrawfor99 Why did you make this change? Doesn't indexMetaData.getAliases() still return an ImmutableOpenMap? Seems like you should avoid making a copy of the map in any case.

Hi @andrross, I made this swap because we could not use keysIt anymore. We needed aliases to be a normal map to then grab and iterate over the keyset. Is there an issue with this implementation?

@nknize What's happening here? Pretty sure making a copy of the the map is the exact opposite of your intention with this change.

This was the issue that arose when the change was made to core: #2714. I just made changes to get things Green. Sorry if this was not the intended implementation...

Sorry @scrawfor99! Definitely not trying to blame you here! I don't understand why keysIt doesn't work anymore, and I don't think the intent with @nknize's change is to force downstream dependencies to copy data structures when doing a simple iteration.

Hi @andrross, no need to apologize :). I just want to make sure I did the correct thing. I did not look that closely into the overall reasoning for the change in core. I just checked to find the overall cause of the issue we were facing and then took the fastest route to unblocking. If you or Nick, have an alternative solution that is the preferred method now just let me know and I can make a swap. In the linked issue, I showed an example of the errors we were getting which was basically Java complaining about not being able to find the ImmutableOpenMap method definitions.

No copies unless you need to make changes to the map (even then the core should return an unmodifiableMap so you would only be able to change a clone).

If you just need an iterator over the keys then use .keySet().iterator()

@scrawfor99 The code in question changed to be a plain map. I think you should be able to simplify this and get rid of the copy.

…#2715) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

…#2715) Signed-off-by: Sam <samuel.costa@eliatra.com>

…#2715)

* Resolve ImmutableOpenMap issue from core refactor (#2715) * Switch from ImmutableOpenMap to Map in AliasExistsMatcher (#2725) * Remove references to ObjectObjectCursor Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch from ImmutableOpenMap to Map in AliasExistsMatcher (#2725) * Remove references to ObjectObjectCursor Signed-off-by: Craig Perkins <cwperx@amazon.com> * Removes a missed reference for ImmutableOpenMap (#2726) Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Update ClusterInfoHolder Signed-off-by: Craig Perkins <cwperx@amazon.com> * Run spotlessApply Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>

remove immutable open map use

3575d98

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford requested review from DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied and reta as code owners April 21, 2023 16:17

reta previously approved these changes Apr 21, 2023

View reviewed changes

RyanL1997 previously approved these changes Apr 21, 2023

View reviewed changes

RyanL1997 reviewed Apr 21, 2023

View reviewed changes

stephen-crawford dismissed reta’s stale review via cbfa154 April 24, 2023 15:44

Remove alias dependency

cbfa154

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford force-pushed the valuesItFix branch 2 times, most recently from 93ef49d to cbfa154 Compare April 24, 2023 17:04

stephen-crawford marked this pull request as draft April 24, 2023 18:29

stephen-crawford and others added 3 commits April 24, 2023 14:29

Swap over AliasExistsMatcher

d40f98b

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Fix indexMappingIsEqual

cf6788d

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Merge branch 'opensearch-project:main' into valuesItFix

47f08b9

peternied previously approved these changes Apr 24, 2023

View reviewed changes

stephen-crawford mentioned this pull request Apr 24, 2023

[Bug] Core changes to ImmutableOpenMap causing various failures #2714

Closed

peternied reviewed Apr 24, 2023

View reviewed changes

stephen-crawford added 2 commits April 24, 2023 16:11

Try to fix privEval iterator

2b6774b

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Update iterator

876871e

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford marked this pull request as ready for review April 24, 2023 21:51

RyanL1997 approved these changes Apr 24, 2023

View reviewed changes

RyanL1997 mentioned this pull request Apr 25, 2023

[Security/Extension] Extension Authentication Backend #2672

Merged

3 tasks

cwperks mentioned this pull request Apr 25, 2023

Extensions config for JWT signing/encryption key #2671

Merged

3 tasks

cwperks reviewed Apr 25, 2023

View reviewed changes

peternied approved these changes Apr 25, 2023

View reviewed changes

peternied merged commit 6ace852 into opensearch-project:main Apr 25, 2023

andrross reviewed Apr 25, 2023

View reviewed changes

cwperks mentioned this pull request Apr 26, 2023

Switch from ImmutableOpenMap to Map in AliasExistsMatcher #2725

Merged

3 tasks

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Apr 27, 2023

Resolve ImmutableOpenMap issue from core refactor (opensearch-project…

0b6f8db

…#2715) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

DarshitChanpura mentioned this pull request Apr 27, 2023

Removes all ImmutableOpenMap references #2726

Merged

1 task

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Jun 13, 2023

Resolve ImmutableOpenMap issue from core refactor (opensearch-project…

c567bb1

…#2715) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Jun 13, 2023

Resolve ImmutableOpenMap issue from core refactor (opensearch-project…

205b30a

…#2715) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

samuelcostae pushed a commit to samuelcostae/security that referenced this pull request Jun 19, 2023

Resolve ImmutableOpenMap issue from core refactor (opensearch-project…

14f1b36

…#2715) Signed-off-by: Sam <samuel.costa@eliatra.com>

cwperks pushed a commit to cwperks/security that referenced this pull request Jun 27, 2023

Resolve ImmutableOpenMap issue from core refactor (opensearch-project…

2407128

…#2715)

cwperks mentioned this pull request Jun 27, 2023

[Backport 2.x] Resolve ImmutableOpenMap issue from core refactor #2908

Merged

Conversation

stephen-crawford commented Apr 21, 2023

Description

Check List

Uh oh!

stephen-crawford commented Apr 21, 2023

Uh oh!

RyanL1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-crawford Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 24, 2023

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrross Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-crawford Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stephen-crawford Apr 24, 2023 •

edited

Loading

cwperks left a comment •

edited

Loading

andrross Apr 25, 2023 •

edited

Loading

stephen-crawford Apr 25, 2023 •

edited

Loading