Skip to content

Conversation

nielsbauman
Copy link
Contributor

@nielsbauman nielsbauman commented Jul 3, 2025

This commit makes the ScriptService, the ScriptCache, and the stored
script APIs project-aware. By adding the project ID to the cache key in
the ScriptCache, we avoid data leakage between projects. We resolve the
project ID from the thread context inside ScriptService#compile instead of
passing the project ID explicitly, to avoid making a large number of changes.
Since scripts should always be compiled in a user context, resolving the
project ID this way should be fine.

This commit makes the `ScriptService`, the `ScriptCache`, and the stored
script APIs project-aware. By adding the project ID to the cache key in
the `ScriptCache`, we avoid data leakage between projects. The method
`ScriptService#compile` has a lot of usages, so we introduce a temporary
method that hard-codes the default project ID. Follow-up PRs will update
the remaining usages to pass the correct project ID.
@nielsbauman nielsbauman requested a review from a team as a code owner July 3, 2025 20:06
@nielsbauman nielsbauman added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jul 3, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added v9.2.0 Team:Core/Infra Meta label for core/infra team labels Jul 3, 2025
@rjernst
Copy link
Member

rjernst commented Jul 3, 2025

ScriptService#compile has a lot of usages
Follow-up PRs will update the remaining usages to pass the correct project ID.

Compilation should normally be in the context of a user request. Shouldn't it then get the project id from the thread context?

@nielsbauman
Copy link
Contributor Author

Compilation should normally be in the context of a user request. Shouldn't it then get the project id from the thread context?

@rjernst, thanks for chiming in! We usually try to explicitly pass the project ID around and minimize the reliance on the thread context, as it can be difficult to track down where the project ID is supposed to be set in a call chain. Since there are a lot of usages all over the codebase here, I'm open to considering using the thread context. Can you think of any usages where compilation happens outside of the context of a user request? A perhaps counterintuitive example is cluster state update tasks; those are run without the thread context of the request. Do you happen to know if we do any script compilation in cluster state update tasks, for example? I can do some investigation if you don't already happen to know.

@rjernst
Copy link
Member

rjernst commented Jul 3, 2025

Can you think of any usages where compilation happens outside of the context of a user request?

One I'm not sure of is async search. I assume the search happens in the user context, even after being forked to the async threadpool as a task, but if not then that would be one. Otherwise, scripts are a way for users to customize functionality, so it wouldn't make sense to compile a script outside a user request.

@nielsbauman nielsbauman requested a review from a team as a code owner July 7, 2025 15:04
@nielsbauman
Copy link
Contributor Author

Thanks, @rjernst. I pushed some changes to use the project resolver instead of explicitly passing a project ID to ScriptService#compile. I manually tested an async search with a stored script, and that seemed to work fine. I suggest that we stick to this approach until we run into any issues (which we probably won't based on your statement about scripts being compiled in user contexts). It would be great if you (or someone else from the Core/Infra team) could have a look, thanks!

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine but one question

return;
}
for (var project : clusterState.metadata().projects().values()) {
ScriptMetadata scriptMetadata = project.custom(ScriptMetadata.TYPE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with non MP clusters here? Will existing cluster states have their script metadata moved into project state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's exactly right. ScriptMetadata extends Metadata.ProjectCustom and will thus always be put in ProjectMetadata#customs. See the Metadata deserialization for instance, we read the metadata from an old node here:


and put that in the ProjectMetadata.Bulider#putCustom here:
projectBuilder.putCustom(custom.getWriteableName(), custom);

Does that answer your question?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nielsbauman nielsbauman enabled auto-merge (squash) August 2, 2025 07:43
@nielsbauman nielsbauman merged commit d6e59ad into elastic:main Aug 2, 2025
33 checks passed
@nielsbauman nielsbauman deleted the stored-script-mp branch August 2, 2025 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >non-issue Team:Core/Infra Meta label for core/infra team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants