Hi Don,
Thanks for your note.
1) The intention is to avoid blocking in the calling thread. We already
have several requests that are put into a purgatory when waiting for remote
communication, for example produce request waiting for replication. Since
we have a limited number of request threads in the broker, we want to make
progress with other requests, while one is waiting on any form of remote
communication.
2) Async management calls will be useful with the default authorizer when
KIP-500 removes ZK and we rely on Kafka instead. Our current ZK-based
implementation as well as any custom implementations that don't want to be
async will just need to return a sync'ed value. So instead of returning `
value`, the code would just return `CompletableFuture.completedFuture(value)
`. So it would be just a single line change in the implementation with the
new API. The caller would treat completedFuture exactly as it does today,
processing the request synchronously without using a purgatory.
3) For implementations that return a completedFuture as described in 2),
the behaviour would remain exactly the same. No additional threads or
purgatory will be used for this case. So there would be no performance
penalty. For implementations that return a future that is not complete, we
prioritise running more requests concurrently. So in a deployment with a
large number of clients, we would improve performance by allowing other
requests to be processed on the request threads while some are waiting for
authorization metadata.
4) I was concerned about this too. The goal is to make the API flexible
enough to handle large scale deployments in future when caching all
authorization metadata in each broker is not viable. Using an async API
that returns CompletionStage, the caller has the option to handle the
result synchronously or asynchronously, so we don't necessarily need to
update the calling code right away. Custom authorizers using the async API
have full control over whether authorization is performed in-line since
completedFuture will always be handled synchronously. We do need to update
KafkaApis to take advantage of the asynchronous API to improve scale. Even
though this is a big change, since we will be doing the same for all
requests, it shouldn't be too hard to maintain since the same pattern will
be used for all requests.
Regards,
Rajini
On Tue, Sep 3, 2019 at 11:48 PM Don Bosco Durai <bo...@apache.org> wrote:
> Hi Rajini
>
> Help me understand this a bit more.
>
> 1. For all practical purpose, without authorization you can't go to the
> next step. The calling code needs to block anyway. So should we just let
> the implementation code do the async part?
> 2. If you feel management calls need to be async, then we should consider
> another set of async APIs. I don't feel we should complicate it further (
> 3. Another concern I have is wrt performance. Kafka has been built to
> handle 1000s per second requests. Not sure whether making it async will add
> any unnecessary overhead.
> 4. How much complication would this add on the calling side? And is it
> worth it?
>
> Thanks
>
> Bosco
>
>
> On 9/3/19, 8:50 AM, "Rajini Sivaram" <rajinisiva...@gmail.com> wrote:
>
> Hi all,
>
> Ismael brought up a point that it will be good to make the Authorizer
> interface asynchronous to avoid blocking request threads during remote
> operations.
>
> 1) Since we want to support different backends for authorization
> metadata,
> making createAcls() and deleteAcls() asynchronous makes sense since
> these
> always involve remote operations. When KIP-500 removes ZooKeeper, we
> would
> want to move ACLs to Kafka and async updates will avoid unnecessary
> blocking.
> 2) For authorize() method, we currently use cached ACLs in the built-in
> authorizers, so synchronous authorise operations work well now. But
> async
> authorize() would support this scenario as well as authorizers in large
> organisations where an LRU cache would enable a smaller cache even
> when the
> backend holds a large amount of ACLs for infrequently used resources or
> users who don't use the system frequently.
>
> For both cases, the built-in authorizer will continue to be
> synchronous,
> using CompletableFuture.completedFuture() to return the actual result.
> But
> async API will make custom authorizer implementations more flexible. I
> would like to know if there are any concerns with these changes before
> updating the KIP.
>
> *Proposed API:*
> public interface Authorizer extends Configurable, Closeable {
>
> Map<Endpoint, CompletionStage<Void>> start(AuthorizerServerInfo
> serverInfo);
> List<CompletionStage<AuthorizationResult>>
> authorize(AuthorizableRequestContext requestContext, List<Action>
> actions);
> List<CompletionStage<AclCreateResult>>
> createAcls(AuthorizableRequestContext requestContext, List<AclBinding>
> aclBindings);
> List<CompletionStage<AclDeleteResult>>
> deleteAcls(AuthorizableRequestContext requestContext,
> List<AclBindingFilter> aclBindingFilters);
> CompletionStage<Collection<AclBinding>> acls(AclBindingFilter
> filter);
> }
>
>
> Thank you,
>
> Rajini
>
> On Sun, Aug 18, 2019 at 6:25 PM Don Bosco Durai <bo...@apache.org>
> wrote:
>
> > Hi Rajini
> >
> > Thanks for clarifying. I am good for now.
> >
> > Regards
> >
> > Bosco
> >
> >
> > On 8/16/19, 11:30 AM, "Rajini Sivaram" <rajinisiva...@gmail.com>
> wrote:
> >
> > Hi Don,
> >
> > That should be fine. I guess Ranger loads policies from the
> database
> > synchronously when the authorizer is configured, similar to
> > SimpleAclAuthorizer loading from ZooKeeper. Ranger can continue
> to load
> > synchronously from `configure()` or `start()` and return an
> empty map
> > from
> > `start()`. That would retain the existing behaviour.. When the
> same
> > database stores policies for all listeners and the policies are
> not
> > stored
> > in Kafka, there is no value in making the load asynchronous.
> >
> > Regards,
> >
> > Rajini
> >
> >
> > On Fri, Aug 16, 2019 at 6:43 PM Don Bosco Durai <
> bo...@apache.org>
> > wrote:
> >
> > > Hi Rajini
> > >
> > > Assuming this doesn't affect custom plugins, I don't have any
> > concerns
> > > regarding this.
> > >
> > > I do have one question regarding:
> > >
> > > "For authorizers that don’t store metadata in ZooKeeper,
> ensure that
> > > authorizer metadata for each listener is available before
> starting
> > up the
> > > listener. This enables different authorization metadata stores
> for
> > > different listeners."
> > >
> > > Since Ranger uses its own database for storing policies, do you
> > anticipate
> > > any issues?
> > >
> > > Thanks
> > >
> > > Bosco
> > >
> > >
> > > On 8/16/19, 6:49 AM, "Rajini Sivaram" <
> rajinisiva...@gmail.com>
> > wrote:
> > >
> > > Hi all,
> > >
> > > I made another change to the KIP. The KIP was originally
> > proposing to
> > > extend SimpleAclAuthorizer to also implement the new API
> (in
> > addition
> > > to
> > > the existing API). But since we use the new API when
> available,
> > this
> > > can
> > > break custom authorizers that extend this class and
> override
> > specific
> > > methods of the old API. To avoid breaking any existing
> custom
> > > implementations that extend this class, particularly
> because it
> > is in
> > > the
> > > public package kafka.security.auth, the KIP now proposes to
> > retain the
> > > old
> > > authorizer as-is. SimpleAclAuthorizer will continue to
> > implement the
> > > old
> > > API, but will be deprecated. A new authorizer
> implementation
> > > kafka.security.authorizer.AclAuthorizer will be added for
> the
> > new API
> > > (this
> > > will not be in the public package).
> > >
> > > Please let me know if you have any concerns.
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > >
> > > On Fri, Aug 16, 2019 at 8:48 AM Rajini Sivaram <
> > > rajinisiva...@gmail.com>
> > > wrote:
> > >
> > > > Thanks Colin.
> > > >
> > > > If there are no other concerns, I will start vote later
> today.
> > Many
> > > thanks
> > > > to every one for the feedback.
> > > >
> > > > Regards,
> > > >
> > > > Rajini
> > > >
> > > >
> > > > On Thu, Aug 15, 2019 at 11:57 PM Colin McCabe <
> > cmcc...@apache.org>
> > > wrote:
> > > >
> > > >> Thanks, Rajini. It looks good to me.
> > > >>
> > > >> best,
> > > >> Colin
> > > >>
> > > >>
> > > >> On Thu, Aug 15, 2019, at 11:37, Rajini Sivaram wrote:
> > > >> > Hi Colin,
> > > >> >
> > > >> > Thanks for the review. I have updated the KIP to move
> the
> > > interfaces for
> > > >> > request context and server info to the authorizer
> package.
> > These
> > > are now
> > > >> > called AuthorizableRequestContext and
> AuthorizerServerInfo.
> > > Endpoint is
> > > >> now
> > > >> > a class in org.apache.kafka.common to make it reusable
> > since we
> > > already
> > > >> > have multiple implementations of it. I have removed
> > requestName
> > > from the
> > > >> > request context interface since authorizers can
> distinguish
> > > follower
> > > >> fetch
> > > >> > and consumer fetch from the operation being
> authorized. So
> > 16-bit
> > > >> request
> > > >> > type should be sufficient for audit logging. Also
> replaced
> > > AuditFlag
> > > >> with
> > > >> > two booleans as you suggested.
> > > >> >
> > > >> > Can you take another look and see if the KIP is ready
> for
> > voting?
> > > >> >
> > > >> > Thanks for all your help!
> > > >> >
> > > >> > Regards,
> > > >> >
> > > >> > Rajini
> > > >> >
> > > >> > On Wed, Aug 14, 2019 at 8:59 PM Colin McCabe <
> > cmcc...@apache.org>
> > > >> wrote:
> > > >> >
> > > >> > > Hi Rajini,
> > > >> > >
> > > >> > > I think it would be good to rename
> KafkaRequestContext to
> > > something
> > > >> like
> > > >> > > AuthorizableRequestContext, and put it in the
> > > >> > > org.apache.kafka.server.authorizer namespace. If
> we put
> > it in
> > > the
> > > >> > > org.apache.kafka.common namespace, then it's not
> really
> > clear
> > > that
> > > >> it's
> > > >> > > part of the Authorizer API. Since this class is
> purely an
> > > interface,
> > > >> with
> > > >> > > no concrete implementation of anything, there's
> nothing
> > common
> > > to
> > > >> really
> > > >> > > reuse in any case. We definitely don't want
> someone to
> > > accidentally
> > > >> add or
> > > >> > > remove methods because they think this is just
> another
> > internal
> > > class
> > > >> used
> > > >> > > for requests.
> > > >> > >
> > > >> > > The BrokerInfo class is a nice improvement. It
> looks
> > like it
> > > will be
> > > >> > > useful for passing in information about the context
> we're
> > > running
> > > >> in. It
> > > >> > > would be nice to call this class ServerInfo rather
> than
> > > BrokerInfo,
> > > >> since
> > > >> > > we will want to run the authorizer on controllers
> as well
> > as on
> > > >> brokers,
> > > >> > > and the controller may run as a separate process
> post
> > KIP-500.
> > > I also
> > > >> > > think that this class should be in the
> > > >> org.apache.kafka.server.authorizer
> > > >> > > namespace. Again, it is an interface, not a
> concrete
> > > implementation,
> > > >> and
> > > >> > > it's an interface that is very specifically for the
> > authorizer.
> > > >> > >
> > > >> > > I agree that we probably don't have enough
> information
> > > preserved for
> > > >> > > requests currently to always know what entity made
> them.
> > So we
> > > can
> > > >> leave
> > > >> > > that out for now (except in the special case of
> Fetch).
> > > Perhaps we
> > > >> can add
> > > >> > > this later if it's needed.
> > > >> > >
> > > >> > > I understand the intention behind AuthorizationMode
> > (which is
> > > now
> > > >> called
> > > >> > > AuditFlag in the latest revision). But it still
> feels
> > > complex. What
> > > >> if we
> > > >> > > just had two booleans in Action: logSuccesses and
> > logFailures?
> > > That
> > > >> seems
> > > >> > > to cover all the cases here. MANDATORY_AUTHORIZE =
> true,
> > true.
> > > >> > > OPTIONAL_AUTHORIZE = true, false. FILTER = true,
> false.
> > > >> LIST_AUTHORIZED =
> > > >> > > false, false. Would there be anything lost versus
> having
> > the
> > > enum?
> > > >> > >
> > > >> > > best,
> > > >> > > Colin
> > > >> > >
> > > >> > >
> > > >> > > On Wed, Aug 14, 2019, at 06:29, Mickael Maison
> wrote:
> > > >> > > > Hi Rajini,
> > > >> > > >
> > > >> > > > Thanks for the KIP!
> > > >> > > > I really like that authorize() will be able to
> take a
> > batch of
> > > >> > > > requests, this will speed up many implementations!
> > > >> > > >
> > > >> > > > On Tue, Aug 13, 2019 at 5:57 PM Rajini Sivaram <
> > > >> rajinisiva...@gmail.com>
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > Thanks David! I have fixed the typo.
> > > >> > > > >
> > > >> > > > > Also made a couple of changes to make the
> context
> > > interfaces more
> > > >> > > generic.
> > > >> > > > > KafkaRequestContext now returns the 16-bit API
> key as
> > Colin
> > > >> suggested
> > > >> > > as
> > > >> > > > > well as the friendly name used in metrics which
> are
> > useful
> > > in
> > > >> audit
> > > >> > > logs.
> > > >> > > > > `Authorizer#start` is now provided a generic
> > `BrokerInfo`
> > > >> interface
> > > >> > > that
> > > >> > > > > gives cluster id, broker id and endpoint
> information.
> > The
> > > generic
> > > >> > > interface
> > > >> > > > > can potentially be used in other broker plugins
> in
> > future
> > > and
> > > >> provides
> > > >> > > > > dynamically generated configs like broker id
> and ports
> > > which are
> > > >> > > currently
> > > >> > > > > not available to plugins unless these configs
> are
> > statically
> > > >> > > configured.
> > > >> > > > > Please let me know if there are any concerns.
> > > >> > > > >
> > > >> > > > > Regards,
> > > >> > > > >
> > > >> > > > > Rajini
> > > >> > > > >
> > > >> > > > > On Tue, Aug 13, 2019 at 4:30 PM David Jacot <
> > > dja...@confluent.io>
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi Rajini,
> > > >> > > > > >
> > > >> > > > > > Thank you for the update! It looks good to me.
> > There is a
> > > typo
> > > >> in the
> > > >> > > > > > `AuditFlag` enum: `MANDATORY_AUTHOEIZE` ->
> > > >> `MANDATORY_AUTHORIZE`.
> > > >> > > > > >
> > > >> > > > > > Regards,
> > > >> > > > > > David
> > > >> > > > > >
> > > >> > > > > > On Mon, Aug 12, 2019 at 2:54 PM Rajini
> Sivaram <
> > > >> > > rajinisiva...@gmail.com>
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > Hi David,
> > > >> > > > > > >
> > > >> > > > > > > Thanks for reviewing the KIP! Since
> questions
> > about
> > > >> `authorization
> > > >> > > mode`
> > > >> > > > > > > and `count` have come up multiple times, I
> have
> > renamed
> > > both.
> > > >> > > > > > >
> > > >> > > > > > > 1) Renamed `count` to
> `resourceReferenceCount`.
> > It is
> > > the
> > > >> number
> > > >> > > of times
> > > >> > > > > > > the resource being authorized is referenced
> > within the
> > > >> request.
> > > >> > > > > > >
> > > >> > > > > > > 2) Renamed `AuthorizationMode` to
> `AuditFlag`. It
> > is
> > > provided
> > > >> to
> > > >> > > improve
> > > >> > > > > > > audit logging in the authorizer. The enum
> values
> > have
> > > javadoc
> > > >> which
> > > >> > > > > > > indicate how the authorization result is
> used in
> > each
> > > of the
> > > >> modes
> > > >> > > to
> > > >> > > > > > > enable authorizers to log audit messages at
> the
> > right
> > > severity
> > > >> > > level.
> > > >> > > > > > >
> > > >> > > > > > > Regards,
> > > >> > > > > > >
> > > >> > > > > > > Rajini
> > > >> > > > > > >
> > > >> > > > > > > On Mon, Aug 12, 2019 at 5:54 PM David Jacot
> <
> > > >> dja...@confluent.io>
> > > >> > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Hi Rajini,
> > > >> > > > > > > >
> > > >> > > > > > > > Thank you for the KIP. Overall, it looks
> good
> > to me.
> > > I have
> > > >> few
> > > >> > > > > > > > questions/suggestions:
> > > >> > > > > > > >
> > > >> > > > > > > > 1. It is hard to grasp what
> `Action#count` is
> > for. I
> > > guess I
> > > >> > > understand
> > > >> > > > > > > > where you want to go with it but it took
> me a
> > while to
> > > >> figure it
> > > >> > > out.
> > > >> > > > > > > > Perhaps, we could come up with a better
> name
> > than
> > > `count`?
> > > >> > > > > > > >
> > > >> > > > > > > > 2. I had a hard time trying to understand
> the
> > > >> `AuthorizationMode`
> > > >> > > > > > > concept,
> > > >> > > > > > > > especially wrt. the OPTIONAL one. My
> > understanding is
> > > that
> > > >> an
> > > >> > > ACL is
> > > >> > > > > > > either
> > > >> > > > > > > > defined or not. Could you elaborate a bit
> more
> > on
> > > that?
> > > >> > > > > > > >
> > > >> > > > > > > > Thanks,
> > > >> > > > > > > > David
> > > >> > > > > > > >
> > > >> > > > > > > > On Fri, Aug 9, 2019 at 10:26 PM Don Bosco
> Durai
> > <
> > > >> > > bo...@apache.org>
> > > >> > > > > > > wrote:
> > > >> > > > > > > >
> > > >> > > > > > > > > Hi Rajini
> > > >> > > > > > > > >
> > > >> > > > > > > > > 3.2 - This makes sense. Thanks for
> clarifying.
> > > >> > > > > > > > >
> > > >> > > > > > > > > Rest looks fine. Once the
> implementations are
> > done,
> > > it
> > > >> will be
> > > >> > > more
> > > >> > > > > > > clear
> > > >> > > > > > > > > on the different values RequestType and
> Mode.
> > > >> > > > > > > > >
> > > >> > > > > > > > > Thanks
> > > >> > > > > > > > >
> > > >> > > > > > > > > Bosco
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > > On 8/9/19, 5:08 AM, "Rajini Sivaram" <
> > > >> rajinisiva...@gmail.com
> > > >> > > >
> > > >> > > > > > wrote:
> > > >> > > > > > > > >
> > > >> > > > > > > > > Hi Don,
> > > >> > > > > > > > >
> > > >> > > > > > > > > Thanks for the suggestions. A few
> > responses
> > > below:
> > > >> > > > > > > > >
> > > >> > > > > > > > > 3.1 Can rename and improve docs if
> we keep
> > > this. Let's
> > > >> > > finish the
> > > >> > > > > > > > > discussion on Colin's suggestions
> > regarding this
> > > >> first.
> > > >> > > > > > > > > 3.2 No, I was thinking of some
> requests
> > that
> > > have an
> > > >> old
> > > >> > > way of
> > > >> > > > > > > > > authorizing
> > > >> > > > > > > > > and a new way where we have
> retained the
> > old
> > > way for
> > > >> > > backward
> > > >> > > > > > > > > compatibility. One example is
> > Cluster:Create
> > > >> permission to
> > > >> > > create
> > > >> > > > > > > > > topics.
> > > >> > > > > > > > > We have replaced this with
> fine-grained
> > topic
> > > create
> > > >> > > access using
> > > >> > > > > > > > > Topic:Create
> > > >> > > > > > > > > for topic patterns. But we still
> check if
> > user
> > > has
> > > >> > > Cluster:Create
> > > >> > > > > > > > > first. If
> > > >> > > > > > > > > Denied, the deny is ignored and we
> check
> > > >> Topic:Create. We
> > > >> > > dont
> > > >> > > > > > want
> > > >> > > > > > > > to
> > > >> > > > > > > > > log
> > > >> > > > > > > > > DENY for Cluster:Create at INFO
> level for
> > this,
> > > since
> > > >> this
> > > >> > > is
> > > >> > > > > > not a
> > > >> > > > > > > > > mandatory ACL for creating topics.
> We
> > will get
> > > >> appropriate
> > > >> > > logs
> > > >> > > > > > > from
> > > >> > > > > > > > > the
> > > >> > > > > > > > > subsequent Topic:Create in this
> case.
> > > >> > > > > > > > > 3.3 They are not quite the same.
> FILTER
> > implies
> > > that
> > > >> user
> > > >> > > > > > actually
> > > >> > > > > > > > > requested access to and performed
> some
> > > operation on
> > > >> the
> > > >> > > filtered
> > > >> > > > > > > > > resources.
> > > >> > > > > > > > > LIST_AUTHORZED did not result in any
> > actual
> > > access.
> > > >> User
> > > >> > > simply
> > > >> > > > > > > > wanted
> > > >> > > > > > > > > to
> > > >> > > > > > > > > know what they are allowed to
> access.
> > > >> > > > > > > > > 3.4 Request types are Produce,
> JoinGroup,
> > > OffsetCommit
> > > >> > > etc. So
> > > >> > > > > > that
> > > >> > > > > > > > is
> > > >> > > > > > > > > different from authorization mode,
> > operation
> > > etc.
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > > On Thu, Aug 8, 2019 at 11:36 PM Don
> Bosco
> > Durai
> > > <
> > > >> > > > > > bo...@apache.org>
> > > >> > > > > > > > > wrote:
> > > >> > > > > > > > >
> > > >> > > > > > > > > > Hi Rajini
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Thanks for clarifying. This is
> very
> > helpful...
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > #1 - On the Ranger side, we
> should be
> > able to
> > > handle
> > > >> > > multiple
> > > >> > > > > > > > > requests at
> > > >> > > > > > > > > > the same time. I was just not
> sure how
> > much
> > > >> processing
> > > >> > > overhead
> > > >> > > > > > > > will
> > > >> > > > > > > > > be
> > > >> > > > > > > > > > there on the Broker side to split
> and
> > then
> > > >> consolidate
> > > >> > > the
> > > >> > > > > > > results.
> > > >> > > > > > > > > If it
> > > >> > > > > > > > > > is negligible, then this is the
> better
> > way.
> > > It will
> > > >> > > make it
> > > >> > > > > > > future
> > > >> > > > > > > > > proof.
> > > >> > > > > > > > > > #2 - I agree, having a single
> "start"
> > call
> > > makes it
> > > >> > > cleaner.
> > > >> > > > > > The
> > > >> > > > > > > > > > Authorization implementation will
> only
> > have
> > > to do
> > > >> the
> > > >> > > > > > > > initialization
> > > >> > > > > > > > > only
> > > >> > > > > > > > > > once.
> > > >> > > > > > > > > > #3.1 - Thanks for the
> clarification. I
> > think
> > > it
> > > >> makes
> > > >> > > sense to
> > > >> > > > > > > have
> > > >> > > > > > > > > this.
> > > >> > > > > > > > > > The term "Mode" might not be
> explicit
> > enough.
> > > >> > > Essentially it
> > > >> > > > > > > seems
> > > >> > > > > > > > > you want
> > > >> > > > > > > > > > the Authorizer to know the
> > Intent/Purpose of
> > > the
> > > >> > > authorize call
> > > >> > > > > > > and
> > > >> > > > > > > > > let the
> > > >> > > > > > > > > > Authorizer decide what to log as
> Audit
> > event.
> > > >> Changing
> > > >> > > the
> > > >> > > > > > > > > class/field name
> > > >> > > > > > > > > > or giving more documentation will
> do.
> > > >> > > > > > > > > > #3.2 - Regarding the option
> "OPTIONAL".
> > Are
> > > you
> > > >> thinking
> > > >> > > from
> > > >> > > > > > > > > chaining
> > > >> > > > > > > > > > multiple Authorizer? If so, I am
> not
> > sure
> > > whether
> > > >> the
> > > >> > > Broker
> > > >> > > > > > > would
> > > >> > > > > > > > > have
> > > >> > > > > > > > > > enough information to make that
> > decision. I
> > > feel the
> > > >> > > Authorizer
> > > >> > > > > > > > will
> > > >> > > > > > > > > be the
> > > >> > > > > > > > > > one who would have that
> knowledge. E.g.
> > in
> > > Ranger
> > > >> we have
> > > >> > > > > > > explicit
> > > >> > > > > > > > > deny,
> > > >> > > > > > > > > > which means no matter what, the
> request
> > > should be
> > > >> denied
> > > >> > > for
> > > >> > > > > > the
> > > >> > > > > > > > > user/group
> > > >> > > > > > > > > > or condition. So if you are
> thinking of
> > > chaining
> > > >> > > Authorizers,
> > > >> > > > > > > then
> > > >> > > > > > > > > > responses should have the third
> state,
> > e.g.
> > > >> > > "DENIED_FINAL", in
> > > >> > > > > > > > which
> > > >> > > > > > > > > case
> > > >> > > > > > > > > > if there is an Authorization
> chain, it
> > will
> > > be stop
> > > >> and
> > > >> > > the
> > > >> > > > > > > request
> > > >> > > > > > > > > will be
> > > >> > > > > > > > > > denied and if it is just denied,
> then
> > you
> > > might fall
> > > >> > > back to
> > > >> > > > > > next
> > > >> > > > > > > > > > authorizer. If we don't have
> chaining of
> > > >> Authorizing in
> > > >> > > mind,
> > > >> > > > > > > then
> > > >> > > > > > > > we
> > > >> > > > > > > > > > should reconsider OPTIONAL for
> now. Or
> > > clarify under
> > > >> > > which
> > > >> > > > > > > scenario
> > > >> > > > > > > > > > OPTIONAL will be called.
> > > >> > > > > > > > > > #3.3 Regarding, FILTER v/s
> > LIST_AUTHORIZED,
> > > isn't
> > > >> > > > > > > LIST_AUTHORIZED a
> > > >> > > > > > > > > > special case for "FILTER"?
> > > >> > > > > > > > > > #3.4 KafkaRequestContext.
> requestType()
> > v/s
> > > Action.
> > > >> > > > > > > > > authorizationMode. I
> > > >> > > > > > > > > > am not sure about the overlap or
> > ambiguity.
> > > >> > > > > > > > > > #4 - Cool. This is good, it will
> be less
> > > stress on
> > > >> the
> > > >> > > > > > > Authorizer.
> > > >> > > > > > > > > Ranger
> > > >> > > > > > > > > > already supports the "count"
> concept
> > and also
> > > has
> > > >> > > batching
> > > >> > > > > > > > > capability to
> > > >> > > > > > > > > > aggregate similar requests to
> reduce the
> > > number of
> > > >> audit
> > > >> > > logs
> > > >> > > > > > to
> > > >> > > > > > > > > write. We
> > > >> > > > > > > > > > should be able to pass this
> through.
> > > >> > > > > > > > > > #5 - Assuming if the object
> instance is
> > going
> > > out of
> > > >> > > scope, we
> > > >> > > > > > > > > should be
> > > >> > > > > > > > > > fine. Not a super important ask.
> Ranger
> > is
> > > already
> > > >> > > catching
> > > >> > > > > > KILL
> > > >> > > > > > > > > signal for
> > > >> > > > > > > > > > clean up.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Thanks again. These are good
> > enhancements.
> > > >> Appreciate
> > > >> > > your
> > > >> > > > > > > efforts
> > > >> > > > > > > > > here.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Bosco
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > On 8/8/19, 2:03 AM, "Rajini
> Sivaram" <
> > > >> > > rajinisiva...@gmail.com
> > > >> > > > > > >
> > > >> > > > > > > > > wrote:
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Hi Don,
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Thanks for reviewing the KIP.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > 1. I had this originally as a
> single
> > > Action, but
> > > >> > > thought it
> > > >> > > > > > > may
> > > >> > > > > > > > > be
> > > >> > > > > > > > > > useful
> > > >> > > > > > > > > > to support batched authorize
> calls
> > as
> > > well and
> > > >> keep
> > > >> > > it
> > > >> > > > > > > > > consistent with
> > > >> > > > > > > > > > other methods. Single
> requests can
> > contain
> > > >> multiple
> > > >> > > topics.
> > > >> > > > > > > For
> > > >> > > > > > > > > > example a
> > > >> > > > > > > > > > produce request can contain
> records
> > for
> > > several
> > > >> > > partitions
> > > >> > > > > > of
> > > >> > > > > > > > > different
> > > >> > > > > > > > > > topics. Broker could
> potentially
> > > authorize these
> > > >> > > together.
> > > >> > > > > > > For
> > > >> > > > > > > > > > SimpleAclAuthorizer, batched
> > authorize
> > > methods
> > > >> don't
> > > >> > > > > > provide
> > > >> > > > > > > > any
> > > >> > > > > > > > > > optimisation since lookup is
> based
> > on
> > > resources
> > > >> > > followed by
> > > >> > > > > > > the
> > > >> > > > > > > > > > matching
> > > >> > > > > > > > > > logic. But some authorizers
> may
> > manage
> > > ACLs by
> > > >> user
> > > >> > > > > > principal
> > > >> > > > > > > > > rather
> > > >> > > > > > > > > > than
> > > >> > > > > > > > > > resource and may be able to
> optimize
> > > batched
> > > >> > > requests. I am
> > > >> > > > > > > ok
> > > >> > > > > > > > > with
> > > >> > > > > > > > > > using
> > > >> > > > > > > > > > single Action if this is
> likely to
> > cause
> > > issues.
> > > >> > > > > > > > > > 2. If you have two listeners,
> one
> > for
> > > >> inter-broker
> > > >> > > traffic
> > > >> > > > > > > and
> > > >> > > > > > > > > another
> > > >> > > > > > > > > > for
> > > >> > > > > > > > > > external clients, start
> method is
> > invoked
> > > twice,
> > > >> > > once for
> > > >> > > > > > > each
> > > >> > > > > > > > > > listener. On
> > > >> > > > > > > > > > second thought, that may be
> > confusing and
> > > a
> > > >> single
> > > >> > > start()
> > > >> > > > > > > > > invocation
> > > >> > > > > > > > > > that
> > > >> > > > > > > > > > provides all listener
> information
> > and
> > > returns
> > > >> > > multiple
> > > >> > > > > > > futures
> > > >> > > > > > > > > would be
> > > >> > > > > > > > > > better. Will update the KIP.
> > > >> > > > > > > > > > 3. A typical example is a
> consumer
> > > subscribing
> > > >> to a
> > > >> > > regex
> > > >> > > > > > > > > pattern. We
> > > >> > > > > > > > > > request all topic metadata
> from the
> > > broker in
> > > >> order
> > > >> > > to
> > > >> > > > > > decide
> > > >> > > > > > > > > whether
> > > >> > > > > > > > > > the
> > > >> > > > > > > > > > pattern matches, expecting to
> > receive a
> > > list of
> > > >> > > authorised
> > > >> > > > > > > > > topics. The
> > > >> > > > > > > > > > user
> > > >> > > > > > > > > > is not asking to subscribe to
> an
> > > unauthorized
> > > >> topic.
> > > >> > > If
> > > >> > > > > > there
> > > >> > > > > > > > > are 10000
> > > >> > > > > > > > > > topics in the cluster and the
> user
> > has
> > > access
> > > >> to 100
> > > >> > > of
> > > >> > > > > > them,
> > > >> > > > > > > > at
> > > >> > > > > > > > > the
> > > >> > > > > > > > > > moment
> > > >> > > > > > > > > > we log 9900 DENIED log
> entries at
> > INFO
> > > level in
> > > >> > > > > > > > > SimpleAclAuthorizer.
> > > >> > > > > > > > > > The
> > > >> > > > > > > > > > proposal is to authorize this
> > request with
> > > >> > > > > > > > > AuthorizationMode.FILTER, so
> > > >> > > > > > > > > > that authorizers can log
> resources
> > that
> > > are
> > > >> filtered
> > > >> > > out at
> > > >> > > > > > > > > lower level
> > > >> > > > > > > > > > like DEBUG since this is not
> an
> > attempt to
> > > >> access
> > > >> > > > > > > unauthorized
> > > >> > > > > > > > > > resources.
> > > >> > > > > > > > > > Brokers already handle these
> > differently
> > > since
> > > >> no
> > > >> > > > > > > authorization
> > > >> > > > > > > > > error
> > > >> > > > > > > > > > is
> > > >> > > > > > > > > > returned to the client in
> these
> > cases.
> > > Providing
> > > >> > > > > > > authorization
> > > >> > > > > > > > > mode to
> > > >> > > > > > > > > > authorizers enables authorizer
> > > implementations
> > > >> to
> > > >> > > generate
> > > >> > > > > > > > > better audit
> > > >> > > > > > > > > > logs.
> > > >> > > > > > > > > > 4. Each request may contain
> multiple
> > > instances
> > > >> of
> > > >> > > the same
> > > >> > > > > > > > > authorizable
> > > >> > > > > > > > > > resource. For example a
> produce
> > request
> > > may
> > > >> contain
> > > >> > > records
> > > >> > > > > > > for
> > > >> > > > > > > > > 10
> > > >> > > > > > > > > > partitions of the same topic.
> At the
> > > moment, we
> > > >> > > invoke
> > > >> > > > > > > > authorize
> > > >> > > > > > > > > > method 10
> > > >> > > > > > > > > > times. The proposal is to
> invoke it
> > once
> > > with
> > > >> > > count=10. The
> > > >> > > > > > > > > count is
> > > >> > > > > > > > > > provided to authorizer just
> for
> > audit
> > > logging
> > > >> > > purposes.
> > > >> > > > > > > > > > 5. Authorizer implements
> Closeable,
> > so you
> > > >> could use
> > > >> > > > > > close()
> > > >> > > > > > > to
> > > >> > > > > > > > > flush
> > > >> > > > > > > > > > audits?
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > On Thu, Aug 8, 2019 at 7:01
> AM Don
> > Bosco
> > > Durai <
> > > >> > > > > > > > bo...@apache.org
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > wrote:
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > > Rajini
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Thanks for putting this
> together.
> > It is
> > > >> looking
> > > >> > > good. I
> > > >> > > > > > > have
> > > >> > > > > > > > > few
> > > >> > > > > > > > > > > questions...
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > 1. List<AuthorizationResult>
> > > authorize(...,
> > > >> > > List<Action>
> > > >> > > > > > > > > actions).
> > > >> > > > > > > > > > Do you
> > > >> > > > > > > > > > > see a scenario where the
> broker
> > will
> > > call
> > > >> > > authorize for
> > > >> > > > > > > > > multiple
> > > >> > > > > > > > > > topics at
> > > >> > > > > > > > > > > the same time? I can
> understand
> > that
> > > during
> > > >> > > > > > > creating/deleting
> > > >> > > > > > > > > ACLS,
> > > >> > > > > > > > > > > multiple permissions for
> multiple
> > > resources
> > > >> might
> > > >> > > be
> > > >> > > > > > done.
> > > >> > > > > > > > For
> > > >> > > > > > > > > > authorize
> > > >> > > > > > > > > > > call, would this be a case?
> And
> > does the
> > > >> Authorize
> > > >> > > > > > > > > implementation
> > > >> > > > > > > > > > will be
> > > >> > > > > > > > > > > able to do performance
> > optimization
> > > because of
> > > >> > > this? Or
> > > >> > > > > > > > should
> > > >> > > > > > > > > we
> > > >> > > > > > > > > > just keep
> > > >> > > > > > > > > > > it simple? I don't see it
> as an
> > issue
> > > from
> > > >> Apache
> > > >> > > Ranger
> > > >> > > > > > > > side,
> > > >> > > > > > > > > but
> > > >> > > > > > > > > > just
> > > >> > > > > > > > > > > checking to see whether we
> need
> > to be
> > > aware of
> > > >> > > something.
> > > >> > > > > > > > > > > 2. Should I assume that the
> > > SecurityProtocol
> > > >> passed
> > > >> > > > > > during
> > > >> > > > > > > > > start and
> > > >> > > > > > > > > > the
> > > >> > > > > > > > > > > one return by
> > > >> > > KafkaRequestContext.securityProtocol() will
> > > >> > > > > > > be
> > > >> > > > > > > > > the
> > > >> > > > > > > > > > same?
> > > >> > > > > > > > > > > CompletableFuture<Void>
> > start(String
> > > >> listenerName,
> > > >> > > > > > > > > SecurityProtocol
> > > >> > > > > > > > > > > securityProtocol);
> > > >> > > > > > > > > > >
> > KafkaRequestContext.securityProtocol()
> > > >> > > > > > > > > > > 3. What is the purpose of
> > > AuthorizationMode?
> > > >> How
> > > >> > > does the
> > > >> > > > > > > > > broker
> > > >> > > > > > > > > > decide
> > > >> > > > > > > > > > > what mode to use when the
> > authorize()
> > > method
> > > >> is
> > > >> > > called?
> > > >> > > > > > > > > > > 4. Can we clarify "count" in
> > Action a
> > > bit
> > > >> more?
> > > >> > > How is it
> > > >> > > > > > > > used?
> > > >> > > > > > > > > > > 5. Do you feel having "stop"
> > along with
> > > >> "start" be
> > > >> > > > > > helpful?
> > > >> > > > > > > > > E.g. In
> > > >> > > > > > > > > > Ranger
> > > >> > > > > > > > > > > we try to optimize the Audit
> > writing by
> > > >> caching
> > > >> > > the logs
> > > >> > > > > > > for
> > > >> > > > > > > > a
> > > >> > > > > > > > > fixed
> > > >> > > > > > > > > > > interval. But when the
> Broker
> > > terminates, we
> > > >> do a
> > > >> > > forced
> > > >> > > > > > > > flush.
> > > >> > > > > > > > > > Having an
> > > >> > > > > > > > > > > explicit "stop" might give
> us a
> > formal
> > > way to
> > > >> > > flush our
> > > >> > > > > > > > audits.
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Thanks
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Bosco
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > On 8/7/19, 3:59 PM, "Rajini
> > Sivaram" <
> > > >> > > > > > > > rajinisiva...@gmail.com
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > wrote:
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Hi Ron/Harsha/Satish,
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Thanks for reviewing
> the KIP!
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > We should perhaps have
> a wider
> > > discussion
> > > >> > > outside
> > > >> > > > > > this
> > > >> > > > > > > > KIP
> > > >> > > > > > > > > for
> > > >> > > > > > > > > > > refactoring
> > > >> > > > > > > > > > > clients so that others
> who
> > are not
> > > >> following
> > > >> > > this KIP
> > > >> > > > > > > > also
> > > >> > > > > > > > > > notice the
> > > >> > > > > > > > > > > discussion. Satish,
> would you
> > like
> > > to
> > > >> start a
> > > >> > > > > > > discussion
> > > >> > > > > > > > > thread
> > > >> > > > > > > > > > on dev?
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Regards,
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Rajini
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > On Wed, Aug 7, 2019 at
> 6:21 PM
> > > Satish
> > > >> Duggana <
> > > >> > > > > > > > > > > satish.dugg...@gmail.com>
> > > >> > > > > > > > > > > wrote:
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > > I felt the same need
> when
> > we want
> > > to
> > > >> add a
> > > >> > > > > > pluggable
> > > >> > > > > > > > API
> > > >> > > > > > > > > for
> > > >> > > > > > > > > > core
> > > >> > > > > > > > > > > > server functionality.
> This
> > does
> > > not
> > > >> need to
> > > >> > > be part
> > > >> > > > > > > of
> > > >> > > > > > > > > this
> > > >> > > > > > > > > > KIP, it
> > > >> > > > > > > > > > > > can be a separate
> KIP. I can
> > > contribute
> > > >> those
> > > >> > > > > > > > refactoring
> > > >> > > > > > > > > > changes if
> > > >> > > > > > > > > > > > others are OK with
> that.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > It is better to have a
> > structure
> > > like
> > > >> below.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > kafka-common:
> > > >> > > > > > > > > > > > common classes which
> can be
> > used
> > > in any
> > > >> of
> > > >> > > the
> > > >> > > > > > other
> > > >> > > > > > > > > modules
> > > >> > > > > > > > > > in Kafka
> > > >> > > > > > > > > > > > like client,
> > Kafka-server-common
> > > and
> > > >> server
> > > >> > > etc.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > kafka-client-common:
> > > >> > > > > > > > > > > > common classes which
> can be
> > used
> > > in the
> > > >> > > client
> > > >> > > > > > > module.
> > > >> > > > > > > > > This
> > > >> > > > > > > > > > can be
> > > >> > > > > > > > > > > > part of client module
> > itself.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > kafka-server-common:
> > > >> > > > > > > > > > > > classes required only
> for
> > > kafka-server.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > Thanks.
> > > >> > > > > > > > > > > > Satish.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > On Wed, Aug 7, 2019
> at 9:28
> > PM
> > > Harsha
> > > >> > > Chintalapani
> > > >> > > > > > <
> > > >> > > > > > > > > > ka...@harsha.io>
> > > >> > > > > > > > > > > > wrote:
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Thanks for the KIP
> Rajini.
> > > >> > > > > > > > > > > > > Quick thought, it
> would
> > be good
> > > to
> > > >> have a
> > > >> > > common
> > > >> > > > > > > > module
> > > >> > > > > > > > > > outside of
> > > >> > > > > > > > > > > > clients
> > > >> > > > > > > > > > > > > that only applies to
> > server side
> > > >> > > interfaces &
> > > >> > > > > > > > changes.
> > > >> > > > > > > > > It
> > > >> > > > > > > > > > looks
> > > >> > > > > > > > > > > like we
> > > >> > > > > > > > > > > > are
> > > >> > > > > > > > > > > > > increasingly in
> favor of
> > using
> > > Java
> > > >> > > interface for
> > > >> > > > > > > > > pluggable
> > > >> > > > > > > > > > > modules on
> > > >> > > > > > > > > > > > the
> > > >> > > > > > > > > > > > > broker side.
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > > > > Harsha
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > On Tue, Aug 06,
> 2019 at
> > 2:31 PM,
> > > >> Rajini
> > > >> > > Sivaram <
> > > >> > > > > > > > > > > rajinisiva...@gmail.com
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > wrote:
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > Hi all,
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > I have created a
> KIP to
> > > replace the
> > > >> Scala
> > > >> > > > > > > > Authorizer
> > > >> > > > > > > > > API
> > > >> > > > > > > > > > with a
> > > >> > > > > > > > > > > new
> > > >> > > > > > > > > > > > Java
> > > >> > > > > > > > > > > > > > API:
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > -
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/
> > > >> > > > > > > > > > > > > >
> > > >> > > KIP-504+-+Add+new+Java+Authorizer+Interface
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > This is
> replacement for
> > KIP-50
> > > >> which was
> > > >> > > > > > accepted
> > > >> > > > > > > > > but never
> > > >> > > > > > > > > > > merged.
> > > >> > > > > > > > > > > > Apart
> > > >> > > > > > > > > > > > > > from moving to a
> Java
> > API
> > > consistent
> > > >> > > with other
> > > >> > > > > > > > > pluggable
> > > >> > > > > > > > > > > interfaces
> > > >> > > > > > > > > > > > in the
> > > >> > > > > > > > > > > > > > broker, KIP-504
> also
> > attempts
> > > to
> > > >> address
> > > >> > > known
> > > >> > > > > > > > > limitations
> > > >> > > > > > > > > > in the
> > > >> > > > > > > > > > > > > > authorizer. If
> you have
> > come
> > > across
> > > >> other
> > > >> > > > > > > > > limitations that
> > > >> > > > > > > > > > you
> > > >> > > > > > > > > > > would
> > > >> > > > > > > > > > > > like
> > > >> > > > > > > > > > > > > > to see addressed
> in the
> > new
> > > API,
> > > >> please
> > > >> > > raise
> > > >> > > > > > > these
> > > >> > > > > > > > > on the
> > > >> > > > > > > > > > > discussion
> > > >> > > > > > > > > > > > > > thread so that we
> can
> > > consider those
> > > >> > > too. All
> > > >> > > > > > > > > suggestions
> > > >> > > > > > > > > > and
> > > >> > > > > > > > > > > feedback
> > > >> > > > > > > > > > > > are
> > > >> > > > > > > > > > > > > > welcome.
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > Thank you,
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > Rajini
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>
>
