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.0] Revert to Netty 4.1.79.Final #4440

Merged

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Sep 7, 2022

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

Description

See related issue: #4427

The last minor version of Netty included a change to eagerly load certificates using BouncyCastle if BouncyCastle is on the classpath: netty/netty#12670

This change has led to errors on startup with the security plugin (See #4427) because of a bug in checking all required BouncyCastle dependencies (See netty/netty#12743)

There is another bug related to the change that impacts custom bundles for private keys (netty/netty#12746). Both bugs have since been fixed and are targeted for the next 4.1.X release of netty.

This PR reverts netty until the next 4.1.X (4.1.81.Final) release. The full list of changes in netty 4.1.80.Final can be found here

Issues Resolved

#4427

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)

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.

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

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit fb64a85)
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks requested a review from a team as a code owner September 7, 2022 14:03
@cwperks cwperks mentioned this pull request Sep 7, 2022
6 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member

@cwperks : Both precommit & gradle check failed due to unused sha file netty-transport-native-unix-common-4.1.79.Final.jar.sha1. Can you resolve this issue ?

Pre-commit

> Unused sha files found: 
  netty-transport-native-unix-common-4.1.79.Final.jar.sha1

Gradle check

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':modules:transport-netty4:dependencyLicenses'.
> Unused sha files found: 
  netty-transport-native-unix-common-4.1.79.Final.jar.sha1

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

cwperks commented Sep 7, 2022

@dreamer-89 I removed that license file. That was added in this change (#3848) which was added in 2.1.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Sep 7, 2022

@dreamer-89 could you merge it please? For some reasons I cannot for 2.0

@dreamer-89
Copy link
Member

dreamer-89 commented Sep 7, 2022

@dreamer-89 could you merge it please? For some reasons I cannot for 2.0

Yes, it is showing merging is blocked. I don't see any pending approval step. Do you want me to force merge this change @reta ?

CC @dblock @reta

@kotwanikunal kotwanikunal merged commit 6822d3b into opensearch-project:2.0 Sep 7, 2022
@kotwanikunal
Copy link
Member

@dreamer-89 could you merge it please? For some reasons I cannot for 2.0

Yes, it is showing merging is blocked. I don't see any pending approval step. Do you want me to force merge this change @reta ?

CC @dblock @reta

Merged it in.

@dreamer-89
Copy link
Member

@dreamer-89 could you merge it please? For some reasons I cannot for 2.0

Yes, it is showing merging is blocked. I don't see any pending approval step. Do you want me to force merge this change @reta ?
CC @dblock @reta

Merged it in.

@kotwanikunal : Thanks for the approval.
Any reasons, it wasn't showing merge option even with approvals ?

@kotwanikunal
Copy link
Member

@dreamer-89 It looks like the maintainers need to be backported to 2.0 - https://github.com/opensearch-project/OpenSearch/blob/2.0/MAINTAINERS.md
I don't see you here, which might be causing permission issues.

The PR was still waiting for an approval from opensearch-core, if that helps drill this down further.

@dreamer-89
Copy link
Member

@dreamer-89 It looks like the maintainers need to be backported to 2.0 - https://github.com/opensearch-project/OpenSearch/blob/2.0/MAINTAINERS.md I don't see you here, which might be causing permission issues.

The PR was still waiting for an approval from opensearch-core, if that helps drill this down further.

Thanks @kotwanikunal.

@reta
Copy link
Collaborator

reta commented Sep 7, 2022

@dreamer-89 It looks like the maintainers need to be backported to 2.0 - https://github.com/opensearch-project/OpenSearch/blob/2.0/MAINTAINERS.md I don't see you here, which might be causing permission issues.

The PR was still waiting for an approval from opensearch-core, if that helps drill this down further.

@kartg that's what I mentioned here #4342 (comment)

@kartg
Copy link
Member

kartg commented Sep 9, 2022

@reta I'll try and look into what's going on here. I still don't think it's tied to backporting MAINTAINERS.md since both you (#3598) and @dreamer-89 (#4149) have previously merged PRs into 2.0 😕

cc @kotwanikunal

@kartg
Copy link
Member

kartg commented Sep 9, 2022

Ah, it looks like we didn't backport the change in Codeowners from 2.x to 2.0. The reason why @dreamer-89 found the merge blocked was because, as @kotwanikunal pointed out, no one from opensearch-core had approved this PR.

I've triggered the backport now - #4475

Sorry about that @reta ! 😅

@reta
Copy link
Collaborator

reta commented Sep 9, 2022

Thanks a mill, @kartg and @kotwanikunal !

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.

5 participants