Skip to content

[Entitlements] Fix EntitlementInitialization to work across multiple versions #121132

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

Closed
wants to merge 2 commits into from
Closed

[Entitlements] Fix EntitlementInitialization to work across multiple versions #121132

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ed4932
Fix EntitlementInitialization to work across multiple versions
ldematte Jan 29, 2025
7670d06
[CI] Auto commit changes from spotless
Jan 29, 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
56 changes: 40 additions & 16 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 @@ -32,6 +32,8 @@
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -48,6 +50,7 @@ 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 final int MINIMUM_SUPPORTED_JAVA_VERSION = 21;
Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

@rjernst do we have a better way than hardcoding this here? E.g. some other place where we already have 21 as a minimum supported version?
Also, are we enforcing it, or we allow ES to start even with lower versions? Because if that's the case, we need to change the logic below to always include at least org.elasticsearch.entitlement.bridge.EntitlementChecker, or ES will fail to start on the selfTest().

Copy link
Contributor

@mosche mosche Jan 29, 2025

Choose a reason for hiding this comment

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

There's JavaVersionChecker to enforce the minimum version, @ldematte

Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

Thank you @mosche! TIL
Probably it's OK to have 21 hardcoded here, it's not going to change. I'd hate to add a depdendency just for this :D Wdyt?

Copy link
Contributor

@mosche mosche Jan 29, 2025

Choose a reason for hiding this comment

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

yes, agreed, we can't depend on this here 💯


private static ElasticsearchEntitlementChecker manager;

Expand All @@ -60,11 +63,24 @@ public static EntitlementChecker checker() {
public static void initialize(Instrumentation inst) throws Exception {
manager = initChecker();

Map<MethodKey, CheckMethod> checkMethods = INSTRUMENTER_FACTORY.lookupMethods(EntitlementChecker.class);
Map<MethodKey, CheckMethod> checkMethods = new HashMap<>();
int javaVersion = Runtime.version().feature();
Set<Class<?>> interfaces = new HashSet<>();
for (int i = MINIMUM_SUPPORTED_JAVA_VERSION; i <= javaVersion; ++i) {
interfaces.add(getVersionSpecificCheckerClass(i, "org.elasticsearch.entitlement.bridge", "EntitlementChecker"));
Copy link
Contributor

@prdoyle prdoyle Jan 29, 2025

Choose a reason for hiding this comment

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

We could derive the package name programmatically from EntitlementChecker.class.getPackageName().

}
for (var checkerInterface : interfaces) {
checkMethods.putAll(INSTRUMENTER_FACTORY.lookupMethods(checkerInterface));
Copy link
Contributor

@mosche mosche Jan 29, 2025

Choose a reason for hiding this comment

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

why do we need the lookup on each of the prior interfaces? Isn't the latest sufficient? Especially if using this pattern:

public interface Java23EntitlementChecker extends Java22EntitlementChecker {}

Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

We examine the class for check$ methods using ASM, which means looking at things class-by-class (it's like looking at the files and reading the methods in the file).
An alternative would be to move the lookup on the ASM side, recursively finding and visiting the base interfaces; that would complicate the ASM code, but might be cleaner.

Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

It would also cover the case in which we want to "skip" a version, e.g. because we don't want/need to instrment these methods anymore:

public interface Java23EntitlementChecker extends Java21EntitlementChecker {}

(suppose Java22 just contains some preview methods that becomes official in 23, but with a different signature)

Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

I'm kind of convincing myself that doing this on the ASM side will be a bit more complicated code-wise, but better; wdyt?

Copy link
Contributor

@prdoyle prdoyle Jan 29, 2025

Choose a reason for hiding this comment

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

How would Moritz's inheritance idea work if a method were removed?

Copy link
Contributor Author

@ldematte ldematte Jan 29, 2025

Choose a reason for hiding this comment

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

Short answer: the same as this, it does not solve the problem.

Longer answer: it helps in some limited cases/if you structure your interfaces correctly.
Suppose we have a preview feature in Java 22:

public interface EntitlementChecker {}

public interface Java22EntitlementChecker extends EntitlementChecker {
   void check$NewClass$newPreviewMethod(...);
}

Then, the API is finalized in 23 but changes signature wrt 22. We can "skip" Java22EntitlementChecker

public interface Java23EntitlementChecker extends EntitlementChecker {
   void check$NewClass$newMethod(...); 
}

And/or separate interfaces, which is possible to do with inheritance:

interface Java22PreviewMethods {
   void check$NewClass$newPreviewMethod(...);
}

public interface Java22StableMethods extends EntitlementChecker {}

public interface Java22EntitlementChecker extends Java22StableMethods, Java22PreviewMethods {}

public interface Java23EntitlementChecker extends EntitlementChecker, Java22StableMethods {
   void check$NewClass$newMethod(...); 
}

I'm not saying we should do that, it probably does not cover everything, and also would make managing these interface complicated ... I think it would look difficult very fast. It's a possibility though.

}

var latestCheckerInterface = getVersionSpecificCheckerClass(
javaVersion,
"org.elasticsearch.entitlement.bridge",
"EntitlementChecker"
);
var classesToTransform = checkMethods.keySet().stream().map(MethodKey::className).collect(Collectors.toSet());

Instrumenter instrumenter = INSTRUMENTER_FACTORY.newInstrumenter(EntitlementChecker.class, checkMethods);
Instrumenter instrumenter = INSTRUMENTER_FACTORY.newInstrumenter(latestCheckerInterface, checkMethods);
inst.addTransformer(new Transformer(instrumenter, classesToTransform), true);
inst.retransformClasses(findClassesToRetransform(inst.getAllLoadedClasses(), classesToTransform));
}
Expand Down Expand Up @@ -114,20 +130,11 @@ private static PolicyManager createPolicyManager() {
private static ElasticsearchEntitlementChecker initChecker() {
final PolicyManager policyManager = createPolicyManager();

int javaVersion = Runtime.version().feature();
final String classNamePrefix;
if (javaVersion >= 23) {
classNamePrefix = "Java23";
} else {
classNamePrefix = "";
}
final String className = "org.elasticsearch.entitlement.runtime.api." + classNamePrefix + "ElasticsearchEntitlementChecker";
Class<?> clazz;
try {
clazz = Class.forName(className);
} catch (ClassNotFoundException e) {
throw new AssertionError("entitlement lib cannot find entitlement impl", e);
}
Class<?> clazz = getVersionSpecificCheckerClass(
Runtime.version().feature(),
"org.elasticsearch.entitlement.runtime.api",
"ElasticsearchEntitlementChecker"
);
Constructor<?> constructor;
try {
constructor = clazz.getConstructor(PolicyManager.class);
Expand All @@ -141,6 +148,23 @@ private static ElasticsearchEntitlementChecker initChecker() {
}
}

private static Class<?> getVersionSpecificCheckerClass(int javaVersion, String packageName, String baseClassName) {
final String classNamePrefix;
if (javaVersion >= 23) {
classNamePrefix = "Java23";
} else {
classNamePrefix = "";
}
Copy link
Contributor

@prdoyle prdoyle Jan 29, 2025

Choose a reason for hiding this comment

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

This prefix logic might need an explanatory comment. It looks a bit random at first glance.

final String className = packageName + "." + classNamePrefix + baseClassName;
Class<?> clazz;
try {
clazz = Class.forName(className);
} catch (ClassNotFoundException e) {
throw new AssertionError("entitlement lib cannot find entitlement class " + className, e);
}
return clazz;
}

private static final InstrumentationService INSTRUMENTER_FACTORY = new ProviderLocator<>(
"entitlement",
InstrumentationService.class,
Expand Down