Hi Piyush, I have added a PR (https://github.com/apache/kafka/pull/5030) with tests to show how group principals can be used for authorization with custom principal builders. One of the tests uses SASL. It is not quite the same as a full-fledged user groups, but since it works with all security protocols, it could be an alternative to wildcarded principals.
Let us know if we can help in any way to get this KIP updated and ready for voting to include in 2.0.0. Thanks, Rajini On Wed, May 16, 2018 at 10:21 PM, Colin McCabe <cmcc...@apache.org> wrote: > > On Tue, May 15, 2018 at 7:18 PM, Rajini Sivaram <rajinisiva...@gmail.com > > > > wrote: > > > > > Hi Piyush, > > > > > > It is possible to configure PrincipalBuilder for SASL ( > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > 189%3A+Improve+principal+builder+interface+and+add+support+for+SASL). > If > > > that satisfies your requirements, perhaps we can move wildcarded > principals > > > out of this KIP and focus on wildcarded resources? > > +1. > > We also need to determine which characters will be reserved for the > future. I think previously we thought about @, #, $, %, ^, &, *. > > > > On Tue, May 15, 2018 at 7:15 PM, Piyush Vijay <piyushvij...@gmail.com> > > > wrote: > > > > > >> Hi Colin, > > >> > > >> Escaping at this level is making sense to me but let me think more > and get > > >> back to you. > > Thanks, Piyush. What questions do you think are still open regarding > escape characters? > As Rajini mentioned, we have to get this in soon in order to make the KIP > freeze. > > > >> > > >> But should we not just get rid of one of AclBinding or > AclBindingFilter > > >> then? Is there a reason to keep both given that AclBindingFilter and > > >> AclBinding look exact copy of each other after this change? This will > be a > > >> one-time breaking change in APIs marked as "Evolving", but makes > sense in > > >> the long term? Am I missing something here? > > AclBinding represents an ACL. AclBindingFilter is a filter which can be > used to locate AclBinding objects. Similarly with Resource and > ResourceFilter. There is no reason to combine them because they represent > different things. Although they contain many of the same fields, they are > not exact copies. Many fields can be null in AclBindingFilter-- fields can > never be null in AclBinding. > > For example, you can have an AclBindingFilter that matches every > AclBinding. There is more discussion of this on the original KIP that > added ACL support to AdminClient. > > best, > Colin > > > >> > > >> > > >> > > >> Piyush Vijay > > >> > > >> On Tue, May 15, 2018 at 9:01 AM, Colin McCabe <cmcc...@apache.org> > wrote: > > >> > > >> > Hi Piyush, > > >> > > > >> > I think AclBinding should operate the same way as AclBindingFilter. > > >> > > > >> > So you should be able to do something like this: > > >> > > AclBindingFilter filter = new AclBindingFiler(new > > >> > ResourceFilter(ResourceType.GROUP, "foo*")) > > >> > > AclBinding binding = new AclBinding(new > Resource(ResourceType.GROUP, > > >> > "foo*")) > > >> > > assertTrue(filter.matches(binding)); > > >> > > > >> > Thinking about this more, it's starting to feel really messy to > create > > >> new > > >> > "pattern" constructors for Resource and ResourceFilter. I don't > think > > >> > people will be able to figure this out. Maybe we should just have a > > >> > limited compatibility break here, where it is now required to escape > > >> weird > > >> > consumer group names when creating ACLs for them. > > >> > > > >> > To future-proof this, we should reserve a bunch of characters at > once, > > >> > like *, @, $, %, ^, &, +, [, ], etc. If these characters appear in > a > > >> > resource name, it should be an error, unless they are escaped with a > > >> > backslash. That way, we can use them in the future. We should > create a > > >> > Resource.escapeName function which adds the correct escape > characters to > > >> > resource names (so it would translate foo* into foo\*, foo+bar into > > >> > foo\+bar, etc. etc. > > >> > > > >> > best, > > >> > Colin > > >> > > > >> > > > >> > On Mon, May 14, 2018, at 17:08, Piyush Vijay wrote: > > >> > > Colin, > > >> > > > > >> > > createAcls take a AclBinding, however, instead of > AclBindingFilter. > > >> What > > >> > > are your thoughts here? > > >> > > > > >> > > public abstract DescribeAclsResult describeAcls(AclBindingFilter > > >> > > filter, DescribeAclsOptions options); > > >> > > > > >> > > public abstract CreateAclsResult createAcls(Collection< > AclBinding> > > >> > > acls, CreateAclsOptions options); > > >> > > > > >> > > public abstract DeleteAclsResult > > >> > > deleteAcls(Collection<AclBindingFilter> filters, > DeleteAclsOptions > > >> > > options); > > >> > > > > >> > > > > >> > > Thanks > > >> > > > > >> > > Piyush Vijay > > >> > > > > >> > > On Mon, May 14, 2018 at 9:26 AM, Andy Coates <a...@confluent.io> > > >> wrote: > > >> > > > > >> > > > +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=GR > > >> OUP, > > >> > > > > 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/com > > >> mon/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 > > >> > > > > > > > >>>>> > > >> > > > > > > > >>>> > > >> > > > > > > > >> > > >> > > > > > > > > >> > > > > > > >> > > > > > >> > > > >> > > > > > > >
