Add option to set user for test cluster to perform initial health checks #11641
Conversation
jmazanec15
requested review from
abbashus,
adnapibar,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
dreamer-89,
gbbafna,
kartg,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
ryanbogan,
sachinpkale,
saratvemulapalli,
setiah,
shwetathareja,
sohami,
tlfeng and
VachaShah
as code owners
github-actions
bot
added
bug
|
❌ Gradle check result for 2458c4f: FAILURE 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? |
| @@ -483,16 +483,16 @@ public Stream<String> logLines() throws IOException { | |||
| @Override | |||
| public synchronized void start() { | |||
| LOGGER.info("Starting `{}`", this); | |||
| if (System.getProperty("tests.opensearch.secure") != null | |||
There was a problem hiding this comment.
As you identified the issue, could we not pass tests.opensearch props to the node instead of modifying properties? I am not sure what the impact this change would make (renaming properties) since we are basically modifying publicly available Gradle plugin
There was a problem hiding this comment.
As you identified the issue, could we not pass tests.opensearch props to the node instead of modifying properties?
Yes, we could do this. The main reason I didnt do it was that the credentials did not seem to belong in the system properties for the node itself - in other words, the running node does not need these credentials for anything.
That being said, I was thinking that maybe just adding an option to set the credentials directly via cluster.setCredentialsForHealthCheck or cluster.setCredentials might make the most sense.
There was a problem hiding this comment.
Or @reta , alternatively, I see that there is a user option in the TestClusterConfiguration: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClusterConfiguration.java#L125.
In the OpenSearchNode, its a no-op: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L706, but Im wondering if we can just put the following (which is what we do in the start with the sys props):
@Override
public void user(Map<String, String> userSpec) {
this.credentials.get(0).put("username", userSpec.get("username")))
this.credentials.get(0).put("password", userSpec.get("password")));
}Im not really sure what outside of this health check credentials/user would be used for aside from. @cwperks do you think this sounds good?
There was a problem hiding this comment.
Or @reta , alternatively, I see that there is a
useroption in the TestClusterConfiguration: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClusterConfiguration.java#L125.
You mean as an alternative mechanism to system properties?
There was a problem hiding this comment.
Right, I updated the PR and confirmed it works for my k-NN change locally.
|
@jmazanec15 The security plugin passes these params into the gradle task using Are you able to pass these values into the |
|
@cwperks I think bwc-test will most likely work because it does not have this logic in the task: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/RunTask.java#L143-L152. Passing via I did try setting via gradle commands awhile ago with |
jmazanec15
changed the title
|
❌ Gradle check result for b68d7ba: FAILURE 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? |
| } | ||
|
|
||
| if (userSpec.containsKey("username") && userSpec.containsKey("password")) { | ||
| this.credentials.get(0).put("username", userSpec.get("username")); |
There was a problem hiding this comment.
The credentials would conflict with start method I believe
There was a problem hiding this comment.
For start, we would allow users to override if the sys properties are set. We could get rid of reading from sys properties during start and require user to be used, but I think this would break things for other plugins.
There was a problem hiding this comment.
I am trying to trace where this method is called but the only place I found is OpenSearchCluster::user, could you please help me to figure out who actually populates the userSpec in the first place?
There was a problem hiding this comment.
Right, this would be from the consumer of the gradle plugin. So for instance in k-NN, we configure the test cluster here: https://github.com/opensearch-project/k-NN/blob/main/build.gradle#L259-L280.
For security, it would look something like:
testClusters.integTest {
testDistribution = "ARCHIVE"
// Optionally install security
if (System.getProperty("security.enabled") != null) {
configurations.zipArchive.asFileTree.each {
plugin(provider(new Callable<RegularFile>() {
@Override
RegularFile call() throws Exception {
return new RegularFile() {
@Override
File getAsFile() {
return it
}
}
}
}))
}
user(Map.of("username", "admin", "password", "admin"))
extraConfigFile("admin-cert.pem", new File("$rootDir/src/test/resources/security/admin-cert.pem"))
extraConfigFile("admin-key.pem", new File("$rootDir/src/test/resources/security/admin-key.pem"))
extraConfigFile("node-cert.pem", new File("$rootDir/src/test/resources/security/node-cert.pem"))
extraConfigFile("node-key.pem", new File("$rootDir/src/test/resources/security/node-key.pem"))
extraConfigFile("root-ca.pem", new File("$rootDir/src/test/resources/security/root-ca.pem"))
setting("plugins.security.ssl.transport.pemcert_filepath", "node-cert.pem")
setting("plugins.security.ssl.transport.pemkey_filepath", "node-key.pem")
setting("plugins.security.ssl.transport.pemtrustedcas_filepath", "root-ca.pem")
setting("plugins.security.ssl.transport.enforce_hostname_verification", "false")
setting("plugins.security.ssl.http.enabled", "true")
setting("plugins.security.ssl.http.pemcert_filepath", "node-cert.pem")
setting("plugins.security.ssl.http.pemkey_filepath", "node-key.pem")
setting("plugins.security.ssl.http.pemtrustedcas_filepath", "root-ca.pem")
setting("plugins.security.allow_unsafe_democertificates", "true")
setting("plugins.security.allow_default_init_securityindex", "true")
setting("plugins.security.unsupported.inject_user.enabled", "true")
setting("plugins.security.authcz.admin_dn", "\n- CN=kirk,OU=client,O=client,L=test, C=de")
setting('plugins.security.restapi.roles_enabled', '["all_access", "security_rest_api_access"]')
setting('plugins.security.system_indices.enabled', "true")
setting('plugins.security.system_indices.indices', '[".plugins-ml-config", ".plugins-ml-connector", ".plugins-ml-model-group", ".plugins-ml-model", ".plugins-ml-task", ".plugins-ml-conversation-meta", ".plugins-ml-conversation-interactions", ".opendistro-alerting-config", ".opendistro-alerting-alert*", ".opendistro-anomaly-results*", ".opendistro-anomaly-detector*", ".opendistro-anomaly-checkpoints", ".opendistro-anomaly-detection-state", ".opendistro-reports-*", ".opensearch-notifications-*", ".opensearch-notebooks", ".opensearch-observability", ".ql-datasources", ".opendistro-asynchronous-search-response*", ".replication-metadata-store", ".opensearch-knn-models", ".geospatial-ip2geo-data*"]')
setSecure(true)
}There was a problem hiding this comment.
Right, this would be from the consumer of the gradle plugin.
Gotcha, thanks a lot @jmazanec15 , it looks quite logical to me to allow user credentials configuration this way, so what about altering OpenSearchNode::start() to consult system properties only if credentials were not set? That should not break anyone I think (hard to imagine cluster and node(s) to have different credentials)
There was a problem hiding this comment.
That should not break anyone I think (hard to imagine cluster and node(s) to have different credentials)
Btw: its also possible from consuming plugin to call user per node via something like:
cluster.getFirstNode.user()I think thats an option. However, right now, it looks like the system properties are intended to perform the override:
if (System.getProperty("tests.opensearch.username") != null) {
this.credentials.get(0).put("username", System.getProperty("tests.opensearch.username"));
LOGGER.info("Overwriting username to: " + this.getCredentials().get(0).get("username"));
}
if (System.getProperty("tests.opensearch.password") != null) {
this.credentials.get(0).put("password", System.getProperty("tests.opensearch.password"));
LOGGER.info("Overwriting password to: " + this.getCredentials().get(0).get("password"));
}Id be curious if the security team has any strong preference before doing that.
There was a problem hiding this comment.
Id be curious if the security team has any strong preference before doing that.
Sure, let's wait for their feedback, thanks!
Compatibility status:Checks if related components are compatible with change 0feaa31 Incompatible componentsIncompatible components: [https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/sql.git] |
Avoids adding prefix "tests.opensearch." to system properties that interact with gradle test cluster. Using this prefix causes these properties to be passed to the nodes as settings: https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#run-opensearch. Signed-off-by: John Mazanec <[email protected]>
This reverts commit 2458c4f. Signed-off-by: John Mazanec <[email protected]>
Sets the user option for the OpenSearchNode to set the credentials the node should use. This allows us to avoid passing in the username and password as system properties in order to launch the cluster with security plugin installed. These credentials will be used for the health check. Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
jmazanec15
force-pushed
the
issue-11219
branch
from
6f70b12
to
50a027c
Compare
|
❌ Gradle check result for 6f70b12: FAILURE 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? |
|
❌ Gradle check result for 50a027c: FAILURE 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: John Mazanec <[email protected]>
|
❕ Gradle check result for 0feaa31: UNSTABLE
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11641 +/- ##
============================================
- Coverage 71.46% 71.35% -0.11%
+ Complexity 59257 59160 -97
============================================
Files 4906 4906
Lines 278184 278194 +10
Branches 40421 40424 +3
============================================
- Hits 198791 198497 -294
- Misses 62959 63225 +266
- Partials 16434 16472 +38 ☔ View full report in Codecov by Sentry. |
|
org.opensearch.search.SearchWeightedRoutingIT.testMultiGetWithNetworkDisruption_FailOpenEnabled Test looks flaky: #10755. This code will not impact that. |
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
|
@jmazanec15 What do we need to take this PR forward? You mentioned wanting feedback from the security team. Can we rope in the relevant folks to provide feedback to hopefully get to a conclusion here? |
|
PR was pending review from correct team. We were able to get around it by doing the following for each node in the cluster: https://github.com/opensearch-project/k-NN/blob/main/build.gradle#L95-L102. So we are unblocked. This change would make it so that: |
Description
I am working on adding gradle task for spinning up k-NN+security plugin for integration testing and debugging: opensearch-project/k-NN#1307. Ive been able to get the cluster to come up and pass the health check for the
integTesttask, but forrunhad been failing for awhile with a 401 during the wait for yellow health check: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java#L559:The problem is that it was unable to get the username and password from the credentials. These are set during Node start here: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L485-L497. These need to be set via system properties and cannot be passed via cluster.systemProperty call. So, I modified my task to run:
However, I get the following error:
This is because in the
RunTask, it will pass any system properties with the prefixtests.opensearchas settings to the nodes: https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/RunTask.java#L143-L152. This is documented here: https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#run-opensearch.EDIT
I updated the PR to implement the
useroption in OpenSearchNode. When a user passes in a UserSpec map with "username" and "password" keys, they will be set for the initial credentials. I validated this works forgradlew runfor my change in k-NN.I am not sure if this is the correct solution or not and wanted to get feedback. A few alternatives I could think of:
@scrawfor99 @cwperks let me know if you have any thoughts.
Related Issues
Resolves #11219
Related change: #8900
Check List
New functionality includes testing.All tests passNew functionality has been documented.New functionality has javadoc addedPublic documentation issue/PR createdBy 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.