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