+1 On 11 May 2018 at 17:14, Colin McCabe <cmcc...@apache.org> wrote:
> Hi Andy, > > I see what you mean. I guess my thought here is that if the fields are > private, we can change it later if we need to. > > I definitely agree that we should use the scheme you describe for sending > ACLs over the wire (just the string + version number) > > cheers, > Colin > > > On Fri, May 11, 2018, at 09:39, Andy Coates wrote: > > i think I'm agreeing with you. I was merely suggesting that having an > > additional field that controls how the current field is interpreted is > more > > flexible / extensible in the future than using a 'union' style approach, > > where only one of several possible fields should be populated. But it's a > > minor thing. > > > > > > > > > > > > > > On 10 May 2018 at 09:29, Colin McCabe <cmcc...@apache.org> wrote: > > > > > Hi Andy, > > > > > > The issue that I was trying to solve here is the Java API. Right now, > > > someone can write "new ResourceFilter(ResourceType.TRANSACTIONAL_ID, > > > "foo*") and have a ResourceFilter that applies to a Transactional ID > named > > > "foo*". This has to continue to work, or else we have broken > compatibility. > > > > > > I was proposing that there would be something like a new function like > > > ResourceFilter.fromPattern(ResourceType.TRANSACTIONAL_ID, "foo*") > which > > > would create a ResourceFilter that applied to transactional IDs > starting > > > with "foo", rather than transactional IDs named "foo*" specifically. > > > > > > I don't think it's important whether the Java class has an integer, an > > > enum, or two string fields. The important thing is that there's a new > > > static function, or new constructor overload, etc. that works for > patterns > > > rather than literal strings. > > > > > > On Thu, May 10, 2018, at 03:30, Andy Coates wrote: > > > > Rather than having name and pattern fields on the ResourceFilter, > where > > > > it’s only valid for one to be set, and we want to restrict the > character > > > > set in case future enhancements need them, we could instead add a new > > > > integer ‘nameType’ field, and use constants to indicate how the name > > > > field should be interpreted, e.g. 0 = literal, 1 = wildcard. This > would > > > > be extendable, e.g we can later add 2 = regex, or what ever, and > > > > wouldn’t require any escaping. > > > > > > This is very user-unfriendly, though. Users don't want to have to > > > explicitly supply a version number when using the API, which is what > this > > > would force them to do. I don't think users are going to want to > memorize > > > that version 4 supprted "+", whereas version 3 only supported "[0-9]", > or > > > whatever. > > > > > > Just as an example, do you remember which versions of FetchRequest > added > > > which features? I don't. I always have to look at the code to > remember. > > > > > > Also, escaping is still required any time you overload a character to > mean > > > two things. Escaping is required in the current proposal to be able to > > > create a pattern that matches only "foo*". You have to type "foo\*" > It > > > would be required if we forced users to specify a version, as well. > > > > > > best, > > > Colin > > > > > > > > > > > Sent from my iPhone > > > > > > > > > On 7 May 2018, at 05:16, Piyush Vijay <piyushvij...@gmail.com> > wrote: > > > > > > > > > > Makes sense. I'll update the KIP. > > > > > > > > > > Does anyone have any other comments? :) > > > > > > > > > > Thanks > > > > > > > > > > > > > > > Piyush Vijay > > > > > > > > > >> On Thu, May 3, 2018 at 11:55 AM, Colin McCabe <cmcc...@apache.org > > > > > wrote: > > > > >> > > > > >> Yeah, I guess that's a good point. It probably makes sense to > > > support the > > > > >> prefix scheme for consumer groups and transactional IDs as well as > > > topics. > > > > >> > > > > >> I agree that the current situation where anything goes in consumer > > > group > > > > >> names and transactional ID names is not ideal. I wish we could > > > rewind the > > > > >> clock and impose restrictions on the names. However, it doesn't > seem > > > > >> practical at the moment. Adding new restrictions would break a > lot of > > > > >> existing users after an upgrade. It would be a really bad upgrade > > > > >> experience. > > > > >> > > > > >> However, I think we can support this in a compatible way. From > the > > > > >> perspective of AdminClient, we just have to add a new field to > > > > >> ResourceFilter. Currently, it has two fields, resourceType and > name: > > > > >> > > > > >>> /** > > > > >>> * A filter which matches Resource objects. > > > > >>> * > > > > >>> * The API for this class is still evolving and we may break > > > > >> compatibility in minor releases, if necessary. > > > > >>> */ > > > > >>> @InterfaceStability.Evolving > > > > >>> public class ResourceFilter { > > > > >>> private final ResourceType resourceType; > > > > >>> private final String name; > > > > >> > > > > >> We can add a third field, pattern. > > > > >> > > > > >> So the API will basically be, if I create a > > > ResourceFilter(resourceType=GROUP, > > > > >> name=foo*, pattern=null), it applies only to the consumer group > named > > > > >> "foo*". If I create a ResourceFilter(resourceType=GROUP, > name=null, > > > > >> pattern=foo*), it applies to any consumer group starting in "foo". > > > name > > > > >> and pattern cannot be both set at the same time. This preserves > > > > >> compatibility at the AdminClient level. > > > > >> > > > > >> It's possible that we will want to add more types of pattern in > the > > > > >> future. So we should reserve "special characters" such as +, /, > &, > > > %, #, > > > > >> $, etc. These characters should be treated as special unless > they are > > > > >> prefixed with a backslash to escape them. This will allow us to > add > > > > >> support for using these characters in the future without breaking > > > > >> compatibility. > > > > >> > > > > >> At the protocol level, we need a new API version for > > > CreateAclsRequest / > > > > >> DeleteAclsRequest. The new API version will send all special > > > characters > > > > >> over the wire escaped rather than directly. (So there is no need > for > > > the > > > > >> equivalent of both "name" and "pattern"-- we translate name into a > > > validly > > > > >> escaped pattern that matches only one thing, by adding escape > > > characters as > > > > >> appropriate.) The broker will validate the new API version and > reject > > > > >> malformed of unsupported patterns. > > > > >> > > > > >> At the ZK level, we can introduce a protocol version to the data > in > > > ZK-- > > > > >> or store it under a different root. > > > > >> > > > > >> best, > > > > >> Colin > > > > >> > > > > >> > > > > >>> On Wed, May 2, 2018, at 18:09, Piyush Vijay wrote: > > > > >>> Thank you everyone for the interest and, prompt and valuable > > > feedback. I > > > > >>> really appreciate the quick turnaround. I’ve tried to organize > the > > > > >> comments > > > > >>> into common headings. See my replies below: > > > > >>> > > > > >>> > > > > >>> > > > > >>> *Case of ‘*’ might already be present in consumer groups and > > > > >> transactional > > > > >>> ids* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - We definitely need wildcard ACLs support for resources like > > > consumer > > > > >>> groups and transactional ids for the reason Andy mentioned. A > big > > > win > > > > >> of > > > > >>> this feature is that service providers don’t have to track and > keep > > > > >>> up-to-date all the consumer groups their customers are using. > > > > >>> - I agree with Andy’s thoughts on the two possible ways. > > > > >>> - My vote would be to do the breaking change because we should > > > > >> restrict > > > > >>> the format of consumer groups and transactional ids sooner than > > > later. > > > > >>> - Consumer groups and transactional ids are basic Kafka > > > concepts. > > > > >>> There is a lot of value in having a defined naming > convention on > > > > >> these > > > > >>> concepts. > > > > >>> - This will help us not run into more issues down the line. > > > > >>> - I’m not sure if people actually use ‘*’ in their consumer > > > group > > > > >>> names anyway. > > > > >>> - Escaping ‘*’ isn’t trivial because ‘\’ is an allowed > character > > > > >> too. > > > > >>> > > > > >>> > > > > >>> *Why introduce two new APIs?* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - It’s possible to make this change without introducing new > APIs > > > but > > > > >> new > > > > >>> APIs are required for inspection. > > > > >>> - For example: If I want to fetch all ACLs that match > ’topicA*’, > > > it’s > > > > >>> not possible without introducing new APIs AND maintaining > backwards > > > > >>> compatibility. > > > > >>> > > > > >>> > > > > >>> *Matching multiple hosts* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - Rajini is right that wildcard ACLs aren’t the correct fit for > > > > >>> specifying range of hosts. > > > > >>> - We will rely on KIP-252 for the proper functionality ( > > > > >>> > > > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > >> 252+-+Extend+ACLs+to+allow+filtering+based+on+ip+ranges+ > and+subnets > > > > >>> ) > > > > >>> > > > > >>> > > > > >>> *Implementation of matching algorithm and performance concerns* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - Updated the KIP with an implementation. > > > > >>> - Andy, you’re right. The length doesn’t play a part. The > request > > > will > > > > >>> be authorized *iff* there is at least one matching ALLOW and no > > > > >> matching > > > > >>> DENY irrespective of the prefix length. Included this detail > in the > > > > >> KIP. > > > > >>> - Since everything is stored in memory, the performance hit > isn’t > > > > >> really > > > > >>> significantly worse than the current behavior. > > > > >>> - Stephane’s performance improvement suggestion is a great > idea but > > > > >>> orthogonal. > > > > >>> > > > > >>> > > > > >>> *Why extend this for principal?* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - Thanks for the amazing points Rajini. > > > > >>> - There is a use case where ALL principals from an org might > want > > > to > > > > >>> access fix set of topics. > > > > >>> - User group functionality is currently missing. > > > > >>> - IIRC SASL doesn’t use custom principal builder. > > > > >>> - However, prefixing is not the right choice in all cases. > Agreed. > > > > >>> - Thoughts? > > > > >>> > > > > >>> > > > > >>> *Changes to AdminClient to support wildcard ACLs* > > > > >>> > > > > >>> > > > > >>> > > > > >>> - Thanks Colin for the implementation. It’s good to have you > and > > > > >>> others > > > > >>> here for the expert opinions. > > > > >>> - The current implementation uses two classes: AclBinding and > > > > >>> AclBindingFilter. ( > > > > >>> > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/ > > > > >> main/java/org/apache/kafka/common/acl/AclBinding.java > > > > >>> and > > > > >>> > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/ > > > > >> main/java/org/apache/kafka/common/acl/AclBindingFilter.java > > > > >>> ) > > > > >>> - AclBinding is definition of an Acl. It’s used to create ACLs. > > > > >>> - AclBindingFilter is used to fetch or delete “matching’ ACLs. > > > > >>> - In the context of wildcard suffixed ACLs, a stored ACL may > have > > > ‘*’ > > > > >>> in > > > > >>> it. *It really removes the distinction between these two > classes.* > > > > >>> - The current implementation uses ‘null’ to define wildcard ACL > > > > >>> (‘*’). I > > > > >>> think it’s not a good pattern and we should use ‘*’ for the > > > wildcard > > > > >>> ACL ( > > > > >>> > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/ > > > > >> main/java/org/apache/kafka/common/acl/AclBindingFilter.java#L39 > > > > >>> and > > > > >>> > > > > >>> https://github.com/apache/kafka/blob/trunk/clients/src/ > > > > >> main/java/org/apache/kafka/common/resource/ > ResourceFilter.java#L37 > > > > >>> ). > > > > >>> - However, the above two changes are breaking change but it > should > > > be > > > > >>> acceptable because the API is marked with > > > > >>> @InterfaceStability.Evolving. > > > > >>> - If everyone agrees to the above two changes (merging the two > > > > >>> classes > > > > >>> and using non-null values for blanket access), the only other > > > change > > > > >>> is > > > > >>> using the matching algorithm from the KIP to match ACLs. > > > > >>> > > > > >>> > > > > >>> Other comments: > > > > >>> > > > > >>> > > > > >>> > > > > >>> - > It may be worth excluding delegation token ACLs from using > > > > >> prefixed > > > > >>> wildcards since it doesn't make much sense. > > > > >>> > > > > >>> I want to ask for clarification on what delegation token ACLs are > > > before > > > > >>> commenting. Wildcard suffixed ACLs are supported only for > resource > > > and > > > > >>> principal names. > > > > >>> > > > > >>> > > > > >>> > > > > >>> - A quick read makes me believe that I’ve fixed the formatting > > > issues > > > > >>> reported by Ted. Let me know if something is still wrong and I > > > would > > > > >> be > > > > >>> happy to fix it. > > > > >>> - I’ve fixed the mismatch in signature reported by Ron. > > > > >>> - Andy, I’ve updated the KIP with the security hole related to > DENY > > > > >>> wildcard ACLs ‘*’ on the downgrade path. > > > > >>> - Wrt naming, wildcard suffixed ACLs sound reasonable to me > until > > > > >>> someone raise a major objection. > > > > >>> > > > > >>> > > > > >>> > > > > >>> Let me know your thoughts. Looking forward to the next iteration. > > > > >>> > > > > >>> > > > > >>> Best, > > > > >>> > > > > >>> Piyush > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> Piyush Vijay > > > > >>> > > > > >>> On Wed, May 2, 2018 at 1:33 PM, Andy Coates <a...@confluent.io> > > > wrote: > > > > >>> > > > > >>>>> Perhaps there is a simpler way. It seems like the resource > that > > > > >> people > > > > >>>> really want to use prefixed ACLs with is topic names. Because > topic > > > > >> names > > > > >>>> can't contain "*", there is no ambiguity there, either. I think > > > maybe > > > > >> we > > > > >>>> should restrict the prefix handling to topic resources. > > > > >>>> > > > > >>>> The KIP should cover consumer groups for sure, otherwise we're > back > > > to > > > > >> the > > > > >>>> situation users need to know, up front, the set of consumer > groups > > > an > > > > >>>> KStreams topology is going to use. > > > > >>>> > > > > >>>> I'm assuming transactional producer Ids would be the same. > > > > >>>> > > > > >>>> The cleanest solution would be to restrict the characters in > group > > > and > > > > >>>> transaction producer ids, but that's a breaking change that > might > > > > >> affect a > > > > >>>> lot of people. > > > > >>>> > > > > >>>> Another solution might be to add a protocol version to the `Acl` > > > type, > > > > >> (not > > > > >>>> the current `version` field used for optimistic concurrency > > > control), > > > > >>>> defaulting it to version 1 if it is not present, and releasing > this > > > > >> change > > > > >>>> as version 2. This would at least allow us to leave the > version 1 > > > ACLs > > > > >>>> as-is, (which makes for a cleaner storey if a cluster is > > > downgraded). > > > > >> There > > > > >>>> is then potential to escape '*' when writing version 2 ACLs. > (And > > > > >> introduce > > > > >>>> code to check for supported ACL versions going forward). > Though... > > > how > > > > >> do > > > > >>>> we escape? If the consumer group is free form, then any escape > > > > >> sequence is > > > > >>>> also valid. Aren't there metrics that use the group name? If > > > there > > > > >> are, > > > > >>>> I'd of thought we'd need to restrict the char set anyway. > > > > >>>> > > > > >>>>> On 2 May 2018 at 18:57, charly molter <charly.mol...@gmail.com > > > > > wrote: > > > > >>>>> > > > > >>>>> The fact that consumer groups and transactionalProducerId don't > > > have > > > > >> a > > > > >>>>> strict format is problematic (we have problems with people with > > > empty > > > > >>>>> spaces at the end of their consumer group for example). > > > > >>>>> With 2.0 around the corner and the possibility to fix these > errors > > > > >> from > > > > >>>> the > > > > >>>>> past should we create a KIP to restrict these names like we do > for > > > > >>>> topics? > > > > >>>>> > > > > >>>>> Thanks! > > > > >>>>> Charly > > > > >>>>> > > > > >>>>> On Wed, May 2, 2018 at 6:22 PM Colin McCabe < > cmcc...@apache.org> > > > > >> wrote: > > > > >>>>> > > > > >>>>>> Hi Piyush, > > > > >>>>>> > > > > >>>>>> Thanks for the KIP! It seems like it will be really useful. > > > > >>>>>> > > > > >>>>>> As Rajini commented, the names for some resources (such as > > > consumer > > > > >>>>>> groups) can include stars. So your consumer group might be > named > > > > >>>> "foo*". > > > > >>>>>> We need a way of explicitly referring to that consumer group > name, > > > > >>>> rather > > > > >>>>>> than to the foo prefix. A simple way would be to escape the > star > > > > >> with > > > > >>>> a > > > > >>>>>> backslash: "foo\*" During the software upgrade process, we > also > > > > >> need > > > > >>>> to > > > > >>>>>> translate all ACLs that refer to "foo*" into "foo\*". > Otherwise, > > > > >> the > > > > >>>>>> upgrade could create a security hole by granting more access > than > > > > >> the > > > > >>>>>> administrator intended. > > > > >>>>>> > > > > >>>>>> Perhaps there is a simpler way. It seems like the resource > that > > > > >> people > > > > >>>>>> really want to use prefixed ACLs with is topic names. Because > > > > >> topic > > > > >>>>> names > > > > >>>>>> can't contain "*", there is no ambiguity there, either. I > think > > > > >> maybe > > > > >>>> we > > > > >>>>>> should restrict the prefix handling to topic resources. > > > > >>>>>> > > > > >>>>>> Are the new "getMatchingAcls" methods needed? It seems like > they > > > > >> break > > > > >>>>>> encapsulation. All that the calling code really needs to do > is > > > > >> call > > > > >>>>>> Authorizer#authorize(session, operation, resource). The > > > authorizer > > > > >>>> knows > > > > >>>>>> that if it has an ACL allowing access to topics starting with > > > > >> "foo" and > > > > >>>>>> someone calls authorize(foobar), it should allow access. > It's not > > > > >>>>>> necessary for the calling code to know exactly what the rules > are > > > > >> for > > > > >>>>>> authorization. The Authorizer#getAcls, APIs are only needed > when > > > > >> the > > > > >>>>>> AdminClient wants to list ACLs. > > > > >>>>>> > > > > >>>>>> best, > > > > >>>>>> Colin > > > > >>>>>> > > > > >>>>>> > > > > >>>>>>> On Wed, May 2, 2018, at 03:31, Rajini Sivaram wrote: > > > > >>>>>>> Hi Piyush, > > > > >>>>>>> > > > > >>>>>>> Thanks for the KIP for this widely requested feature. A few > > > > >>>>>>> comments/questions: > > > > >>>>>>> > > > > >>>>>>> 1. Some resource names can contain '*'. For example, consumer > > > > >> groups > > > > >>>> or > > > > >>>>>>> transactional ids. I am wondering whether we need to restrict > > > > >>>>> characters > > > > >>>>>>> for these entities or provide a way to distinguish wildcarded > > > > >>>> resource > > > > >>>>>> from > > > > >>>>>>> a resource containing the wildcard character. > > > > >>>>>>> > > > > >>>>>>> 2. I am not sure we want to do wildcarded principals. It > feels > > > > >> like a > > > > >>>>>>> workaround in the absence of user groups. In the longer > term, I > > > > >> think > > > > >>>>>>> groups (without wildcards) would be a better option to > configure > > > > >> ACLs > > > > >>>>> for > > > > >>>>>>> groups of users, rather than building user principals which > have > > > > >>>> common > > > > >>>>>>> prefixes. In the shorter term, since a configurable > > > > >> PrincipalBuilder > > > > >>>> is > > > > >>>>>>> used to build the principal used for authorization, perhaps > > > > >> using the > > > > >>>>>>> prefix itself as the principal would work (unless different > > > > >> quotas > > > > >>>> are > > > > >>>>>>> required for the full user name)? User principal strings take > > > > >>>> different > > > > >>>>>>> formats for different security protocols (eg. > CN=xxx,O=org,C=UK > > > > >> for > > > > >>>>> SSL) > > > > >>>>>>> and simple prefixing isn't probably the right grouping in > many > > > > >> cases. > > > > >>>>>>> > > > > >>>>>>> 3. I am assuming we don't want to do wildcarded hosts in > this KIP > > > > >>>> since > > > > >>>>>>> wildcard suffix doesn't really work for hosts. > > > > >>>>>>> > > > > >>>>>>> 4. It may be worth excluding delegation token ACLs from using > > > > >>>> prefixed > > > > >>>>>>> wildcards since it doesn't make much sense. > > > > >>>>>>> > > > > >>>>>>> Regards, > > > > >>>>>>> > > > > >>>>>>> Rajini > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> On Wed, May 2, 2018 at 2:05 AM, Stephane Maarek < > > > > >>>>>>> steph...@simplemachines.com.au> wrote: > > > > >>>>>>> > > > > >>>>>>>> Hi, thanks for this badly needed feature > > > > >>>>>>>> > > > > >>>>>>>> 1) Why introduce two new APIs in authorizer instead of > > > > >> replacing > > > > >>>> the > > > > >>>>>>>> implementation for simple ACL authorizer with adding the > > > > >> wildcard > > > > >>>>>>>> capability? > > > > >>>>>>>> > > > > >>>>>>>> 2) is there an impact to performance as now we're evaluating > > > > >> more > > > > >>>>>> rules ? A > > > > >>>>>>>> while back I had evaluated the concept of cached Acl result > so > > > > >>>>> swallow > > > > >>>>>> the > > > > >>>>>>>> cost of computing an authorisation cost once and then doing > in > > > > >>>> memory > > > > >>>>>>>> lookups. CF: https://issues.apache.org/ > jira/browse/KAFKA-5261 > > > > >>>>>>>> > > > > >>>>>>>> 3) is there any need to also extend this to consumer group > > > > >>>> resources > > > > >>>>> ? > > > > >>>>>>>> > > > > >>>>>>>> 4) create topics KIP as recently moved permissions out of > > > > >> Cluster > > > > >>>>> into > > > > >>>>>>>> Topic. Will wildcard be supported for this action too? > > > > >>>>>>>> > > > > >>>>>>>> Thanks a lot for this ! > > > > >>>>>>>> > > > > >>>>>>>> On Wed., 2 May 2018, 1:37 am Ted Yu, <yuzhih...@gmail.com> > > > > >> wrote: > > > > >>>>>>>> > > > > >>>>>>>>> w.r.t. naming, we can keep wildcard and drop 'prefixed' (or > > > > >>>>>> 'suffixed') > > > > >>>>>>>>> since the use of regex would always start with non-wildcard > > > > >>>>> portion. > > > > >>>>>>>>> > > > > >>>>>>>>> Cheers > > > > >>>>>>>>> > > > > >>>>>>>>> On Tue, May 1, 2018 at 12:13 PM, Andy Coates < > > > > >> a...@confluent.io> > > > > >>>>>> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>>> Hi Piyush, > > > > >>>>>>>>>> > > > > >>>>>>>>>> Can you also document in the Compatibility section what > > > > >> would > > > > >>>>>> happen > > > > >>>>>>>>> should > > > > >>>>>>>>>> the cluster be upgraded, wildcard-suffixed ACLs are added, > > > > >> and > > > > >>>>>> then the > > > > >>>>>>>>>> cluster is rolled back to the previous version. On > > > > >> downgrade > > > > >>>> the > > > > >>>>>>>> partial > > > > >>>>>>>>>> wildcard ACLs will be treated as literals and hence never > > > > >> match > > > > >>>>>>>> anything. > > > > >>>>>>>>>> This is fine for ALLOW ACLs, but some might consider this > a > > > > >>>>>> security > > > > >>>>>>>> hole > > > > >>>>>>>>>> if a DENY ACL is ignored, so this needs to be documented, > > > > >> both > > > > >>>> in > > > > >>>>>> the > > > > >>>>>>>> KIP > > > > >>>>>>>>>> and the final docs. > > > > >>>>>>>>>> > > > > >>>>>>>>>> For some reason I find the term 'prefixed wildcard ACLs' > > > > >> easier > > > > >>>>> to > > > > >>>>>> grok > > > > >>>>>>>>>> than 'wildcard suffixed ACLs'. Probably because in the > > > > >> former > > > > >>>> the > > > > >>>>>>>>>> 'wildcard' term comes after the positional adjective, > which > > > > >>>>>> matches the > > > > >>>>>>>>>> position of the wildcard char in the resource name, i.e. > > > > >>>> "some*". > > > > >>>>>> It's > > > > >>>>>>>>>> most likely a person thing, but I thought I'd mention it > as > > > > >>>>> naming > > > > >>>>>> is > > > > >>>>>>>>>> important when it comes to making this initiative for > > > > >> users. > > > > >>>>>>>>>> > > > > >>>>>>>>>> On 1 May 2018 at 19:57, Andy Coates <a...@confluent.io> > > > > >> wrote: > > > > >>>>>>>>>> > > > > >>>>>>>>>>> Hi Piyush, > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Thanks for raising this KIP - it's very much appreciated. > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> I've not had chance to digest it yet, but... > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> 1. you might want to add details of how the internals of > > > > >> the > > > > >>>>>>>>>>> `getMatchingAcls` is implemented. We'd want to make sure > > > > >> the > > > > >>>>>>>> complexity > > > > >>>>>>>>>> of > > > > >>>>>>>>>>> the operation isn't adversely affected. > > > > >>>>>>>>>>> 2. You might want to be more explicit that the length of > > > > >> a > > > > >>>>> prefix > > > > >>>>>>>> does > > > > >>>>>>>>>> not > > > > >>>>>>>>>>> play a part in the `authorize` call, e.g. given ACLs > > > > >> {DENY, > > > > >>>>>> some.*}, > > > > >>>>>>>>>> {ALLOW, > > > > >>>>>>>>>>> some.prefix.*}, the longer, i.e. more specific, allow ACL > > > > >>>> does > > > > >>>>>> _not_ > > > > >>>>>>>>>>> override the more general deny ACL. > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Thanks, > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Andy > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> On 1 May 2018 at 16:59, Ron Dagostino <rndg...@gmail.com > > > > >>> > > > > >>>>> wrote: > > > > >>>>>>>>>>> > > > > >>>>>>>>>>>> Hi Piyush. I appreciated your talk at Kafka Summit and > > > > >>>>>> appreciate > > > > >>>>>>>> the > > > > >>>>>>>>>> KIP > > > > >>>>>>>>>>>> -- thanks. > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> Could you explain these mismatching references? Near > > > > >> the > > > > >>>> top > > > > >>>>>> of the > > > > >>>>>>>>> KIP > > > > >>>>>>>>>>>> you refer to these proposed new method signatures: > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> def getMatchingAcls(resource: Resource): Set[Acl] > > > > >>>>>>>>>>>> def getMatchingAcls(principal: KafkaPrincipal): > > > > >>>> Map[Resource, > > > > >>>>>>>>> Set[Acl]] > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> But near the bottom of the KIP you refer to different > > > > >> method > > > > >>>>>>>>>>>> signatures that don't seem to match the above ones: > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> getMatchingAcls(topicRegex) > > > > >>>>>>>>>>>> getMatchingAcls(principalRegex) > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> Ron > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> On Tue, May 1, 2018 at 11:48 AM, Ted Yu < > > > > >>>> yuzhih...@gmail.com> > > > > >>>>>>>> wrote: > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>>> The KIP was well written. Minor comment on formatting: > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> https://github.com/apache/kafka/blob/trunk/core/src/ > > > > >>>>>>>>>>>>> main/scala/kafka/admin/AclCommand.scala > > > > >>>>>>>>>>>>> to > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> Leave space between the URL and 'to' > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> Can you describe changes for the AdminClient ? > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> Thanks > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> On Tue, May 1, 2018 at 8:12 AM, Piyush Vijay < > > > > >>>>>>>>> piyushvij...@gmail.com> > > > > >>>>>>>>>>>>> wrote: > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> Hi all, > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> I just opened a KIP to add support for wildcard > > > > >> suffixed > > > > >>>>>> ACLs. > > > > >>>>>>>>> This > > > > >>>>>>>>>> is > > > > >>>>>>>>>>>>> one > > > > >>>>>>>>>>>>>> of the feature I talked about in my Kafka summit > > > > >> talk > > > > >>>> and > > > > >>>>> we > > > > >>>>>>>>>> promised > > > > >>>>>>>>>>>> to > > > > >>>>>>>>>>>>>> upstream it :) > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> The details are here - > > > > >>>>>>>>>>>>>> https://cwiki.apache.org/ > > > > >> confluence/display/KAFKA/KIP- > > > > >>>>>>>>>>>>>> 290%3A+Support+for+wildcard+suffixed+ACLs > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> There is an open question about the way to add the > > > > >>>> support > > > > >>>>>> in > > > > >>>>>>>> the > > > > >>>>>>>>>>>>>> AdminClient, which I can discuss here in more detail > > > > >>>> once > > > > >>>>>>>> everyone > > > > >>>>>>>>>> has > > > > >>>>>>>>>>>>>> taken a first look at the KIP. > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> Looking forward to discuss the change. > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> Best, > > > > >>>>>>>>>>>>>> Piyush Vijay > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>> > > > > >>>>>>>> > > > > >>>>>> > > > > >>>>> > > > > >>>>> > > > > >>>>> -- > > > > >>>>> Charly Molter > > > > >>>>> > > > > >>>> > > > > >> > > > >