Piyush Vijay On Fri, May 18, 2018 at 1:55 PM, 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. > > getAcls() return only ACLs at a particular path. It doesn't return full set of ACLs today. > 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'? > > Yes, I've been thinking about this. Looks like we need more CLI flags and more APIs in Authorizer if we want to do this in a backward compatible fashion. There might be existing tooling which writes ACL for consumer group literal "foo*" today. We don't want to break things there. > 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= > > 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 > > >>> > > > > > > >> > > >> > > > > > >
