> 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.
>
> Jun Rao wrote:
> 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)))
So I tried this again and found out the issue. Nothing to do with double
parsing ( I was probably mixing some other project here) but our current
JSON.parseFull does not support custom objects. it only supports primitive
types , lists, maps and iterables. It throws IllegalArg if I try to pass the
Acl object as is because of following lines.
case other: AnyRef => throw new IllegalArgumentException("Unknown arguement of
type " + other.getClass + ": " + other)
> 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?
>
> Jun Rao wrote:
> 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.
can we file another jira so this one is not blocked on KAFKA-2249? Or you feel
as this is a config change we are better off just waiting for KAFKA-2249.
> 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 ":"
>
> Jun Rao wrote:
> Oh, I was just saying that if the group name itself can contain ":",
> parsing will be more difficult if ":" is the separator.
Fixed, users and groups can have ":" in them, also added a unit test to verify
the same.
> 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?.
>
> Jun Rao wrote:
> 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?
The design it self does not assume that so the authentication layer could
authenticate with some other principalType. My current implementation of
authorizer makes this assumption (I think) but that is easy to fix and its part
of another review request.
> 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
>
> Jun Rao wrote:
> 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).
Done.
> 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?
>
> Jun Rao wrote:
> I was thinking of just embedding the acl json string in the code.
Done. Don't really like the ugliness.
- Parth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34492/#review86267
-----------------------------------------------------------
On July 10, 2015, 1 a.m., Parth Brahmbhatt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34492/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 1 a.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
>
>
> 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.java 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
> c1f0ccad4900d74e41936fae4c6c2235fe9314fe
> 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
>
>
