Skip to content

Commit 33b8fb1

Browse files
authored
Move expensive role building off transport thread (#113020) (#117002)
# Backport This will backport the following commits from `main` to `8.16`: - [Move expensive role building off transport thread (#113020)](#113020)
1 parent 771bea1 commit 33b8fb1

File tree

4 files changed

+225
-10
lines changed

4 files changed

+225
-10
lines changed

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,15 @@
5454
import org.elasticsearch.common.transport.BoundTransportAddress;
5555
import org.elasticsearch.common.util.BigArrays;
5656
import org.elasticsearch.common.util.PageCacheRecycler;
57+
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
5758
import org.elasticsearch.common.util.concurrent.EsExecutors;
5859
import org.elasticsearch.common.util.concurrent.ListenableFuture;
5960
import org.elasticsearch.common.util.concurrent.ThreadContext;
61+
import org.elasticsearch.common.util.concurrent.ThrottledTaskRunner;
6062
import org.elasticsearch.common.util.set.Sets;
6163
import org.elasticsearch.core.IOUtils;
6264
import org.elasticsearch.core.Nullable;
65+
import org.elasticsearch.core.Releasable;
6366
import org.elasticsearch.env.Environment;
6467
import org.elasticsearch.env.NodeMetadata;
6568
import org.elasticsearch.features.FeatureService;
@@ -1037,6 +1040,7 @@ Collection<Object> createComponents(
10371040
serviceAccountService,
10381041
dlsBitsetCache.get(),
10391042
restrictedIndices,
1043+
buildRoleBuildingExecutor(threadPool, settings),
10401044
new DeprecationRoleDescriptorConsumer(clusterService, threadPool)
10411045
);
10421046
systemIndices.getMainIndexManager().addStateListener(allRolesStore::onSecurityIndexStateChange);
@@ -1268,6 +1272,29 @@ private void submitPersistentMigrationTask(int migrationsVersion, boolean securi
12681272
);
12691273
}
12701274

1275+
private static Executor buildRoleBuildingExecutor(ThreadPool threadPool, Settings settings) {
1276+
final int allocatedProcessors = EsExecutors.allocatedProcessors(settings);
1277+
final ThrottledTaskRunner throttledTaskRunner = new ThrottledTaskRunner("build_roles", allocatedProcessors, threadPool.generic());
1278+
return r -> throttledTaskRunner.enqueueTask(new ActionListener<>() {
1279+
@Override
1280+
public void onResponse(Releasable releasable) {
1281+
try (releasable) {
1282+
r.run();
1283+
}
1284+
}
1285+
1286+
@Override
1287+
public void onFailure(Exception e) {
1288+
if (r instanceof AbstractRunnable abstractRunnable) {
1289+
abstractRunnable.onFailure(e);
1290+
}
1291+
// should be impossible, GENERIC pool doesn't reject anything
1292+
logger.error("unexpected failure running " + r, e);
1293+
assert false : new AssertionError("unexpected failure running " + r, e);
1294+
}
1295+
});
1296+
}
1297+
12711298
private AuthorizationEngine getAuthorizationEngine() {
12721299
return findValueFromExtensions("authorization engine", extension -> extension.getAuthorizationEngine(settings));
12731300
}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

Lines changed: 70 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.apache.logging.log4j.Logger;
1111
import org.elasticsearch.ElasticsearchException;
1212
import org.elasticsearch.action.ActionListener;
13+
import org.elasticsearch.action.ActionRunnable;
1314
import org.elasticsearch.common.Strings;
1415
import org.elasticsearch.common.bytes.BytesReference;
1516
import org.elasticsearch.common.cache.Cache;
@@ -65,6 +66,7 @@
6566
import java.util.Objects;
6667
import java.util.Optional;
6768
import java.util.Set;
69+
import java.util.concurrent.Executor;
6870
import java.util.concurrent.atomic.AtomicLong;
6971
import java.util.function.Consumer;
7072
import java.util.stream.Collectors;
@@ -91,6 +93,11 @@ public class CompositeRolesStore {
9193
Property.NodeScope
9294
);
9395
private static final Logger logger = LogManager.getLogger(CompositeRolesStore.class);
96+
/**
97+
* See {@link #shouldForkRoleBuilding(Set)}
98+
*/
99+
private static final int ROLE_DESCRIPTOR_FORK_THRESHOLD = 100;
100+
private static final int INDEX_PRIVILEGE_FORK_THRESHOLD = 1000;
94101

