Skip to content

Fully initialize policy checker before instrumenting #128703

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 6 commits into from
May 30, 2025
Merged

Fully initialize policy checker before instrumenting #128703

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77587a2
Fully initialize policy checker before instrumenting
rjernst May 30, 2025
cebe21f
add comment
rjernst May 30, 2025
f482ea8
[CI] Auto commit changes from spotless
May 30, 2025
8b8eece
fix test
rjernst May 30, 2025
b5c50ba
Merge branch 'main' into entitlements/checker_then_instrumentation
rjernst May 30, 2025
198077a
unmute another test
rjernst May 30, 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
14 changes: 10 additions & 4 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 @@ -63,7 +63,9 @@ public static EntitlementChecker checker() {
* @param inst the JVM instrumentation class instance
*/
public static void initialize(Instrumentation inst) throws Exception {
checker = initChecker(inst, createPolicyManager());
// the checker _MUST_ be set before _any_ instrumentation is done
checker = initChecker(createPolicyManager());
initInstrumentation(inst);
Copy link
Contributor

@prdoyle prdoyle May 30, 2025

Choose a reason for hiding this comment

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

Oh that is subtle. I had to look at it for a while.

Copy link
Contributor

@prdoyle prdoyle May 30, 2025

Choose a reason for hiding this comment

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

The way you've written this is much more clear and robust: initialize the checker, then fire up the instrumentation.

}

private static PolicyCheckerImpl createPolicyChecker(PolicyManager policyManager) {
Expand Down Expand Up @@ -115,7 +117,7 @@ private static void ensureClassesSensitiveToVerificationAreInitialized() {
}
}

static ElasticsearchEntitlementChecker initChecker(Instrumentation inst, PolicyManager policyManager) throws Exception {
static ElasticsearchEntitlementChecker initChecker(PolicyManager policyManager) {
final PolicyChecker policyChecker = createPolicyChecker(policyManager);
final Class<?> clazz = EntitlementCheckerUtils.getVersionSpecificCheckerClass(
ElasticsearchEntitlementChecker.class,
Expand All @@ -136,17 +138,21 @@ static ElasticsearchEntitlementChecker initChecker(Instrumentation inst, PolicyM
throw new AssertionError(e);
}


return checker;
}

static void initInstrumentation(Instrumentation instrumentation) throws Exception {
var verifyBytecode = Booleans.parseBoolean(System.getProperty("es.entitlements.verify_bytecode", "false"));
if (verifyBytecode) {
ensureClassesSensitiveToVerificationAreInitialized();
}

DynamicInstrumentation.initialize(
inst,
instrumentation,
EntitlementCheckerUtils.getVersionSpecificCheckerClass(EntitlementChecker.class, Runtime.version().feature()),
verifyBytecode
);

return checker;
}
}
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,9 +501,6 @@ tests:
- class: org.elasticsearch.xpack.esql.expression.function.scalar.string.WildcardLikeTests
method: testEvaluateInManyThreads {TestCase=100 random code points matches self case insensitive with text}
issue: https://github.com/elastic/elasticsearch/issues/128677
- class: org.elasticsearch.test.apmintegration.MetricsApmIT
Copy link
Contributor

@jdconrad jdconrad May 30, 2025

Choose a reason for hiding this comment

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

There's also TraceApmIT that needs to be unmuted.

method: testApmIntegration
issue: https://github.com/elastic/elasticsearch/issues/128678

# Examples:
#
Expand Down