On Thu, Aug 29, 2024, at 01:34, Claude Warren, Jr wrote: > Colin, > > Thanks for your insightful comments. I came to the same conclusion. > > I do have 2 Jira tickets to simplify some of this. > > 1) KAFKA-17316 <https://issues.apache.org/jira/browse/KAFKA-17316> - Makes > developing a new Authorizer by creating a new implementation of the > StandardAuthorizerData easier. Basically adds interfaces, abstract classes > and lots of tests. > > 2) KAFKA-17423 <https://issues.apache.org/jira/browse/KAFKA-17423> - Builds > on KAFKA-17316 by creating a Trie implementation (without the new wildcards > from KIP-1042). This provides an order of magnitude faster processing of ACL > requests. > > My plan is to get these through the process, so I have a better understanding > of the Kafka development process, and then rework KIP-1042 to create a new > PatternType with wildcard processing. > > I would appreciate your feedback on the above tickets if you have the time.
Hi Claude, Thanks for the response. However, I don't want to fork the StandardAuthorizer. I would be -1 on that since it would greatly enlarge our maintenance burden, for no good reason. And if we did fork it, I wouldn't want the forked version to depend on the internal classes of the original one, since that would make evolving either one much more difficult. We should be able to add wildcards to the existing authorizer without too much trouble, just by adding a new type alongside LITERAL, PREFIXED. Perhaps GLOB ? We always planned on adding more types, to cover exactly this scenario. best, Colin > Claude > > On Fri, Aug 23, 2024 at 9:43 PM Colin McCabe <cmcc...@apache.org> wrote: >> On Sat, Jul 27, 2024, at 04:20, Claude Warren, Jr wrote: >> > I have updated the KIP with results from the Trie implementation and they >> > are dramatic to say the least. For most searches they are at least an >> > order of magnitude faster and use less memory. The wildcard search is not >> > a regular expression but rather a file type wild card (*=1 or more >> > character, ?=1 character). >> > >> >> Hi Claude, >> >> One issue here is that your change is incompatible. For example, if I have a >> consumer group named '?', and an ALLOW ACL for '?', this change alters the >> meaning of that ACL. Previously it just ALLOWed '?' Now it allows any >> single-character group. This obviously could be a major security issue for >> people doing upgrades. >> >> (This wouldn't be an issue if we restricted consumer group names to a >> sensible set of characters, like we did with topics. But that ship has >> sailed...) >> >> If you want to add new behavior in a backwards-compatible fashion, your best >> bet probably is to create a new PatternType. Unfortunately this makes the >> complexity issue worse, but I don't see a way around it. Perhaps we can >> deprecate LITERAL and PREFIXED at some future date, if this wildcard thing >> makes them unecessary. >> >> Another issue I see, somewhat related, is that Authorizers are pluggable, >> and probably won't be updated all at once to support this new type. That >> should be fine, but we should document the error that is returned when >> someone tries to create your new-style ACLs with an authorizer that does not >> support them. >> >> > Code is available on my personal branch [1]. It is not fully documented >> > and does not meet the checkstyle requirements yet. I also modified the jmh >> > tests to run the Trie tests and limit the test to the single case mentioned >> > in the KIP documentation. >> > >> > If there are no issues with this code, I will complete the documentation >> > and fix the checkstyle and then open a pull request. >> >> It's fine to open a pull request. You can link it on the KIP page. As >> always, we don't want to commit it until the KIP is accepted. >> >> best, >> Colin >> >> > >> > Claude >> > >> > [1] https://github.com/Claudenw/kafka/pull/new/KIP-1042_Trie_Implementation >> > >> > >> > On Wed, Jul 3, 2024 at 2:21 PM Claude Warren, Jr <claude.war...@aiven.io> >> > wrote: >> > >> >> I think that if we put in a trie based system we should be able to halve >> >> the normal searhc times and still be able to locate wild card matches very >> >> quickly. Users should be warned that "head wildcard" matches are slow and >> >> to use them sparingly. I am going to see if I can work out how to do >> >> wildcard matches within the trie. >> >> >> >> But in all cases can show that the trie is faster than the current >> >> implementation. >> >> >> >> Claude >> >> >> >> >> >> >> >> On Wed, Jun 19, 2024 at 7:53 PM Muralidhar Basani >> >> <muralidhar.bas...@aiven.io.invalid> wrote: >> >> >> >>> There are some test results mentioned in the Test Plan section of the >> >>> Kip, >> >>> but we need to do more testing with various patterns and permission >> >>> types. >> >>> As mentioned in the discuss thread, the trie implementation could >> >>> potentially surpass the current speed of ACL match. >> >>> >> >>> However, we can only accurately assess the results after updating the >> >>> actual classes and analysing them with AuthorizerBenchmark. >> >>> >> >>> Thanks, >> >>> >> >>> Murali >> >>> >> >>> On Mon, 17 Jun 2024 at 20:39, Colin McCabe <cmcc...@apache.org> wrote: >> >>> >> >>> > My concern is that the extra complexity may actually slow us down. In >> >>> > general people already complain about the speed of ACL matches, and >> >>> adding >> >>> > another "degree of freedom" seems likely to make things worse. >> >>> > >> >>> > It would be useful to understand how much faster or slower the code is >> >>> > with the propsed changes, versus without them. >> >>> > >> >>> > best, >> >>> > Colin >> >>> > >> >>> > >> >>> > On Mon, Jun 17, 2024, at 01:26, Muralidhar Basani wrote: >> >>> > > Hi all, >> >>> > > >> >>> > > I would like to call a vote on KIP-1042 which extends creation of >> >>> > > acls >> >>> > with >> >>> > > MATCH pattern type. >> >>> > > >> >>> > > KIP - >> >>> > > >> >>> > >> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1042%3A+Support+for+wildcard+when+creating+new+acls >> >>> > > >> >>> > > Discussion thread - >> >>> > > https://lists.apache.org/thread/xx3lcg60kp4v34x0j9p6xobby8l4cfq2 >> >>> > > >> >>> > > Thanks, >> >>> > > Murali >> >>> > >> >>> >> >>
