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