Skip to content
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.

Sign up for GitHub

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

Test queryable built-in role synchronization #118964

Merged
merged 11 commits into from
Dec 19, 2024
Merged

Test queryable built-in role synchronization #118964

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
990c65b
Test queryable built-in role synchronization
slobodanadamovic Dec 18, 2024
e59e51b
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 18, 2024
cb8b8be
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 18, 2024
8b5156d
fix issue where synchronizationInProgress would stay true without bei…
slobodanadamovic Dec 18, 2024
f7ad20c
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 18, 2024
28d9afb
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 19, 2024
c376a06
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 19, 2024
8b46332
properly fix the release of synchronizationInProgress
slobodanadamovic Dec 19, 2024
a5035f0
address review feedback
slobodanadamovic Dec 19, 2024
cdc108f
Merge branch 'main' of github.com:elastic/elasticsearch into sa-testi…
slobodanadamovic Dec 19, 2024
0db0545
Merge branch 'main' into sa-testing-queryable-built-in-roles
slobodanadamovic Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions ...main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,17 +201,18 @@ public void clusterChanged(ClusterChangedEvent event) {

private void syncBuiltInRoles(final QueryableBuiltInRoles roles) {
if (synchronizationInProgress.compareAndSet(false, true)) {
try {
final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state());
if (roles.rolesDigest().equals(indexedRolesDigests)) {
logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization");
} else {
executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> {
logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index");
}, QueryableBuiltInRolesSynchronizer::handleException)));
}
} finally {
final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state());
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is getting to be a lot but I wonder if we want a try-catch around this whole block after all -- readIndexedBuiltInRolesDigests can technically throw and so can executor.execute.

I spent a bit of time thinking of how to refactor this to avoid four separate calls to synchronizationInProgress.set(false) but nothing straight-forward comes to mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, I'll wrap it with try-catch block. I don't see other option.

if (roles.rolesDigest().equals(indexedRolesDigests)) {
logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization");
synchronizationInProgress.set(false);
} else {
executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> {
logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index");
synchronizationInProgress.set(false);
}, e -> {
synchronizationInProgress.set(false);
handleException(e);
})));
}
}
}
Expand Down