Skip to content

Refactor: ScopeResolver #126921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 21, 2025
Merged

Refactor: ScopeResolver #126921

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3dc5f2a
Fix: use getScopeName consistently
prdoyle Apr 14, 2025
db3f55f
Rename PolicyManagerTests method
prdoyle Apr 15, 2025
9fb36ac
Refacor: simplify PluginsResolver.create
prdoyle Apr 16, 2025
baf49a3
Change PluginsResolver to ScopeResolver
prdoyle Apr 16, 2025
04eb46b
Move boot layer test to ScopeResolverTests
prdoyle Apr 15, 2025
75291e6
[CI] Auto commit changes from spotless
Apr 16, 2025
99a3d75
Merge branch 'main' into scope-resolver
prdoyle Apr 16, 2025
7d17e07
Rename PolicyScope
prdoyle Apr 17, 2025
1871801
Merge branch 'main' into scope-resolver
prdoyle Apr 17, 2025
a3fd00b
Merge remote-tracking branch 'upstream/main' into scope-resolver
prdoyle Apr 21, 2025
986236a
Add ComponentKind enum
prdoyle Apr 21, 2025
39fe73a
Merge remote-tracking branch 'origin/scope-resolver' into scope-resolver
prdoyle Apr 21, 2025
76a433f
Merge remote-tracking branch 'upstream/main' into scope-resolver
prdoyle Apr 21, 2025
b175466
Package private componentName field
prdoyle Apr 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions ...titlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.entitlement.initialization.EntitlementInitialization;
import org.elasticsearch.entitlement.runtime.policy.Policy;
import org.elasticsearch.entitlement.runtime.policy.PolicyManager;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;

Expand All @@ -36,7 +37,7 @@ public class EntitlementBootstrap {
public record BootstrapArgs(
@Nullable Policy serverPolicyPatch,
Map<String, Policy> pluginPolicies,
Function<Class<?>, String> pluginResolver,
Function<Class<?>, PolicyManager.PolicyScope> scopeResolver,
Function<String, Stream<String>> settingResolver,
Path[] dataDirs,
Path[] sharedRepoDirs,
Expand All @@ -52,7 +53,7 @@ public record BootstrapArgs(
) {
public BootstrapArgs {
requireNonNull(pluginPolicies);
requireNonNull(pluginResolver);
requireNonNull(scopeResolver);
requireNonNull(settingResolver);
requireNonNull(dataDirs);
if (dataDirs.length == 0) {
Expand Down Expand Up @@ -82,7 +83,7 @@ public static BootstrapArgs bootstrapArgs() {
*
* @param serverPolicyPatch a policy with additional entitlements to patch the embedded server layer policy
* @param pluginPolicies a map holding policies for plugins (and modules), by plugin (or module) name.
* @param pluginResolver a functor to map a Java Class to the plugin it belongs to (the plugin name).
* @param scopeResolver a functor to map a Java Class to the component and module it belongs to.
* @param settingResolver a functor to resolve a setting name pattern for one or more Elasticsearch settings.
* @param dataDirs data directories for Elasticsearch
* @param sharedRepoDirs shared repository directories for Elasticsearch
Expand All @@ -99,7 +100,7 @@ public static BootstrapArgs bootstrapArgs() {
public static void bootstrap(
Policy serverPolicyPatch,
Map<String, Policy> pluginPolicies,
Function<Class<?>, String> pluginResolver,
Function<Class<?>, PolicyManager.PolicyScope> scopeResolver,
Function<String, Stream<String>> settingResolver,
Path[] dataDirs,
Path[] sharedRepoDirs,
Expand All @@ -120,7 +121,7 @@ public static void bootstrap(
EntitlementBootstrap.bootstrapArgs = new BootstrapArgs(
serverPolicyPatch,
pluginPolicies,
pluginResolver,
scopeResolver,
settingResolver,
dataDirs,
sharedRepoDirs,
Expand Down
4 changes: 1 addition & 3 deletions ...src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@
*/
public class EntitlementInitialization {

private static final String AGENTS_PACKAGE_NAME = "co.elastic.apm.agent";
private static final Module ENTITLEMENTS_MODULE = PolicyManager.class.getModule();

private static ElasticsearchEntitlementChecker manager;
Expand Down Expand Up @@ -319,9 +318,8 @@ private static PolicyManager createPolicyManager() {
serverPolicy,
agentEntitlements,
pluginPolicies,
EntitlementBootstrap.bootstrapArgs().pluginResolver(),
EntitlementBootstrap.bootstrapArgs().scopeResolver(),
EntitlementBootstrap.bootstrapArgs().sourcePaths(),
AGENTS_PACKAGE_NAME,
ENTITLEMENTS_MODULE,
pathLookup,
bootstrapArgs.suppressFailureLogClasses()
Expand Down
106 changes: 50 additions & 56 deletions ...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,26 @@ public class PolicyManager {
*/
private static final Logger generalLogger = LogManager.getLogger(PolicyManager.class);

static final String UNKNOWN_COMPONENT_NAME = "(unknown)";
static final String SERVER_COMPONENT_NAME = "(server)";
static final String APM_AGENT_COMPONENT_NAME = "(APM agent)";
public static final String UNKNOWN_COMPONENT_NAME = "(unknown)";
Copy link
Member

@rjernst rjernst Apr 17, 2025

Choose a reason for hiding this comment

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

Instead of exposing these strings, can we have constants defined in ScopeInfo for each of these?

Copy link
Contributor Author

@prdoyle prdoyle Apr 17, 2025

Choose a reason for hiding this comment

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

Oh like an enum? Yeah I'd much prefer that actually. I was considering it but didn't want to hold up the PR.

I was thinking of:

enum ComponentKind { UNKNOWN, SERVER, PLUGIN, AGENT }

Is that what you were picturing?

Copy link
Contributor Author

@prdoyle prdoyle Apr 17, 2025

Choose a reason for hiding this comment

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

If we didn't have to support JDK 17, we could go all the way to algebraic datatypes:

sealed interface PolicyScope {
  String component();
  String module();

  record Unknown() implements PolicyScope { ... };
  record Server(String module) implements PolicyScope { ... };
  record ApmAgent() implements PolicyScope { ... };
  record Plugin(String component, String module) {}
}

Copy link
Member

@rjernst rjernst Apr 17, 2025

Choose a reason for hiding this comment

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

what about Java 17 prohibits that? Still seems doable, even if with static methods?

Copy link
Contributor Author

@prdoyle prdoyle Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Consuming it is harder than the enum because you can't use switch patterns until Java 21.

Copy link
Member

@rjernst rjernst Apr 18, 2025

Choose a reason for hiding this comment

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

Where do we consume it in a switch? Don't we just use the two strings right now? I view the helpers as making construction if the scope more robust, and allowing us to keep some eg logging strings for server internal to entitlements.

Copy link
Contributor Author

@prdoyle prdoyle Apr 21, 2025

Choose a reason for hiding this comment

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

Here.

Copy link
Contributor Author

@prdoyle prdoyle Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've gone with an enum plus some factory method helpers.

public static final String SERVER_COMPONENT_NAME = "(server)";
public static final String APM_AGENT_COMPONENT_NAME = "(APM agent)";

static final Class<?> DEFAULT_FILESYSTEM_CLASS = PathUtils.getDefaultFileSystem().getClass();

static final Set<String> MODULES_EXCLUDED_FROM_SYSTEM_MODULES = Set.of("java.desktop");

/**
* Identifies a particular entitlement {@link Scope} within a {@link Policy}.
* @param componentName
* @param moduleName
*/
public record PolicyScope(String componentName, String moduleName) {
public PolicyScope {
requireNonNull(componentName);
requireNonNull(moduleName);
}
}

/**
* @param componentName the plugin name; or else one of the special component names
* like {@link #SERVER_COMPONENT_NAME} or {@link #APM_AGENT_COMPONENT_NAME}.
Expand Down Expand Up @@ -136,7 +148,7 @@ ModuleEntitlements policyEntitlements(String componentName, Path componentPath,
private final Map<String, List<Entitlement>> serverEntitlements;
private final List<Entitlement> apmAgentEntitlements;
private final Map<String, Map<String, List<Entitlement>>> pluginsEntitlements;
private final Function<Class<?>, String> pluginResolver;
private final Function<Class<?>, PolicyScope> scopeResolver;
private final PathLookup pathLookup;
private final Set<Class<?>> mutedClasses;

Expand Down Expand Up @@ -172,10 +184,6 @@ private static Set<Module> findSystemLayerModules() {
.collect(Collectors.toUnmodifiableSet());

private final Map<String, Path> sourcePaths;
/**
* The package name containing classes from the APM agent.
*/
private final String apmAgentPackageName;

/**
* Frames originating from this module are ignored in the permission logic.
Expand All @@ -193,9 +201,8 @@ public PolicyManager(
Policy serverPolicy,
List<Entitlement> apmAgentEntitlements,
Map<String, Policy> pluginPolicies,
Function<Class<?>, String> pluginResolver,
Function<Class<?>, PolicyScope> scopeResolver,
Map<String, Path> sourcePaths,
String apmAgentPackageName,
Module entitlementsModule,
PathLookup pathLookup,
Set<Class<?>> suppressFailureLogClasses
Expand All @@ -205,9 +212,8 @@ public PolicyManager(
this.pluginsEntitlements = requireNonNull(pluginPolicies).entrySet()
.stream()
.collect(toUnmodifiableMap(Map.Entry::getKey, e -> buildScopeEntitlementsMap(e.getValue())));
this.pluginResolver = pluginResolver;
this.scopeResolver = scopeResolver;
this.sourcePaths = sourcePaths;
this.apmAgentPackageName = apmAgentPackageName;
this.entitlementsModule = entitlementsModule;
this.pathLookup = requireNonNull(pathLookup);
this.mutedClasses = suppressFailureLogClasses;
Expand Down Expand Up @@ -609,50 +615,42 @@ ModuleEntitlements getEntitlements(Class<?> requestingClass) {
}

private ModuleEntitlements computeEntitlements(Class<?> requestingClass) {
Module requestingModule = requestingClass.getModule();
if (isServerModule(requestingModule)) {
return getModuleScopeEntitlements(
serverEntitlements,
requestingModule.getName(),
SERVER_COMPONENT_NAME,
getComponentPathFromClass(requestingClass)
);
}
var policyScope = scopeResolver.apply(requestingClass);
var componentName = policyScope.componentName();
var moduleName = policyScope.moduleName();

// plugins
var pluginName = pluginResolver.apply(requestingClass);
if (pluginName != null) {
var pluginEntitlements = pluginsEntitlements.get(pluginName);
if (pluginEntitlements == null) {
return defaultEntitlements(pluginName, sourcePaths.get(pluginName), requestingModule.getName());
} else {
switch (componentName) {
case SERVER_COMPONENT_NAME -> {
return getModuleScopeEntitlements(
pluginEntitlements,
getScopeName(requestingModule),
pluginName,
sourcePaths.get(pluginName)
serverEntitlements,
moduleName,
SERVER_COMPONENT_NAME,
getComponentPathFromClass(requestingClass)
);
}
}

if (requestingModule.isNamed() == false && requestingClass.getPackageName().startsWith(apmAgentPackageName)) {
// The APM agent is the only thing running non-modular in the system classloader
return policyEntitlements(
APM_AGENT_COMPONENT_NAME,
getComponentPathFromClass(requestingClass),
ALL_UNNAMED,
apmAgentEntitlements
);
}

return defaultEntitlements(UNKNOWN_COMPONENT_NAME, null, requestingModule.getName());
}

private static String getScopeName(Module requestingModule) {
if (requestingModule.isNamed() == false) {
return ALL_UNNAMED;
} else {
return requestingModule.getName();
case APM_AGENT_COMPONENT_NAME -> {
// The APM agent is the only thing running non-modular in the system classloader
return policyEntitlements(
APM_AGENT_COMPONENT_NAME,
getComponentPathFromClass(requestingClass),
ALL_UNNAMED,
apmAgentEntitlements
);
}
case UNKNOWN_COMPONENT_NAME -> {
return defaultEntitlements(UNKNOWN_COMPONENT_NAME, null, moduleName);
}
default -> {
// Must be a plugin
assert componentName.startsWith("(") == false
: "Parentheses indicate a special component name that isn't a plugin: " + componentName;
var pluginEntitlements = pluginsEntitlements.get(componentName);
if (pluginEntitlements == null) {
return defaultEntitlements(componentName, sourcePaths.get(componentName), moduleName);
} else {
return getModuleScopeEntitlements(pluginEntitlements, moduleName, componentName, sourcePaths.get(componentName));
}
}
}
}

Expand Down Expand Up @@ -688,10 +686,6 @@ private ModuleEntitlements getModuleScopeEntitlements(
return policyEntitlements(componentName, componentPath, scopeName, entitlements);
}

private static boolean isServerModule(Module requestingModule) {
return requestingModule.isNamed() && requestingModule.getLayer() == ModuleLayer.boot();
}

/**
* Walks the stack to determine which class should be checked for entitlements.
*
Expand Down
Loading