-
Notifications
You must be signed in to change notification settings - Fork 126
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
Install security plugin from individual artifacts #1307
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1307 +/- ##
=========================================
Coverage 85.10% 85.10%
Complexity 1251 1251
=========================================
Files 162 162
Lines 5101 5101
Branches 477 477
=========================================
Hits 4341 4341
Misses 554 554
Partials 206 206 ☔ View full report in Codecov by Sentry. |
@@ -177,6 +195,8 @@ dependencies { | |||
testImplementation group: 'org.objenesis', name: 'objenesis', version: '3.2' | |||
testImplementation group: 'net.bytebuddy', name: 'byte-buddy-agent', version: '1.14.7' | |||
testFixturesImplementation "org.opensearch:common-utils:${version}" | |||
|
|||
zipArchive group: 'org.opensearch.plugin', name:'opensearch-security', version: "${opensearch_build}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this add a dependency on security plugin? In other words, should user install security plugin before using knn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add a dependency for when the user wants to spin up a gradlew cluster using security.
For instance, if gradlew run
is executed, it will not require this dependency. Only if ./gradlew run -Dsecurity.enabled=1
is run will it pull it.
I verified this by changing the version to 3.1 (which doesnt exist). When running:
./gradlew run -Dsecurity.enabled=1
Picked up JAVA_TOOL_OPTIONS: -Dlog4j2.formatMsgNoLookups=true
FAILURE: Build failed with an exception.
* Where:
Build file '/Users/jmazane/workspace/Opensearch/DockerRunner/k-NN-1/build.gradle' line: 287
* What went wrong:
A problem occurred evaluating root project 'opensearch-knn'.
> Could not resolve all files for configuration ':zipArchive'.
> Could not find org.opensearch.plugin:opensearch-security:3.1.
Searched in the following locations:
- https://repo.maven.apache.org/maven2/org/opensearch/plugin/opensearch-security/3.1/opensearch-security-3.1.pom
- file:/Users/jmazane/.m2/repository/org/opensearch/plugin/opensearch-security/3.1/opensearch-security-3.1.pom
- https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/plugin/opensearch-security/3.1/opensearch-security-3.1.pom
- https://plugins.gradle.org/m2/org/opensearch/plugin/opensearch-security/3.1/opensearch-security-3.1.pom
Required by:
project :
but ./gradlew run
has no issue.
We can further separate the tasks if we want to provide more separation, but left it like this for development purposes.
build.gradle
Outdated
systemProperty("username", "admin") | ||
systemProperty("password", "admin") | ||
|
||
extraConfigFile("admin-cert.pem", new File("$rootDir/src/test/resources/security/admin-cert.pem")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you explored to use plugins.security.ssl.transport.keystore_filepath
instead of pem file? Wondering if plugins.security.ssl.transport.keystore_filepath
could end up simpler setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a little bit, but was not able to get it to work. I dont see any keys in the https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java#L581 where it the call is getting hung up. I reached out to security team to see if they have more info on it.
build.gradle
Outdated
systemProperty "https", System.getProperty("https") | ||
systemProperty "user", System.getProperty("user") | ||
systemProperty "password", System.getProperty("password") | ||
systemProperty("tests.opensearch.https", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be tests.opensearch.secure
See security BWC tests here which spin up testclusters w/ security: https://github.com/opensearch-project/security/blob/main/.github/actions/run-bwc-suite/action.yaml#L47-L49
adc2d58
to
9bf3e27
Compare
6b8a2d1
to
6882d45
Compare
WIP commit that tries to create a local cluster using the demo certificates that the security plugin provides. The goal of this change is to run security tests directly instead of through using the docker image. It pulls the individual artifacts from the snapshot repo. Right now, it is able to bring up a cluster. However, the wait for cluster health yellow checks are failing, so the tests are not able to run. Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
6882d45
to
1a54cf8
Compare
@@ -139,22 +136,6 @@ public void testKNNModelDefault() throws Exception { | |||
} | |||
} | |||
|
|||
// KNN Delete Model test for model in Training State | |||
public void testDeleteTrainingModel() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic no longer works because with @ryanbogan change, we mark models stuck in training state as failed. So, I deleted it. Also, we should avoid directly accessing the system index in future tests.
Signed-off-by: John Mazanec <[email protected]>
81a57c1
to
db931fe
Compare
cluster.extraConfigFile(file, local) | ||
} | ||
|
||
cluster.setting("plugins.security.ssl.transport.pemcert_filepath", "esnode.pem") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please include the ref where the list of plugins and indices can be checked? This may be required for future plugin maintenance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by plugins/indices that can be checked? Do you mean where this setting is coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any source that we can use in future to update the list of settings if our tests start failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see let me add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-gaievski added
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
This reverts commit 0d6222c. Signed-off-by: John Mazanec <[email protected]>
Windows is failing with flaky test: #1368. |
The backport to
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-1307-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8d60054ab1d5a71a323d545060841f7063e7eba6
# Push it to GitHub
git push --set-upstream origin backport/backport-1307-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 |
…#1307) Changes how security tests are executed. Instead of setting up docker container with security enabled, we now can directly spin up a gradle local cluster with security which we can use to run tests against. To enable this option, we just have to pass `-Dsecurity.enabled=true` as a flag. Along with this, some refactoring was done for the ODFERestTestCase for configuring the client and cleaning up. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 8d60054)
…#1307) Changes how security tests are executed. Instead of setting up docker container with security enabled, we now can directly spin up a gradle local cluster with security which we can use to run tests against. To enable this option, we just have to pass `-Dsecurity.enabled=true` as a flag. Along with this, some refactoring was done for the ODFERestTestCase for configuring the client and cleaning up. Signed-off-by: John Mazanec <[email protected]>
Description
Create a local cluster using the demo certificates that the security plugin provides. The goal of this change is to run security tests directly instead of through using the docker image. It pulls the individual artifacts from the snapshot repo and tries to install them if security is enabled. To do this, in the build.gradle file, if the option
-Dsecurity.enabled=true
is passed, we configure the testcluster.integTest to install the security plugin. This can be seen in the configureSecurityPlugin option.To run cluster:
To run integ tests:
Issues Resolved
#901
Check List
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.