Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate requests are not impacted during security cache invalidation #3637

Merged

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Nov 1, 2023

Description

Opening a PR that adds a test to ensure that requests during a cache invalidation succeed and fail if any unsuccessful status code is detected. This is related to adding an automated test for #1961

I'm currently encountering a couple of issues and am able to reproduce a separate concurrency error, but not the OptionalDataException.

This test will fail if both of these PRs are reverted:

When the above 2 PRs are reverted, the test case fails with:

Caused by: org.opensearch.OpenSearchException: Instance User [name=admin, backend_roles=[admin], requestedTenant=null] of class class org.opensearch.security.user.User is not serializable
        at org.opensearch.security.support.Base64CustomHelper.serializeObject(Base64CustomHelper.java:102) ~[main/:?]
        at org.opensearch.security.support.Base64Helper.serializeObject(Base64Helper.java:34) ~[main/:?]
        at org.opensearch.security.transport.SecurityInterceptor.ensureCorrectHeaders(SecurityInterceptor.java:312) ~[main/:?]
        at org.opensearch.security.transport.SecurityInterceptor.sendRequestDecorate(SecurityInterceptor.java:240) ~[main/:?]
        at org.opensearch.security.OpenSearchSecurityPlugin$6$2.sendRequest(OpenSearchSecurityPlugin.java:811) ~[main/:?]
        at org.opensearch.transport.TransportService.sendRequest(TransportService.java:913) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        ... 21 more
Caused by: java.lang.NullPointerException: Cannot invoke "String.length()" because "str" is null
        at org.opensearch.core.common.io.stream.StreamOutput.writeString(StreamOutput.java:443) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        at org.opensearch.core.common.io.stream.StreamOutput.writeCollection(StreamOutput.java:1187) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        at org.opensearch.core.common.io.stream.StreamOutput.writeStringCollection(StreamOutput.java:1199) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        at org.opensearch.security.user.User.writeTo(User.java:263) ~[main/:?]
        at org.opensearch.security.support.Base64CustomHelper.serializeObject(Base64CustomHelper.java:86) ~[main/:?]
        at org.opensearch.security.support.Base64Helper.serializeObject(Base64Helper.java:34) ~[main/:?]
        at org.opensearch.security.transport.SecurityInterceptor.ensureCorrectHeaders(SecurityInterceptor.java:312) ~[main/:?]
        at org.opensearch.security.transport.SecurityInterceptor.sendRequestDecorate(SecurityInterceptor.java:240) ~[main/:?]
        at org.opensearch.security.OpenSearchSecurityPlugin$6$2.sendRequest(OpenSearchSecurityPlugin.java:811) ~[main/:?]
        at org.opensearch.transport.TransportService.sendRequest(TransportService.java:913) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
        ... 21 more

This test provides assurances that there are no errors related to concurrency and cache invalidation. I had to update the integrationTest framework to add backendRoles onto the User object in the integration test suite.

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

Adds a test

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.

@cwperks cwperks changed the title Add async test to ensure bulk requests succeed while cache is invalidated Add async test with mix of requests including cache invalidation Nov 1, 2023
@cwperks cwperks marked this pull request as ready for review November 2, 2023 19:57
@cwperks
Copy link
Member Author

cwperks commented Nov 2, 2023

To replicate the OptionalDataException you need to revert the 2 changes in the PR description and also change every instance of final boolean useJDKSerialization = connection.getVersion().before(ConfigConstants.FIRST_CUSTOM_SERIALIZATION_SUPPORTED_OS_VERSION); to final boolean useJDKSerialization = true;

Signed-off-by: Craig Perkins <[email protected]>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@peternied peternied changed the title Add async test with mix of requests including cache invalidation Validate requests are not impacted during security cache invalidation Nov 3, 2023
@peternied peternied merged commit 850b7dc into opensearch-project:main Nov 3, 2023
56 checks passed
@cwperks cwperks added the backport 2.x backport to 2.x branch label Nov 3, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 3, 2023
…#3637)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit 850b7dc)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
cwperks added a commit to cwperks/security that referenced this pull request Nov 3, 2023
peternied pushed a commit that referenced this pull request Nov 3, 2023
…he invalidation (#3648)

Backport
850b7dc
from #3637.

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
willyborankin pushed a commit that referenced this pull request Nov 17, 2023
#3725)

### Description
This change backports #1970 in order to fix the OptionalDataException
issue encountered on the 1.3.x line.
The original change did not have any tests but #3637 added tests, so I
backported those as well and the changes required to support them. This
resulted in changes to the TestSecurityConfig.User class, the addition
of a IndexStateIsEqualToMatcher and User.java.

### Issues Resolved
- Resolves #3531
- Backports #1970 
- Backports #3637

### Check List
- [X] New functionality includes testing
- [ ] ~New functionality has been documented~
- [x] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
peternied pushed a commit to peternied/security that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants