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

Add netty-transport-native-unix-common to modules/transport-netty4/bu… #3848

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Jul 11, 2022

…ild.gradle to satisfy transitive dependency

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

Description

The upgrade from Netty 4.1.73.Final -> 4.1.78.Final introduced a new transitive dependency on netty-handler which is not getting included in the netty-transport4 module and causing build failures for 1.3.4 because the security plugin pulls in this module and uses the impacted jar.

This change pulls in the transitive dependency on netty-transport-native-unix-common into the module's build.gradle.

Details of the change in Netty can be found here: netty/netty@bc085b0

Issues Resolved

Security Plugin #1934

Check List

  • 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 requested review from a team and reta as code owners July 11, 2022 20:28
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

…ild.gradle to satisfy transitive dependency

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks force-pushed the add-netty-transport-native-unix-common-to-transport-netty4-module branch from 6b2fb61 to 9a2d837 Compare July 11, 2022 20:37
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@reta reta added backport 2.x Backport to 2.x branch backport 2.1 dependencies Pull requests that update a dependency file and removed backport 2.1 labels Jul 11, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta reta merged commit 33138ed into opensearch-project:main Jul 11, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 11, 2022
#3848)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)
@zelinh
Copy link
Member

zelinh commented Jul 11, 2022

I think we might also need to backported to 1.3 branch as it's related to recent 1.3.4 build failure.

@reta
Copy link
Collaborator

reta commented Jul 11, 2022

I think we might also need to backported to 1.3 branch as it's related to recent 1.3.4 build failure.

This dependency was introduced in Netty 4.1.78 (or 4.1.77), 1.x uses older version

@cliu123
Copy link
Member

cliu123 commented Jul 11, 2022

@reta @zelinh This fix needs to be backported to all branches to which this PR got merged, including 1.x and 1.3 branches. 1.3.4 release is being blocked by this fix. Would you please help add the backport labels? I don't have the permissions.

@reta
Copy link
Collaborator

reta commented Jul 11, 2022

@reta @zelinh This fix needs to be backported to all branches to which this PR got merged, including 1.x and 1.3 branches. 1.3.4 release is being blocked by this fix. Would you please help add the backport labels? I don't have the permissions.

We have somewhat messy state here, 2.0 and 2.1 are missed:

@mch2
Copy link
Member

mch2 commented Jul 12, 2022

Initiated a backport of the version bump to 2.1 - #3855. Waiting on this for passing gradle checks.

reta pushed a commit that referenced this pull request Jul 12, 2022
#3848) (#3853)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)

Co-authored-by: Craig Perkins <cwperx@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 12, 2022
#3848)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 12, 2022
#3848)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 12, 2022
#3848)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)
@mch2
Copy link
Member

mch2 commented Jul 12, 2022

I don't think we actually need this change in 2.x or main correct @cwperks? Only for 1.3.4?

@cwperks
Copy link
Member Author

cwperks commented Jul 12, 2022

@mch2 The fix should be backported to all branches that received the netty 4.1.73.Final -> 4.1.78.Final update. Since the transport-netty4 module requires netty-handler we should be pulling in required transitive dependencies so that anywhere this module is imported all of the required classes can be found.

reta pushed a commit that referenced this pull request Jul 12, 2022
#3848) (#3862)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)

Co-authored-by: Craig Perkins <cwperx@amazon.com>
reta pushed a commit that referenced this pull request Jul 12, 2022
#3848) (#3860)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)

Co-authored-by: Craig Perkins <cwperx@amazon.com>
mch2 pushed a commit that referenced this pull request Jul 12, 2022
#3848) (#3861)

* Add netty-transport-native-unix-common to modules/transport-netty4/build.gradle to satisfy transitive dependency

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

* Update third party audit check

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 33138ed)

Co-authored-by: Craig Perkins <cwperx@amazon.com>
@cwperks
Copy link
Member Author

cwperks commented Sep 7, 2022

@dreamer-89 I think this needs to be backported to 2.0 as well.

@dreamer-89
Copy link
Member

@dreamer-89 I think this needs to be backported to 2.0 as well.

#4440 is merged to 2.0. Are you talking about a different backport @cwperks ?

@cwperks
Copy link
Member Author

cwperks commented Sep 7, 2022

@dreamer-89 This change needs to be on all branches that were upgraded to netty >=4.1.78.Final. See details in the PR description.

The upgrade from Netty 4.1.73.Final -> 4.1.78.Final introduced a new transitive dependency on netty-handler which is not getting included in the netty-transport4 module and causing build failures for 1.3.4 because the security plugin pulls in this module and uses the impacted jar.

This change pulls in the transitive dependency on netty-transport-native-unix-common into the module's build.gradle.

Details of the change in Netty can be found here: netty/netty@bc085b0

As you pointed out in this comment (#4440 (comment)), one of the reasons the backport PR failed was because a SHA file for this jar was present. Since this change was not backported to 2.0 it does not have the additional dependency.

See @cliu123 's comment here (opensearch-project/security#1934 (comment)) to see why this is necessary for the security plugin. The security plugin uses SSLHandler which requires this dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.x backport 1.3 Backport to 1.3 branch backport 2.x Backport to 2.x branch backport 2.1 dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants