ES-12295 [S2D30] Painless/execute in CPS requires qualified index expression#138435
ES-12295 [S2D30] Painless/execute in CPS requires qualified index expression#138435alexey-ivanov-es merged 13 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| if (request.getContextSetup() == null || request.getContextSetup().getClusterAlias() == null) { | ||
| // By this point index resolution has completed, and we should not try to resolve indices for child requests | ||
| // to avoid the second attempt of project authorization in CPS | ||
| request.resolveCrossProject = false; |
There was a problem hiding this comment.
Do we have any canonical way to set IndicesOptions.resolveCrossProjectIndexExpression to false for child requests?
There was a problem hiding this comment.
maybe org.elasticsearch.search.crossproject.CrossProjectIndexResolutionValidator.indicesOptionsForCrossProjectFanout ?
I've just started to read your PR, so not sure that's the right method for your scenario, but maybe start there?
quux00
left a comment
There was a problem hiding this comment.
Didn't finish reviewing yet. I'll pick it up again Monday, but sending a few initial comments/questions for now.
server/src/main/java/org/elasticsearch/action/IndicesRequest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/crossproject/CrossProjectModeDecider.java
Outdated
Show resolved
Hide resolved
...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java
Outdated
Show resolved
Hide resolved
| return true; | ||
| } | ||
| String index = request.index(); | ||
| return index == null || RemoteClusterAware.isRemoteIndexName(index) == false; |
There was a problem hiding this comment.
Where do you handle _origin:myindex? Will it ("_origin") have been stripped off before this is called? Otherwise RemoteClusterAware.isRemoteIndexName("_origin:myindex) would return true right?
There was a problem hiding this comment.
The flow is:
- In
IndicesAndAliasesResolver, we check if the request isSingleIndexNoWildcardsandindicesOptions().resolveCrossProjectIndexExpression() - We detect if the index expression is local (no cluster prefix or has
_originor has origin alias prefix) - If local, we call
setLocal(true)which strips off the cluster alias from the index in the request
So the cluster alias stripping happens during the security action filter call. This differs from my original approach, but I think it's the best way since we have the origin alias available there but not when execute is called on the transport action.
...s/lang-painless/src/test/java/org/elasticsearch/painless/action/PainlessExecuteApiTests.java
Outdated
Show resolved
Hide resolved
ywangd
left a comment
There was a problem hiding this comment.
I have a few comments.
Also, ContextSetup is a bit odd in that it parses remote index synatx on its own and keeps them separately in clusterAlias and index fields. Then it still exposes the original expression via SingleShardRequest#index by combing them again. Is it really necessary? I think it is mainly responsible for the different methods of index, setIndex and setLocal that do almost the same thing. Can we look into simplify them?
server/src/main/java/org/elasticsearch/search/crossproject/CrossProjectModeDecider.java
Outdated
Show resolved
Hide resolved
...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
Show resolved
Hide resolved
| private ResolvedIndices resolveSingleIndexNoWildcardsCrossProject(TargetProjects authorizedProjects, String[] indices) { | ||
| assert authorizedProjects != TargetProjects.LOCAL_ONLY_FOR_CPS_DISABLED | ||
| : "resolving cross-project request but authorized project is local only"; | ||
| Map<String, List<String>> clusterIndices = remoteClusterResolver.groupProjectIndices(authorizedProjects, indices); | ||
|
|
||
| List<String> unqualifiedIndices = clusterIndices.remove(LOCAL_CLUSTER_GROUP_KEY); | ||
|
|
||
| List<String> local = new ArrayList<>(); | ||
| if (unqualifiedIndices != null) { | ||
| local.addAll(unqualifiedIndices); | ||
| } | ||
| List<String> originIndices = clusterIndices.remove(ProjectRoutingResolver.ORIGIN); | ||
| if (originIndices != null) { | ||
| local.addAll(originIndices); | ||
| } | ||
| List<String> originAliasIndices = clusterIndices.remove(authorizedProjects.originProjectAlias()); | ||
| if (originAliasIndices != null) { | ||
| local.addAll(originAliasIndices); | ||
| } | ||
| List<String> remote = clusterIndices.entrySet() | ||
| .stream() | ||
| .flatMap(e -> e.getValue().stream().map(v -> e.getKey() + RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR + v)) | ||
| .toList(); | ||
|
|
||
| return new ResolvedIndices(local, remote); | ||
| } |
There was a problem hiding this comment.
Can we tigthen this method so that it is obviously only a single index is supported?
- Instead
String[] indices, can we haveString indexas the parameter? - Do we really need
groupProjectIndices? I thinkRemoteClusterAware#splitIndexNameshould do that job since there is only one index?
There was a problem hiding this comment.
The API does not support wildcard in remote cluster/project name (doc). The current implementation throws NoSuchRemoteClusterException when it tries to get remote client. I think we can consider erroring out earlier here. It is rather odd that we resolve an expression such as *:index to all remote clusters/projects which seems both incorrect and unnecessary.
There was a problem hiding this comment.
This is already checked in this method:
There was a problem hiding this comment.
Can we tigthen this method so that it is obviously only a single index is supported?
Instead String[] indices, can we have String index as the parameter?
Do we really need groupProjectIndices? I think RemoteClusterAware#splitIndexName should do that job since there is only one index?
I wanted to keep CPS and CCS consistent, so I used the same approach we use in CCS but with a different set of cluster aliases.
I believe, we use RemoteClusterAware.groupClusterIndices because it supports cluster exclusion expressions. The original intention was probably to allow SingleIndexNoWildcards requests with complex index expressions that resolve to a single index.
However, Painless/execute only supports one index expression, and it's currently the only request implementing SingleIndexNoWildcards. So I agree we can simplify this to only allow a single index without cluster exclusion support for both CCS/CPS.
@quux00 wdyt?
There was a problem hiding this comment.
Painless/execute supports neither wildcards nor index expression exclusions:
"context_setup": {
"index": "*:blogs",
// results in error
"reason": "no such remote cluster: [*]"
"index": "-blogs",
// results in error:
"type": "index_not_found_exception",
"reason": "no such index [-blogs] and if you intended to exclude this index, ensure that you use wildcards that include it before explicitly excluding it",
And that last error message is actually misleading as you can't do that in painless/execute.
So that's an argument in favor of what Yang recommends. The tricky part is that if you switch to using RemoteClusterAware#splitIndexName you can't just blindly accept the project alias - it could be invalid (a wildcard, start with a - sign or reference a project that doesn't exist), so you'd need to have something check for it's validity. I haven't traced the code enough to know if that would happen downstream of this method call or if you need to detect that error here.
There was a problem hiding this comment.
I wanted to keep CPS and CCS consistent, so I used the same approach we use in CCS but with a different set of cluster aliases.
If we do change this for CSP, we can file a follow-on ticket to change CCS as well, so I wouldn't let consistency between them be a blocker when the CCS error messages here aren't that great.
There was a problem hiding this comment.
The tricky part is that if you switch to using RemoteClusterAware#splitIndexName you can't just blindly accept the project alias - it could be invalid (a wildcard, start with a - sign or reference a project that doesn't exist), so you'd need to have something check for it's validity. I haven't traced the code enough to know if that would happen downstream of this method call or if you need to detect that error here.
It shouldn't be a problem - there is TargetProjects authorizedProjects containing aliases of the linked projects, so I can check if the cluster alias valid. And for CCS I can use IndicesAndAliasesResolver.RemoteClusterResolver.clusters for the validation
| if (resolveCrossProject == false) { | ||
| return super.indicesOptions(); | ||
| } | ||
| return IndicesOptions.builder(super.indicesOptions()) | ||
| .crossProjectModeOptions(new IndicesOptions.CrossProjectModeOptions(true)) | ||
| .build(); |
There was a problem hiding this comment.
I don't think we want to use a transient volatile field to controle this. In other places, e.g. ResolveIndex, we construct the IndicesOptions with cross-project when CPS is enabled. Cross-project indices options is then disabled before sending a newly instantiated child request to the remote.
There was a problem hiding this comment.
I've updated the implementation, but still need a transient field here since the request doesn't have an existing field for storing indices options. I am not sure about volatile, but it's probably better to have it than to have some concurrent bugs
There was a problem hiding this comment.
Thanks for the update 👍
I don't think volatile is necessary since there is no concurrent access to the field, i.e. the request object is passed around, maybe cross threads, in a serial fashion. Same reason for it not being a volatile field in other request types, such as SearchRequest.
...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java
Outdated
Show resolved
Hide resolved
Just want to double confirm this is what we want. It means the flat-world model does NOT apply to this API, i.e. |
The problem with the current |
@ywangd - Yes, this is the agreed upon model with Product. We discussed several options, all of them bad. This option is the least-bad option based on Najwa's analysis of how Kibana uses this endpoint. I'll send you a link to the Slack discussion. |
...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java
Outdated
Show resolved
Hide resolved
Thanks for explaining. I see the issue now. The two methods still look a bit confusing to me. Can I suggest the followings:
That is, something like Request(Script script, String scriptContextName, ContextSetup setup) {
...
if (setup != null) {
...
if (contextSetup.getClusterAlias() == null) {
index(contextSetup.getIndex());
} else {
index(contextSetup.getClusterAlias() + RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR + contextSetup.getIndex());
}
} else { ... }
}
@Override
public void setLocal() {
index(contextSetup.getIndex());
} |
| /** | ||
| * Determines whether the request type allows cross-project processing. Cross-project processing entails cross-project search | ||
| * index resolution and error handling. Note: this method only determines in the request _supports_ cross-project. | ||
| * Whether cross-project processing is actually performed is determined by {@link IndicesOptions}. | ||
| */ | ||
| default boolean allowsCrossProject() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
We making change so that UIAM authentication and authorized project resolution are separated from CPS index resolution so that they can be available for more request types. Please refer to the relevant change for this file here. Thanks!
…ces options are set in PainlessExecuteAction.Request
quux00
left a comment
There was a problem hiding this comment.
This looks good to me. Thank you!
## Summary see title. Based on a Slack thread and elastic/elasticsearch#138435 See also the corresponding ES reference doc update in elastic/elasticsearch#144895 ## Screenshots of updated docs because the docs preview is temporarily unavailable expand for screenshots: <details> 1. **/explore-analyze/cross-project-search** current page: https://www.elastic.co/docs/explore-analyze/cross-project-search#cps-supported-apis addition: <img width="912" height="1102" alt="Screenshot 2026-03-24 at 6 57 55 PM" src="https://github.com/user-attachments/assets/ac351da9-2c88-4777-987b-77e649f8d582" /> 2. **/explore-analyze/cross-project-search/cross-project-search-manage-scope** current page: https://docs-v3-preview.elastic.dev/elastic/docs-content/pull/5498/explore-analyze/cross-project-search/cross-project-search-manage-scope#cps-availability <img width="911" height="833" alt="Screenshot 2026-03-24 at 6 46 07 PM" src="https://github.com/user-attachments/assets/62f1756d-6942-4c69-bf1f-d7738c47f01a" /> 3. **deploy-manage/cross-project-search-config/cps-config-impacts-and-limitations** <img width="903" height="788" alt="Screenshot 2026-03-24 at 5 59 07 PM" src="https://github.com/user-attachments/assets/b8ab728f-4084-4fce-aafd-44095d657ba6" /> </details> ## Generative AI disclosure 1. Did you use a generative AI (GenAI) tool to assist in creating this contribution? - [x] Yes - [ ] No Tool(s) and model(s) used: claude-4.6-opus-high in cursor; cursor composer-2
This change add support for cross-project requests to Painless/execute endpoint.
The painless/execute API is unique in that it works cross-cluster but it only allows you to query one index at a time.
In contrast to other CPS-enabled endpoints all expressions should be interpreted as "canonical" like GET _settings or GET _mappings, but also "happens" to allow to specify a remote. So
logsmeans_origin:logs. Origin project alias is also allowed, so if the origin project has the aliasp1p1:logsalso means_origin:logs.Endpoint does not support project routing.
Implementation details:
PainlessExecuteAction.RequestimplementsIndicesRequest.SingleIndexNoWildcards(the only such request) which handled by a branch inIndicesAndAliasesResolverparallel to one handlingIndicesRequest.Replaceble. In CPS, this branch resolves the index expression in the request against the authorized projects. If the expression resolves to a local index, the request is marked as local, so even if it hasclusterAlias, it won't be sent to a remote cluster when executed.