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
> > > >
> >
>
>
>
>
