Simplify adding plugins and modules to testclusters (#61886)

There are currently half a dozen ways to add plugins and modules for
test clusters to use. All of them require the calling project to peek
into the plugin or module they want to use to grab its bundlePlugin
task, and then both depend on that task, as well as extract the archive
path the task will produce. This creates cross project dependencies that
are difficult to detect, and if the dependent plugin/module has not yet
been configured, the build will fail because the task does not yet
exist.

This commit makes the plugin and module methods for testclusters
symmetetric, and simply adding a file provider directly, or a project
path that will produce the plugin/module zip. Internally this new
variant uses normal configuration/dependencies across projects to get
the zip artifact. It also has the added benefit of no longer needing the
caller to add to the test task a dependsOn for bundlePlugin task.

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

@@ -28,7 +28,6 @@ import org.elasticsearch.gradle.info.BuildParams
 import org.elasticsearch.gradle.test.RestIntegTestTask
 import org.elasticsearch.gradle.test.RestTestBasePlugin
 import org.elasticsearch.gradle.testclusters.RunTask
-import org.elasticsearch.gradle.testclusters.TestClustersPlugin
 import org.elasticsearch.gradle.util.Util
 import org.gradle.api.InvalidUserDataException
 import org.gradle.api.Plugin
@@ -44,9 +43,6 @@ import org.gradle.api.tasks.TaskProvider
 import org.gradle.api.tasks.bundling.Zip
 import org.gradle.jvm.tasks.Jar
 
-import java.util.regex.Matcher
-import java.util.regex.Pattern
-
 /**
  * Encapsulates build configuration for an Elasticsearch plugin.
  */
@@ -81,10 +77,9 @@ class PluginBuildPlugin implements Plugin<Project> {
             project.extensions.getByType(PluginPropertiesExtension).extendedPlugins.each { pluginName ->
                 // Auto add dependent modules to the test cluster
                 if (project.findProject(":modules:${pluginName}") != null) {
-                    project.integTest.dependsOn(project.project(":modules:${pluginName}").tasks.bundlePlugin)
-                    project.testClusters.integTest.module(
-                            project.project(":modules:${pluginName}").tasks.bundlePlugin.archiveFile
-                    )
+                    project.testClusters.all {
+                        module(":modules:${pluginName}")
+                    }
                 }
             }
             PluginPropertiesExtension extension1 = project.getExtensions().getByType(PluginPropertiesExtension.class)

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java

@@ -27,7 +27,6 @@ import org.gradle.api.Named;
 import org.gradle.api.NamedDomainObjectContainer;
 import org.gradle.api.Project;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
 import org.gradle.api.provider.Provider;
@@ -37,7 +36,6 @@ import org.gradle.api.tasks.Nested;
 import java.io.File;
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.security.GeneralSecurityException;
@@ -138,28 +136,13 @@ public class ElasticsearchCluster implements TestClusterConfiguration, Named {
     }
 
     @Override
-    public void plugin(URI plugin) {
+    public void plugin(Provider<RegularFile> plugin) {
         nodes.all(each -> each.plugin(plugin));
     }
 
     @Override
-    public void plugin(File plugin) {
-        nodes.all(each -> each.plugin(plugin));
-    }
-
-    @Override
-    public void plugin(Provider<URI> plugin) {
-        nodes.all(each -> each.plugin(plugin));
-    }
-
-    @Override
-    public void plugin(RegularFileProperty plugin) {
-        nodes.all(each -> each.plugin(plugin));
-    }
-
-    @Override
-    public void module(File module) {
-        nodes.all(each -> each.module(module));
+    public void plugin(String pluginProjectPath) {
+        nodes.all(each -> each.plugin(pluginProjectPath));
     }
 
     @Override
@@ -167,6 +150,11 @@ public class ElasticsearchCluster implements TestClusterConfiguration, Named {
         nodes.all(each -> each.module(module));
     }
 
+    @Override
+    public void module(String moduleProjectPath) {
+        nodes.all(each -> each.module(moduleProjectPath));
+    }
+
     @Override
     public void keystore(String key, String value) {
         nodes.all(each -> each.keystore(key, value));

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -38,12 +38,11 @@ import org.gradle.api.Action;
 import org.gradle.api.Named;
 import org.gradle.api.NamedDomainObjectContainer;
 import org.gradle.api.Project;
+import org.gradle.api.artifacts.Configuration;
 import org.gradle.api.file.FileTree;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
-import org.gradle.api.provider.Property;
 import org.gradle.api.provider.Provider;
 import org.gradle.api.tasks.Classpath;
 import org.gradle.api.tasks.Input;
@@ -62,7 +61,6 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.LineNumberReader;
 import java.io.UncheckedIOException;
-import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -71,6 +69,7 @@ import java.nio.file.StandardOpenOption;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -128,7 +127,8 @@ public class ElasticsearchNode implements TestClusterConfiguration {
     private final Path workingDir;
 
     private final LinkedHashMap<String, Predicate<TestClusterConfiguration>> waitConditions = new LinkedHashMap<>();
-    private final List<Provider<URI>> plugins = new ArrayList<>();
+    private final Map<String, Configuration> pluginAndModuleConfigurations = new HashMap<>();
+    private final List<Provider<File>> plugins = new ArrayList<>();
     private final List<Provider<File>> modules = new ArrayList<>();
     final LazyPropertyMap<String, CharSequence> settings = new LazyPropertyMap<>("Settings", this);
     private final LazyPropertyMap<String, CharSequence> keystoreSettings = new LazyPropertyMap<>("Keystore", this);
@@ -265,47 +265,52 @@ public class ElasticsearchNode implements TestClusterConfiguration {
         }
     }
 
-    @Override
-    public void plugin(RegularFileProperty plugin) {
-        this.plugins.add(plugin.map(p -> p.getAsFile().toURI()));
+    // package protected so only TestClustersAware can access
+    @Internal
+    Collection<Configuration> getPluginAndModuleConfigurations() {
+        return pluginAndModuleConfigurations.values();
+    }
+
+    // creates a configuration to depend on the given plugin project, then wraps that configuration
+    // to grab the zip as a file provider
+    private Provider<RegularFile> maybeCreatePluginOrModuleDependency(String path) {
+        Configuration configuration = pluginAndModuleConfigurations.computeIfAbsent(
+            path,
+            key -> project.getConfigurations()
+                .detachedConfiguration(project.getDependencies().project(Map.of("path", path, "configuration", "zip")))
+        );
+        Provider<File> fileProvider = configuration.getElements()
+            .map(
+                s -> s.stream()
+                    .findFirst()
+                    .orElseThrow(() -> new IllegalStateException("zip configuration of project " + path + " had no files"))
+                    .getAsFile()
+            );
+        return project.getLayout().file(fileProvider);
     }
 
     @Override
-    public void plugin(Provider<URI> plugin) {
-        requireNonNull(plugin, "Plugin name can't be null");
+    public void plugin(Provider<RegularFile> plugin) {
         checkFrozen();
-        if (plugins.contains(plugin)) {
-            throw new TestClustersException("Plugin already configured for installation " + plugin);
-        }
-        this.plugins.add(plugin);
+        this.plugins.add(plugin.map(RegularFile::getAsFile));
     }
 
     @Override
-    public void plugin(URI plugin) {
-        Property<URI> uri = project.getObjects().property(URI.class);
-        uri.set(plugin);
-        this.plugin(uri);
-    }
-
-    @Override
-    public void plugin(File plugin) {
-        Property<URI> uri = project.getObjects().property(URI.class);
-        uri.set(plugin.toURI());
-        this.plugin(uri);
-    }
-
-    @Override
-    public void module(File module) {
-        RegularFileProperty file = project.getObjects().fileProperty();
-        file.fileValue(module);
-        this.module(file);
+    public void plugin(String pluginProjectPath) {
+        plugin(maybeCreatePluginOrModuleDependency(pluginProjectPath));
     }
 
     @Override
     public void module(Provider<RegularFile> module) {
+        checkFrozen();
         this.modules.add(module.map(RegularFile::getAsFile));
     }
 
+    @Override
+    public void module(String moduleProjectPath) {
+        module(maybeCreatePluginOrModuleDependency(moduleProjectPath));
+    }
+
     @Override
     public void keystore(String key, String value) {
         keystoreSettings.put(key, value);
@@ -449,7 +454,7 @@ public class ElasticsearchNode implements TestClusterConfiguration {
 
         final List<String> pluginsToInstall = new ArrayList<>();
         if (plugins.isEmpty() == false) {
-            pluginsToInstall.addAll(plugins.stream().map(Provider::get).map(URI::toString).collect(Collectors.toList()));
+            pluginsToInstall.addAll(plugins.stream().map(Provider::get).map(p -> p.toURI().toString()).collect(Collectors.toList()));
         }
 
         if (requiresAddingXPack()) {
@@ -1273,10 +1278,7 @@ public class ElasticsearchNode implements TestClusterConfiguration {
     }
 
     private List<File> getInstalledFileSet(Action<? super PatternFilterable> filter) {
-        return Stream.concat(
-            plugins.stream().map(Provider::get).filter(uri -> uri.getScheme().equalsIgnoreCase("file")).map(File::new),
-            modules.stream().map(p -> p.get())
-        )
+        return Stream.concat(plugins.stream().map(Provider::get), modules.stream().map(Provider::get))
             .filter(File::exists)
             // TODO: We may be able to simplify this with Gradle 5.6
             // https://docs.gradle.org/nightly/release-notes.html#improved-handling-of-zip-archives-on-classpaths
@@ -1286,15 +1288,6 @@ public class ElasticsearchNode implements TestClusterConfiguration {
             .collect(Collectors.toList());
     }
 
-    @Input
-    public Set<URI> getRemotePlugins() {
-        Set<URI> file = plugins.stream()
-            .map(Provider::get)
-            .filter(uri -> uri.getScheme().equalsIgnoreCase("file") == false)
-            .collect(Collectors.toSet());
-        return file;
-    }
-
     @Classpath
     public List<File> getInstalledClasspath() {
         return getInstalledFileSet(filter -> filter.include("**/*.jar"));

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java

@@ -21,13 +21,11 @@ package org.elasticsearch.gradle.testclusters;
 import org.elasticsearch.gradle.FileSupplier;
 import org.elasticsearch.gradle.PropertyNormalization;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logging;
 import org.gradle.api.provider.Provider;
 import org.slf4j.Logger;
 
 import java.io.File;
-import java.net.URI;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -44,18 +42,14 @@ public interface TestClusterConfiguration {
 
     void setTestDistribution(TestDistribution distribution);
 
-    void plugin(URI plugin);
+    void plugin(Provider<RegularFile> plugin);
 
-    void plugin(File plugin);
-
-    void plugin(Provider<URI> plugin);
-
-    void plugin(RegularFileProperty plugin);
-
-    void module(File module);
+    void plugin(String pluginProjectPath);
 
     void module(Provider<RegularFile> module);
 
+    void module(String moduleProjectPath);
+
     void keystore(String key, String value);
 
     void keystore(String key, Supplier<CharSequence> valueSupplier);

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

@@ -20,6 +20,7 @@ package org.elasticsearch.gradle.testclusters;
 
 import org.elasticsearch.gradle.Jdk;
 import org.gradle.api.Task;
+import org.gradle.api.artifacts.Configuration;
 import org.gradle.api.tasks.Nested;
 
 import java.util.Collection;
@@ -41,6 +42,7 @@ public interface TestClustersAware extends Task {
         // Add legacy BWC JDK runtime as a dependency so it's downloaded before starting the cluster if necessary
         cluster.getNodes().stream().map(node -> (Callable<Jdk>) node::getBwcJdk).forEach(this::dependsOn);
 
+        cluster.getNodes().forEach(node -> dependsOn((Callable<Collection<Configuration>>) node::getPluginAndModuleConfigurations));
         getClusters().add(cluster);
     }
 

docs/build.gradle

@@ -94,13 +94,7 @@ project.rootProject.subprojects.findAll { it.parent.path == ':plugins' }.each {
   if (subproj.path.startsWith(':plugins:ingest-attachment') && BuildParams.inFipsJvm) {
     return
   }
-  // FIXME
-  subproj.afterEvaluate { // need to wait until the project has been configured
-    testClusters.integTest {
-      plugin subproj.bundlePlugin.archiveFile
-    }
-    tasks.integTest.dependsOn subproj.bundlePlugin
-  }
+  testClusters.integTest.plugin subproj.path
 }
 
 buildRestTests.docs = fileTree(projectDir) {

modules/ingest-common/build.gradle

@@ -40,7 +40,7 @@ restResources {
 testClusters.all {
   // Needed in order to test ingest pipeline templating:
   // (this is because the integTest node is not using default distribution, but only the minimal number of required modules)
-  module project(':modules:lang-mustache').tasks.bundlePlugin.archiveFile
+  module ':modules:lang-mustache'
 }
 
 thirdPartyAudit.ignoreMissingClasses(

modules/kibana/build.gradle

@@ -28,6 +28,6 @@ dependencies {
 }
 
 testClusters.all {
-  module file(project(':modules:reindex').tasks.bundlePlugin.archiveFile)
+  module ':modules:reindex'
 }
 

modules/lang-painless/build.gradle

@@ -27,7 +27,7 @@ esplugin {
 }
 
 testClusters.all {
-  module project(':modules:mapper-extras').tasks.bundlePlugin.archiveFile
+  module ':modules:mapper-extras'
   systemProperty 'es.scripting.update.ctx_in_params', 'false'
   // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
   systemProperty 'es.transport.cname_in_publish_address', 'true'

modules/rank-eval/build.gradle

@@ -33,5 +33,5 @@ restResources {
 
 testClusters.all {
   // Modules who's integration is explicitly tested in integration tests
-  module project(':modules:lang-mustache').tasks.bundlePlugin.archiveFile
+  module ':modules:lang-mustache'
 }

modules/reindex/build.gradle

@@ -36,8 +36,8 @@ esplugin {
 
 testClusters.all {
   // Modules who's integration is explicitly tested in integration tests
-  module project(':modules:parent-join').tasks.bundlePlugin.archiveFile
-  module project(':modules:lang-painless').tasks.bundlePlugin.archiveFile
+  module ':modules:parent-join'
+  module ':modules:lang-painless'
   // Whitelist reindexing from the local node so we can test reindex-from-remote.
   setting 'reindex.remote.whitelist', '127.0.0.1:*'
 }

plugins/discovery-ec2/qa/amazon-ec2/build.gradle

@@ -72,7 +72,7 @@ yamlRestTest.enabled = false
   }
 
   tasks.create(name: "yamlRestTest${action}", type: RestIntegTestTask) {
-    dependsOn fixture, project(':plugins:discovery-ec2').bundlePlugin
+    dependsOn fixture
   }
   SourceSetContainer sourceSets = project.getExtensions().getByType(SourceSetContainer.class);
   SourceSet yamlRestTestSourceSet = sourceSets.getByName(YamlRestTestPlugin.SOURCE_SET_NAME)
@@ -84,7 +84,7 @@ yamlRestTest.enabled = false
 
   testClusters."yamlRestTest${action}" {
     numberOfNodes = ec2NumberOfNodes
-    plugin project(':plugins:discovery-ec2').bundlePlugin.archiveFile
+    plugin ':plugins:discovery-ec2'
 
     setting 'discovery.seed_providers', 'ec2'
     setting 'network.host', '_ec2_'

plugins/discovery-gce/qa/gce/build.gradle

@@ -56,12 +56,12 @@ tasks.named("processYamlRestTestResources").configure {
 }
 
 yamlRestTest {
-  dependsOn gceFixture, project(':plugins:discovery-gce').bundlePlugin
+  dependsOn gceFixture
 }
 
 testClusters.yamlRestTest {
   numberOfNodes = gceNumberOfNodes
-  plugin project(':plugins:discovery-gce').bundlePlugin.archiveFile
+  plugin ':plugins:discovery-gce'
   // use gce fixture for Auth calls instead of http://metadata.google.internal
   environment 'GCE_METADATA_HOST', { "http://${gceFixture.addressAndPort}" }, IGNORE_VALUE
   // allows to configure hidden settings (`cloud.gce.host` and `cloud.gce.root_url`)

plugins/repository-gcs/build.gradle

@@ -294,7 +294,7 @@ testClusters {
  * an additional test that forces the large blob threshold to be small to exercise the resumable upload path.
  */
 task largeBlobYamlRestTest(type: RestIntegTestTask) {
-  dependsOn project(':plugins:repository-gcs').bundlePlugin
+  dependsOn bundlePlugin
   if (useFixture) {
     dependsOn createServiceAccountFile
   }
@@ -308,7 +308,7 @@ check.dependsOn largeBlobYamlRestTest
 
 testClusters {
   largeBlobYamlRestTest {
-    plugin project(':plugins:repository-gcs').bundlePlugin.archiveFile
+    plugin bundlePlugin.archiveFile
 
     // force large blob uploads by setting the threshold small, forcing this code path to be tested
     systemProperty 'es.repository_gcs.large_blob_threshold_byte_size', '256'

qa/smoke-test-plugins/build.gradle

@@ -33,8 +33,7 @@ testClusters.integTest {
       //Do not attempt to install ingest-attachment in FIPS 140 as it is not supported (it depends on non-FIPS BouncyCastle
       return
     }
-    plugin pluginProject.tasks.bundlePlugin.archiveFile
-    tasks.integTest.dependsOn pluginProject.tasks.bundlePlugin
+    plugin pluginProject.path
     pluginsCount += 1
   }
 }

x-pack/plugin/searchable-snapshots/qa/azure/build.gradle

@@ -43,14 +43,13 @@ if (useFixture) {
 }
 
 integTest {
-  dependsOn repositoryPlugin.bundlePlugin
   systemProperty 'test.azure.container', azureContainer
   nonInputProperties.systemProperty 'test.azure.base_path', azureBasePath + "_searchable_snapshots_tests_" + BuildParams.testSeed
 }
 
 testClusters.integTest {
   testDistribution = 'DEFAULT'
-  plugin repositoryPlugin.bundlePlugin.archiveFile
+  plugin repositoryPlugin.path
 
   if (BuildParams.isSnapshotBuild() == false) {
     systemProperty 'es.searchable_snapshots_feature_enabled', 'true'

x-pack/plugin/searchable-snapshots/qa/gcs/build.gradle

@@ -87,14 +87,13 @@ if (useFixture) {
 }
 
 integTest {
-  dependsOn repositoryPlugin.bundlePlugin
   systemProperty 'test.gcs.bucket', gcsBucket
   nonInputProperties.systemProperty 'test.gcs.base_path', gcsBasePath + "_searchable_snapshots_tests" + BuildParams.testSeed
 }
 
 testClusters.integTest {
   testDistribution = 'DEFAULT'
-  plugin repositoryPlugin.bundlePlugin.archiveFile
+  plugin repositoryPlugin.path
 
   if (BuildParams.isSnapshotBuild() == false) {
     systemProperty 'es.searchable_snapshots_feature_enabled', 'true'

x-pack/plugin/searchable-snapshots/qa/minio/build.gradle

@@ -29,14 +29,13 @@ def fixtureAddress = {
 }
 
 integTest {
-  dependsOn repositoryPlugin.bundlePlugin
   systemProperty 'test.minio.bucket', 'bucket'
   systemProperty 'test.minio.base_path', 'searchable_snapshots_tests'
 }
 
 testClusters.integTest {
   testDistribution = 'DEFAULT'
-  plugin repositoryPlugin.bundlePlugin.archiveFile
+  plugin repositoryPlugin.path
 
   if (BuildParams.isSnapshotBuild() == false) {
     systemProperty 'es.searchable_snapshots_feature_enabled', 'true'

x-pack/plugin/searchable-snapshots/qa/s3/build.gradle

@@ -43,14 +43,13 @@ if (useFixture) {
 }
 
 integTest {
-  dependsOn repositoryPlugin.bundlePlugin
   systemProperty 'test.s3.bucket', s3Bucket
   nonInputProperties.systemProperty 'test.s3.base_path', s3BasePath ? s3BasePath + "_searchable_snapshots_tests" + BuildParams.testSeed : 'base_path'
 }
 
 testClusters.integTest {
   testDistribution = 'DEFAULT'
-  plugin repositoryPlugin.bundlePlugin.archiveFile
+  plugin repositoryPlugin.path
 
   if (BuildParams.isSnapshotBuild() == false) {
     systemProperty 'es.searchable_snapshots_feature_enabled', 'true'

x-pack/qa/smoke-test-plugins-ssl/build.gradle

@@ -67,8 +67,7 @@ testClusters.integTest {
   user username: "monitoring_agent", password: "x-pack-test-password", role: "remote_monitoring_agent"
 
   project(':plugins').getChildProjects().each { pluginName, pluginProject ->
-    plugin pluginProject.tasks.bundlePlugin.archiveFile
-    tasks.integTest.dependsOn pluginProject.tasks.bundlePlugin
+    plugin pluginProject.path
     pluginsCount += 1
   }
 }

x-pack/qa/smoke-test-plugins/build.gradle

@@ -16,8 +16,7 @@ testClusters.integTest {
   setting 'xpack.license.self_generated.type', 'trial'
   user username: "test_user", password: "x-pack-test-password"
   project(':plugins').getChildProjects().each { pluginName, pluginProject ->
-    plugin pluginProject.tasks.bundlePlugin.archiveFile
-    tasks.integTest.dependsOn pluginProject.tasks.bundlePlugin
+    plugin pluginProject.path
     pluginsCount += 1
   }
 }