KAFKA-9386; Apply delete ACL filters to resources from filter even if not in cache#7911
Conversation
|
retest this please |
omkreddy
left a comment
omkreddy
left a comment
There was a problem hiding this comment.
@rajinisivaram Thanks for the PR. LGTM.
|
@rajinisivaram We look for the resource directly by querying ZK? |
|
@ijuma When deleting ACLs, we always use the latest ACLs from ZK since the updates use the version. But in order to look up those ACLs, we need to know which resources to look up. For ACL filters that specify a resource pattern (e.g If you do |
|
Thanks for the explanation. Two comments:
|
|
@ijuma Opened https://issues.apache.org/jira/browse/KAFKA-9392 to document the limitation and add a deterministic test. |
|
Thanks. We should also check that we can continue to offer this guarantee after KIP-500. |
| val resourcesToUpdate = aclCache.keys.map { resource => | ||
| // Find all potentially matching resource patterns from the provided filters and ACL cache and apply the filters | ||
| val resources = aclCache.keys ++ filters.map(_._1.patternFilter).filter(_.matchesAtMostOne).flatMap(filterToResources) | ||
| val resourcesToUpdate = resources.map { resource => |
There was a problem hiding this comment.
Another thing to consider for a separate PR: this method seems highly inefficient. We take all the keys in the cache and do a bunch of transformations. It seems like we should avoid that, no? Intuitively, we would do a filter operation first and then transform only the cache items that match.
There was a problem hiding this comment.
I will do https://issues.apache.org/jira/browse/KAFKA-8847 first to remove references to the old classes and deprecate those. And that would avoid a lot of the unnecessary conversions. I can see if more can be done to improve this in the same PR.
|
@ijuma I guess we don't know yet the exact format in which ACLs will be stored for KIP-500. But I think this guarantee is similar to supporting concurrent updates which we need to do anyway. A deterministic test will at least ensure we will notice if it doesn't work. |
|
@omkreddy @ijuma Thanks for the reviews, merging to trunk. Will address remaining comments under KAFKA-9392 and KAFKA-8847. |
3 participants
With the old SimpleAclAuthorizer, we were handling delete filters that matched a single resource by looking up that resource directly, even if it wasn't in the cache. AclAuthorizerTest.testHighConcurrencyDeletionOfResourceAcls relies on this behaviour and fails intermittently when the cache is not up-to-date. This PR includes the resource from non-matching filters even if it is not in the cache to retain the old behaviour.
Committer Checklist (excluded from commit message)