Handle providers of optional services in ubermodule classloader (#91217)

* When scanning a jar bundle for services to add to the
ubermodule descriptor, we must make sure that any optional
dependencies match up to services that are defined in the
code. If we naively declare that our module uses any service
that is implemented by one of its jars, we might fail to load
the plugin because the original implementation is not included
in our bundle.

Here we add a fairly detailed test scenario for required and
optional services defined outside the bundle, as well as for
services defined within the bundle, either by module declaration
or by META-INF/services entry.

Author: williamrandolph

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

@@ -45,27 +45,41 @@ private ModuleSupport() {
         throw new AssertionError("Utility class, should not be instantiated");
     }
 
-    static ModuleFinder ofSyntheticPluginModule(String name, Path[] jarPaths, Set<String> requires, Set<String> uses) {
+    static ModuleFinder ofSyntheticPluginModule(
+        String name,
+        Path[] jarPaths,
+        Set<String> requires,
+        Set<String> uses,
+        Predicate<String> isPackageInParentLayers
+    ) {
         try {
             return new InMemoryModuleFinder(
-                new InMemoryModuleReference(createModuleDescriptor(name, jarPaths, requires, uses), URI.create("module:/" + name))
+                new InMemoryModuleReference(
+                    createModuleDescriptor(name, jarPaths, requires, uses, isPackageInParentLayers),
+                    URI.create("module:/" + name)
+                )
             );
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }
     }
 
     @SuppressForbidden(reason = "need access to the jar file")
-    static ModuleDescriptor createModuleDescriptor(String name, Path[] jarPaths, Set<String> requires, Set<String> uses)
-        throws IOException {
+    static ModuleDescriptor createModuleDescriptor(
+        String name,
+        Path[] jarPaths,
+        Set<String> requires,
+        Set<String> uses,
+        Predicate<String> isPackageInParentLayers
+    ) throws IOException {
         var builder = ModuleDescriptor.newOpenModule(name); // open module, for now
         requires.stream().forEach(builder::requires);
         uses.stream().forEach(builder::uses);
 
         // scan the names of the entries in the JARs
         Set<String> pkgs = new HashSet<>();
         Map<String, List<String>> allBundledProviders = new HashMap<>();
-        Set<String> allBundledServices = new HashSet<>();
+        Set<String> servicesUsedInBundle = new HashSet<>();
         for (Path path : jarPaths) {
             assert path.getFileName().toString().endsWith(".jar") : "expected jars suffix, in path: " + path;
             try (JarFile jf = new JarFile(path.toFile(), true, ZipFile.OPEN_READ, Runtime.version())) {
@@ -74,13 +88,13 @@ static ModuleDescriptor createModuleDescriptor(String name, Path[] jarPaths, Set
                 if (moduleInfo != null) {
                     var descriptor = getDescriptorForModularJar(path);
                     pkgs.addAll(descriptor.packages());
-                    allBundledServices.addAll(descriptor.uses());
+                    servicesUsedInBundle.addAll(descriptor.uses());
                     for (ModuleDescriptor.Provides p : descriptor.provides()) {
                         String serviceName = p.service();
                         List<String> providersInModule = p.providers();
 
                         allBundledProviders.compute(serviceName, (k, v) -> createListOrAppend(v, providersInModule));
-                        allBundledServices.add(serviceName);
+                        servicesUsedInBundle.add(serviceName);
                     }
                 } else {
                     var scan = scan(jf);
@@ -92,21 +106,29 @@ static ModuleDescriptor createModuleDescriptor(String name, Path[] jarPaths, Set
                         List<String> providersInJar = getProvidersFromServiceFile(jf, serviceFileName);
 
                         allBundledProviders.compute(serviceName, (k, v) -> createListOrAppend(v, providersInJar));
-                        allBundledServices.add(serviceName);
+                        servicesUsedInBundle.add(serviceName);
                     }
                 }
             }
         }
 
         builder.packages(pkgs);
 
-        // the module needs to use all services it provides, for the case of internal use
-        allBundledServices.addAll(allBundledProviders.keySet());
-        // but we don't want to add any services we already got from the parent layer
-        allBundledServices.removeAll(uses);
+        // we don't want to add any services we already got from the parent layer
+        servicesUsedInBundle.removeAll(uses);
 
-        allBundledServices.forEach(builder::uses);
-        allBundledProviders.forEach(builder::provides);
+        // Services that aren't exported in the parent layer or defined in our
+        // bundle. This can happen for optional (compile-time) dependencies
+        Set<String> missingServices = servicesUsedInBundle.stream()
+            .filter(s -> isPackageInParentLayers.test(toPackageName(s, ".").orElseThrow()) == false)
+            .filter(s -> pkgs.contains(toPackageName(s, ".").orElseThrow()) == false)
+            .collect(Collectors.toSet());
+
+        servicesUsedInBundle.stream().filter(s -> missingServices.contains(s) == false).forEach(builder::uses);
+        allBundledProviders.entrySet()
+            .stream()
+            .filter(e -> missingServices.contains(e.getKey()) == false)
+            .forEach(e -> builder.provides(e.getKey(), e.getValue()));
         return builder.build();
     }
 
@@ -246,7 +268,7 @@ static boolean isJavaPlatformModule(ModuleDescriptor md) {
     @SuppressForbidden(reason = "need access to the jar file")
     private static List<String> getProvidersFromServiceFile(JarFile jf, String sf) throws IOException {
         try (BufferedReader bf = new BufferedReader(new InputStreamReader(jf.getInputStream(jf.getEntry(sf)), StandardCharsets.UTF_8))) {
-            return bf.lines().toList();
+            return bf.lines().filter(Predicate.not(l -> l.startsWith("#"))).filter(Predicate.not(String::isEmpty)).toList();
         }
     }
 

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

@@ -57,21 +57,16 @@ public class UberModuleClassLoader extends SecureClassLoader implements AutoClos
     private final ModuleLayer.Controller moduleController;
     private final Set<String> packageNames;
 
-    private static final Map<String, Set<String>> platformModulesToServices;
-
-    static {
-        Set<String> unqualifiedExports = ModuleLayer.boot()
-            .modules()
+    private static Map<String, Set<String>> getModuleToServiceMap(ModuleLayer moduleLayer) {
+        Set<String> unqualifiedExports = moduleLayer.modules()
             .stream()
             .flatMap(module -> module.getDescriptor().exports().stream())
             .filter(Predicate.not(ModuleDescriptor.Exports::isQualified))
             .map(ModuleDescriptor.Exports::source)
             .collect(Collectors.toSet());
-        platformModulesToServices = ModuleLayer.boot()
-            .modules()
+        return moduleLayer.modules()
             .stream()
             .map(Module::getDescriptor)
-            .filter(ModuleSupport::isJavaPlatformModule)
             .filter(ModuleSupport::hasAtLeastOneUnqualifiedExport)
             .collect(
                 Collectors.toMap(
@@ -86,27 +81,38 @@ public class UberModuleClassLoader extends SecureClassLoader implements AutoClos
     }
 
     static UberModuleClassLoader getInstance(ClassLoader parent, String moduleName, Set<URL> jarUrls) {
-        return getInstance(parent, moduleName, jarUrls, Set.of());
+        return getInstance(parent, ModuleLayer.boot(), moduleName, jarUrls, Set.of());
     }
 
     @SuppressWarnings("removal")
-    static UberModuleClassLoader getInstance(ClassLoader parent, String moduleName, Set<URL> jarUrls, Set<String> moduleDenyList) {
+    static UberModuleClassLoader getInstance(
+        ClassLoader parent,
+        ModuleLayer parentLayer,
+        String moduleName,
+        Set<URL> jarUrls,
+        Set<String> moduleDenyList
+    ) {
         Path[] jarPaths = jarUrls.stream().map(UberModuleClassLoader::urlToPathUnchecked).toArray(Path[]::new);
-
-        Set<String> requires = platformModulesToServices.keySet()
+        var parentLayerModuleToServiceMap = getModuleToServiceMap(parentLayer);
+        Set<String> requires = parentLayerModuleToServiceMap.keySet()
             .stream()
             .filter(Predicate.not(moduleDenyList::contains))
             .collect(Collectors.toSet());
-        Set<String> uses = platformModulesToServices.entrySet()
+        Set<String> uses = parentLayerModuleToServiceMap.entrySet()
             .stream()
             .filter(Predicate.not(entry -> moduleDenyList.contains(entry.getKey())))
             .flatMap(entry -> entry.getValue().stream())
             .collect(Collectors.toSet());
 
-        ModuleFinder finder = ModuleSupport.ofSyntheticPluginModule(moduleName, jarPaths, requires, uses);
-        ModuleLayer mparent = ModuleLayer.boot();
+        ModuleFinder finder = ModuleSupport.ofSyntheticPluginModule(
+            moduleName,
+            jarPaths,
+            requires,
+            uses,
+            s -> isPackageInLayers(s, parentLayer)
+        );
         // TODO: check that denied modules are not brought as transitive dependencies (or switch to allow-list?)
-        Configuration cf = mparent.configuration().resolve(finder, ModuleFinder.of(), Set.of(moduleName));
+        Configuration cf = parentLayer.configuration().resolve(finder, ModuleFinder.of(), Set.of(moduleName));
 
         Set<String> packageNames = finder.find(moduleName).map(ModuleReference::descriptor).map(ModuleDescriptor::packages).orElseThrow();
 
@@ -115,12 +121,22 @@ static UberModuleClassLoader getInstance(ClassLoader parent, String moduleName,
             moduleName,
             jarUrls.toArray(new URL[0]),
             cf,
-            mparent,
+            parentLayer,
             packageNames
         );
         return AccessController.doPrivileged(pa);
     }
 
+    private static boolean isPackageInLayers(String packageName, ModuleLayer moduleLayer) {
+        if (moduleLayer.modules().stream().map(Module::getPackages).anyMatch(p -> p.contains(packageName))) {
+            return true;
+        }
+        if (moduleLayer.parents().equals(List.of(ModuleLayer.empty()))) {
+            return false;
+        }
+        return moduleLayer.parents().stream().anyMatch(ml -> isPackageInLayers(packageName, ml));
+    }
+
     /**
      * Constructor
      */