Add a specialized PainlessExecuteIndexAction #78825
Conversation
This change refactors the painless execute action into two actions: * PainlessExecuteAction * PainlessExecuteIndexAction Script context that require an index to run should use the newly added PainlessExecuteIndexAction. This action checks if the user has the read permission on the index to run but unlike the existing PainlessExecuteAction, no cluster privilege is required. For scripts that run outside of an index context, the existing PainlessExecuteAction can still be used. Note that passing the index in the body continues to work for backward compatibility purpose but in such case it's the old action that requires the cluster privilege **and** the read privilege that will run. Relates elastic#48856
jimczi
added
>enhancement
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/clients-team (Team:Clients) |
There was a problem hiding this comment.
This looks really good, thanks for doing this!
I do have two comments I'd like to discuss prior to approval.
The first is the path for this action as noted in a comment on the pr.
The second is I wonder if we should allow execute w/o any index to run under the most lenient permissions possible. Maybe we deprecate index usage as part of the existing execute action, and then reduce the required permissions in a future pr?
| @@ -113,7 +113,7 @@ In this example, `message` is the `<field_name>` and `response` is the | |||
|
|
|||
| [source,console] | |||
| ---- | |||
| POST /_scripts/painless/_execute | |||
| POST /my_index/_scripts/painless/_execute | |||
There was a problem hiding this comment.
Did we not want to use my-index-000001 as in the other examples?
| new Route(GET, "{index}/_scripts/painless/_execute"), | ||
| new Route(POST, "{index}/_scripts/painless/_execute")); |
There was a problem hiding this comment.
I have concerns about this path. Do we really want the index name to lead the path for an action that is exclusive to Painless and in the future possibly scripting? I understand that for most index actions this is how we describe them, but in this case I'm not sure of the fit.
I would think of something more like /_scripts/{action} where lang and index end up specified as part of body. Or /scripts/{action}/lang/{lang}/index/{index} where lang and index become optional interchangeable pieces of the path, and other parameters can extend this path easily.
There was a problem hiding this comment.
I think we want the index to be part of the path. That's the differentiator for this route compared to the other execute action. I didn't want to introduce a whole new path since the existing rest action will stay so we need some consistency ?
+1 to deprecate in a follow up. I tried to do it in this PR but there are some BWC tests that we'll need to adapt. It's also quite late in 7x so that deserves a separate discussion. |
|
After discussing with @jdconrad and @rjernst we've decided to close this PR. It doesn't solve the original issue, the manage cluster permission is needed to run a script without an index context, so it's more an hack than a long-term solution. |
This change refactors the painless execute action into two actions:
Script context that require an index to run should use the newly added PainlessExecuteIndexAction.
This action checks if the user has the read permission on the index to run but unlike the existing
PainlessExecuteAction, no cluster privilege is required.
For scripts that run outside of an index context, the existing PainlessExecuteAction can still be used.
Note that passing the index in the body continues to work for backward compatibility purpose but in such
case it's the old action that requires the cluster privilege and the read privilege that will run.
Relates #48856