Make RestController pluggable #98187
Conversation
This commit changes the ActionModules to allow the RestController to be provided by an internal plugin. It rename `RestInterceptorActionPlugin` to `RestServerActionPlugin` and adds a new `getRestController` method to it. There may be multiple RestServerActionPlugins installed on a node, but only 1 may provide a Rest Wrapper (getRestHandlerInterceptor) and only 1 may provide a RestController (getRestController).
tvernum
added
>enhancement
:Core/Infra/REST API
elasticsearchmachine
added
v8.10.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @tvernum, I've created a changelog YAML for you. |
|
@elasticmachine update branch |
| @@ -40,7 +40,15 @@ public void testQualifiedExports() { | |||
|
|
|||
| // The package containing the RestInterceptor type, org.elasticsearch.plugins.interceptor, | |||
| // should only be exported to security. | |||
There was a problem hiding this comment.
perhaps a comment needs to be updated too?
| } | ||
| } | ||
|
|
||
| public void test3rdPartyRestControllerIsNotInstalled() { |
There was a problem hiding this comment.
can this really happen? we only export the org.elasticsearch.plugins.interceptor to serverless and security.
Someone would have to run ES server without java9 modules? (tests don't have them enabled)
There was a problem hiding this comment.
Yes, it's possible to have a plugin that is not a module, in which case qualified exports are not respected.
Perhaps we need to get to a place where we require that plugins are modularized, but we don't today.
There was a problem hiding this comment.
I think that if a plugin is not a module it would compile, but it would fail at runtime
I would expect that we would get a runtime exception along the lines
Exception in thread "main" java.lang.IllegalAccessError: superinterface check failed: class wrong.plugin.XYZ (in unnamed module @0x19469ea2) cannot access class org.elasticsearch.plugins.interceptor.RestServerActionPlugin (in module org.elasticserach.server) because module org.elasticsearch.server does not export org.elasticsearch.plugins.interceptor to unnamed module @0x19469ea2
(note the above message I 'hand crafted' basing on a simple reproduction project structure with 4 classes)
at the same time I don't mind this test, I just thought we don't need to wory about this (I don't test for other internal plugins)
There was a problem hiding this comment.
TIL.
I thought I had tested with a non-modularized plugin, but I suspect it might have worked because I temporarily had an open export.
| ); | ||
| headersToCopy = headers; | ||
|
|
||
| var customController = getRestServerComponent( |
There was a problem hiding this comment.
the plan is to support multiple API Versions on the same cluster, right?
isn't the limit of one plugin implementing RestServerActionPlugin prevent us to do this?
When reading the doc I was imagining we would like to provide multiple RestControllers - one per one Version supported.
how will customController support multiple API versions?
There was a problem hiding this comment.
Right now we're only supporting 1 API version, so that's a future problem.
However, I'd expect it to be a single rest controller in that case with some additional logic for dispatch - similar to how the existing controller handles V7 endpoints.
tvernum
added
the
auto-merge
This commit changes the ActionModules to allow the RestController to be provided by an internal plugin.
It renames
RestInterceptorActionPlugintoRestServerActionPluginand adds a newgetRestControllermethod to it.There may be multiple RestServerActionPlugins installed on a node, but only 1 may provide a Rest Wrapper (getRestHandlerInterceptor) and only 1 may provide a RestController (getRestController).