From 04d0e2979fec4d7d260bf6ddebe2382d6e60a051 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 16 Jun 2022 17:28:54 -0400 Subject: [PATCH 1/8] Add OperatorHandler and SPI --- server/src/main/java/module-info.java | 1 + .../operator/OperatorHandler.java | 118 ++++++++++++++++++ .../operator/OperatorHandlerProvider.java | 25 ++++ .../operator/TransformState.java | 21 ++++ .../elasticsearch/plugins/PluginsService.java | 28 ++++- .../elasticsearch/bootstrap/security.policy | 3 + .../plugins/PluginsServiceTests.java | 88 +++++++++++++ .../plugins/MockPluginsService.java | 2 +- 8 files changed, 284 insertions(+), 2 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/operator/OperatorHandler.java create mode 100644 server/src/main/java/org/elasticsearch/operator/OperatorHandlerProvider.java create mode 100644 server/src/main/java/org/elasticsearch/operator/TransformState.java diff --git a/server/src/main/java/module-info.java b/server/src/main/java/module-info.java index f0b4ccd3ff049..0ae63370a22d4 100644 --- a/server/src/main/java/module-info.java +++ b/server/src/main/java/module-info.java @@ -272,6 +272,7 @@ exports org.elasticsearch.monitor.os; exports org.elasticsearch.monitor.process; exports org.elasticsearch.node; + exports org.elasticsearch.operator; exports org.elasticsearch.persistent; exports org.elasticsearch.persistent.decider; exports org.elasticsearch.plugins; diff --git a/server/src/main/java/org/elasticsearch/operator/OperatorHandler.java b/server/src/main/java/org/elasticsearch/operator/OperatorHandler.java new file mode 100644 index 0000000000000..29ed71c3c29f4 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/operator/OperatorHandler.java @@ -0,0 +1,118 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator; + +import org.elasticsearch.ElasticsearchGenerationException; +import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.action.support.master.MasterNodeRequest; +import org.elasticsearch.common.Strings; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; + +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.Map; + +/** + * Updating cluster state in operator mode, for file based settings and modules/plugins, requires + * that we have a separate update handler interface that is different than REST handlers. This interface declares + * the basic contract for implementing cluster state update handlers in operator mode. + */ +public interface OperatorHandler { + String CONTENT = "content"; + + /** + * The operator handler name is a unique identifier that is matched to a section in a + * cluster state update content. The operator cluster state updates are done as a single + * cluster state update and the cluster state is typically supplied as a combined content, + * unlike the REST handlers. This name must match a desired content key name in the combined + * cluster state update, e.g. "ilm" or "cluster_settings" (for persistent cluster settings update). + * + * @return a String with the operator key name, e.g "ilm". + */ + String name(); + + /** + * The transform method of the operator handler should apply the necessary changes to + * the cluster state as it normally would in a REST handler. One difference is that the + * transform method in an operator handler must perform all CRUD operations of the cluster + * state in one go. For that reason, we supply a wrapper class to the cluster state called + * {@link TransformState}, which contains the current cluster state as well as any previous keys + * set by this handler on prior invocation. + * + * @param source The parsed information specific to this handler from the combined cluster state content + * @param prevState The previous cluster state and keys set by this handler (if any) + * @return The modified state and the current keys set by this handler + * @throws Exception + */ + TransformState transform(Object source, TransformState prevState) throws Exception; + + /** + * Sometimes certain parts of the cluster state cannot be created/updated without previously + * setting other cluster state components, e.g. composable templates. Since the cluster state handlers + * are processed in random order by the OperatorClusterStateController, this method gives an opportunity + * to any operator handler to declare other operator handlers it depends on. Given dependencies exist, + * the OperatorClusterStateController will order those handlers such that the handlers that are dependent + * on are processed first. + * + * @return a collection of operator handler names + */ + default Collection dependencies() { + return Collections.emptyList(); + } + + /** + * All implementations of OperatorHandler should call the request validate method, by calling this default + * implementation. To aid in any special validation logic that may need to be implemented by the operator handler + * we provide this convenience method. + * + * @param request the master node request that we base this operator handler on + */ + default void validate(MasterNodeRequest request) { + ActionRequestValidationException exception = request.validate(); + if (exception != null) { + throw new IllegalStateException("Validation error", exception); + } + } + + /** + * Convenience method to convert the incoming passed in input to the transform method into a map. + * + * @param input the input passed into the operator handler after parsing the content + * @return + */ + @SuppressWarnings("unchecked") + default Map asMap(Object input) { + if (input instanceof Map source) { + return (Map) source; + } + throw new IllegalStateException("Unsupported " + name() + " request format"); + } + + /** + * Convenience method that creates a {@link XContentParser} from a content map so that it can be passed to + * existing REST based code for input parsing. + * + * @param config XContentParserConfiguration for this mapper + * @param source the operator content as a map + * @return + */ + default XContentParser mapToXContentParser(XContentParserConfiguration config, Map source) { + try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) { + builder.map(source); + return XContentFactory.xContent(builder.contentType()).createParser(config, Strings.toString(builder)); + } catch (IOException e) { + throw new ElasticsearchGenerationException("Failed to generate [" + source + "]", e); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/operator/OperatorHandlerProvider.java b/server/src/main/java/org/elasticsearch/operator/OperatorHandlerProvider.java new file mode 100644 index 0000000000000..0f168ea6e4d61 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/operator/OperatorHandlerProvider.java @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator; + +import java.util.Collection; + +/** + * SPI service interface for supplying OperatorHandler implementations to Elasticsearch + * from plugins/modules. + */ +public interface OperatorHandlerProvider { + /** + * Returns a list of OperatorHandler implementations that a module/plugin supplies. + * @see OperatorHandler + * + * @return a list of ${@link OperatorHandler}s + */ + Collection> handlers(); +} diff --git a/server/src/main/java/org/elasticsearch/operator/TransformState.java b/server/src/main/java/org/elasticsearch/operator/TransformState.java new file mode 100644 index 0000000000000..a68ce7e1f2290 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/operator/TransformState.java @@ -0,0 +1,21 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator; + +import org.elasticsearch.cluster.ClusterState; + +import java.util.Set; + +/** + * A {@link ClusterState} wrapper used by the OperatorClusterStateController to pass the + * current state as well as previous keys set by an {@link OperatorHandler} to each transform + * step of the cluster state update. + * + */ +public record TransformState(ClusterState state, Set keys) {} diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index d5af93af4a2c4..4d72a2731492d 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -47,11 +47,13 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.ServiceLoader; import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; @@ -281,7 +283,6 @@ private Map loadBundles(Set bundles) { // package-private for test visibility static void loadExtensions(Collection plugins) { - Map> extendingPluginsByName = plugins.stream() .flatMap(t -> t.descriptor().getExtendedPlugins().stream().map(extendedPlugin -> Tuple.tuple(extendedPlugin, t.instance()))) .collect(Collectors.groupingBy(Tuple::v1, Collectors.mapping(Tuple::v2, Collectors.toList()))); @@ -295,6 +296,31 @@ static void loadExtensions(Collection plugins) { } } + // package private for testing + Iterator providersIterator(Class service, ClassLoader classLoader) { + return ServiceLoader.load(service, classLoader).iterator(); + } + + /** + * SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers + * from plugins/modules. For example: + * + * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); + * + * @param service A templated service class to look for providers in plugins + * @return an immutable {@link List} of discovered providers in the plugins/modules + */ + public List loadServiceProviders(Class service) { + List result = new ArrayList<>(); + getClass().getModule().addUses(service); + + for (LoadedPlugin pluginTuple : plugins()) { + providersIterator(service, pluginTuple.loader()).forEachRemaining(c -> result.add(c)); + } + + return Collections.unmodifiableList(result); + } + private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, List extendingPlugins) { ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() { @Override diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy index f9b37f65538f2..b4c4500f62daa 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -170,4 +170,7 @@ grant { // system memory on Linux systems affected by JDK bug (#66629) permission java.io.FilePermission "/proc/meminfo", "read"; + + // needed for PluginsServiceTests + permission java.lang.RuntimePermission "createClassLoader"; }; diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 78f2d39c1eddc..b4b9c4bba3cf0 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -12,9 +12,14 @@ import org.apache.lucene.util.Constants; import org.elasticsearch.Version; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.PathUtils; +import org.elasticsearch.core.Strings; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.IndexModule; +import org.elasticsearch.operator.OperatorHandler; +import org.elasticsearch.operator.OperatorHandlerProvider; +import org.elasticsearch.operator.TransformState; import org.elasticsearch.test.ESTestCase; import java.io.IOException; @@ -27,6 +32,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Set; @@ -38,6 +44,9 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.sameInstance; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS") public class PluginsServiceTests extends ESTestCase { @@ -652,6 +661,66 @@ public void testThrowingConstructor() { assertThat(e.getCause().getCause(), hasToString(containsString("test constructor failure"))); } + public void testLoadServiceProviders() throws ClassNotFoundException { + FakeClassLoader fakeClassLoader = new FakeClassLoader(); + @SuppressWarnings("unchecked") + Class fakePluginClass = (Class) fakeClassLoader.findClass(FakePlugin.class.getName()); + + FakeClassLoader fakeClassLoader1 = new FakeClassLoader(); + @SuppressWarnings("unchecked") + Class fakePluginClass1 = (Class) fakeClassLoader1.findClass(FakePlugin.class.getName()); + + assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader())); + + OperatorHandlerProvider provider1 = () -> List.of(new OperatorHandler() { + @Override + public String name() { + return "integer"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) { + return prevState; + } + }); + + OperatorHandlerProvider provider2 = () -> List.of(new OperatorHandler() { + @Override + public String name() { + return "string"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) { + return prevState; + } + }); + PluginsService service = spy(newMockPluginsService(List.of(fakePluginClass, fakePluginClass1))); + + doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); + doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); + + List providers = service.loadServiceProviders(OperatorHandlerProvider.class); + assertEquals(2, providers.size()); + List handlers = providers.stream().map(p -> p.handlers()).flatMap(l -> l.stream()).map(h -> h.name()).toList(); + + assertThat(handlers, containsInAnyOrder("string", "integer")); + + doReturn(Collections.emptyList().iterator()).when(service) + .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); + doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); + + assertEquals(1, service.loadServiceProviders(OperatorHandlerProvider.class).size()); + + doReturn(Collections.emptyList().iterator()).when(service) + .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); + doReturn(Collections.emptyList().iterator()).when(service) + .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); + + assertEquals(0, service.loadServiceProviders(OperatorHandlerProvider.class).size()); + + } + private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin { private List extensions; @@ -691,4 +760,23 @@ public ThrowingConstructorExtension() { throw new IllegalArgumentException("test constructor failure"); } } + + static class FakeClassLoader extends ClassLoader { + @Override + public Class findClass(String name) throws ClassNotFoundException { + byte[] classBytes = fromFile(getClass().getClassLoader(), name); + return defineClass(name, classBytes, 0, classBytes.length); + } + + private static byte[] fromFile(ClassLoader realLoader, String fileName) { + try { + InputStream input = realLoader.getResourceAsStream( + Strings.format("%s.class", fileName.replace(".", PathUtils.get(".").getFileSystem().getSeparator())) + ); + return input.readAllBytes(); + } catch (Exception x) { + throw new IllegalStateException("Error loading class", x); + } + } + } } diff --git a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java index 4f44e0be17249..f824201a93f4e 100644 --- a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java +++ b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java @@ -60,7 +60,7 @@ public MockPluginsService(Settings settings, Environment environment, Collection if (logger.isTraceEnabled()) { logger.trace("plugin loaded from classpath [{}]", pluginInfo); } - pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin)); + pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin, pluginClass.getClassLoader(), ModuleLayer.boot())); } this.classpathPlugins = List.copyOf(pluginsLoaded); From 4a67eded2f1deea8670e74e5e0659e52458534c2 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 10:23:52 -0400 Subject: [PATCH 2/8] Better docs --- .../java/org/elasticsearch/plugins/PluginsService.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index 125def8d79770..f72ad2c876979 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -301,10 +301,13 @@ Iterator providersIterator(Class service, ClassLoader classLoader) { /** * SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers - * from plugins/modules. For example: + * from plugins/modules. * - * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); + *

+ * For example: * + * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); + *

* @param service A templated service class to look for providers in plugins * @return an immutable {@link List} of discovered providers in the plugins/modules */ From 4517be80b0d5ab1a0961372a278fe73d278a6b98 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 10:33:24 -0400 Subject: [PATCH 3/8] Fix wrong security policy file --- .../resources/org/elasticsearch/bootstrap/security.policy | 4 +--- .../org/elasticsearch/bootstrap/test-framework.policy | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy index b4c4500f62daa..24b4483964fc9 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -170,7 +170,5 @@ grant { // system memory on Linux systems affected by JDK bug (#66629) permission java.io.FilePermission "/proc/meminfo", "read"; - - // needed for PluginsServiceTests - permission java.lang.RuntimePermission "createClassLoader"; + permission java.io.FilePermission "/proc/meminfo", "read"; }; diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy index 8907c4d463d3a..caa014c0c5059 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy @@ -125,4 +125,6 @@ grant codeBase "${codebase.netty-transport}" { grant { permission java.net.SocketPermission "127.0.0.1", "connect, resolve"; permission java.nio.file.LinkPermission "symbolic"; + // needed for PluginsServiceTests + permission java.lang.RuntimePermission "createClassLoader"; }; From 6d6b0bf7f27340b5dad87dec9aa05b6ee3fe8694 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 10:34:25 -0400 Subject: [PATCH 4/8] Fix line dup --- .../main/resources/org/elasticsearch/bootstrap/security.policy | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy index 24b4483964fc9..f9b37f65538f2 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -170,5 +170,4 @@ grant { // system memory on Linux systems affected by JDK bug (#66629) permission java.io.FilePermission "/proc/meminfo", "read"; - permission java.io.FilePermission "/proc/meminfo", "read"; }; From ec62beab4b30069cf7396250389c6d391f70eb9e Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 13:57:15 -0400 Subject: [PATCH 5/8] Fake the classloaders instead of using real --- .../bootstrap/test-framework.policy | 2 - .../plugins/PluginsServiceTests.java | 54 ++++++++----------- .../plugins/MockPluginsService.java | 8 ++- 3 files changed, 28 insertions(+), 36 deletions(-) diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy index caa014c0c5059..8907c4d463d3a 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy @@ -125,6 +125,4 @@ grant codeBase "${codebase.netty-transport}" { grant { permission java.net.SocketPermission "127.0.0.1", "connect, resolve"; permission java.nio.file.LinkPermission "symbolic"; - // needed for PluginsServiceTests - permission java.lang.RuntimePermission "createClassLoader"; }; diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index c82983d83b1f2..0054fdc569df3 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -12,8 +12,6 @@ import org.apache.lucene.util.Constants; import org.elasticsearch.Version; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.core.PathUtils; -import org.elasticsearch.core.Strings; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.IndexModule; @@ -46,6 +44,7 @@ import static org.hamcrest.Matchers.sameInstance; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS") @@ -623,15 +622,10 @@ public void testThrowingConstructor() { } public void testLoadServiceProviders() throws ClassNotFoundException { - FakeClassLoader fakeClassLoader = new FakeClassLoader(); - @SuppressWarnings("unchecked") - Class fakePluginClass = (Class) fakeClassLoader.findClass(FakePlugin.class.getName()); - - FakeClassLoader fakeClassLoader1 = new FakeClassLoader(); - @SuppressWarnings("unchecked") - Class fakePluginClass1 = (Class) fakeClassLoader1.findClass(FakePlugin.class.getName()); - - assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader())); + ClassLoader fakeClassLoader = mock(ClassLoader.class); + ClassLoader fakeClassLoader1 = mock(ClassLoader.class); + FakePlugin fakePlugin = new FakePlugin(); + FakePlugin fakePlugin1 = new FakePlugin(); OperatorHandlerProvider provider1 = () -> List.of(new OperatorHandler() { @Override @@ -656,7 +650,23 @@ public TransformState transform(Object source, TransformState prevState) { return prevState; } }); - PluginsService service = spy(newMockPluginsService(List.of(fakePluginClass, fakePluginClass1))); + // Make the service load without plugins, we mock and force set them after + PluginsService service = newMockPluginsService(new ArrayList<>()); + PluginsService.LoadedPlugin plugin1 = new PluginsService.LoadedPlugin( + mock(PluginDescriptor.class), + fakePlugin, + fakeClassLoader, + ModuleLayer.boot() + ); + PluginsService.LoadedPlugin plugin2 = new PluginsService.LoadedPlugin( + mock(PluginDescriptor.class), + fakePlugin1, + fakeClassLoader1, + ModuleLayer.boot() + ); + ((MockPluginsService) service).setLoadedPlugins(List.of(plugin1, plugin2)); + + service = spy(service); doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); @@ -679,7 +689,6 @@ public TransformState transform(Object source, TransformState prevState) { .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); assertEquals(0, service.loadServiceProviders(OperatorHandlerProvider.class).size()); - } private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin { @@ -721,23 +730,4 @@ public ThrowingConstructorExtension() { throw new IllegalArgumentException("test constructor failure"); } } - - static class FakeClassLoader extends ClassLoader { - @Override - public Class findClass(String name) throws ClassNotFoundException { - byte[] classBytes = fromFile(getClass().getClassLoader(), name); - return defineClass(name, classBytes, 0, classBytes.length); - } - - private static byte[] fromFile(ClassLoader realLoader, String fileName) { - try { - InputStream input = realLoader.getResourceAsStream( - Strings.format("%s.class", fileName.replace(".", PathUtils.get(".").getFileSystem().getSeparator())) - ); - return input.readAllBytes(); - } catch (Exception x) { - throw new IllegalStateException("Error loading class", x); - } - } - } } diff --git a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java index 60c10f430c911..21a5d051d97b6 100644 --- a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java +++ b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java @@ -25,7 +25,7 @@ public class MockPluginsService extends PluginsService { private static final Logger logger = LogManager.getLogger(MockPluginsService.class); - private final List classpathPlugins; + private List classpathPlugins; /** * Constructs a new PluginService @@ -58,7 +58,7 @@ public MockPluginsService(Settings settings, Environment environment, Collection if (logger.isTraceEnabled()) { logger.trace("plugin loaded from classpath [{}]", pluginInfo); } - pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin, pluginClass.getClassLoader(), ModuleLayer.boot())); + pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin)); } this.classpathPlugins = List.copyOf(pluginsLoaded); @@ -69,6 +69,10 @@ protected final List plugins() { return this.classpathPlugins; } + void setLoadedPlugins(List plugins) { + this.classpathPlugins = List.copyOf(plugins); + } + @Override public PluginsAndModules info() { return new PluginsAndModules( From 20cf2e7d07ceb14386cef85764b66f499206303a Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 15:02:20 -0400 Subject: [PATCH 6/8] Bring back real class loaders in tests --- .../elasticsearch/plugins/PluginsService.java | 5 +- .../bootstrap/test-framework.policy | 2 + .../plugins/PluginsServiceTests.java | 54 +++++++++++-------- .../plugins/MockPluginsService.java | 8 +-- 4 files changed, 38 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index f72ad2c876979..50f950023f8a4 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -303,17 +303,16 @@ Iterator providersIterator(Class service, ClassLoader classLoader) { * SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers * from plugins/modules. * - *

+ * 

      * For example:
      *
      * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class);
-     * 

+     *
* @param service A templated service class to look for providers in plugins * @return an immutable {@link List} of discovered providers in the plugins/modules */ public List loadServiceProviders(Class service) { List result = new ArrayList<>(); - getClass().getModule().addUses(service); for (LoadedPlugin pluginTuple : plugins()) { providersIterator(service, pluginTuple.loader()).forEachRemaining(c -> result.add(c)); diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy index 8907c4d463d3a..caa014c0c5059 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy @@ -125,4 +125,6 @@ grant codeBase "${codebase.netty-transport}" { grant { permission java.net.SocketPermission "127.0.0.1", "connect, resolve"; permission java.nio.file.LinkPermission "symbolic"; + // needed for PluginsServiceTests + permission java.lang.RuntimePermission "createClassLoader"; }; diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 0054fdc569df3..c82983d83b1f2 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -12,6 +12,8 @@ import org.apache.lucene.util.Constants; import org.elasticsearch.Version; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.PathUtils; +import org.elasticsearch.core.Strings; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.IndexModule; @@ -44,7 +46,6 @@ import static org.hamcrest.Matchers.sameInstance; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS") @@ -622,10 +623,15 @@ public void testThrowingConstructor() { } public void testLoadServiceProviders() throws ClassNotFoundException { - ClassLoader fakeClassLoader = mock(ClassLoader.class); - ClassLoader fakeClassLoader1 = mock(ClassLoader.class); - FakePlugin fakePlugin = new FakePlugin(); - FakePlugin fakePlugin1 = new FakePlugin(); + FakeClassLoader fakeClassLoader = new FakeClassLoader(); + @SuppressWarnings("unchecked") + Class fakePluginClass = (Class) fakeClassLoader.findClass(FakePlugin.class.getName()); + + FakeClassLoader fakeClassLoader1 = new FakeClassLoader(); + @SuppressWarnings("unchecked") + Class fakePluginClass1 = (Class) fakeClassLoader1.findClass(FakePlugin.class.getName()); + + assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader())); OperatorHandlerProvider provider1 = () -> List.of(new OperatorHandler() { @Override @@ -650,23 +656,7 @@ public TransformState transform(Object source, TransformState prevState) { return prevState; } }); - // Make the service load without plugins, we mock and force set them after - PluginsService service = newMockPluginsService(new ArrayList<>()); - PluginsService.LoadedPlugin plugin1 = new PluginsService.LoadedPlugin( - mock(PluginDescriptor.class), - fakePlugin, - fakeClassLoader, - ModuleLayer.boot() - ); - PluginsService.LoadedPlugin plugin2 = new PluginsService.LoadedPlugin( - mock(PluginDescriptor.class), - fakePlugin1, - fakeClassLoader1, - ModuleLayer.boot() - ); - ((MockPluginsService) service).setLoadedPlugins(List.of(plugin1, plugin2)); - - service = spy(service); + PluginsService service = spy(newMockPluginsService(List.of(fakePluginClass, fakePluginClass1))); doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); @@ -689,6 +679,7 @@ public TransformState transform(Object source, TransformState prevState) { .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1)); assertEquals(0, service.loadServiceProviders(OperatorHandlerProvider.class).size()); + } private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin { @@ -730,4 +721,23 @@ public ThrowingConstructorExtension() { throw new IllegalArgumentException("test constructor failure"); } } + + static class FakeClassLoader extends ClassLoader { + @Override + public Class findClass(String name) throws ClassNotFoundException { + byte[] classBytes = fromFile(getClass().getClassLoader(), name); + return defineClass(name, classBytes, 0, classBytes.length); + } + + private static byte[] fromFile(ClassLoader realLoader, String fileName) { + try { + InputStream input = realLoader.getResourceAsStream( + Strings.format("%s.class", fileName.replace(".", PathUtils.get(".").getFileSystem().getSeparator())) + ); + return input.readAllBytes(); + } catch (Exception x) { + throw new IllegalStateException("Error loading class", x); + } + } + } } diff --git a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java index 21a5d051d97b6..60c10f430c911 100644 --- a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java +++ b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java @@ -25,7 +25,7 @@ public class MockPluginsService extends PluginsService { private static final Logger logger = LogManager.getLogger(MockPluginsService.class); - private List classpathPlugins; + private final List classpathPlugins; /** * Constructs a new PluginService @@ -58,7 +58,7 @@ public MockPluginsService(Settings settings, Environment environment, Collection if (logger.isTraceEnabled()) { logger.trace("plugin loaded from classpath [{}]", pluginInfo); } - pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin)); + pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin, pluginClass.getClassLoader(), ModuleLayer.boot())); } this.classpathPlugins = List.copyOf(pluginsLoaded); @@ -69,10 +69,6 @@ protected final List plugins() { return this.classpathPlugins; } - void setLoadedPlugins(List plugins) { - this.classpathPlugins = List.copyOf(plugins); - } - @Override public PluginsAndModules info() { return new PluginsAndModules( From 3fe50077ad2840130ecb986812add6819b8fd109 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 29 Jun 2022 17:02:46 -0400 Subject: [PATCH 7/8] Apply review feedback --- .../elasticsearch/plugins/PluginsService.java | 4 +- .../bootstrap/test-framework.policy | 2 - .../plugins/PluginsServiceTests.java | 123 ++++++++---------- .../plugins/spi/TestService.java | 13 ++ 4 files changed, 67 insertions(+), 75 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/plugins/spi/TestService.java diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index 50f950023f8a4..eaaa7810f1ce3 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -302,10 +302,10 @@ Iterator providersIterator(Class service, ClassLoader classLoader) { /** * SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers * from plugins/modules. - * - * 
+     * 

      * For example:
      *
+     * 
      * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class);
      * 

      * @param service A templated service class to look for providers in plugins
diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
index caa014c0c5059..8907c4d463d3a 100644
--- a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
+++ b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
@@ -125,6 +125,4 @@ grant codeBase "${codebase.netty-transport}" {
 grant {
   permission java.net.SocketPermission "127.0.0.1", "connect, resolve";
   permission java.nio.file.LinkPermission "symbolic";
-  // needed for PluginsServiceTests
-  permission java.lang.RuntimePermission "createClassLoader";
 };
diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
index c82983d83b1f2..b440923b5f723 100644
--- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
+++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
@@ -12,19 +12,21 @@
 import org.apache.lucene.util.Constants;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.core.PathUtils;
 import org.elasticsearch.core.Strings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.TestEnvironment;
 import org.elasticsearch.index.IndexModule;
-import org.elasticsearch.operator.OperatorHandler;
-import org.elasticsearch.operator.OperatorHandlerProvider;
-import org.elasticsearch.operator.TransformState;
+import org.elasticsearch.plugins.spi.TestService;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.compiler.InMemoryJavaCompiler;
+import org.elasticsearch.test.jar.JarUtils;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.InvocationTargetException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.FileSystemException;
 import java.nio.file.Files;
 import java.nio.file.NoSuchFileException;
@@ -32,9 +34,10 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -44,9 +47,6 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.sameInstance;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.spy;
 
 @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
 public class PluginsServiceTests extends ESTestCase {
@@ -622,64 +622,64 @@ public void testThrowingConstructor() {
         assertThat(e.getCause().getCause(), hasToString(containsString("test constructor failure")));
     }
 
-    public void testLoadServiceProviders() throws ClassNotFoundException {
-        FakeClassLoader fakeClassLoader = new FakeClassLoader();
-        @SuppressWarnings("unchecked")
-        Class fakePluginClass = (Class) fakeClassLoader.findClass(FakePlugin.class.getName());
+    private ClassLoader buildTestProviderPlugin(String name) throws Exception {
+        String pluginClass = Strings.format("""
+            package r;
+            import org.elasticsearch.plugins.spi.TestService;
+            import org.elasticsearch.plugins.ActionPlugin;
+            import org.elasticsearch.plugins.Plugin;
+            public final class FooPlugin extends Plugin implements ActionPlugin, TestService {
+                @Override
+                public String name() {
+                    return "%s";
+                }
+            }
+            """, name);
 
-        FakeClassLoader fakeClassLoader1 = new FakeClassLoader();
-        @SuppressWarnings("unchecked")
-        Class fakePluginClass1 = (Class) fakeClassLoader1.findClass(FakePlugin.class.getName());
+        var classToBytes = InMemoryJavaCompiler.compile("r.FooPlugin", pluginClass);
 
-        assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader()));
+        Map jarEntries = new HashMap<>();
+        jarEntries.put("r/FooPlugin.class", classToBytes);
+        jarEntries.put("META-INF/services/org.elasticsearch.plugins.spi.TestService", "r.FooPlugin".getBytes(StandardCharsets.UTF_8));
 
-        OperatorHandlerProvider provider1 = () -> List.of(new OperatorHandler() {
-            @Override
-            public String name() {
-                return "integer";
-            }
+        Path topLevelDir = createTempDir(getTestName());
+        Path jar = topLevelDir.resolve(Strings.format("custom_plugin_%s.jar", name));
+        JarUtils.createJarWithEntries(jar, jarEntries);
+        URL[] urls = new URL[] { jar.toUri().toURL() };
 
-            @Override
-            public TransformState transform(Object source, TransformState prevState) {
-                return prevState;
-            }
-        });
+        URLClassLoader loader = URLClassLoader.newInstance(urls, this.getClass().getClassLoader());
+        return loader;
+    }
 
-        OperatorHandlerProvider provider2 = () -> List.of(new OperatorHandler() {
-            @Override
-            public String name() {
-                return "string";
-            }
+    public void testLoadServiceProviders() throws Exception {
+        ClassLoader fakeClassLoader = buildTestProviderPlugin("integer");
+        @SuppressWarnings("unchecked")
+        Class fakePluginClass = (Class) fakeClassLoader.loadClass("r.FooPlugin");
 
-            @Override
-            public TransformState transform(Object source, TransformState prevState) {
-                return prevState;
-            }
-        });
-        PluginsService service = spy(newMockPluginsService(List.of(fakePluginClass, fakePluginClass1)));
+        ClassLoader fakeClassLoader1 = buildTestProviderPlugin("string");
+        @SuppressWarnings("unchecked")
+        Class fakePluginClass1 = (Class) fakeClassLoader1.loadClass("r.FooPlugin");
 
-        doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
-        doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));
+        assertFalse(fakePluginClass.getClassLoader().equals(fakePluginClass1.getClassLoader()));
 
-        List providers = service.loadServiceProviders(OperatorHandlerProvider.class);
-        assertEquals(2, providers.size());
-        List handlers = providers.stream().map(p -> p.handlers()).flatMap(l -> l.stream()).map(h -> h.name()).toList();
+        getClass().getModule().addUses(TestService.class);
 
-        assertThat(handlers, containsInAnyOrder("string", "integer"));
+        PluginsService service = newMockPluginsService(List.of(fakePluginClass, fakePluginClass1));
 
-        doReturn(Collections.emptyList().iterator()).when(service)
-            .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
-        doReturn(List.of(provider2).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));
+        List providers = service.loadServiceProviders(TestService.class);
+        assertEquals(2, providers.size());
+        assertThat(providers.stream().map(p -> p.name()).toList(), containsInAnyOrder("string", "integer"));
 
-        assertEquals(1, service.loadServiceProviders(OperatorHandlerProvider.class).size());
+        service = newMockPluginsService(List.of(fakePluginClass));
+        providers = service.loadServiceProviders(TestService.class);
 
-        doReturn(Collections.emptyList().iterator()).when(service)
-            .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader));
-        doReturn(Collections.emptyList().iterator()).when(service)
-            .providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader1));
+        assertEquals(1, providers.size());
+        assertThat(providers.stream().map(p -> p.name()).toList(), containsInAnyOrder("integer"));
 
