From dev-return-107119-archive-asf-public=cust-asf.ponee.io@kafka.apache.org Tue Sep 3 17:23:24 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id AB530180637 for ; Tue, 3 Sep 2019 19:23:23 +0200 (CEST) Received: (qmail 30549 invoked by uid 500); 3 Sep 2019 19:51:01 -0000 Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kafka.apache.org Delivered-To: mailing list dev@kafka.apache.org Received: (qmail 30538 invoked by uid 99); 3 Sep 2019 19:51:01 -0000 Received: from Unknown (HELO mailrelay1-lw-us.apache.org) (10.10.3.159) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 03 Sep 2019 19:51:01 +0000 Received: from auth2-smtp.messagingengine.com (auth2-smtp.messagingengine.com [66.111.4.228]) by mailrelay1-lw-us.apache.org (ASF Mail Server at mailrelay1-lw-us.apache.org) with ESMTPSA id 6E35A59D9 for ; Tue, 3 Sep 2019 17:23:22 +0000 (UTC) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailauth.nyi.internal (Postfix) with ESMTP id 23E5D220AF for ; Tue, 3 Sep 2019 13:23:22 -0400 (EDT) Received: from imap1 ([10.202.2.51]) by compute2.internal (MEProxy); Tue, 03 Sep 2019 13:23:22 -0400 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduvddrudejfedgkeefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefofgggkfgjfhffhffvufgtgfesth hqredtreerjeenucfhrhhomhepfdevohhlihhnucfotgevrggsvgdfuceotghmtggtrggs vgesrghprggthhgvrdhorhhgqeenucffohhmrghinheprghprggthhgvrdhorhhgnecurf grrhgrmhepmhgrihhlfhhrohhmpegtmhgttggrsggvfedugedomhgvshhmthhprghuthhh phgvrhhsohhnrghlihhthidqgeeiudekgedufedtqdduheehkeekhedugedqtghmtggtrg gsvgeppegrphgrtghhvgdrohhrghesfhgrshhtmhgrihhlrdgtohhmnecuvehluhhsthgv rhfuihiivgeptd X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id C5246C200A5; Tue, 3 Sep 2019 13:23:21 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.1.7-154-gfa7592a-fmstable-20190829v1 Mime-Version: 1.0 Message-Id: In-Reply-To: References: <48CB3DB7-709A-4479-8EDB-A4D95B26728E@apache.org> <2FB0D77D-5E49-4232-B7DE-BD83780C9CFD@apache.org> Date: Tue, 03 Sep 2019 10:22:16 -0700 From: "Colin McCabe" To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-504 - Add new Java Authorizer Interface Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Rajini, That's an interesting point. I guess I assumed that we would always cac= he metadata locally, so that a synchronous operation would be OK here. = But, I suppose as time goes on, we will eventually want paged authorizat= ion metadata. If the operations are done asynchronously, then that implies background = thread(s) at work somewhere. We should specify when that these thread(s= ) are created, and when they are stopped. Probably they should be creat= ed in start, and stopped in close? We should update the JavaDoc. best, Colin On Tue, Sep 3, 2019, at 05:50, Rajini Sivaram wrote: > Hi all, >=20 > Ismael brought up a point that it will be good to make the Authorizer > interface asynchronous to avoid blocking request threads during remote= > operations. >=20 > 1) Since we want to support different backends for authorization metad= ata, > making createAcls() and deleteAcls() asynchronous makes sense since th= ese > always involve remote operations. When KIP-500 removes ZooKeeper, we w= ould > 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-i= n > authorizers, so synchronous authorise operations work well now. But as= ync > authorize() would support this scenario as well as authorizers in larg= e > organisations where an LRU cache would enable a smaller cache even whe= n the > backend holds a large amount of ACLs for infrequently used resources o= r > users who don't use the system frequently. >=20 > For both cases, the built-in authorizer will continue to be synchronou= s, > 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. >=20 > *Proposed API:* > public interface Authorizer extends Configurable, Closeable { >=20 > Map> start(AuthorizerServerInfo se= rverInfo); > List> > authorize(AuthorizableRequestContext requestContext, List > actions); > List> > createAcls(AuthorizableRequestContext requestContext, List= > aclBindings); > List> > deleteAcls(AuthorizableRequestContext requestContext, > List aclBindingFilters); > CompletionStage> acls(AclBindingFilter filt= er); > } >=20 >=20 > Thank you, >=20 > Rajini >=20 > On Sun, Aug 18, 2019 at 6:25 PM Don Bosco Durai wro= te: >=20 > > Hi Rajini > > > > Thanks for clarifying. I am good for now. > > > > Regards > > > > Bosco > > > > > > =EF=BB=BFOn 8/16/19, 11:30 AM, "Rajini Sivaram" wrote: > > > > Hi Don, > > > > That should be fine. I guess Ranger loads policies from the data= base > > 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 empt= y map > > from > > `start()`. That would retain the existing behaviour.. When the s= ame > > 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 > > 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=E2=80=99t store metadata in ZooKeepe= r, ensure that > > > authorizer metadata for each listener is available before star= ting > > up the > > > listener. This enables different authorization metadata stores= for > > > different listeners." > > > > > > Since Ranger uses its own database for storing policies, do yo= u > > anticipate > > > any issues? > > > > > > Thanks > > > > > > Bosco > > > > > > > > > =EF=BB=BFOn 8/16/19, 6:49 AM, "Rajini Sivaram" > > 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 avail= able, > > this > > > can > > > break custom authorizers that extend this class and overri= de > > specific > > > methods of the old API. To avoid breaking any existing cus= tom > > > implementations that extend this class, particularly becau= se it > > is in > > > the > > > public package kafka.security.auth, the KIP now proposes t= o > > retain the > > > old > > > authorizer as-is. SimpleAclAuthorizer will continue to > > implement the > > > old > > > API, but will be deprecated. A new authorizer implementati= on > > > 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 < > > > rajinisivaram@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 < > > cmccabe@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 pac= kage. > > These > > > are now > > > >> > called AuthorizableRequestContext and AuthorizerServe= rInfo. > > > Endpoint is > > > >> now > > > >> > a class in org.apache.kafka.common to make it reusabl= e > > since we > > > already > > > >> > have multiple implementations of it. I have removed > > requestName > > > from the > > > >> > request context interface since authorizers can disti= nguish > > > follower > > > >> fetch > > > >> > and consumer fetch from the operation being authorize= d. So > > 16-bit > > > >> request > > > >> > type should be sufficient for audit logging. Also re= placed > > > 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 < > > cmccabe@apache.org> > > > >> wrote: > > > >> > > > > >> > > Hi Rajini, > > > >> > > > > > >> > > I think it would be good to rename KafkaRequestCont= ext to > > > something > > > >> like > > > >> > > AuthorizableRequestContext, and put it in the > > > >> > > org.apache.kafka.server.authorizer namespace. If w= e put > > it in > > > the > > > >> > > org.apache.kafka.common namespace, then it's not re= ally > > clear > > > that > > > >> it's > > > >> > > part of the Authorizer API. Since this class is pu= rely an > > > interface, > > > >> with > > > >> > > no concrete implementation of anything, there's not= hing > > common > > > to > > > >> really > > > >> > > reuse in any case. We definitely don't want someon= e to > > > accidentally > > > >> add or > > > >> > > remove methods because they think this is just anot= her > > internal > > > class > > > >> used > > > >> > > for requests. > > > >> > > > > > >> > > The BrokerInfo class is a nice improvement. It loo= ks > > 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 a= s well > > as on > > > >> brokers, > > > >> > > and the controller may run as a separate process po= st > > 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 concre= te > > > implementation, > > > >> and > > > >> > > it's an interface that is very specifically for the= > > authorizer. > > > >> > > > > > >> > > I agree that we probably don't have enough informat= ion > > > 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 Fet= ch). > > > 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 fe= els > > > complex. What > > > >> if we > > > >> > > just had two booleans in Action: logSuccesses and > > logFailures? > > > That > > > >> seems > > > >> > > to cover all the cases here. MANDATORY_AUTHORIZE =3D= true, > > true. > > > >> > > OPTIONAL_AUTHORIZE =3D true, false. FILTER =3D tru= e, false. > > > >> LIST_AUTHORIZED =3D > > > >> > > false, false. Would there be anything lost versus = having > > the > > > enum? > > > >> > > > > > >> > > best, > > > >> > > Colin > > > >> > > > > > >> > > > > > >> > > On Wed, Aug 14, 2019, at 06:29, Mickael Maison wrot= e: > > > >> > > > Hi Rajini, > > > >> > > > > > > >> > > > Thanks for the KIP! > > > >> > > > I really like that authorize() will be able to ta= ke a > > batch of > > > >> > > > requests, this will speed up many implementations= ! > > > >> > > > > > > >> > > > On Tue, Aug 13, 2019 at 5:57 PM Rajini Sivaram < > > > >> rajinisivaram@gmail.com> > > > >> > > wrote: > > > >> > > > > > > > >> > > > > Thanks David! I have fixed the typo. > > > >> > > > > > > > >> > > > > Also made a couple of changes to make the conte= xt > > > 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 inform= ation. > > The > > > generic > > > >> > > interface > > > >> > > > > can potentially be used in other broker plugins= in > > future > > > and > > > >> provides > > > >> > > > > dynamically generated configs like broker id an= d ports > > > which are > > > >> > > currently > > > >> > > > > not available to plugins unless these configs a= re > > statically > > > >> > > configured. > > > >> > > > > Please let me know if there are any concerns. > > > >> > > > > > > > >> > > > > Regards, > > > >> > > > > > > > >> > > > > Rajini > > > >> > > > > > > > >> > > > > On Tue, Aug 13, 2019 at 4:30 PM David Jacot < > > > djacot@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 Sivara= m < > > > >> > > rajinisivaram@gmail.com> > > > >> > > > > > wrote: > > > >> > > > > > > > > >> > > > > > > Hi David, > > > >> > > > > > > > > > >> > > > > > > Thanks for reviewing the KIP! Since questio= ns > > about > > > >> `authorization > > > >> > > mode` > > > >> > > > > > > and `count` have come up multiple times, I = have > > renamed > > > both. > > > >> > > > > > > > > > >> > > > > > > 1) Renamed `count` to `resourceReferenceCou= nt`. > > It is > > > the > > > >> number > > > >> > > of times > > > >> > > > > > > the resource being authorized is referenced= > > within the > > > >> request. > > > >> > > > > > > > > > >> > > > > > > 2) Renamed `AuthorizationMode` to `AuditFla= g`. It > > is > > > provided > > > >> to > > > >> > > improve > > > >> > > > > > > audit logging in the authorizer. The enum v= alues > > have > > > javadoc > > > >> which > > > >> > > > > > > indicate how the authorization result is us= ed 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= < > > > >> djacot@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 n= ame > > 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 > > < > > > >> > > bosco@apache.org> > > > >> > > > > > > wrote: > > > >> > > > > > > > > > > >> > > > > > > > > Hi Rajini > > > >> > > > > > > > > > > > >> > > > > > > > > 3.2 - This makes sense. Thanks for clar= ifying. > > > >> > > > > > > > > > > > >> > > > > > > > > Rest looks fine. Once the implementatio= ns are > > done, > > > it > > > >> will be > > > >> > > more > > > >> > > > > > > clear > > > >> > > > > > > > > on the different values RequestType and= Mode. > > > >> > > > > > > > > > > > >> > > > > > > > > Thanks > > > >> > > > > > > > > > > > >> > > > > > > > > Bosco > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > =EF=BB=BFOn 8/9/19, 5:08 AM, "Rajini Si= varam" < > > > >> rajinisivaram@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 requ= ests > > that > > > have an > > > >> old > > > >> > > way of > > > >> > > > > > > > > authorizing > > > >> > > > > > > > > and a new way where we have retaine= d the > > old > > > way for > > > >> > > backward > > > >> > > > > > > > > compatibility. One example is > > Cluster:Create > > > >> permission to > > > >> > > create > > > >> > > > > > > > > topics. > > > >> > > > > > > > > We have replaced this with fine-gra= ined > > topic > > > create > > > >> > > access using > > > >> > > > > > > > > Topic:Create > > > >> > > > > > > > > for topic patterns. But we still ch= eck 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 lev= el for > > this, > > > since > > > >> this > > > >> > > is > > > >> > > > > > not a > > > >> > > > > > > > > mandatory ACL for creating topics. = We > > will get > > > >> appropriate > > > >> > > logs > > > >> > > > > > > from > > > >> > > > > > > > > the > > > >> > > > > > > > > subsequent Topic:Create in this cas= e. > > > >> > > > > > > > > 3.3 They are not quite the same. FI= LTER > > implies > > > that > > > >> user > > > >> > > > > > actually > > > >> > > > > > > > > requested access to and performed s= ome > > > operation on > > > >> the > > > >> > > filtered > > > >> > > > > > > > > resources. > > > >> > > > > > > > > LIST_AUTHORZED did not result in an= y > > actual > > > access. > > > >> User > > > >> > > simply > > > >> > > > > > > > wanted > > > >> > > > > > > > > to > > > >> > > > > > > > > know what they are allowed to acces= s. > > > >> > > > > > > > > 3.4 Request types are Produce, Join= Group, > > > OffsetCommit > > > >> > > etc. So > > > >> > > > > > that > > > >> > > > > > > > is > > > >> > > > > > > > > different from authorization mode, > > operation > > > etc. > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > On Thu, Aug 8, 2019 at 11:36 PM Don= Bosco > > Durai > > > < > > > >> > > > > > bosco@apache.org> > > > >> > > > > > > > > wrote: > > > >> > > > > > > > > > > > >> > > > > > > > > > Hi Rajini > > > >> > > > > > > > > > > > > >> > > > > > > > > > Thanks for clarifying. This is ve= ry > > helpful... > > > >> > > > > > > > > > > > > >> > > > > > > > > > #1 - On the Ranger side, we shoul= d be > > able to > > > handle > > > >> > > multiple > > > >> > > > > > > > > requests at > > > >> > > > > > > > > > the same time. I was just not sur= e 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 "s= tart" > > call > > > makes it > > > >> > > cleaner. > > > >> > > > > > The > > > >> > > > > > > > > > Authorization implementation will= only > > have > > > to do > > > >> the > > > >> > > > > > > > initialization > > > >> > > > > > > > > only > > > >> > > > > > > > > > once. > > > >> > > > > > > > > > #3.1 - Thanks for the clarificati= on. I > > think > > > it > > > >> makes > > > >> > > sense to > > > >> > > > > > > have > > > >> > > > > > > > > this. > > > >> > > > > > > > > > The term "Mode" might not be expl= icit > > 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 "OPTI= ONAL". > > 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 r= equest > > > should be > > > >> denied > > > >> > > for > > > >> > > > > > the > > > >> > > > > > > > > user/group > > > >> > > > > > > > > > or condition. So if you are think= ing of > > > chaining > > > >> > > Authorizers, > > > >> > > > > > > then > > > >> > > > > > > > > > responses should have the third s= tate, > > e.g. > > > >> > > "DENIED_FINAL", in > > > >> > > > > > > > which > > > >> > > > > > > > > case > > > >> > > > > > > > > > if there is an Authorization chai= n, 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 chai= ning of > > > >> Authorizing in > > > >> > > mind, > > > >> > > > > > > then > > > >> > > > > > > > we > > > >> > > > > > > > > > should reconsider OPTIONAL for no= w. 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. request= Type() > > 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" conc= ept > > and also > > > has > > > >> > > batching > > > >> > > > > > > > > capability to > > > >> > > > > > > > > > aggregate similar requests to red= uce the > > > number of > > > >> audit > > > >> > > logs > > > >> > > > > > to > > > >> > > > > > > > > write. We > > > >> > > > > > > > > > should be able to pass this throu= gh. > > > >> > > > > > > > > > #5 - Assuming if the object insta= nce 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 > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > =EF=BB=BFOn 8/8/19, 2:03 AM, "Raj= ini Sivaram" < > > > >> > > rajinisivaram@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 request= s can > > contain > > > >> multiple > > > >> > > topics. > > > >> > > > > > > For > > > >> > > > > > > > > > example a > > > >> > > > > > > > > > produce request can contain r= ecords > > for > > > several > > > >> > > partitions > > > >> > > > > > of > > > >> > > > > > > > > different > > > >> > > > > > > > > > topics. Broker could potentia= lly > > > 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 m= ay > > manage > > > ACLs by > > > >> user > > > >> > > > > > principal > > > >> > > > > > > > > rather > > > >> > > > > > > > > > than > > > >> > > > > > > > > > resource and may be able to o= ptimize > > > batched > > > >> > > requests. I am > > > >> > > > > > > ok > > > >> > > > > > > > > with > > > >> > > > > > > > > > using > > > >> > > > > > > > > > single Action if this is like= ly to > > cause > > > issues. > > > >> > > > > > > > > > 2. If you have two listeners,= one > > for > > > >> inter-broker > > > >> > > traffic > > > >> > > > > > > and > > > >> > > > > > > > > another > > > >> > > > > > > > > > for > > > >> > > > > > > > > > external clients, start metho= d is > > invoked > > > twice, > > > >> > > once for > > > >> > > > > > > each > > > >> > > > > > > > > > listener. On > > > >> > > > > > > > > > second thought, that may be > > confusing and > > > a > > > >> single > > > >> > > start() > > > >> > > > > > > > > invocation > > > >> > > > > > > > > > that > > > >> > > > > > > > > > provides all listener informa= tion > > and > > > returns > > > >> > > multiple > > > >> > > > > > > futures > > > >> > > > > > > > > would be > > > >> > > > > > > > > > better. Will update the KIP. > > > >> > > > > > > > > > 3. A typical example is a con= sumer > > > subscribing > > > >> to a > > > >> > > regex > > > >> > > > > > > > > pattern. We > > > >> > > > > > > > > > request all topic metadata fr= om 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 entrie= s at > > INFO > > > level in > > > >> > > > > > > > > SimpleAclAuthorizer. > > > >> > > > > > > > > > The > > > >> > > > > > > > > > proposal is to authorize this= > > request with > > > >> > > > > > > > > AuthorizationMode.FILTER, so > > > >> > > > > > > > > > that authorizers can log reso= urces > > 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 the= se > > cases. > > > Providing > > > >> > > > > > > authorization > > > >> > > > > > > > > mode to > > > >> > > > > > > > > > authorizers enables authorize= r > > > implementations > > > >> to > > > >> > > generate > > > >> > > > > > > > > better audit > > > >> > > > > > > > > > logs. > > > >> > > > > > > > > > 4. Each request may contain m= ultiple > > > instances > > > >> of > > > >> > > the same > > > >> > > > > > > > > authorizable > > > >> > > > > > > > > > resource. For example a produ= ce > > request > > > may > > > >> contain > > > >> > > records > > > >> > > > > > > for > > > >> > > > > > > > > 10 > > > >> > > > > > > > > > partitions of the same topic.= At the > > > moment, we > > > >> > > invoke > > > >> > > > > > > > authorize > > > >> > > > > > > > > > method 10 > > > >> > > > > > > > > > times. The proposal is to inv= oke it > > once > > > with > > > >> > > count=3D10. The > > > >> > > > > > > > > count is > > > >> > > > > > > > > > provided to authorizer just f= or > > audit > > > logging > > > >> > > purposes. > > > >> > > > > > > > > > 5. Authorizer implements Clos= eable, > > so you > > > >> could use > > > >> > > > > > close() > > > >> > > > > > > to > > > >> > > > > > > > > flush > > > >> > > > > > > > > > audits? > > > >> > > > > > > > > > > > > >> > > > > > > > > > On Thu, Aug 8, 2019 at 7:01 A= M Don > > Bosco > > > Durai < > > > >> > > > > > > > bosco@apache.org > > > >> > > > > > > > > > > > > >> > > > > > > > > > wrote: > > > >> > > > > > > > > > > > > >> > > > > > > > > > > Rajini > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Thanks for putting this tog= ether. > > It is > > > >> looking > > > >> > > good. I > > > >> > > > > > > have > > > >> > > > > > > > > few > > > >> > > > > > > > > > > questions... > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > 1. List > > > authorize(..., > > > >> > > List > > > >> > > > > > > > > actions). > > > >> > > > > > > > > > Do you > > > >> > > > > > > > > > > see a scenario where the br= oker > > will > > > call > > > >> > > authorize for > > > >> > > > > > > > > multiple > > > >> > > > > > > > > > topics at > > > >> > > > > > > > > > > the same time? I can unders= tand > > that > > > during > > > >> > > > > > > creating/deleting > > > >> > > > > > > > > ACLS, > > > >> > > > > > > > > > > multiple permissions for mu= ltiple > > > 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 a= s 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 > > 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" i= n > > 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 Audi= t > > writing by > > > >> caching > > > >> > > the logs > > > >> > > > > > > for > > > >> > > > > > > > a > > > >> > > > > > > > > fixed > > > >> > > > > > > > > > > interval. But when the Brok= er > > > terminates, we > > > >> do a > > > >> > > forced > > > >> > > > > > > > flush. > > > >> > > > > > > > > > Having an > > > >> > > > > > > > > > > explicit "stop" might give = us a > > formal > > > way to > > > >> > > flush our > > > >> > > > > > > > audits. > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Thanks > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Bosco > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > =EF=BB=BFOn 8/7/19, 3:59 PM= , "Rajini > > Sivaram" < > > > >> > > > > > > > rajinisivaram@gmail.com > > > >> > > > > > > > > > > > > >> > > > > > > > > > wrote: > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Hi Ron/Harsha/Satish, > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Thanks for reviewing th= e 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, wou= ld you > > like > > > to > > > >> start a > > > >> > > > > > > discussion > > > >> > > > > > > > > thread > > > >> > > > > > > > > > on dev? > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Regards, > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > Rajini > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > On Wed, Aug 7, 2019 at = 6:21 PM > > > Satish > > > >> Duggana < > > > >> > > > > > > > > > > satish.duggana@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 th= at. > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > 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 a= t 9:28 > > PM > > > Harsha > > > >> > > Chintalapani > > > >> > > > > > < > > > >> > > > > > > > > > kafka@harsha.io> > > > >> > > > > > > > > > > > wrote: > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > Thanks for the KIP = Rajini. > > > >> > > > > > > > > > > > > Quick thought, it w= ould > > be good > > > to > > > >> have a > > > >> > > common > > > >> > > > > > > > module > > > >> > > > > > > > > > outside of > > > >> > > > > > > > > > > > clients > > > >> > > > > > > > > > > > > that only applies t= o > > server side > > > >> > > interfaces & > > > >> > > > > > > > changes. > > > >> > > > > > > > > It > > > >> > > > > > > > > > looks > > > >> > > > > > > > > > > like we > > > >> > > > > > > > > > > > are > > > >> > > > > > > > > > > > > increasingly in fav= or of > > using > > > Java > > > >> > > interface for > > > >> > > > > > > > > pluggable > > > >> > > > > > > > > > > modules on > > > >> > > > > > > > > > > > the > > > >> > > > > > > > > > > > > broker side. > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > Thanks, > > > >> > > > > > > > > > > > > Harsha > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > On Tue, Aug 06, 201= 9 at > > 2:31 PM, > > > >> Rajini > > > >> > > Sivaram < > > > >> > > > > > > > > > > rajinisivaram@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 replaceme= nt 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 a= lso > > attempts > > > to > > > >> address > > > >> > > known > > > >> > > > > > > > > limitations > > > >> > > > > > > > > > in the > > > >> > > > > > > > > > > > > > authorizer. If yo= u 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 > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >