Ashish, So, you want to take a shot at option 2 for 0.10.0? That's fine with me too. I am just not sure if we have enough time to think through the changes.
Thanks, Jun On Mon, Apr 11, 2016 at 6:05 PM, Ashish Singh <asi...@cloudera.com> wrote: > Hello Jun, > > The 3rd option will require Apache Sentry to go GA with current authorizer > interface, and at this point it seems that the interface won't last long. > Within a few months, Sentry will have to make a breaking change. I do > understand that Kafka should not have to delay its release due to one of > the authorizer implementations. However, can we assist Sentry users to > avoid that breaking upgrade? I think it is worth a shot. If the changes are > not done by 0.10 code freeze, then sure lets punt it to next release. Does > this seem reasonable to you? > > On Sun, Apr 10, 2016 at 11:42 AM, Jun Rao <j...@confluent.io> wrote: > > > Ashish, > > > > A 3rd option is to in 0.10.0, just sanity check the principal type in the > > implementation of addAcls/removeAcls of Authorizer, but don't change the > > Authorizer api to add the getDescription() method. This fixes the > immediate > > issue that an acl rule with the wrong principal type is silently ignored. > > Knowing valid user types is nice, but not critical (we can include the > > supported user type in the UnsupportedPrincipalTypeException thrown from > > addAcls/removeAcls). This will give us more time to clean up the > Authorizer > > api post 0.10.0. > > > > Thanks > > > > Jun > > > > On Fri, Apr 8, 2016 at 9:04 AM, Ashish Singh <asi...@cloudera.com> > wrote: > > > > > Thanks for the input Don. One of the possible paths for Option 2 is to > > > completely drop Scala interface, would that be Ok with you folks? > > > > > > On Thursday, April 7, 2016, Don Bosco Durai <bo...@apache.org> wrote: > > > > > > > Ranger team would prefer option #2. Right now, we have to access some > > of > > > > the nested constants using constructs like Group$.MODULE$, which is > not > > > > intuitive in Java. > > > > > > > > Thanks > > > > > > > > Bosco > > > > > > > > > > > > > > > > > > > > On 4/7/16, 4:30 PM, "Ashish Singh" <asi...@cloudera.com > > <javascript:;>> > > > > wrote: > > > > > > > > >Harsha/ Don, > > > > > > > > > >Are you guys OK with option 2? I am not aware of all the existing > > > > >authorizer implementations, however ranger has one for sure. Getting > > > > direct > > > > >feedback from you guys will be really valuable. > > > > > > > > > >On Thu, Apr 7, 2016 at 3:52 PM, Ismael Juma <ism...@juma.me.uk > > > > <javascript:;>> wrote: > > > > > > > > > >> Hi Don, > > > > >> > > > > >> This is true in Java 7, but Java 8 introduces default methods and > > this > > > > >> workaround is no longer required. During the Interceptor KIP > > > > discussion, it > > > > >> was decided that it was fine to stick to interfaces given that we > > are > > > > >> likely to move to Java 8 in the nearish future (probably no later > > than > > > > the > > > > >> Java 9 release). > > > > >> > > > > >> Ismael > > > > >> > > > > >> On Thu, Apr 7, 2016 at 11:36 PM, Don Bosco Durai < > bo...@apache.org > > > > <javascript:;>> wrote: > > > > >> > > > > >> > Hi Ashish > > > > >> > > > > > >> > If we are going by option #2, then I can suggest we give an > > abstract > > > > >> > implementation of the Interface and recommend anyone > implementing > > > > their > > > > >> own > > > > >> > plugin to extend from the abstract class, rather than implement > > the > > > > >> > interface? > > > > >> > > > > > >> > The advantage is, in the future if we add add any new methods in > > the > > > > >> > Interface (e.g. Similar to getDescription()), then we can give a > > > dummy > > > > >> > implementation of the new method and this won’t break the > > > compilation > > > > of > > > > >> > any external implementation. Else over the time it will be > > > challenging > > > > >> for > > > > >> > anyone customizing the implementation to keep track of changes > to > > > the > > > > >> > Interface. > > > > >> > > > > > >> > Thanks > > > > >> > > > > > >> > Bosco > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > On 4/7/16, 11:21 AM, "Ashish Singh" <asi...@cloudera.com > > > > <javascript:;>> wrote: > > > > >> > > > > > >> > >Hello Harsha, > > > > >> > > > > > > >> > >On Thu, Apr 7, 2016 at 11:03 AM, Harsha <m...@harsha.io > > > > <javascript:;>> wrote: > > > > >> > > > > > > >> > >"My only ask is to have this in 0.10. As Jay pointed out, right > > now > > > > >> > >> there > > > > >> > >> are not many implementations out there, we might want to fix > it > > > > ASAP." > > > > >> > >> > > > > >> > >> Probably there aren't many implementations but there are lot > of > > > > users > > > > >> > >> using these implementations in production clusters. > > > > >> > >> Isn't this going to break the rolling upgrade? > > > > >> > > > > > > >> > >It will and it is a concern, in my previous mail I have > mentioned > > > > this > > > > >> as > > > > >> > >an issue if we choose to go this route. However, if we actually > > > > decide > > > > >> to > > > > >> > >do this, I would say it is better to do it sooner than later, > as > > > > fewer > > > > >> > >implementations will be affected. Below is excerpt from my > > previous > > > > >> mail. > > > > >> > > > > > > >> > >Increase scope of KIP-50 to move authorizer and related classes > > to > > > a > > > > >> > >separate package. The new package will have java interface. > This > > > will > > > > >> > allow > > > > >> > >implementations to not depend on kafka core and just on > > authorizer > > > > >> > package, > > > > >> > >make authorization interface follow kafka’s coding standards > and > > > will > > > > >> > allow > > > > >> > >java implementations to be cleaner. We can either completely > drop > > > > scala > > > > >> > >interface, which might be a pain for existing implementations, > or > > > we > > > > can > > > > >> > >have scala interface wrap java interface. Later allows a > cleaner > > > > >> > >deprecation path for existing scala authorizer interface, > however > > > it > > > > may > > > > >> > or > > > > >> > >may not be feasible as Kafka server will have to somehow decide > > > which > > > > >> > >interface it should be looking for while loading authorizer > > > > >> > implementation, > > > > >> > >this can probably be solved with a config or some reflection. > If > > we > > > > >> choose > > > > >> > >to go this route, I can dig deeper. > > > > >> > > > > > > >> > >If we go with option 2 and commit on getting this in ASAP, > > > > preferably in > > > > >> > >0.10, there will be fewer implementations that will be > affected. > > > > >> > > > > > > >> > >and also moving to Java , > > > > >> > >> a authorizer implementation going to run inside a KafkaBroker > > > and I > > > > >> > >> don't see why this is necessary to move to clients package. > > > > >> > >> Are we planning on introducing common module to have it > > > > independent of > > > > >> > >> broker and client code? > > > > >> > >> > > > > >> > >Yes, I think that would take away the requirement of depending > on > > > > Kafka > > > > >> > >core from authorizer implementations. Do you suggest otherwise? > > > > >> > > > > > > >> > > > > > > >> > >> -Harsha > > > > >> > >> > > > > >> > >> On Thu, Apr 7, 2016, at 10:52 AM, Ashish Singh wrote: > > > > >> > >> > We might want to take a call here. Following are the > options. > > > > >> > >> > > > > > >> > >> > 1. Let KIP-50 be the way it is, i.e., just add > > > > getDescription() > > > > >> to > > > > >> > >> > existing scala authorizer interface. It will break > binary > > > > >> > >> > compatibility > > > > >> > >> > (only when using CLI and/or AdminCommand from >= 0.10 > > > against > > > > >> > >> > authorizer > > > > >> > >> > implementations based on 0.9.). If we go this route, it > > is a > > > > >> > simpler > > > > >> > >> > change > > > > >> > >> > and existing implementations won’t have to change > anything > > > on > > > > >> their > > > > >> > >> > end. > > > > >> > >> > 2. Increase scope of KIP-50 to move authorizer and > related > > > > >> classes > > > > >> > to > > > > >> > >> > a > > > > >> > >> > separate package. The new package will have java > > interface. > > > > This > > > > >> > will > > > > >> > >> > allow > > > > >> > >> > implementations to not depend on kafka core and just on > > > > >> authorizer > > > > >> > >> > package, > > > > >> > >> > make authorization interface follow kafka’s coding > > standards > > > > and > > > > >> > will > > > > >> > >> > allow > > > > >> > >> > java implementations to be cleaner. We can either > > completely > > > > drop > > > > >> > >> > scala > > > > >> > >> > interface, which might be a pain for existing > > > > implementations, or > > > > >> > we > > > > >> > >> > can > > > > >> > >> > have scala interface wrap java interface. Later allows a > > > > cleaner > > > > >> > >> > deprecation path for existing scala authorizer > interface, > > > > however > > > > >> > it > > > > >> > >> > may or > > > > >> > >> > may not be feasible as Kafka server will have to somehow > > > > decide > > > > >> > which > > > > >> > >> > interface it should be looking for while loading > > authorizer > > > > >> > >> > implementation, > > > > >> > >> > this can probably be solved with a config or some > > > reflection. > > > > If > > > > >> we > > > > >> > >> > choose > > > > >> > >> > to go this route, I can dig deeper. > > > > >> > >> > > > > > >> > >> > If we decide to go with option 1, I think it would be fair > to > > > say > > > > >> that > > > > >> > >> > scala authorizer interface will be around for some time, as > > > there > > > > >> > will be > > > > >> > >> > more implementations relying on it. If we go with option 2 > > and > > > > >> commit > > > > >> > on > > > > >> > >> > getting this in ASAP, preferably in 0.10, there will be > fewer > > > > >> > >> > implementations that will be affected. > > > > >> > >> > > > > > >> > >> > *Another thing to notice is that scala authorizer interface > > is > > > > not > > > > >> > >> > annotated as unstable.* > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > On Wed, Apr 6, 2016 at 9:41 AM, Ashish Singh < > > > > asi...@cloudera.com <javascript:;>> > > > > >> > >> wrote: > > > > >> > >> > > > > > >> > >> > > I see value in minimizing breaking changes and I do not > > > oppose > > > > the > > > > >> > >> idea of > > > > >> > >> > > increasing scope of KIP-50 to move auth interface to > java. > > > > >> > >> > > > > > > >> > >> > > As authorizer implementations do not really need to > depend > > on > > > > >> Kafka > > > > >> > >> core, > > > > >> > >> > > I would suggest that we keep authorizer interface and its > > > > >> components > > > > >> > >> in a > > > > >> > >> > > separate package. I share the concern that right now > using > > > > >> Resource, > > > > >> > >> > > Operation, etc, in java implementations is messy. I had > to > > > deal > > > > >> with > > > > >> > >> lot of > > > > >> > >> > > it while writing Apache Sentry plugin. > > > > >> > >> > > > > > > >> > >> > > My only ask is to have this in 0.10. As Jay pointed out, > > > right > > > > now > > > > >> > >> there > > > > >> > >> > > are not many implementations out there, we might want to > > fix > > > it > > > > >> > ASAP. > > > > >> > >> I can > > > > >> > >> > > only speak of Sentry integration and I think 0.10 will be > > the > > > > best > > > > >> > for > > > > >> > >> such > > > > >> > >> > > a change, as I should be able to adopt the changes in > > Sentry > > > > >> > >> integration > > > > >> > >> > > before a lot of users start using it. > > > > >> > >> > > > > > > >> > >> > > On Wed, Apr 6, 2016 at 9:25 AM, Ismael Juma < > > > ism...@juma.me.uk > > > > <javascript:;>> > > > > >> > wrote: > > > > >> > >> > > > > > > >> > >> > >> It is small, but breaks binary compatibility. > > > > >> > >> > >> > > > > >> > >> > >> Ismael > > > > >> > >> > >> > > > > >> > >> > >> On Wed, Apr 6, 2016 at 5:20 PM, Grant Henke < > > > > ghe...@cloudera.com <javascript:;> > > > > >> > > > > > >> > >> wrote: > > > > >> > >> > >> > > > > >> > >> > >> > KIP-50 as defined is very small. I don't see any harm > in > > > > >> putting > > > > >> > it > > > > >> > >> in > > > > >> > >> > >> as > > > > >> > >> > >> > is and then tackling the follow up work. > > > > >> > >> > >> > > > > > >> > >> > >> > > > > > >> > >> > >> > On Wed, Apr 6, 2016 at 11:16 AM, Ismael Juma < > > > > >> ism...@juma.me.uk <javascript:;>> > > > > >> > >> wrote: > > > > >> > >> > >> > > > > > >> > >> > >> > > Thanks Grant. I wonder if KIP-50 should just be done > > as > > > > part > > > > >> of > > > > >> > >> this > > > > >> > >> > >> > work. > > > > >> > >> > >> > > > > > > >> > >> > >> > > Ismael > > > > >> > >> > >> > > > > > > >> > >> > >> > > On Wed, Apr 6, 2016 at 5:12 PM, Grant Henke < > > > > >> > ghe...@cloudera.com <javascript:;>> > > > > >> > >> > >> wrote: > > > > >> > >> > >> > > > > > > >> > >> > >> > > > My work with KIP-4 found that many of the Scala > > > classes > > > > >> used > > > > >> > in > > > > >> > >> the > > > > >> > >> > >> > > > Authorizer interface are needed in the Clients > > package > > > > when > > > > >> > >> adding > > > > >> > >> > >> the > > > > >> > >> > >> > > > various ACL requests and responses. I also found > > that > > > we > > > > >> > don't > > > > >> > >> have > > > > >> > >> > >> > > > standard Exceptions defined for the authorizer > > > > interface. > > > > >> > This > > > > >> > >> means > > > > >> > >> > >> > that > > > > >> > >> > >> > > > when I add the Authorizer calls to the broker and > > wire > > > > >> > >> protocols all > > > > >> > >> > >> > > > exceptions will be reported as an "Unknown Error" > > back > > > > to > > > > >> the > > > > >> > >> user > > > > >> > >> > >> via > > > > >> > >> > >> > > the > > > > >> > >> > >> > > > wire protocol. > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > I have written more about it on the KIP-4 wiki and > > > > created > > > > >> > >> jiras to > > > > >> > >> > >> > track > > > > >> > >> > >> > > > those issues (See below). I think we should wrap > up > > > this > > > > >> KIP > > > > >> > as > > > > >> > >> is > > > > >> > >> > >> and > > > > >> > >> > >> > > > tackle the Java/Exception changes as a part of > those > > > > >> > jiras/kips. > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > - KIP-4 "Follow Up Changes" > > > > >> > >> > >> > > > < > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > > > > >> > >> > >> > > > > > >> > >> > >> > > > > >> > >> > > > > >> > > > > > >> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-4+-+Command+line+and+centralized+administrative+operations#KIP-4-Commandlineandcentralizedadministrativeoperations-FollowUpChangesfollow-up-changes > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > - KAFKA-3509 < > > > > >> > >> https://issues.apache.org/jira/browse/KAFKA-3509>: > > > > >> > >> > >> > > > Provide > > > > >> > >> > >> > > > an Authorizer interface using the Java client > > > > enumerator > > > > >> > >> classes > > > > >> > >> > >> > > > - KAFKA-3507 < > > > > >> > >> https://issues.apache.org/jira/browse/KAFKA-3507>: > > > > >> > >> > >> > > Define > > > > >> > >> > >> > > > standard exceptions for the Authorizer > interface > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > Thank you, > > > > >> > >> > >> > > > Grant > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > On Wed, Apr 6, 2016 at 10:58 AM, Jay Kreps < > > > > >> j...@confluent.io <javascript:;> > > > > >> > > > > > > >> > >> > >> wrote: > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > > Hey Ismael, > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > Yeah I think this is a minor cleanliness thing. > > > Since > > > > >> this > > > > >> > is > > > > >> > >> kind > > > > >> > >> > >> > of a > > > > >> > >> > >> > > > > power user interface I don't feel strongly > either > > > way. > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > My motivation with Scala is just that we've > tried > > to > > > > move > > > > >> > to > > > > >> > >> > >> having > > > > >> > >> > >> > the > > > > >> > >> > >> > > > > public interfaces be Java, and as a group we > > > > definitely > > > > >> > >> struggled > > > > >> > >> > >> a > > > > >> > >> > >> > lot > > > > >> > >> > >> > > > > with understanding and maintaining Scala > > > > compatibility in > > > > >> > the > > > > >> > >> > >> older > > > > >> > >> > >> > > > > clients. > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > -Jay > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > On Tue, Apr 5, 2016 at 11:46 PM, Ismael Juma < > > > > >> > >> ism...@juma.me.uk <javascript:;>> > > > > >> > >> > >> > > wrote: > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > > Hi Jay, > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > On Wed, Apr 6, 2016 at 3:48 AM, Jay Kreps < > > > > >> > j...@confluent.io <javascript:;> > > > > >> > >> > > > > > >> > >> > >> > wrote: > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > Given that we're breaking compatibility > anyway > > > > should > > > > >> > we: > > > > >> > >> > >> > > > > > > > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > We are not breaking source compatibility since > > the > > > > new > > > > >> > >> method > > > > >> > >> > >> has a > > > > >> > >> > >> > > > > default > > > > >> > >> > >> > > > > > implementation. I take it that you mean binary > > > > >> > >> compatibility? > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > 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) > > > > >> > >> > >> > > > > > > > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > I did flag this during the discussion and > Ashish > > > > said > > > > >> he > > > > >> > >> would > > > > >> > >> > >> > change > > > > >> > >> > >> > > > it > > > > >> > >> > >> > > > > if > > > > >> > >> > >> > > > > > other people felt that it should be changed. > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > 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. > > > > >> > >> > >> > > > > > > > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > Can you please explain the motivation? It did > > come > > > > up > > > > >> in > > > > >> > >> > >> previous > > > > >> > >> > >> > > > > > discussions that some things like Operation > and > > > > >> > ResourceType > > > > >> > >> > >> should > > > > >> > >> > >> > > be > > > > >> > >> > >> > > > in > > > > >> > >> > >> > > > > > the clients library, but not Authorizer > itself. > > > Are > > > > we > > > > >> > >> saying > > > > >> > >> > >> that > > > > >> > >> > >> > > any > > > > >> > >> > >> > > > > > pluggable interface should be in Java so that > > > users > > > > can > > > > >> > >> > >> implement > > > > >> > >> > >> > it > > > > >> > >> > >> > > > > > without including the Scala library? > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > Grant, you originally suggested that some > things > > > > would > > > > >> > have > > > > >> > >> to > > > > >> > >> > >> be > > > > >> > >> > >> > in > > > > >> > >> > >> > > > the > > > > >> > >> > >> > > > > > Java side as well. Can you please elaborate on > > > this? > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > Ismael > > > > >> > >> > >> > > > > > > > > > >> > >> > >> > > > > > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > -- > > > > >> > >> > >> > > > Grant Henke > > > > >> > >> > >> > > > Software Engineer | Cloudera > > > > >> > >> > >> > > > gr...@cloudera.com <javascript:;> | > > > twitter.com/gchenke > > > > | > > > > >> > >> > >> linkedin.com/in/granthenke > > > > >> > >> > >> > > > > > > > >> > >> > >> > > > > > > >> > >> > >> > > > > > >> > >> > >> > > > > > >> > >> > >> > > > > > >> > >> > >> > -- > > > > >> > >> > >> > Grant Henke > > > > >> > >> > >> > Software Engineer | Cloudera > > > > >> > >> > >> > gr...@cloudera.com <javascript:;> | > twitter.com/gchenke > > | > > > > >> > >> linkedin.com/in/granthenke > > > > >> > >> > >> > > > > > >> > >> > >> > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > -- > > > > >> > >> > > > > > > >> > >> > > Regards, > > > > >> > >> > > Ashish > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > -- > > > > >> > >> > > > > > >> > >> > Regards, > > > > >> > >> > Ashish > > > > >> > >> > > > > >> > > > > > > >> > >-- > > > > >> > > > > > > >> > >Regards, > > > > >> > >Ashish > > > > >> > > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >-- > > > > > > > > > >Regards, > > > > >Ashish > > > > > > > > > > > > > > -- > > > Ashish 🎤h > > > > > > > > > -- > > Regards, > Ashish >
