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