Use Collections.synchronizedSet and Collections.synchronizedMap for r…

[cwperks]

…oles, securityRoles and attributes in User

Signed-off-by: Craig Perkins cwperx@amazon.com

Description

Creating a draft PR to solicit feedback on a potential fix for #1961 and #1927.

Users are reporting that when the security cache expires that an intermittent java.io.OptionalDataException occurs.

The error is thrown while trying to readObject() from a SafeObjectInputStream while deserializing getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER).

I got the idea for this fix from here and here. Since the error was occurring on deserialization of the User object, I turned the HashSet and HashMap members of User into their corresponding Collections.synchronizedSet and Collections.synchronizedMap respectively.

Automated tests for this issue are being written.

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug Fix

• What is the old behavior before changes and new behavior after changes?

Old behavior: java.io.OptionalDataException exception is intermittently thrown on expiry of plugins.security.cache.ttl_minutes

New behavior: No exceptions thrown, bulk api does not throw exception at the expiration of the security cache

Issues Resolved

• [BUG] java.io.OptionalDataException error in OpenSearch 2.0 rc #1961
• [BUG] java.io.OptionalDataException - Cache Timeout #1927

Testing

Testing was performed by following the steps outlined here: #1961 (comment)

Before the change: Cluster would throw the exception within a minute

After the change: Cluster is stable and running without runtime exception for a long time

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.

[DarshitChanpura]

Good find @cwperks !!

[codecov-commenter]

Codecov Report

Merging #1970 (0b2280f) into main (d96da6c) will increase coverage by 0.01%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #1970      +/-   ##
============================================
+ Coverage     61.04%   61.05%   +0.01%     
- Complexity     3233     3234       +1     
============================================
  Files           256      256              
  Lines         18085    18085              
  Branches       3222     3222              
============================================
+ Hits          11040    11042       +2     
+ Misses         5470     5467       -3     
- Partials       1575     1576       +1     

Impacted Files	Coverage Δ
...c/main/java/org/opensearch/security/user/User.java	52.63% <50.00%> (ø)
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	56.48% <0.00%> (-1.53%)	⬇️
...ecurity/configuration/ConfigurationRepository.java	74.31% <0.00%> (+2.18%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

[peternied]

These kinds of bugs are the worst thank you for digging in on this issue.

As there was a provable (albeit complex) reproduction shouldn't we be able to create a test case? I spent some time trying to author a test that would create same state as the much more convoluted repro to no avail. Any other test that involves more systems running in concern - concurrently means that CI wouldn't be as reliable if its running at lower clock speed that dev boxes.

What about updating Base64Helper to ensure that collections objects are synchronized? We cannot easily do this with reflection as the classes do not implement a clear interface, and the definitions themselves are private, see Collections.java#L2005

I'd recommend we take the fix as is, if other folks have ideas on how we can test it I would be interested.

[msoler8785]

@peternied I'm willing to test on my prod cluster if you can provide build artifacts.

[peternied]

@msoler8785 Thanks for the generous offer. Unfortunately due to version number alignment you can only install plugins with the same version number as OpenSearch. We don't have a clean way to produce a build pointing to an older number.

The next scheduled release is OpenSearch 2.2.0 which will include this fix. According to the schedule there should be a viable built by Aug 4th if not sooner.

At your own risk; For a more immediate deployment, one could build the security plugin by checking out the version number of this plugin that corresponds to the branch of your cluster, cherry-pick this change, ./gradlew assemble -Dbuild.snapshot=false, replace the security jar on the machine under test with build/distributions/opensearch-security-X.Y.Z.0.jar.

[msoler8785]

@peternied thanks to your guidance I was able to get the plugin built and installed on all my cluster nodes. Initial testing looks good and I am no longer encountering those errors. I'll review over the weekend and update if anything changes. Thanks @cwperks!

[peternied]

@opensearch-project/security I'd like to see this merged for 2.2.0 what do y'all think?

[DarshitChanpura]

@opensearch-project/security I'd like to see this merged for 2.2.0 what do y'all think?

I agree