gradle/security-setup.gradle (2)

9-10: Use exact string matching to avoid false positives.

The current check uses contains('opensearch-security'), which could match plugin names like opensearch-security-analytics or opensearch-security-extended. Consider using exact matching or a more precise pattern.

🔎 Suggested improvement

-ext.securityEnabled = project.hasProperty('installedPlugins') && 
-    project.property('installedPlugins').toString().contains('opensearch-security')
+ext.securityEnabled = project.hasProperty('installedPlugins') && 
+    project.property('installedPlugins').toString().matches('.*[\\[\\s,\'\"]opensearch-security[\\]\\s,\'\"].*')

This ensures the plugin name is properly delimited within the list.

---

17-24: Add error handling for certificate download failures.

Network failures during certificate downloads will cause cryptic build errors. Consider adding try-catch blocks with meaningful error messages to help users diagnose issues.

🔎 Proposed error handling

     tasks.register("download${file.replace('-', '').replace('.', '').capitalize()}") {
       doLast {
         if (!local.exists()) {
           local.parentFile.mkdirs()
-          new URL("https://raw.githubusercontent.com/opensearch-project/security/refs/heads/main/bwc-test/src/test/resources/security/" + file).withInputStream { i ->
-            local.withOutputStream { it << i }
-          }
+          try {
+            new URL("https://raw.githubusercontent.com/opensearch-project/security/refs/heads/main/bwc-test/src/test/resources/security/" + file).withInputStream { i ->
+              local.withOutputStream { it << i }
+            }
+          } catch (IOException e) {
+            throw new GradleException("Failed to download security certificate ${file}: ${e.message}. Ensure you have internet connectivity.", e)
+          }
         }
       }
     }

build.gradle (1)

74-78: Consider extracting the plugin check to avoid duplication.

The same contains('opensearch-security') check appears both here and in gradle/security-setup.gradle (lines 9-10). If you improve the matching logic in one place, remember to update both locations, or consider extracting it to a shared function.

Also, as mentioned in the review of security-setup.gradle, the current string matching could have false positives with plugin names containing opensearch-security as a substring.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

gradle/security-setup.gradle (2)

20-20: Extract the base URL as a constant for maintainability.

The long URL is hardcoded and repeated across task registrations. Extracting it as a constant would improve maintainability and make it easier to update the reference in the future.

🔎 Proposed refactor

 if (securityEnabled) {
+  def SECURITY_CERTS_BASE_URL = "https://raw.githubusercontent.com/opensearch-project/security/refs/heads/main/bwc-test/src/test/resources/security/"
+
   // Download security certificates
   ['esnode.pem', 'esnode-key.pem', 'kirk.pem', 'kirk-key.pem', 'root-ca.pem'].forEach { file ->
     File local = project.layout.buildDirectory.file(file).get().asFile
     tasks.register("download${file.replace('-', '').replace('.', '').capitalize()}") {
       doLast {
         if (!local.exists()) {
           local.parentFile.mkdirs()
-          new URL("https://raw.githubusercontent.com/opensearch-project/security/refs/heads/main/bwc-test/src/test/resources/security/" + file).withInputStream { i ->
+          new URL(SECURITY_CERTS_BASE_URL + file).withInputStream { i ->
             local.withOutputStream { it << i }
           }
         }
       }
     }
   }

---

33-37: Consider reducing duplication in certificate file registration.

The pattern of calling node.extraConfigFile() is repeated five times with similar structure. This could be refactored to reduce duplication and make future changes easier.

🔎 Proposed refactor

       testClusters.runTask.nodes.each { node ->
         // Add certificate files
-        node.extraConfigFile("kirk.pem", project.layout.buildDirectory.file("kirk.pem").get().asFile)
-        node.extraConfigFile("kirk-key.pem", project.layout.buildDirectory.file("kirk-key.pem").get().asFile)
-        node.extraConfigFile("esnode.pem", project.layout.buildDirectory.file("esnode.pem").get().asFile)
-        node.extraConfigFile("esnode-key.pem", project.layout.buildDirectory.file("esnode-key.pem").get().asFile)
-        node.extraConfigFile("root-ca.pem", project.layout.buildDirectory.file("root-ca.pem").get().asFile)
+        ['kirk.pem', 'kirk-key.pem', 'esnode.pem', 'esnode-key.pem', 'root-ca.pem'].each { certFile ->
+          node.extraConfigFile(certFile, project.layout.buildDirectory.file(certFile).get().asFile)
+        }
 
         // Security settings

Configuration used: defaults

Review profile: CHILL

Plan: Pro