OK I've read it now. 1. I see you have an example: > For example: If I want to fetch all ACLs that match ’topicA*’, it’s not possible without introducing new API AND maintaining backwards compatibility. getAcls takes a Resource, right, which would be either a full resource name or 'ALL', i.e. '*', right? The point of the call is to get all ACLs relating to a specific resource, not a partial resource like 'topicA*'. Currently, I'm guessing / half-remembering that if you ask it for ACLs for topic 'foo' it doesn't include global 'ALL' ACLs in the list - that would be a different call. With the introduction of partial wildcards I think the _most_ backwards compatible change would be to have getAcls("topic:foo") to return all the ACLs, including that affect this topic. This could include any '*'/ALL Acls, (which would be a small backwards compatible change), or exclude them as it current does. Excluding any matching partial wildcard acl, e.g. 'f*' would break compatibility IMHO.
2. Example command lines, showing how to add ACLs to specific resources that *end* with an asterisk char and adding wildcard-suffixed ACLs, would really help clarify the KIP. e.g. bin/kafka-acls.sh --authorizer-properties zookeeper.connect=localhost:2181 --add --allow-principal User:Bob --allow-principal User:Alice --allow-host 198.51.100.0 --allow-host 198.51.100.1 --operation Read --group my-app-* With the above command I can't see how the code can know if the user means a literal group called 'my-app-*', or a wildcard suffix for any group starting with 'my-app-'. Escaping isn't enough as the escape char can clash too, e.g. escaping a literal to 'my-app-\*' can still clash with someone wanting a wildcard sufiix matching any group starting with 'my-app-\'. So there needs to be a syntax change here, I think. Maybe some new command line switch to either explicitly enable or disable 'wildcard-suffix' support? Probably defaulting to wildcard-suffix being on, (better experience going forward), though off is more backwards compatible. 3. Again, examples of how to store ACLs for specific resources that *end* with an asterisk and wildcard-suffix ACLs, with any escaping would really help. On 18 May 2018 at 13:55, Andy Coates <a...@confluent.io> wrote: > Hey Piyush, > > Thanks for getting this in! :D > > About to read now. But just quickly... > > 1. I'll read up on the need for getMatchingAcls - but just playing devils > advocate for a moment - if a current caller of getAcls() expects it to > return the full set of ACLs for a given resource, would post this change > only returning a sub set and requiring them to return getMatchingAcls to > get the full set not itself be a break in compatibility? I'm thinking about > any tooling / UI / etc people may have built on top of this. If Im missing > the point, then maybe a concrete example, (if you've not already added one > to the doc), may help. > > 2. Something must change on the command line, surely? e.g. as command line > user how would the command differ if I wanted to add an ACL onto a group > called 'foo*' as opposed to a all groups starting with 'foo'? > > 3. Thinking this through, I actually bracktracked - I don't think it will > work due to path collisions, even with escaping - as the escaped version > can still collide. > > Off to read the doc now. > > On 18 May 2018 at 13:33, Piyush Vijay <piyushvij...@gmail.com> wrote: > >> 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=GR >> OUP, >> >>> > > > > > > > > > > > >> > > > > > > > >> 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 >> >>> > > > > >> >> >> >> >> > >> > >