Given that we're breaking compatibility anyway should we: 1. Remove the get prefix on this method and the existing one which violate our own code style guidelines (Oops! Kind of sad we went through the whole KIP process and no one even flagged this) 2. Move the interface out of scala to be a normal Java interface
This breaks source compatibility but probably what we should have done originally I suspect. Probably there are few enough implementations of this that it is better to just rip the bandaid off. -Jay On Sat, Apr 2, 2016 at 11:18 AM, Ashish Singh <asi...@cloudera.com> wrote: > Hello Ismael, > > On Fri, Apr 1, 2016 at 4:08 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi Ashish, > > > > A few comments on the proposal: > > > > 1. In Kafka, we don't use use getter convention generally. However, the > > other methods in the `Authorizer` interface do follow the getter > > convention, which is unfortunate. So, I am OK with the name you suggested > > (getSupportedPrincipalTypes instead of supportedPrincipalTypes), but I > > wanted to mention this in case others have a different opinion. > > > If multiple people feel the same, I am happy to rename the method. > > > > > 2. The proposed change to the Authorizer trait (adding a method with a > > default implementation) is source compatible, but _not_ binary > compatible. > > So, it won't be possible for someone to implement the Authorizer and > > compile it once so that it works with both Kafka 0.9.0.x and Kafka > > 0.10.0.x. Not sure how much of an issue this is, but it's worth > mentioning > > it in the KIP. > > > Right, will mention that. It is binary incompatible only when someone uses > >= 0.10 AdminCommand with an Authorizer implementation based on 0.9. > > > > > 3. If an Authorizer wanted to support any type of principal without > > specifying them, is there a way to do that? Is it something that we want > to > > support? Before the KIP was proposed, there was a discussion in the PR > > about different principal types potentially being used by the > > authentication layer where the Authorizer is agnostic. > > > Authorizer is agnostic of principal types right now. As we have already > seen that this opens up space for user errors. We want to keep kafka-acls > be generic so that various authorizer implementation can use the same cli. > Each implementation has freedom of supporting their own principal types. > How do we expect users to know what principal types are supported by a > particular implementation? Sure we can document it, we have it documented > for SimpleAclAuthorizer, still users ran into issue. Worst thing is that > this error happens silently, invalid acls are persisted by authorizer, as > authorizers themselves are not aware of principal types it supports. This > is what the KIP is trying to solve. > > > > 4. There is a PR for introducing a "group" principal type ( > > https://github.com/apache/kafka/pull/483/files), would that have any > > impact > > on this proposal? > > > Override of getSupportedPrincipalTypes in SimpleAclAuthorizer will have to > return group as well. > > > > > Ismael > > > > On Mon, Mar 28, 2016 at 9:28 PM, Ashish Singh <asi...@cloudera.com> > wrote: > > > > > Hello Harsha, > > > > > > Pinging again. This is a minor KIP and it has been lying around for > quite > > > some time. If providing supported principal types via a config is what > > you > > > suggest, I am fine with it. > > > > > > On Wed, Mar 9, 2016 at 12:32 PM, Ashish Singh <asi...@cloudera.com> > > wrote: > > > > > > > Hi Harsha, > > > > > > > > On Wed, Mar 9, 2016 at 12:04 PM, Harsha <ka...@harsha.io> wrote: > > > > > > > >> Why we need to add this additional method just for validation. This > > will > > > >> invalidate all the existing authorizer implementations. > > > > > > > > As PrincipalTypes is implementation specific, wouldn't it be required > > for > > > > users to know what principal types they can use in add/removeAcls? > > > > > > > > All the existing authorizer implementations will continue to work, as > > the > > > > method by default will return List(User), as User is the only > principal > > > > type that is supported out of the box as of now. Let me know if I > > missed > > > > your question here. > > > > > > > > > > > >> Why can't we add > > > >> the logic for validation and pass it as authorizer config. > > > >> > > > > Do you mean passing PrincipalTypes as authorizer config? If I am > > getting > > > > your question correctly, then we are asking users to be aware of what > > > > PrincipalTypes an authorizer supports. That defeats the purpose of > > > > validation, right? > > > > > > > >> > > > >> -Harsha > > > >> > > > >> On Mon, Mar 7, 2016, at 04:33 PM, Ashish Singh wrote: > > > >> > + Parth, Harsha > > > >> > > > > >> > On Mon, Mar 7, 2016 at 4:32 PM, Ashish Singh <asi...@cloudera.com > > > > > >> wrote: > > > >> > > > > >> > > Thanks Gwen. > > > >> > > > > > >> > > @Parth, @Harsha pinging you guys for your feedback. Based on > > > >> discussion on > > > >> > > JIRA, we have following open questions. > > > >> > > > > > >> > > 1. > > > >> > > > > > >> > > How to allow an authorizer implementation to specify > supported > > > >> > > principal types? > > > >> > > > > > >> > > An alternative of providing supported Principal types via > > > >> interface is > > > >> > > via a config option. Having a config option will be helpful > for > > > >> certain > > > >> > > third party implementations that uses SimpleAclAuthorizer but > > > >> support more > > > >> > > PrincipalTypes. However, it requires adds one more config. > > > >> > > > > > >> > > 2. > > > >> > > > > > >> > > ACLs validation should be done by client or by authorizer? > > > >> > > > > > >> > > Once this method is added we expect the Client of the > > authorizer > > > >> to do > > > >> > > the validation on principal types and the authorizer will > still > > > >> not do any > > > >> > > validation by it self. As an alternative we can add the > > > validation > > > >> at > > > >> > > Authorizer level. Having validation done at client side > enables > > > >> clients to > > > >> > > fail fast for invalid principal types, whereas implementing > it > > at > > > >> > > authorization level removes the requirement of having the > > > >> validation done > > > >> > > on each client implementation. > > > >> > > > > > >> > > > > > >> > > On Mon, Mar 7, 2016 at 3:47 PM, Gwen Shapira <g...@confluent.io > > > > > >> wrote: > > > >> > > > > > >> > > Ashish, > > > >> > >> > > > >> > >> I'm neutral on this (+0), but would be good to have feedback > from > > > >> > >> Harsha and Parth. If you can get their "sounds good", we can > > > probably > > > >> > >> get it through fairly soon and in time for 0.10.0. > > > >> > >> > > > >> > >> Gwen > > > >> > >> > > > >> > >> On Wed, Mar 2, 2016 at 9:47 AM, Ashish Singh < > > asi...@cloudera.com> > > > >> wrote: > > > >> > >> > Here is link to the KIP, > > > >> > >> > > > > >> > >> > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Enhance+Authorizer+interface+to+be+aware+of+supported+Principal+Types > > > >> > >> > > > > >> > >> > On Wed, Mar 2, 2016 at 9:46 AM, Ashish Singh < > > > asi...@cloudera.com> > > > >> > >> wrote: > > > >> > >> > > > > >> > >> >> Hi Guys, > > > >> > >> >> > > > >> > >> >> I would like to initiate a discuss thread on KIP-50. Kafka > > > >> authorizer > > > >> > >> is > > > >> > >> >> agnostic of principal types it supports, so are the acls > CRUD > > > >> methods > > > >> > >> >> in kafka.security.auth.Authorizer. The intent behind is to > > keep > > > >> Kafka > > > >> > >> >> authorization pluggable, which is really great. However, > this > > > >> leads to > > > >> > >> Acls > > > >> > >> >> CRUD methods not performing any check on validity of acls, > as > > > >> they are > > > >> > >> not > > > >> > >> >> aware of what principal types Authorizer implementation > > > supports. > > > >> This > > > >> > >> >> opens up space for lots of user errors, KAFKA-3097 > > > >> > >> >> <https://issues.apache.org/jira/browse/KAFKA-3097> for an > > > >> instance. > > > >> > >> >> > > > >> > >> >> This KIP proposes adding a getSupportedPrincipalTypes method > > to > > > >> > >> authorizer > > > >> > >> >> and use that for acls verification during acls CRUD. > > > >> > >> >> > > > >> > >> >> Feedbacks and comments are welcome. > > > >> > >> >> > > > >> > >> >> -- > > > >> > >> >> > > > >> > >> >> Regards, > > > >> > >> >> Ashish > > > >> > >> >> > > > >> > >> > > > > >> > >> > > > > >> > >> > > > > >> > >> > -- > > > >> > >> > > > > >> > >> > Regards, > > > >> > >> > Ashish > > > >> > >> > > > >> > > > > > >> > > -- > > > >> > > > > > >> > > Regards, > > > >> > > Ashish > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > -- > > > >> > > > > >> > Regards, > > > >> > Ashish > > > >> > > > > > > > > > > > > > > > > -- > > > > > > > > Regards, > > > > Ashish > > > > > > > > > > > > > > > > -- > > > > > > Regards, > > > Ashish > > > > > > > > > -- > > Regards, > Ashish >
