Skip to content

Stable Plugin API: Add Ubermodule ClassLoader for non-modularized stable plugins#89365

Merged
williamrandolph merged 46 commits intoelastic:mainfrom
williamrandolph:sp/classloader-scratch
Sep 20, 2022
Merged

Stable Plugin API: Add Ubermodule ClassLoader for non-modularized stable plugins#89365
williamrandolph merged 46 commits intoelastic:mainfrom
williamrandolph:sp/classloader-scratch

Conversation

@williamrandolph
Copy link
Copy Markdown
Contributor

@williamrandolph williamrandolph commented Aug 16, 2022

This PR's UberModuleClassLoader takes a group of jars and loads them as a single synthetic module.

The motivation for this work is the Stable Plugin API project, but the classloader is general enough for other use cases. In the case of stable plugins, we don't want to require that plugin developers modularize their plugins. (If a plugin developer chooses to modularize their plugin, then we can load it using JDK-provided utilities.)

The advantage of a synthetic module is that we can programmatically use module layers to control what the code from the synthetic module has access to. A non-modularized plugin will be provided in the form of one or more jars. When this classloader loads a class from one of those jars, that class will be bound to the synthetic module. We can use a deny-list to prevent the class from accessing packages from particular modules.

In the world of modules, each module owns the packages it contains, and it is forbidden to split packages across module boundaries. This means that when we load a class, we can check its package name. If our synthetic module owns the package, then we don't need to search for the class in parent classloaders. When fetching jar resources, however, we simply delegate to the internal URLClassLoader.

@williamrandolph williamrandolph added the test-windows Trigger CI checks on Windows label Aug 26, 2022
@williamrandolph williamrandolph changed the title WIP - Stable plugin classloader Stable Plugin API: Add Ubermodule ClassLoader for non-modularized stable plugins Aug 31, 2022
@williamrandolph williamrandolph marked this pull request as ready for review August 31, 2022 18:37
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Aug 31, 2022

this.internalLoader = new URLClassLoader(jarURLs);
// code source is always the first jar on the list
this.codeSource = new CodeSource(jarURLs[0], (CodeSigner[]) null);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to revisit this, but it is fine for the initial implementation.

.stream()
.filter(Module::isNamed)
.filter(m -> "java.base".equals(m.getName()) == false)
.filter(m -> moduleDenyList.contains(m.getName()) == false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deny-list is great, but at some point it might be more straightforward to have an allow-list, say to allow readability to all the JDK modules and a subset of the ES modules.

return defineClass(name, bytes, 0, bytes.length, codeSource);
} catch (IOException e) {
// TODO
throw new IllegalStateException(e);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably log here or something, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched it to UncheckedIOException, since that's what we're throwing in EmbeddedImplClassLoader.


/**
* This classloader does not restrict access to resources in its jars. Users should
* expect the same behavior as that provided by {@link URLClassLoader}.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems pragmatic.

// if cn's package is in this ubermodule, look here only (or just first?)
String packageName = packageName(cn);

// TODO: do we need to check the security manager here?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's revisit this later, I wanna spend a bit more time thinking on it.

@williamrandolph williamrandolph mentioned this pull request Sep 6, 2022
25 tasks
@williamrandolph
Copy link
Copy Markdown
Contributor Author

@elasticsearchmachine please run elasticsearch-ci/part-1-windows

Copy link
Copy Markdown
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

// code source is always the first jar on the list
this.codeSource = new CodeSource(jarURLs[0], (CodeSigner[]) null);
// we need a module layer to bind our module to this classloader
this.moduleController = ModuleLayer.defineModules(cf, List.of(mparent), s -> this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be helpful to have a more detailed comment how this binding works. I think it might not be obvious at first and I feel like this is the crucial bit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll add that comment before merging.

@williamrandolph
Copy link
Copy Markdown
Contributor Author

@elasticmachine update branch

@williamrandolph williamrandolph merged commit 635b553 into elastic:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Plugins Plugin API and infrastructure >non-issue Team:Core/Infra Meta label for core/infra team test-windows Trigger CI checks on Windows v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants