Drop role cache entries when project is deleted #131908
Conversation
The `CompositeRoleStore` caches roles by project id, which includes an invalidation counter per project. This change clears all entries from the cache and drops the invalidation counter when a project is removed from the cluster.
tvernum
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
| /** | ||
| * Utility class to make it easy to run a block of code whenever a project is deleted (e.g. to cleanup cache entries) | ||
| */ | ||
| public class ProjectDeletedListener { |
There was a problem hiding this comment.
This class doesn't do a lot, but it makes the code that uses it a lot clearer.
The 1 line in CompositeRolesStore becomes really obvious:
new ProjectDeletedListener(this::removeProject).attach(clusterService);
There was a problem hiding this comment.
It's useful if the class does not already listen for cluster state changes. I guess there are probably a number of such classes.
| @@ -633,6 +638,12 @@ public void invalidateProject(ProjectId projectId) { | |||
| } | |||
| } | |||
|
|
|||
| final void removeProject(ProjectId projectId) { | |||
| numInvalidation.remove(projectId); | |||
There was a problem hiding this comment.
There is a fix annotation for this field on line 116 which can now be removed.
| var cs = mock(ClusterService.class); | ||
|
|
||
| final AtomicReference<ClusterStateListener> csl = new AtomicReference<>(); | ||
| doAnswer(inv -> { | ||
| assertThat(inv.getArguments(), arrayWithSize(1)); | ||
| csl.set(inv.getArgument(0)); | ||
| return null; | ||
| }).when(cs).addListener(any(ClusterStateListener.class)); |
There was a problem hiding this comment.
Instead of mocking, we could create an actual cluster service with something like ClusterServiceUtils.createClusterService(new DeterministicTaskQueue().getThreadPool()) and ClusterServiceUtils.setState to apply new states. This is often preferrable to excercise the entire event loop for the listener.
| /** | ||
| * Utility class to make it easy to run a block of code whenever a project is deleted (e.g. to cleanup cache entries) | ||
| */ | ||
| public class ProjectDeletedListener { |
There was a problem hiding this comment.
It's useful if the class does not already listen for cluster state changes. I guess there are probably a number of such classes.
| final ClusterService clusterService = mock(ClusterService.class); | ||
|
|
||
| final AtomicReference<ClusterStateListener> listener = new AtomicReference<>(); | ||
| doAnswer(inv -> { | ||
| assertThat(inv.getArguments(), Matchers.arrayWithSize(1)); | ||
| assertThat(listener.get(), nullValue()); | ||
| listener.set(inv.getArgument(0)); | ||
| return null; | ||
| }).when(clusterService).addListener(any(ClusterStateListener.class)); |
|
I didn't check it too closely. It seems possible that we can cache roles for non-existing project if the role can be found in files? CP should not route request with non-existing project-id to ES. But I am not entirely sure that's guaranteed during projects creation/deletion. No change is needed for this PR. Just writing down a thought came to me when reading this PR. |
The
CompositeRoleStorecaches roles by project id, which includes an invalidation counter per project.This change clears all entries from the cache and drops the invalidation counter when a project is removed from the cluster.