Bulk invalidate API keys using a list of IDs #63224
Conversation
ywangd
added
>enhancement
:Security/Security
|
Pinging @elastic/es-security (:Security/Security) |
...core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java
Outdated
Show resolved
Hide resolved
| } | ||
| } else { | ||
| out.writeOptionalString(apiKeyId); | ||
| } |
There was a problem hiding this comment.
Oh wow. I can't believe we need to have this. We definitely need to look at removing this class in a follow up PR.
| security.invalidate_api_key: | ||
| body: > | ||
| { | ||
| "ids": [ "${api_key_id_2}", "${api_key_id_3}" ] |
There was a problem hiding this comment.
Did you consider putting key _1 in here, and checking previously_invalidated_api_keys: 1 ?
It seems like a useful test of the error handling for multiple ids.
There was a problem hiding this comment.
Unfortunately no. The code filters out invalidated keys before calling indexInvalidate, i.e.
Not sure if it is intended?
There was a problem hiding this comment.
I mean I will add key 1 here, but the response won't change.
Add a new ids field to the API of invalidating API keys so that it supports bulk invalidation with a list of IDs. Note the existing id field is kept as is and it is an error if both id and ids are specified.
I intentionally left it out because I wasn't sure whether we should start advertising the new field without deprecating the existing |
|
Just remember the other reason that made me not updating the docs, the HLRC. This PR didn't touch the HLRC code and docs. So I thought for consistency, I should not update the Rest API docs either. Now if we are updating the Rest API docs, I think we should also update the HLRC side (including code and docs). Does it make sense to you? |
Follow-up for #63224 and adds the missing doc update for the new ids field.
Follow-up for elastic#63224 and adds the missing doc update for the new ids field.
Follow-up for elastic#63224 and adds the missing doc update for the new ids field.
This PR deprecates the usage of the id field in the payload for the InvalidateApiKey API. The ids field introduced in #63224 is now the recommended way for performing (bulk) API key invalidation.
This PR deprecates the usage of the id field in the payload for the InvalidateApiKey API. The ids field introduced in elastic#63224 is now the recommended way for performing (bulk) API key invalidation.
Following discussions in #47609, I am reverting back to the approach of adding a new
idsfield to support bulk invalidation by specifying an array of API key ids.This PR intentionally keeps the existing
idfield as is on the API level. Deprecation may as well be introduced for it in the future. But it is not done in this PR to reduce the scope.I would also like to raise the priority of this PR because of #59376, where a new cache is introduced for API key document. The new cache requires a cache clear call to be issued after API key invalidation. It is helpful if the cache clear call is issued for a collection of API key ids instead of one for each of them. Hence being able to specifiying multiple IDs for invalidation improves the efficiency of the cache clearing call.
Resolves: #47609
Supersedes: #63081, #63127