Ready to review. Let me know if something looks missing or not clear. Thanks
Piyush Vijay On Fri, May 18, 2018 at 12:54 PM, Piyush Vijay <piyushvij...@gmail.com> wrote: > Andy, > > 1. Updated the KIP about need for getMatchingAcls(). Basically, it's > required to add an inspection method without breaking compatibility. > getAcls() only looks at a single location. > > 2. Nothing will change from user's perspective. they will add / delete/ > list ACLs as earlier. > > 3. Good point about moving everything to a v2 path. We might still have to > support snowflakes but I will this better. > > I'm giving it a final read. I'll update here once I think it's ready. > > Thanks > > > Piyush Vijay > > On Fri, May 18, 2018 at 12:18 PM, Piyush Vijay <piyushvij...@gmail.com> > wrote: > >> On it, Andy. It should be out in next 30 mins. >> >> On Fri, May 18, 2018 at 12:17 PM Andy Coates <a...@confluent.io> wrote: >> >>> Hey Piyush, >>> >>> How are you getting on updating the KIP? Can we offer any support? We're >>> starting to fly really close to the required 72 hours cut off for KIPs. >>> This doesn't leave much room for resolving any issues any committers >>> find. >>> Also, we now require at least three committers to review this KIP today >>> _and_ find no issues if we're to get this KIP accepted. >>> >>> Thanks, >>> >>> Andy >>> >>> On 18 May 2018 at 01:21, Piyush Vijay <piyushvij...@gmail.com> wrote: >>> >>> > Hi Andy, >>> > >>> > I still have some minor changes left to the KIP. I'll make them in the >>> > morning. I'm sorry I got caught up in some other things today. But that >>> > would still give us 72 hours before the deadline :) >>> > >>> > Thanks >>> > >>> > >>> > Piyush Vijay >>> > >>> > On Thu, May 17, 2018 at 1:27 PM, Andy Coates <a...@confluent.io> >>> wrote: >>> > >>> > > Hey Piyush - my bad. Sorry. >>> > > >>> > > On 17 May 2018 at 13:23, Piyush Vijay <piyushvij...@gmail.com> >>> wrote: >>> > > >>> > > > It's still not complete. I'll drop a message here when I'm done >>> with >>> > the >>> > > > updates. >>> > > > >>> > > > Thanks >>> > > > >>> > > > >>> > > > Piyush Vijay >>> > > > >>> > > > On Thu, May 17, 2018 at 12:04 PM, Andy Coates <a...@confluent.io> >>> > wrote: >>> > > > >>> > > > > Thanks for the update to the KIP Piyush! >>> > > > > >>> > > > > Reading it through again, I've a couple of questions: >>> > > > > >>> > > > > 1. Why is there a need for a new 'getMatchingAcls' method, over >>> the >>> > > > > existing getAcls method? They both take a Resource instance and >>> > return >>> > > a >>> > > > > set of Acls. What is the difference in their behaviour? >>> > > > > 2. It's not clear to me from the KIP alone what will change, >>> from a >>> > > users >>> > > > > perspective, on how they add / list / delete ACLs. I'm assuming >>> this >>> > > > won't >>> > > > > change. >>> > > > > 3. Writing ACLs to a new location to get around the issues of >>> > embedded >>> > > > > wildcards in existing group ACLs makes sense to me - but just a >>> > > thought, >>> > > > > will we be writing all new ACLs under this new path, or just >>> those >>> > that >>> > > > are >>> > > > > partial wildcards? I'm assuming its the latter, but it could >>> just be >>> > > > 'all' >>> > > > > right? As we could escape illegal chars. So we could just make >>> this >>> > > new >>> > > > > path 'v2' rather wildcard. >>> > > > > >>> > > > > Andy >>> > > > > >>> > > > > On 17 May 2018 at 09:32, Colin McCabe <cmcc...@apache.org> >>> wrote: >>> > > > > >>> > > > > > On Thu, May 17, 2018, at 09:28, Piyush Vijay wrote: >>> > > > > > > I was planning to do that. >>> > > > > > > >>> > > > > > > Another unrelated detail is the presence of the support for >>> ‘*’ >>> > ACL >>> > > > > > > currently. Looks like we’ll have to keep supporting this as a >>> > > special >>> > > > > > case, >>> > > > > > > even though using a different location for wildcard-suffix >>> ACLs >>> > on >>> > > > Zk. >>> > > > > > >>> > > > > > +1. >>> > > > > > >>> > > > > > Thanks, Piyush. >>> > > > > > >>> > > > > > Colin >>> > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > On Thu, May 17, 2018 at 9:15 AM Colin McCabe < >>> cmcc...@apache.org >>> > > >>> > > > > wrote: >>> > > > > > > >>> > > > > > > > Thanks, Piyush. +1 for starting the vote soon. >>> > > > > > > > >>> > > > > > > > Can you please also add a discussion about escaping? For >>> > > example, >>> > > > > > earlier >>> > > > > > > > we discussed using backslashes to escape special >>> characters. >>> > So >>> > > > that >>> > > > > > users >>> > > > > > > > can create an ACL referring to a literal "foo*" group by >>> > creating >>> > > > an >>> > > > > > ACL >>> > > > > > > > for "foo\*" Similarly, you can get a literal backslash >>> with >>> > > "\\". >>> > > > > > This is >>> > > > > > > > the standard UNIX escaping mechanism. >>> > > > > > > > >>> > > > > > > > Also, for the section that says "Changes to AdminClient >>> (needs >>> > > > > > > > discussion)", we need a new method that will allow users to >>> > > escape >>> > > > > > consumer >>> > > > > > > > group names and other names. So you can feed this method >>> your >>> > > > > "foo\*" >>> > > > > > > > consumer group name, and it will give you "foo\\\*", which >>> is >>> > > what >>> > > > > you >>> > > > > > > > would need to use to create an ACL for this consumer group >>> in >>> > > > > > AdminClient. >>> > > > > > > > I think that's the only change we need to admin client >>> > > > > > > > >>> > > > > > > > regards, >>> > > > > > > > Colin >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > On Thu, May 17, 2018, at 08:55, Piyush Vijay wrote: >>> > > > > > > > > Hi Rajini/Colin, >>> > > > > > > > > >>> > > > > > > > > I will remove the wildcard principals from the scope for >>> now, >>> > > > > > updating >>> > > > > > > > KIP >>> > > > > > > > > right now and will open it for vote. >>> > > > > > > > > >>> > > > > > > > > Thanks >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > Piyush Vijay >>> > > > > > > > > >>> > > > > > > > > On Thu, May 17, 2018 at 6:59 AM, Rajini Sivaram < >>> > > > > > rajinisiva...@gmail.com >>> > > > > > > > > >>> > > > > > > > > wrote: >>> > > > > > > > > >>> > > > > > > > > > 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+build >>> er+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 >>> > > > > >> >> >
