kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ashish Singh <asi...@cloudera.com>
Subject Re: [DISCUSS] KIP-50 - Enhance Authorizer interface to be aware of supported Principal Types
Date Tue, 12 Apr 2016 01:05:09 GMT
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 <jun@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 <asingh@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 <bosco@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" <asingh@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 <ismael@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 <bosco@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" <asingh@cloudera.com
> > > <javascript:;>> wrote:
> > > >> >
> > > >> > >Hello Harsha,
> > > >> > >
> > > >> > >On Thu, Apr 7, 2016 at 11:03 AM, Harsha <mail@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 <
> > > asingh@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
<
> > ismael@juma.me.uk
> > > <javascript:;>>
> > > >> > wrote:
> > > >> > >> > >
> > > >> > >> > >> It is small, but breaks binary compatibility.
> > > >> > >> > >>
> > > >> > >> > >> Ismael
> > > >> > >> > >>
> > > >> > >> > >> On Wed, Apr 6, 2016 at 5:20 PM, Grant
Henke <
> > > ghenke@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 <
> > > >> ismael@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 <
> > > >> > ghenke@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 <
> > > >> jay@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 <
> > > >> > >> ismael@juma.me.uk <javascript:;>>
> > > >> > >> > >> > > wrote:
> > > >> > >> > >> > > > >
> > > >> > >> > >> > > > > > Hi Jay,
> > > >> > >> > >> > > > > >
> > > >> > >> > >> > > > > > On Wed, Apr 6,
2016 at 3:48 AM, Jay Kreps <
> > > >> > jay@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
> > > >> > >> > >> > > > grant@cloudera.com <javascript:;>
|
> > twitter.com/gchenke
> > > |
> > > >> > >> > >> linkedin.com/in/granthenke
> > > >> > >> > >> > > >
> > > >> > >> > >> > >
> > > >> > >> > >> >
> > > >> > >> > >> >
> > > >> > >> > >> >
> > > >> > >> > >> > --
> > > >> > >> > >> > Grant Henke
> > > >> > >> > >> > Software Engineer | Cloudera
> > > >> > >> > >> > grant@cloudera.com <javascript:;>
| twitter.com/gchenke
> |
> > > >> > >> linkedin.com/in/granthenke
> > > >> > >> > >> >
> > > >> > >> > >>
> > > >> > >> > >
> > > >> > >> > >
> > > >> > >> > >
> > > >> > >> > > --
> > > >> > >> > >
> > > >> > >> > > Regards,
> > > >> > >> > > Ashish
> > > >> > >> > >
> > > >> > >> >
> > > >> > >> >
> > > >> > >> >
> > > >> > >> > --
> > > >> > >> >
> > > >> > >> > Regards,
> > > >> > >> > Ashish
> > > >> > >>
> > > >> > >​
> > > >> > >--
> > > >> > >
> > > >> > >Regards,
> > > >> > >Ashish
> > > >> >
> > > >> >
> > > >>
> > > >
> > > >
> > > >
> > > >--
> > > >
> > > >Regards,
> > > >Ashish
> > >
> > >
> >
> > --
> > Ashish 🎤h
> >
>



-- 

Regards,
Ashish

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message