Skip to content

Conversation

@ldematte
Copy link
Contributor

This PR fixes an issue that become apparent with #121017: when we initialize Entitlements, and we get the list of methods to instrument, and instrument them, we need to get them from all the "versioned" interfaces compatible with the current runtime version. We also need to use the most specific (latest) "versioned" interface in the instrumentation code.

To do that, I cherry picked the changes to EntitlementInitialization in #121017, and adapted it to the current layout in main (where we just support Java >= 21)

@ldematte ldematte requested review from a team and rjernst January 29, 2025 08:18
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 29, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)


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

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

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

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

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 💯

interfaces.add(getVersionSpecificCheckerClass(i, "org.elasticsearch.entitlement.bridge", "EntitlementChecker"));
}
for (var checkerInterface : interfaces) {
checkMethods.putAll(INSTRUMENTER_FACTORY.lookupMethods(checkerInterface));
Copy link
Contributor

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

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

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

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

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

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.

classNamePrefix = "Java23";
} else {
classNamePrefix = "";
}
Copy link
Contributor

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.

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

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().

@ldematte
Copy link
Contributor Author

Closing in favor of #121192 (comments addressed there)

@ldematte ldematte closed this Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants