test(integ-test): gate optional integration environments

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d99219b)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources block ensures closure on normal completion, an exception during readAllBytes() (e.g., network interruption) leaves the stream open, potentially exhausting connection pool resources in long-running test suites. try (InputStream content = response.getEntity().getContent()) { body = new String(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d99219b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Cache system property check result The Boolean.getBoolean() call is evaluated on every test method execution. Consider caching this system property check in a static final field to avoid repeated lookups and improve performance, especially when running multiple test methods. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [40-48] +private static final boolean REQUIRE_REMOTE_CLUSTER = + Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY); + ClusterPlugins.requireOrAssume( HAS_REMOTE_CLUSTER, - Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY), + REQUIRE_REMOTE_CLUSTER, "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" + " 'remote*' cluster); skipping", "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" + " 'remote*' cluster) but none was found, and this task marks it required via -D" + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY); Suggestion importance[1-10]: 5 __ Why: While caching Boolean.getBoolean() in a static field would avoid repeated lookups, the performance gain is minimal since this is only called once per test class in a @BeforeClass method. The suggestion is technically correct but offers marginal improvement.	Low

---

Previous suggestions

Suggestions up to commit 0179dc6

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) { +for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume( - HAS_REMOTE_CLUSTER, - Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY), - "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" - + " 'remote*' cluster); skipping", - "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" - + " 'remote*' cluster) but none was found, and this task marks it required via -D" - + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY); +@BeforeClass +public static void requireRemoteCluster() { + try { + ClusterPlugins.requireOrAssume( + HAS_REMOTE_CLUSTER, + Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY), + "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" + + " 'remote*' cluster); skipping", + "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a" + + " 'remote*' cluster) but none was found, and this task marks it required via -D" + + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY); + } catch (Exception e) { + throw new RuntimeException("Failed to verify remote cluster configuration", e); + } +} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) { - remote = cluster; + String trimmed = cluster.trim(); + if (trimmed.startsWith("remote")) { + remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8); + byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit + if (content.read() != -1) { + throw new IOException("Response exceeds maximum allowed size"); + } + body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName); +return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) { - remote = cluster; + if (cluster.trim().startsWith("remote")) { + remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[dai-chen]

Just thinking is it possible to make this like our capability annotation, like `@RequiresEnv("xxx-plugin") I see currently CapabilityRule is initialized in base class and may access to the client?

[mengweieric]

Good idea. A rule can access the static client(), but CapabilityRule runs before @Before initializes it on the first method; these probes are at the start of init() so they can skip before index and security setup. The remote-cluster path also resolves static-final values at class load, so I would keep this PR scoped and handle an annotation refactor separately.

[github-actions]

Persistent review updated to latest commit d99219b