Update TestClustersPlugin to accept system props installedPlugins and security.enabled for convenience installing plugins for integ tests#20393
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
📝 WalkthroughWalkthroughThe TestClustersPlugin now accepts system properties to automatically configure installed plugins and security for integration tests. It resolves plugins from Maven repositories or local projects, downloads security certificates when needed, and applies security configurations to test cluster nodes. Changes
Sequence Diagram(s)sequenceDiagram
participant TP as TestClustersPlugin
participant PROJ as Local Project
participant MVN as Maven Repository
participant FS as Filesystem
participant CERT as Remote Cert Server
participant CLUSTER as Test Cluster
TP->>TP: Read system properties<br/>(installedPlugins, security.enabled)
loop For each plugin
alt Local project plugin
TP->>PROJ: Locate plugin artifact
PROJ-->>TP: Return plugin path
else Maven-resolved plugin
TP->>MVN: Setup repositories<br/>(mavenLocal, snapshots)
TP->>MVN: Resolve plugin artifact<br/>(with version/snapshot)
MVN-->>TP: Return plugin artifact
end
end
alt Security enabled
TP->>CERT: Check for certificate files
FS->>FS: Create security-resources dir
TP->>CERT: Download missing certs
CERT-->>FS: Store certificates
FS-->>TP: Cert paths ready
end
TP->>CLUSTER: Install plugins
TP->>CLUSTER: Apply security settings<br/>(TLS, certs, admin DN, audit)
CLUSTER-->>TP: Configuration complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
Opened a PR in the job-scheduler to show how these changes in core will simplify the job-scheduler repo: opensearch-project/job-scheduler#873 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java:
- Around line 153-182: The security plugin configuration is only applied for
Maven-resolved plugins; when a local project is detected via
project.findProject(":plugins:" + pluginName) the configureSecurityPlugin(...)
call is skipped. Fix this by adding the same conditional check used later to
call configureSecurityPlugin(project, cluster) immediately after the local
branch where cluster.plugin("plugins:" + pluginName) is invoked (i.e., inside
the if block that checks project.findProject), so that pluginName containing
"opensearch-security" triggers configureSecurityPlugin for local project plugins
as well.
- Around line 203-222: The certificate download loop in TestClustersPlugin (the
for loop iterating SECURITY_CERT_FILES) is insecure and racy: replace the
on-the-fly download logic with a safe strategy — either bundle certificates as
resources/artifacts or, if remote retrieval is required, implement checksum
verification and an atomic, locked download flow: compute/obtain expected
checksums (per cert name), download to a temp file, verify the checksum, then
atomically move/rename into place, and guard concurrent access with a file lock
(or create-exclusive temp file + Files.move ATOMIC_MOVE) to prevent simultaneous
writers; update the RuntimeException message in the download block to include
the cert name and the underlying exception details to aid debugging.
- Around line 229-232: Replace the cluster.getNodes().forEach(...) call with
cluster.getNodes().all(...) so security certificate files and settings are
applied to nodes added later (e.g., via setNumberOfNodes()); inside the all(...)
lambda, keep the logic that iterates SECURITY_CERT_FILES and calls
node.extraConfigFile(...) and any other per-node security configuration so every
current and future node receives the certificate and security settings.
🧹 Nitpick comments (2)
buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java (2)
199-199: Deprecated API:project.getBuildDir()is deprecated.
Project.getBuildDir()was deprecated in Gradle 7.1. Consider using the layout API for forward compatibility.♻️ Proposed fix
- File buildDir = project.getBuildDir(); - File securityResourcesDir = new File(buildDir, "security-resources"); + File securityResourcesDir = project.getLayout().getBuildDirectory().dir("security-resources").get().getAsFile();
165-165: Add a comment explaining the version transformation logic.The version manipulation
.replace("-SNAPSHOT", ".0-SNAPSHOT")is unclear. If the security plugin uses a different versioning scheme (e.g.,3.0.0.0-SNAPSHOTvs3.0.0-SNAPSHOT), please document this for future maintainers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdbuildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
🔇 Additional comments (4)
CHANGELOG.md (1)
13-13: LGTM!The changelog entry is well-formatted and correctly placed in the "Added" section.
buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java (3)
245-245: Verify YAML formatting foradmin_dnsetting.The embedded newline and YAML list syntax (
"\n - CN=...") in the value string is unusual. Verify this produces valid OpenSearch configuration when serialized. The typical pattern would be a JSON array format or proper YAML list handling.
38-66: LGTM on imports.All new imports are used in the added functionality.
121-124: Integration point is well-placed.The call to
configureInstalledPluginsafter registering cluster hooks is appropriate. Note the issues flagged in other comments regarding the implementation details.
| if (installedPluginsProp != null) { | ||
| try { | ||
| plugins.addAll((List<String>) groovy.util.Eval.me(installedPluginsProp)); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("Invalid installedPlugins format. Expected list like: ['plugin1','plugin2']", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Code injection vulnerability via groovy.util.Eval.me().
Using groovy.util.Eval.me() to parse user input is dangerous. An attacker could inject arbitrary Groovy code via the installedPlugins system property, leading to arbitrary code execution during the build.
Consider using a safe JSON parser or simple string parsing instead.
🔒 Proposed fix using simple string parsing
- if (installedPluginsProp != null) {
- try {
- plugins.addAll((List<String>) groovy.util.Eval.me(installedPluginsProp));
- } catch (Exception e) {
- throw new IllegalArgumentException("Invalid installedPlugins format. Expected list like: ['plugin1','plugin2']", e);
- }
- }
+ if (installedPluginsProp != null) {
+ // Parse simple list format: ['plugin1','plugin2'] or plugin1,plugin2
+ String cleaned = installedPluginsProp.trim();
+ if (cleaned.startsWith("[") && cleaned.endsWith("]")) {
+ cleaned = cleaned.substring(1, cleaned.length() - 1);
+ }
+ for (String part : cleaned.split(",")) {
+ String pluginName = part.trim().replaceAll("^['\"]|['\"]$", "");
+ if (!pluginName.isEmpty()) {
+ plugins.add(pluginName);
+ }
+ }
+ }| for (String pluginName : plugins) { | ||
| // Check if it's a local project plugin | ||
| if (project.findProject(":plugins:" + pluginName) != null) { | ||
| cluster.plugin("plugins:" + pluginName); | ||
| } else { | ||
| // Resolve from Maven | ||
| project.getRepositories().mavenLocal(); | ||
| project.getRepositories().maven(repo -> { | ||
| repo.setName("OpenSearch Snapshots"); | ||
| repo.setUrl("https://ci.opensearch.org/ci/dbc/snapshots/maven/"); | ||
| }); | ||
|
|
||
| String version = VersionProperties.getOpenSearch().replace("-SNAPSHOT", ".0-SNAPSHOT"); | ||
| String coords = pluginName.contains(":") ? pluginName : (pluginName + ":" + version); | ||
| if (!coords.contains("@")) { | ||
| coords += "@zip"; | ||
| } | ||
| String fullCoords = coords.split(":").length < 3 ? "org.opensearch.plugin:" + coords : coords; | ||
|
|
||
| org.gradle.api.artifacts.Configuration config = project.getConfigurations() | ||
| .detachedConfiguration(project.getDependencies().create(fullCoords)); | ||
| config.getResolutionStrategy().cacheChangingModulesFor(0, "seconds"); | ||
| cluster.plugin(project.getLayout().file(project.provider(() -> config.getSingleFile()))); | ||
|
|
||
| // Configure security if opensearch-security plugin is installed | ||
| if (pluginName.contains("opensearch-security")) { | ||
| configureSecurityPlugin(project, cluster); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Security plugin configuration skipped for local project installations.
When opensearch-security is found as a local project (line 155-156), the configureSecurityPlugin() method is never called. This means local builds of the security plugin won't have the necessary TLS/certificate configuration applied, causing inconsistent behavior between local and Maven-resolved plugins.
🐛 Proposed fix
for (String pluginName : plugins) {
// Check if it's a local project plugin
if (project.findProject(":plugins:" + pluginName) != null) {
cluster.plugin("plugins:" + pluginName);
+ // Configure security if opensearch-security plugin is installed locally
+ if (pluginName.contains("opensearch-security")) {
+ configureSecurityPlugin(project, cluster);
+ }
} else {
// Resolve from Maven
// ... existing code ...
}
}🤖 Prompt for AI Agents
In
@buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java
around lines 153 - 182, The security plugin configuration is only applied for
Maven-resolved plugins; when a local project is detected via
project.findProject(":plugins:" + pluginName) the configureSecurityPlugin(...)
call is skipped. Fix this by adding the same conditional check used later to
call configureSecurityPlugin(project, cluster) immediately after the local
branch where cluster.plugin("plugins:" + pluginName) is invoked (i.e., inside
the if block that checks project.findProject), so that pluginName containing
"opensearch-security" triggers configureSecurityPlugin for local project plugins
as well.
| for (String certFile : SECURITY_CERT_FILES) { | ||
| File localFile = new File(securityResourcesDir, certFile); | ||
| if (!localFile.exists()) { | ||
| try { | ||
| localFile.getParentFile().mkdirs(); | ||
| try ( | ||
| InputStream in = URI.create(SECURITY_CERTS_URL + certFile).toURL().openStream(); | ||
| FileOutputStream out = new FileOutputStream(localFile) | ||
| ) { | ||
| byte[] buffer = new byte[8192]; | ||
| int bytesRead; | ||
| while ((bytesRead = in.read(buffer)) != -1) { | ||
| out.write(buffer, 0, bytesRead); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to download security certificate: " + certFile, e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Security and reliability concerns with certificate download.
-
Supply chain risk: Certificates are downloaded from a remote URL without integrity verification (no checksums). A compromised GitHub or MITM attack could inject malicious certificates.
-
Race condition: The
!localFile.exists()check followed by download is not atomic. Parallel builds could attempt to download simultaneously, potentially corrupting the file. -
Network reliability: Build failures will occur if the network is unavailable or the URL changes.
Consider:
- Bundling certificates in the repository or a published artifact
- Adding checksum verification for downloaded files
- Using file locking for concurrent access
🔒 Suggested improvement with file locking
File localFile = new File(securityResourcesDir, certFile);
if (!localFile.exists()) {
+ File lockFile = new File(securityResourcesDir, certFile + ".lock");
try {
localFile.getParentFile().mkdirs();
+ // Use file-based locking for concurrent builds
+ try (java.nio.channels.FileChannel channel = new java.io.RandomAccessFile(lockFile, "rw").getChannel();
+ java.nio.channels.FileLock lock = channel.lock()) {
+ if (localFile.exists()) {
+ continue; // Another process downloaded it
+ }
try (
InputStream in = URI.create(SECURITY_CERTS_URL + certFile).toURL().openStream();
FileOutputStream out = new FileOutputStream(localFile)
) {
// ... download logic ...
}
+ }
} catch (Exception e) {
throw new RuntimeException("Failed to download security certificate: " + certFile, e);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java
around lines 203 - 222, The certificate download loop in TestClustersPlugin (the
for loop iterating SECURITY_CERT_FILES) is insecure and racy: replace the
on-the-fly download logic with a safe strategy — either bundle certificates as
resources/artifacts or, if remote retrieval is required, implement checksum
verification and an atomic, locked download flow: compute/obtain expected
checksums (per cert name), download to a temp file, verify the checksum, then
atomically move/rename into place, and guard concurrent access with a file lock
(or create-exclusive temp file + Files.move ATOMIC_MOVE) to prevent simultaneous
writers; update the RuntimeException message in the download block to include
the cert name and the underlying exception details to aid debugging.
| // Configure each node with certificates and security settings | ||
| cluster.getNodes().forEach(node -> { | ||
| // Add certificate files | ||
| Arrays.stream(SECURITY_CERT_FILES).forEach(cert -> node.extraConfigFile(cert, new File(securityResourcesDir, cert))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the OpenSearchCluster class and understand getNodes()
find . -name "OpenSearchCluster.java" -type fRepository: opensearch-project/OpenSearch
Length of output: 155
🏁 Script executed:
# Also search for any class that might define getNodes() for clusters
rg -l "class.*Cluster" --type java | head -20Repository: opensearch-project/OpenSearch
Length of output: 1791
🏁 Script executed:
# Check the TestClustersPlugin file to understand the full context
rg -n "afterEvaluate" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ --type java -B5 -A10Repository: opensearch-project/OpenSearch
Length of output: 2140
🏁 Script executed:
# Search for numberOfNodes to understand how nodes are added
rg -n "numberOfNodes" --type java -C3Repository: opensearch-project/OpenSearch
Length of output: 50385
🏁 Script executed:
# Read the OpenSearchCluster.java file
head -100 buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.javaRepository: opensearch-project/OpenSearch
Length of output: 3839
🏁 Script executed:
# Look for the getNodes() method and node initialization
rg -n "getNodes|addNode|numberOfNodes" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java -A3 -B3Repository: opensearch-project/OpenSearch
Length of output: 2235
🏁 Script executed:
# Check how nodes are created in TestClustersPlugin - look at the full configuration context
sed -n '220,250p' buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.javaRepository: opensearch-project/OpenSearch
Length of output: 2143
🏁 Script executed:
# Search for node creation/addition patterns in the plugin
rg -n "node|Node" buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java | grep -i "add\|create\|new" | head -20Repository: opensearch-project/OpenSearch
Length of output: 206
🏁 Script executed:
# Search for setNumberOfNodes usage to understand when it's typically called
rg -n "setNumberOfNodes" --type java -B5 -A2Repository: opensearch-project/OpenSearch
Length of output: 1003
🏁 Script executed:
# Check if there's documentation or examples of how clusters are configured
find buildSrc -name "*.gradle" -o -name "*.java" | xargs grep -l "setNumberOfNodes" | head -5Repository: opensearch-project/OpenSearch
Length of output: 153
🏁 Script executed:
# Look for how test clusters are typically defined/configured
rg -n "testClusters" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ -A3 -B3 | head -40Repository: opensearch-project/OpenSearch
Length of output: 1462
🏁 Script executed:
# Search for actual usage of setNumberOfNodes in build.gradle files
find . -name "*.gradle" -type f -exec grep -l "setNumberOfNodes\|numberOfNodes" {} \; | head -10Repository: opensearch-project/OpenSearch
Length of output: 462
🏁 Script executed:
# Check one of these build.gradle files to see how testClusters are configured
rg -n "testClusters\|numberOfNodes" -A5 -B5 $(find . -name "*.gradle" -type f | head -3 | tr '\n' ' ')Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check when the configuration freeze happens in OpenSearchCluster
rg -n "checkFrozen\|configurationFrozen\|freeze" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java -B2 -A2Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Look at the full afterEvaluate block to understand the complete configuration flow
sed -n '135,270p' buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.javaRepository: opensearch-project/OpenSearch
Length of output: 7166
🏁 Script executed:
# Search for when freeze() is called on OpenSearchCluster
rg -n "freeze\|freeze()" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ -B3 -A3Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Look for the checkFrozen method to understand the lifecycle
rg -n "checkFrozen" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java -B2 -A5Repository: opensearch-project/OpenSearch
Length of output: 598
🏁 Script executed:
# Check how test cluster DSL is typically used in actual build.gradle files
head -60 ./qa/smoke-test-multinode/build.gradleRepository: opensearch-project/OpenSearch
Length of output: 1584
🏁 Script executed:
# Search for where freeze() is called to understand when configuration is locked
rg -n "\.freeze()" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ -B5 -A2Repository: opensearch-project/OpenSearch
Length of output: 1230
🏁 Script executed:
# Check the task execution flow to understand when configuration is frozen
rg -n "beforeTasks\|afterTasks\|Task\|freeze" buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java | head -30Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check when claimCluster is called relative to task execution
rg -n "claimCluster" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ -B3 -A3Repository: opensearch-project/OpenSearch
Length of output: 1899
🏁 Script executed:
# Look for when nodes can be added after configuration
rg -n "setNumberOfNodes\|addNode" buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.javaRepository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check the full lifecycle - when does configuration actually freeze
rg -n "configurationFrozen\|freeze" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java -B5 -A5Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check if configureEach is applied to newly created nodes
rg -n "configureEach\|allObjects\|whenObjectAdded" buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java -B2 -A3Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Look at line 136 to understand how configureEach is used
sed -n '135,145p' buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.javaRepository: opensearch-project/OpenSearch
Length of output: 668
🏁 Script executed:
# Check if there's a way to listen for node additions
rg -n "node\|Node" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java | grep -i "all\|when\|listen" | head -10Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Look at the nodes container more closely to see if there's a better pattern
sed -n '100,160p' buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.javaRepository: opensearch-project/OpenSearch
Length of output: 2266
🏁 Script executed:
# Check how nodes.all() is used elsewhere - this appears to be the pattern for deferred configuration
rg -n "\.all(" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java -B2 -A2Repository: opensearch-project/OpenSearch
Length of output: 135
🏁 Script executed:
# Verify the nodes container supports all() method (NamedDomainObjectContainer)
rg -n "NamedDomainObjectContainer" buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.javaRepository: opensearch-project/OpenSearch
Length of output: 267
🏁 Script executed:
# Check if there are other forEach usages that might have the same issue
rg -n "\.forEach\(" buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java | grep -i nodeRepository: opensearch-project/OpenSearch
Length of output: 396
🏁 Script executed:
# Check if setNumberOfNodes is always called before afterEvaluate in actual usage
rg -n "setNumberOfNodes\|numberOfNodes" $(find . -name "build.gradle" -type f) -B2 -A2 | head -50Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check the Gradle container documentation behavior - does all() apply to future items?
# Let's look at how nodes.all() is used elsewhere in the codebase
rg -n "all\(" buildSrc/src/main/java/org/opensearch/gradle/testclusters/ -B2 -A3Repository: opensearch-project/OpenSearch
Length of output: 22957
🏁 Script executed:
# Let's verify the exact flow - when is configureSecurityPlugin called vs when can setNumberOfNodes be called
sed -n '310,325p' buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.javaRepository: opensearch-project/OpenSearch
Length of output: 928
Use nodes.all() instead of forEach() to ensure dynamically-added nodes receive security configuration.
The forEach() call only applies settings to nodes that exist when the method is executed. If nodes are added later via setNumberOfNodes(), they won't receive the security certificate and settings. Replace the forEach() with all(), which is the established pattern throughout OpenSearchCluster for applying configurations to all nodes (current and future).
Instead of:
cluster.getNodes().forEach(node -> {
Arrays.stream(SECURITY_CERT_FILES).forEach(cert -> node.extraConfigFile(cert, new File(securityResourcesDir, cert)));
// ... settings
});Use:
cluster.getNodes().all(node -> {
Arrays.stream(SECURITY_CERT_FILES).forEach(cert -> node.extraConfigFile(cert, new File(securityResourcesDir, cert)));
// ... settings
});🤖 Prompt for AI Agents
In
@buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClustersPlugin.java
around lines 229 - 232, Replace the cluster.getNodes().forEach(...) call with
cluster.getNodes().all(...) so security certificate files and settings are
applied to nodes added later (e.g., via setNumberOfNodes()); inside the all(...)
lambda, keep the logic that iterates SECURITY_CERT_FILES and calls
node.extraConfigFile(...) and any other per-node security configuration so every
current and future node receives the certificate and security settings.
I am very much unsure about this change (the security plugin was also a stretch but at least it is quite contained), now we essentially have not only bidirectional (binary) dependencies (core <-> plugins), it would also be unfair for plugins outside the OpenSearch organization. I think the May be we could come up with |
|
This would greatly simplify integ test setup for many plugins. The security configuration only takes place in 2 scenarios:
I can remove 2) if that is too much coupling. Also happy to explore a separate gradle plugin route, but IMO its more sensible within the current TestClusterPlugin. As always, thank you for reviewing @reta ! |
|
❌ Gradle check result for bf72ab7: 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? |
I will try to find the time to explore the idea, thanks @cwperks |
|
❌ Gradle check result for bf72ab7: 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? |
|
This PR is stalled because it has been open for 30 days with no activity. |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 296f912.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 6257992.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
2 participants
Description
This PR adds support in the TestClustersPlugin to allow plugins utilizing testclusters to specify:
For convenience to install general plugins or to install the security plugin and have it run with the demo security configuration.
Currently, many plugins re-implement similar logic to install plugins and I believe its simpler to have a common core solution that can be used.
This also allows the certs to be read on the test classpath because many plugins require the admin certificates to run requests to the cluster utilizing the admin cert (
adminClient)Related Issues
Resolves opensearch-project/opensearch-plugins#245
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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.