Skip to content

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 11, 2025

This updates the Security Index Migration task to detect changes across all projects and run the necessary migration steps in each one

This updates the Security Index Migration task to detect changes
across all projects and run the necessary migration steps in each one
@tvernum tvernum added >non-issue :Security/Security Security issues without another label v9.2.0 labels Aug 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Aug 11, 2025
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

}

private void decrementAttemptCount(ProjectId project) {
taskSubmissionAttemptCounter.computeIfAbsent(project, ignore -> new AtomicInteger(0)).decrementAndGet();
Copy link
Member

Choose a reason for hiding this comment

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

This can potentially lead to negative value as well as recreating the entry after its deletion. I think it's better to be a noop if the counter is not found for the project. The increment method seems fine since it is invoked inside the applier thread.

Copy link
Contributor

@slobodanadamovic slobodanadamovic left a comment

Choose a reason for hiding this comment

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

LGTM (left a non-blocking suggestion and a question)

return builder.build();
}

public void testMigrateSecurityIndex() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU This test is testing only migration version updates, since all security migrations are skipped. This is because the .security index is always newly created without the need for migrations. But for my understanding: there are still changes needed to the concrete SecurityMigration implementations, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm mistaken there are no supported migrations on multi project today.
The newest migration was for #115823 which was merged into 9.0.0 before Multi Project.
Which means if a cluster has multiple projects then it should also have a security index that can skip all migrations.

I can probably fake the data it in order to test it, but it won't be a real production scenario.

Copy link
Contributor

@slobodanadamovic slobodanadamovic Aug 19, 2025

Choose a reason for hiding this comment

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

Agreed, no need to adjust or test existing migrations.

To clarify my original concern, there were few bits I was missing which prompted my question:
Missed the fact that the project ID is correctly set in the thread context when migration task is submitted (in sendProjectStartRequest) and it's carried over throughout the task's lifecycle, even though we fork and stash context in migration executor. All migrations are executed within the correct project context and there are no changes needed.

@tvernum tvernum enabled auto-merge (squash) August 21, 2025 05:59
@tvernum tvernum merged commit 733d841 into elastic:main Aug 29, 2025
39 checks passed
JeremyDahlgren pushed a commit to JeremyDahlgren/elasticsearch that referenced this pull request Aug 29, 2025
This updates the Security Index Migration task to detect changes
across all projects and run the necessary migration steps in each one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants