----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review85791 -----------------------------------------------------------
Thanks for that patch. A few comments below. Also, two common types of users are consumers and publishers. Currently, if you want to allow a user to consume from topic t in consumer group g, you have to grant (1) read permission on topic t; (2) read permission on group g; (3) describe permission on topic t; (4) describe permission on group g; (5) create permission on offset topic; (6) describe permission on offset topic. Similarly, if you want to allow a user to publish to a topic t, you need to grant (1) write permission to topic t; (2) create permission on the cluster; (3) describe permission on topic t. These are a quite a few individual permission for the admin to remember and set. I am wondering if we can grant permissions to these two types of users in a simpler way, at least through the cli. For example, for a consumer, based on the topics and the consumer group, it would be nice if the cli can grant the necessary permissions automatically, instead of having to require the admin to set each indivial permission. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137600> Could we document the ZK format in the comment. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137598> Currently, the convention for constants in our scala code is CamelCase (see ZkUtils). core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137603> aclMap.get(key).get can just be aclMap(key). core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137613> Instead of this, would it be better to model Acls as a class? Then, we just need Acls.fromJson and Acls.toJson. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137609> Would it be better to use a case class here so that we don't have to override equals and hashcode, and perhaps toString()? core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment137628> Hmm, not sure why we can't just implement a toJson directly. The json library is only used for decode, but not encode, right? core/src/main/scala/kafka/security/auth/Authorizer.scala <https://reviews.apache.org/r/34492/#comment137634> I am not sure if it's enough to just take KafkaConfig. KafkaConfig will only contain pre-defined properties. However, the plugin module may need some plugin specific properties that haven't been predefined. It seems that we need to take in the original properties as what we do in the client (see Configurable). core/src/main/scala/kafka/security/auth/Authorizer.scala <https://reviews.apache.org/r/34492/#comment137637> Since this is just for authorization, perhaps the semantic should be removing all acls associated with a resource? The removing of the resource itself should be done somewhere else. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment137567> So groups typically don't include ':' ? core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment137565> Since userType is public constant, we want to capitalize U. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment137564> Perhaps Harsh can comment on this. Are we able to populate the principalType through an ssl or sasl connection? core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment137568> Should we test for null? Also, is principal name always case insensitive? core/src/main/scala/kafka/security/auth/Operation.java <https://reviews.apache.org/r/34492/#comment137594> Currently, core is completely scala. Is there a particular reason that this and a few other new classes in core are in java? core/src/main/scala/kafka/security/auth/Resource.scala <https://reviews.apache.org/r/34492/#comment137576> Capitalize c and s. core/src/main/scala/kafka/security/auth/Resource.scala <https://reviews.apache.org/r/34492/#comment137580> Capitalize expected and allowed. Space before allowed. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137738> Instead if generating the response here directly, it's probably simpler to just throw an AuthroizationException. The caller already has a generic way of generating the response on exceptions. We can do this for all requests of CLUSTER_ACTION. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137739> Not sure why we need to do this for each partition. It seems that doing the authorization check once for the whole request is enough. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137737> The resource should be consumer group. So, either the whole request will be authorized or not. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137687> It would be good to avoid using ._1 since it's not clear what kind of data it is. To make it clear, we could do partition { case (TopicAndPartition, _) => ... } core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137740> space btw the two parameters passed into Resource. There are a few other places like that. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137742> Using Nil here is correct. However, the usage in OffsetRequest line 117 is incorrect. Instead of passing in null, we should pass in Nil. This is not introduced in the patch, but could you fix it in this patch to make the usage consistent? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137749> We should also check the user has read permission on the consumer group, right? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment137743> Could you do what's described in TODO? Also, it seems that we should check for the read permission on the consumer group. The KIP wiki needs to be updatd as well. core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/34492/#comment137736> This doesn't seem to be in the KIP wiki. Could you explain a bit how it will be used? core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/34492/#comment137735> We probably shouldn't introduce a separate config path for authorization. Instead of, it's simpler if authorization related properties can be specified in the same broker property file. So, we probably should do this in the same way as we do in KafkaProducer. Bascially, we pass a key/value property map to KafkaConfig. We then configure the authorizer instance by pass in the original property map. We can make "authorizer.class.name" of type class and make it implement Configurable. See ProducerConfig.METRIC_REPORTER_CLASSES_CONFIG as an example. This will also allow people to pass in the configure to Kafka server directly, instead of always assuming the configs have to be read from a file. core/src/test/scala/unit/kafka/security/auth/AclTest.scala <https://reviews.apache.org/r/34492/#comment137745> Instead of using acl.json, could we just use a local string to represent the acl value stored in ZK? core/src/test/scala/unit/kafka/security/auth/AclTest.scala <https://reviews.apache.org/r/34492/#comment137746> This is probably not needed if we turn ACL to a case class. core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala <https://reviews.apache.org/r/34492/#comment137748> Hmm, interesting. Currently, topic name is actually case sensitive. - Jun Rao On May 20, 2015, 8:02 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated May 20, 2015, 8:02 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2210 > https://issues.apache.org/jira/browse/KAFKA-2210 > > > Repository: kafka > > > Description > ------- > > KAFKA-2210: Kafka authorizer public entities and changes to KafkaAPI and > KafkaServer to allow custom authorizer implementation. > > > Diffs > ----- > > core/src/main/scala/kafka/common/AuthorizationException.scala PRE-CREATION > core/src/main/scala/kafka/common/ErrorMapping.scala > c75c68589681b2c9d6eba2b440ce5e58cddf6370 > core/src/main/scala/kafka/security/auth/Acl.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Authorizer.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Operation.java PRE-CREATION > core/src/main/scala/kafka/security/auth/PermissionType.java PRE-CREATION > core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/ResourceType.java PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 387e387998fc3a6c9cb585dab02b5f77b0381fbf > core/src/main/scala/kafka/server/KafkaConfig.scala > 9efa15ca5567b295ab412ee9eea7c03eb4cdc18b > core/src/main/scala/kafka/server/KafkaServer.scala > ea6d165d8e5c3146d2c65e8ad1a513308334bf6f > core/src/test/resources/acl.json PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/AclTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/KafkaPrincipalTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 8014a5a6c362785539f24eb03d77278434614fe6 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
