> On Aug. 31, 2015, 6:47 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 99 > > <https://reviews.apache.org/r/34492/diff/13/?file=1055322#file1055322line99> > > > > 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.
Changed. > On Aug. 31, 2015, 6:47 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 53 > > <https://reviews.apache.org/r/34492/diff/13/?file=1055322#file1055322line53> > > > > principal => principals fixed. > On Aug. 31, 2015, 6:47 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 48-51 > > <https://reviews.apache.org/r/34492/diff/13/?file=1055322#file1055322line48> > > > > DENY, READ, WRITE should probably be camel case? fixed. > On Aug. 31, 2015, 6:47 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/common/ErrorMapping.scala, line 57 > > <https://reviews.apache.org/r/34492/diff/13/?file=1055321#file1055321line57> > > > > 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. fixed. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review96909 ----------------------------------------------------------- On Sept. 1, 2015, 10:36 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 10:36 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 > > Making Acl structure take only one principal, operation and host. > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/common/network/PlaintextTransportLayer.java > 35d41685dd178bbdf77b2476e03ad51fc4adcbb6 > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > e17e390c507eca0eba28a2763c0e35d66077d1f2 > > 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 > >
