----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review96909 -----------------------------------------------------------
Thanks for the patch. We are getting pretty close. A few more comments below. core/src/main/scala/kafka/common/ErrorMapping.scala (line 56) <https://reviews.apache.org/r/34492/#comment152871> We will need to add the new error code and the exception in org.apache.kafka.common.errors in the client as well. Currently, there are a few error codes in Errors that are missing in ErrorMapping. This will be fixed in a separate jira. In this jira, we can just start with error code 29, which is the next unused error code in Errors. The exception in the client should be of ApiException. core/src/main/scala/kafka/security/auth/Acl.scala (lines 48 - 51) <https://reviews.apache.org/r/34492/#comment152878> DENY, READ, WRITE should probably be camel case? core/src/main/scala/kafka/security/auth/Acl.scala (line 53) <https://reviews.apache.org/r/34492/#comment152595> principal => principals core/src/main/scala/kafka/security/auth/Acl.scala (line 99) <https://reviews.apache.org/r/34492/#comment152854> I had a comment on this earlier that the current ACL representation makes it a bit hard to do dedup. Also, this makes it a bit inconvenient to remove ACLs. Basically, you have to specify the exact set of principals, hosts and operations to remove an existing ACL. Your reply is that this makes it convenient when an admin wants to add the same permission to say 5 principals. I was thinking that perhaps we can separate the CLI options from the underlying ACL representation. For the ACL representation, it seems that it's more natural to represent each ACL as either a <principal, permissionType, operation> or a <host, permissionType, operation> tuple. This will allow ACLs to be represented uniquely and makes dedupping easy. On the CLI side, we can take batch options for convenience, but translate them to a set of unique ACLs. - Jun Rao On Aug. 26, 2015, 9:29 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2015, 9:29 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. > > > Addressing more comments from Jun. > > > Addressing more comments. > > > Now addressing Ismael's comments. Case sensitive checks. > > > Addressing Jun's comments. > > > Merge remote-tracking branch 'origin/trunk' into az > > Conflicts: > core/src/main/scala/kafka/server/KafkaApis.scala > core/src/main/scala/kafka/server/KafkaServer.scala > > Deleting KafkaConfigDefTest > > > Addressing comments from Ismael. > > > Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into az > > > Consolidating KafkaPrincipal. > > > Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into az > > Conflicts: > > clients/src/main/java/org/apache/kafka/common/network/PlaintextTransportLayer.java > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > core/src/main/scala/kafka/server/KafkaApis.scala > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/common/network/PlaintextTransportLayer.java > 35d41685dd178bbdf77b2476e03ad51fc4adcbb6 > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > b640ea0f4bdb694fc5524ef594aa125cc1ba4cf3 > > clients/src/test/java/org/apache/kafka/common/security/auth/KafkaPrincipalTest.java > PRE-CREATION > 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/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 > a3a8df0545c3f9390e0e04b8d2fab0134f5fd019 > core/src/main/scala/kafka/server/KafkaConfig.scala > d547a01cf7098f216a3775e1e1901c5794e1b24c > core/src/main/scala/kafka/server/KafkaServer.scala > 17db4fa3c3a146f03a35dd7671ad1b06d122bb59 > core/src/test/scala/unit/kafka/security/auth/AclTest.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/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 3da666f73227fc7ef7093e3790546344065f6825 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
