Entitlements: manage_threads #122261

prdoyle · 2025-02-11T15:01:07Z

Adds a manage_threads entitlements covering Thread.start() as well as various set methods for changing thread properties.

See ES-10358.

This reverts commit 443ca76.

elasticsearchmachine · 2025-02-12T13:34:29Z

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

...entitled-plugin/src/main/java/org/elasticsearch/entitlement/qa/entitled/EntitledActions.java

…rties

...itlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java

…rties

…imple-thread-properties

rjernst

Looks pretty good, just a couple things I think should be split out for separate discussion.

...ntitlement/bridge/src/main/java/org/elasticsearch/entitlement/bridge/EntitlementChecker.java

...-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java

libs/entitlement/src/main/java/module-info.java

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java

ldematte

Looks good, just a couple of comments (nothing blocking I think)

ldematte · 2025-02-13T14:29:03Z

...-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java

        getTestEntries(FileCheckActions.class),
-        getTestEntries(SpiActions.class),
-        getTestEntries(SystemActions.class),
+        getTestEntries(FileStoreActions.class),


Thanks for ordering this; maybe mention it to everyone so we keep it ordered (and help in reducing merge conflicts)

ldematte · 2025-02-13T14:29:38Z

...-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java

        return channel -> {
            logger.info("Calling check action [{}]", actionName);
            checkAction.action().run();
+            logger.debug("Check action [{}] returned", actionName);


Intentional or leftover from debugging?

At one point, I wanted to know whether the action actually returned, as opposed to throwing. By and large, when I add log statements during debugging, I don't remove them before merging because if they're helpful once, they're likely to be helpful again.

👍
Should we change the "matching" logger level too? (2 lines above) (if it makes sense)

ldematte · 2025-02-13T14:32:04Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

        }

-        throw new NotEntitledException(
+        notEntitled(


I'm not the biggest fan of this change tbh; it does not make things more readable, and it adds a frame to the stack trace I think?
Not blocking, but I preferred the way it was.

I did this so the throws are all done from one place in order to switch them all to reporting only in this commit. That reporting mode was really useful; I suspect we'll end up making that "for real" somehow and merging it, but I didn't do that in this PR.

I should say... in isolation, with no context, I agree with you and probably would have left the same review comment. 😂

Yes! :) thanks for the context, the change makes sense now

ldematte · 2025-02-13T14:33:19Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java

        InboundNetworkEntitlement.class,
-        WriteSystemPropertiesEntitlement.class,
-        LoadNativeLibrariesEntitlement.class
+        LoadNativeLibrariesEntitlement.class,


Same; good that you ordered them, but please share with the team so we keep doing it.

rjernst

LGTM

elasticsearchmachine · 2025-02-13T18:47:20Z

💔 Backport failed

Status	Branch	Result
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts
✅	9.0

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122261

* Refactor: protected -> private * Initial thread-related entitlements * Entitlements from manual test runs * Refactor: notEntitled method * Entitlements reporting mode * Entitlements from CI * Revert "Entitlements reporting mode" This reverts commit 443ca76. * Remove unnecessary EntitledActions.newThread * Don't log in entitlements ITs by default * Import SuppressForbidden * Respond to PR comments * Move manage_threads tests to their own file

* Entitlements: manage_threads (#122261) * Refactor: protected -> private * Initial thread-related entitlements * Entitlements from manual test runs * Refactor: notEntitled method * Entitlements reporting mode * Entitlements from CI * Revert "Entitlements reporting mode" This reverts commit 443ca76. * Remove unnecessary EntitledActions.newThread * Don't log in entitlements ITs by default * Import SuppressForbidden * Respond to PR comments * Move manage_threads tests to their own file * Move ForkJoinPool.setParallelism to VersionSpecificManageThreadsActions * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

prdoyle added >non-issue :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 v9.1.0 labels Feb 11, 2025

prdoyle self-assigned this Feb 11, 2025

prdoyle force-pushed the simple-thread-properties branch from 53d2975 to 04b6e8e Compare February 12, 2025 12:01

prdoyle added 7 commits February 12, 2025 08:29

Refactor: protected -> private

13e0d54

Initial thread-related entitlements

a30fc17

Entitlements from manual test runs

14f70df

Refactor: notEntitled method

d0e428d

Entitlements reporting mode

7835c36

Entitlements from CI

d075cc3

Revert "Entitlements reporting mode"

99ab3d9

This reverts commit 443ca76.

prdoyle force-pushed the simple-thread-properties branch from 04b6e8e to 99ab3d9 Compare February 12, 2025 13:32

prdoyle marked this pull request as ready for review February 12, 2025 13:34

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 12, 2025

prdoyle commented Feb 12, 2025

View reviewed changes

...entitled-plugin/src/main/java/org/elasticsearch/entitlement/qa/entitled/EntitledActions.java Outdated Show resolved Hide resolved

prdoyle added 2 commits February 12, 2025 08:40

Remove unnecessary EntitledActions.newThread

7a4ba3c

Merge remote-tracking branch 'upstream/main' into simple-thread-prope…

0953768

…rties

prdoyle commented Feb 12, 2025

View reviewed changes

...itlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java Outdated Show resolved Hide resolved

prdoyle and others added 5 commits February 12, 2025 09:50

Don't log in entitlements ITs by default

bcd0183

Merge branch 'main' into simple-thread-properties

47fc38d

Merge remote-tracking branch 'upstream/main' into simple-thread-prope…

56aa68d

…rties

Merge remote-tracking branch 'origin/simple-thread-properties' into s…

0c1d79f

…imple-thread-properties

Import SuppressForbidden

f46110b

rjernst reviewed Feb 12, 2025

View reviewed changes

prdoyle added 2 commits February 13, 2025 08:38

Respond to PR comments

a2de8d2

Move manage_threads tests to their own file

ce9cce7

ldematte reviewed Feb 13, 2025

View reviewed changes

Merge branch 'main' into simple-thread-properties

92bbfe4

rjernst approved these changes Feb 13, 2025

View reviewed changes

prdoyle merged commit f8aa047 into elastic:main Feb 13, 2025
22 checks passed

prdoyle deleted the simple-thread-properties branch February 13, 2025 18:45

prdoyle mentioned this pull request Feb 13, 2025

[9.0] Entitlements: manage_threads (#122261) #122526

Merged

elasticsearchmachine added the backport pending label Feb 13, 2025

This was referenced Feb 13, 2025

Entitlements: manage_threads (#122261) #122530

Merged

Entitlements: manage_threads (#122261) #122533

Merged

prdoyle removed the backport pending label Apr 11, 2025

Entitlements: manage_threads #122261

Entitlements: manage_threads #122261

Uh oh!

Conversation

prdoyle commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2025

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prdoyle commented Feb 11, 2025 •

edited

Loading