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