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+ >> 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 >> > > > > > >
