Split plugin loading into two different phases to support entitlements #116998

jdconrad · 2024-11-18T23:57:59Z

This change loads all the modules and creates the module layers for plugins prior to entitlement checking during the 2nd phase of bootstrap initialization. This will allow us to know what modules exist for both validation and checking prior to actually loading any plugin classes (in a follow up change).

There are now two classes:

PluginsLoader which does the module loading and layer creation
PluginsService which uses a PluginsLoader to create the main plugin classes and start the plugins

elasticsearchmachine · 2024-11-18T23:58:23Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

ldematte

Looks good! A couple of (optional) notes.
I've tried to follow the movement of code from PluginsService to PluginsLoader, and it seems pretty much matching.

server/src/main/java/org/elasticsearch/plugins/PluginsLoader.java

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

rjernst

LGTM, a few suggestions but most of them are subjective and could be followups if we decide to do them.

rjernst · 2024-11-19T01:03:57Z

benchmarks/src/main/java/org/elasticsearch/benchmark/script/ScriptScoreBenchmark.java

        null,
-        null,
-        Path.of(System.getProperty("plugins.dir"))
+        new PluginsLoader(null, Path.of(System.getProperty("plugins.dir")))


nit: Could this pass an empty directory for module dir? Then we could enforce non-null paths. We needed to support null before because the mock plugins service needed to call its super, but I think we could get around that with the mock loader.

Let's do this as a follow up PR as part of adding MockPluginsLoader potentially.

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

rjernst · 2024-11-19T22:08:58Z

test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java

     */
    public MockPluginsService(Settings settings, Environment environment, Collection<Class<? extends Plugin>> classpathPlugins) {
-        super(settings, environment.configFile(), environment.modulesFile(), environment.pluginsFile());
+        super(settings, environment.configFile(), new PluginsLoader(environment.modulesFile(), environment.pluginsFile()) {


Perhaps we should have a MockPluginsLoader? This anonymous class is difficult to read within a super call

Another idea I had in my POC: what if we remove the protected method, and we pass down a ServerExportsService (or better name!) to the ctor?

Something like

@FunctionalInterface public interface ServerExportsService { void add(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports); }

And then have

public static void noServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {} public static void defaultServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) { // current addServerExportsService implementation }

The ctor called from prod code will pass down PluginsLoader::defaultServerExportsService, but test can call a full ctor passing explicitly PluginsLoader::noServerExportsService:

PluginsLoader(/* same args as today */) { // Called by Elasticsearch.java this(..., PluginsLoader::defaultServerExportsService) } PluginsLoader(/* same args as today */, ServerExportsService additionalServerExports) { this.additionalServerExports = additionalServerExports; // and then call additionalServerExports.add(...); }

Test will call
new PluginsLoader(..., PluginsLoader::noServerExportService) (or whatever they want)
(this will also remove the need to suppress for this-escape)

@rjernst What do you think of the @ldematte's idea instead of a MockPluginLoader? I don't have a strong opinion either way so I'm happy to implement either idea.

If you both like my idea, I think it should go in a follow-up PR anyway :)

On second thought, with multiple different ideas here, let's get together to discuss this as a follow up PR.

Yup, totally agree!

🤔 FWIW I'm not seeing the problem. The code looks pretty clear to me.

The problem is (1) having an anonymous subclass with an overridden method inside a super call packs a lot into that super call. Additionally (2) the overridden implementation is the same in both places (makes the method a no-op).

But we can work this out in a followup.

I agree the anonymous class should go. It's confusing, but I'm wondering if we should also add in @ldematte's idea to replace the exports method. It does seem clearer to me on the surface. The main downside I see to some of these changes it pushes test-only code into PluginsLoader.

ldematte

LGTM2; the optional suggestions still stand, but can go in another PR (or even discarded)

ldematte · 2024-11-20T08:01:58Z

test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java

     */
    public MockPluginsService(Settings settings, Environment environment, Collection<Class<? extends Plugin>> classpathPlugins) {
-        super(settings, environment.configFile(), environment.modulesFile(), environment.pluginsFile());
+        super(settings, environment.configFile(), new PluginsLoader(environment.modulesFile(), environment.pluginsFile()) {


Another idea I had in my POC: what if we remove the protected method, and we pass down a ServerExportsService (or better name!) to the ctor?

Something like

@FunctionalInterface public interface ServerExportsService { void add(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports); }

And then have

public static void noServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {} public static void defaultServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) { // current addServerExportsService implementation }

The ctor called from prod code will pass down PluginsLoader::defaultServerExportsService, but test can call a full ctor passing explicitly PluginsLoader::noServerExportsService:

PluginsLoader(/* same args as today */) { // Called by Elasticsearch.java this(..., PluginsLoader::defaultServerExportsService) } PluginsLoader(/* same args as today */, ServerExportsService additionalServerExports) { this.additionalServerExports = additionalServerExports; // and then call additionalServerExports.add(...); }

Test will call
new PluginsLoader(..., PluginsLoader::noServerExportService) (or whatever they want)
(this will also remove the need to suppress for this-escape)

jdconrad · 2024-11-20T17:16:11Z

@rjernst @ldematte Thank you for the reviews. I will follow up with you both about refactoring the mock code a bit more, but will leave that for a follow up PR.

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

ldematte · 2024-11-20T17:20:26Z

refactoring the mock code a bit more, but will leave that for a follow up PR.

Definitely, thanks for the changes, looks very clean! Merge at will, for me.

elastic#116998) This change loads all the modules and creates the module layers for plugins prior to entitlement checking during the 2nd phase of bootstrap initialization. This will allow us to know what modules exist for both validation and checking prior to actually loading any plugin classes (in a follow up change). There are now two classes: PluginsLoader which does the module loading and layer creation PluginsService which uses a PluginsLoader to create the main plugin classes and start the plugins

#116998) (#117215) This change loads all the modules and creates the module layers for plugins prior to entitlement checking during the 2nd phase of bootstrap initialization. This will allow us to know what modules exist for both validation and checking prior to actually loading any plugin classes (in a follow up change). There are now two classes: PluginsLoader which does the module loading and layer creation PluginsService which uses a PluginsLoader to create the main plugin classes and start the plugins

elastic#116998) This change loads all the modules and creates the module layers for plugins prior to entitlement checking during the 2nd phase of bootstrap initialization. This will allow us to know what modules exist for both validation and checking prior to actually loading any plugin classes (in a follow up change). There are now two classes: PluginsLoader which does the module loading and layer creation PluginsService which uses a PluginsLoader to create the main plugin classes and start the plugins

jdconrad added 6 commits November 13, 2024 14:22

add class plugin loader

ebcbf83

plumb plugins loader into plugins service

632a832

clean up extraneous methods

a197032

removed settings and config path from plugins loader

ee4ce9c

fix mocks and remove extraneous loader info from loaded plugins

8480e75

fix tests

bbc61d1

jdconrad added :Core/Infra/Plugins Plugin API and infrastructure >refactoring labels Nov 18, 2024

jdconrad requested a review from a team as a code owner November 18, 2024 23:58

elasticsearchmachine added v9.0.0 Team:Core/Infra Meta label for core/infra team labels Nov 18, 2024

ldematte mentioned this pull request Nov 19, 2024

Policy manager for entitlements #116695

Merged

ldematte reviewed Nov 19, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/plugins/PluginsLoader.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/plugins/PluginsService.java Outdated Show resolved Hide resolved

Merge branch 'main' into pluginloader

0e7e157

rjernst approved these changes Nov 19, 2024

View reviewed changes

ldematte approved these changes Nov 20, 2024

View reviewed changes

jdconrad added 2 commits November 20, 2024 08:34

Merge branch 'main' into pluginloader

1ca520c

changes based on pr comments

ac55388

ldematte reviewed Nov 20, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/plugins/PluginsService.java Outdated Show resolved Hide resolved

jdconrad added 2 commits November 20, 2024 09:18

response to pr comments

427da67

Merge branch 'main' into pluginloader

5e7712f

jdconrad added 2 commits November 20, 2024 09:24

change to list to foreach

0f724a2

Merge branch 'main' into pluginloader

c1f5993

jdconrad merged commit 4f46924 into elastic:main Nov 20, 2024
15 of 16 checks passed

jdconrad mentioned this pull request Nov 20, 2024

Backport to split plugin loading #117215

Merged

This was referenced Dec 9, 2024

[WIP] Entitlements - Minimal map-backed plugins resolver #116665

Closed

[DO NOT MERGE] Entitlements/split plugin service #116509

Closed

Split plugin loading into two different phases to support entitlements #116998

Split plugin loading into two different phases to support entitlements #116998

Uh oh!

Conversation

jdconrad commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Nov 18, 2024

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Nov 20, 2024

Uh oh!

Uh oh!

ldematte commented Nov 20, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jdconrad commented Nov 18, 2024 •

edited

Loading

jdconrad Nov 20, 2024 •

edited

Loading