> On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > 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
1. Ok, but based on my experience any discussion thread would mean we will end up holding up this PR for longer and KafkaAPI class keeps changing which makes upmerging to trunk a PITA. Can we go with case sensitive for now which is more restrictive and if the discussion thread comes back with case insensitive vote I can change the behavior at that point. 2. This change assumes https://issues.apache.org/jira/browse/KAFKA-1683 is committed and uses session object from that Patch. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 23 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line23> > > > > Intead of using "user", it's probably better to reference > > KafkaPrincipal.UserType. Done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 52 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line52> > > > > principal should be principals. > > > > Also, would it be better to represent each principal as {"type": > > "user", "name": "alice"}? fixed principal -> principals. I like it the way it is right now, easier on my eyes but dont mind changing it. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 80 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line80> > > > > Is this needed? acls is already a set. This is where scala is baffelling me right now and I need to read up. acls is collection.mutable.HashSet and it won't let me return that where a Set is expected so I must do a .toSet > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 91-101 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line91> > > > > 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. The problem with that approach is increased Admin pain when trying to grant same access to multiple principals. When an admin wants to add same permission to say 5 principals he will now have to issue the same command 5 times with only the principal param changing. I think if we want to avoid duplicate acls, the implementation might get a bit more complicated but it is still doable and it might be better to reduce the admin pain given the added complexity for de-duping is probably very small. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 101 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line101> > > > > To be consistent, need space after comma. There are a few other places > > like that. Could you address them all? Done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 120 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line120> > > > > No need for the bracket on isInstanceOf. done > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 41 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011773#file1011773line41> > > > > Space after comma. done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 104-106 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011778#file1011778line104> > > > > 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. Done, for all 4 occurrences. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 628 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011778#file1011778line628> > > > > space after if. Done, fixed this in few other classes. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 657-662 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011778#file1011778line657> > > > > 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. Done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 70 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011771#file1011771line70> > > > > aclMap.get(AclsKey).get can just be aclMap(AclsKey). done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, lines 43-44 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011774#file1011774line43> > > > > Should we throw KafkaException to the caller if the input doesn't match > > any operation? Done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, line 170 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011779#file1011779line170> > > > > 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. Done. > On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Authorizer.scala, line 36 > > <https://reviews.apache.org/r/34492/diff/7/?file=1011772#file1011772line36> > > > > It's better to make the Authorizer implement the Configurable interface. Done. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91836 ----------------------------------------------------------- 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 > >
