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