-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Cross-project search index expression rewriting #135346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…v0lg/elasticsearch into record-local-index-resolution-results
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Outdated
Show resolved
Hide resolved
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.
Authorization changes LGTM 👍
(left one question and suggestion which should not require another round of review)
// we will always store them | ||
if (recordResolvedIndexExpressions) { | ||
assert indicesRequest.indices() != null : "indices() cannot be null when resolving non-all-index expressions"; | ||
if (crossProjectModeDecider.resolvesCrossProject(replaceable) |
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.
Perhaps this entire block can go into a new class CrossProjectIndicesResolver
.
That class could then have the following methods, combining CrossProjectModeDecider
, CrossProjectIndexExpressionsRewriter
, and CrossProjectIndexResolutionValidator
into one coherent class:
class CrossProjectIndicesResolver {
boolean crossProjectEnabled() { ... }
boolean resolvesCrossProject(Indices.Replaceable request) { ... }
ResolvedIndices resolve(Indices.Replaceable request) { ... }
ElasticsearchException validate(
IndicesOptions indicesOptions,
ResolvedIndexExpressions localResolvedExpressions,
Map<String, ResolvedIndexExpressions> remoteResolvedExpressions
) { ... }
}
I'd rather do that refactor in a follow up though.
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 think this is somewhat related to @slobodanadamovic 's comment in other place about having IndicesAndAliasesResolver
to be pluggable for CPS. I can see this being a more packaged and self-contained solution. But yeah let's defer it for the time being.
Pinging @elastic/es-security (Team:Security) |
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.
LGTM
Thanks for the iterations!
server/src/main/java/org/elasticsearch/search/crossproject/TargetProjects.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java
Show resolved
Hide resolved
this.remoteClusterResolver = new RemoteClusterResolver(settings, linkedProjectConfigService); | ||
this.recordResolvedIndexExpressions = recordResolvedIndexExpressions; | ||
this.crossProjectModeDecider = crossProjectModeDecider; | ||
this.recordResolvedIndexExpressions = crossProjectModeDecider.crossProjectEnabled(); |
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.
Do we still need this field separately now that we have crossProjectModeDecider
?
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.
Good catch, no sorry we don't need it, just forgot to remove it
// we will always store them | ||
if (recordResolvedIndexExpressions) { | ||
assert indicesRequest.indices() != null : "indices() cannot be null when resolving non-all-index expressions"; | ||
if (crossProjectModeDecider.resolvesCrossProject(replaceable) |
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 think this is somewhat related to @slobodanadamovic 's comment in other place about having IndicesAndAliasesResolver
to be pluggable for CPS. I can see this being a more packaged and self-contained solution. But yeah let's defer it for the time being.
// we need an early return here, instead of relying on the outer none expression logic since the outer handling will | ||
// prematurely throw an IndexNotFound exception if the resolved indices are empty and allow_no_indices is false. | ||
if (resolvedIndicesBuilder.isEmpty()) { | ||
setNoneExpression(replaceable, resolvedIndicesBuilder); | ||
} else { | ||
replaceable.indices(resolvedIndicesBuilder.build().toArray()); | ||
} | ||
return resolvedIndicesBuilder.build(); |
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.
Do we still need this? It seems that we can have
if (indicesOptions.allowNoIndices() || crossProjectModeDecider.crossProjectEnabled())
at line 427 and it should work?
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.
Yes, but crossProjectModeDecider.resolvesCrossProject(replaceable)
instead -- otherwise, we don't correctly handle the case where CPS is enabled but we are handling a local request that did not get CPS resolved (e.g., because it does not allowCrossProject
).
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.
A few more comments while reviewing the serveless PR.
...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
Outdated
Show resolved
Hide resolved
// we still need to call response validation for local results, since qualified expressions like `_origin:index` or | ||
// `<alias-pattern-matching-origin-only>:index` get deferred validation, also |
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.
This sentense is not completed. Also, in this case, TargetProjects#linkedProjects
should be empty? If that's the case, we might be able to short circuit it in IndicesAndAliasesResolver
and avoid going into the application code. Something to look into in future.
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.
Ah it's just very poorly phrase -- ", also" should be ", too" or ", as well" -- let me wordsmith it a bit
If that's the case, we might be able to short circuit it in IndicesAndAliasesResolver and avoid going into the application code.
Yup -- very much something to look into as an optimization once more of the core pieces are in place
I'll log a Jira for it
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
|
||
public record TargetProjects(@Nullable ProjectRoutingInfo originProject, List<ProjectRoutingInfo> linkedProjects) { |
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 nullability confused me a bit. It might worth adding a comment to say (IIUC) that null
originProject is really a placeholder and only possible when a request is not eligible for CPS expansion after checking all three layers. It does not mean the user has no access to the origin project. In fact, we use a non-null ProjectRoutingInfo
for CPS request even when the user has no access to the origin project, the locally accessible indices are resolved separately.
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.
Discussed this sync: originProject
can be null if the project was excluded by project routing. I added Javadoc to explain this in more detail.
This PR implements cross-project search index resolution and ports the resolve index API to use it.
Relates: ES-12690