Enable testing for ExtensiblePlugins using classpath plugins

CHANGELOG.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Enabled Async Shard Batch Fetch by default ([#18139](https://github.com/opensearch-project/OpenSearch/pull/18139))
 - Allow to get the search request from the QueryCoordinatorContext ([#17818](https://github.com/opensearch-project/OpenSearch/pull/17818))
 - Improve sort-query performance by retaining the default `totalHitsThreshold` for approximated `match_all` queries ([#18189](https://github.com/opensearch-project/OpenSearch/pull/18189))
+- Enable testing for ExtensiblePlugins using classpath plugins ([#16908](https://github.com/opensearch-project/OpenSearch/pull/16908))
 
 ### Changed
 

server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java

@@ -0,0 +1,75 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.plugins;
+
+import org.opensearch.Version;
+import org.opensearch.test.OpenSearchIntegTestCase;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
+import static org.hamcrest.Matchers.equalTo;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)
+public class ClasspathPluginIT extends OpenSearchIntegTestCase {
+
+    public interface SampleExtension {}
+
+    public static class SampleExtensiblePlugin extends Plugin implements ExtensiblePlugin {
+        public SampleExtensiblePlugin() {}
+
+        @Override
+        public void loadExtensions(ExtensiblePlugin.ExtensionLoader loader) {
+            int nLoaded = 0;
+            for (SampleExtension e : loader.loadExtensions(SampleExtension.class)) {
+                nLoaded++;
+            }
+
+            assertThat(nLoaded, equalTo(1));
+        }
+    }
+
+    public static class SampleExtendingPlugin extends Plugin implements SampleExtension {
+        public SampleExtendingPlugin() {}
+    };
+
+    @Override
+    protected Collection<PluginInfo> additionalNodePlugins() {
+        return List.of(
+            new PluginInfo(
+                SampleExtensiblePlugin.class.getName(),
+                "classpath plugin",
+                "NA",
+                Version.CURRENT,
+                "1.8",
+                SampleExtensiblePlugin.class.getName(),
+                null,
+                Collections.emptyList(),
+                false
+            ),
+            new PluginInfo(
+                SampleExtendingPlugin.class.getName(),
+                "classpath plugin",
+                "NA",
+                Version.CURRENT,
+                "1.8",
+                SampleExtendingPlugin.class.getName(),
+                null,
+                List.of(SampleExtensiblePlugin.class.getName()),
+                false
+            )
+        );
+    }
+
+    public void testPluginExtensionWithClasspathPlugins() throws IOException {
+        internalCluster().startNode();
+    }
+}

server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension

@@ -0,0 +1,8 @@
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+# The OpenSearch Contributors require contributions made to
+# this file be licensed under the Apache-2.0 license or a
+# compatible open source license.
+
+org.opensearch.plugins.ClasspathPluginIT$SampleExtendingPlugin

server/src/main/java/org/opensearch/node/Node.java

@@ -216,6 +216,7 @@
 import org.opensearch.plugins.NetworkPlugin;
 import org.opensearch.plugins.PersistentTaskPlugin;
 import org.opensearch.plugins.Plugin;
+import org.opensearch.plugins.PluginInfo;
 import org.opensearch.plugins.PluginsService;
 import org.opensearch.plugins.RepositoryPlugin;
 import org.opensearch.plugins.ScriptPlugin;
@@ -469,11 +470,7 @@ public Node(Environment environment) {
      * @param forbidPrivateIndexSettings whether or not private index settings are forbidden when creating an index; this is used in the
      *                                   test framework for tests that rely on being able to set private settings
      */
-    protected Node(
-        final Environment initialEnvironment,
-        Collection<Class<? extends Plugin>> classpathPlugins,
-        boolean forbidPrivateIndexSettings
-    ) {
+    protected Node(final Environment initialEnvironment, Collection<PluginInfo> classpathPlugins, boolean forbidPrivateIndexSettings) {
         final List<Closeable> resourcesToClose = new ArrayList<>(); // register everything we need to release in the case of an error
         boolean success = false;
         try {

server/src/main/java/org/opensearch/plugins/PluginsService.java

@@ -127,10 +127,48 @@
      */
     public PluginsService(
         Settings settings,
-        Path configPath,
         Path modulesDirectory,
         Path pluginsDirectory,
         Collection<Class<? extends Plugin>> classpathPlugins
+    ) {
+        // Used for testing
+        this(
+            settings,
+            null,
+            modulesDirectory,
+            pluginsDirectory,
+            classpathPlugins.stream()
+                .map(
+                    p -> new PluginInfo(
+                        p.getName(),
+                        "classpath plugin",
+                        "NA",
+                        Version.CURRENT,
+                        "1.8",
+                        p.getName(),
+                        null,
+                        Collections.emptyList(),
+                        false
+                    )
+                )
+                .collect(Collectors.toList())
+        );
+    }
+
+    /**
+     * Constructs a new PluginService
+     * @param settings The settings of the system
+     * @param modulesDirectory The directory modules exist in, or null if modules should not be loaded from the filesystem
+     * @param pluginsDirectory The directory plugins exist in, or null if plugins should not be loaded from the filesystem
+     * @param classpathPlugins Plugins that exist in the classpath which should be loaded
+     */
+    @SuppressWarnings("unchecked")
+    public PluginsService(
+        Settings settings,
+        Path configPath,
+        Path modulesDirectory,
+        Path pluginsDirectory,
+        Collection<PluginInfo> classpathPlugins
     ) {
         this.settings = settings;
         this.configPath = configPath;
@@ -140,25 +178,19 @@
         // we need to build a List of plugins for checking mandatory plugins
         final List<String> pluginsNames = new ArrayList<>();
         // first we load plugins that are on the classpath. this is for tests
-        for (Class<? extends Plugin> pluginClass : classpathPlugins) {
-            Plugin plugin = loadPlugin(pluginClass, settings, configPath);
-            PluginInfo pluginInfo = new PluginInfo(
-                pluginClass.getName(),
-                "classpath plugin",
-                "NA",
-                Version.CURRENT,
-                "1.8",
-                pluginClass.getName(),
-                null,
-                Collections.emptyList(),
-                false
-            );
-            if (logger.isTraceEnabled()) {
-                logger.trace("plugin loaded from classpath [{}]", pluginInfo);
+        for (PluginInfo pluginInfo : classpathPlugins) {
+            try {
+                Class<? extends Plugin> pluginClazz = (Class<? extends Plugin>) Class.forName(pluginInfo.getClassname());
+                Plugin plugin = loadPlugin(pluginClazz, settings, configPath);
+                if (logger.isTraceEnabled()) {
+                    logger.trace("plugin loaded from classpath [{}]", pluginInfo);
+                }
+                pluginsLoaded.add(new Tuple<>(pluginInfo, plugin));
+                pluginsList.add(pluginInfo);
+                pluginsNames.add(pluginInfo.getName());
+            } catch (ClassNotFoundException e) {
+                logger.error("Failed to load classpath plugin: " + pluginInfo.getClassname());
             }
-            pluginsLoaded.add(new Tuple<>(pluginInfo, plugin));
-            pluginsList.add(pluginInfo);
-            pluginsNames.add(pluginInfo.getName());
         }
 
         Set<Bundle> seenBundles = new LinkedHashSet<>();
@@ -196,6 +228,7 @@
 
         List<Tuple<PluginInfo, Plugin>> loaded = loadBundles(seenBundles);
         pluginsLoaded.addAll(loaded);
+        loadExtensions(pluginsLoaded);
 
         this.info = new PluginsAndModules(pluginsList, modulesList);
         this.plugins = Collections.unmodifiableList(pluginsLoaded);
@@ -583,7 +616,6 @@
             plugins.add(new Tuple<>(bundle.plugin, plugin));
         }
 
-        loadExtensions(plugins);
         return Collections.unmodifiableList(plugins);
     }
 

server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java

@@ -167,8 +167,7 @@ private Node startNode() throws NodeValidationException {
 
         Node node = new MockNode(
             settings,
-            Arrays.asList(MockNioTransportPlugin.class, MockHttpTransport.TestPlugin.class, InternalSettingsPlugin.class),
-            true
+            Arrays.asList(MockNioTransportPlugin.class, MockHttpTransport.TestPlugin.class, InternalSettingsPlugin.class)
         );
         node.start();
         return node;

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java

@@ -100,13 +100,7 @@ public Settings additionalSettings() {
     public static class FilterablePlugin extends Plugin implements ScriptPlugin {}
 
     static PluginsService newPluginsService(Settings settings, Class<? extends Plugin>... classpathPlugins) {
-        return new PluginsService(
-            settings,
-            null,
-            null,
-            TestEnvironment.newEnvironment(settings).pluginsDir(),
-            Arrays.asList(classpathPlugins)
-        );
+        return new PluginsService(settings, null, TestEnvironment.newEnvironment(settings).pluginsDir(), Arrays.asList(classpathPlugins));
     }
 
     public void testAdditionalSettings() {

test/framework/src/main/java/org/opensearch/node/MockNode.java

@@ -32,6 +32,7 @@
 
 package org.opensearch.node;
 
+import org.opensearch.Version;
 import org.opensearch.cluster.ClusterInfoService;
 import org.opensearch.cluster.MockInternalClusterInfoService;
 import org.opensearch.cluster.node.DiscoveryNode;
@@ -50,6 +51,7 @@
 import org.opensearch.http.HttpServerTransport;
 import org.opensearch.indices.IndicesService;
 import org.opensearch.plugins.Plugin;
+import org.opensearch.plugins.PluginInfo;
 import org.opensearch.script.MockScriptService;
 import org.opensearch.script.ScriptContext;
 import org.opensearch.script.ScriptEngine;
@@ -76,6 +78,7 @@
 import java.util.Set;
 import java.util.concurrent.Executor;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 /**
  * A node for testing which allows:
@@ -86,23 +89,20 @@
  */
 public class MockNode extends Node {
 
-    private final Collection<Class<? extends Plugin>> classpathPlugins;
+    private final Collection<PluginInfo> classpathPlugins;
 
-    public MockNode(final Settings settings, final Collection<Class<? extends Plugin>> classpathPlugins) {
-        this(settings, classpathPlugins, true);
-    }
-
-    public MockNode(
-        final Settings settings,
-        final Collection<Class<? extends Plugin>> classpathPlugins,
+    private MockNode(
+        final Environment environment,
+        final Collection<PluginInfo> classpathPlugins,
         final boolean forbidPrivateIndexSettings
     ) {
-        this(settings, classpathPlugins, null, forbidPrivateIndexSettings);
+        super(environment, classpathPlugins, forbidPrivateIndexSettings);
+        this.classpathPlugins = classpathPlugins;
     }
 
     public MockNode(
         final Settings settings,
-        final Collection<Class<? extends Plugin>> classpathPlugins,
+        final Collection<PluginInfo> classpathPlugins,
         final Path configPath,
         final boolean forbidPrivateIndexSettings
     ) {
@@ -113,19 +113,32 @@ public MockNode(
         );
     }
 
-    private MockNode(
-        final Environment environment,
-        final Collection<Class<? extends Plugin>> classpathPlugins,
-        final boolean forbidPrivateIndexSettings
-    ) {
-        super(environment, classpathPlugins, forbidPrivateIndexSettings);
-        this.classpathPlugins = classpathPlugins;
+    public MockNode(final Settings settings, final Collection<Class<? extends Plugin>> classpathPlugins) {
+        this(
+            InternalSettingsPreparer.prepareEnvironment(settings, Collections.emptyMap(), null, () -> "mock_ node"),
+            classpathPlugins.stream()
+                .map(
+                    p -> new PluginInfo(
+                        p.getName(),
+                        "classpath plugin",
+                        "NA",
+                        Version.CURRENT,
+                        "1.8",
+                        p.getName(),
+                        null,
+                        Collections.emptyList(),
+                        false
+                    )
+                )
+                .collect(Collectors.toList()),
+            true
+        );
     }
 
     /**
      * The classpath plugins this node was constructed with.
      */
-    public Collection<Class<? extends Plugin>> getClasspathPlugins() {
+    public Collection<PluginInfo> getClasspathPlugins() {
         return classpathPlugins;
     }
 

test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java

@@ -389,7 +389,7 @@ private static class ServiceHolder implements Closeable {
                 throw new AssertionError("node.name must be set");
             });
             PluginsService pluginsService;
-            pluginsService = new PluginsService(nodeSettings, null, env.modulesDir(), env.pluginsDir(), plugins);
+            pluginsService = new PluginsService(nodeSettings, env.modulesDir(), env.pluginsDir(), plugins);
 
             client = (Client) Proxy.newProxyInstance(Client.class.getClassLoader(), new Class[] { Client.class }, clientInvocationHandler);
             ScriptModule scriptModule = createScriptModule(pluginsService.filterPlugins(ScriptPlugin.class));