-        assertEquals(0, service.loadServiceProviders(OperatorHandlerProvider.class).size());
+        service = newMockPluginsService(new ArrayList<>());
+        providers = service.loadServiceProviders(TestService.class);
 
+        assertEquals(0, providers.size());
     }
 
     private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin {
@@ -721,23 +721,4 @@ public ThrowingConstructorExtension() {
             throw new IllegalArgumentException("test constructor failure");
         }
     }
-
-    static class FakeClassLoader extends ClassLoader {
-        @Override
-        public Class findClass(String name) throws ClassNotFoundException {
-            byte[] classBytes = fromFile(getClass().getClassLoader(), name);
-            return defineClass(name, classBytes, 0, classBytes.length);
-        }
-
-        private static byte[] fromFile(ClassLoader realLoader, String fileName) {
-            try {
-                InputStream input = realLoader.getResourceAsStream(
-                    Strings.format("%s.class", fileName.replace(".", PathUtils.get(".").getFileSystem().getSeparator()))
-                );
-                return input.readAllBytes();
-            } catch (Exception x) {
-                throw new IllegalStateException("Error loading class", x);
-            }
-        }
-    }
 }
diff --git a/server/src/test/java/org/elasticsearch/plugins/spi/TestService.java b/server/src/test/java/org/elasticsearch/plugins/spi/TestService.java
new file mode 100644
index 0000000000000..1156ebc1f809a
--- /dev/null
+++ b/server/src/test/java/org/elasticsearch/plugins/spi/TestService.java
@@ -0,0 +1,13 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.plugins.spi;
+
+public interface TestService {
+    String name();
+}

From e38c2fc395eb38ba7c2051ab585fe03012541db5 Mon Sep 17 00:00:00 2001
From: Nikola Grcevski 
Date: Wed, 29 Jun 2022 19:30:02 -0400
Subject: [PATCH 8/8] Split the provider class in the test

---
 .../elasticsearch/plugins/PluginsService.java  |  8 +-------
 .../plugins/PluginsServiceTests.java           | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
index eaaa7810f1ce3..058885de79f81 100644
--- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
+++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
@@ -47,7 +47,6 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Locale;
@@ -294,11 +293,6 @@ static void loadExtensions(Collection plugins) {
         }
     }
 
-    // package private for testing
-     Iterator providersIterator(Class service, ClassLoader classLoader) {
-        return ServiceLoader.load(service, classLoader).iterator();
-    }
-
     /**
      * SPI convenience method that uses the {@link ServiceLoader} JDK class to load various SPI providers
      * from plugins/modules.
@@ -315,7 +309,7 @@ public  List loadServiceProviders(Class service) {
         List result = new ArrayList<>();
 
         for (LoadedPlugin pluginTuple : plugins()) {
-            providersIterator(service, pluginTuple.loader()).forEachRemaining(c -> result.add(c));
+            ServiceLoader.load(service, pluginTuple.loader()).iterator().forEachRemaining(c -> result.add(c));
         }
 
         return Collections.unmodifiableList(result);
diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
index b440923b5f723..e9960f22ff3d1 100644
--- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
+++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
@@ -623,24 +623,28 @@ public void testThrowingConstructor() {
     }
 
     private ClassLoader buildTestProviderPlugin(String name) throws Exception {
-        String pluginClass = Strings.format("""
+        Map sources = Map.of("r.FooPlugin", """
             package r;
-            import org.elasticsearch.plugins.spi.TestService;
             import org.elasticsearch.plugins.ActionPlugin;
             import org.elasticsearch.plugins.Plugin;
-            public final class FooPlugin extends Plugin implements ActionPlugin, TestService {
+            public final class FooPlugin extends Plugin implements ActionPlugin { }
+            """, "r.FooTestService", Strings.format("""
+            package r;
+            import org.elasticsearch.plugins.spi.TestService;
+            public final class FooTestService implements TestService {
                 @Override
                 public String name() {
                     return "%s";
                 }
             }
-            """, name);
+            """, name));
 
-        var classToBytes = InMemoryJavaCompiler.compile("r.FooPlugin", pluginClass);
+        var classToBytes = InMemoryJavaCompiler.compile(sources);
 
         Map jarEntries = new HashMap<>();
-        jarEntries.put("r/FooPlugin.class", classToBytes);
-        jarEntries.put("META-INF/services/org.elasticsearch.plugins.spi.TestService", "r.FooPlugin".getBytes(StandardCharsets.UTF_8));
+        jarEntries.put("r/FooPlugin.class", classToBytes.get("r.FooPlugin"));
+        jarEntries.put("r/FooTestService.class", classToBytes.get("r.FooTestService"));
+        jarEntries.put("META-INF/services/org.elasticsearch.plugins.spi.TestService", "r.FooTestService".getBytes(StandardCharsets.UTF_8));
 
         Path topLevelDir = createTempDir(getTestName());
         Path jar = topLevelDir.resolve(Strings.format("custom_plugin_%s.jar", name));