Hi Rajini, Thanks for the KIP. This will be a great improvement.
Why not just pass the cluster ID directly to Authorizer#start, rather than dealing with the ClusterResourceListener interface? That seems like it would be simpler. If authorizers don't need that information, they can ignore that parameter. Do we really need to change the configuration key name from authorizer.class.name to authorizer.class? We should be able to use introspection to see whether it's a subclass of the old or the new interface, right? This would avoid having two configuration keys that were very similar. The Authorizer JavaDoc should have a comment about thread safety. I assume that all of the Authorizer methods need to be thread-safe, right? For CreateAcls and DeleteAcls, it seems like we would be better off just using AclBinding objects. An AclBinding object contains a ResourcePattern and an AccessControlEntry. There is no need to pass them separately. It seems like we don't need all the different overloads of deleteAcls. We can just pass in an AclBindingFilter, and that can delete whatever is needed. For example, if you want to delete all the ACLs for a specific resource, you could pass in an AclBindingFilter with AccessControlEntryFilter.ANY. It would be nice to have deleteAcls return the number of entries that were deleted. I like the way KafkaRequestContext is split from Action. That will be helpful. We need a way of listing all the ACLs, because that will be needed for the AdminClient. The simplest API would be just passing in an AclBindingFilter. Listing the ACLs for a specific resource is just a special case, which doesn't need its own function. Since the number of ACLs could be quite large, we probably want to return an iterator rather than a full collection. We can eventually make the iterator load results lazily if needed. AuthorizationMode doesn't really make sense to me. It seems to be designed to allow us to log different kinds of authorization failures differently. But it would be simpler to just split the API into two parts: getting an authorization result, and logging it. The Authorizer#authorize method should return a result that we can later pass to an Authorizer#logFailure method. Then, when code wants to try to authorize against X, and then if that fails authorize against Y, it can simply only pass the second failure result to Authorizer#logFailure to avoid creating extra log messages. Or we could create a method AuthorizationResult#logFaliure(). For requestType, I don't think this should be a string. The 16-bit API key is clearer and more concise. The request names currently aren't part of the stable API, and it would be nice to keep it that way. To distinguish between follower fetches and consumer fetches, we should just have a separate way of knowing that the broker made the request. Maybe something like KafkaRequestContext#Originator. Then we can have Originator#CLIENT and Originator#BROKER. It's hard to understand what Action#count is for. If we want to log something like "failed to authorize deleting 123 partitions", it would be better to just let the caller supply some custom text in Authorizer#logFailure. Thanks again for the KIP. best, Colin On Thu, Aug 8, 2019, at 02:03, Rajini Sivaram 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 > > > > > > > > > > > > > > > > >
