Allow extended plugins to be optional (opensearch-project#16909)

* Make extended plugins optional Signed-off-by: Craig Perkins <cwperx@amazon.com> * Make extended plugins optional Signed-off-by: Craig Perkins <cwperx@amazon.com> * Load extensions for classpath plugins Signed-off-by: Craig Perkins <cwperx@amazon.com> * Ensure only single instance for each classpath extension Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add test for classpath plugin extended plugin loading Signed-off-by: Craig Perkins <cwperx@amazon.com> * Modify test to allow optional extended plugin Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only optional extended plugins Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add additional warning message Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add to CHANGELOG Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add tag to make extended plugin optional Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only send plugin names when serializing PluginInfo Signed-off-by: Craig Perkins <cwperx@amazon.com> * Keep track of optional extended plugins in separate set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Include in ser/de of PluginInfo Signed-off-by: Craig Perkins <cwperx@amazon.com> * Change to 3_0_0 Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com>
meet-v25 · Jan 16, 2025 · 29429f8 · 29429f8
1 parent 99b273f
commit 29429f8
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -59,6 +59,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Changed
 - Indexed IP field supports `terms_query` with more than 1025 IP masks [#16391](https://github.com/opensearch-project/OpenSearch/pull/16391)
 - Make entries for dependencies from server/build.gradle to gradle version catalog ([#16707](https://github.com/opensearch-project/OpenSearch/pull/16707))
+- Allow extended plugins to be optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909))
 
 ### Deprecated
 - Performing update operation with default pipeline or final pipeline is deprecated ([#16712](https://github.com/opensearch-project/OpenSearch/pull/16712))

diff --git a/server/src/main/java/org/opensearch/plugins/PluginInfo.java b/server/src/main/java/org/opensearch/plugins/PluginInfo.java
@@ -86,6 +86,8 @@ public class PluginInfo implements Writeable, ToXContentObject {
     private final String classname;
     private final String customFolderName;
     private final List<String> extendedPlugins;
+    // Optional extended plugins are a subset of extendedPlugins that only contains the optional extended plugins
+    private final List<String> optionalExtendedPlugins;
     private final boolean hasNativeController;
 
     /**
@@ -149,7 +151,11 @@ public PluginInfo(
         this.javaVersion = javaVersion;
         this.classname = classname;
         this.customFolderName = customFolderName;
-        this.extendedPlugins = Collections.unmodifiableList(extendedPlugins);
+        this.extendedPlugins = extendedPlugins.stream().map(s -> s.split(";")[0]).collect(Collectors.toUnmodifiableList());
+        this.optionalExtendedPlugins = extendedPlugins.stream()
+            .filter(PluginInfo::isOptionalExtension)
+            .map(s -> s.split(";")[0])
+            .collect(Collectors.toUnmodifiableList());
         this.hasNativeController = hasNativeController;
     }
 
@@ -209,6 +215,16 @@ public PluginInfo(final StreamInput in) throws IOException {
         this.customFolderName = in.readString();
         this.extendedPlugins = in.readStringList();
         this.hasNativeController = in.readBoolean();
+        if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
+            this.optionalExtendedPlugins = in.readStringList();
+        } else {
+            this.optionalExtendedPlugins = new ArrayList<>();
+        }
+    }
+
+    static boolean isOptionalExtension(String extendedPlugin) {
+        String[] dependency = extendedPlugin.split(";");
+        return dependency.length > 1 && "optional=true".equals(dependency[1]);
     }
 
     @Override
@@ -234,6 +250,9 @@ This works for currently supported range notations (=,~)
         }
         out.writeStringCollection(extendedPlugins);
         out.writeBoolean(hasNativeController);
+        if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
+            out.writeStringCollection(optionalExtendedPlugins);
+        }
     }
 
     /**
@@ -417,8 +436,17 @@ public String getFolderName() {
      *
      * @return the names of the plugins extended
      */
+    public boolean isExtendedPluginOptional(String extendedPlugin) {
+        return optionalExtendedPlugins.contains(extendedPlugin);
+    }
+
+    /**
+     * Other plugins this plugin extends through SPI
+     *
+     * @return the names of the plugins extended
+     */
     public List<String> getExtendedPlugins() {
-        return extendedPlugins;
+        return extendedPlugins.stream().map(s -> s.split(";")[0]).collect(Collectors.toUnmodifiableList());
     }
 
     /**
@@ -493,6 +521,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
             builder.field("custom_foldername", customFolderName);
             builder.field("extended_plugins", extendedPlugins);
             builder.field("has_native_controller", hasNativeController);
+            builder.field("optional_extended_plugins", optionalExtendedPlugins);
         }
         builder.endObject();
 

diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java
@@ -524,7 +524,13 @@ private static void addSortedBundle(
         for (String dependency : bundle.plugin.getExtendedPlugins()) {
             Bundle depBundle = bundles.get(dependency);
             if (depBundle == null) {
-                throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]");
+                if (bundle.plugin.isExtendedPluginOptional(dependency)) {
+                    logger.warn("Missing plugin [" + dependency + "], dependency of [" + name + "]");
+                    logger.warn("Some features of this plugin may not function without the dependencies being installed.\n");
+                    continue;
+                } else {
+                    throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]");
+                }
             }
             addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack);
             assert sortedBundles.contains(depBundle);
@@ -653,6 +659,9 @@ static void checkBundleJarHell(Set<URL> classpath, Bundle bundle, Map<String, Se
             Set<URL> urls = new HashSet<>();
             for (String extendedPlugin : exts) {
                 Set<URL> pluginUrls = transitiveUrls.get(extendedPlugin);
+                if (pluginUrls == null && bundle.plugin.isExtendedPluginOptional(extendedPlugin)) {
+                    continue;
+                }
                 assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin;
 
                 Set<URL> intersection = new HashSet<>(urls);
@@ -704,6 +713,10 @@ private Plugin loadBundle(Bundle bundle, Map<String, Plugin> loaded) {
         List<ClassLoader> extendedLoaders = new ArrayList<>();
         for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) {
             Plugin extendedPlugin = loaded.get(extendedPluginName);
+            if (extendedPlugin == null && bundle.plugin.isExtendedPluginOptional(extendedPluginName)) {
+                // extended plugin is optional and is not installed
+                continue;
+            }
             assert extendedPlugin != null;
             if (ExtensiblePlugin.class.isInstance(extendedPlugin) == false) {
                 throw new IllegalStateException("Plugin [" + name + "] cannot extend non-extensible plugin [" + extendedPluginName + "]");

diff --git a/server/src/test/java/org/opensearch/plugins/PluginInfoTests.java b/server/src/test/java/org/opensearch/plugins/PluginInfoTests.java
@@ -44,6 +44,7 @@
 import org.opensearch.semver.SemverRange;
 import org.opensearch.test.OpenSearchTestCase;
 
+import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -55,6 +56,7 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 
 public class PluginInfoTests extends OpenSearchTestCase {
 
@@ -281,6 +283,30 @@ public void testReadFromPropertiesJvmMissingClassname() throws Exception {
         assertThat(e.getMessage(), containsString("property [classname] is missing"));
     }
 
+    public void testExtendedPluginsSingleOptionalExtension() throws IOException {
+        Path pluginDir = createTempDir().resolve("fake-plugin");
+        PluginTestUtil.writePluginProperties(
+            pluginDir,
+            "description",
+            "fake desc",
+            "name",
+            "my_plugin",
+            "version",
+            "1.0",
+            "opensearch.version",
+            Version.CURRENT.toString(),
+            "java.version",
+            System.getProperty("java.specification.version"),
+            "classname",
+            "FakePlugin",
+            "extended.plugins",
+            "foo;optional=true"
+        );
+        PluginInfo info = PluginInfo.readFromProperties(pluginDir);
+        assertThat(info.getExtendedPlugins(), contains("foo"));
+        assertThat(info.isExtendedPluginOptional("foo"), is(true));
+    }
+
     public void testExtendedPluginsSingleExtension() throws Exception {
         Path pluginDir = createTempDir().resolve("fake-plugin");
         PluginTestUtil.writePluginProperties(
@@ -302,6 +328,7 @@ public void testExtendedPluginsSingleExtension() throws Exception {
         );
         PluginInfo info = PluginInfo.readFromProperties(pluginDir);
         assertThat(info.getExtendedPlugins(), contains("foo"));
+        assertThat(info.isExtendedPluginOptional("foo"), is(false));
     }
 
     public void testExtendedPluginsMultipleExtensions() throws Exception {

diff --git a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java
@@ -361,7 +361,7 @@ public void testSortBundlesNoDeps() throws Exception {
         assertThat(sortedBundles, Matchers.contains(bundle1, bundle2, bundle3));
     }
 
-    public void testSortBundlesMissingDep() throws Exception {
+    public void testSortBundlesMissingRequiredDep() throws Exception {
         Path pluginDir = createTempDir();
         PluginInfo info = new PluginInfo("foo", "desc", "1.0", Version.CURRENT, "1.8", "MyPlugin", Collections.singletonList("dne"), false);
         PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir);
@@ -372,6 +372,33 @@ public void testSortBundlesMissingDep() throws Exception {
         assertEquals("Missing plugin [dne], dependency of [foo]", e.getMessage());
     }
 
+    public void testSortBundlesMissingOptionalDep() throws Exception {
+        try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(LogManager.getLogger(PluginsService.class))) {
+            mockLogAppender.addExpectation(
+                new MockLogAppender.SeenEventExpectation(
+                    "[.test] warning",
+                    "org.opensearch.plugins.PluginsService",
+                    Level.WARN,
+                    "Missing plugin [dne], dependency of [foo]"
+                )
+            );
+            Path pluginDir = createTempDir();
+            PluginInfo info = new PluginInfo(
+                "foo",
+                "desc",
+                "1.0",
+                Version.CURRENT,
+                "1.8",
+                "MyPlugin",
+                Collections.singletonList("dne;optional=true"),
+                false
+            );
+            PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir);
+            PluginsService.sortBundles(Collections.singleton(bundle));
+            mockLogAppender.assertAllExpectationsMatched();
+        }
+    }
+
     public void testSortBundlesCommonDep() throws Exception {
         Path pluginDir = createTempDir();
         Set<PluginsService.Bundle> bundles = new LinkedHashSet<>(); // control iteration order