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

Ensure support of the transport-nio by security plugin (HTTP) #16474

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 24, 2024

Description

Ensure support of the transport-nio by security plugin (HTTP only), changes in opensearch.yml:

http.type: nio-http-transport-secure
$ curl -kv https://localhost:9200/ -u admin:_ad0m#Ns_ --http1.1                                                                                                                                                                                                                              08:32:15 [39/437]
* Host localhost:9200 was resolved.                                                  
* IPv6: ::1                                                                          
* IPv4: 127.0.0.1                                                                    
*   Trying [::1]:9200...                                                             
* Connected to localhost (::1) port 9200                                             
* ALPN: curl offers http/1.1                                                         
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Request CERT (13):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Certificate (11):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256 / x25519 / RSASSA-PSS
* ALPN: server accepted http/1.1                                                     
* using HTTP/1.x                                                                                                                                                                                                                                                                                                                                      
* Server auth using Basic with user 'admin'                                                                                                                                                                                                                                                                                                           
> GET / HTTP/1.1                                                                                                                                                                                                                                                                                                                                      
> Host: localhost:9200                                                               
> Authorization: Basic YWRtaW46X2FkMG0jTnNf
> User-Agent: curl/8.9.1                                                             
> Accept: */*                                                                        
>                                                                                    
* Request completely sent off                                                        
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
< HTTP/1.1 200 OK                                                                    
< X-OpenSearch-Version: OpenSearch/3.0.0 (opensearch)
< content-type: application/json; charset=UTF-8
< content-length: 576                                                                
<                                                                                    
{                                                                                    
  "name" : "host",                                              
  "cluster_name" : "opensearch",                                                     
  "cluster_uuid" : "ByTSGP1rSGOnak0LKonwWw",
  "version" : {                                                                      
    "distribution" : "opensearch",                                                   
    "number" : "3.0.0",                                                              
    "build_type" : "tar",                                                            
    "build_hash" : "9dd1a59847ad8c2716d002716521ac40afc69355",
    "build_date" : "2024-10-24T04:39:37.808432550Z",
    "build_snapshot" : false,                                                        
    "lucene_version" : "9.12.0",                                                     
    "minimum_wire_compatibility_version" : "2.18.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },                                                                                 
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
} 

Related Issues

Closes #13245

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

@github-actions github-actions bot added enhancement Enhancement or improvement to existing feature or request Plugins v2.19.0 Issues and PRs related to version 2.19.0 v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 24, 2024
Copy link
Contributor

❌ Gradle check result for 864d575: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta reta added backport 2.x Backport to 2.x branch and removed skip-changelog labels Oct 24, 2024
Copy link
Contributor

❌ Gradle check result for 68d4f33: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c56c85b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for d295b86: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta reta force-pushed the issue-13245 branch 2 times, most recently from 0b169a7 to c99bf17 Compare October 29, 2024 16:00
Copy link
Contributor

❌ Gradle check result for c99bf17: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c99bf17: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 6562fa5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 8aafff0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for f4f30d4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 93a1491: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 019c2ae: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta
Copy link
Collaborator Author

reta commented Nov 4, 2024

@cwperks @andrross @dblock would really appreciate if you folks could take a look, thanks!

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Nice work @reta! I'll take a few passes at this PR and get feedback ASAP.

There was a pretty sizable change to the netty pipeline in 2.11 in these 2 PRs which should be extended to any http transport.

There is an automated test here which performs a stress test.

@reta
Copy link
Collaborator Author

reta commented Nov 4, 2024

Nice work @reta! I'll take a few passes at this PR and get feedback ASAP.

Thanks @cwperks !

There was a pretty sizable change to the netty pipeline in 2.11 in these 2 PRs which should be extended to any http transport.

I think these improvements could go independently of each other, right?

@cwperks
Copy link
Member

cwperks commented Nov 4, 2024

I think we need to get that test passing w/ the security plugin to declare that this transport works w/ the security plugin. You can run it with the changes in this branch: cwperks/security#33

Copy link
Contributor

github-actions bot commented Nov 4, 2024

❌ Gradle check result for 7abf006: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta
Copy link
Collaborator Author

reta commented Nov 4, 2024

I think we need to get that test passing w/ the security plugin to declare that this transport works w/ the security plugin. You can run it with the changes in this branch: cwperks/security#33

This is not as straightforward sadly: the security plugin (at the moment) still only recognizes SecureNetty4HttpServerTransport (see please OpenSearchSecureSettingsFactory). We definitely could work towards removing these last pieces of knowledge from it.

Copy link
Contributor

github-actions bot commented Nov 4, 2024

✅ Gradle check result for 7abf006: SUCCESS

Copy link
Contributor

github-actions bot commented Nov 4, 2024

❌ Gradle check result for dcb05d0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Nov 5, 2024

❌ Gradle check result for dcb05d0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Nov 5, 2024

❕ Gradle check result for dcb05d0: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringQueryPhase
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=cluster.health/10_basic/cluster health with closed index}

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@cwperks
Copy link
Member

cwperks commented Nov 5, 2024

Thank you @reta! This PR looks good to me. I'm not a maintainer of this repo so I will leave a comment approval :shipit:

@reta reta merged commit b25e10a into opensearch-project:main Nov 5, 2024
38 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-16474-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b25e10afb9e216c547a59409d909ec1ecae101ec
# Push it to GitHub
git push --set-upstream origin backport/backport-16474-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-16474-to-2.x.

@andrross
Copy link
Member

andrross commented Nov 5, 2024

Just curious @reta, is there any reason to prefer transport-nio over the Netty version? I'm wondering if this is something to consider removing in the next major version.

@reta
Copy link
Collaborator Author

reta commented Nov 5, 2024

Just curious @reta, is there any reason to prefer transport-nio over the Netty version? I'm wondering if this is something to consider removing in the next major version.

Thanks @andrross I think we've never benchmarked / compared these transports side by side. I will try to research the history of nio transport, thanks!

reta added a commit to reta/OpenSearch that referenced this pull request Nov 5, 2024
…arch-project#16474)

* Ensure support of the transport-nio by security plugin (HTTP)

Signed-off-by: Andriy Redko <[email protected]>

* Add header verifier and decompressor support of secure NIO transport variant

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b25e10a)
@andrross
Copy link
Member

andrross commented Nov 5, 2024

Thanks @reta. I'm always looking for opportunity to remove code!

reta added a commit that referenced this pull request Nov 5, 2024
#16565)

* Ensure support of the transport-nio by security plugin (HTTP)

Signed-off-by: Andriy Redko <[email protected]>

* Add header verifier and decompressor support of secure NIO transport variant

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b25e10a)
@reta
Copy link
Collaborator Author

reta commented Nov 6, 2024

Thanks @reta. I'm always looking for opportunity to remove code!

@andrross traced it back to elastic/elasticsearch#27260 (which basically what I also suspected):

We are interested in introducing alternative transport options that do not depend on external libraries. From time to time we have had some friction with using netty related to buffer pooling, oom excetions, exception handling, unsafe, etc. We want to look explore if a homegrown transport could potentially meet our needs.

Not sure it is relevant these day, unless java.nio shines in some flows

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 backport-failed enhancement Enhancement or improvement to existing feature or request Plugins v2.19.0 Issues and PRs related to version 2.19.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[transport] Ensure support of the transport-nio by security plugin (HTTP)
3 participants