Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow extended plugins to be optional #16909

Merged
merged 18 commits into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
33 changes: 31 additions & 2 deletions server/src/main/java/org/opensearch/plugins/PluginInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@
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;

/**
Expand Down Expand Up @@ -149,7 +151,11 @@
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;
}

Expand Down Expand Up @@ -209,6 +215,16 @@
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<>();

Check warning on line 221 in server/src/main/java/org/opensearch/plugins/PluginInfo.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/plugins/PluginInfo.java#L221

Added line #L221 was not covered by tests
}
}

static boolean isOptionalExtension(String extendedPlugin) {
String[] dependency = extendedPlugin.split(";");
return dependency.length > 1 && "optional=true".equals(dependency[1]);
}

@Override
Expand All @@ -234,6 +250,9 @@
}
out.writeStringCollection(extendedPlugins);
out.writeBoolean(hasNativeController);
if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
out.writeStringCollection(optionalExtendedPlugins);
}
}

/**
Expand Down Expand Up @@ -417,8 +436,17 @@
*
* @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());
reta marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -493,6 +521,7 @@
builder.field("custom_foldername", customFolderName);
builder.field("extended_plugins", extendedPlugins);
builder.field("has_native_controller", hasNativeController);
builder.field("optional_extended_plugins", optionalExtendedPlugins);
}
builder.endObject();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,13 @@
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);
Expand Down Expand Up @@ -653,6 +659,9 @@
Set<URL> urls = new HashSet<>();
for (String extendedPlugin : exts) {
Set<URL> pluginUrls = transitiveUrls.get(extendedPlugin);
if (pluginUrls == null && bundle.plugin.isExtendedPluginOptional(extendedPlugin)) {
continue;

Check warning on line 663 in server/src/main/java/org/opensearch/plugins/PluginsService.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L663 was not covered by tests
}
assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin;

Set<URL> intersection = new HashSet<>(urls);
Expand Down Expand Up @@ -704,6 +713,10 @@
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;

Check warning on line 718 in server/src/main/java/org/opensearch/plugins/PluginsService.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L718 was not covered by tests
}
assert extendedPlugin != null;
if (ExtensiblePlugin.class.isInstance(extendedPlugin) == false) {
throw new IllegalStateException("Plugin [" + name + "] cannot extend non-extensible plugin [" + extendedPluginName + "]");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -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(
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
Loading