kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gwen Shapira <g...@confluent.io>
Subject Re: [DISCUSS] KIP-50 - Enhance Authorizer interface to be aware of supported Principal Types
Date Thu, 07 Apr 2016 18:25:46 GMT
Can you guys go into details on what will happen during a rolling upgrade
exactly?

Gwen

On Thu, Apr 7, 2016 at 11:21 AM, Ashish Singh <asingh@cloudera.com> wrote:

> Hello Harsha,
>
> On Thu, Apr 7, 2016 at 11:03 AM, Harsha <mail@harsha.io> 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>
> > 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>
> wrote:
> > > >
> > > >> It is small, but breaks binary compatibility.
> > > >>
> > > >> Ismael
> > > >>
> > > >> On Wed, Apr 6, 2016 at 5:20 PM, Grant Henke <ghenke@cloudera.com>
> > 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>
> > 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>
> > > >> 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>
> > > >> 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>
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi Jay,
> > > >> > > > > >
> > > >> > > > > > On Wed, Apr 6, 2016 at 3:48 AM, Jay Kreps
<
> jay@confluent.io
> > >
> > > >> > 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 | twitter.com/gchenke |
> > > >> linkedin.com/in/granthenke
> > > >> > > >
> > > >> > >
> > > >> >
> > > >> >
> > > >> >
> > > >> > --
> > > >> > Grant Henke
> > > >> > Software Engineer | Cloudera
> > > >> > grant@cloudera.com | twitter.com/gchenke |
> > linkedin.com/in/granthenke
> > > >> >
> > > >>
> > > >
> > > >
> > > >
> > > > --
> > > >
> > > > Regards,
> > > > Ashish
> > > >
> > >
> > >
> > >
> > > --
> > >
> > > Regards,
> > > Ashish
> >
> ​
> --
>
> Regards,
> Ashish
>

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