-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34492/#review91836
-----------------------------------------------------------
Thanks for the patch. A few comments below.
Also,
1. For making user/group case-insensitive, could you start a discussion thread
on the dev mailing list to see if anyone has concerns on this?
2. Got the following compilation error. It seems that some files like Session
are missing.
/Users/junrao/intellij/kafka/core/src/main/scala/kafka/security/auth/Authorizer.scala:20:
value Session is not a member of object kafka.network.RequestChannel
import kafka.network.RequestChannel.Session
^
/Users/junrao/intellij/kafka/core/src/main/scala/kafka/security/auth/Authorizer.scala:44:
not found: type Session
def authorize(session: Session, operation: Operation, resource: Resource):
Boolean
core/src/main/scala/kafka/security/auth/Acl.scala (line 23)
<https://reviews.apache.org/r/34492/#comment145482>
Intead of using "user", it's probably better to reference
KafkaPrincipal.UserType.
core/src/main/scala/kafka/security/auth/Acl.scala (line 52)
<https://reviews.apache.org/r/34492/#comment145529>
principal should be principals.
Also, would it be better to represent each principal as {"type": "user",
"name": "alice"}?
core/src/main/scala/kafka/security/auth/Acl.scala (line 70)
<https://reviews.apache.org/r/34492/#comment145518>
aclMap.get(AclsKey).get can just be aclMap(AclsKey).
core/src/main/scala/kafka/security/auth/Acl.scala (line 80)
<https://reviews.apache.org/r/34492/#comment145531>
Is this needed? acls is already a set.
core/src/main/scala/kafka/security/auth/Acl.scala (lines 91 - 101)
<https://reviews.apache.org/r/34492/#comment145540>
Since during the authorization, we are already dealing with a set of Acls.
Would it be better to just model each Acl as a single KafkaPrincipal, with a
single permission, on a single host and single operation? Intuitively, an Acl
should probably also include a Resource.
This will make deduping Acls easier. In the current data structure. You can
have
acl1 = {principal1,principal2) ALLOW READ
acl2 = {principal1) ALLOW READ
They are not equal, but Acl2 is redundant given acl1. It's a bit involved
to determine that Acl2 is redundant. Instead, if you have
acl1 = principal1 ALLOW READ
acl2 = principal2 ALLOW READ
you can't add redundant acl2 again.
core/src/main/scala/kafka/security/auth/Acl.scala (line 101)
<https://reviews.apache.org/r/34492/#comment145533>
To be consistent, need space after comma. There are a few other places like
that. Could you address them all?
core/src/main/scala/kafka/security/auth/Acl.scala (line 120)
<https://reviews.apache.org/r/34492/#comment145534>
No need for the bracket on isInstanceOf.
core/src/main/scala/kafka/security/auth/Authorizer.scala (line 36)
<https://reviews.apache.org/r/34492/#comment145536>
It's better to make the Authorizer implement the Configurable interface.
core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 23)
<https://reviews.apache.org/r/34492/#comment145481>
core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 41)
<https://reviews.apache.org/r/34492/#comment145480>
Space after comma.
core/src/main/scala/kafka/security/auth/Operation.scala (lines 43 - 44)
<https://reviews.apache.org/r/34492/#comment145541>
Should we throw KafkaException to the caller if the input doesn't match any
operation?
core/src/main/scala/kafka/server/KafkaApis.scala (lines 101 - 103)
<https://reviews.apache.org/r/34492/#comment145543>
Our current coding convention is not to wrap single line statement with {}.
Also, in the message string, we should use the specific leaderAndIsrRequest.
Ditto below.
core/src/main/scala/kafka/server/KafkaApis.scala (line 207)
<https://reviews.apache.org/r/34492/#comment145544>
This is not introduced in this patch, but could you change map(_._1 to map
{case(topicAndPartition, _ ?
core/src/main/scala/kafka/server/KafkaApis.scala (line 624)
<https://reviews.apache.org/r/34492/#comment145545>
space after if.
core/src/main/scala/kafka/server/KafkaApis.scala (lines 653 - 658)
<https://reviews.apache.org/r/34492/#comment145546>
Hmm, since there is a single error code in JoinGroupResponse, we should
probably just return with an error and an empty partition list if at least one
of the topics is not authorized. Returning an error with a non-empty partition
list will make it hard for the clients to deal with.
core/src/main/scala/kafka/server/KafkaConfig.scala (line 170)
<https://reviews.apache.org/r/34492/#comment145535>
Now that KafkaConfig is an AbstractConfig, it's better to embed the
authorizer specific properties through KafkaConfig itself and make the
Authorizer a Configurable. See how
org.apache.kafka.clients.producer.partitioner is implemented and used through
the producer config.
- Jun Rao
On July 14, 2015, 9:13 p.m., Parth Brahmbhatt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34492/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 9:13 p.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.
>
>
> 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/ResourceTest.scala
> PRE-CREATION
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala
> 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/34492/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Parth Brahmbhatt
>
>