95102
private final RoleProviders roleProviders;
96103
private final NativePrivilegeStore privilegeStore;
@@ -106,6 +113,7 @@ public class CompositeRolesStore {
106113
private final Map<String, Role> internalUserRoles;
107114
private final RestrictedIndices restrictedIndices;
108115
private final ThreadContext threadContext;
116+
private final Executor roleBuildingExecutor;
109117

110118
public CompositeRolesStore(
111119
Settings settings,
@@ -118,6 +126,7 @@ public CompositeRolesStore(
118126
ServiceAccountService serviceAccountService,
119127
DocumentSubsetBitsetCache dlsBitsetCache,
120128
RestrictedIndices restrictedIndices,
129+
Executor roleBuildingExecutor,
121130
Consumer<Collection<RoleDescriptor>> effectiveRoleDescriptorsConsumer
122131
) {
123132
this.roleProviders = roleProviders;
@@ -179,6 +188,7 @@ public void providersChanged() {
179188
);
180189
this.anonymousUser = new AnonymousUser(settings);
181190
this.threadContext = threadContext;
191+
this.roleBuildingExecutor = roleBuildingExecutor;
182192
}
183193

184194
public void getRoles(Authentication authentication, ActionListener<Tuple<Role, Role>> roleActionListener) {
@@ -276,21 +286,70 @@ public void buildRoleFromRoleReference(RoleReference roleReference, ActionListen
276286
} else if (RolesRetrievalResult.SUPERUSER == rolesRetrievalResult) {
277287
roleActionListener.onResponse(superuserRole);
278288
} else {
279-
buildThenMaybeCacheRole(
280-
roleKey,
281-
rolesRetrievalResult.getRoleDescriptors(),
282-
rolesRetrievalResult.getMissingRoles(),
283-
rolesRetrievalResult.isSuccess(),
284-
invalidationCounter,
285-
ActionListener.wrap(roleActionListener::onResponse, failureHandler)
286-
);
289+
final ActionListener<Role> wrapped = ActionListener.wrap(roleActionListener::onResponse, failureHandler);
290+
if (shouldForkRoleBuilding(rolesRetrievalResult.getRoleDescriptors())) {
291+
roleBuildingExecutor.execute(
292+
ActionRunnable.wrap(
293+
wrapped,
294+
l -> buildThenMaybeCacheRole(
295+
roleKey,
296+
rolesRetrievalResult.getRoleDescriptors(),
297+
rolesRetrievalResult.getMissingRoles(),
298+
rolesRetrievalResult.isSuccess(),
299+
invalidationCounter,
300+
l
301+
)
302+
)
303+
);
304+
} else {
305+
buildThenMaybeCacheRole(
306+
roleKey,
307+
rolesRetrievalResult.getRoleDescriptors(),
308+
rolesRetrievalResult.getMissingRoles(),
309+
rolesRetrievalResult.isSuccess(),
310+
invalidationCounter,
311+
wrapped
312+
);
313+
}
287314
}
288315
}, failureHandler));
289316
} else {
290317
roleActionListener.onResponse(existing);
291318
}
292319
}
293320

321+
/**
322+
* Uses heuristics such as presence of application privileges to determine if role building will be expensive
323+
* and therefore warrants forking.
324+
* Package-private for testing.
325+
*/
326+
boolean shouldForkRoleBuilding(Set<RoleDescriptor> roleDescriptors) {
327+
// A role with many role descriptors is likely expensive to build
328+
if (roleDescriptors.size() > ROLE_DESCRIPTOR_FORK_THRESHOLD) {
329+
return true;
330+
}
331+
int totalIndexPrivileges = 0;
332+
int totalRemoteIndexPrivileges = 0;
333+
for (RoleDescriptor roleDescriptor : roleDescriptors) {
334+
// Application privileges can also result in big automata; it's difficult to determine how big application privileges
335+
// are so err on the side of caution
336+
if (roleDescriptor.hasApplicationPrivileges()) {
337+
return true;
338+
}
339+
// Index privilege names or remote index privilege names can result in big and complex automata
340+
totalIndexPrivileges += roleDescriptor.getIndicesPrivileges().length;
341+
totalRemoteIndexPrivileges += roleDescriptor.getRemoteIndicesPrivileges().length;
342+
if (totalIndexPrivileges > INDEX_PRIVILEGE_FORK_THRESHOLD || totalRemoteIndexPrivileges > INDEX_PRIVILEGE_FORK_THRESHOLD) {
343+
return true;
344+
}
345+
// Likewise for FLS/DLS
346+
if (roleDescriptor.isUsingDocumentOrFieldLevelSecurity()) {
347+
return true;
348+
}
349+
}
350+
return false;
351+
}
352+
294353
private static boolean includesSuperuserRole(RoleReference roleReference) {
295354
if (roleReference instanceof RoleReference.NamedRoleReference namedRoles) {
296355
return Arrays.asList(namedRoles.getRoleNames()).contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName());
@@ -313,10 +372,11 @@ private void buildThenMaybeCacheRole(
313372
ActionListener<Role> listener
314373
) {
315374
logger.trace(
316-
"Building role from descriptors [{}] for names [{}] from source [{}]",
375+
"Building role from descriptors [{}] for names [{}] from source [{}] on [{}]",
317376
roleDescriptors,
318377
roleKey.getNames(),
319-
roleKey.getSource()
378+
roleKey.getSource(),
379+
Thread.currentThread().getName()
320380
);
321381
buildRoleFromDescriptors(
322382
roleDescriptors,

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import org.elasticsearch.common.settings.ClusterSettings;
4848
import org.elasticsearch.common.settings.Settings;
4949
import org.elasticsearch.common.time.DateFormatter;
50+
import org.elasticsearch.common.util.concurrent.EsExecutors;
5051
import org.elasticsearch.common.util.concurrent.ThreadContext;
5152
import org.elasticsearch.core.Tuple;
5253
import org.elasticsearch.index.Index;
@@ -242,6 +243,7 @@ public void setup() {
242243
mock(ServiceAccountService.class),
243244
new DocumentSubsetBitsetCache(Settings.EMPTY, mock(ThreadPool.class)),
244245
RESTRICTED_INDICES,
246+
EsExecutors.DIRECT_EXECUTOR_SERVICE,
245247
rds -> {}
246248
)
247249
);

0 commit comments

Comments
 (0)