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.
> > >> > > > > > > > >> > > > > > >
> > >> > > > > > > > >> > > > > > > Regards,
> > >> > > > > > > > >> > > > > > >
> > >> > > > > > > > >> > > > > > > Rajini
> > >> > > > > > > > >> > > > > > >
> > >> > > > > > > > >> > > > > > > On Mon, Aug 12, 2019 at
> > 5:54 PM
> > >> > David
> > >> > > > > Jacot
> > >> > > > > > <
> > >> > > > > > > > >> dja...@confluent.io>
> > >> > > > > > > > >> > > wrote:
> > >> > > > > > > > >> > > > > > >
> > >> > > > > > > > >> > > > > > > > Hi Rajini,
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > Thank you for the KIP.
> > >> Overall, it
> > >> > > > looks
> > >> > > > > > good
> > >> > > > > > > to me.
> > >> > > > > > > > I have
> > >> > > > > > > > >> few
> > >> > > > > > > > >> > > > > > > > questions/suggestions:
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > 1. It is hard to grasp
> > what
> > >> > > > > > `Action#count` is
> > >> > > > > > > for. I
> > >> > > > > > > > guess I
> > >> > > > > > > > >> > > understand
> > >> > > > > > > > >> > > > > > > > where you want to go
> > with it
> > >> but
> > >> > it
> > >> > > > took
> > >> > > > > > me a
> > >> > > > > > > while to
> > >> > > > > > > > >> figure it
> > >> > > > > > > > >> > > out.
> > >> > > > > > > > >> > > > > > > > Perhaps, we could come
> > up with
> > >> a
> > >> > > > better
> > >> > > > > > name
> > >> > > > > > > than
> > >> > > > > > > > `count`?
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > 2. I had a hard time
> > trying to
> > >> > > > > understand
> > >> > > > > > the
> > >> > > > > > > > >> `AuthorizationMode`
> > >> > > > > > > > >> > > > > > > concept,
> > >> > > > > > > > >> > > > > > > > especially wrt. the
> > OPTIONAL
> > >> one.
> > >> > My
> > >> > > > > > > understanding is
> > >> > > > > > > > that
> > >> > > > > > > > >> an
> > >> > > > > > > > >> > > ACL is
> > >> > > > > > > > >> > > > > > > either
> > >> > > > > > > > >> > > > > > > > defined or not. Could
> you
> > >> > elaborate
> > >> > > a
> > >> > > > > bit
> > >> > > > > > more
> > >> > > > > > > on
> > >> > > > > > > > that?
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > Thanks,
> > >> > > > > > > > >> > > > > > > > David
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > On Fri, Aug 9, 2019 at
> > 10:26 PM
> > >> > Don
> > >> > > > > Bosco
> > >> > > > > > Durai
> > >> > > > > > > <
> > >> > > > > > > > >> > > bo...@apache.org>
> > >> > > > > > > > >> > > > > > > wrote:
> > >> > > > > > > > >> > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Hi Rajini
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > 3.2 - This makes
> sense.
> > >> Thanks
> > >> > for
> > >> > > > > > clarifying.
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Rest looks fine. Once
> > the
> > >> > > > > > implementations are
> > >> > > > > > > done,
> > >> > > > > > > > it
> > >> > > > > > > > >> will be
> > >> > > > > > > > >> > > more
> > >> > > > > > > > >> > > > > > > clear
> > >> > > > > > > > >> > > > > > > > > on the different
> values
> > >> > > RequestType
> > >> > > > > and
> > >> > > > > > Mode.
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Thanks
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Bosco
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > On 8/9/19, 5:08 AM,
> > "Rajini
> > >> > > > Sivaram"
> > >> > > > > <
> > >> > > > > > > > >> rajinisiva...@gmail.com
> > >> > > > > > > > >> > > >
> > >> > > > > > > > >> > > > > > wrote:
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Hi Don,
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > Thanks for the
> > >> suggestions.
> > >> > A
> > >> > > > few
> > >> > > > > > > responses
> > >> > > > > > > > below:
> > >> > > > > > > > >> > > > > > > > >
> > >> > > > > > > > >> > > > > > > > > 3.1 Can rename
> and
> > >> improve
> > >> > > docs
> > >> > > > if
> > >> > > > > > we keep
> > >> > > > > > > > this. Let's
> > >> > > > > > > > >> > > finish the
> > >> > > > > > > > >> > > > > > > > > discussion on
> > Colin's
> > >> > > > suggestions
> > >> > > > > > > regarding this
> > >> > > > > > > > >> first.
> > >> > > > > > > > >> > > > > > > > > 3.2 No, I was
> > thinking of
> > >> > some
> > >> > > > > > requests
> > >> > > > > > > that
> > >> > > > > > > > have an
> > >> > > > > > > > >> old
> > >> > > > > > > > >> > > way of
> > >> > > > > > > > >> > > > > > > > > authorizing
> > >> > > > > > > > >> > > > > > > > > and a new way
> > where we
> > >> have
> > >> > > > > > retained the
> > >> > > > > > > old
> > >> > > > > > > > way for
> > >> > > > > > > > >> > > backward
> > >> > > > > > > > >> > > > > > > > > compatibility.
> One
> > >> example
> > >> > is
> > >> > > > > > > Cluster:Create
> > >> > > > > > > > >> permission to
> > >> > > > > > > > >> > > create
> > >> > > > > > > > >> > > > > > > > > topics.
> > >> > > > > > > > >> > > > > > > > > We have replaced
> > this
> > >> with
> > >> > > > > > fine-grained
> > >> > > > > > > topic
> > >> > > > > > > > create
> > >> > > > > > > > >> > > access using
> > >> > > >
>
