[FEATURE] Endpoint to purge cache entries for specific users #5325
Conversation
Signed-off-by: David Lin <dlin2028@gmail.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
Signed-off-by: David Lin <dlin2028@gmail.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
Signed-off-by: Rishav Kumar <rishavaz@amazon.com>
Rishav9852Kumar
requested review from
DarshitChanpura,
RyanL1997,
cwperks,
derek-ho,
nibix,
peternied,
reta,
shikharj05 and
willyborankin
as code owners
|
Thanks @Rishav9852Kumar! can you add some testing details? |
src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/LdapAuthenticationCacheTest.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateRequest.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java
Show resolved
Hide resolved
|
|
||
| requestHandlersBuilder.allMethodsNotImplemented().override(Method.DELETE, (channel, request, client) -> { | ||
| final ConfigUpdateRequest configUpdateRequest; | ||
| if (request.path().contains("/user/")) { |
There was a problem hiding this comment.
There seems to be patch API where more than 1 user can be updatec new Route(Method.PATCH, "/internalusers"). How will this work then?
There was a problem hiding this comment.
To follow convention, this should use legacy as well as current paths: /user and /internalusers. What are the expected paths here? Also, this should be documented.
@nagarajg17 PATCH api should not be on concern as it doesn't aim to remove user entries correct?
There was a problem hiding this comment.
PATCH api should not be on concern as it doesn't aim to remove user entries correct?
Yes
src/integrationTest/java/org/opensearch/security/api/DefaultApiAvailabilityIntegrationTest.java
Show resolved
Hide resolved
| public ActionRequestValidationException validate() { | ||
| if (configTypes == null || configTypes.length == 0) { | ||
| return new ActionRequestValidationException(); | ||
| } else if (configTypes.length > 1 && (entityNames != null && entityNames.length > 1)) { |
There was a problem hiding this comment.
is this to check that if entityNames are supplied then only type of entity, i.e. ConfigType is supplied? This will need to be documented. Also, this behavior opens it for other config-types as well, to allow passing of entityNames.
src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java
Show resolved
Hide resolved
|
|
||
| requestHandlersBuilder.allMethodsNotImplemented().override(Method.DELETE, (channel, request, client) -> { | ||
| final ConfigUpdateRequest configUpdateRequest; | ||
| if (request.path().contains("/user/")) { |
There was a problem hiding this comment.
To follow convention, this should use legacy as well as current paths: /user and /internalusers. What are the expected paths here? Also, this should be documented.
@nagarajg17 PATCH api should not be on concern as it doesn't aim to remove user entries correct?
src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/LdapAuthenticationCacheTest.java
Show resolved
Hide resolved
8 participants
Description
Currently, our system only allows for the invalidation of the entire user authentication cache, which can lead to numerous cache misses and inefficiencies. This change is required to allow for more precise cache management, specifically targeting stale cache entries at the individual user level without disrupting the cache state of other users
Adding my changes on top of prev contributors 4571 [changes in progress]
Category: New feature
Why these changes are required?
Currently, our system only allows for the invalidation of the entire user authentication cache. This change is required to allow for more precise cache management, specifically targeting stale cache entries at the individual user level without disrupting the cache state of other users.
What is the old behavior before changes and new behavior after changes?
Previously, invalidating a user's cache required clearing the entire cache, affecting all users. The new behavior introduces an endpoint that allows for the invalidation of cache entries on a per-user basis, thereby maintaining cache integrity for other users and reducing unnecessary cache misses.
Issues Resolved
[List any issues this PR will resolve]
2829
Testing
Unit testing was written in FlushCacheApiTest.java
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.