Local index resolution records resolved expressions#135081
Local index resolution records resolved expressions#135081elasticsearchmachine merged 34 commits intoelastic:mainfrom
Conversation
|
@elasticmachine update branch plz |
|
There are no new commits on the base branch. |
…v0lg/elasticsearch into record-local-index-resolution-results
| public List<String> resolveIndexAbstractions( | ||
| Iterable<String> indices, | ||
| public ResolvedIndexExpressions resolveIndexAbstractions( | ||
| List<String> indices, |
There was a problem hiding this comment.
It's always a list in our codebase so I don't see a good reason to use the overly broad Iterable interface.
| } else if (indicesOptions.ignoreUnavailable() == false || isAuthorized.test(indexAbstraction, selector)) { | ||
| resolvedExpressionsBuilder.excludeAll(resolvedIndices); | ||
| } else { | ||
| boolean authorized = isAuthorized.test(indexAbstraction, selector); |
There was a problem hiding this comment.
This has a performance implication:
Previously, we would only call isAuthorized if ignoreUnavailable = true. Now, we always run the authorization check. Since it's a single resource check I think it's acceptable overhead to introduce.
We could avoid this overhead by:
- When ignoreUnavailable = false, mark the result as SUCCESS without checking
isAuthorizedand letting downstream authorization code throw, or - Using ResolvedIndexExpressions in downstream authorization code to skip the corresponding
isAuthorizedthere
My preference is to proceed without the above (or include them in a follow up) in favor of simplicity and intuitive behavior of resolveIndexAbstractions.
| this.auditTrailService = auditTrailService; | ||
| this.restrictedIndices = restrictedIndices; | ||
| this.indicesAndAliasesResolver = new IndicesAndAliasesResolver(settings, linkedProjectConfigService, resolver); | ||
| this.indicesAndAliasesResolver = new IndicesAndAliasesResolver(settings, linkedProjectConfigService, resolver, false); |
There was a problem hiding this comment.
For now, don't record resolved index expressions for any request. In the next PRs that bring in end-to-end CPS index expression handling, we'll turn index resolution recording on if CPS is enabled.
| assertThat( | ||
| actual.expressions(), | ||
| contains( | ||
| resolvedIndexExpression("-index10", Set.of("-index10"), SUCCESS), |
There was a problem hiding this comment.
:shudder: it turns out one can create indices prefixed with - in older versions of ES...
| */ | ||
| public record LocalExpressions( | ||
| List<String> expressions, | ||
| Set<String> expressions, |
There was a problem hiding this comment.
Converting this to a set for efficient exclusion handling (see here)
| * A collection of {@link ResolvedIndexExpression}. | ||
| */ | ||
| public record ResolvedIndexExpressions(Map<String, ResolvedIndexExpression> expressions) {} | ||
| public record ResolvedIndexExpressions(List<ResolvedIndexExpression> expressions) { |
There was a problem hiding this comment.
Converting to a list because of some edge-case behavior around duplicate index expressions:
See the two tests.
I'm fairly confident we can ultimately optimize this back to a map for efficient lookups but I want to change as little behavior as possible in the first round of changes so ensuring that duplicate index expressions are processed exactly as before (there may be some weird exclusion cases where we need the duplicates).
|
Pinging @elastic/es-security (Team:Security) |
This PR extends index resolution logic to capture the resolution mapping from original to resolution result via
ResolvedIndexExpressions(introduced in #134783). This lets consumers such as cross-project search that require full resolution context to access it via theIndicesRequest.Replaceableinterface.To maximize code re-use, the PR updates the existing
resolveIndexAbstractionsmethod to always return aResolvedIndexExpressionsinstance. The result is only recorded on the request if a boolean flag is set to avoid unnecessary memory overhead in contexts where storing the expressions is not necessary. In a (much bigger) future refactor, we plan to move away from storing a raw indices list and always useResolvedIndexExpressionsas the source of truth for indices resources accessed by a request. Once that's complete, we will move to always storing the full resolution result.This PR does not include the following, which we'll address in immediate follow ups:
_allindex expression handlingp:logs -> p1:logs)Relates: ES-12690