From 44154b5e4e624e88add11880415977ba17ed3680 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 14 Oct 2024 23:50:23 -0400 Subject: [PATCH 01/14] Make extended plugins optional Signed-off-by: Craig Perkins --- .../gradle/plugin/PluginPropertiesExtension.java | 13 ++++++++++++- .../java/org/opensearch/plugins/PluginsService.java | 8 ++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java b/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java index d6117923973fa..189b0e773d0a2 100644 --- a/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java +++ b/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java @@ -53,9 +53,12 @@ public class PluginPropertiesExtension { private String customFolderName = ""; - /** Other plugins this plugin extends through SPI */ + /** Other plugins this plugin extends through SPI, these plugins are required to install this plugin */ private List extendedPlugins = new ArrayList<>(); + /** Other plugins this plugin extends through SPI, these plugins are not required for install */ + private List optionalExtendedPlugins = new ArrayList<>(); + private boolean hasNativeController; /** True if the plugin requires the opensearch keystore to exist, false otherwise. */ @@ -122,6 +125,10 @@ public List getExtendedPlugins() { return this.extendedPlugins; } + public List getOptionalExtendedPlugins() { + return this.optionalExtendedPlugins; + } + public boolean isHasNativeController() { return hasNativeController; } @@ -164,6 +171,10 @@ public void setExtendedPlugins(List extendedPlugins) { this.extendedPlugins = extendedPlugins; } + public void setOptionalExtendedPlugins(List optionalExtendedPlugins) { + this.optionalExtendedPlugins = optionalExtendedPlugins; + } + public boolean isHasClientJar() { return hasClientJar; } diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index f08c9c738f1b4..760c5352a1820 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -524,7 +524,8 @@ 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 + "]"); + continue; + // throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]"); } addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); assert sortedBundles.contains(depBundle); @@ -653,7 +654,9 @@ static void checkBundleJarHell(Set classpath, Bundle bundle, Map urls = new HashSet<>(); for (String extendedPlugin : exts) { Set pluginUrls = transitiveUrls.get(extendedPlugin); - assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin; + if (pluginUrls == null) { + continue; + } Set intersection = new HashSet<>(urls); intersection.retainAll(pluginUrls); @@ -690,6 +693,7 @@ static void checkBundleJarHell(Set classpath, Bundle bundle, Map Date: Mon, 14 Oct 2024 23:57:21 -0400 Subject: [PATCH 02/14] Make extended plugins optional Signed-off-by: Craig Perkins --- .../gradle/plugin/PluginPropertiesExtension.java | 13 +------------ .../java/org/opensearch/plugins/PluginsService.java | 1 - 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java b/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java index 189b0e773d0a2..d6117923973fa 100644 --- a/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java +++ b/buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java @@ -53,12 +53,9 @@ public class PluginPropertiesExtension { private String customFolderName = ""; - /** Other plugins this plugin extends through SPI, these plugins are required to install this plugin */ + /** Other plugins this plugin extends through SPI */ private List extendedPlugins = new ArrayList<>(); - /** Other plugins this plugin extends through SPI, these plugins are not required for install */ - private List optionalExtendedPlugins = new ArrayList<>(); - private boolean hasNativeController; /** True if the plugin requires the opensearch keystore to exist, false otherwise. */ @@ -125,10 +122,6 @@ public List getExtendedPlugins() { return this.extendedPlugins; } - public List getOptionalExtendedPlugins() { - return this.optionalExtendedPlugins; - } - public boolean isHasNativeController() { return hasNativeController; } @@ -171,10 +164,6 @@ public void setExtendedPlugins(List extendedPlugins) { this.extendedPlugins = extendedPlugins; } - public void setOptionalExtendedPlugins(List optionalExtendedPlugins) { - this.optionalExtendedPlugins = optionalExtendedPlugins; - } - public boolean isHasClientJar() { return hasClientJar; } diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 760c5352a1820..f96bae2f054a7 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -693,7 +693,6 @@ static void checkBundleJarHell(Set classpath, Bundle bundle, Map Date: Fri, 13 Dec 2024 19:39:06 -0500 Subject: [PATCH 03/14] Load extensions for classpath plugins Signed-off-by: Craig Perkins --- .../src/main/java/org/opensearch/plugins/PluginsService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index f96bae2f054a7..a6298b8b4da6f 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -150,7 +150,7 @@ public PluginsService( "1.8", pluginClass.getName(), null, - Collections.emptyList(), + classpathPlugins.stream().map(Class::getName).filter(cp -> !pluginClass.getName().equals(cp)).collect(Collectors.toList()), false ); if (logger.isTraceEnabled()) { @@ -159,6 +159,7 @@ public PluginsService( pluginsLoaded.add(new Tuple<>(pluginInfo, plugin)); pluginsList.add(pluginInfo); pluginsNames.add(pluginInfo.getName()); + loadExtensions(pluginsLoaded); } Set seenBundles = new LinkedHashSet<>(); From 7b4d7d7ad27d1321b4aa49c6d03e14e1365cbc63 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 13 Dec 2024 20:29:36 -0500 Subject: [PATCH 04/14] Ensure only single instance for each classpath extension Signed-off-by: Craig Perkins --- .../java/org/opensearch/plugins/PluginsService.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index a6298b8b4da6f..c3db32874d260 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -159,8 +159,8 @@ public PluginsService( pluginsLoaded.add(new Tuple<>(pluginInfo, plugin)); pluginsList.add(pluginInfo); pluginsNames.add(pluginInfo.getName()); - loadExtensions(pluginsLoaded); } + loadExtensions(pluginsLoaded); Set seenBundles = new LinkedHashSet<>(); List modulesList = new ArrayList<>(); @@ -571,9 +571,17 @@ private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, L ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() { @Override public List loadExtensions(Class extensionPointType) { + Set> seenClasses = new LinkedHashSet<>(); List result = new ArrayList<>(); + for (Plugin extendingPlugin : extendingPlugins) { - result.addAll(createExtensions(extensionPointType, extendingPlugin)); + List extensions = createExtensions(extensionPointType, extendingPlugin); + for (T extension : extensions) { + // Only add if we haven't seen this class before, needed for classpath extensions for testing + if (seenClasses.add(extension.getClass())) { + result.add(extension); + } + } } return Collections.unmodifiableList(result); } From 16147068fbd6760b697b3c4c587519efc0925c16 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Sun, 22 Dec 2024 12:26:48 -0500 Subject: [PATCH 05/14] Add test for classpath plugin extended plugin loading Signed-off-by: Craig Perkins --- .../opensearch/plugins/ClasspathPluginIT.java | 52 +++++++++++++++++++ ....plugins.ClasspathPluginIT$SampleExtension | 8 +++ 2 files changed, 60 insertions(+) create mode 100644 server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java create mode 100644 server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension diff --git a/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java b/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java new file mode 100644 index 0000000000000..fffef3f55c5b3 --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java @@ -0,0 +1,52 @@ +/* + * 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.test.OpenSearchIntegTestCase; + +import java.io.IOException; +import java.util.Collection; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +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> nodePlugins() { + return Stream.concat(super.nodePlugins().stream(), Stream.of(SampleExtensiblePlugin.class, SampleExtendingPlugin.class)) + .collect(Collectors.toList()); + } + + public void testPluginExtensionWithClasspathPlugins() throws IOException { + internalCluster().startNode(); + } +} diff --git a/server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension b/server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension new file mode 100644 index 0000000000000..f369b9d0f63b7 --- /dev/null +++ b/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 From 13c7bee35aa90b421607cf9c34cba34796bbd557 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 23 Dec 2024 17:17:41 -0500 Subject: [PATCH 06/14] Modify test to allow optional extended plugin Signed-off-by: Craig Perkins --- .../opensearch/plugins/PluginsService.java | 2 +- .../plugins/PluginsServiceTests.java | 32 ++++++++++++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index c3db32874d260..a4b3bec0c1db8 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -525,8 +525,8 @@ private static void addSortedBundle( for (String dependency : bundle.plugin.getExtendedPlugins()) { Bundle depBundle = bundles.get(dependency); if (depBundle == null) { + logger.warn("Missing plugin [" + dependency + "], dependency of [" + name + "]"); continue; - // throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]"); } addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); assert sortedBundles.contains(depBundle); diff --git a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java index bd9ee33856f14..db73c41448fee 100644 --- a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java @@ -362,14 +362,30 @@ public void testSortBundlesNoDeps() throws Exception { } public void testSortBundlesMissingDep() 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); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> PluginsService.sortBundles(Collections.singleton(bundle)) - ); - assertEquals("Missing plugin [dne], dependency of [foo]", e.getMessage()); + 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"), + false + ); + PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir); + PluginsService.sortBundles(Collections.singleton(bundle)); + mockLogAppender.assertAllExpectationsMatched(); + } } public void testSortBundlesCommonDep() throws Exception { From 29ff5b0b5b843f43a7e8a64b444bd645b4e8231d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 23 Dec 2024 19:10:59 -0500 Subject: [PATCH 07/14] Only optional extended plugins Signed-off-by: Craig Perkins --- .../opensearch/plugins/ClasspathPluginIT.java | 52 ------------------- ....plugins.ClasspathPluginIT$SampleExtension | 8 --- .../opensearch/plugins/PluginsService.java | 13 +---- 3 files changed, 2 insertions(+), 71 deletions(-) delete mode 100644 server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java delete mode 100644 server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension diff --git a/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java b/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java deleted file mode 100644 index fffef3f55c5b3..0000000000000 --- a/server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * 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.test.OpenSearchIntegTestCase; - -import java.io.IOException; -import java.util.Collection; -import java.util.stream.Collectors; -import java.util.stream.Stream; - -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> nodePlugins() { - return Stream.concat(super.nodePlugins().stream(), Stream.of(SampleExtensiblePlugin.class, SampleExtendingPlugin.class)) - .collect(Collectors.toList()); - } - - public void testPluginExtensionWithClasspathPlugins() throws IOException { - internalCluster().startNode(); - } -} diff --git a/server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension b/server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension deleted file mode 100644 index f369b9d0f63b7..0000000000000 --- a/server/src/internalClusterTest/resources/META-INF/services/org.opensearch.plugins.ClasspathPluginIT$SampleExtension +++ /dev/null @@ -1,8 +0,0 @@ -# -# 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 diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index a4b3bec0c1db8..f96e34c27442c 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -150,7 +150,7 @@ public PluginsService( "1.8", pluginClass.getName(), null, - classpathPlugins.stream().map(Class::getName).filter(cp -> !pluginClass.getName().equals(cp)).collect(Collectors.toList()), + Collections.emptyList(), false ); if (logger.isTraceEnabled()) { @@ -160,7 +160,6 @@ public PluginsService( pluginsList.add(pluginInfo); pluginsNames.add(pluginInfo.getName()); } - loadExtensions(pluginsLoaded); Set seenBundles = new LinkedHashSet<>(); List modulesList = new ArrayList<>(); @@ -571,17 +570,9 @@ private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, L ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() { @Override public List loadExtensions(Class extensionPointType) { - Set> seenClasses = new LinkedHashSet<>(); List result = new ArrayList<>(); - for (Plugin extendingPlugin : extendingPlugins) { - List extensions = createExtensions(extensionPointType, extendingPlugin); - for (T extension : extensions) { - // Only add if we haven't seen this class before, needed for classpath extensions for testing - if (seenClasses.add(extension.getClass())) { - result.add(extension); - } - } + result.addAll(createExtensions(extensionPointType, extendingPlugin)); } return Collections.unmodifiableList(result); } From 3ebe10ec4e8c691bafd63d00f332103f046e0739 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 31 Dec 2024 15:35:30 -0500 Subject: [PATCH 08/14] Add additional warning message Signed-off-by: Craig Perkins --- server/src/main/java/org/opensearch/plugins/PluginsService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index f96e34c27442c..3a439b970980d 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -525,6 +525,7 @@ private static void addSortedBundle( Bundle depBundle = bundles.get(dependency); if (depBundle == null) { 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; } addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); From 28d80e76d9ba27f5b1b9a53622dea894fe085bbf Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 2 Jan 2025 20:13:59 -0500 Subject: [PATCH 09/14] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45bc56b505eb3..c7f189fad38ad 100644 --- 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)) +- Make extended plugins 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)) From 7e75335524724f7776ae96395fc021b1a607e85b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 3 Jan 2025 10:27:35 -0500 Subject: [PATCH 10/14] Add tag to make extended plugin optional Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 +- .../opensearch/plugins/PluginsService.java | 21 +++++++++++++----- .../plugins/PluginsServiceTests.java | 22 +++++++++++++++++-- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7f189fad38ad..5f813fecf66cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +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)) -- Make extended plugins optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909)) +- 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/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 3a439b970980d..96de0adede066 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -497,6 +497,11 @@ static List sortBundles(Set bundles) { return new ArrayList<>(sortedBundles); } + static boolean isExtendedPluginOptional(String extendedPlugin) { + String[] dependency = extendedPlugin.split(";"); + return dependency.length > 1 && "optional=true".equals(dependency[1]); + } + // add the given bundle to the sorted bundles, first adding dependencies private static void addSortedBundle( Bundle bundle, @@ -522,11 +527,16 @@ private static void addSortedBundle( dependencyStack.add(name); for (String dependency : bundle.plugin.getExtendedPlugins()) { - Bundle depBundle = bundles.get(dependency); + String dependencyName = dependency.split(";")[0]; + Bundle depBundle = bundles.get(dependencyName); if (depBundle == null) { - 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; + if (isExtendedPluginOptional(dependency)) { + logger.warn("Missing plugin [" + dependencyName + "], 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 [" + dependencyName + "], dependency of [" + name + "]"); + } } addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); assert sortedBundles.contains(depBundle); @@ -655,9 +665,10 @@ static void checkBundleJarHell(Set classpath, Bundle bundle, Map urls = new HashSet<>(); for (String extendedPlugin : exts) { Set pluginUrls = transitiveUrls.get(extendedPlugin); - if (pluginUrls == null) { + if (pluginUrls == null && isExtendedPluginOptional(extendedPlugin)) { continue; } + assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin; Set intersection = new HashSet<>(urls); intersection.retainAll(pluginUrls); diff --git a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java index db73c41448fee..47e120fe9968c 100644 --- a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java @@ -75,6 +75,7 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.sameInstance; @@ -361,7 +362,24 @@ public void testSortBundlesNoDeps() throws Exception { assertThat(sortedBundles, Matchers.contains(bundle1, bundle2, bundle3)); } - public void testSortBundlesMissingDep() throws Exception { + public void testIsOptionalExtendedPlugin() { + assertThat(PluginsService.isExtendedPluginOptional("plugin-dep"), is(false)); + assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=true"), is(true)); + assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=false"), is(false)); + } + + 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); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> PluginsService.sortBundles(Collections.singleton(bundle)) + ); + 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( @@ -379,7 +397,7 @@ public void testSortBundlesMissingDep() throws Exception { Version.CURRENT, "1.8", "MyPlugin", - Collections.singletonList("dne"), + Collections.singletonList("dne;optional=true"), false ); PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir); From 0032cc31381a22c32395b08c5c501fb8dd9a4149 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 3 Jan 2025 14:45:04 -0500 Subject: [PATCH 11/14] Only send plugin names when serializing PluginInfo Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/plugins/PluginInfo.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginInfo.java b/server/src/main/java/org/opensearch/plugins/PluginInfo.java index b6030f4ded5e5..73b960a86eba8 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginInfo.java +++ b/server/src/main/java/org/opensearch/plugins/PluginInfo.java @@ -232,7 +232,7 @@ This works for currently supported range notations (=,~) } else { out.writeString(name); } - out.writeStringCollection(extendedPlugins); + out.writeStringCollection(getExtendedPluginNames()); out.writeBoolean(hasNativeController); } @@ -417,6 +417,15 @@ public String getFolderName() { * * @return the names of the plugins extended */ + public List getExtendedPluginNames() { + return extendedPlugins.stream().map(s -> s.split(";")[0]).collect(Collectors.toUnmodifiableList()); + } + + /** + * Other plugins this plugin extends through SPI including information about optionality. + * + * @return the names of the plugins extended including optionality. i.e. opensearch-job-scheduler;optional=true + */ public List getExtendedPlugins() { return extendedPlugins; } From 4a6d065f24f06f73140fecb6650fd8000affe978 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 3 Jan 2025 15:19:56 -0500 Subject: [PATCH 12/14] Keep track of optional extended plugins in separate set Signed-off-by: Craig Perkins --- .../org/opensearch/plugins/PluginInfo.java | 28 ++++++++++++++----- .../opensearch/plugins/PluginsService.java | 16 ++++------- .../opensearch/plugins/PluginInfoTests.java | 27 ++++++++++++++++++ .../plugins/PluginsServiceTests.java | 7 ----- 4 files changed, 53 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginInfo.java b/server/src/main/java/org/opensearch/plugins/PluginInfo.java index 73b960a86eba8..559bcb4f512ce 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginInfo.java +++ b/server/src/main/java/org/opensearch/plugins/PluginInfo.java @@ -56,10 +56,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Properties; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -86,6 +88,8 @@ public class PluginInfo implements Writeable, ToXContentObject { private final String classname; private final String customFolderName; private final List extendedPlugins; + // Optional extended plugins are a subset of extendedPlugins that only contains the optional extended plugins + private final Set optionalExtendedPlugins; private final boolean hasNativeController; /** @@ -149,7 +153,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.toUnmodifiableSet()); this.hasNativeController = hasNativeController; } @@ -209,6 +217,12 @@ public PluginInfo(final StreamInput in) throws IOException { this.customFolderName = in.readString(); this.extendedPlugins = in.readStringList(); this.hasNativeController = in.readBoolean(); + this.optionalExtendedPlugins = new HashSet<>(); + } + + static boolean isOptionalExtension(String extendedPlugin) { + String[] dependency = extendedPlugin.split(";"); + return dependency.length > 1 && "optional=true".equals(dependency[1]); } @Override @@ -232,7 +246,7 @@ This works for currently supported range notations (=,~) } else { out.writeString(name); } - out.writeStringCollection(getExtendedPluginNames()); + out.writeStringCollection(extendedPlugins); out.writeBoolean(hasNativeController); } @@ -417,17 +431,17 @@ public String getFolderName() { * * @return the names of the plugins extended */ - public List getExtendedPluginNames() { - return extendedPlugins.stream().map(s -> s.split(";")[0]).collect(Collectors.toUnmodifiableList()); + public boolean isExtendedPluginOptional(String extendedPlugin) { + return optionalExtendedPlugins.contains(extendedPlugin); } /** - * Other plugins this plugin extends through SPI including information about optionality. + * Other plugins this plugin extends through SPI * - * @return the names of the plugins extended including optionality. i.e. opensearch-job-scheduler;optional=true + * @return the names of the plugins extended */ public List getExtendedPlugins() { - return extendedPlugins; + return extendedPlugins.stream().map(s -> s.split(";")[0]).collect(Collectors.toUnmodifiableList()); } /** diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 96de0adede066..b35901052d218 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -497,11 +497,6 @@ static List sortBundles(Set bundles) { return new ArrayList<>(sortedBundles); } - static boolean isExtendedPluginOptional(String extendedPlugin) { - String[] dependency = extendedPlugin.split(";"); - return dependency.length > 1 && "optional=true".equals(dependency[1]); - } - // add the given bundle to the sorted bundles, first adding dependencies private static void addSortedBundle( Bundle bundle, @@ -527,15 +522,14 @@ private static void addSortedBundle( dependencyStack.add(name); for (String dependency : bundle.plugin.getExtendedPlugins()) { - String dependencyName = dependency.split(";")[0]; - Bundle depBundle = bundles.get(dependencyName); + Bundle depBundle = bundles.get(dependency); if (depBundle == null) { - if (isExtendedPluginOptional(dependency)) { - logger.warn("Missing plugin [" + dependencyName + "], 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 [" + dependencyName + "], dependency of [" + name + "]"); + throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]"); } } addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); @@ -665,7 +659,7 @@ static void checkBundleJarHell(Set classpath, Bundle bundle, Map urls = new HashSet<>(); for (String extendedPlugin : exts) { Set pluginUrls = transitiveUrls.get(extendedPlugin); - if (pluginUrls == null && isExtendedPluginOptional(extendedPlugin)) { + if (pluginUrls == null && bundle.plugin.isExtendedPluginOptional(extendedPlugin)) { continue; } assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin; diff --git a/server/src/test/java/org/opensearch/plugins/PluginInfoTests.java b/server/src/test/java/org/opensearch/plugins/PluginInfoTests.java index 12c7dc870c104..76294d85c64d4 100644 --- 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 index 47e120fe9968c..f5702fa1a7ade 100644 --- a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java @@ -75,7 +75,6 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.sameInstance; @@ -362,12 +361,6 @@ public void testSortBundlesNoDeps() throws Exception { assertThat(sortedBundles, Matchers.contains(bundle1, bundle2, bundle3)); } - public void testIsOptionalExtendedPlugin() { - assertThat(PluginsService.isExtendedPluginOptional("plugin-dep"), is(false)); - assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=true"), is(true)); - assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=false"), is(false)); - } - 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); From 754bc6ffa5db46935888dd56ac6b0c00de7ac7b2 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 3 Jan 2025 16:02:13 -0500 Subject: [PATCH 13/14] Include in ser/de of PluginInfo Signed-off-by: Craig Perkins --- .../java/org/opensearch/plugins/PluginInfo.java | 16 +++++++++++----- .../org/opensearch/plugins/PluginsService.java | 4 ++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginInfo.java b/server/src/main/java/org/opensearch/plugins/PluginInfo.java index 559bcb4f512ce..4ff699e8017ba 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginInfo.java +++ b/server/src/main/java/org/opensearch/plugins/PluginInfo.java @@ -56,12 +56,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Properties; -import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -89,7 +87,7 @@ public class PluginInfo implements Writeable, ToXContentObject { private final String customFolderName; private final List extendedPlugins; // Optional extended plugins are a subset of extendedPlugins that only contains the optional extended plugins - private final Set optionalExtendedPlugins; + private final List optionalExtendedPlugins; private final boolean hasNativeController; /** @@ -157,7 +155,7 @@ public PluginInfo( this.optionalExtendedPlugins = extendedPlugins.stream() .filter(PluginInfo::isOptionalExtension) .map(s -> s.split(";")[0]) - .collect(Collectors.toUnmodifiableSet()); + .collect(Collectors.toUnmodifiableList()); this.hasNativeController = hasNativeController; } @@ -217,7 +215,11 @@ public PluginInfo(final StreamInput in) throws IOException { this.customFolderName = in.readString(); this.extendedPlugins = in.readStringList(); this.hasNativeController = in.readBoolean(); - this.optionalExtendedPlugins = new HashSet<>(); + if (in.getVersion().onOrAfter(Version.V_2_19_0)) { + this.optionalExtendedPlugins = in.readStringList(); + } else { + this.optionalExtendedPlugins = new ArrayList<>(); + } } static boolean isOptionalExtension(String extendedPlugin) { @@ -248,6 +250,9 @@ This works for currently supported range notations (=,~) } out.writeStringCollection(extendedPlugins); out.writeBoolean(hasNativeController); + if (out.getVersion().onOrAfter(Version.V_2_19_0)) { + out.writeStringCollection(optionalExtendedPlugins); + } } /** @@ -516,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 index b35901052d218..9bc1f1334122e 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -713,6 +713,10 @@ private Plugin loadBundle(Bundle bundle, Map loaded) { List 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 + "]"); From ef6bbfba3065192a2385296fb0d059c53debc1f4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 3 Jan 2025 16:46:29 -0500 Subject: [PATCH 14/14] Change to 3_0_0 Signed-off-by: Craig Perkins --- server/src/main/java/org/opensearch/plugins/PluginInfo.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginInfo.java b/server/src/main/java/org/opensearch/plugins/PluginInfo.java index 4ff699e8017ba..7173a653ebc9a 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginInfo.java +++ b/server/src/main/java/org/opensearch/plugins/PluginInfo.java @@ -215,7 +215,7 @@ 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_2_19_0)) { + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { this.optionalExtendedPlugins = in.readStringList(); } else { this.optionalExtendedPlugins = new ArrayList<>(); @@ -250,7 +250,7 @@ This works for currently supported range notations (=,~) } out.writeStringCollection(extendedPlugins); out.writeBoolean(hasNativeController); - if (out.getVersion().onOrAfter(Version.V_2_19_0)) { + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { out.writeStringCollection(optionalExtendedPlugins); } }