> On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, lines > > 55-57 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line55> > > > > To be consistent with how we pass in configs for pluggable components, > > we need to get the external properties directly through > > KafkaConfig.originals() instead of from an external property file.
Again, all this code is too old. We now have Authorizer extending configurable so we dont get KafkaConfig instance here. We actually get the original properties map and we initiliaze the variables from that map. We dont expect a new properties file anymore. > On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, lines > > 66-68 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line66> > > > > Perhaps this can be moved to ZkUtils.setupCommonPaths()? This path is kind of optional as we only need it if someone wants to use authorizer + if they want to use our authorizer. I think creating this path as part of commonPaths will be confusing. > On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, lines > > 70-72 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line70> > > > > Hmm, not sure why we need the scheduler. It seems that it's enough to > > just read every acl from ZK first on initialization. That's how > > topic/client config changes works. What's the re-connection issue that you > > mentioned? Please read http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html section "Things to Remember about Watches". Bottomline is there are pathological cases where a watcher can get missed. All I really need is set a TTL On cache entry, I could add that or keep the scheduler so it expires all cache entries every hour. > On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, lines > > 110-111 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line110> > > > > Is this right? It seems the expansion should happen on the acl side. > > Basically, if you configure an ACL to read from a topic, you automatically > > get an ACL to describe the topic. I am not sure what you mean by "the expansion should happen on the acl side". Do you mean we should add the describe acl when someone adds an acl for read or write? That does not seem right because in that case when they remove the READ or WRITE acl, do we also remove describe acl? or removal will have to be explicit? Seems confusing. I think the current way is cleaner. > On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, line 134 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line134> > > > > Hmm, the acl should only match if operation is a subset of > > acl.operations, right? again code has changed quite a bit given acl only stores one operation now. The original code was doing the right thing as the list only had > 1 element in describe case and in that case the match method was checking if the acl.oprations had atleast one of the 3 DESCRIBE,READ or WRITE. > On Aug. 23, 2015, 9:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala, lines > > 220-227 > > <https://reviews.apache.org/r/34493/diff/1/?file=965666#file965666line220> > > > > Not sure why we need to read from ZK during reads since writes should > > only be propagated from the ZK listner. You can take a look at how > > ConfigChangeListener is implemented. Again my understanding of how zookeeper watchers work with zkClient is that it is possible to miss watchers so I don't completely rely on the watchers always firing. If that understading is incorrect i can remove the scheduler thread and change the code to only read from zookeeper as part of handling the watcher notification. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34493/#review96128 ----------------------------------------------------------- On May 20, 2015, 8:03 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34493/ > ----------------------------------------------------------- > > (Updated May 20, 2015, 8:03 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2211 > https://issues.apache.org/jira/browse/KAFKA-2211 > > > Repository: kafka > > > Description > ------- > > KAFKA-2211: Out of box implementation for authorizer interface. > > > Diffs > ----- > > core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala > PRE-CREATION > core/src/test/resources/authorizer-config.properties PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/SimpleAclAuthorizerTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/34493/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
