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 > something is > > > > > still > > > > > > > >> wrong > > > > > > > >> > and > > > > > > > >> > > > I > > > > > > > >> > > > > > > would > > > > > > > >> > > > > > > > >> be > > > > > > > >> > > > > > > > >>> happy to fix it. > > > > > > > >> > > > > > > > >>> - I’ve fixed the mismatch in signature > > > reported > > > > > by > > > > > > > >> Ron. > > > > > > > >> > > > > > > > >>> - Andy, I’ve updated the KIP with the > > > security > > > > > > hole > > > > > > > >> > related > > > > > > > >> > > > to > > > > > > > >> > > > > DENY > > > > > > > >> > > > > > > > >>> wildcard ACLs ‘*’ on the downgrade path. > > > > > > > >> > > > > > > > >>> - Wrt naming, wildcard suffixed ACLs > sound > > > > > > reasonable > > > > > > > >> to > > > > > > > >> > me > > > > > > > >> > > > > until > > > > > > > >> > > > > > > > >>> someone raise a major objection. > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> Let me know your thoughts. Looking > forward to > > > the > > > > > > next > > > > > > > >> > > > iteration. > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> Best, > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> Piyush > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> Piyush Vijay > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>> On Wed, May 2, 2018 at 1:33 PM, Andy > Coates < > > > > > > > >> > a...@confluent.io > > > > > > > >> > > > > > > > > > > > >> > > > > > > wrote: > > > > > > > >> > > > > > > > >>> > > > > > > > >> > > > > > > > >>>>> Perhaps there is a simpler way. It > seems > > > like > > > > > the > > > > > > > >> > resource > > > > > > > >> > > > > that > > > > > > > >> > > > > > > > >> people > > > > > > > >> > > > > > > > >>>> really want to use prefixed ACLs with is > > > topic > > > > > > names. > > > > > > > >> > Because > > > > > > > >> > > > > topic > > > > > > > >> > > > > > > > >> names > > > > > > > >> > > > > > > > >>>> can't contain "*", there is no ambiguity > > > there, > > > > > > > >> either. I > > > > > > > >> > > > think > > > > > > > >> > > > > > > maybe > > > > > > > >> > > > > > > > >> we > > > > > > > >> > > > > > > > >>>> should restrict the prefix handling to > topic > > > > > > resources. > > > > > > > >> > > > > > > > >>>> > > > > > > > >> > > > > > > > >>>> The KIP should cover consumer groups for > > > sure, > > > > > > > >> otherwise > > > > > > > >> > we're > > > > > > > >> > > > > back > > > > > > > >> > > > > > > to > > > > > > > >> > > > > > > > >> the > > > > > > > >> > > > > > > > >>>> situation users need to know, up front, > the > > > set > > > > > of > > > > > > > >> > consumer > > > > > > > >> > > > > groups > > > > > > > >> > > > > > > an > > > > > > > >> > > > > > > > >>>> KStreams topology is going to use. > > > > > > > >> > > > > > > > >>>> > > > > > > > >> > > > > > > > >>>> I'm assuming transactional producer Ids > > > would be > > > > > > the > > > > > > > >> same. > > > > > > > >> > > > > > > > >>>> > > > > > > > >> > > > > > > > >>>> The cleanest solution would be to > restrict > > > the > > > > > > > >> characters > > > > > > > >> > in > > > > > > > >> > > > > group > > > > > > > >> > > > > > > and > > > > > > > >> > > > > > > > >>>> transaction producer ids, but that's a > > > breaking > > > > > > change > > > > > > > >> > that > > > > > > > >> > > > > might > > > > > > > >> > > > > > > > >> affect a > > > > > > > >> > > > > > > > >>>> lot of people. > > > > > > > >> > > > > > > > >>>> > > > > > > > >> > > > > > > > >>>> Another solution might be to add a > protocol > > > > > > version to > > > > > > > >> the > > > > > > > >> > > > `Acl` > > > > > > > >> > > > > > > type, > > > > > > > >> > > > > > > > >> (not > > > > > > > >> > > > > > > > >>>> the current `version` field used for > > > optimistic > > > > > > > >> > concurrency > > > > > > > >> > > > > > > control), > > > > > > > >> > > > > > > > >>>> defaulting it to version 1 if it is not > > > present, > > > > > > and > > > > > > > >> > releasing > > > > > > > >> > > > > this > > > > > > > >> > > > > > > > >> change > > > > > > > >> > > > > > > > >>>> as version 2. This would at least allow > us > > > to > > > > > > leave > > > > > > > >> the > > > > > > > >> > > > > version 1 > > > > > > > >> > > > > > > ACLs > > > > > > > >> > > > > > > > >>>> as-is, (which makes for a cleaner storey > if a > > > > > > cluster > > > > > > > >> is > > > > > > > >> > > > > > > downgraded). > > > > > > > >> > > > > > > > >> There > > > > > > > >> > > > > > > > >>>> is then potential to escape '*' when > writing > > > > > > version 2 > > > > > > > >> > ACLs. > > > > > > > >> > > > > (And > > > > > > > >> > > > > > > > >> introduce > > > > > > > >> > > > > > > > >>>> code to check for supported ACL versions > > > going > > > > > > > >> forward). > > > > > > > >> > > > > Though... > > > > > > > >> > > > > > > how > > > > > > > >> > > > > > > > >> do > > > > > > > >> > > > > > > > >>>> we escape? If the consumer group is free > > > form, > > > > > > then any > > > > > > > >> > escape > > > > > > > >> > > > > > > > >> sequence is > > > > > > > >> > > > > > > > >>>> also valid. Aren't there metrics that > use > > > the > > > > > > group > > > > > > > >> > name? > > > > > > > >> > > > If > > > > > > > >> > > > > > > there > > > > > > > >> > > > > > > > >> are, > > > > > > > >> > > > > > > > >>>> I'd of thought we'd need to restrict the > > > char set > > > > > > > >> anyway. > > > > > > > >> > > > > > > > >>>> > > > > > > > >> > > > > > > > >>>>> On 2 May 2018 at 18:57, charly molter < > > > > > > > >> > > > charly.mol...@gmail.com > > > > > > > >> > > > > > > > > > > > > >> > > > > > > wrote: > > > > > > > >> > > > > > > > >>>>> > > > > > > > >> > > > > > > > >>>>> The fact that consumer groups and > > > > > > > >> transactionalProducerId > > > > > > > >> > > > don't > > > > > > > >> > > > > > > have > > > > > > > >> > > > > > > > >> a > > > > > > > >> > > > > > > > >>>>> strict format is problematic (we have > > > problems > > > > > > with > > > > > > > >> > people > > > > > > > >> > > > with > > > > > > > >> > > > > > > empty > > > > > > > >> > > > > > > > >>>>> spaces at the end of their consumer > group > > > for > > > > > > > >> example). > > > > > > > >> > > > > > > > >>>>> With 2.0 around the corner and the > > > possibility > > > > > to > > > > > > fix > > > > > > > >> > these > > > > > > > >> > > > > errors > > > > > > > >> > > > > > > > >> from > > > > > > > >> > > > > > > > >>>> the > > > > > > > >> > > > > > > > >>>>> past should we create a KIP to restrict > > > these > > > > > > names > > > > > > > >> like > > > > > > > >> > we > > > > > > > >> > > > do > > > > > > > >> > > > > for > > > > > > > >> > > > > > > > >>>> topics? > > > > > > > >> > > > > > > > >>>>> > > > > > > > >> > > > > > > > >>>>> Thanks! > > > > > > > >> > > > > > > > >>>>> Charly > > > > > > > >> > > > > > > > >>>>> > > > > > > > >> > > > > > > > >>>>> On Wed, May 2, 2018 at 6:22 PM Colin > McCabe > > > < > > > > > > > >> > > > > cmcc...@apache.org> > > > > > > > >> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >>>>> > > > > > > > >> > > > > > > > >>>>>> Hi Piyush, > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> Thanks for the KIP! It seems like it > will > > > be > > > > > > really > > > > > > > >> > useful. > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> As Rajini commented, the names for some > > > > > resources > > > > > > > >> (such > > > > > > > >> > as > > > > > > > >> > > > > > > consumer > > > > > > > >> > > > > > > > >>>>>> groups) can include stars. So your > > > consumer > > > > > > group > > > > > > > >> > might be > > > > > > > >> > > > > named > > > > > > > >> > > > > > > > >>>> "foo*". > > > > > > > >> > > > > > > > >>>>>> We need a way of explicitly referring > to > > > that > > > > > > > >> consumer > > > > > > > >> > group > > > > > > > >> > > > > name, > > > > > > > >> > > > > > > > >>>> rather > > > > > > > >> > > > > > > > >>>>>> than to the foo prefix. A simple way > > > would be > > > > > to > > > > > > > >> > escape the > > > > > > > >> > > > > star > > > > > > > >> > > > > > > > >> with > > > > > > > >> > > > > > > > >>>> a > > > > > > > >> > > > > > > > >>>>>> backslash: "foo\*" During the software > > > upgrade > > > > > > > >> > process, we > > > > > > > >> > > > > also > > > > > > > >> > > > > > > > >> need > > > > > > > >> > > > > > > > >>>> to > > > > > > > >> > > > > > > > >>>>>> translate all ACLs that refer to "foo*" > > > into > > > > > > "foo\*". > > > > > > > >> > > > > Otherwise, > > > > > > > >> > > > > > > > >> the > > > > > > > >> > > > > > > > >>>>>> upgrade could create a security hole by > > > > > granting > > > > > > more > > > > > > > >> > access > > > > > > > >> > > > > than > > > > > > > >> > > > > > > > >> the > > > > > > > >> > > > > > > > >>>>>> administrator intended. > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> Perhaps there is a simpler way. It > seems > > > like > > > > > > the > > > > > > > >> > resource > > > > > > > >> > > > > that > > > > > > > >> > > > > > > > >> people > > > > > > > >> > > > > > > > >>>>>> really want to use prefixed ACLs with > is > > > topic > > > > > > names. > > > > > > > >> > > > Because > > > > > > > >> > > > > > > > >> topic > > > > > > > >> > > > > > > > >>>>> names > > > > > > > >> > > > > > > > >>>>>> can't contain "*", there is no > ambiguity > > > there, > > > > > > > >> > either. I > > > > > > > >> > > > > think > > > > > > > >> > > > > > > > >> maybe > > > > > > > >> > > > > > > > >>>> we > > > > > > > >> > > > > > > > >>>>>> should restrict the prefix handling to > > > topic > > > > > > > >> resources. > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> Are the new "getMatchingAcls" methods > > > needed? > > > > > It > > > > > > > >> seems > > > > > > > >> > like > > > > > > > >> > > > > they > > > > > > > >> > > > > > > > >> break > > > > > > > >> > > > > > > > >>>>>> encapsulation. All that the calling > code > > > > > really > > > > > > > >> needs > > > > > > > >> > to do > > > > > > > >> > > > > is > > > > > > > >> > > > > > > > >> call > > > > > > > >> > > > > > > > >>>>>> Authorizer#authorize(session, > operation, > > > > > > resource). > > > > > > > >> The > > > > > > > >> > > > > > > authorizer > > > > > > > >> > > > > > > > >>>> knows > > > > > > > >> > > > > > > > >>>>>> that if it has an ACL allowing access > to > > > topics > > > > > > > >> starting > > > > > > > >> > > > with > > > > > > > >> > > > > > > > >> "foo" and > > > > > > > >> > > > > > > > >>>>>> someone calls authorize(foobar), it > should > > > > > allow > > > > > > > >> access. > > > > > > > >> > > > > It's not > > > > > > > >> > > > > > > > >>>>>> necessary for the calling code to know > > > exactly > > > > > > what > > > > > > > >> the > > > > > > > >> > > > rules > > > > > > > >> > > > > are > > > > > > > >> > > > > > > > >> for > > > > > > > >> > > > > > > > >>>>>> authorization. The Authorizer#getAcls, > > > APIs > > > > > are > > > > > > only > > > > > > > >> > needed > > > > > > > >> > > > > when > > > > > > > >> > > > > > > > >> the > > > > > > > >> > > > > > > > >>>>>> AdminClient wants to list ACLs. > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> best, > > > > > > > >> > > > > > > > >>>>>> Colin > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>> > > > > > > > >> > > > > > > > >>>>>>> On Wed, May 2, 2018, at 03:31, Rajini > > > Sivaram > > > > > > wrote: > > > > > > > >> > > > > > > > >>>>>>> Hi Piyush, > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> Thanks for the KIP for this widely > > > requested > > > > > > > >> feature. > > > > > > > >> > A few > > > > > > > >> > > > > > > > >>>>>>> comments/questions: > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> 1. Some resource names can contain > '*'. > > > For > > > > > > example, > > > > > > > >> > > > consumer > > > > > > > >> > > > > > > > >> groups > > > > > > > >> > > > > > > > >>>> or > > > > > > > >> > > > > > > > >>>>>>> transactional ids. I am wondering > whether > > > we > > > > > > need to > > > > > > > >> > > > restrict > > > > > > > >> > > > > > > > >>>>> characters > > > > > > > >> > > > > > > > >>>>>>> for these entities or provide a way to > > > > > > distinguish > > > > > > > >> > > > wildcarded > > > > > > > >> > > > > > > > >>>> resource > > > > > > > >> > > > > > > > >>>>>> from > > > > > > > >> > > > > > > > >>>>>>> a resource containing the wildcard > > > character. > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> 2. I am not sure we want to do > wildcarded > > > > > > > >> principals. > > > > > > > >> > It > > > > > > > >> > > > > feels > > > > > > > >> > > > > > > > >> like a > > > > > > > >> > > > > > > > >>>>>>> workaround in the absence of user > groups. > > > In > > > > > the > > > > > > > >> longer > > > > > > > >> > > > > term, I > > > > > > > >> > > > > > > > >> think > > > > > > > >> > > > > > > > >>>>>>> groups (without wildcards) would be a > > > better > > > > > > option > > > > > > > >> to > > > > > > > >> > > > > configure > > > > > > > >> > > > > > > > >> ACLs > > > > > > > >> > > > > > > > >>>>> for > > > > > > > >> > > > > > > > >>>>>>> groups of users, rather than building > user > > > > > > > >> principals > > > > > > > >> > which > > > > > > > >> > > > > have > > > > > > > >> > > > > > > > >>>> common > > > > > > > >> > > > > > > > >>>>>>> prefixes. In the shorter term, since a > > > > > > configurable > > > > > > > >> > > > > > > > >> PrincipalBuilder > > > > > > > >> > > > > > > > >>>> is > > > > > > > >> > > > > > > > >>>>>>> used to build the principal used for > > > > > > authorization, > > > > > > > >> > perhaps > > > > > > > >> > > > > > > > >> using the > > > > > > > >> > > > > > > > >>>>>>> prefix itself as the principal would > work > > > > > > (unless > > > > > > > >> > different > > > > > > > >> > > > > > > > >> quotas > > > > > > > >> > > > > > > > >>>> are > > > > > > > >> > > > > > > > >>>>>>> required for the full user name)? User > > > > > principal > > > > > > > >> > strings > > > > > > > >> > > > take > > > > > > > >> > > > > > > > >>>> different > > > > > > > >> > > > > > > > >>>>>>> formats for different security > protocols > > > (eg. > > > > > > > >> > > > > CN=xxx,O=org,C=UK > > > > > > > >> > > > > > > > >> for > > > > > > > >> > > > > > > > >>>>> SSL) > > > > > > > >> > > > > > > > >>>>>>> and simple prefixing isn't probably > the > > > right > > > > > > > >> grouping > > > > > > > >> > in > > > > > > > >> > > > > many > > > > > > > >> > > > > > > > >> cases. > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> 3. I am assuming we don't want to do > > > > > wildcarded > > > > > > > >> hosts > > > > > > > >> > in > > > > > > > >> > > > > this KIP > > > > > > > >> > > > > > > > >>>> since > > > > > > > >> > > > > > > > >>>>>>> wildcard suffix doesn't really work > for > > > hosts. > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> 4. It may be worth excluding > delegation > > > token > > > > > > ACLs > > > > > > > >> from > > > > > > > >> > > > using > > > > > > > >> > > > > > > > >>>> prefixed > > > > > > > >> > > > > > > > >>>>>>> wildcards since it doesn't make much > > > sense. > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> Regards, > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> Rajini > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>> On Wed, May 2, 2018 at 2:05 AM, > Stephane > > > > > Maarek > > > > > > < > > > > > > > >> > > > > > > > >>>>>>> steph...@simplemachines.com.au> > wrote: > > > > > > > >> > > > > > > > >>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> Hi, thanks for this badly needed > feature > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> 1) Why introduce two new APIs in > > > authorizer > > > > > > > >> instead of > > > > > > > >> > > > > > > > >> replacing > > > > > > > >> > > > > > > > >>>> the > > > > > > > >> > > > > > > > >>>>>>>> implementation for simple ACL > authorizer > > > with > > > > > > > >> adding > > > > > > > >> > the > > > > > > > >> > > > > > > > >> wildcard > > > > > > > >> > > > > > > > >>>>>>>> capability? > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> 2) is there an impact to performance > as > > > now > > > > > > we're > > > > > > > >> > > > evaluating > > > > > > > >> > > > > > > > >> more > > > > > > > >> > > > > > > > >>>>>> rules ? A > > > > > > > >> > > > > > > > >>>>>>>> while back I had evaluated the > concept of > > > > > > cached > > > > > > > >> Acl > > > > > > > >> > > > result > > > > > > > >> > > > > so > > > > > > > >> > > > > > > > >>>>> swallow > > > > > > > >> > > > > > > > >>>>>> the > > > > > > > >> > > > > > > > >>>>>>>> cost of computing an authorisation > cost > > > once > > > > > > and > > > > > > > >> then > > > > > > > >> > > > doing > > > > > > > >> > > > > in > > > > > > > >> > > > > > > > >>>> memory > > > > > > > >> > > > > > > > >>>>>>>> lookups. CF: > https://issues.apache.org/ > > > > > > > >> > > > > jira/browse/KAFKA-5261 > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> 3) is there any need to also extend > this > > > to > > > > > > > >> consumer > > > > > > > >> > group > > > > > > > >> > > > > > > > >>>> resources > > > > > > > >> > > > > > > > >>>>> ? > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> 4) create topics KIP as recently > moved > > > > > > permissions > > > > > > > >> > out of > > > > > > > >> > > > > > > > >> Cluster > > > > > > > >> > > > > > > > >>>>> into > > > > > > > >> > > > > > > > >>>>>>>> Topic. Will wildcard be supported for > > > this > > > > > > action > > > > > > > >> too? > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> Thanks a lot for this ! > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>> On Wed., 2 May 2018, 1:37 am Ted Yu, > < > > > > > > > >> > yuzhih...@gmail.com > > > > > > > >> > > > > > > > > > > > >> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>> w.r.t. naming, we can keep wildcard > and > > > drop > > > > > > > >> > 'prefixed' > > > > > > > >> > > > (or > > > > > > > >> > > > > > > > >>>>>> 'suffixed') > > > > > > > >> > > > > > > > >>>>>>>>> since the use of regex would always > > > start > > > > > with > > > > > > > >> > > > non-wildcard > > > > > > > >> > > > > > > > >>>>> portion. > > > > > > > >> > > > > > > > >>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>> Cheers > > > > > > > >> > > > > > > > >>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>> On Tue, May 1, 2018 at 12:13 PM, > Andy > > > > > Coates < > > > > > > > >> > > > > > > > >> a...@confluent.io> > > > > > > > >> > > > > > > > >>>>>> wrote: > > > > > > > >> > > > > > > > >>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>> Hi Piyush, > > > > > > > >> > > > > > > > >>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>> Can you also document in the > > > Compatibility > > > > > > > >> section > > > > > > > >> > what > > > > > > > >> > > > > > > > >> would > > > > > > > >> > > > > > > > >>>>>> happen > > > > > > > >> > > > > > > > >>>>>>>>> should > > > > > > > >> > > > > > > > >>>>>>>>>> the cluster be upgraded, > > > wildcard-suffixed > > > > > > ACLs > > > > > > > >> are > > > > > > > >> > > > added, > > > > > > > >> > > > > > > > >> and > > > > > > > >> > > > > > > > >>>>>> then the > > > > > > > >> > > > > > > > >>>>>>>>>> cluster is rolled back to the > previous > > > > > > version. > > > > > > > >> On > > > > > > > >> > > > > > > > >> downgrade > > > > > > > >> > > > > > > > >>>> the > > > > > > > >> > > > > > > > >>>>>>>> partial > > > > > > > >> > > > > > > > >>>>>>>>>> wildcard ACLs will be treated as > > > literals > > > > > and > > > > > > > >> hence > > > > > > > >> > > > never > > > > > > > >> > > > > > > > >> match > > > > > > > >> > > > > > > > >>>>>>>> anything. > > > > > > > >> > > > > > > > >>>>>>>>>> This is fine for ALLOW ACLs, but > some > > > might > > > > > > > >> consider > > > > > > > >> > > > this > > > > > > > >> > > > > a > > > > > > > >> > > > > > > > >>>>>> security > > > > > > > >> > > > > > > > >>>>>>>> hole > > > > > > > >> > > > > > > > >>>>>>>>>> if a DENY ACL is ignored, so this > > > needs to > > > > > be > > > > > > > >> > > > documented, > > > > > > > >> > > > > > > > >> both > > > > > > > >> > > > > > > > >>>> in > > > > > > > >> > > > > > > > >>>>>> the > > > > > > > >> > > > > > > > >>>>>>>> KIP > > > > > > > >> > > > > > > > >>>>>>>>>> and the final docs. > > > > > > > >> > > > > > > > >>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>> For some reason I find the term > > > 'prefixed > > > > > > > >> wildcard > > > > > > > >> > ACLs' > > > > > > > >> > > > > > > > >> easier > > > > > > > >> > > > > > > > >>>>> to > > > > > > > >> > > > > > > > >>>>>> grok > > > > > > > >> > > > > > > > >>>>>>>>>> than 'wildcard suffixed ACLs'. > Probably > > > > > > because > > > > > > > >> in > > > > > > > >> > the > > > > > > > >> > > > > > > > >> former > > > > > > > >> > > > > > > > >>>> the > > > > > > > >> > > > > > > > >>>>>>>>>> 'wildcard' term comes after the > > > positional > > > > > > > >> > adjective, > > > > > > > >> > > > > which > > > > > > > >> > > > > > > > >>>>>> matches the > > > > > > > >> > > > > > > > >>>>>>>>>> position of the wildcard char in > the > > > > > resource > > > > > > > >> name, > > > > > > > >> > i.e. > > > > > > > >> > > > > > > > >>>> "some*". > > > > > > > >> > > > > > > > >>>>>> It's > > > > > > > >> > > > > > > > >>>>>>>>>> most likely a person thing, but I > > > thought > > > > > I'd > > > > > > > >> > mention it > > > > > > > >> > > > > as > > > > > > > >> > > > > > > > >>>>> naming > > > > > > > >> > > > > > > > >>>>>> is > > > > > > > >> > > > > > > > >>>>>>>>>> important when it comes to making > this > > > > > > initiative > > > > > > > >> > for > > > > > > > >> > > > > > > > >> users. > > > > > > > >> > > > > > > > >>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>> On 1 May 2018 at 19:57, Andy > Coates < > > > > > > > >> > a...@confluent.io> > > > > > > > >> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>>> Hi Piyush, > > > > > > > >> > > > > > > > >>>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>>> Thanks for raising this KIP - it's > > > very > > > > > much > > > > > > > >> > > > appreciated. > > > > > > > >> > > > > > > > >>>>>>>>>>> > > > > > > > >> > > > > > > > >>>>>>>>>>> I've not had chance to digest it > yet, > > > >
