Add TestEntitlementsRule and node grants for testing
#132077
Conversation
…n and management of entitlement related state during tests.
mosche
added
>refactoring
auto-backport
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
.../framework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Revoke all open node grants. | ||
| */ | ||
| public void revokeNodeGrants() { |
There was a problem hiding this comment.
I'm a bit surprised by this, but I see some explanation further down...
| // wipePendingDataDirectories in tests requires entitlement delegation to work as this uses server's FileSystemUtils. | ||
| // until ES-10920 is solved, node grants cannot be removed until the test suite completes unless explicitly removing all node | ||
| // grants using revokeNodeGrants where feasible. | ||
| // onClose.accept(this); |
There was a problem hiding this comment.
...here. Am I right that when we fix ES-10920 that the revokeNodeGrants can become private?
There was a problem hiding this comment.
Right, strictly speaking at that point we don't need this anymore. However, I'd rather keep it even then.
ESIntegTestCase can be configured to run with a TEST node scope, meaning each test case will use new nodes. revokeNodeGrants is a safety measure to ensure there's no node paths of nodes of a previous tests lingering around. The class rule only resets everything after the entire test suite.
test/framework/src/main/java/org/elasticsearch/entitlement/bootstrap/TestEntitlementsRule.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks great. I have one nit about the structure. I think the parallelism we had before with EntitlementBootstrap (which handles loading and managing the agent) and TestEntitlementBootstrap was good. Could we keep that, and have the test rule be simpler, just about calling the right methods on bootstrap?
| } | ||
|
|
||
| public static void initialize(Path tempDir) throws IOException { | ||
| if (POLICY_MANAGER != null) { |
There was a problem hiding this comment.
the tempDir is only ever java.io.tmpdir right? So couldn't this be known statically and not need this initialize method?
There was a problem hiding this comment.
I think the parallelism we had before with EntitlementBootstrap (which handles loading and managing the agent) and TestEntitlementBootstrap was good. Could we keep that, and have the test rule be simpler, just about calling the right methods on bootstrap?
Not 100% sure what you mean with above, @rjernst.TestEntitlementsRule#initialize(Path tempDir) corresponds to the previous TestEntitlementBootstrap#bootstrap(@Nullable Path tempDir) which also created the policy manager and loaded the agent. I've moved this into static init of this class to prevent this from not being initialized correctly (and making it more self contained). The test rule still contains / calls the very same code for creating the policy manager / loading the agent.
public static void bootstrap(@Nullable Path tempDir) throws IOException {
...
policyManager = createPolicyManager(pathLookup);
EntitlementInitialization.initializeArgs = new EntitlementInitialization.InitializeArgs(pathLookup, Set.of(), policyManager);
EntitlementBootstrap.loadAgent(EntitlementBootstrap.findAgentJar(), EntitlementInitialization.class.getName());
}
Are you suggesting to revert to above bootstrap method rather than doing that statically in TestEntitlementsRule? If so, I can absolutely do that.
There was a problem hiding this comment.
Sort of. I'm suggesting:
- Keep TestEntitlementBootstrap class, with that being the thing that manages bootstrapping the agent (note that the loadAgent methods et al can be package private)
- Continue calling the bootstrap method from BootstrapForTesting, which sets up the agent
- Have the TestRule call methods on bootstrap, the test rule is then just about calling the appropriate methods at the right time, with no inherent state itself
There was a problem hiding this comment.
Done, implemented here 4f5e939
| } | ||
|
|
||
| public static boolean isEnabledForTest() { | ||
| public static boolean isEnabledForTests() { | ||
| return Booleans.parseBoolean(System.getProperty("es.entitlement.enableForTests", "false")); |
There was a problem hiding this comment.
will this be enabled by default in a later change?
There was a problem hiding this comment.
it depends on the final shap of the gradle plugins, that's still an open task and pending on Rene's return.
for some esoteric modules (e.g. plugin-scanner )entitlements cannot be enabled and we currently rely on this defaulting to false in the test code.
…hs for testing (elastic#132077) Use TestEntitlementsRule and closable entitled node paths for better isolation and management of entitlement related state during tests. However, wipePendingDataDirectories in tests requires entitlement delegation to work as this uses server's FileSystemUtils. Until ES-10920 is solved, entitled node paths cannot be removed unless the test suite completes or explicitly removing all entitled node paths in-between tests (if cluster scope is TEST). Relates to ES-12042
💔 Backport failed
You can use sqren/backport to manually backport by running |
…hs for testing (#132077) (#132431) Use TestEntitlementsRule and closable entitled node paths for better isolation and management of entitlement related state during tests. However, wipePendingDataDirectories in tests requires entitlement delegation to work as this uses server's FileSystemUtils. Until ES-10920 is solved, entitled node paths cannot be removed unless the test suite completes or explicitly removing all entitled node paths in-between tests (if cluster scope is TEST). Relates to ES-12042
…cking * upstream/main: (26 commits) [Fleet] add privileges to `kibana_system` to read integrations data (elastic#132400) Add `TestEntitlementsRule` with support for dynamic entitled node paths for testing (elastic#132077) Reduce logging frequency for GCS per project clients (elastic#132429) Skip update/100_synthetic_source tests in yamlRestCompatTests (elastic#132296) Correct exception for missing nested path (elastic#132408) Fixing esql release tests elastic#132369 (elastic#132406) Adjust date docvalue formatting to return 4xx instead of 5xx (elastic#132414) Handle nested fields with the termvectors REST API in artificial docs (elastic#92568) Only collect bulk scored vectors when exceeding min competitive (elastic#132293) Fix release tests diskbbq update (elastic#132405) ESQL: Fix skipping of generative tests (elastic#132390) Short circuit failure handling in OIDC flow (elastic#130618) Small optimization in OptimizedScalarQuantizer by using mul instead of div (elastic#132397) Aggs: Add validation to Bucket script pipeline agg (elastic#132320) ESQL: Multiple parameters in ungrouped aggs (elastic#132375) ESQL: Explain test operators (elastic#132374) EQL: Deal with internally created IN in a different way for EQL (elastic#132167) Speed up hierarchical k-means by computing distances in bulk (elastic#132384) Reduce the number of fields per document (elastic#132322) Assert current thread in ESQL (elastic#132324) ...
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…hs for testing (elastic#132077) Use TestEntitlementsRule and closable entitled node paths for better isolation and management of entitlement related state during tests. However, wipePendingDataDirectories in tests requires entitlement delegation to work as this uses server's FileSystemUtils. Until ES-10920 is solved, entitled node paths cannot be removed unless the test suite completes or explicitly removing all entitled node paths in-between tests (if cluster scope is TEST). Relates to ES-12042 (cherry picked from commit 76dac08)
…hs for testing (#132077) (#132636) Use TestEntitlementsRule and closable entitled node paths for better isolation and management of entitlement related state during tests. However, wipePendingDataDirectories in tests requires entitlement delegation to work as this uses server's FileSystemUtils. Until ES-10920 is solved, entitled node paths cannot be removed unless the test suite completes or explicitly removing all entitled node paths in-between tests (if cluster scope is TEST). Relates to ES-12042 (cherry picked from commit 76dac08)
Use
TestEntitlementsRuleand closable node grants for better isolation and management of entitlement related state during tests.wipePendingDataDirectoriesin tests requires entitlement delegation to work as this uses server's FileSystemUtils.Until ES-10920 is solved, node grants cannot be removed unless the test suite completes or explicitly removing all node
grants using
revokeNodeGrantswhere feasible.Relates to ES-12042