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

[Build Break] Update imports for files refactored in core PR #8157 #3003

Merged
merged 9 commits into from
Jul 17, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Jul 13, 2023

Description

Update imports in security plugin to react to changes introduced in opensearch-project/OpenSearch#8157

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Maintenance

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.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
reta
reta previously approved these changes Jul 13, 2023
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
willyborankin
willyborankin previously approved these changes Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #3003 (51271d0) into main (0e6608d) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3003      +/-   ##
============================================
- Coverage     62.43%   62.27%   -0.16%     
+ Complexity     3380     3368      -12     
============================================
  Files           267      267              
  Lines         19772    19772              
  Branches       3355     3355              
============================================
- Hits          12345    12314      -31     
- Misses         5790     5813      +23     
- Partials       1637     1645       +8     
Impacted Files Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java 58.42% <ø> (ø)
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java 84.48% <ø> (ø)
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java 0.00% <ø> (ø)
...dlic/auth/http/saml/AuthTokenProcessorHandler.java 46.48% <ø> (ø)
...zon/dlic/auth/http/saml/HTTPSamlAuthenticator.java 69.02% <ø> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java 84.38% <ø> (-0.31%) ⬇️
.../action/configupdate/ConfigUpdateNodeResponse.java 77.27% <ø> (ø)
...urity/action/configupdate/ConfigUpdateRequest.java 94.11% <ø> (ø)
...rity/action/configupdate/ConfigUpdateResponse.java 64.28% <ø> (ø)
...tion/configupdate/TransportConfigUpdateAction.java 100.00% <ø> (ø)
... and 76 more

... and 4 files with indirect coverage changes

@cwperks
Copy link
Member Author

cwperks commented Jul 13, 2023

This needs updated common-utils artifacts which may be available now after merge of opensearch-project/common-utils#486

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@@ -69,7 +69,7 @@ repositories {
dependencies {
testImplementation 'com.google.guava:guava:30.0-jre'
testImplementation "org.opensearch.test:framework:${opensearch_version}"
testImplementation 'org.apache.logging.log4j:log4j-core:2.17.1'
testImplementation 'org.apache.logging.log4j:log4j-core:2.20.0'
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this to match the version in core to try to resolve the issue in bwc test. I'm not sure why this dependency is required here or if the version from core's buildSrc/version.properties can be referenced in this build.gradle file. This is a separate gradle build file from the one at the top level.

Copy link
Member Author

Choose a reason for hiding this comment

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

@peternied @willyborankin Are either of you available to help diagnose the issue with bwc test on this PR? I'm not very familiar with testclusters and not sure if any change needs to be done here or another branch to get those checks working.

Copy link
Member Author

Choose a reason for hiding this comment

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

The root cause is:

»  java.lang.NoClassDefFoundError: org/opensearch/core/common/settings/SecureString
»  	at org.opensearch.security.ssl.SecureSSLSettings$InsecureFallbackStringSetting.<init>(SecureSSLSettings.java:113)
»  	at org.opensearch.security.ssl.SecureSSLSettings$SSLSetting.asSetting(SecureSSLSettings.java:78)
»  	at org.opensearch.security.ssl.SecureSSLSettings.lambda$getSecureSettings$0(SecureSSLSettings.java:101)
»  	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271)
»  	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
»  	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
»  	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
»  	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
»  	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
»  	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
»  	at org.opensearch.security.ssl.SecureSSLSettings.getSecureSettings(SecureSSLSettings.java:102)

but I can't tell why that class isn't found because it exists on the main branch of core. It does not exist on 2.x, but in 2.x it uses the correct import for that class.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like its not using the HEAD of main for core when building the testcluster based off main

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding BWC it does not use OS versioning for log4j ... I missed it when prepared fix in other places

Copy link
Collaborator

@willyborankin willyborankin Jul 13, 2023

Choose a reason for hiding this comment

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

@cwperks hope it will fix #3004

Copy link
Member Author

@cwperks cwperks Jul 13, 2023

Choose a reason for hiding this comment

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

@willyborankin I invited you to be a collaborator on my fork if you want to push changes directly to this branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this line in the bwc test workflow mean its checking out main instead of the PR branch? https://github.com/opensearch-project/security/blob/main/.github/workflows/bwc-tests.yml#L41

Copy link
Collaborator

@willyborankin willyborankin Jul 13, 2023

Choose a reason for hiding this comment

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

@cwperks now it fails since it takes latest version. If i understand gradle script. So theoretically after merging of this PR it should start building the plugin :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this line in the bwc test workflow mean its checking out main instead of the PR branch? https://github.com/opensearch-project/security/blob/main/.github/workflows/bwc-tests.yml#L41

yes it is. thats why i think if we merge your changes most probably it will fix BWC since previous problems were related to libraries only

cwperks added 2 commits July 13, 2023 11:44
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
reta
reta previously approved these changes Jul 13, 2023
@reta
Copy link
Collaborator

reta commented Jul 13, 2023

@cwperks it looks to me we have to move 2.x to 2.10.0-SNAPSHOT first, on it ... and we are stuck #3005

RyanL1997
RyanL1997 previously approved these changes Jul 13, 2023
@cwperks
Copy link
Member Author

cwperks commented Jul 13, 2023

Thank you @reta! I was hoping to unblock the release notes PR, but I'll instead make a targeted manual backport to ensure that gets into the 2.9 branch

@cwperks
Copy link
Member Author

cwperks commented Jul 13, 2023

Release notes PR targeting 2.9 branch: #3007

@willyborankin
Copy link
Collaborator

Thank you @reta! I was hoping to unblock the release notes PR, but I'll instead make a targeted manual backport to ensure that gets into the 2.9 branch

@cwperks I think in any case we need to merge this PR first

willyborankin
willyborankin previously approved these changes Jul 14, 2023
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks dismissed stale reviews from willyborankin, RyanL1997, and reta via c1eb5b8 July 17, 2023 13:56
davidlago
davidlago previously approved these changes Jul 17, 2023
@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2023

Some tests started failing after this PR: opensearch-project/OpenSearch#8084

Updating the tests that assert expected number of HTTP Headers in a response now.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2023

Failing tests:

Tests with failures:
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterMinimalRoundtripSearchTests.testCcs
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterSearchTest.testCcs
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterMinimalRoundtripSearchTests.testCcsDifferentConfig
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterSearchTest.testCcsDifferentConfig
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterMinimalRoundtripSearchTests.testCcsDifferentConfigBoth
 - org.opensearch.security.dlic.dlsfls.DlsFlsCrossClusterSearchTest.testCcsDifferentConfigBoth
 - org.opensearch.security.dlic.dlsfls.DlsTest.testDls

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

cwperks commented Jul 17, 2023

@opensearch-project/engineering-effectiveness I believe the failing checks on this PR are due to a stale minimum distribution of 3.0.0. This PR is needed to unblock many security PRs and make the main branch stable. Can you trigger a new minimum distribution build?

Security needs this change included: opensearch-project/OpenSearch#8157

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2023

@jordarlu Could you help in creating a new min distribution build for 3.0.0 to unblock this PR?

@jordarlu
Copy link

@jordarlu Could you help in creating a new min distribution build for 3.0.0 to unblock this PR?

Hi, @cwperks , pls check out this latest 3.0.0 min build with only the Core - https://build.ci.opensearch.org/job/distribution-build-opensearch/8177/console. thanks

@peternied
Copy link
Member

Looks like the BWC test are having trouble that we might not be able to easly mitigate - let retrigger and make sure we have a clean integration test run and the we can seperately work on the BWC issue

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

cwperks commented Jul 17, 2023

@peternied FYI I ran the failing checks on an EC2 instance and they passed.

git checkout --track cwperks/check-main-build
./gradlew assemble -Dbuild.snapshot=false
git checkout --track origin/2.x
./gradlew assemble -Dbuild.snapshot=false

cd bwc-test
mkdir -p src/test/resources/3.0.0.0
mkdir -p src/test/resources/2.10.0.0

cp ../build/distributions/opensearch-security-3.0.0.0.zip src/test/resources/3.0.0.0
cp ../build/distributions/opensearch-security-2.10.0.0.zip src/test/resources/2.10.0.0
> ./gradlew bwcTestSuite -Dtests.security.manager=false -Dbwc.version.previous=2.10.0.0 -Dbwc.version.next=3.0.0.0 -i

...

BUILD SUCCESSFUL in 3m 38s
8 actionable tasks: 7 executed, 1 up-to-date

Tested using JDK 14

@DarshitChanpura DarshitChanpura merged commit 37aacdc into opensearch-project:main Jul 17, 2023
@cwperks cwperks added the backport 2.x backport to 2.x branch label Jul 19, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3003-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 37aacdc0ceab4462498c9bdf339e6e0b5d30ef80
# Push it to GitHub
git push --set-upstream origin backport/backport-3003-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

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

DarshitChanpura pushed a commit to DarshitChanpura/security that referenced this pull request Jul 19, 2023
…ect#3003)

* Update imports for files refactored in core PR #8157

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

* Update references to old packages in test files

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

* Get remaining bad imports in integrationTest

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

* Update log4j in bwc build.gradle

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

* Use versions.log4j

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

* Also reference guava version

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

* Update integtest.sh

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

* Update tests that expect certain amount of headers in a response

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

* Empty commit

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

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 37aacdc)
cwperks added a commit that referenced this pull request Jul 21, 2023
…3003) (#3036)

* Update imports for files refactored in core PR #8157 (#3003)

* Update imports for files refactored in core PR #8157

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

* Update references to old packages in test files

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

* Get remaining bad imports in integrationTest

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

* Update log4j in bwc build.gradle

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

* Use versions.log4j

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

* Also reference guava version

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

* Update integtest.sh

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

* Update tests that expect certain amount of headers in a response

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

* Empty commit

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

---------

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

* Fixes Index import

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Addresses dlicDlsFlsTest failures

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Addresses PR feedback

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Removed Integration Tests

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

* Updates bwc versions to fix bwc tests

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

---------

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
@peternied peternied changed the title Update imports for files refactored in core PR #8157 [Build Break] Update imports for files refactored in core PR #8157 Aug 31, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants