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
> > > > > > > >> > > > > > > > > Topic:Create
> > > > > > > >> > > > > > > > > for topic patterns. But we
> > still
> > > > > check if
> > > > > > user
> > > > > > > has
> > > > > > > >> > > Cluster:Create
> > > > > > > >> > > > > > > > > first. If
> > > > > > > >> > > > > > > > > Denied, the deny is ignored
> > and
> > > we
> > > > > check
> > > > > > > >> Topic:Create. We
> > > > > > > >> > > dont
> > > > > > > >> > > > > > want
> > > > > > > >> > > > > > > > to
> > > > > > > >> > > > > > > > > log
> > > > > > > >> > > > > > > > > DENY for Cluster:Create at
> > INFO
> > > > > level for
> > > > > > this,
> > > > > > > since
> > > > > > > >> this
> > > > > > > >> > > is
> > > > > > > >> > > > > > not a
> > > > > > > >> > > > > > > > > mandatory ACL for creating
> > > topics.
> > > > > We
> > > > > > will get
> > > > > > > >> appropriate
> > > > > > > >> > > logs
> > > > > > > >> > > > > > > from
> > > > > > > >> > > > > > > > > the
> > > > > > > >> > > > > > > > > subsequent Topic:Create in
> > this
> > > > > case.
> > > > > > > >> > > > > > > > > 3.3 They are not quite the
> > same.
> > > > > FILTER
> > > > > > implies
> > > > > > > that
> > > > > > > >> user
> > > > > > > >> > > > > > actually
> > > > > > > >> > > > > > > > > requested access to and
> > > performed
> > > > > some
> > > > > > > operation on
> > > > > > > >> the
> > > > > > > >> > > filtered
> > > > > > > >> > > > > > > > > resources.
> > > > > > > >> > > > > > > > > LIST_AUTHORZED did not
> result
> > in
> > > > any
> > > > > > actual
> > > > > > > access.
> > > > > > > >> User
> > > > > > > >> > > simply
> > > > > > > >> > > > > > > > wanted
> > > > > > > >> > > > > > > > > to
> > > > > > > >> > > > > > > > > know what they are allowed
> to
> > > > > access.
> > > > > > > >> > > > > > > > > 3.4 Request types are
> Produce,
> > > > > JoinGroup,
> > > > > > > OffsetCommit
> > > > > > > >> > > etc. So
> > > > > > > >> > > > > > that
> > > > > > > >> > > > > > > > is
> > > > > > > >> > > > > > > > > different from authorization
> > > mode,
> > > > > > operation
> > > > > > > etc.
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > > On Thu, Aug 8, 2019 at 11:36
> > PM
> > > > Don
> > > > > Bosco
> > > > > > Durai
> > > > > > > <
> > > > > > > >> > > > > > bo...@apache.org>
> > > > > > > >> > > > > > > > > wrote:
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > > > Hi Rajini
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > Thanks for clarifying.
> This
> > is
> > > > > very
> > > > > > helpful...
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > #1 - On the Ranger side,
> we
> > > > > should be
> > > > > > able to
> > > > > > > handle
> > > > > > > >> > > multiple
> > > > > > > >> > > > > > > > > requests at
> > > > > > > >> > > > > > > > > > the same time. I was just
> > not
> > > > > sure how
> > > > > > much
> > > > > > > >> processing
> > > > > > > >> > > overhead
> > > > > > > >> > > > > > > > will
> > > > > > > >> > > > > > > > > be
> > > > > > > >> > > > > > > > > > there on the Broker side
> to
> > > > split
> > > > > and
> > > > > > then
> > > > > > > >> consolidate
> > > > > > > >> > > the
> > > > > > > >> > > > > > > results.
> > > > > > > >> > > > > > > > > If it
> > > > > > > >> > > > > > > > > > is negligible, then this
> is
> > > the
> > > > > better
> > > > > > way.
> > > > > > > It will
> > > > > > > >> > > make it
> > > > > > > >> > > > > > > future
> > > > > > > >> > > > > > > > > proof.
> > > > > > > >> > > > > > > > > > #2 - I agree, having a
> > single
> > > > > "start"
> > > > > > call
> > > > > > > makes it
> > > > > > > >> > > cleaner.
> > > > > > > >> > > > > > The
> > > > > > > >> > > > > > > > > > Authorization
> implementation
> > > > will
> > > > > only
> > > > > > have
> > > > > > > to do
> > > > > > > >> the
> > > > > > > >> > > > > > > > initialization
> > > > > > > >> > > > > > > > > only
> > > > > > > >> > > > > > > > > > once.
> > > > > > > >> > > > > > > > > > #3.1 - Thanks for the
> > > > > clarification. I
> > > > > > think
> > > > > > > it
> > > > > > > >> makes
> > > > > > > >> > > sense to
> > > > > > > >> > > > > > > have
> > > > > > > >> > > > > > > > > this.
> > > > > > > >> > > > > > > > > > The term "Mode" might not
> be
> > > > > explicit
> > > > > > enough.
> > > > > > > >> > > Essentially it
> > > > > > > >> > > > > > > seems
> > > > > > > >> > > > > > > > > you want
> > > > > > > >> > > > > > > > > > the Authorizer to know the
> > > > > > Intent/Purpose of
> > > > > > > the
> > > > > > > >> > > authorize call
> > > > > > > >> > > > > > > and
> > > > > > > >> > > > > > > > > let the
> > > > > > > >> > > > > > > > > > Authorizer decide what to
> > log
> > > as
> > > > > Audit
> > > > > > event.
> > > > > > > >> Changing
> > > > > > > >> > > the
> > > > > > > >> > > > > > > > > class/field name
> > > > > > > >> > > > > > > > > > or giving more
> documentation
> > > > will
> > > > > do.
> > > > > > > >> > > > > > > > > > #3.2 - Regarding the
> option
> > > > > "OPTIONAL".
> > > > > > Are
> > > > > > > you
> > > > > > > >> thinking
> > > > > > > >> > > from
> > > > > > > >> > > > > > > > > chaining
> > > > > > > >> > > > > > > > > > multiple Authorizer? If
> > so, I
> > > > am
> > > > > not
> > > > > > sure
> > > > > > > whether
> > > > > > > >> the
> > > > > > > >> > > Broker
> > > > > > > >> > > > > > > would
> > > > > > > >> > > > > > > > > have
> > > > > > > >> > > > > > > > > > enough information to make
> > > that
> > > > > > decision. I
> > > > > > > feel the
> > > > > > > >> > > Authorizer
> > > > > > > >> > > > > > > > will
> > > > > > > >> > > > > > > > > be the
> > > > > > > >> > > > > > > > > > one who would have that
> > > > > knowledge. E.g.
> > > > > > in
> > > > > > > Ranger
> > > > > > > >> we have
> > > > > > > >> > > > > > > explicit
> > > > > > > >> > > > > > > > > deny,
> > > > > > > >> > > > > > > > > > which means no matter
> what,
> > > the
> > > > > request
> > > > > > > should be
> > > > > > > >> denied
> > > > > > > >> > > for
> > > > > > > >> > > > > > the
> > > > > > > >> > > > > > > > > user/group
> > > > > > > >> > > > > > > > > > or condition. So if you
> are
> > > > > thinking of
> > > > > > > chaining
> > > > > > > >> > > Authorizers,
> > > > > > > >> > > > > > > then
> > > > > > > >> > > > > > > > > > responses should have the
> > > third
> > > > > state,
> > > > > > e.g.
> > > > > > > >> > > "DENIED_FINAL", in
> > > > > > > >> > > > > > > > which
> > > > > > > >> > > > > > > > > case
> > > > > > > >> > > > > > > > > > if there is an
> Authorization
> > > > > chain, it
> > > > > > will
> > > > > > > be stop
> > > > > > > >> and
> > > > > > > >> > > the
> > > > > > > >> > > > > > > request
> > > > > > > >> > > > > > > > > will be
> > > > > > > >> > > > > > > > > > denied and if it is just
> > > denied,
> > > > > then
> > > > > > you
> > > > > > > might fall
> > > > > > > >> > > back to
> > > > > > > >> > > > > > next
> > > > > > > >> > > > > > > > > > authorizer. If we don't
> have
> > > > > chaining of
> > > > > > > >> Authorizing in
> > > > > > > >> > > mind,
> > > > > > > >> > > > > > > then
> > > > > > > >> > > > > > > > we
> > > > > > > >> > > > > > > > > > should reconsider OPTIONAL
> > for
> > > > > now. Or
> > > > > > > clarify under
> > > > > > > >> > > which
> > > > > > > >> > > > > > > scenario
> > > > > > > >> > > > > > > > > > OPTIONAL will be called.
> > > > > > > >> > > > > > > > > > #3.3 Regarding, FILTER v/s
> > > > > > LIST_AUTHORIZED,
> > > > > > > isn't
> > > > > > > >> > > > > > > LIST_AUTHORIZED a
> > > > > > > >> > > > > > > > > > special case for "FILTER"?
> > > > > > > >> > > > > > > > > > #3.4 KafkaRequestContext.
> > > > > requestType()
> > > > > > v/s
> > > > > > > Action.
> > > > > > > >> > > > > > > > > authorizationMode. I
> > > > > > > >> > > > > > > > > > am not sure about the
> > overlap
> > > or
> > > > > > ambiguity.
> > > > > > > >> > > > > > > > > > #4 - Cool. This is good,
> it
> > > will
> > > > > be less
> > > > > > > stress on
> > > > > > > >> the
> > > > > > > >> > > > > > > Authorizer.
> > > > > > > >> > > > > > > > > Ranger
> > > > > > > >> > > > > > > > > > already supports the
> "count"
> > > > > concept
> > > > > > and also
> > > > > > > has
> > > > > > > >> > > batching
> > > > > > > >> > > > > > > > > capability to
> > > > > > > >> > > > > > > > > > aggregate similar requests
> > to
> > > > > reduce the
> > > > > > > number of
> > > > > > > >> audit
> > > > > > > >> > > logs
> > > > > > > >> > > > > > to
> > > > > > > >> > > > > > > > > write. We
> > > > > > > >> > > > > > > > > > should be able to pass
> this
> > > > > through.
> > > > > > > >> > > > > > > > > > #5 - Assuming if the
> object
> > > > > instance is
> > > > > > going
> > > > > > > out of
> > > > > > > >> > > scope, we
> > > > > > > >> > > > > > > > > should be
> > > > > > > >> > > > > > > > > > fine. Not a super
> important
> > > ask.
> > > > > Ranger
> > > > > > is
> > > > > > > already
> > > > > > > >> > > catching
> > > > > > > >> > > > > > KILL
> > > > > > > >> > > > > > > > > signal for
> > > > > > > >> > > > > > > > > > clean up.
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > Thanks again. These are
> good
> > > > > > enhancements.
> > > > > > > >> Appreciate
> > > > > > > >> > > your
> > > > > > > >> > > > > > > efforts
> > > > > > > >> > > > > > > > > here.
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > Bosco
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > On 8/8/19, 2:03 AM,
> "Rajini
> > > > > Sivaram" <
> > > > > > > >> > > rajinisiva...@gmail.com
> > > > > > > >> > > > > > >
> > > > > > > >> > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > Hi Don,
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > Thanks for reviewing
> the
> > > > KIP.
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > 1. I had this
> originally
> > > as
> > > > a
> > > > > single
> > > > > > > Action, but
> > > > > > > >> > > thought it
> > > > > > > >> > > > > > > may
> > > > > > > >> > > > > > > > > be
> > > > > > > >> > > > > > > > > > useful
> > > > > > > >> > > > > > > > > > to support batched
> > > authorize
> > > > > calls
> > > > > > as
> > > > > > > well and
> > > > > > > >> keep
> > > > > > > >> > > it
> > > > > > > >> > > > > > > > > consistent with
> > > > > > > >> > > > > > > > > > other methods. Single
> > > > > requests can
> > > > > > contain
> > > > > > > >> multiple
> > > > > > > >> > > topics.
> > > > > > > >> > > > > > > For
> > > > > > > >> > > > > > > > > > example a
> > > > > > > >> > > > > > > > > > produce request can
> > > contain
> > > > > records
> > > > > > for
> > > > > > > several
> > > > > > > >> > > partitions
> > > > > > > >> > > > > > of
> > > > > > > >> > > > > > > > > different
> > > > > > > >> > > > > > > > > > topics. Broker could
> > > > > potentially
> > > > > > > authorize these
> > > > > > > >> > > together.
> > > > > > > >> > > > > > > For
> > > > > > > >> > > > > > > > > > SimpleAclAuthorizer,
> > > batched
> > > > > > authorize
> > > > > > > methods
> > > > > > > >> don't
> > > > > > > >> > > > > > provide
> > > > > > > >> > > > > > > > any
> > > > > > > >> > > > > > > > > > optimisation since
> > lookup
> > > is
> > > > > based
> > > > > > on
> > > > > > > resources
> > > > > > > >> > > followed by
> > > > > > > >> > > > > > > the
> > > > > > > >> > > > > > > > > > matching
> > > > > > > >> > > > > > > > > > logic. But some
> > > authorizers
> > > > > may
> > > > > > manage
> > > > > > > ACLs by
> > > > > > > >> user
> > > > > > > >> > > > > > principal
> > > > > > > >> > > > > > > > > rather
> > > > > > > >> > > > > > > > > > than
> > > > > > > >> > > > > > > > > > resource and may be
> able
> > > to
> > > > > optimize
> > > > > > > batched
> > > > > > > >> > > requests. I am
> > > > > > > >> > > > > > > ok
> > > > > > > >> > > > > > > > > with
> > > > > > > >> > > > > > > > > > using
> > > > > > > >> > > > > > > > > > single Action if this
> is
> > > > > likely to
> > > > > > cause
> > > > > > > issues.
> > > > > > > >> > > > > > > > > > 2. If you have two
> > > > listeners,
> > > > > one
> > > > > > for
> > > > > > > >> inter-broker
> > > > > > > >> > > traffic
> > > > > > > >> > > > > > > and
> > > > > > > >> > > > > > > > > another
> > > > > > > >> > > > > > > > > > for
> > > > > > > >> > > > > > > > > > external clients,
> start
> > > > > method is
> > > > > > invoked
> > > > > > > twice,
> > > > > > > >> > > once for
> > > > > > > >> > > > > > > each
> > > > > > > >> > > > > > > > > > listener. On
> > > > > > > >> > > > > > > > > > second thought, that
> may
> > > be
> > > > > > confusing and
> > > > > > > a
> > > > > > > >> single
> > > > > > > >> > > start()
> > > > > > > >> > > > > > > > > invocation
> > > > > > > >> > > > > > > > > > that
> > > > > > > >> > > > > > > > > > provides all listener
> > > > > information
> > > > > > and
> > > > > > > returns
> > > > > > > >> > > multiple
> > > > > > > >> > > > > > > futures
> > > > > > > >> > > > > > > > > would be
> > > > > > > >> > > > > > > > > > better. Will update
> the
> > > KIP.
> > > > > > > >> > > > > > > > > > 3. A typical example
> is
> > a
> > > > > consumer
> > > > > > > subscribing
> > > > > > > >> to a
> > > > > > > >> > > regex
> > > > > > > >> > > > > > > > > pattern. We
> > > > > > > >> > > > > > > > > > request all topic
> > metadata
> > > > > from the
> > > > > > > broker in
> > > > > > > >> order
> > > > > > > >> > > to
> > > > > > > >> > > > > > decide
> > > > > > > >> > > > > > > > > whether
> > > > > > > >> > > > > > > > > > the
> > > > > > > >> > > > > > > > > > pattern matches,
> > expecting
> > > > to
> > > > > > receive a
> > > > > > > list of
> > > > > > > >> > > authorised
> > > > > > > >> > > > > > > > > topics. The
> > > > > > > >> > > > > > > > > > user
> > > > > > > >> > > > > > > > > > is not asking to
> > subscribe
> > > > to
> > > > > an
> > > > > > > unauthorized
> > > > > > > >> topic.
> > > > > > > >> > > If
> > > > > > > >> > > > > > there
> > > > > > > >> > > > > > > > > are 10000
> > > > > > > >> > > > > > > > > > topics in the cluster
> > and
> > > > the
> > > > > user
> > > > > > has
> > > > > > > access
> > > > > > > >> to 100
> > > > > > > >> > > of
> > > > > > > >> > > > > > them,
> > > > > > > >> > > > > > > > at
> > > > > > > >> > > > > > > > > the
> > > > > > > >> > > > > > > > > > moment
> > > > > > > >> > > > > > > > > > we log 9900 DENIED log
> > > > > entries at
> > > > > > INFO
> > > > > > > level in
> > > > > > > >> > > > > > > > > SimpleAclAuthorizer.
> > > > > > > >> > > > > > > > > > The
> > > > > > > >> > > > > > > > > > proposal is to
> authorize
> > > > this
> > > > > > request with
> > > > > > > >> > > > > > > > > AuthorizationMode.FILTER, so
> > > > > > > >> > > > > > > > > > that authorizers can
> log
> > > > > resources
> > > > > > that
> > > > > > > are
> > > > > > > >> filtered
> > > > > > > >> > > out at
> > > > > > > >> > > > > > > > > lower level
> > > > > > > >> > > > > > > > > > like DEBUG since this
> is
> > > not
> > > > > an
> > > > > > attempt to
> > > > > > > >> access
> > > > > > > >> > > > > > > unauthorized
> > > > > > > >> > > > > > > > > > resources.
> > > > > > > >> > > > > > > > > > Brokers already handle
> > > these
> > > > > > differently
> > > > > > > since
> > > > > > > >> no
> > > > > > > >> > > > > > > authorization
> > > > > > > >> > > > > > > > > error
> > > > > > > >> > > > > > > > > > is
> > > > > > > >> > > > > > > > > > returned to the client
> > in
> > > > > these
> > > > > > cases.
> > > > > > > Providing
> > > > > > > >> > > > > > > authorization
> > > > > > > >> > > > > > > > > mode to
> > > > > > > >> > > > > > > > > > authorizers enables
> > > > authorizer
> > > > > > > implementations
> > > > > > > >> to
> > > > > > > >> > > generate
> > > > > > > >> > > > > > > > > better audit
> > > > > > > >> > > > > > > > > > logs.
> > > > > > > >> > > > > > > > > > 4. Each request may
> > > contain
> > > > > multiple
> > > > > > > instances
> > > > > > > >> of
> > > > > > > >> > > the same
> > > > > > > >> > > > > > > > > authorizable
> > > > > > > >> > > > > > > > > > resource. For example
> a
> > > > > produce
> > > > > > request
> > > > > > > may
> > > > > > > >> contain
> > > > > > > >> > > records
> > > > > > > >> > > > > > > for
> > > > > > > >> > > > > > > > > 10
> > > > > > > >> > > > > > > > > > partitions of the same
> > > > topic.
> > > > > At the
> > > > > > > moment, we
> > > > > > > >> > > invoke
> > > > > > > >> > > > > > > > authorize
> > > > > > > >> > > > > > > > > > method 10
> > > > > > > >> > > > > > > > > > times. The proposal is
> > to
> > > > > invoke it
> > > > > > once
> > > > > > > with
> > > > > > > >> > > count=10. The
> > > > > > > >> > > > > > > > > count is
> > > > > > > >> > > > > > > > > > provided to authorizer
> > > just
> > > > > for
> > > > > > audit
> > > > > > > logging
> > > > > > > >> > > purposes.
> > > > > > > >> > > > > > > > > > 5. Authorizer
> implements
> > > > > Closeable,
> > > > > > so you
> > > > > > > >> could use
> > > > > > > >> > > > > > close()
> > > > > > > >> > > > > > > to
> > > > > > > >> > > > > > > > > flush
> > > > > > > >> > > > > > > > > > audits?
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > On Thu, Aug 8, 2019 at
> > > 7:01
> > > > > AM Don
> > > > > > Bosco
> > > > > > > Durai <
> > > > > > > >> > > > > > > > bo...@apache.org
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Rajini
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Thanks for putting
> > this
> > > > > together.
> > > > > > It is
> > > > > > > >> looking
> > > > > > > >> > > good. I
> > > > > > > >> > > > > > > have
> > > > > > > >> > > > > > > > > few
> > > > > > > >> > > > > > > > > > > questions...
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > 1.
> > > > List<AuthorizationResult>
> > > > > > > authorize(...,
> > > > > > > >> > > List<Action>
> > > > > > > >> > > > > > > > > actions).
> > > > > > > >> > > > > > > > > > Do you
> > > > > > > >> > > > > > > > > > > see a scenario where
> > the
> > > > > broker
> > > > > > will
> > > > > > > call
> > > > > > > >> > > authorize for
> > > > > > > >> > > > > > > > > multiple
> > > > > > > >> > > > > > > > > > topics at
> > > > > > > >> > > > > > > > > > > the same time? I can
> > > > > understand
> > > > > > that
> > > > > > > during
> > > > > > > >> > > > > > > creating/deleting
> > > > > > > >> > > > > > > > > ACLS,
> > > > > > > >> > > > > > > > > > > multiple permissions
> > for
> > > > > multiple
> > > > > > > resources
> > > > > > > >> might
> > > > > > > >> > > be
> > > > > > > >> > > > > > done.
> > > > > > > >> > > > > > > > For
> > > > > > > >> > > > > > > > > > authorize
> > > > > > > >> > > > > > > > > > > call, would this be
> a
> > > > case?
> > > > > And
> > > > > > does the
> > > > > > > >> Authorize
> > > > > > > >> > > > > > > > > implementation
> > > > > > > >> > > > > > > > > > will be
> > > > > > > >> > > > > > > > > > > able to do
> performance
> > > > > > optimization
> > > > > > > because of
> > > > > > > >> > > this? Or
> > > > > > > >> > > > > > > > should
> > > > > > > >> > > > > > > > > we
> > > > > > > >> > > > > > > > > > just keep
> > > > > > > >> > > > > > > > > > > it simple? I don't
> see
> > > it
> > > > > as an
> > > > > > issue
> > > > > > > from
> > > > > > > >> Apache
> > > > > > > >> > > Ranger
> > > > > > > >> > > > > > > > side,
> > > > > > > >> > > > > > > > > but
> > > > > > > >> > > > > > > > > > just
> > > > > > > >> > > > > > > > > > > checking to see
> > whether
> > > we
> > > > > need
> > > > > > to be
> > > > > > > aware of
> > > > > > > >> > > something.
> > > > > > > >> > > > > > > > > > > 2. Should I assume
> > that
> > > > the
> > > > > > > SecurityProtocol
> > > > > > > >> passed
> > > > > > > >> > > > > > during
> > > > > > > >> > > > > > > > > start and
> > > > > > > >> > > > > > > > > > the
> > > > > > > >> > > > > > > > > > > one return by
> > > > > > > >> > > KafkaRequestContext.securityProtocol() will
> > > > > > > >> > > > > > > be
> > > > > > > >> > > > > > > > > the
> > > > > > > >> > > > > > > > > > same?
> > > > > > > >> > > > > > > > > > >
> > CompletableFuture<Void>
> > > > > > start(String
> > > > > > > >> listenerName,
> > > > > > > >> > > > > > > > > SecurityProtocol
> > > > > > > >> > > > > > > > > > > securityProtocol);
> > > > > > > >> > > > > > > > > > >
> > > > > > KafkaRequestContext.securityProtocol()
> > > > > > > >> > > > > > > > > > > 3. What is the
> purpose
> > > of
> > > > > > > AuthorizationMode?
> > > > > > > >> How
> > > > > > > >> > > does the
> > > > > > > >> > > > > > > > > broker
> > > > > > > >> > > > > > > > > > decide
> > > > > > > >> > > > > > > > > > > what mode to use
> when
> > > the
> > > > > > authorize()
> > > > > > > method
> > > > > > > >> is
> > > > > > > >> > > called?
> > > > > > > >> > > > > > > > > > > 4. Can we clarify
> > > "count"
> > > > in
> > > > > > Action a
> > > > > > > bit
> > > > > > > >> more?
> > > > > > > >> > > How is it
> > > > > > > >> > > > > > > > used?
> > > > > > > >> > > > > > > > > > > 5. Do you feel
> having
> > > > "stop"
> > > > > > along with
> > > > > > > >> "start" be
> > > > > > > >> > > > > > helpful?
> > > > > > > >> > > > > > > > > E.g. In
> > > > > > > >> > > > > > > > > > Ranger
> > > > > > > >> > > > > > > > > > > we try to optimize
> the
> > > > Audit
> > > > > > writing by
> > > > > > > >> caching
> > > > > > > >> > > the logs
> > > > > > > >> > > > > > > for
> > > > > > > >> > > > > > > > a
> > > > > > > >> > > > > > > > > fixed
> > > > > > > >> > > > > > > > > > > interval. But when
> the
> > > > > Broker
> > > > > > > terminates, we
> > > > > > > >> do a
> > > > > > > >> > > forced
> > > > > > > >> > > > > > > > flush.
> > > > > > > >> > > > > > > > > > Having an
> > > > > > > >> > > > > > > > > > > explicit "stop"
> might
> > > give
> > > > > us a
> > > > > > formal
> > > > > > > way to
> > > > > > > >> > > flush our
> > > > > > > >> > > > > > > > audits.
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Thanks
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Bosco
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > On 8/7/19, 3:59 PM,
> > > > "Rajini
> > > > > > Sivaram" <
> > > > > > > >> > > > > > > > rajinisiva...@gmail.com
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Hi
> > > Ron/Harsha/Satish,
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Thanks for
> > reviewing
> > > > > the KIP!
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > We should
> perhaps
> > > have
> > > > > a wider
> > > > > > > discussion
> > > > > > > >> > > outside
> > > > > > > >> > > > > > this
> > > > > > > >> > > > > > > > KIP
> > > > > > > >> > > > > > > > > for
> > > > > > > >> > > > > > > > > > > refactoring
> > > > > > > >> > > > > > > > > > > clients so that
> > > others
> > > > > who
> > > > > > are not
> > > > > > > >> following
> > > > > > > >> > > this KIP
> > > > > > > >> > > > > > > > also
> > > > > > > >> > > > > > > > > > notice the
> > > > > > > >> > > > > > > > > > > discussion.
> > Satish,
> > > > > would you
> > > > > > like
> > > > > > > to
> > > > > > > >> start a
> > > > > > > >> > > > > > > discussion
> > > > > > > >> > > > > > > > > thread
> > > > > > > >> > > > > > > > > > on dev?
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Regards,
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > Rajini
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > On Wed, Aug 7,
> > 2019
> > > at
> > > > > 6:21 PM
> > > > > > > Satish
> > > > > > > >> Duggana <
> > > > > > > >> > > > > > > > > > >
> > > satish.dugg...@gmail.com>
> > > > > > > >> > > > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > I felt the
> same
> > > need
> > > > > when
> > > > > > we want
> > > > > > > to
> > > > > > > >> add a
> > > > > > > >> > > > > > pluggable
> > > > > > > >> > > > > > > > API
> > > > > > > >> > > > > > > > > for
> > > > > > > >> > > > > > > > > > core
> > > > > > > >> > > > > > > > > > > > server
> > > > functionality.
> > > > > This
> > > > > > does
> > > > > > > not
> > > > > > > >> need to
> > > > > > > >> > > be part
> > > > > > > >> > > > > > > of
> > > > > > > >> > > > > > > > > this
> > > > > > > >> > > > > > > > > > KIP, it
> > > > > > > >> > > > > > > > > > > > can be a
> > separate
> > > > > KIP. I can
> > > > > > > contribute
> > > > > > > >> those
> > > > > > > >> > > > > > > > refactoring
> > > > > > > >> > > > > > > > > > changes if
> > > > > > > >> > > > > > > > > > > > others are OK
> > with
> > > > > that.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > It is better
> to
> > > > have a
> > > > > > structure
> > > > > > > like
> > > > > > > >> below.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > kafka-common:
> > > > > > > >> > > > > > > > > > > > common classes
> > > which
> > > > > can be
> > > > > > used
> > > > > > > in any
> > > > > > > >> of
> > > > > > > >> > > the
> > > > > > > >> > > > > > other
> > > > > > > >> > > > > > > > > modules
> > > > > > > >> > > > > > > > > > in Kafka
> > > > > > > >> > > > > > > > > > > > like client,
> > > > > > Kafka-server-common
> > > > > > > and
> > > > > > > >> server
> > > > > > > >> > > etc.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > >
> > > kafka-client-common:
> > > > > > > >> > > > > > > > > > > > common classes
> > > which
> > > > > can be
> > > > > > used
> > > > > > > in the
> > > > > > > >> > > client
> > > > > > > >> > > > > > > module.
> > > > > > > >> > > > > > > > > This
> > > > > > > >> > > > > > > > > > can be
> > > > > > > >> > > > > > > > > > > > part of client
> > > > module
> > > > > > itself.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > >
> > > kafka-server-common:
> > > > > > > >> > > > > > > > > > > > classes
> required
> > > > only
> > > > > for
> > > > > > > kafka-server.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > Thanks.
> > > > > > > >> > > > > > > > > > > > Satish.
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > On Wed, Aug 7,
> > > 2019
> > > > > at 9:28
> > > > > > PM
> > > > > > > Harsha
> > > > > > > >> > > Chintalapani
> > > > > > > >> > > > > > <
> > > > > > > >> > > > > > > > > > ka...@harsha.io>
> > > > > > > >> > > > > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > Thanks for
> the
> > > KIP
> > > > > Rajini.
> > > > > > > >> > > > > > > > > > > > > Quick
> thought,
> > > it
> > > > > would
> > > > > > be good
> > > > > > > to
> > > > > > > >> have a
> > > > > > > >> > > common
> > > > > > > >> > > > > > > > module
> > > > > > > >> > > > > > > > > > outside of
> > > > > > > >> > > > > > > > > > > > clients
> > > > > > > >> > > > > > > > > > > > > that only
> > > applies
> > > > to
> > > > > > server side
> > > > > > > >> > > interfaces &
> > > > > > > >> > > > > > > > changes.
> > > > > > > >> > > > > > > > > It
> > > > > > > >> > > > > > > > > > looks
> > > > > > > >> > > > > > > > > > > like we
> > > > > > > >> > > > > > > > > > > > are
> > > > > > > >> > > > > > > > > > > > > increasingly
> > in
> > > > > favor of
> > > > > > using
> > > > > > > Java
> > > > > > > >> > > interface for
> > > > > > > >> > > > > > > > > pluggable
> > > > > > > >> > > > > > > > > > > modules on
> > > > > > > >> > > > > > > > > > > > the
> > > > > > > >> > > > > > > > > > > > > broker side.
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > Thanks,
> > > > > > > >> > > > > > > > > > > > > Harsha
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > On Tue, Aug
> > 06,
> > > > > 2019 at
> > > > > > 2:31 PM,
> > > > > > > >> Rajini
> > > > > > > >> > > Sivaram <
> > > > > > > >> > > > > > > > > > >
> > rajinisiva...@gmail.com
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > wrote:
> > > > > > > >> > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > Hi all,
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > I have
> > > created a
> > > > > KIP to
> > > > > > > replace the
> > > > > > > >> Scala
> > > > > > > >> > > > > > > > Authorizer
> > > > > > > >> > > > > > > > > API
> > > > > > > >> > > > > > > > > > with a
> > > > > > > >> > > > > > > > > > > new
> > > > > > > >> > > > > > > > > > > > Java
> > > > > > > >> > > > > > > > > > > > > > API:
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > -
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > KIP-504+-+Add+new+Java+Authorizer+Interface
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > This is
> > > > > replacement for
> > > > > > KIP-50
> > > > > > > >> which was
> > > > > > > >> > > > > > accepted
> > > > > > > >> > > > > > > > > but never
> > > > > > > >> > > > > > > > > > > merged.
> > > > > > > >> > > > > > > > > > > > Apart
> > > > > > > >> > > > > > > > > > > > > > from
> moving
> > > to a
> > > > > Java
> > > > > > API
> > > > > > > consistent
> > > > > > > >> > > with other
> > > > > > > >> > > > > > > > > pluggable
> > > > > > > >> > > > > > > > > > > interfaces
> > > > > > > >> > > > > > > > > > > > in the
> > > > > > > >> > > > > > > > > > > > > > broker,
> > > KIP-504
> > > > > also
> > > > > > attempts
> > > > > > > to
> > > > > > > >> address
> > > > > > > >> > > known
> > > > > > > >> > > > > > > > > limitations
> > > > > > > >> > > > > > > > > > in the
> > > > > > > >> > > > > > > > > > > > > >
> authorizer.
> > If
> > > > > you have
> > > > > > come
> > > > > > > across
> > > > > > > >> other
> > > > > > > >> > > > > > > > > limitations that
> > > > > > > >> > > > > > > > > > you
> > > > > > > >> > > > > > > > > > > would
> > > > > > > >> > > > > > > > > > > > like
> > > > > > > >> > > > > > > > > > > > > > to see
> > > addressed
> > > > > in the
> > > > > > new
> > > > > > > API,
> > > > > > > >> please
> > > > > > > >> > > raise
> > > > > > > >> > > > > > > these
> > > > > > > >> > > > > > > > > on the
> > > > > > > >> > > > > > > > > > > discussion
> > > > > > > >> > > > > > > > > > > > > > thread so
> > that
> > > > we
> > > > > can
> > > > > > > consider those
> > > > > > > >> > > too. All
> > > > > > > >> > > > > > > > > suggestions
> > > > > > > >> > > > > > > > > > and
> > > > > > > >> > > > > > > > > > > feedback
> > > > > > > >> > > > > > > > > > > > are
> > > > > > > >> > > > > > > > > > > > > > welcome.
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > Thank you,
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > > > > Rajini
> > > > > > > >> > > > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > > >
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > > >
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > > >
> > > > > > > >> > > > > > > >
> > > > > > > >> > > > > > >
> > > > > > > >> > > > > >
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
