Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting by reta · Pull Request #10562 · opensearch-project/OpenSearch

reta · 2023-10-11T13:32:32Z

Description

While instrumenting the transports (both TCP and HTTP), we found out that security plugin heavily depends on netty transport (in fact, it does not support anything else) but it is excessive typecasting the different transport object to their Netty4Xxx equivalents for accessing handlers and attributes. It significantly complicates any instrumentation efforts and needs general mechanism to access contextual information without typecasting.

Related Issues

Related to opensearch-project/security#3515

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

reta · 2023-10-11T13:33:52Z

@peternied @Gaganjuneja fyi

reta · 2023-10-11T13:36:40Z

(I will add changelog entry right when Gradle check fails, since it should)

cwperks · 2023-10-11T13:43:20Z

Thank you @reta! There are a couple of areas that the security plugin is coupled to netty:

The security plugin's implementation of SSL relies on adding a step to the netty pipeline. The SSL Handler is the first step of the pipeline: https://github.com/opensearch-project/security/blob/c5f8c83cba3b4d6dfb7f9ec268a23874f7909003/src/main/java/org/opensearch/security/ssl/http/netty/SecuritySSLNettyHttpServerTransport.java#L147-L152
As part of Add early rejection from RestHandler for unauthorized requests security#3418, a step in the netty pipeline was added before decompression to verify the request based on headers and uses netty channel attributes

As far as I can see, there are no automated tests to verify the behavior amongst other http engines

reta · 2023-10-11T13:54:59Z

I think for the instrumentation part with this change we should be set, what do you think?

The security plugin would need to set attributes as well. Can that be added?

Which ones?

github-actions · 2023-10-11T14:02:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/27631/
CommitID: 0eb752b
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?

github-actions · 2023-10-11T14:04:12Z

Compatibility status:

Checks if related components are compatible with change 1c184e2

Incompatible components

Incompatible components: [https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git]

github-actions · 2023-10-11T14:54:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      2 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testReadRangeBlobWithRetries

URL: https://build.ci.opensearch.org/job/gradle-check/27636/
CommitID: c84c736
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2023-10-11T14:56:13Z

Codecov Report

Merging #10562 (1c184e2) into main (9bcd7ea) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               main   #10562   +/-   ##
=========================================
  Coverage     71.16%   71.16%           
- Complexity    58399    58403    +4     
=========================================
  Files          4844     4844           
  Lines        275289   275306   +17     
  Branches      40083    40086    +3     
=========================================
+ Hits         195898   195928   +30     
+ Misses        62997    62956   -41     
- Partials      16394    16422   +28

Files	Coverage Δ
...src/main/java/org/opensearch/http/HttpChannel.java	`50.00% <0.00%> (-50.00%)`	⬇️
...lemetry/tracing/channels/TraceableHttpChannel.java	`54.54% <0.00%> (-2.60%)`	⬇️
...tracing/channels/TraceableTcpTransportChannel.java	`83.87% <0.00%> (-2.80%)`	⬇️
...org/opensearch/transport/TaskTransportChannel.java	`80.00% <0.00%> (-5.72%)`	⬇️
...main/java/org/opensearch/transport/TcpChannel.java	`85.71% <0.00%> (-14.29%)`	⬇️
.../org/opensearch/transport/TcpTransportChannel.java	`76.66% <0.00%> (-2.65%)`	⬇️
...ava/org/opensearch/transport/TransportChannel.java	`40.00% <0.00%> (-4.45%)`	⬇️
.../opensearch/transport/netty4/Netty4TcpChannel.java	`81.13% <0.00%> (-6.63%)`	⬇️
.../org/opensearch/http/netty4/Netty4HttpChannel.java	`65.38% <0.00%> (-19.62%)`	⬇️

... and 469 files with indirect coverage changes

…TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta · 2023-10-11T15:09:54Z

The security plugin would need to set attributes as well. Can that be added?

Discussed with @cwperks offline, we'll looking into that separately

peternied

