On it, Andy. It should be out in next 30 mins. On Fri, May 18, 2018 at 12:17 PM Andy Coates <a...@confluent.io> wrote:
> Hey Piyush, > > How are you getting on updating the KIP? Can we offer any support? We're > starting to fly really close to the required 72 hours cut off for KIPs. > This doesn't leave much room for resolving any issues any committers find. > Also, we now require at least three committers to review this KIP today > _and_ find no issues if we're to get this KIP accepted. > > Thanks, > > Andy > > On 18 May 2018 at 01:21, Piyush Vijay <piyushvij...@gmail.com> wrote: > > > Hi Andy, > > > > I still have some minor changes left to the KIP. I'll make them in the > > morning. I'm sorry I got caught up in some other things today. But that > > would still give us 72 hours before the deadline :) > > > > Thanks > > > > > > Piyush Vijay > > > > On Thu, May 17, 2018 at 1:27 PM, Andy Coates <a...@confluent.io> wrote: > > > > > Hey Piyush - my bad. Sorry. > > > > > > On 17 May 2018 at 13:23, Piyush Vijay <piyushvij...@gmail.com> wrote: > > > > > > > It's still not complete. I'll drop a message here when I'm done with > > the > > > > updates. > > > > > > > > Thanks > > > > > > > > > > > > Piyush Vijay > > > > > > > > On Thu, May 17, 2018 at 12:04 PM, Andy Coates <a...@confluent.io> > > wrote: > > > > > > > > > Thanks for the update to the KIP Piyush! > > > > > > > > > > Reading it through again, I've a couple of questions: > > > > > > > > > > 1. Why is there a need for a new 'getMatchingAcls' method, over the > > > > > existing getAcls method? They both take a Resource instance and > > return > > > a > > > > > set of Acls. What is the difference in their behaviour? > > > > > 2. It's not clear to me from the KIP alone what will change, from a > > > users > > > > > perspective, on how they add / list / delete ACLs. I'm assuming > this > > > > won't > > > > > change. > > > > > 3. Writing ACLs to a new location to get around the issues of > > embedded > > > > > wildcards in existing group ACLs makes sense to me - but just a > > > thought, > > > > > will we be writing all new ACLs under this new path, or just those > > that > > > > are > > > > > partial wildcards? I'm assuming its the latter, but it could just > be > > > > 'all' > > > > > right? As we could escape illegal chars. So we could just make > this > > > new > > > > > path 'v2' rather wildcard. > > > > > > > > > > Andy > > > > > > > > > > On 17 May 2018 at 09:32, Colin McCabe <cmcc...@apache.org> wrote: > > > > > > > > > > > On Thu, May 17, 2018, at 09:28, Piyush Vijay wrote: > > > > > > > I was planning to do that. > > > > > > > > > > > > > > Another unrelated detail is the presence of the support for ‘*’ > > ACL > > > > > > > currently. Looks like we’ll have to keep supporting this as a > > > special > > > > > > case, > > > > > > > even though using a different location for wildcard-suffix ACLs > > on > > > > Zk. > > > > > > > > > > > > +1. > > > > > > > > > > > > Thanks, Piyush. > > > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, May 17, 2018 at 9:15 AM Colin McCabe < > cmcc...@apache.org > > > > > > > > wrote: > > > > > > > > > > > > > > > Thanks, Piyush. +1 for starting the vote soon. > > > > > > > > > > > > > > > > Can you please also add a discussion about escaping? For > > > example, > > > > > > earlier > > > > > > > > we discussed using backslashes to escape special characters. > > So > > > > that > > > > > > users > > > > > > > > can create an ACL referring to a literal "foo*" group by > > creating > > > > an > > > > > > ACL > > > > > > > > for "foo\*" Similarly, you can get a literal backslash with > > > "\\". > > > > > > This is > > > > > > > > the standard UNIX escaping mechanism. > > > > > > > > > > > > > > > > Also, for the section that says "Changes to AdminClient > (needs > > > > > > > > discussion)", we need a new method that will allow users to > > > escape > > > > > > consumer > > > > > > > > group names and other names. So you can feed this method > your > > > > > "foo\*" > > > > > > > > consumer group name, and it will give you "foo\\\*", which is > > > what > > > > > you > > > > > > > > would need to use to create an ACL for this consumer group in > > > > > > AdminClient. > > > > > > > > I think that's the only change we need to admin client > > > > > > > > > > > > > > > > regards, > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > On Thu, May 17, 2018, at 08:55, Piyush Vijay wrote: > > > > > > > > > Hi Rajini/Colin, > > > > > > > > > > > > > > > > > > I will remove the wildcard principals from the scope for > now, > > > > > > updating > > > > > > > > KIP > > > > > > > > > right now and will open it for vote. > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > Piyush Vijay > > > > > > > > > > > > > > > > > > On Thu, May 17, 2018 at 6:59 AM, Rajini Sivaram < > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Piyush, > > > > > > > > > > > > > > > > > > > > I have added a PR (https://github.com/apache/ > > kafka/pull/5030 > > > ) > > > > > with > > > > > > > > tests > > > > > > > > > > to > > > > > > > > > > show how group principals can be used for authorization > > with > > > > > custom > > > > > > > > > > principal builders. One of the tests uses SASL. It is not > > > quite > > > > > the > > > > > > > > same as > > > > > > > > > > a full-fledged user groups, but since it works with all > > > > security > > > > > > > > protocols, > > > > > > > > > > it could be an alternative to wildcarded principals. > > > > > > > > > > > > > > > > > > > > Let us know if we can help in any way to get this KIP > > updated > > > > and > > > > > > > > ready for > > > > > > > > > > voting to include in 2.0.0. > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, May 16, 2018 at 10:21 PM, Colin McCabe < > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Tue, May 15, 2018 at 7:18 PM, Rajini Sivaram < > > > > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Piyush, > > > > > > > > > > > > > > > > > > > > > > > > > > It is possible to configure PrincipalBuilder for > > SASL ( > > > > > > > > > > > > > https://cwiki.apache.org/ > > confluence/display/KAFKA/KIP- > > > > > > > > > > > > > 189%3A+Improve+principal+builder+interface+and+add+ > > > > > > > > > > support+for+SASL). > > > > > > > > > > > If > > > > > > > > > > > > > that satisfies your requirements, perhaps we can > move > > > > > > wildcarded > > > > > > > > > > > principals > > > > > > > > > > > > > out of this KIP and focus on wildcarded resources? > > > > > > > > > > > > > > > > > > > > > > +1. > > > > > > > > > > > > > > > > > > > > > > We also need to determine which characters will be > > reserved > > > > for > > > > > > the > > > > > > > > > > > future. I think previously we thought about @, #, $, > %, > > ^, > > > > &, > > > > > *. > > > > > > > > > > > > > > > > > > > > > > > > On Tue, May 15, 2018 at 7:15 PM, Piyush Vijay < > > > > > > > > > > piyushvij...@gmail.com> > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > >> Hi Colin, > > > > > > > > > > > > >> > > > > > > > > > > > > >> Escaping at this level is making sense to me but > let > > > me > > > > > > think > > > > > > > > more > > > > > > > > > > > and get > > > > > > > > > > > > >> back to you. > > > > > > > > > > > > > > > > > > > > > > Thanks, Piyush. What questions do you think are still > > open > > > > > > regarding > > > > > > > > > > > escape characters? > > > > > > > > > > > As Rajini mentioned, we have to get this in soon in > order > > > to > > > > > make > > > > > > > > the KIP > > > > > > > > > > > freeze. > > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > >> But should we not just get rid of one of > AclBinding > > or > > > > > > > > > > > AclBindingFilter > > > > > > > > > > > > >> then? Is there a reason to keep both given that > > > > > > > > AclBindingFilter and > > > > > > > > > > > > >> AclBinding look exact copy of each other after > this > > > > > change? > > > > > > This > > > > > > > > > > will > > > > > > > > > > > be a > > > > > > > > > > > > >> one-time breaking change in APIs marked as > > "Evolving", > > > > but > > > > > > makes > > > > > > > > > > > sense in > > > > > > > > > > > > >> the long term? Am I missing something here? > > > > > > > > > > > > > > > > > > > > > > AclBinding represents an ACL. AclBindingFilter is a > > filter > > > > > which > > > > > > > > can be > > > > > > > > > > > used to locate AclBinding objects. Similarly with > > Resource > > > > and > > > > > > > > > > > ResourceFilter. There is no reason to combine them > > because > > > > > they > > > > > > > > > > represent > > > > > > > > > > > different things. Although they contain many of the > same > > > > > fields, > > > > > > > > they > > > > > > > > > > are > > > > > > > > > > > not exact copies. Many fields can be null in > > > > > AclBindingFilter-- > > > > > > > > fields > > > > > > > > > > can > > > > > > > > > > > never be null in AclBinding. > > > > > > > > > > > > > > > > > > > > > > For example, you can have an AclBindingFilter that > > matches > > > > > every > > > > > > > > > > > AclBinding. There is more discussion of this on the > > > original > > > > > KIP > > > > > > > > that > > > > > > > > > > > added ACL support to AdminClient. > > > > > > > > > > > > > > > > > > > > > > best, > > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> Piyush Vijay > > > > > > > > > > > > >> > > > > > > > > > > > > >> On Tue, May 15, 2018 at 9:01 AM, Colin McCabe < > > > > > > > > cmcc...@apache.org> > > > > > > > > > > > wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > >> > Hi Piyush, > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > I think AclBinding should operate the same way > as > > > > > > > > > > AclBindingFilter. > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > So you should be able to do something like this: > > > > > > > > > > > > >> > > AclBindingFilter filter = new > > AclBindingFiler(new > > > > > > > > > > > > >> > ResourceFilter(ResourceType.GROUP, "foo*")) > > > > > > > > > > > > >> > > AclBinding binding = new AclBinding(new > > > > > > > > > > > Resource(ResourceType.GROUP, > > > > > > > > > > > > >> > "foo*")) > > > > > > > > > > > > >> > > assertTrue(filter.matches(binding)); > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > Thinking about this more, it's starting to feel > > > really > > > > > > messy > > > > > > > > to > > > > > > > > > > > create > > > > > > > > > > > > >> new > > > > > > > > > > > > >> > "pattern" constructors for Resource and > > > > > ResourceFilter. I > > > > > > > > don't > > > > > > > > > > > think > > > > > > > > > > > > >> > people will be able to figure this out. Maybe > we > > > > should > > > > > > just > > > > > > > > > > have a > > > > > > > > > > > > >> > limited compatibility break here, where it is > now > > > > > > required to > > > > > > > > > > escape > > > > > > > > > > > > >> weird > > > > > > > > > > > > >> > consumer group names when creating ACLs for > them. > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > To future-proof this, we should reserve a bunch > of > > > > > > characters > > > > > > > > at > > > > > > > > > > > once, > > > > > > > > > > > > >> > like *, @, $, %, ^, &, +, [, ], etc. If these > > > > > characters > > > > > > > > appear > > > > > > > > > > in > > > > > > > > > > > a > > > > > > > > > > > > >> > resource name, it should be an error, unless > they > > > are > > > > > > escaped > > > > > > > > > > with a > > > > > > > > > > > > >> > backslash. That way, we can use them in the > > future. > > > > We > > > > > > > > should > > > > > > > > > > > create a > > > > > > > > > > > > >> > Resource.escapeName function which adds the > > correct > > > > > escape > > > > > > > > > > > characters to > > > > > > > > > > > > >> > resource names (so it would translate foo* into > > > foo\*, > > > > > > foo+bar > > > > > > > > > > into > > > > > > > > > > > > >> > foo\+bar, etc. etc. > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > best, > > > > > > > > > > > > >> > Colin > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > On Mon, May 14, 2018, at 17:08, Piyush Vijay > > wrote: > > > > > > > > > > > > >> > > Colin, > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > createAcls take a AclBinding, however, instead > > of > > > > > > > > > > > AclBindingFilter. > > > > > > > > > > > > >> What > > > > > > > > > > > > >> > > are your thoughts here? > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > public abstract DescribeAclsResult > > > > > > > > describeAcls(AclBindingFilter > > > > > > > > > > > > >> > > filter, DescribeAclsOptions options); > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > public abstract CreateAclsResult > > > > > createAcls(Collection< > > > > > > > > > > > AclBinding> > > > > > > > > > > > > >> > > acls, CreateAclsOptions options); > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > public abstract DeleteAclsResult > > > > > > > > > > > > >> > > deleteAcls(Collection<AclBindingFilter> > > filters, > > > > > > > > > > > DeleteAclsOptions > > > > > > > > > > > > >> > > options); > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > Thanks > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > Piyush Vijay > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > On Mon, May 14, 2018 at 9:26 AM, Andy Coates < > > > > > > > > a...@confluent.io > > > > > > > > > > > > > > > > > > > > > > > >> wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > +1 > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > On 11 May 2018 at 17:14, Colin McCabe < > > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > Hi Andy, > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > I see what you mean. I guess my thought > > here > > > is > > > > > > that > > > > > > > > if the > > > > > > > > > > > > >> fields > > > > > > > > > > > > >> > are > > > > > > > > > > > > >> > > > > private, we can change it later if we need > > to. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > I definitely agree that we should use the > > > scheme > > > > > you > > > > > > > > > > describe > > > > > > > > > > > for > > > > > > > > > > > > >> > sending > > > > > > > > > > > > >> > > > > ACLs over the wire (just the string + > > version > > > > > > number) > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > cheers, > > > > > > > > > > > > >> > > > > Colin > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > On Fri, May 11, 2018, at 09:39, Andy > Coates > > > > wrote: > > > > > > > > > > > > >> > > > > > i think I'm agreeing with you. I was > > merely > > > > > > suggesting > > > > > > > > > > that > > > > > > > > > > > > >> having > > > > > > > > > > > > >> > an > > > > > > > > > > > > >> > > > > > additional field that controls how the > > > current > > > > > > field > > > > > > > > is > > > > > > > > > > > > >> > interpreted is > > > > > > > > > > > > >> > > > > more > > > > > > > > > > > > >> > > > > > flexible / extensible in the future than > > > > using a > > > > > > > > 'union' > > > > > > > > > > > style > > > > > > > > > > > > >> > > > approach, > > > > > > > > > > > > >> > > > > > where only one of several possible > fields > > > > should > > > > > > be > > > > > > > > > > > populated. > > > > > > > > > > > > >> But > > > > > > > > > > > > >> > > > it's a > > > > > > > > > > > > >> > > > > > minor thing. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > On 10 May 2018 at 09:29, Colin McCabe < > > > > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > > > > > > > > > >> wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > Hi Andy, > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > The issue that I was trying to solve > > here > > > is > > > > > the > > > > > > > > Java > > > > > > > > > > API. > > > > > > > > > > > > >> Right > > > > > > > > > > > > >> > > > now, > > > > > > > > > > > > >> > > > > > > someone can write "new > > > > > > ResourceFilter(ResourceType. > > > > > > > > > > > > >> > TRANSACTIONAL_ID, > > > > > > > > > > > > >> > > > > > > "foo*") and have a ResourceFilter that > > > > applies > > > > > > to a > > > > > > > > > > > > >> > Transactional ID > > > > > > > > > > > > >> > > > > named > > > > > > > > > > > > >> > > > > > > "foo*". This has to continue to work, > > or > > > > else > > > > > > we > > > > > > > > have > > > > > > > > > > > broken > > > > > > > > > > > > >> > > > > compatibility. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > I was proposing that there would be > > > > something > > > > > > like > > > > > > > > a new > > > > > > > > > > > > >> function > > > > > > > > > > > > >> > > > like > > > > > > > > > > > > >> > > > > > > ResourceFilter.fromPattern( > > > > > > > > > > ResourceType.TRANSACTIONAL_ID, > > > > > > > > > > > > >> > "foo*") > > > > > > > > > > > > >> > > > > which > > > > > > > > > > > > >> > > > > > > would create a ResourceFilter that > > applied > > > > to > > > > > > > > > > > transactional > > > > > > > > > > > > >> IDs > > > > > > > > > > > > >> > > > > starting > > > > > > > > > > > > >> > > > > > > with "foo", rather than transactional > > IDs > > > > > named > > > > > > > > "foo*" > > > > > > > > > > > > >> > specifically. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > I don't think it's important whether > the > > > > Java > > > > > > class > > > > > > > > has > > > > > > > > > > an > > > > > > > > > > > > >> > integer, > > > > > > > > > > > > >> > > > an > > > > > > > > > > > > >> > > > > > > enum, or two string fields. The > > important > > > > > > thing is > > > > > > > > that > > > > > > > > > > > > >> there's > > > > > > > > > > > > >> > a > > > > > > > > > > > > >> > > > new > > > > > > > > > > > > >> > > > > > > static function, or new constructor > > > > overload, > > > > > > etc. > > > > > > > > that > > > > > > > > > > > works > > > > > > > > > > > > >> for > > > > > > > > > > > > >> > > > > patterns > > > > > > > > > > > > >> > > > > > > rather than literal strings. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > On Thu, May 10, 2018, at 03:30, Andy > > > Coates > > > > > > wrote: > > > > > > > > > > > > >> > > > > > > > Rather than having name and pattern > > > fields > > > > > on > > > > > > the > > > > > > > > > > > > >> > ResourceFilter, > > > > > > > > > > > > >> > > > > where > > > > > > > > > > > > >> > > > > > > > it’s only valid for one to be set, > and > > > we > > > > > > want to > > > > > > > > > > > restrict > > > > > > > > > > > > >> the > > > > > > > > > > > > >> > > > > character > > > > > > > > > > > > >> > > > > > > > set in case future enhancements need > > > them, > > > > > we > > > > > > > > could > > > > > > > > > > > instead > > > > > > > > > > > > >> > add a > > > > > > > > > > > > >> > > > new > > > > > > > > > > > > >> > > > > > > > integer ‘nameType’ field, and use > > > > constants > > > > > to > > > > > > > > > > indicate > > > > > > > > > > > how > > > > > > > > > > > > >> the > > > > > > > > > > > > >> > > > name > > > > > > > > > > > > >> > > > > > > > field should be interpreted, e.g. 0 > = > > > > > > literal, 1 = > > > > > > > > > > > wildcard. > > > > > > > > > > > > >> > This > > > > > > > > > > > > >> > > > > would > > > > > > > > > > > > >> > > > > > > > be extendable, e.g we can later add > 2 > > = > > > > > > regex, or > > > > > > > > what > > > > > > > > > > > ever, > > > > > > > > > > > > >> > and > > > > > > > > > > > > >> > > > > > > > wouldn’t require any escaping. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > This is very user-unfriendly, though. > > > Users > > > > > > don't > > > > > > > > want > > > > > > > > > > to > > > > > > > > > > > > >> have > > > > > > > > > > > > >> > to > > > > > > > > > > > > >> > > > > > > explicitly supply a version number > when > > > > using > > > > > > the > > > > > > > > API, > > > > > > > > > > > which > > > > > > > > > > > > >> is > > > > > > > > > > > > >> > what > > > > > > > > > > > > >> > > > > this > > > > > > > > > > > > >> > > > > > > would force them to do. I don't think > > > users > > > > > are > > > > > > > > going > > > > > > > > > > to > > > > > > > > > > > > >> want to > > > > > > > > > > > > >> > > > > memorize > > > > > > > > > > > > >> > > > > > > that version 4 supprted "+", whereas > > > > version 3 > > > > > > only > > > > > > > > > > > supported > > > > > > > > > > > > >> > > > "[0-9]", > > > > > > > > > > > > >> > > > > or > > > > > > > > > > > > >> > > > > > > whatever. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > Just as an example, do you remember > > which > > > > > > versions > > > > > > > > of > > > > > > > > > > > > >> > FetchRequest > > > > > > > > > > > > >> > > > > added > > > > > > > > > > > > >> > > > > > > which features? I don't. I always > have > > > to > > > > > > look at > > > > > > > > the > > > > > > > > > > > code > > > > > > > > > > > > >> to > > > > > > > > > > > > >> > > > > remember. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > Also, escaping is still required any > > time > > > > you > > > > > > > > overload a > > > > > > > > > > > > >> > character to > > > > > > > > > > > > >> > > > > mean > > > > > > > > > > > > >> > > > > > > two things. Escaping is required in > the > > > > > current > > > > > > > > > > proposal > > > > > > > > > > > to > > > > > > > > > > > > >> be > > > > > > > > > > > > >> > able > > > > > > > > > > > > >> > > > to > > > > > > > > > > > > >> > > > > > > create a pattern that matches only > > "foo*". > > > > > You > > > > > > > > have to > > > > > > > > > > > type > > > > > > > > > > > > >> > "foo\*" > > > > > > > > > > > > >> > > > > It > > > > > > > > > > > > >> > > > > > > would be required if we forced users > to > > > > > specify > > > > > > a > > > > > > > > > > > version, as > > > > > > > > > > > > >> > well. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > best, > > > > > > > > > > > > >> > > > > > > Colin > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > > > Sent from my iPhone > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > > > > On 7 May 2018, at 05:16, Piyush > > Vijay > > > < > > > > > > > > > > > > >> > piyushvij...@gmail.com> > > > > > > > > > > > > >> > > > > wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > Makes sense. I'll update the KIP. > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > Does anyone have any other > comments? > > > :) > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > Thanks > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > Piyush Vijay > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > >> On Thu, May 3, 2018 at 11:55 AM, > > > Colin > > > > > > McCabe < > > > > > > > > > > > > >> > > > cmcc...@apache.org > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > wrote: > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> Yeah, I guess that's a good > point. > > > It > > > > > > probably > > > > > > > > > > makes > > > > > > > > > > > > >> sense > > > > > > > > > > > > >> > to > > > > > > > > > > > > >> > > > > > > support the > > > > > > > > > > > > >> > > > > > > > >> prefix scheme for consumer groups > > and > > > > > > > > transactional > > > > > > > > > > > IDs > > > > > > > > > > > > >> as > > > > > > > > > > > > >> > well > > > > > > > > > > > > >> > > > as > > > > > > > > > > > > >> > > > > > > topics. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> I agree that the current > situation > > > > where > > > > > > > > anything > > > > > > > > > > > goes in > > > > > > > > > > > > >> > > > consumer > > > > > > > > > > > > >> > > > > > > group > > > > > > > > > > > > >> > > > > > > > >> names and transactional ID names > is > > > not > > > > > > > > ideal. I > > > > > > > > > > > wish we > > > > > > > > > > > > >> > could > > > > > > > > > > > > >> > > > > > > rewind the > > > > > > > > > > > > >> > > > > > > > >> clock and impose restrictions on > > the > > > > > names. > > > > > > > > > > > However, it > > > > > > > > > > > > >> > doesn't > > > > > > > > > > > > >> > > > > seem > > > > > > > > > > > > >> > > > > > > > >> practical at the moment. Adding > > new > > > > > > > > restrictions > > > > > > > > > > > would > > > > > > > > > > > > >> > break a > > > > > > > > > > > > >> > > > > lot of > > > > > > > > > > > > >> > > > > > > > >> existing users after an upgrade. > > It > > > > > would > > > > > > be a > > > > > > > > > > > really > > > > > > > > > > > > >> bad > > > > > > > > > > > > >> > > > upgrade > > > > > > > > > > > > >> > > > > > > > >> experience. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> However, I think we can support > > this > > > > in a > > > > > > > > > > compatible > > > > > > > > > > > way. > > > > > > > > > > > > >> > From > > > > > > > > > > > > >> > > > > the > > > > > > > > > > > > >> > > > > > > > >> perspective of AdminClient, we > just > > > > have > > > > > to > > > > > > > > add a > > > > > > > > > > new > > > > > > > > > > > > >> field > > > > > > > > > > > > >> > to > > > > > > > > > > > > >> > > > > > > > >> ResourceFilter. Currently, it > has > > > two > > > > > > fields, > > > > > > > > > > > > >> resourceType > > > > > > > > > > > > >> > and > > > > > > > > > > > > >> > > > > name: > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >>> /** > > > > > > > > > > > > >> > > > > > > > >>> * A filter which matches > Resource > > > > > objects. > > > > > > > > > > > > >> > > > > > > > >>> * > > > > > > > > > > > > >> > > > > > > > >>> * The API for this class is > still > > > > > evolving > > > > > > > > and we > > > > > > > > > > > may > > > > > > > > > > > > >> break > > > > > > > > > > > > >> > > > > > > > >> compatibility in minor releases, > if > > > > > > necessary. > > > > > > > > > > > > >> > > > > > > > >>> */ > > > > > > > > > > > > >> > > > > > > > >>> @InterfaceStability.Evolving > > > > > > > > > > > > >> > > > > > > > >>> public class ResourceFilter { > > > > > > > > > > > > >> > > > > > > > >>> private final ResourceType > > > > > > resourceType; > > > > > > > > > > > > >> > > > > > > > >>> private final String name; > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> We can add a third field, > pattern. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> So the API will basically be, if > I > > > > > create a > > > > > > > > > > > > >> > > > > > > ResourceFilter(resourceType=GROUP, > > > > > > > > > > > > >> > > > > > > > >> name=foo*, pattern=null), it > > applies > > > > only > > > > > > to > > > > > > > > the > > > > > > > > > > > consumer > > > > > > > > > > > > >> > group > > > > > > > > > > > > >> > > > > named > > > > > > > > > > > > >> > > > > > > > >> "foo*". If I create a > > > > > > > > > > ResourceFilter(resourceType=GR > > > > > > > > > > > > >> OUP, > > > > > > > > > > > > >> > > > > name=null, > > > > > > > > > > > > >> > > > > > > > >> pattern=foo*), it applies to any > > > > consumer > > > > > > group > > > > > > > > > > > starting > > > > > > > > > > > > >> in > > > > > > > > > > > > >> > > > "foo". > > > > > > > > > > > > >> > > > > > > name > > > > > > > > > > > > >> > > > > > > > >> and pattern cannot be both set at > > the > > > > > same > > > > > > > > time. > > > > > > > > > > > This > > > > > > > > > > > > >> > preserves > > > > > > > > > > > > >> > > > > > > > >> compatibility at the AdminClient > > > level. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> It's possible that we will want > to > > > add > > > > > more > > > > > > > > types > > > > > > > > > > of > > > > > > > > > > > > >> > pattern in > > > > > > > > > > > > >> > > > > the > > > > > > > > > > > > >> > > > > > > > >> future. So we should reserve > > > "special > > > > > > > > characters" > > > > > > > > > > > such > > > > > > > > > > > > >> as > > > > > > > > > > > > >> > +, /, > > > > > > > > > > > > >> > > > > &, > > > > > > > > > > > > >> > > > > > > %, #, > > > > > > > > > > > > >> > > > > > > > >> $, etc. These characters should > be > > > > > > treated as > > > > > > > > > > > special > > > > > > > > > > > > >> > unless > > > > > > > > > > > > >> > > > > they are > > > > > > > > > > > > >> > > > > > > > >> prefixed with a backslash to > escape > > > > them. > > > > > > This > > > > > > > > > > will > > > > > > > > > > > > >> allow > > > > > > > > > > > > >> > us to > > > > > > > > > > > > >> > > > > add > > > > > > > > > > > > >> > > > > > > > >> support for using these > characters > > in > > > > the > > > > > > > > future > > > > > > > > > > > without > > > > > > > > > > > > >> > > > breaking > > > > > > > > > > > > >> > > > > > > > >> compatibility. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> At the protocol level, we need a > > new > > > > API > > > > > > > > version > > > > > > > > > > for > > > > > > > > > > > > >> > > > > > > CreateAclsRequest / > > > > > > > > > > > > >> > > > > > > > >> DeleteAclsRequest. The new API > > > version > > > > > > will > > > > > > > > send > > > > > > > > > > all > > > > > > > > > > > > >> > special > > > > > > > > > > > > >> > > > > > > characters > > > > > > > > > > > > >> > > > > > > > >> over the wire escaped rather than > > > > > directly. > > > > > > > > (So > > > > > > > > > > > there > > > > > > > > > > > > >> is no > > > > > > > > > > > > >> > > > need > > > > > > > > > > > > >> > > > > for > > > > > > > > > > > > >> > > > > > > the > > > > > > > > > > > > >> > > > > > > > >> equivalent of both "name" and > > > > "pattern"-- > > > > > > we > > > > > > > > > > > translate > > > > > > > > > > > > >> name > > > > > > > > > > > > >> > > > into a > > > > > > > > > > > > >> > > > > > > validly > > > > > > > > > > > > >> > > > > > > > >> escaped pattern that matches only > > one > > > > > > thing, by > > > > > > > > > > > adding > > > > > > > > > > > > >> > escape > > > > > > > > > > > > >> > > > > > > characters as > > > > > > > > > > > > >> > > > > > > > >> appropriate.) The broker will > > > validate > > > > > the > > > > > > > > new API > > > > > > > > > > > > >> version > > > > > > > > > > > > >> > and > > > > > > > > > > > > >> > > > > reject > > > > > > > > > > > > >> > > > > > > > >> malformed of unsupported > patterns. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> At the ZK level, we can > introduce a > > > > > > protocol > > > > > > > > > > version > > > > > > > > > > > to > > > > > > > > > > > > >> the > > > > > > > > > > > > >> > data > > > > > > > > > > > > >> > > > > in > > > > > > > > > > > > >> > > > > > > ZK-- > > > > > > > > > > > > >> > > > > > > > >> or store it under a different > root. > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> best, > > > > > > > > > > > > >> > > > > > > > >> Colin > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >>> On Wed, May 2, 2018, at 18:09, > > > Piyush > > > > > > Vijay > > > > > > > > wrote: > > > > > > > > > > > > >> > > > > > > > >>> Thank you everyone for the > > interest > > > > and, > > > > > > > > prompt > > > > > > > > > > and > > > > > > > > > > > > >> > valuable > > > > > > > > > > > > >> > > > > > > feedback. I > > > > > > > > > > > > >> > > > > > > > >>> really appreciate the quick > > > > turnaround. > > > > > > I’ve > > > > > > > > tried > > > > > > > > > > > to > > > > > > > > > > > > >> > organize > > > > > > > > > > > > >> > > > > the > > > > > > > > > > > > >> > > > > > > > >> comments > > > > > > > > > > > > >> > > > > > > > >>> into common headings. See my > > replies > > > > > > below: > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Case of ‘*’ might already be > > > present > > > > in > > > > > > > > consumer > > > > > > > > > > > groups > > > > > > > > > > > > >> > and > > > > > > > > > > > > >> > > > > > > > >> transactional > > > > > > > > > > > > >> > > > > > > > >>> ids* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - We definitely need wildcard > > ACLs > > > > > > support > > > > > > > > for > > > > > > > > > > > > >> resources > > > > > > > > > > > > >> > like > > > > > > > > > > > > >> > > > > > > consumer > > > > > > > > > > > > >> > > > > > > > >>> groups and transactional ids > for > > > the > > > > > > reason > > > > > > > > Andy > > > > > > > > > > > > >> > mentioned. A > > > > > > > > > > > > >> > > > > big > > > > > > > > > > > > >> > > > > > > win > > > > > > > > > > > > >> > > > > > > > >> of > > > > > > > > > > > > >> > > > > > > > >>> this feature is that service > > > > providers > > > > > > don’t > > > > > > > > > > have > > > > > > > > > > > to > > > > > > > > > > > > >> > track > > > > > > > > > > > > >> > > > and > > > > > > > > > > > > >> > > > > keep > > > > > > > > > > > > >> > > > > > > > >>> up-to-date all the consumer > > groups > > > > > their > > > > > > > > > > > customers are > > > > > > > > > > > > >> > using. > > > > > > > > > > > > >> > > > > > > > >>> - I agree with Andy’s thoughts > > on > > > > the > > > > > > two > > > > > > > > > > possible > > > > > > > > > > > > >> ways. > > > > > > > > > > > > >> > > > > > > > >>> - My vote would be to do the > > > > breaking > > > > > > change > > > > > > > > > > > because > > > > > > > > > > > > >> we > > > > > > > > > > > > >> > > > should > > > > > > > > > > > > >> > > > > > > > >> restrict > > > > > > > > > > > > >> > > > > > > > >>> the format of consumer groups > > and > > > > > > > > transactional > > > > > > > > > > > ids > > > > > > > > > > > > >> > sooner > > > > > > > > > > > > >> > > > than > > > > > > > > > > > > >> > > > > > > later. > > > > > > > > > > > > >> > > > > > > > >>> - Consumer groups and > > > > transactional > > > > > > ids > > > > > > > > are > > > > > > > > > > > basic > > > > > > > > > > > > >> > Kafka > > > > > > > > > > > > >> > > > > > > concepts. > > > > > > > > > > > > >> > > > > > > > >>> There is a lot of value in > > > > having a > > > > > > > > defined > > > > > > > > > > > naming > > > > > > > > > > > > >> > > > > convention on > > > > > > > > > > > > >> > > > > > > > >> these > > > > > > > > > > > > >> > > > > > > > >>> concepts. > > > > > > > > > > > > >> > > > > > > > >>> - This will help us not run > > > into > > > > > more > > > > > > > > issues > > > > > > > > > > > down > > > > > > > > > > > > >> the > > > > > > > > > > > > >> > > > line. > > > > > > > > > > > > >> > > > > > > > >>> - I’m not sure if people > > > actually > > > > > use > > > > > > > > ‘*’ in > > > > > > > > > > > their > > > > > > > > > > > > >> > > > consumer > > > > > > > > > > > > >> > > > > > > group > > > > > > > > > > > > >> > > > > > > > >>> names anyway. > > > > > > > > > > > > >> > > > > > > > >>> - Escaping ‘*’ isn’t > trivial > > > > > because > > > > > > ‘\’ > > > > > > > > is > > > > > > > > > > an > > > > > > > > > > > > >> allowed > > > > > > > > > > > > >> > > > > character > > > > > > > > > > > > >> > > > > > > > >> too. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Why introduce two new APIs?* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - It’s possible to make this > > > change > > > > > > without > > > > > > > > > > > > >> introducing > > > > > > > > > > > > >> > new > > > > > > > > > > > > >> > > > > APIs > > > > > > > > > > > > >> > > > > > > but > > > > > > > > > > > > >> > > > > > > > >> new > > > > > > > > > > > > >> > > > > > > > >>> APIs are required for > > inspection. > > > > > > > > > > > > >> > > > > > > > >>> - For example: If I want to > > fetch > > > > all > > > > > > ACLs > > > > > > > > that > > > > > > > > > > > match > > > > > > > > > > > > >> > > > > ’topicA*’, > > > > > > > > > > > > >> > > > > > > it’s > > > > > > > > > > > > >> > > > > > > > >>> not possible without > introducing > > > new > > > > > > APIs > > > > > > > > AND > > > > > > > > > > > > >> maintaining > > > > > > > > > > > > >> > > > > backwards > > > > > > > > > > > > >> > > > > > > > >>> compatibility. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Matching multiple hosts* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - Rajini is right that > wildcard > > > ACLs > > > > > > aren’t > > > > > > > > the > > > > > > > > > > > > >> correct > > > > > > > > > > > > >> > fit > > > > > > > > > > > > >> > > > for > > > > > > > > > > > > >> > > > > > > > >>> specifying range of hosts. > > > > > > > > > > > > >> > > > > > > > >>> - We will rely on KIP-252 for > > the > > > > > proper > > > > > > > > > > > > >> functionality ( > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> https://cwiki.apache.org/ > > > > > > > > > > > confluence/display/KAFKA/KIP- > > > > > > > > > > > > >> > > > > > > > >> 252+-+Extend+ACLs+to+allow+ > > > > > > > > > > > filtering+based+on+ip+ranges+ > > > > > > > > > > > > >> > > > > and+subnets > > > > > > > > > > > > >> > > > > > > > >>> ) > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Implementation of matching > > > algorithm > > > > > and > > > > > > > > > > > performance > > > > > > > > > > > > >> > concerns* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - Updated the KIP with an > > > > > > implementation. > > > > > > > > > > > > >> > > > > > > > >>> - Andy, you’re right. The > length > > > > > doesn’t > > > > > > > > play a > > > > > > > > > > > part. > > > > > > > > > > > > >> The > > > > > > > > > > > > >> > > > > request > > > > > > > > > > > > >> > > > > > > will > > > > > > > > > > > > >> > > > > > > > >>> be authorized *iff* there is > at > > > > least > > > > > > one > > > > > > > > > > matching > > > > > > > > > > > > >> ALLOW > > > > > > > > > > > > >> > and > > > > > > > > > > > > >> > > > no > > > > > > > > > > > > >> > > > > > > > >> matching > > > > > > > > > > > > >> > > > > > > > >>> DENY irrespective of the > prefix > > > > > length. > > > > > > > > Included > > > > > > > > > > > this > > > > > > > > > > > > >> > detail > > > > > > > > > > > > >> > > > > in the > > > > > > > > > > > > >> > > > > > > > >> KIP. > > > > > > > > > > > > >> > > > > > > > >>> - Since everything is stored > in > > > > > memory, > > > > > > the > > > > > > > > > > > > >> performance > > > > > > > > > > > > >> > hit > > > > > > > > > > > > >> > > > > isn’t > > > > > > > > > > > > >> > > > > > > > >> really > > > > > > > > > > > > >> > > > > > > > >>> significantly worse than the > > > current > > > > > > > > behavior. > > > > > > > > > > > > >> > > > > > > > >>> - Stephane’s performance > > > improvement > > > > > > > > suggestion > > > > > > > > > > > is a > > > > > > > > > > > > >> > great > > > > > > > > > > > > >> > > > > idea but > > > > > > > > > > > > >> > > > > > > > >>> orthogonal. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Why extend this for principal?* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - Thanks for the amazing > points > > > > > Rajini. > > > > > > > > > > > > >> > > > > > > > >>> - There is a use case where > ALL > > > > > > principals > > > > > > > > from > > > > > > > > > > > an org > > > > > > > > > > > > >> > might > > > > > > > > > > > > >> > > > > want > > > > > > > > > > > > >> > > > > > > to > > > > > > > > > > > > >> > > > > > > > >>> access fix set of topics. > > > > > > > > > > > > >> > > > > > > > >>> - User group functionality is > > > > > currently > > > > > > > > missing. > > > > > > > > > > > > >> > > > > > > > >>> - IIRC SASL doesn’t use custom > > > > > principal > > > > > > > > > > builder. > > > > > > > > > > > > >> > > > > > > > >>> - However, prefixing is not > the > > > > right > > > > > > > > choice in > > > > > > > > > > > all > > > > > > > > > > > > >> > cases. > > > > > > > > > > > > >> > > > > Agreed. > > > > > > > > > > > > >> > > > > > > > >>> - Thoughts? > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> *Changes to AdminClient to > support > > > > > > wildcard > > > > > > > > ACLs* > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - Thanks Colin for the > > > > implementation. > > > > > > It’s > > > > > > > > good > > > > > > > > > > > to > > > > > > > > > > > > >> have > > > > > > > > > > > > >> > you > > > > > > > > > > > > >> > > > > and > > > > > > > > > > > > >> > > > > > > > >>> others > > > > > > > > > > > > >> > > > > > > > >>> here for the expert opinions. > > > > > > > > > > > > >> > > > > > > > >>> - The current implementation > > uses > > > > two > > > > > > > > classes: > > > > > > > > > > > > >> > AclBinding and > > > > > > > > > > > > >> > > > > > > > >>> AclBindingFilter. ( > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> https://github.com/apache/ > > > > > > > > > > > kafka/blob/trunk/clients/src/ > > > > > > > > > > > > >> > > > > > > > >> main/java/org/apache/kafka/ > > > > > > > > > > > common/acl/AclBinding.java > > > > > > > > > > > > >> > > > > > > > >>> and > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> https://github.com/apache/ > > > > > > > > > > > kafka/blob/trunk/clients/src/ > > > > > > > > > > > > >> > > > > > > > >> main/java/org/apache/kafka/com > > > > > > > > > > > > >> mon/acl/AclBindingFilter.java > > > > > > > > > > > > >> > > > > > > > >>> ) > > > > > > > > > > > > >> > > > > > > > >>> - AclBinding is definition of > an > > > > Acl. > > > > > > It’s > > > > > > > > used > > > > > > > > > > to > > > > > > > > > > > > >> create > > > > > > > > > > > > >> > > > ACLs. > > > > > > > > > > > > >> > > > > > > > >>> - AclBindingFilter is used to > > > fetch > > > > or > > > > > > > > delete > > > > > > > > > > > > >> “matching’ > > > > > > > > > > > > >> > > > ACLs. > > > > > > > > > > > > >> > > > > > > > >>> - In the context of wildcard > > > > suffixed > > > > > > ACLs, > > > > > > > > a > > > > > > > > > > > stored > > > > > > > > > > > > >> ACL > > > > > > > > > > > > >> > may > > > > > > > > > > > > >> > > > > have > > > > > > > > > > > > >> > > > > > > ‘*’ > > > > > > > > > > > > >> > > > > > > > >>> in > > > > > > > > > > > > >> > > > > > > > >>> it. *It really removes the > > > > distinction > > > > > > > > between > > > > > > > > > > > these > > > > > > > > > > > > >> two > > > > > > > > > > > > >> > > > > classes.* > > > > > > > > > > > > >> > > > > > > > >>> - The current implementation > > uses > > > > > > ‘null’ to > > > > > > > > > > define > > > > > > > > > > > > >> > wildcard > > > > > > > > > > > > >> > > > ACL > > > > > > > > > > > > >> > > > > > > > >>> (‘*’). I > > > > > > > > > > > > >> > > > > > > > >>> think it’s not a good pattern > > and > > > we > > > > > > should > > > > > > > > use > > > > > > > > > > > ‘*’ > > > > > > > > > > > > >> for > > > > > > > > > > > > >> > the > > > > > > > > > > > > >> > > > > > > wildcard > > > > > > > > > > > > >> > > > > > > > >>> ACL ( > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> https://github.com/apache/ > > > > > > > > > > > kafka/blob/trunk/clients/src/ > > > > > > > > > > > > >> > > > > > > > >> main/java/org/apache/kafka/ > > > > > > > > > > > common/acl/AclBindingFilter. > > > > > > > > > > > > >> > java#L39 > > > > > > > > > > > > >> > > > > > > > >>> and > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> https://github.com/apache/ > > > > > > > > > > > kafka/blob/trunk/clients/src/ > > > > > > > > > > > > >> > > > > > > > >> main/java/org/apache/kafka/ > > > > > > common/resource/ > > > > > > > > > > > > >> > > > > ResourceFilter.java#L37 > > > > > > > > > > > > >> > > > > > > > >>> ). > > > > > > > > > > > > >> > > > > > > > >>> - However, the above two > changes > > > are > > > > > > > > breaking > > > > > > > > > > > change > > > > > > > > > > > > >> but > > > > > > > > > > > > >> > it > > > > > > > > > > > > >> > > > > should > > > > > > > > > > > > >> > > > > > > be > > > > > > > > > > > > >> > > > > > > > >>> acceptable because the API is > > > marked > > > > > > with > > > > > > > > > > > > >> > > > > > > > >>> @InterfaceStability.Evolving. > > > > > > > > > > > > >> > > > > > > > >>> - If everyone agrees to the > > above > > > > two > > > > > > > > changes > > > > > > > > > > > (merging > > > > > > > > > > > > >> > the > > > > > > > > > > > > >> > > > two > > > > > > > > > > > > >> > > > > > > > >>> classes > > > > > > > > > > > > >> > > > > > > > >>> and using non-null values for > > > > blanket > > > > > > > > access), > > > > > > > > > > the > > > > > > > > > > > > >> only > > > > > > > > > > > > >> > other > > > > > > > > > > > > >> > > > > > > change > > > > > > > > > > > > >> > > > > > > > >>> is > > > > > > > > > > > > >> > > > > > > > >>> using the matching algorithm > > from > > > > the > > > > > > KIP to > > > > > > > > > > match > > > > > > > > > > > > >> ACLs. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> Other comments: > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - > It may be worth excluding > > > > > delegation > > > > > > > > token > > > > > > > > > > > ACLs > > > > > > > > > > > > >> from > > > > > > > > > > > > >> > > > using > > > > > > > > > > > > >> > > > > > > > >> prefixed > > > > > > > > > > > > >> > > > > > > > >>> wildcards since it doesn't > make > > > much > > > > > > sense. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> I want to ask for clarification > on > > > > what > > > > > > > > delegation > > > > > > > > > > > token > > > > > > > > > > > > >> > ACLs > > > > > > > > > > > > >> > > > are > > > > > > > > > > > > >> > > > > > > before > > > > > > > > > > > > >> > > > > > > > >>> commenting. Wildcard suffixed > ACLs > > > are > > > > > > > > supported > > > > > > > > > > > only > > > > > > > > > > > > >> for > > > > > > > > > > > > >> > > > > resource > > > > > > > > > > > > >> > > > > > > and > > > > > > > > > > > > >> > > > > > > > >>> principal names. > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> > > > > > > > > > > > > >> > > > > > > > >>> - A quick read makes me > believe > > > that > > > > > > I’ve > > > > > > > > fixed > > > > > > > > > > > the > > > > > > > > > > > > >> > > > formatting > > > > > > > > > > > > >> > > > > > > issues > > > > > > > > > > > > >> > > > > > > > >>> reported by Ted. Let me know > if > > > > >
