You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/04/30 13:33:26 UTC

[GitHub] [kafka] viktorsomogyi opened a new pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

viktorsomogyi opened a new pull request #8591:
URL: https://github.com/apache/kafka/pull/8591


    PR #4303 introduced parsing for some invalid JSONs, such as escaped SSL names. This PR moves the parsing directly under AclEntry to localize this logic.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r418741006



##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclEntryTest.scala
##########
@@ -23,27 +23,28 @@ import org.apache.kafka.common.acl.AclOperation.READ
 import org.apache.kafka.common.acl.AclPermissionType.{ALLOW, DENY}
 import org.apache.kafka.common.security.auth.KafkaPrincipal
 import org.junit.{Assert, Test}
-import org.scalatestplus.junit.JUnitSuite
 
-import scala.jdk.CollectionConverters._
+import scala.collection.JavaConverters._

Review comment:
       Need to revert this. It is causing the build to fail.

##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       I'm probably missing something obvious, but I found the following callers of this function: `DeleteRecordsCommand`, `LeaderElectionCommand`, `PreferredReplicaLeaderElectionCommand`, and `ReassignPartitionsCommand`. As far as I can tell, none of these uses involve the parsing of ACLs. Did we lose this compatibility handling at some point or is there some magic invocation?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420878993



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       Ok, that's fair. I've uploaded the changes.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi removed a comment on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi removed a comment on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-621919693


   ok to test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r418778470



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       Discussed this offline with @hachikuji. I had misunderstood his point. It looks like the compatibility code has not been used since 1.1.0:
   
   https://github.com/apache/kafka/commit/9b44c3e7f89b55528b12ba280007c804682beb0b#diff-81f2ec590f4e047be7e3a7e5267cbc9aR60




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420802979



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       @viktorsomogyi I think the argument is that if no-one complained since 1.1 which was released more than 2 years ago, then maybe this issue is very rare.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-625251354


   Thank you guys for the review!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420040567



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       I looks like `parseFull` is used in a couple of places but I can perhaps just delegate this to `parseBytes` like this:
   ```
   def parseFull(input: String): Option[JsonValue] = parseBytes(input.getBytes(Charset.defaultCharset()))
   ```
   Or shall I just get rid of this code and use `parseBytes` everywhere?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420040567



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       I looks like `parseFull` is used in a couple of places as Json mentioned but I can perhaps just delegate this to `parseBytes` like this below and can continue using that in the mentioned callers.
   ```
   def parseFull(input: String): Option[JsonValue] = parseBytes(input.getBytes(Charset.defaultCharset()))
   ```
   Or shall I just get rid of this code and use `parseBytes` everywhere?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-621875694


   ok to test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-622565563


   retest this please


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-621919693


   ok to test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r418766141



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       The compatibility fix was only required for ACLs, but we added it here to keep the fix small for backporting. The idea was to move to the appropriate place soon after, but it got stuck for a while. Do you have some concerns?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420124856



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       This code is fine as it is. We are suggesting that we don't need `parseBytesWithAclFallback` in `AclEntry`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-621919298


   Removed `TopicCount`. That was probably a leftover from the rebase, I guess the merge tool didn't delete it correctly or something.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-622252761


   retest this please


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-624765632






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] viktorsomogyi commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
viktorsomogyi commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r420753993



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       Ok, then I misunderstood it too. :)
   However isn't it possible that this issue resurfaces again if we remove this? I suspect the user who raised it may still use 1.0 where it got fixed and when they upgrade they'd run into this again. Or am I missing something else too?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r418778603



##########
File path: core/src/main/scala/kafka/utils/Json.scala
##########
@@ -35,16 +35,7 @@ object Json {
    */
   def parseFull(input: String): Option[JsonValue] =

Review comment:
       Maybe we should just drop this code.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#discussion_r418035530



##########
File path: core/src/main/scala/kafka/security/authorizer/AclEntry.scala
##########
@@ -92,6 +95,23 @@ object AclEntry {
     }.getOrElse(Set.empty)
   }
 
+  /**
+   * Parse a JSON string into a JsonValue if possible. `None` is returned if `input` is not valid JSON. This method is currently used
+   * to read the already stored invalid ACLs JSON which was persisted using older versions of Kafka (prior to Kafka 1.1.0). KAFKA-6319
+   */
+  private def parseBytesWithAclFallback(input: Array[Byte]): Option[JsonValue] = {
+    // Before 1.0.1, Json#encode did not escape backslash or any other special characters. SSL principals
+    // stored in ACLs may contain backslash as an escape char, making the JSON generated in earlier versions invalid.
+    // Escape backslash and retry to handle these strings which may have been persisted in ZK.
+    // Note that this does not handle all special characters (e.g. non-escaped double quotes are not supported)
+    Json.tryParseBytes(input) match {
+      case Left(e) =>

Review comment:
       Nit: `e` -> `_`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #8591: KAFKA-6342: Move workaround for JSON parsing of non-escaped strings

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8591:
URL: https://github.com/apache/kafka/pull/8591#issuecomment-622246550


   ok to test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org