Plugins: Add plugin extension capabilities #27881

rjernst · 2017-12-19T00:36:03Z

This commit adds the infrastructure to plugin building and loading to
allow one plugin to extend another. That is, one plugin may extend
another by the "parent" plugin allowing itself to be extended through
java SPI. When all plugins extending a plugin are finished loading, the
"parent" plugin has a callback (through the ExtensiblePlugin interface)
allowing it to reload SPI.

This commit also adds an example plugin which uses as-yet implemented
extensibility (adding to the painless whitelist).

This commit adds the infrastructure to plugin building and loading to allow one plugin to extend another. That is, one plugin may extend another by the "parent" plugin allowing itself to be extended through java SPI. When all plugins extending a plugin are finished loading, the "parent" plugin has a callback (through the ExtensiblePlugin interface) allowing it to reload SPI. This commit also adds an example plugin which uses as-yet implemented extensibility (adding to the painless whitelist).

rjernst · 2017-12-19T00:37:20Z

Note this PR currently has a couple nocommits for testing and moving the createClassLoader permission out to a separate jar (we don't actually want to add this back to core). But I wanted to get this PR up so everything else can be checked.

s1monw

I left some commetns. LGTM in general

s1monw · 2017-12-19T08:20:26Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java

@@ -34,7 +35,7 @@
 /**
 * Registers Painless as a plugin.
 */
-public final class PainlessPlugin extends Plugin implements ScriptPlugin {
+public final class PainlessPlugin extends Plugin implements ScriptPlugin, ExtensiblePlugin {


shouldn't this implement reloadSPI ?

Painless doesn't have any SPI interfaces yet. This is just the scaffolding to add it (as an example of how SPI works in general for this PR).

s1monw · 2017-12-19T08:21:02Z

...s-whitelist/src/main/java/org/elasticsearch/example/painlesswhitelist/MyWhitelistPlugin.java

+
+import org.elasticsearch.plugins.Plugin;
+
+public class MyWhitelistPlugin extends Plugin {


hmm it's unclear to me what this extends can we have a real extension here?

We don't have any actual SPI to extend yet. This will be filled in when painless whitelisting is finished.

s1monw · 2017-12-19T08:25:38Z

core/src/main/java/org/elasticsearch/plugins/PluginsService.java

+    private static class ExtendedPluginsClassLoader extends ClassLoader {
+
+        /** Loaders of plugins extended by a plugin. */
+        private List<ClassLoader> extendedLoaders;


s1monw · 2017-12-19T08:26:32Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -82,6 +88,11 @@ public PluginInfo(final StreamInput in) throws IOException {
        this.description = in.readString();
        this.version = in.readString();
        this.classname = in.readString();
+        if (in.getVersion().onOrAfter(Version.V_6_1_0)) {


this should be 6.2 right?

Ooops, yes. Old branch!

This still needs to be updated to 6.2.

s1monw · 2017-12-19T08:26:41Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -100,6 +111,9 @@ public void writeTo(final StreamOutput out) throws IOException {
        out.writeString(description);
        out.writeString(version);
        out.writeString(classname);
+        if (out.getVersion().onOrAfter(Version.V_6_1_0)) {


this should be 6.2

s1monw · 2017-12-19T08:27:32Z

core/src/main/resources/org/elasticsearch/bootstrap/security.policy

@@ -131,4 +131,7 @@ grant {
  permission java.io.FilePermission "/sys/fs/cgroup/cpuacct/-", "read";
  permission java.io.FilePermission "/sys/fs/cgroup/memory", "read";
  permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read";
+
+  // nocommit: needed by plugin service for extensible plugins, move to a separate jar


do we want to add a elastic-commons jar to do that?

After discussing with @jasontedor this is my plan (although a jar on its own with the extension class loader, not in a general commons jar, since I want to reduce what code is allowed the extra permission to the bare minimum).

client

BootstrapForTesting to fill in libs codebases when testing

jasontedor

Looking good. I left a few comments, nothing major.

jasontedor · 2017-12-19T03:21:11Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -265,6 +291,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
            builder.field("version", version);
            builder.field("description", description);
            builder.field("classname", classname);
+            builder.array("extended_plugins", extendsPlugins);


Nit: For consistency, should all the extends plugins be extendedPlugins?

I went back and forth on the naming, but at this point I'm fine with that. I'll change them all.

jasontedor · 2017-12-19T03:32:17Z

core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java

@@ -47,6 +47,7 @@ public void testReadFromProperties() throws Exception {
        assertEquals("fake desc", info.getDescription());
        assertEquals("1.0", info.getVersion());
        assertEquals("FakePlugin", info.getClassname());
+        assertTrue(info.getExtendsPlugins().isEmpty());


I prefer:

diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index 2a96538e83..a188f3c765 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.stream.Collectors; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.empty; public class PluginInfoTests extends ESTestCase { @@ -47,7 +48,7 @@ public class PluginInfoTests extends ESTestCase { assertEquals("fake desc", info.getDescription()); assertEquals("1.0", info.getVersion()); assertEquals("FakePlugin", info.getClassname()); - assertTrue(info.getExtendsPlugins().isEmpty()); + assertThat(info.getExtendsPlugins(), empty()); } public void testReadFromPropertiesNameMissing() throws Exception {

because it gives better output if the assertion fails.

jasontedor · 2017-12-19T03:38:25Z

core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java

+            "classname", "FakePlugin",
+            "extends.plugins", "foo");
+        PluginInfo info = PluginInfo.readFromProperties(pluginDir);
+        assertThat(info.getExtendsPlugins(), contains("foo"));


I think we can assert stronger here, that it is equal to the singleton list "foo"?

contains does an exact match. From the javadocs:

For a positive match, the examined iterable must be of the same length as the number of specified items.

I forget this every time because it's so horribly named. Thanks.

jasontedor · 2017-12-19T03:38:51Z

core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java

+            "classname", "FakePlugin",
+            "extends.plugins", "foo,bar,baz");
+        PluginInfo info = PluginInfo.readFromProperties(pluginDir);
+        assertThat(info.getExtendsPlugins(), contains("foo", "bar", "baz"));


I think we can assert stronger here, that it is equal to in any order the list "foo", "bar", "baz" (there is a matcher for this)?

Same here. I think expecting the exact order is fine too, since getExtendedPlugins returns a List.

Yes, it's poorly named and it traps me every time.

Same as before, this is already an exact match.

jasontedor · 2017-12-19T03:39:04Z

core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java

+            "classname", "FakePlugin",
+            "extends.plugins", "");
+        PluginInfo info = PluginInfo.readFromProperties(pluginDir);
+        assertTrue(info.getExtendsPlugins().isEmpty());


Can we use the empty collections matchers here?

jasontedor · 2017-12-21T02:36:34Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -82,6 +88,11 @@ public PluginInfo(final StreamInput in) throws IOException {
        this.description = in.readString();
        this.version = in.readString();
        this.classname = in.readString();
+        if (in.getVersion().onOrAfter(Version.V_6_1_0)) {


This still needs to be updated to 6.2.

jasontedor · 2017-12-21T02:45:27Z

test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java

+                throw new IllegalStateException("Already added " + name + " codebase for testing");
+            }
+        } catch (ClassNotFoundException e) {
+            // no class, fall through to not add


When can this happen that it's okay to continue?

It would happen in any tests other than core, since we only add the plugin-classloader jar to the core tests runtime classpath. I'll add a comment.

rjernst · 2017-12-21T03:54:10Z

@jasontedor @s1monw I believe I have addressed all your comments and all the nocommits are now gone.

jasontedor

LGTM.

rjernst · 2017-12-21T20:46:33Z

@elasticmachine test this please

This commit disables the nio transport as an option for the test transport in integration tests. This is because it does not currently run properly in intellij due to socket permissions. It should be reenabled once elastic#27881 is merged (and the proper permissions are added).

This commit disables the nio transport as an option for the test transport in integration tests. This is because it does not currently run properly in intellij due to socket permissions. It should be reenabled once #27881 is merged (and the proper permissions are added).

This commit adds the infrastructure to plugin building and loading to allow one plugin to extend another. That is, one plugin may extend another by the "parent" plugin allowing itself to be extended through java SPI. When all plugins extending a plugin are finished loading, the "parent" plugin has a callback (through the ExtensiblePlugin interface) allowing it to reload SPI. This commit also adds an example plugin which uses as-yet implemented extensibility (adding to the painless whitelist).

* es/master: (53 commits) Bump compat version for local depdendent test to 6.2.0 Pass `java.locale.providers=COMPAT` to Java 9 onwards (#28080) Allow shrinking of indices from a previous major (#28076) Remove deprecated exceptions (#28059) Add Writeable.Reader support to TransportResponseHandler (#28010) Plugins: Add plugin extension capabilities (#27881) Fix cluster.routing.allocation.enable and cluster.routing.rebalance.enable casing (#28037) [Test] Fix scores for dcg in RankEvalRequestIT and RankEvalYamlIT [Docs] Add note on limitation for significant_text with nested objects (#28052) Enable convert processor to support Long and Double. (#27957) Enable Wildfly tests on JDK 9 and JDK 10 [Test] Fix allowed delta for calculated scores in DiscountedCumulativeGainTests [Test] Mute DiscountedCumulativeGainTests on ARM Only bind loopback addresses when binding to local Fix assertion in Wildfly build Fix typo in comment in Wildfly build Use ephemeral ports in Wildfly tests Update fuzzy-query.asciidoc (#28032) Just another elasticsearch library (#27996) Disable nio test transport (#28028) ...

* master: Set the elasticsearch-nio codebase for tests (#28067) Bump compat version for local depdendent test to 6.2.0 Pass `java.locale.providers=COMPAT` to Java 9 onwards (#28080) Allow shrinking of indices from a previous major (#28076) Remove deprecated exceptions (#28059) Add Writeable.Reader support to TransportResponseHandler (#28010) Plugins: Add plugin extension capabilities (#27881)

* es/master: Introduce Gradle wrapper Ignore GIT_COMMIT when calculating commit hash Re-enable bwc tests after #27881 was backported Set the elasticsearch-nio codebase for tests (#28067)

* es/6.x: Fix link to parent join (#28085) Introduce Gradle wrapper Ignore GIT_COMMIT when calculating commit hash Plugins: Add plugin extension capabilities (#27881)

* master: test: replaced try-catch statements with expectThrows(...) Add getWarmer and getTranslog method to NodeIndicesStats (elastic#28092) fix doc mistake Added ASN support for Ingest GeoIP plugin. Fix global aggregation that requires breadth first and scores (elastic#27942) Introduce Gradle wrapper Ignore GIT_COMMIT when calculating commit hash Re-enable bwc tests after elastic#27881 was backported

* master: (25 commits) Remove Gradle cheatsheet Fix reproduction info to point to Gradle wrapper Update platforms tests to use Gradle wrapper Update testing docs to reflect Gradle wrapper Painless: Modify Loader to Load Classes Directly from Definition (elastic#28088) Update contributing docs to use the Gradle wrapper Create nio-transport plugin for NioTransport (elastic#27949) test: replaced try-catch statements with expectThrows(...) Add getWarmer and getTranslog method to NodeIndicesStats (elastic#28092) fix doc mistake Added ASN support for Ingest GeoIP plugin. Fix global aggregation that requires breadth first and scores (elastic#27942) Introduce Gradle wrapper Ignore GIT_COMMIT when calculating commit hash Re-enable bwc tests after elastic#27881 was backported Set the elasticsearch-nio codebase for tests (elastic#28067) Bump compat version for local depdendent test to 6.2.0 Pass `java.locale.providers=COMPAT` to Java 9 onwards (elastic#28080) Allow shrinking of indices from a previous major (elastic#28076) Remove deprecated exceptions (elastic#28059) ...

sscarduzio · 2018-02-10T19:38:04Z

Hi guys, is there a way to load lib/plugin-classloader-6.2.0.jar from maven?
I ask because I'm trying to develop my plugin in IntelliJ, so I have the plugin project that depends on

compile 'org.elasticsearch:elasticsearch:' + esVersion
compile 'org.elasticsearch.plugin:transport-netty4-client:' + esVersion

And I keep on getting:

java.lang.NoClassDefFoundError: org/elasticsearch/plugins/ExtendedPluginsClassLoader

rjernst · 2018-02-10T23:59:09Z

@sscarduzio Please ask questions on our forum. A comment on a closed PR is likely to be missed. And more information would be helpful (eg what you are running which results in the missing class). I had intended this to not be published as it is an implementation detail of how plugins are loaded, but I am happy to investigate if that won't work for plugin authors.

rjernst added :Core/Infra/Plugins Plugin API and infrastructure v6.2.0 v7.0.0 labels Dec 19, 2017

rjernst requested review from jasontedor and s1monw December 19, 2017 00:37

s1monw reviewed Dec 19, 2017

View reviewed changes

rjernst added 5 commits December 19, 2017 13:41

Move pluginc classloader to a separate jar

a9bf9ac

Remove reference to plugin classloader from core pom and fix transport

1adc243

client

fixup test

0f0e067

Add dummy plugin for plugin service tests, and add support in

7b9ff78

BootstrapForTesting to fill in libs codebases when testing

add insurance

083b922

jasontedor reviewed Dec 21, 2017

View reviewed changes

Address PR comments and add forbidden suppression

319e88a

jasontedor approved these changes Dec 21, 2017

View reviewed changes

clintongormley added the >enhancement label Dec 21, 2017

fix precommit

80dae08

rjernst added 5 commits December 21, 2017 13:07

fix evil tests

01596bf

fix more tests

da1f82e

Merge branch 'master' into plugin_spi

52a57b6

disable bwc tests until backport is done

c71bc7c

Merge branch 'master' into plugin_spi

598501d

Tim-Brooks mentioned this pull request Dec 29, 2017

Running integration tests with elasticsearch-nio fails in Intellij #28011

Closed

Tim-Brooks mentioned this pull request Dec 30, 2017

Disable nio test transport #28028

Merged

fix serialization and bwc testing

793344b

rjernst added 5 commits January 2, 2018 12:46

fix checkstyle

56fb85d

fix more tests

768f324

Merge branch 'master' into plugin_spi

871a51f

rearrange test leniency

cba4379

use module getter

4d83c6b

rjernst merged commit d36ec18 into elastic:master Jan 3, 2018

rjernst deleted the plugin_spi branch January 3, 2018 19:12

rjernst added a commit that referenced this pull request Jan 4, 2018

Re-enable bwc tests after #27881 was backported

cb2334a

tvernum mentioned this pull request May 10, 2018

Teach reindex to honor xpack.ssl.certificate_authorities #29755

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugins: Add plugin extension capabilities #27881

Plugins: Add plugin extension capabilities #27881

rjernst commented Dec 19, 2017

rjernst commented Dec 19, 2017

s1monw left a comment

s1monw Dec 19, 2017

rjernst Dec 19, 2017

s1monw Dec 19, 2017

rjernst Dec 19, 2017

s1monw Dec 19, 2017

s1monw Dec 19, 2017

rjernst Dec 19, 2017

jasontedor Dec 21, 2017

s1monw Dec 19, 2017

s1monw Dec 19, 2017

rjernst Dec 19, 2017

jasontedor left a comment

jasontedor Dec 19, 2017

rjernst Dec 21, 2017

jasontedor Dec 19, 2017

jasontedor Dec 19, 2017

rjernst Dec 21, 2017

jasontedor Dec 21, 2017

jasontedor Dec 19, 2017

rjernst Dec 21, 2017

jasontedor Dec 21, 2017

rjernst Dec 21, 2017

jasontedor Dec 19, 2017

jasontedor Dec 21, 2017

jasontedor Dec 21, 2017

rjernst Dec 21, 2017

rjernst commented Dec 21, 2017

jasontedor left a comment

rjernst commented Dec 21, 2017

sscarduzio commented Feb 10, 2018 •

edited

Loading

rjernst commented Feb 10, 2018 •

edited

Loading


		import org.elasticsearch.plugins.Plugin;

		public class MyWhitelistPlugin extends Plugin {

Plugins: Add plugin extension capabilities #27881

Plugins: Add plugin extension capabilities #27881

Conversation

rjernst commented Dec 19, 2017

rjernst commented Dec 19, 2017

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Dec 21, 2017

jasontedor left a comment

Choose a reason for hiding this comment

rjernst commented Dec 21, 2017

sscarduzio commented Feb 10, 2018 • edited Loading

rjernst commented Feb 10, 2018 • edited Loading

sscarduzio commented Feb 10, 2018 •

edited

Loading

rjernst commented Feb 10, 2018 •

edited

Loading