----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34493/#review96128 -----------------------------------------------------------
Thanks for the patch. A few comments below. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (lines 55 - 57) <https://reviews.apache.org/r/34493/#comment151362> 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. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (lines 66 - 68) <https://reviews.apache.org/r/34493/#comment151363> Perhaps this can be moved to ZkUtils.setupCommonPaths()? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (lines 70 - 72) <https://reviews.apache.org/r/34493/#comment151364> 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? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 76) <https://reviews.apache.org/r/34493/#comment151365> It doesn't seem that session, session.principal or session.host can be null. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 84) <https://reviews.apache.org/r/34493/#comment151367> Typically, we expect the logging to be a complete sentence, e.g., Authorize operation x in session y from principal z. Also, for auditing purpose, it may be useful to log all authroized and unauthorized operations. Perhaps we can log them in trace in a authroization logger. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 91) <https://reviews.apache.org/r/34493/#comment151368> Do we expect resource and its members to be null? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 97) <https://reviews.apache.org/r/34493/#comment151369> Is the null check necessary? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (lines 110 - 111) <https://reviews.apache.org/r/34493/#comment151371> 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. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 134) <https://reviews.apache.org/r/34493/#comment151370> Hmm, the acl should only match if operation is a subset of acl.operations, right? core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (lines 220 - 227) <https://reviews.apache.org/r/34493/#comment151373> 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. core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala (line 252) <https://reviews.apache.org/r/34493/#comment151372> Could we name this AclChangeListner? - Jun Rao 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 > >
