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

[Backport 2.x] Disables custom serialization #3826

Closed

Conversation

parasjain1
Copy link
Contributor

Description

Disables custom serialization
Backport of #3789

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #3826 (20fa768) into 2.x (47c977b) will decrease coverage by 7.92%.
Report is 3 commits behind head on 2.x.
The diff coverage is 89.47%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #3826      +/-   ##
============================================
- Coverage     64.90%   56.99%   -7.92%     
+ Complexity     3664     3158     -506     
============================================
  Files           292      292              
  Lines         20648    20652       +4     
  Branches       3402     3402              
============================================
- Hits          13402    11771    -1631     
- Misses         5563     7181    +1618     
- Partials       1683     1700      +17     
Files Coverage Δ
...urity/ssl/transport/SecuritySSLRequestHandler.java 70.00% <100.00%> (-0.33%) ⬇️
.../org/opensearch/security/support/Base64Helper.java 94.44% <100.00%> (+3.53%) ⬆️
...org/opensearch/security/filter/SecurityFilter.java 61.05% <66.66%> (-5.14%) ⬇️
...search/security/transport/SecurityInterceptor.java 66.87% <83.33%> (-6.75%) ⬇️

... and 82 files with indirect coverage changes

cwperks
cwperks previously approved these changes Dec 12, 2023
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.

#3789 has CI failures that need investigation, that should be resolved before we attempt to backport. @parasjain1 Please let us know what you discover

@parasjain1 parasjain1 closed this Dec 12, 2023
@parasjain1 parasjain1 reopened this Dec 12, 2023
Signed-off-by: Paras Jain <parasjaz@amazon.com>
@parasjain1
Copy link
Contributor Author

Updated both the PRs. Will wait for CI tests to complete on the latest changes.

@parasjain1
Copy link
Contributor Author

One of the BWC tests failing due to the below error. 3 other are passing. I do not see any exceptions related to the serde change in the test logs.

org.opensearch.security.bwc.SecurityBackwardsCompatibilityIT > testDataIngestionAndSearchBackwardsCompatibility FAILED
    org.opensearch.client.ResponseException: method [POST], host [https://127.0.0.1:45209/], URI [_bulk?refresh=wait_for], status line [HTTP/1.1 400 Bad Request]
    {"error":{"root_cause":[{"type":"parse_exception","reason":"request body is required"}],"type":"parse_exception","reason":"request body is required"},"status":400}
        at __randomizedtesting.SeedInfo.seed([4D2DFC8E4F8BCABF:3FEFC3364D7ED4BB]:0)
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:376)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:346)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:321)
        at app//org.opensearch.security.bwc.helper.RestHelper.makeRequest(RestHelper.java:63)
        at app//org.opensearch.security.bwc.helper.RestHelper.requestAgainstAllNodes(RestHelper.java:83)
        at app//org.opensearch.security.bwc.helper.RestHelper.requestAgainstAllNodes(RestHelper.java:70)
        at app//org.opensearch.security.bwc.SecurityBackwardsCompatibilityIT.ingestData(SecurityBackwardsCompatibilityIT.java:227)
        at app//org.opensearch.security.bwc.SecurityBackwardsCompatibilityIT.testDataIngestionAndSearchBackwardsCompatibility(SecurityBackwardsCompatibilityIT.java:181)

One of the integration tests failing due to -


org.opensearch.security.HttpIntegrationTests > testHTTPSCompression FAILED
    java.lang.RuntimeException: Could not start all nodes BindHttpException[Failed to bind to 127.0.0.1:9353]; nested: BindException[Address already in use: bind];
        at __randomizedtesting.SeedInfo.seed([BC09ADC20400705D:6098E31279E062BE]:0)
        at org.opensearch.security.test.helper.cluster.ClusterHelper.startCluster(ClusterHelper.java:273)
        at org.opensearch.security.test.helper.cluster.ClusterHelper.startCluster(ClusterHelper.java:124)
        at org.opensearch.security.test.SingleClusterTest.setup(SingleClusterTest.java:119)
        at org.opensearch.security.test.SingleClusterTest.setup(SingleClusterTest.java:83)
        at org.opensearch.security.HttpIntegrationTests.testHTTPSCompression(HttpIntegrationTests.java:449)

Signed-off-by: Paras Jain <parasjaz@amazon.com>
cwperks
cwperks previously approved these changes Dec 18, 2023
@cwperks cwperks dismissed their stale review December 18, 2023 17:17

Failing CI Checks

@cwperks
Copy link
Member

cwperks commented Dec 18, 2023

Looking at BWC failure:

»  Caused by: java.io.IOException: invalid stream header: 02010931
»  	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:936) ~[?:?]
»  	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:375) ~[?:?]
»  	at org.opensearch.security.support.Base64JDKHelper$SafeObjectInputStream.<init>(Base64JDKHelper.java:142) ~[?:?]
»  	at org.opensearch.security.support.Base64JDKHelper.deserializeObject(Base64JDKHelper.java:132) ~[?:?]
»  	at org.opensearch.security.support.Base64Helper.deserializeObject(Base64Helper.java:48) ~[?:?]
»  	at org.opensearch.security.transport.SecurityRequestHandler.messageReceivedDecorate(SecurityRequestHandler.java:196) ~[?:?]
»  	at org.opensearch.security.ssl.transport.SecuritySSLRequestHandler.messageReceived(SecuritySSLRequestHandler.java:171) ~[?:?]
»  	at org.opensearch.security.OpenSearchSecurityPlugin$6$1.messageReceived(OpenSearchSecurityPlugin.java:794) ~[?:?]
»  	at org.opensearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:106) ~[opensearch-2.11.1-SNAPSHOT.jar:2.11.1-SNAPSHOT]
»  	at org.opensearch.transport.InboundHandler$RequestHandler.doRun(InboundHandler.java:480) ~[opensearch-2.11.1-SNAPSHOT.jar:2.11.1-SNAPSHOT]
»  	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:911) ~[opensearch-2.11.1-SNAPSHOT.jar:2.11.1-SNAPSHOT]
»  	at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52) ~[opensearch-2.11.1-SNAPSHOT.jar:2.11.1-SNAPSHOT]
»  	... 3 more

}

public static boolean shouldUseJDKSerialization(Version remoteVersion) {
return !remoteVersion.equals(ConfigConstants.FIRST_CUSTOM_SERIALIZATION_SUPPORTED_OS_VERSION);
Copy link
Member

Choose a reason for hiding this comment

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

@parasjain1 Does this need to include 2.11.1 as well?

return !Set.of(Version.2_11_0, Version.2_11_1).contains(remoteVersion);

@stephen-crawford
Copy link
Contributor

As with the original PR:

Update 1/12/2024: For the time being, a majority of maintainers have voted in favor of keeping the feature as is (not disabling it). The fact that the feature does not work does not represent a good reason to remove it from the code. We have limited anecdotes about this causing issues and so we do not want to set a precedent of removing features which have bugs. We of course hope that this can be fixed but in the meantime will continue to ship with it as is.

I am going to close this pull request given this decision but feel free to re-open it/keep discussing on it. This decision was reached by the maintainers as a group but does not mean it is set in stone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants