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