Thanks for making access to this more generic - I much prefer this approach rather than downstream 'peaking' of the types. Couple of minor comments around the new interface.

modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java

server/src/main/java/org/opensearch/http/HttpChannel.java

github-actions · 2023-10-11T15:54:59Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/27648/
CommitID: ab4140c
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?

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta · 2023-10-11T16:19:36Z

@cwperks @peternied @Gaganjuneja thanks for the review folks, the attributes issue is more complicated, looking if we even need that, but in any case the solution would be different and it would make sense to have a dedicated pull request for it.

github-actions · 2023-10-11T17:04:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/27659/
CommitID: 1c184e2
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?

Gaganjuneja · 2023-10-11T17:05:24Z

LGTM.

github-actions · 2023-10-11T17:55:55Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/27665/
CommitID: 1c184e2

…TcpChannel and TrasportChannel without excessive typecasting (#10562) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit c55e1b4) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…TcpChannel and TrasportChannel without excessive typecasting (#10562) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit c55e1b4) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…TcpChannel and TrasportChannel without excessive typecasting (#10562) (#10570) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting * Address code review comments --------- (cherry picked from commit c55e1b4) Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…TcpChannel and TrasportChannel without excessive typecasting (opensearch-project#10562) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>

…TcpChannel and TrasportChannel without excessive typecasting (opensearch-project#10562) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…TcpChannel and TrasportChannel without excessive typecasting (opensearch-project#10562) * Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Address code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

reta added the skip-changelog label Oct 11, 2023

reta added the enhancement Enhancement or improvement to existing feature or request label Oct 11, 2023

reta mentioned this pull request Oct 11, 2023

Fix TraceableTcpTransportChannel classcast exception opensearch-project/security#3514

Closed

3 tasks

reta added the backport 2.x Backport to 2.x branch label Oct 11, 2023

reta force-pushed the issue.3514 branch from 0eb752b to c84c736 Compare October 11, 2023 14:11

reta changed the title ~~Add the means to extract the contextual properties from HttpChannel, TcpCChannel and TrasportChannel without excessive typecasting~~ Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting Oct 11, 2023

Add the means to extract the contextual properties from HttpChannel, …

ab4140c

…TcpChannel and TrasportChannel without excessive typecasting Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue.3514 branch from c84c736 to ab4140c Compare October 11, 2023 15:09

peternied reviewed Oct 11, 2023

View reviewed changes

modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java Outdated Show resolved Hide resolved

server/src/main/java/org/opensearch/http/HttpChannel.java Outdated Show resolved Hide resolved

Address code review comments

1c184e2

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

peternied approved these changes Oct 11, 2023

View reviewed changes

cwperks approved these changes Oct 11, 2023

View reviewed changes

stephen-crawford approved these changes Oct 11, 2023

View reviewed changes

reta merged commit c55e1b4 into opensearch-project:main Oct 11, 2023

opensearch-trigger-bot bot mentioned this pull request Oct 11, 2023

[Backport 2.x] Add the means to extract the contextual properties from HttpChannel, TcpChannel and TrasportChannel without excessive typecasting #10570

Merged

reta mentioned this pull request Jan 4, 2024

Add the means to extract the contextual underlying channel from HttpChannel without excessive typecasting #11751

Merged

8 tasks

BrewTestBot mentioned this pull request Feb 21, 2024

opensearch 2.12.0 Homebrew/homebrew-core#163463

Merged

Conversation

reta commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

reta commented Oct 11, 2023

Uh oh!

reta commented Oct 11, 2023

Uh oh!

cwperks commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reta commented Oct 11, 2023

Uh oh!

github-actions bot commented Oct 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Compatibility status:

Incompatible components

Skipped components

Compatible components

Uh oh!

github-actions bot commented Oct 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov bot commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reta commented Oct 11, 2023

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

reta commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Gaganjuneja commented Oct 11, 2023

Uh oh!

github-actions bot commented Oct 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

reta commented Oct 11, 2023 •

edited

Loading

cwperks commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

reta commented Oct 11, 2023 •

edited

Loading