Hi Jun,
Thanks for your note. Yes, agree that sync authorize() will be sufficient
in most cases based on the numbers we expect to see in the foreseeable
future.
For async CreateAcls/DeleteAcls, we will need to add a callback in
KafkaApis and add to purgatory if future is not complete. Since we don't
expect large numbers of these requests, we should be able to use the
existing purgatory implementation. Since we support pluggable ACL storage
(e.g. a database), this may be worth doing. I do agree that we don't expect
a large amount of ACL updates, but we could still have multiple concurrent
requests updating ACLs when topics are created or deleted. If the same user
issues large numbers of ACL update requests, we could use throttling to
delay these, enabling other requests to progress, but this still blocks the
request thread during the actual ACL updates.
Since the underlying API to update ACLs in the backend may be async anyway
(e.g. when ACLs are stored in a Kafka topic), an async API to avoid request
threads being blocked seems useful even though it adds some additional
complexity to the Kafka code. Since we don't currently support pluggable
backends for CreateTopic/AlterTopic or AlterConfgs, perhaps we don't need
to process these asynchronously.
Regards,
Rajini
On Mon, Sep 9, 2019 at 5:52 AM Jun Rao <j...@confluent.io> wrote:
> Hi, Rajini,
>
> Thanks for the reply. The 4-step approach that you outlined seems to work.
> Overall, I can see that the async authorize() api could lead to an overall
> more efficient implementation. The tradeoff is that we have to code every
> request with an extra stage. To me, this optimization seems too early. With
> 100K topics, each with 1KB worth of users, the required ACL space is about
> 100MB and we should be able to cache everything. Beyond that, we still have
> the option of dividing up the topic and ACL metadata such that every broker
> is only required to cache a subset of all metadata.
>
> Regarding making create/delete api async, I still have mixed feelings on
> this. I am wondering if the benefit is worth the added complexity. To me,
> both operations are rare. If one request thread is blocked occasionally for
> a few millisecs, it's probably ok since it mostly just affects one client.
> In the case that a particular user issues many those operations in a short
> window. Could we just use CPU throttling to prevent too much resource being
> used? There are a few other similar types of requests such as create/alter
> configs, create/alter topic, etc. Do we plan to add an extra processing
> stage for each of them too in the future?
>
> Thanks,
>
> Jun
>
> On Sun, Sep 8, 2019 at 10:04 AM Rajini Sivaram <rajinisiva...@gmail.com>
> wrote:
>
> > Thanks Ismael. That is good point. I have updated the javadoc.
> >
> > On Sun, Sep 8, 2019 at 5:38 PM Ismael Juma <ism...@juma.me.uk> wrote:
> >
> > > 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.
> > > > > > >> > > > > > > > >> > > > > > >
> > > > > > >> >
> > >
> >
>
