Native roles store uses mget to retrieve roles #33531
Conversation
The native roles store previously would issue a search if attempting to retrieve more than a single role. This is fine when we are attempting to retrieve all of the roles to list them in the api, but could cause issues when attempting to find roles for a user. The search is not prioritized over other search requests, so heavy aggregations/searches or overloaded nodes could cause roles to be cached as missing if the search is rejected. When attempting to load specific roles, we know the document id for the role that we are trying to load, which allows us to use the multi-get api for loading these roles. This change makes use of the multi-get api when attempting to load more than one role by name. This api is also constrained by a threadpool but the tasks in the GET threadpool should be quicker than searches. See elastic#33205
jaymode
added
>enhancement
v7.0.0
:Security/Authorization
|
Pinging @elastic/es-security |
| executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, multiGetRequest, | ||
| ActionListener.<MultiGetResponse>wrap(mGetResponse -> | ||
| listener.onResponse(Arrays.stream(mGetResponse.getResponses()) | ||
| .filter(item -> item.isFailed() == false) |
There was a problem hiding this comment.
Is silently dropping failures the right thing?
If the .security index is No shard available, then won't we just return an empty list of roles which will be stored in the negative cache?
There was a problem hiding this comment.
Given that signaling this failure up the stack would be a mess, I wonder if partial results really bring us any advantage. I suggest we fail (call the listener's failure handle). When we'll move the security searches over a different threadpool, partial results would be even scarier.
| } | ||
| }); | ||
| } else if (names.length == 1) { |
There was a problem hiding this comment.
I don't think we need this branch anymore.
|
test this please |
|
@jaymode Is this ready to review again for 6.5? |
|
I think we should wait until #34568 is incorporated and this has those changes merged before reviewing further. |
|
This is ready for review again |
| executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, multiGetRequest, | ||
| ActionListener.<MultiGetResponse>wrap(mGetResponse -> | ||
| listener.onResponse(RoleRetrievalResult.success(Arrays.stream(mGetResponse.getResponses()) | ||
| .filter(item -> item.isFailed() == false) |
There was a problem hiding this comment.
This still worries me (unnecessarily perhaps?)
This seems to be ignoring (without even logging) any item that is a failure (as distinct from does-not-exist).
If any of the gets failed, shouldn't we be returning RoleRetrievalResult.failure ?
There was a problem hiding this comment.
Yes it should be a failure. I've pushed a fix
...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java
Show resolved
Hide resolved
...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java
Outdated
Show resolved
Hide resolved
...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java
Show resolved
Hide resolved
The native roles store previously would issue a search if attempting to retrieve more than a single role. This is fine when we are attempting to retrieve all of the roles to list them in the api, but could cause issues when attempting to find roles for a user. The search is not prioritized over other search requests, so heavy aggregations/searches or overloaded nodes could cause roles to be cached as missing if the search is rejected. When attempting to load specific roles, we know the document id for the role that we are trying to load, which allows us to use the multi-get api for loading these roles. This change makes use of the multi-get api when attempting to load more than one role by name. This api is also constrained by a threadpool but the tasks in the GET threadpool should be quicker than searches. See #33205
The native roles store previously would issue a search if attempting to
retrieve more than a single role. This is fine when we are attempting
to retrieve all of the roles to list them in the api, but could cause
issues when attempting to find roles for a user. The search is not
prioritized over other search requests, so heavy aggregations/searches
or overloaded nodes could cause roles to be cached as missing if the
search is rejected.
When attempting to load specific roles, we know the document id for the
role that we are trying to load, which allows us to use the multi-get
api for loading these roles. This change makes use of the multi-get api
when attempting to load more than one role by name. This api is also
constrained by a threadpool but the tasks in the GET threadpool should
be quicker than searches.
See #33205