Hi Rajini,
Can you please update the javadoc for every sync method to be explicit
about the fact that they should not block and the reasoning?
Ismael
On Sun, Sep 8, 2019, 9:21 AM Rajini Sivaram <rajinisiva...@gmail.com> wrote:
> Hi all,
>
> Thanks everyone for the very useful discussion. I have updated the KIP to
> make *createAcls()* and *deleteAcls()* asynchronous. Also added a section
> under `*Rejected Alternatives*` for async *authorize()*, which we can
> revisit in future if requirement arises.
>
> Regards,
>
> Rajini
>
>
> On Fri, Sep 6, 2019 at 9:17 PM Ismael Juma <ism...@juma.me.uk> wrote:
>
> > It's not uncommon for people to do large numbers of admin operations per
> > second. We've seen it many times in production. It's often a bug, but the
> > system must cope with it without affecting other users of the same Kafka
> > cluster.
> >
> > Ismael
> >
> > On Fri, Sep 6, 2019, 9:32 AM Don Bosco Durai <bo...@apache.org> wrote:
> >
> > > +1
> > >
> > > I might be wrong here, but here are few of my assumptions..
> > >
> > > 1. Unlike transactional services, in Kafka most of the users are
> > > application users, so once the connection is established, then they are
> > > going to be long running sessions. So the caching is more predictable
> and
> > > heavy lifting can be done during initial session setup and cache can be
> > > frequently updated using background thread. KSQL might have a different
> > > flow, but I don't have enough datapoints for it.
> > > 2. For publish/consume authorization calls, providing async option will
> > be
> > > misleading for Kafka like load
> > > 3. For admin calls, hopefully no one does hundreds of call in multiple
> > > thread within few seconds. This would be a bad design.
> > >
> > > So I feel, we should be okay with sync calls for now. And consider
> async
> > > in the future if needed. At least in Apache Ranger implementation, we
> > would
> > > be okay using sync operation. I am not sure if any other plugin
> > > implementation would benefit from async implementation.
> > >
> > > Thanks
> > >
> > > Bosco
> > >
> > > On 9/6/19, 8:45 AM, "Ismael Juma" <ism...@juma.me.uk> wrote:
> > >
> > > One more thing: if people have strong opinions that authorize has
> to
> > be
> > > sync, we could leave it like that for now. If needed, we can later
> > add
> > > an
> > > authorizeAsync with another method returning a Boolean indicating
> > that
> > > it's
> > > supported. That is, there is a path to evolve the interface (even
> if
> > a
> > > bit
> > > ugly).
> > >
> > > What about the other methods, is there consensus that they should
> be
> > > updated to return CompletionStage?
> > >
> > > Ismael
> > >
> > > On Fri, Sep 6, 2019, 8:32 AM Ismael Juma <ism...@juma.me.uk>
> wrote:
> > >
> > > > I'm on the move, but what Rajini said seems reasonable. I don't
> > think
> > > > using SSDs solves the issue. They can still hang for seconds when
> > > they
> > > > fail. Also, many people may not have local SSDs available (remote
> > > SSDs like
> > > > EBS hang for tens of seconds when there are issues).
> > > >
> > > > We are currently vulnerable to all of these in the normal
> > read/write
> > > path,
> > > > but it seems suboptimal to continue assuming non blocking
> behavior
> > > for
> > > > things that actually do block.
> > > >
> > > > Ismael
> > > >
> > > > On Fri, Sep 6, 2019, 8:24 AM Rajini Sivaram <
> > rajinisiva...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > >> Hi Jun,
> > > >>
> > > >> For ACLs, the size is also dependent on the number of users. So
> in
> > > >> deployments with large number of users where some users are not
> > > active, an
> > > >> LRU cache may be useful. Agree that there may be other solutions
> > > that help
> > > >> avoid the requirement for an async API even for this case.
> > > >>
> > > >> I wasn't expecting our threading model to change very much.
> Having
> > > said
> > > >> that, I haven't figured out how the purgatory can be adapted to
> > work
> > > >> efficiently for this. I was thinking:
> > > >> 1) If authorization future is complete when returning from
> > > authorize()
> > > >> call, we invoke the method immediately on the request thread
> > > >> 2) If future is not complete, the request goes into a purgatory
> > > >> 3) When future completes, we mark the request ready to be run.
> > This
> > > can be
> > > >> done on ForkJoinPool.commonPool or an executor. But we don't
> > handle
> > > the
> > > >> request on that thread
> > > >> 4) We handle the request on the request thread after 3).
> > > >>
> > > >> I totally agree that this adds complexity to the code. Will wait
> > for
> > > >> Ismael
> > > >> to comment on whether he had a different model in mind.
> > > >>
> > > >> Thanks,
> > > >>
> > > >> Rajini
> > > >>
> > > >>
> > > >> On Thu, Sep 5, 2019 at 10:29 PM Jun Rao <j...@confluent.io>
> wrote:
> > > >>
> > > >> > Hi, Ismael,
> > > >> >
> > > >> > Yes, I agree that there are 2 separate discussion points, one
> on
> > > the API
> > > >> > and the other on the implementation.
> > > >> >
> > > >> > First on the API, in general, my feeling is that the
> authorize()
> > > call is
> > > >> > expected to be fast to reduce request latency. Adding an async
> > API
> > > >> could be
> > > >> > mis-leading since it might give users the impression that it
> > > would be
> > > >> > arbitrarily long. In the short term, currently, we cache all
> > > partition
> > > >> > level metadata on every broker. The authorization metadata is
> > > >> proportional
> > > >> > to the number of topics, which typically is less in size. So
> it
> > > we can
> > > >> > cache all partition level metadata, we should be able to cache
> > all
> > > >> > authorization metadata. Longer term, if we want to support
> more
> > > metadata
> > > >> > beyond memory, another option is to put all metadata in an SSD
> > > backed
> > > >> > key/value store. The lookup time could still be under 1ms,
> which
> > > we
> > > >> could
> > > >> > potentially absorb in the request threads. So, I am not sure
> > async
> > > >> > authorize() api is strictly needed even in the long term.
> > > >> >
> > > >> > Second on the implementation. I was more concerned about the
> > > impact on
> > > >> our
> > > >> > threading model. If we make authorize() async, some threads
> have
> > > to
> > > >> > complete the future when it's ready. It seems to me that those
> > > threads
> > > >> have
> > > >> > to be inside the Authorizer implementation since only it knows
> > > when to
> > > >> > complete a future? If so, it feels weird that all the logic of
> > > request
> > > >> > handling post authorize() is now handled by external threads
> > > inside a
> > > >> > plugin. It's not clear how we can configure and monitor them,
> > and
> > > how
> > > >> > throttling would work. Perhaps we can provide a bit more
> detail
> > > on the
> > > >> new
> > > >> > threading model.
> > > >> >
> > > >> > Thanks,
> > > >> >
> > > >> > Jun
> > > >> >
> > > >> > On Wed, Sep 4, 2019 at 9:56 PM Ismael Juma <ism...@juma.me.uk
> >
> > > wrote:
> > > >> >
> > > >> > > Hi Jun,
> > > >> > >
> > > >> > > I think it's important to discuss the pluggable API versus
> the
> > > calling
> > > >> > > implementation as two separate points.
> > > >> > >
> > > >> > > From an API perspective, are you suggesting that we should
> > tell
> > > users
> > > >> > that
> > > >> > > they cannot block in authorize? Or are you saying that it's
> OK
> > > to
> > > >> block
> > > >> > on
> > > >> > > authorize on occasion and the recommendation would be for
> > > people to
> > > >> > > increase the number of threads in the request thread pool?
> > > >> > Architecturally,
> > > >> > > this feels wrong in my opinion.
> > > >> > >
> > > >> > > From the calling implementation perspective, you don't need
> a
> > > >> callback.
> > > >> > You
> > > >> > > can basically say:
> > > >> > >
> > > >> > > authorize(...).map { result =>
> > > >> > > ...
> > > >> > > }
> > > >> > >
> > > >> > > And then have a common method take that Future and handle it
> > > >> > synchronously
> > > >> > > if it's already complete or submit it to the purgatory if
> not.
> > > This is
> > > >> > > similar to how many modern async web libraries work.
> > > >> > >
> > > >> > > As Rajini said, this could be done later. The initial
> > > implementation
> > > >> > could
> > > >> > > simply do `toCompletableFuture().get()` to do it
> > synchronously.
> > > But it
> > > >> > > would mean that we could add this functionality without
> > > changing the
> > > >> > > pluggable interface.
> > > >> > >
> > > >> > > Ismael
> > > >> > >
> > > >> > > On Wed, Sep 4, 2019 at 5:29 AM Jun Rao <j...@confluent.io>
> > > wrote:
> > > >> > >
> > > >> > > > Hi, Rajini,
> > > >> > > >
> > > >> > > > Thanks for the KIP. I was concerned about #4 too. If we
> > > change the
> > > >> > > handling
> > > >> > > > of all requests to use an async authorize() api, will that
> > > cause the
> > > >> > code
> > > >> > > > much harder to understand? There are quite a few callbacks
> > > already.
> > > >> I
> > > >> > am
> > > >> > > > not sure that we want to introduce more of those. The
> > benefit
> > > from
> > > >> > async
> > > >> > > > authorize() api seems limited.
> > > >> > > >
> > > >> > > > Jun
> > > >> > > >
> > > >> > > > On Wed, Sep 4, 2019 at 5:38 PM Rajini Sivaram <
> > > >> rajinisiva...@gmail.com
> > > >> > >
> > > >> > > > wrote:
> > > >> > > >
> > > >> > > > > Hi Don,
> > > >> > > > >
> > > >> > > > > Thanks for your note.
> > > >> > > > >
> > > >> > > > > 1) The intention is to avoid blocking in the calling
> > > thread. We
> > > >> > already
> > > >> > > > > have several requests that are put into a purgatory when
> > > waiting
> > > >> for
> > > >> > > > remote
> > > >> > > > > communication, for example produce request waiting for
> > > >> replication.
> > > >> > > Since
> > > >> > > > > we have a limited number of request threads in the
> broker,
> > > we
> > > >> want to
> > > >> > > > make
> > > >> > > > > progress with other requests, while one is waiting on
> any
> > > form of
> > > >> > > remote
> > > >> > > > > communication.
> > > >> > > > >
> > > >> > > > > 2) Async management calls will be useful with the
> default
> > > >> authorizer
> > > >> > > when
> > > >> > > > > KIP-500 removes ZK and we rely on Kafka instead. Our
> > current
> > > >> ZK-based
> > > >> > > > > implementation as well as any custom implementations
> that
> > > don't
> > > >> want
> > > >> > to
> > > >> > > > be
> > > >> > > > > async will just need to return a sync'ed value. So
> instead
> > > of
> > > >> > > returning `
> > > >> > > > > value`, the code would just return
> > > >> > > > > `CompletableFuture.completedFuture(value)
> > > >> > > > > `. So it would be just a single line change in the
> > > implementation
> > > >> > with
> > > >> > > > the
> > > >> > > > > new API. The caller would treat completedFuture exactly
> as
> > > it does
> > > >> > > today,
> > > >> > > > > processing the request synchronously without using a
> > > purgatory.
> > > >> > > > >
> > > >> > > > > 3) For implementations that return a completedFuture as
> > > described
> > > >> in
> > > >> > > 2),
> > > >> > > > > the behaviour would remain exactly the same. No
> additional
> > > >> threads or
> > > >> > > > > purgatory will be used for this case. So there would be
> no
> > > >> > performance
> > > >> > > > > penalty. For implementations that return a future that
> is
> > > not
> > > >> > complete,
> > > >> > > > we
> > > >> > > > > prioritise running more requests concurrently. So in a
> > > deployment
> > > >> > with
> > > >> > > a
> > > >> > > > > large number of clients, we would improve performance by
> > > allowing
> > > >> > other
> > > >> > > > > requests to be processed on the request threads while
> some
> > > are
> > > >> > waiting
> > > >> > > > for
> > > >> > > > > authorization metadata.
> > > >> > > > >
> > > >> > > > > 4) I was concerned about this too. The goal is to make
> the
> > > API
> > > >> > flexible
> > > >> > > > > enough to handle large scale deployments in future when
> > > caching
> > > >> all
> > > >> > > > > authorization metadata in each broker is not viable.
> Using
> > > an
> > > >> async
> > > >> > API
> > > >> > > > > that returns CompletionStage, the caller has the option
> to
> > > handle
> > > >> the
> > > >> > > > > result synchronously or asynchronously, so we don't
> > > necessarily
> > > >> need
> > > >> > to
> > > >> > > > > update the calling code right away. Custom authorizers
> > > using the
> > > >> > async
> > > >> > > > API
> > > >> > > > > have full control over whether authorization is
> performed
> > > in-line
> > > >> > since
> > > >> > > > > completedFuture will always be handled synchronously. We
> > do
> > > need
> > > >> to
> > > >> > > > update
> > > >> > > > > KafkaApis to take advantage of the asynchronous API to
> > > improve
> > > >> scale.
> > > >> > > > Even
> > > >> > > > > though this is a big change, since we will be doing the
> > > same for
> > > >> all
> > > >> > > > > requests, it shouldn't be too hard to maintain since the
> > > same
> > > >> pattern
> > > >> > > > will
> > > >> > > > > be used for all requests.
> > > >> > > > >
> > > >> > > > > Regards,
> > > >> > > > >
> > > >> > > > > Rajini
> > > >> > > > >
> > > >> > > > > On Tue, Sep 3, 2019 at 11:48 PM Don Bosco Durai <
> > > bo...@apache.org
> > > >> >
> > > >> > > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi Rajini
> > > >> > > > > >
> > > >> > > > > > Help me understand this a bit more.
> > > >> > > > > >
> > > >> > > > > > 1. For all practical purpose, without authorization
> you
> > > can't
> > > >> go to
> > > >> > > the
> > > >> > > > > > next step. The calling code needs to block anyway. So
> > > should we
> > > >> > just
> > > >> > > > let
> > > >> > > > > > the implementation code do the async part?
> > > >> > > > > > 2. If you feel management calls need to be async, then
> > we
> > > should
> > > >> > > > consider
> > > >> > > > > > another set of async APIs. I don't feel we should
> > > complicate it
> > > >> > > > further (
> > > >> > > > > > 3. Another concern I have is wrt performance. Kafka
> has
> > > been
> > > >> built
> > > >> > to
> > > >> > > > > > handle 1000s per second requests. Not sure whether
> > making
> > > it
> > > >> async
> > > >> > > will
> > > >> > > > > add
> > > >> > > > > > any unnecessary overhead.
> > > >> > > > > > 4. How much complication would this add on the calling
> > > side?
> > > >> And is
> > > >> > > it
> > > >> > > > > > worth it?
> > > >> > > > > >
> > > >> > > > > > Thanks
> > > >> > > > > >
> > > >> > > > > > Bosco
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > On 9/3/19, 8:50 AM, "Rajini Sivaram" <
> > > rajinisiva...@gmail.com>
> > > >> > > wrote:
> > > >> > > > > >
> > > >> > > > > > Hi all,
> > > >> > > > > >
> > > >> > > > > > Ismael brought up a point that it will be good to
> > > make the
> > > >> > > > Authorizer
> > > >> > > > > > interface asynchronous to avoid blocking request
> > > threads
> > > >> during
> > > >> > > > > remote
> > > >> > > > > > operations.
> > > >> > > > > >
> > > >> > > > > > 1) Since we want to support different backends for
> > > >> > authorization
> > > >> > > > > > metadata,
> > > >> > > > > > making createAcls() and deleteAcls() asynchronous
> > > makes
> > > >> sense
> > > >> > > since
> > > >> > > > > > these
> > > >> > > > > > always involve remote operations. When KIP-500
> > removes
> > > >> > ZooKeeper,
> > > >> > > > we
> > > >> > > > > > would
> > > >> > > > > > want to move ACLs to Kafka and async updates will
> > > avoid
> > > >> > > unnecessary
> > > >> > > > > > blocking.
> > > >> > > > > > 2) For authorize() method, we currently use cached
> > > ACLs in
> > > >> the
> > > >> > > > > built-in
> > > >> > > > > > authorizers, so synchronous authorise operations
> > work
> > > well
> > > >> now.
> > > >> > > But
> > > >> > > > > > async
> > > >> > > > > > authorize() would support this scenario as well as
> > > >> authorizers
> > > >> > in
> > > >> > > > > large
> > > >> > > > > > organisations where an LRU cache would enable a
> > > smaller
> > > >> cache
> > > >> > > even
> > > >> > > > > > when the
> > > >> > > > > > backend holds a large amount of ACLs for
> > infrequently
> > > used
> > > >> > > > resources
> > > >> > > > > or
> > > >> > > > > > users who don't use the system frequently.
> > > >> > > > > >
> > > >> > > > > > For both cases, the built-in authorizer will
> > continue
> > > to be
> > > >> > > > > > synchronous,
> > > >> > > > > > using CompletableFuture.completedFuture() to
> return
> > > the
> > > >> actual
> > > >> > > > > result.
> > > >> > > > > > But
> > > >> > > > > > async API will make custom authorizer
> > implementations
> > > more
> > > >> > > > flexible.
> > > >> > > > > I
> > > >> > > > > > would like to know if there are any concerns with
> > > these
> > > >> changes
> > > >> > > > > before
> > > >> > > > > > updating the KIP.
> > > >> > > > > >
> > > >> > > > > > *Proposed API:*
> > > >> > > > > > public interface Authorizer extends Configurable,
> > > Closeable
> > > >> {
> > > >> > > > > >
> > > >> > > > > > Map<Endpoint, CompletionStage<Void>>
> > > >> > > start(AuthorizerServerInfo
> > > >> > > > > > serverInfo);
> > > >> > > > > > List<CompletionStage<AuthorizationResult>>
> > > >> > > > > > authorize(AuthorizableRequestContext
> requestContext,
> > > >> > List<Action>
> > > >> > > > > > actions);
> > > >> > > > > > List<CompletionStage<AclCreateResult>>
> > > >> > > > > > createAcls(AuthorizableRequestContext
> > requestContext,
> > > >> > > > > List<AclBinding>
> > > >> > > > > > aclBindings);
> > > >> > > > > > List<CompletionStage<AclDeleteResult>>
> > > >> > > > > > deleteAcls(AuthorizableRequestContext
> > requestContext,
> > > >> > > > > > List<AclBindingFilter> aclBindingFilters);
> > > >> > > > > > CompletionStage<Collection<AclBinding>>
> > > >> > acls(AclBindingFilter
> > > >> > > > > > filter);
> > > >> > > > > > }
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > Thank you,
> > > >> > > > > >
> > > >> > > > > > Rajini
> > > >> > > > > >
> > > >> > > > > > On Sun, Aug 18, 2019 at 6:25 PM Don Bosco Durai <
> > > >> > > bo...@apache.org>
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > Hi Rajini
> > > >> > > > > > >
> > > >> > > > > > > Thanks for clarifying. I am good for now.
> > > >> > > > > > >
> > > >> > > > > > > Regards
> > > >> > > > > > >
> > > >> > > > > > > Bosco
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > On 8/16/19, 11:30 AM, "Rajini Sivaram" <
> > > >> > > rajinisiva...@gmail.com
> > > >> > > > >
> > > >> > > > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > Hi Don,
> > > >> > > > > > >
> > > >> > > > > > > That should be fine. I guess Ranger loads
> > > policies
> > > >> from
> > > >> > the
> > > >> > > > > > database
> > > >> > > > > > > synchronously when the authorizer is
> > configured,
> > > >> similar
> > > >> > to
> > > >> > > > > > > SimpleAclAuthorizer loading from ZooKeeper.
> > > Ranger can
> > > >> > > > continue
> > > >> > > > > > to load
> > > >> > > > > > > synchronously from `configure()` or
> `start()`
> > > and
> > > >> return
> > > >> > an
> > > >> > > > > > empty map
> > > >> > > > > > > from
> > > >> > > > > > > `start()`. That would retain the existing
> > > behaviour..
> > > >> > When
> > > >> > > > the
> > > >> > > > > > same
> > > >> > > > > > > database stores policies for all listeners
> and
> > > the
> > > >> > policies
> > > >> > > > are
> > > >> > > > > > not
> > > >> > > > > > > stored
> > > >> > > > > > > in Kafka, there is no value in making the
> load
> > > >> > > asynchronous.
> > > >> > > > > > >
> > > >> > > > > > > Regards,
> > > >> > > > > > >
> > > >> > > > > > > Rajini
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > On Fri, Aug 16, 2019 at 6:43 PM Don Bosco
> > Durai
> > > <
> > > >> > > > > > bo...@apache.org>
> > > >> > > > > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Hi Rajini
> > > >> > > > > > > >
> > > >> > > > > > > > Assuming this doesn't affect custom
> plugins,
> > > I don't
> > > >> > have
> > > >> > > > any
> > > >> > > > > > > concerns
> > > >> > > > > > > > regarding this.
> > > >> > > > > > > >
> > > >> > > > > > > > I do have one question regarding:
> > > >> > > > > > > >
> > > >> > > > > > > > "For authorizers that don’t store metadata
> > in
> > > >> > ZooKeeper,
> > > >> > > > > > ensure that
> > > >> > > > > > > > authorizer metadata for each listener is
> > > available
> > > >> > before
> > > >> > > > > > starting
> > > >> > > > > > > up the
> > > >> > > > > > > > listener. This enables different
> > authorization
> > > >> metadata
> > > >> > > > > stores
> > > >> > > > > > for
> > > >> > > > > > > > different listeners."
> > > >> > > > > > > >
> > > >> > > > > > > > Since Ranger uses its own database for
> > storing
> > > >> > policies,
> > > >> > > do
> > > >> > > > > you
> > > >> > > > > > > anticipate
> > > >> > > > > > > > any issues?
> > > >> > > > > > > >
> > > >> > > > > > > > Thanks
> > > >> > > > > > > >
> > > >> > > > > > > > Bosco
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > On 8/16/19, 6:49 AM, "Rajini Sivaram" <
> > > >> > > > > > rajinisiva...@gmail.com>
> > > >> > > > > > > wrote:
> > > >> > > > > > > >
> > > >> > > > > > > > Hi all,
> > > >> > > > > > > >
> > > >> > > > > > > > I made another change to the KIP. The
> > KIP
> > > was
> > > >> > > > originally
> > > >> > > > > > > proposing to
> > > >> > > > > > > > extend SimpleAclAuthorizer to also
> > > implement the
> > > >> > new
> > > >> > > > API
> > > >> > > > > > (in
> > > >> > > > > > > addition
> > > >> > > > > > > > to
> > > >> > > > > > > > the existing API). But since we use
> the
> > > new API
> > > >> > when
> > > >> > > > > > available,
> > > >> > > > > > > this
> > > >> > > > > > > > can
> > > >> > > > > > > > break custom authorizers that extend
> > this
> > > class
> > > >> and
> > > >> > > > > > override
> > > >> > > > > > > specific
> > > >> > > > > > > > methods of the old API. To avoid
> > breaking
> > > any
> > > >> > > existing
> > > >> > > > > > custom
> > > >> > > > > > > > implementations that extend this
> class,
> > > >> > particularly
> > > >> > > > > > because it
> > > >> > > > > > > is in
> > > >> > > > > > > > the
> > > >> > > > > > > > public package kafka.security.auth,
> the
> > > KIP now
> > > >> > > > proposes
> > > >> > > > > to
> > > >> > > > > > > retain the
> > > >> > > > > > > > old
> > > >> > > > > > > > authorizer as-is. SimpleAclAuthorizer
> > > will
> > > >> > continue
> > > >> > > to
> > > >> > > > > > > implement the
> > > >> > > > > > > > old
> > > >> > > > > > > > API, but will be deprecated. A new
> > > authorizer
> > > >> > > > > > implementation
> > > >> > > > > > > >
> kafka.security.authorizer.AclAuthorizer
> > > will be
> > > >> > added
> > > >> > > > for
> > > >> > > > > > the
> > > >> > > > > > > new API
> > > >> > > > > > > > (this
> > > >> > > > > > > > will not be in the public package).
> > > >> > > > > > > >
> > > >> > > > > > > > Please let me know if you have any
> > > concerns.
> > > >> > > > > > > >
> > > >> > > > > > > > Regards,
> > > >> > > > > > > >
> > > >> > > > > > > > Rajini
> > > >> > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > > > On Fri, Aug 16, 2019 at 8:48 AM Rajini
> > > Sivaram <
> > > >> > > > > > > > rajinisiva...@gmail.com>
> > > >> > > > > > > > wrote:
> > > >> > > > > > > >
> > > >> > > > > > > > > Thanks Colin.
> > > >> > > > > > > > >
> > > >> > > > > > > > > If there are no other concerns, I
> will
> > > start
> > > >> vote
> > > >> > > > later
> > > >> > > > > > today.
> > > >> > > > > > > Many
> > > >> > > > > > > > thanks
> > > >> > > > > > > > > to every one for the feedback.
> > > >> > > > > > > > >
> > > >> > > > > > > > > Regards,
> > > >> > > > > > > > >
> > > >> > > > > > > > > Rajini
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > > On Thu, Aug 15, 2019 at 11:57 PM
> Colin
> > > McCabe
> > > >> <
> > > >> > > > > > > cmcc...@apache.org>
> > > >> > > > > > > > wrote:
> > > >> > > > > > > > >
> > > >> > > > > > > > >> Thanks, Rajini. It looks good to
> me.
> > > >> > > > > > > > >>
> > > >> > > > > > > > >> best,
> > > >> > > > > > > > >> Colin
> > > >> > > > > > > > >>
> > > >> > > > > > > > >>
> > > >> > > > > > > > >> On Thu, Aug 15, 2019, at 11:37,
> > Rajini
> > > >> Sivaram
> > > >> > > > wrote:
> > > >> > > > > > > > >> > Hi Colin,
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > Thanks for the review. I have
> > > updated the
> > > >> KIP
> > > >> > to
> > > >> > > > > move
> > > >> > > > > > the
> > > >> > > > > > > > interfaces for
> > > >> > > > > > > > >> > request context and server info
> to
> > > the
> > > >> > > authorizer
> > > >> > > > > > package.
> > > >> > > > > > > These
> > > >> > > > > > > > are now
> > > >> > > > > > > > >> > called AuthorizableRequestContext
> > and
> > > >> > > > > > AuthorizerServerInfo.
> > > >> > > > > > > > Endpoint is
> > > >> > > > > > > > >> now
> > > >> > > > > > > > >> > a class in
> org.apache.kafka.common
> > > to make
> > > >> it
> > > >> > > > > reusable
> > > >> > > > > > > since we
> > > >> > > > > > > > already
> > > >> > > > > > > > >> > have multiple implementations of
> > it.
> > > I have
> > > >> > > > removed
> > > >> > > > > > > requestName
> > > >> > > > > > > > from the
> > > >> > > > > > > > >> > request context interface since
> > > authorizers
> > > >> > can
> > > >> > > > > > distinguish
> > > >> > > > > > > > follower
> > > >> > > > > > > > >> fetch
> > > >> > > > > > > > >> > and consumer fetch from the
> > > operation being
> > > >> > > > > > authorized. So
> > > >> > > > > > > 16-bit
> > > >> > > > > > > > >> request
> > > >> > > > > > > > >> > type should be sufficient for
> audit
> > > >> logging.
> > > >> > > Also
> > > >> > > > > > replaced
> > > >> > > > > > > > AuditFlag
> > > >> > > > > > > > >> with
> > > >> > > > > > > > >> > two booleans as you suggested.
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > Can you take another look and see
> > if
> > > the
> > > >> KIP
> > > >> > is
> > > >> > > > > ready
> > > >> > > > > > for
> > > >> > > > > > > voting?
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > Thanks for all your help!
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > Regards,
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > Rajini
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > On Wed, Aug 14, 2019 at 8:59 PM
> > Colin
> > > >> McCabe <
> > > >> > > > > > > cmcc...@apache.org>
> > > >> > > > > > > > >> wrote:
> > > >> > > > > > > > >> >
> > > >> > > > > > > > >> > > Hi Rajini,
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > I think it would be good to
> > rename
> > > >> > > > > > KafkaRequestContext to
> > > >> > > > > > > > something
> > > >> > > > > > > > >> like
> > > >> > > > > > > > >> > > AuthorizableRequestContext, and
> > > put it in
> > > >> > the
> > > >> > > > > > > > >> > >
> > org.apache.kafka.server.authorizer
> > > >> > namespace.
> > > >> > > > If
> > > >> > > > > > we put
> > > >> > > > > > > it in
> > > >> > > > > > > > the
> > > >> > > > > > > > >> > > org.apache.kafka.common
> > namespace,
> > > then
> > > >> it's
> > > >> > > not
> > > >> > > > > > really
> > > >> > > > > > > clear
> > > >> > > > > > > > that
> > > >> > > > > > > > >> it's
> > > >> > > > > > > > >> > > part of the Authorizer API.
> > Since
> > > this
> > > >> > class
> > > >> > > is
> > > >> > > > > > purely an
> > > >> > > > > > > > interface,
> > > >> > > > > > > > >> with
> > > >> > > > > > > > >> > > no concrete implementation of
> > > anything,
> > > >> > > there's
> > > >> > > > > > nothing
> > > >> > > > > > > common
> > > >> > > > > > > > to
> > > >> > > > > > > > >> really
> > > >> > > > > > > > >> > > reuse in any case. We
> definitely
> > > don't
> > > >> want
> > > >> > > > > > someone to
> > > >> > > > > > > > accidentally
> > > >> > > > > > > > >> add or
> > > >> > > > > > > > >> > > remove methods because they
> think
> > > this is
> > > >> > just
> > > >> > > > > > another
> > > >> > > > > > > internal
> > > >> > > > > > > > class
> > > >> > > > > > > > >> used
> > > >> > > > > > > > >> > > for requests.
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > The BrokerInfo class is a nice
> > > >> improvement.
> > > >> > > It
> > > >> > > > > > looks
> > > >> > > > > > > like it
> > > >> > > > > > > > will be
> > > >> > > > > > > > >> > > useful for passing in
> information
> > > about
> > > >> the
> > > >> > > > > context
> > > >> > > > > > we're
> > > >> > > > > > > > running
> > > >> > > > > > > > >> in. It
> > > >> > > > > > > > >> > > would be nice to call this
> class
> > > >> ServerInfo
> > > >> > > > rather
> > > >> > > > > > than
> > > >> > > > > > > > BrokerInfo,
> > > >> > > > > > > > >> since
> > > >> > > > > > > > >> > > we will want to run the
> > authorizer
> > > on
> > > >> > > > controllers
> > > >> > > > > > as well
> > > >> > > > > > > as on
> > > >> > > > > > > > >> brokers,
> > > >> > > > > > > > >> > > and the controller may run as a
> > > separate
> > > >> > > process
> > > >> > > > > > post
> > > >> > > > > > > KIP-500.
> > > >> > > > > > > > I also
> > > >> > > > > > > > >> > > think that this class should be
> > in
> > > the
> > > >> > > > > > > > >> org.apache.kafka.server.authorizer
> > > >> > > > > > > > >> > > namespace. Again, it is an
> > > interface,
> > > >> not a
> > > >> > > > > > concrete
> > > >> > > > > > > > implementation,
> > > >> > > > > > > > >> and
> > > >> > > > > > > > >> > > it's an interface that is very
> > > >> specifically
> > > >> > > for
> > > >> > > > > the
> > > >> > > > > > > authorizer.
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > I agree that we probably don't
> > have
> > > >> enough
> > > >> > > > > > information
> > > >> > > > > > > > preserved for
> > > >> > > > > > > > >> > > requests currently to always
> know
> > > what
> > > >> > entity
> > > >> > > > made
> > > >> > > > > > them.
> > > >> > > > > > > So we
> > > >> > > > > > > > can
> > > >> > > > > > > > >> leave
> > > >> > > > > > > > >> > > that out for now (except in the
> > > special
> > > >> case
> > > >> > > of
> > > >> > > > > > Fetch).
> > > >> > > > > > > > Perhaps we
> > > >> > > > > > > > >> can add
> > > >> > > > > > > > >> > > this later if it's needed.
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > I understand the intention
> behind
> > > >> > > > > AuthorizationMode
> > > >> > > > > > > (which is
> > > >> > > > > > > > now
> > > >> > > > > > > > >> called
> > > >> > > > > > > > >> > > AuditFlag in the latest
> > > revision). But
> > > >> it
> > > >> > > still
> > > >> > > > > > feels
> > > >> > > > > > > > complex. What
> > > >> > > > > > > > >> if we
> > > >> > > > > > > > >> > > just had two booleans in
> Action:
> > > >> > logSuccesses
> > > >> > > > and
> > > >> > > > > > > logFailures?
> > > >> > > > > > > > That
> > > >> > > > > > > > >> seems
> > > >> > > > > > > > >> > > to cover all the cases here.
> > > >> > > > MANDATORY_AUTHORIZE
> > > >> > > > > =
> > > >> > > > > > true,
> > > >> > > > > > > true.
> > > >> > > > > > > > >> > > OPTIONAL_AUTHORIZE = true,
> false.
> > > >> FILTER =
> > > >> > > > true,
> > > >> > > > > > false.
> > > >> > > > > > > > >> LIST_AUTHORIZED =
> > > >> > > > > > > > >> > > false, false. Would there be
> > > anything
> > > >> lost
> > > >> > > > versus
> > > >> > > > > > having
> > > >> > > > > > > the
> > > >> > > > > > > > enum?
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > best,
> > > >> > > > > > > > >> > > Colin
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > >
> > > >> > > > > > > > >> > > On Wed, Aug 14, 2019, at 06:29,
> > > Mickael
> > > >> > Maison
> > > >> > > > > > wrote:
> > > >> > > > > > > > >> > > > Hi Rajini,
> > > >> > > > > > > > >> > > >
> > > >> > > > > > > > >> > > > Thanks for the KIP!
> > > >> > > > > > > > >> > > > I really like that
> authorize()
> > > will be
> > > >> > able
> > > >> > > to
> > > >> > > > > > take a
> > > >> > > > > > > batch of
> > > >> > > > > > > > >> > > > requests, this will speed up
> > many
> > > >> > > > > implementations!
> > > >> > > > > > > > >> > > >
> > > >> > > > > > > > >> > > > On Tue, Aug 13, 2019 at 5:57
> PM
> > > Rajini
> > > >> > > > Sivaram <
> > > >> > > > > > > > >> rajinisiva...@gmail.com>
> > > >> > > > > > > > >> > > wrote:
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > Thanks David! I have fixed
> > the
> > > typo.
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > Also made a couple of
> changes
> > > to make
> > > >> > the
> > > >> > > > > > context
> > > >> > > > > > > > interfaces more
> > > >> > > > > > > > >> > > generic.
> > > >> > > > > > > > >> > > > > KafkaRequestContext now
> > > returns the
> > > >> > 16-bit
> > > >> > > > API
> > > >> > > > > > key as
> > > >> > > > > > > Colin
> > > >> > > > > > > > >> suggested
> > > >> > > > > > > > >> > > as
> > > >> > > > > > > > >> > > > > well as the friendly name
> > used
> > > in
> > > >> > metrics
> > > >> > > > > which
> > > >> > > > > > are
> > > >> > > > > > > useful
> > > >> > > > > > > > in
> > > >> > > > > > > > >> audit
> > > >> > > > > > > > >> > > logs.
> > > >> > > > > > > > >> > > > > `Authorizer#start` is now
> > > provided a
> > > >> > > generic
> > > >> > > > > > > `BrokerInfo`
> > > >> > > > > > > > >> interface
> > > >> > > > > > > > >> > > that
> > > >> > > > > > > > >> > > > > gives cluster id, broker id
> > and
> > > >> endpoint
> > > >> > > > > > information.
> > > >> > > > > > > The
> > > >> > > > > > > > generic
> > > >> > > > > > > > >> > > interface
> > > >> > > > > > > > >> > > > > can potentially be used in
> > > other
> > > >> broker
> > > >> > > > > plugins
> > > >> > > > > > in
> > > >> > > > > > > future
> > > >> > > > > > > > and
> > > >> > > > > > > > >> provides
> > > >> > > > > > > > >> > > > > dynamically generated
> configs
> > > like
> > > >> > broker
> > > >> > > id
> > > >> > > > > > and ports
> > > >> > > > > > > > which are
> > > >> > > > > > > > >> > > currently
> > > >> > > > > > > > >> > > > > not available to plugins
> > > unless these
> > > >> > > > configs
> > > >> > > > > > are
> > > >> > > > > > > statically
> > > >> > > > > > > > >> > > configured.
> > > >> > > > > > > > >> > > > > Please let me know if there
> > > are any
> > > >> > > > concerns.
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > Regards,
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > Rajini
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > On Tue, Aug 13, 2019 at
> 4:30
> > > PM David
> > > >> > > Jacot
> > > >> > > > <
> > > >> > > > > > > > dja...@confluent.io>
> > > >> > > > > > > > >> > > wrote:
> > > >> > > > > > > > >> > > > >
> > > >> > > > > > > > >> > > > > > Hi Rajini,
> > > >> > > > > > > > >> > > > > >
> > > >> > > > > > > > >> > > > > > Thank you for the update!
> > It
> > > looks
> > > >> > good
> > > >> > > to
> > > >> > > > > me.
> > > >> > > > > > > There is a
> > > >> > > > > > > > typo
> > > >> > > > > > > > >> in the
> > > >> > > > > > > > >> > > > > > `AuditFlag` enum:
> > > >> > `MANDATORY_AUTHOEIZE`
> > > >> > > ->
> > > >> > > > > > > > >> `MANDATORY_AUTHORIZE`.
> > > >> > > > > > > > >> > > > > >
> > > >> > > > > > > > >> > > > > > Regards,
> > > >> > > > > > > > >> > > > > > David
> > > >> > > > > > > > >> > > > > >
> > > >> > > > > > > > >> > > > > > On Mon, Aug 12, 2019 at
> > 2:54
> > > PM
> > > >> Rajini
> > > >> > > > > > Sivaram <
> > > >> > > > > > > > >> > > rajinisiva...@gmail.com>
> > > >> > > > > > > > >> > > > > > wrote:
> > > >> > > > > > > > >> > > > > >
> > > >> > > > > > > > >> > > > > > > Hi David,
> > > >> > > > > > > > >> > > > > > >
> > > >> > > > > > > > >> > > > > > > Thanks for reviewing
> the
> > > KIP!
> > > >> Since
> > > >> > > > > > questions
> > > >> > > > > > > about
> > > >> > > > > > > > >> `authorization
> > > >> > > > > > > > >> > > mode`
> > > >> > > > > > > > >> > > > > > > and `count` have come
> up
> > > multiple
> > > >> > > > times, I
> > > >> > > > > > have
> > > >> > > > > > > renamed
> > > >> > > > > > > > both.
> > > >> > > > > > > > >> > > > > > >
> > > >> > > > > > > > >> > > > > > > 1) Renamed `count` to
> > > >> > > > > > `resourceReferenceCount`.
> > > >> > > > > > > It is
> > > >> > > > > > > > the
> > > >> > > > > > > > >> number
> > > >> > > > > > > > >> > > of times
> > > >> > > > > > > > >> > > > > > > the resource being
> > > authorized is
> > > >> > > > > referenced
> > > >> > > > > > > within the
> > > >> > > > > > > > >> request.
> > > >> > > > > > > > >> > > > > > >
> > > >> > > > > > > > >> > > > > > > 2) Renamed
> > > `AuthorizationMode` to
> > > >> > > > > > `AuditFlag`. It
> > > >> > > > > > > is
> > > >> > > > > > > > provided
> > > >> > > > > > > > >> to
> > > >> > > > > > > > >> > > improve
> > > >> > > > > > > > >> > > > > > > audit logging in the
> > > authorizer.
> > > >> The
> > > >> > > > enum
> > > >> > > > > > values
> > > >> > > > > > > have
> > > >> > > > > > > > javadoc
> > > >> > > > > > > > >> which
> > > >> > > > > > > > >> > > > > > > indicate how the
> > > authorization
> > > >> > result
> > > >> > > is
> > > >> > > > > > used in
> > > >> > > > > > > each
> > > >> > > > > > > > of the
> > > >> > > > > > > > >> modes
> > > >> > > > > > > > >> > > to
> > > >> > > > > > > > >> > > > > > > enable authorizers to
> log
> > > audit
> > > >> > > messages
> > > >> > > > > at
> > > >> > > > > > the
> > > >> > > > > > > right
> > > >> > > > > > > > severity
> > > >> > > > > > > > >> > > level.
> > > >> > > > > > > > >> > > > > > >
> > > >> >
