You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/06/01 03:11:25 UTC
Re: Review Request 34492: Patch for KAFKA-2210
-----------------------------------------------------------
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
>
>
Re: Review Request 34492: Patch for KAFKA-2210
Posted by Parth Brahmbhatt <br...@gmail.com>.
> On June 1, 2015, 1:11 a.m., Jun Rao wrote:
> > 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.
I will handle this as part of the CLI PR.
- Parth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34492/#review85791
-----------------------------------------------------------
On July 22, 2015, 12:08 a.m., Parth Brahmbhatt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34492/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 12:08 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2210
> https://issues.apache.org/jira/browse/KAFKA-2210
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressing review comments from Jun.
>
>
> Adding CREATE check for offset topic only if the topic does not exist already.
>
>
> Addressing some more comments.
>
>
> Removing acl.json file
>
>
> Moving PermissionType to trait instead of enum. Following the convention for defining constants.
>
>
> Adding authorizer.config.path back.
>
>
> Addressing more comments from Jun.
>
>
> Addressing more comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetRequest.scala f418868046f7c99aefdccd9956541a0cb72b1500
> 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.scala PRE-CREATION
> core/src/main/scala/kafka/security/auth/PermissionType.scala PRE-CREATION
> core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION
> core/src/main/scala/kafka/security/auth/ResourceType.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala 18f5b5b895af1469876b2223841fd90a2dd255e0
> core/src/main/scala/kafka/server/KafkaConfig.scala dbe170f87331f43e2dc30165080d2cb7dfe5fdbf
> core/src/main/scala/kafka/server/KafkaServer.scala 18917bc4464b9403b16d85d20c3fd4c24893d1d3
> 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/OperationTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/security/auth/PermissionTypeTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/security/auth/ResourceTypeTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 04a02e08a54139ee1a298c5354731bae009efef3
>
> Diff: https://reviews.apache.org/r/34492/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Parth Brahmbhatt
>
>