> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/main/scala/kafka/security/auth/Acl.scala, lines 57-62
> > <https://reviews.apache.org/r/34492/diff/1/?file=965651#file965651line57>
> >
> > I tried exactly that but it tunrs out our current Json parser does not
> > work when a json string has other special characters, somehow gets into
> > some double parsing and fails. Has been long since I wrote this code so
> > dont exactly remember why it was failing but I did try it and with current
> > JsonUtil it does not work.
Could you explain a bit which part doesn't work? The following simple test
works for me.
scala> val a = "[{\"a\": \"aa\"}]"
a: String = [{"a": "aa"}]
scala> JSON.parseFull(a)
res4: Option[Any] = Some(List(Map(a -> aa)))
> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 22
> > <https://reviews.apache.org/r/34492/diff/1/?file=965653#file965653line22>
> >
> > I haven't added Group support yet but they will be of the form
> > Group:<group-name>. Why did you get the impression that groups will not
> > have ":"
Oh, I was just saying that if the group name itself can contain ":", parsing
will be more difficult if ":" is the separator.
> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 41
> > <https://reviews.apache.org/r/34492/diff/1/?file=965653#file965653line41>
> >
> > Yes we can and as mentioned in the design doc when no authentication is
> > configured it will be set as User:DrWho?.
So, I guess authentication will always authenticate at the user level and it's
up to the Authorization model to implement the user to group mapping?
> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/main/scala/kafka/security/auth/Operation.java, line 22
> > <https://reviews.apache.org/r/34492/diff/1/?file=965654#file965654line22>
> >
> > I grepped through kafka code base to understand how enums were used in
> > other parts and all places used java enums. I assumed that was the
> > convention . If that is not the case I can change all enum classes in core
> > to use http://www.scala-lang.org/api/current/index.html#scala.Enumeration
Under core/, we don't have java files except when defining the java api. We
implement enum using case object in scala (see BrokerStates as an example).
> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/test/scala/unit/kafka/security/auth/AclTest.scala, line 36
> > <https://reviews.apache.org/r/34492/diff/1/?file=965662#file965662line36>
> >
> > can you elloborate why do you think that is a better approach?
I was thinking of just embedding the acl json string in the code.
> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote:
> > core/src/main/scala/kafka/security/auth/Authorizer.scala, line 36
> > <https://reviews.apache.org/r/34492/diff/1/?file=965652#file965652line36>
> >
> > In the KIP dicussion it was proposed to add a config
> > authoizer.config.path which will contain path to a property files on all
> > broker hosts. This is how the plugin specific property file gets passed on.
> > Do we want to instead use configurable?
Sorry, but I missed this in the KIP review. I think it's probably better NOT to
have another config.path inside a configuration file. We already have other
pluggable logic such as the MetrisReporter and will be adding other pluggable
logic such as PrincipalExtractor in KAFKA-1690. Introducing a separate config
path for each pluggable logic may not be ideal. Also, currently, we allow
people to instantiate KafkaServerStartble directly so that people can obtain
the properties from any configuration system and pass them to Kafka, instead of
assuming that the properties are always specified in a file. So, it's probably
better to specify the properties needed by any pluggable logic in the same
property file, then pass them to the pluggable logic through the configure()
api. We have KAFKA-2249 filed to allow KafkaConfig to do this. Perhaps, we can
fix KAFKA-2249 first.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34492/#review86267
-----------------------------------------------------------
On June 4, 2015, 11:07 p.m., Parth Brahmbhatt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34492/
> -----------------------------------------------------------
>
> (Updated June 4, 2015, 11:07 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.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetRequest.scala
> 3d483bc7518ad76f9548772522751afb4d046b78
> 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
> 6f25afd0e5df98258640252661dee271b1795111
> core/src/main/scala/kafka/server/KafkaServer.scala
> e66710d2368334ece66f70d55f57b3f888262620
> 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
> 71f48c07723e334e6489efab500a43fa93a52d0c
>
> Diff: https://reviews.apache.org/r/34492/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Parth Brahmbhatt
>
>
