Split out EntitlementsCache #127774
Split out EntitlementsCache #127774
Conversation
prdoyle
added
>non-issue
auto-backport
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -843,6 +803,10 @@ private static boolean isTriviallyAllowed(Class<?> requestingClass) { | |||
| generalLogger.debug("Entitlement trivially allowed from system module [{}]", requestingClass.getModule().getName()); | |||
| return true; | |||
| } | |||
| if (moduleEntitlementsCache.isAlwaysAllowed(requestingClass)) { | |||
There was a problem hiding this comment.
My thinking was to move the logic in isTriviallyAllowed, so that the test infra can add "is this a test class". Otherwise this additional condition is only ever exercised in tests.
There was a problem hiding this comment.
Good point. A branch that's dead in production feels wrong.
There was a problem hiding this comment.
It's a little awkward because isTriviallyAllowed checks SYSTEM_LAYER_MODULES. PolicyManager seems like a good home for logic that's the same in production and unit tests.
Let me try ending it with an unconditional call to moduleEntitlementsCache and see if you like how that looks.
There was a problem hiding this comment.
Why can't SYSTEM_LAYER_MODULES be defined inside this lookup?
There was a problem hiding this comment.
It could I guess. That seemed like part of PolicyManager's responsibility, along with similar things like DEFAULT_FILESYSTEM_CLASS and MODULES_EXCLUDED_FROM_SYSTEM_MODULES, but yeah, it could go elsewhere.
| /** | ||
| * Quickly provides entitlement info for a given class. Performance-critical. | ||
| */ | ||
| public interface EntitlementsCache { |
There was a problem hiding this comment.
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.
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 ModuleEntitlements object is module-specific. So I went with "cache" after all.
Definitely open to improvements here.
There was a problem hiding this comment.
The picture I'm getting here is that you want this new swappable class to do two things:
IsTriviallyAllowedgetEntitlements
The 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, getEntitlements occurs in two steps:
- Requesting class -> PolicyScope
- PolicyScope -> ModuleEntitlements
#1 is different in production vs testing, and #2 is not, which is why I made an effort to leave #2 inside PolicyManager.
Similarly, isTriviallyAllowed has two parts:
- The actual trivially-allowed rules from production
- Additional allowances for the test framework
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 isTriviallyAllowed and getEntitlements swappable, then that distinction is not as clear, though the code probably ends up simpler.
There was a problem hiding this comment.
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.
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.
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:
- Each test should have its own file structure represented in the permissions, and if there are multiple nodes in the test, they should all have their files represented
- Tests require some additional classes to be trivially allowed
- Tests need to map Class -> Scope differently due to the lack of modules
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.
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?
|
The team consensus seems to be that we're not going with this approach. Continuing with #128004. |
Splits out the caching of entitlements information out of
PolicyManagerinto a separateEntitlementsCachecomponent. This allows us to have different behaviour for production and unit tests:isTriviallyAllowedto return true for test harness code like gradle and junit.Future work
isAlwaysAllowed. It currently just checks fororg.gradleandorg.junitpackages.