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