Entitlements for JDK-wide global state changes #119592
Conversation
prdoyle
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
prdoyle
force-pushed
the
moar-entitlements
branch
from
1f010d7 to
8219a91
Compare
| } | ||
|
|
||
| private record ParsedCheckerMethod( |
There was a problem hiding this comment.
My intention was to use this to implement this TODO. However, InstrumentationServiceImpl is not available at that point in the code, so we'd need to move this parsing logic to a common location.
Deferring that for a subsequent PR seemed like a good idea, but if the team wants me to do it here in this PR, I can.
Regardless, this refactoring made the code more clear anyway, I think. Parsing the name and signature are distinct "phases" with different dependencies (name parsing doesn't require the argument types) so it makes sense to move the name parsing into its own method.
There was a problem hiding this comment.
++ for leaving it for another PR.
I'm not sure if we'll need this refactoring in the end, but I'm OK with it.
|
|
||
| void check$java_lang_Runtime$removeShutdownHook(Class<?> callerClass, Runtime runtime, Thread hook); | ||
|
|
||
| void check$jdk_tools_jlink_internal_Jlink$(Class<?> callerClass); |
There was a problem hiding this comment.
I just realized I left some of these in place despite the fact that we have no tests for them because these classes are not accessible to our code.
I can delete these if we want, or leave them in place "just in case".
There was a problem hiding this comment.
I'm for keeping them "just in case" -- I thought maybe we can check if the jdk.jlink module is loaded at all, but we can't know which Java distribution we are running on, so...
| } | ||
|
|
||
| private record ParsedCheckerMethod( |
There was a problem hiding this comment.
++ for leaving it for another PR.
I'm not sure if we'll need this refactoring in the end, but I'm OK with it.
|
|
||
| void check$java_lang_Runtime$removeShutdownHook(Class<?> callerClass, Runtime runtime, Thread hook); | ||
|
|
||
| void check$jdk_tools_jlink_internal_Jlink$(Class<?> callerClass); |
There was a problem hiding this comment.
I'm for keeping them "just in case" -- I thought maybe we can check if the jdk.jlink module is loaded at all, but we can't know which Java distribution we are running on, so...
| * <p> | ||
| * A bit like Mockito but way more painful. | ||
| */ | ||
| class DummyImplementations { |
There was a problem hiding this comment.
I like the "Dummy" prefix, but I used "Test" in previous code (e.g. TestSSLSocketFactory), as it looked more consistent with the rest of the codebase.
Happy to go either way, but we should decide on one style and use that.
There was a problem hiding this comment.
I went with "Dummy" and renamed your classes.
| @@ -32,7 +32,7 @@ public class EntitlementsDeniedIT extends ESRestTestCase { | |||
| .systemProperty("es.entitlements.enabled", "true") | |||
| .setting("xpack.security.enabled", "false") | |||
| // Logs in libs/entitlement/qa/build/test-results/javaRestTest/TEST-org.elasticsearch.entitlement.qa.EntitlementsDeniedIT.xml | |||
| .setting("logger.org.elasticsearch.entitlement", "TRACE") | |||
| // .setting("logger.org.elasticsearch.entitlement", "DEBUG") | |||
There was a problem hiding this comment.
Is this intentional or a leftover from your own testing/debugging?
There was a problem hiding this comment.
Leaving TRACE enabled was definitely unintentional; commenting this out is an improvement. 😅
Whether it should be there at all is another question. I find it really convenient and hard to remember, but if you don't like this, I can move it out to my own documentation instead.
💚 Backport successful
|
* Refactor: separate check method name vs signature parsing * Cosmetic: change checker comment format * Entitlements for JDK-wide global state * [CI] Auto commit changes from spotless * Comment explaining entitlement add-exports * @SuppressForbidden * Refactor: rename dummy subclases --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Refactor: separate check method name vs signature parsing * Cosmetic: change checker comment format * Entitlements for JDK-wide global state * [CI] Auto commit changes from spotless * Comment explaining entitlement add-exports * @SuppressForbidden * Refactor: rename dummy subclases --------- Co-authored-by: elasticsearchmachine <[email protected]>
These entitlements are always denied.
Implements ES-10357.