Split out EntitlementsCache #127774
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| package org.elasticsearch.entitlement.initialization; | ||
|
|
||
| import org.elasticsearch.entitlement.runtime.policy.EntitlementsCache; | ||
|
|
||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Function; | ||
|
|
||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * The production {@link EntitlementsCache}. (Tests use {@code EntitlementCacheForTesting}.) | ||
| */ | ||
| final class EntitlementsCacheImpl extends ConcurrentHashMap<Module, EntitlementsCache.ModuleEntitlements> implements EntitlementsCache { | ||
| @Override | ||
| public ModuleEntitlements computeIfAbsent(Class<?> key, Function<? super Class<?>, ? extends ModuleEntitlements> mappingFunction) { | ||
| // We cache per module rather than per class to make the cache smaller and increase the hit ratio | ||
| return computeIfAbsent(key.getModule(), m -> requireNonNull(mappingFunction.apply(key))); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| package org.elasticsearch.entitlement.runtime.policy; | ||
|
|
||
| import org.elasticsearch.entitlement.runtime.policy.entitlements.Entitlement; | ||
| import org.elasticsearch.logging.Logger; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Stream; | ||
|
|
||
| /** | ||
| * Quickly provides entitlement info for a given class. Performance-critical. | ||
| */ | ||
| public interface EntitlementsCache { | ||
| /** | ||
| * @return true if entitlement checking should be bypassed for the given class; | ||
| * false if the normal built-in "trivially allowed" rules apply. | ||
| */ | ||
| default boolean isAlwaysAllowed(Class<?> key) { | ||
| return false; | ||
| } | ||
|
|
||
| ModuleEntitlements computeIfAbsent(Class<?> key, Function<? super Class<?>, ? extends ModuleEntitlements> mappingFunction); | ||
|
|
||
| /** | ||
| * This class contains all the entitlements by type, plus the {@link FileAccessTree} for the special case of filesystem entitlements. | ||
| * <p> | ||
| * We use layers when computing {@link ModuleEntitlements}; first, we check whether the module we are building it for is in the | ||
| * server layer ({@link PolicyManager#SERVER_LAYER_MODULES}) (*). | ||
| * If it is, we use the server policy, using the same caller class module name as the scope, and read the entitlements for that scope. | ||
| * Otherwise, we use the {@code PluginResolver} to identify the correct plugin layer and find the policy for it (if any). | ||
| * If the plugin is modular, we again use the same caller class module name as the scope, and read the entitlements for that scope. | ||
| * If it's not, we use the single {@code ALL-UNNAMED} scope – in this case there is one scope and all entitlements apply | ||
| * to all the plugin code. | ||
| * </p> | ||
| * <p> | ||
| * (*) implementation detail: this is currently done in an indirect way: we know the module is not in the system layer | ||
| * (otherwise the check would have been already trivially allowed), so we just check that the module is named, and it belongs to the | ||
| * boot {@link ModuleLayer}. We might want to change this in the future to make it more consistent/easier to maintain. | ||
| * </p> | ||
| * | ||
| * @param componentName the plugin name or else one of the special component names like "(server)". | ||
| */ | ||
| record ModuleEntitlements( | ||
| String componentName, | ||
| Map<Class<? extends Entitlement>, List<Entitlement>> entitlementsByType, | ||
| FileAccessTree fileAccess, | ||
| Logger logger | ||
| ) { | ||
|
|
||
| public ModuleEntitlements { | ||
| entitlementsByType = Map.copyOf(entitlementsByType); | ||
| } | ||
|
|
||
| public boolean hasEntitlement(Class<? extends Entitlement> entitlementClass) { | ||
| return entitlementsByType.containsKey(entitlementClass); | ||
| } | ||
|
|
||
| public <E extends Entitlement> Stream<E> getEntitlements(Class<E> entitlementClass) { | ||
| var entitlements = entitlementsByType.get(entitlementClass); | ||
| if (entitlements == null) { | ||
| return Stream.empty(); | ||
| } | ||
| return entitlements.stream().map(entitlementClass::cast); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| package org.elasticsearch.entitlement.runtime.policy; | ||
|
|
||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| /** | ||
| * When testing, we don't use modules, so we cache per-class. | ||
| */ | ||
| public class EntitlementsCacheForTesting extends ConcurrentHashMap<Class<?>, EntitlementsCache.ModuleEntitlements> | ||
| implements | ||
| EntitlementsCache { | ||
| @Override | ||
| public boolean isAlwaysAllowed(Class<?> key) { | ||
| return key.getPackageName().startsWith("org.gradle") || key.getPackageName().startsWith("org.junit"); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've described this as a cache, but it's not really. We happen to lazily populate it, but it's a system of record: what are the entitlements available for a given class. The fact we lazily populate it is an implementation detail. Can we make this more of a lookup? So rather than "computeIfAbsent", just lookup for a given class. How that get's populated is an implementation detail, in prod we will use a lazily populated map, in tests a map that we clear between tests (maybe).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be nice. I initially avoided the name "cache" but I think we still want a lazily-initialized map of modules that have default entitlements, since the returned
ModuleEntitlementsobject is module-specific. So I went with "cache" after all.Definitely open to improvements here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The picture I'm getting here is that you want this new swappable class to do two things:
IsTriviallyAllowedgetEntitlementsThe testing and production versions of this can share code to whatever extent that makes sense.
Is that what you're picturing?
Let me capture some thoughts on this...
In the current scheme,
getEntitlementsoccurs in two steps:#1 is different in production vs testing, and #2 is not, which is why I made an effort to leave #2 inside
PolicyManager.Similarly,
isTriviallyAllowedhas two parts:My first try at this refactoring was attempting to make these distinctions as clear as possible, so it was obvious what's the same vs what differs. I thought folks diving into this in the future might want to know when they're in the "mock world" where things are potentially less realistic.
If we simply make
isTriviallyAllowedandgetEntitlementsswappable, then that distinction is not as clear, though the code probably ends up simpler.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO wholly replacing these methods makes the code more clear. Having the implementation split across a pluggable thing and PolicyManager makes it more difficult to know where to look. eg by having a clear place where isTriviallyAllowed is implemented (on this interface) a dev can locate the code to walk through easier. That isn't to say isTriviallyAllowed shouldn't be split, we certainly don't want to duplicate production checks in tests, but eg having the test impl of this interface extend the production one (or even just having the prod class be the base and the test variant extends it) and then calling super would make things more clear, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean "make the code more clear" in the abstract. I meant specifically clarifying the distinction between what's the same versus what's different in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, as clean as it might appear to swap out these two methods for testing, that does not align with our understanding of what's actually being swapped out. For testing, we do not want wholly novel implementations of these two methods, because they will be mostly the same between prod and test.
The requirements are:
The code should look like what I just wrote, or else it's not clearly describing our understanding of the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should be clear what additions/changes the test code is doing relative to production. What you described can be done through inheritance of the production class and calling super, right?