KAFKA-6342: Move workaround for JSON parsing of non-escaped strings by viktorsomogyi · Pull Request #8591 · apache/kafka

viktorsomogyi · 2020-04-30T13:33:18Z

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)

ijuma

Thanks, just a nit below. Also, the diff is showing TopicCount with no changes. Do you know what that's about?

ijuma · 2020-04-30T14:07:46Z

ok to test

viktorsomogyi · 2020-04-30T15:16:14Z

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

ijuma · 2020-05-01T04:48:10Z

ok to test

ijuma · 2020-05-01T05:17:03Z

retest this please

hachikuji · 2020-05-01T20:58:12Z

retest this please

hachikuji · 2020-05-01T21:21:53Z

@@ -35,16 +35,7 @@ object Json {
   */
  def parseFull(input: String): Option[JsonValue] =


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?

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?

Discussed this offline with @hachikuji. I had misunderstood his point. It looks like the compatibility code has not been used since 1.1.0:

9b44c3e#diff-81f2ec590f4e047be7e3a7e5267cbc9aR60

Maybe we should just drop this code.

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 code is fine as it is. We are suggesting that we don't need parseBytesWithAclFallback in AclEntry.

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?

@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.

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

hachikuji · 2020-05-06T16:53:13Z

retest this please

hachikuji · 2020-05-06T16:53:23Z

retest this please

hachikuji · 2020-05-06T16:53:31Z

retest this please

hachikuji

LGTM (assuming tests are passing)

ijuma

LGTM too.

viktorsomogyi · 2020-05-07T13:20:32Z

Thank you guys for the review!

@test

* 'trunk' of github.com:apache/kafka: (87 commits) KAFKA-9865: Expose output topic names from TopologyTestDriver (apache#8483) MINOR - Increase the number of Trogdor Histogram buckets to 10000 (apache#8627) KAFKA-9768: Fix handling of rest.advertised.listener config (apache#8360) KAFKA-9419: Fix possible integer overflow in CircularIterator (apache#7950) MINOR: Only add 'Data' suffix for generated request/response/header types (apache#8625) KAFKA-9947; Ensure proper shutdown of services in `TransactionsBounceTest` (apache#8602) KAFKA-6342; Remove unused workaround for JSON parsing of non-escaped strings (apache#8591) MINOR: Pass `-release 8` to scalac and upgrade to Gradle 6.4 (apache#8538) KAFKA-9946; Partition deletion event should only be sent if deletion was requested in the StopReplica request (apache#8609) MINOR: Improve TopologyTestDriver JavaDocs (apache#8619) MINOR: MockAdminClient should return InvalidReplicationFactorException if brokers.size < replicationFactor KAFKA-9748: Add Streams eos-beta integration test (apache#8496) KAFKA-9731: Disable immediate fetch response for hw propagation if replica selector is not defined (apache#8607) HOTFIX: set correct numIterations in shouldAllowConcurrentAccesses MINOR: Clean up some test dependencies on ConfigCommand and TopicCommand (apache#8527) KAFKA-9918; SslEngineFactory is NOT closed when channel is closing (apache#8551) KAFKA-9798: Send one round synchronously before starting the async producer (apache#8565) MINOR: Annotate KafkaAdminClientTest.testAlterClientQuotas() with @test KAFKA-9589: Enable testLogAppendTimeNonCompressedV2 and fix bug in helper method (apache#8533) MINOR: Use min/max function when possible (apache#8577) ... # Conflicts: # core/src/main/scala/kafka/log/Log.scala # gradle/dependencies.gradle # gradle/wrapper/gradle-wrapper.properties # gradlew

…strings (apache#8591) Previously we had fallback logic when parsing ACLs to handle older entries which may contain non-escaped characters. This code became dead after 1.1 since it was no longer used in the parsing of ACLs. This patch removes the fallback logic. Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>

umesh9794 and others added 8 commits April 3, 2020 12:37

Initial Commit

e7c4346

Fixed the applicability of new method based on trunk

5b48c95

Merge branch 'trunk'

752d831

Rebased and applied review comments

1e941b9

Rebased and applied review comments

a14e6cb

Changed the AclJson to have the incorrect format for principals

ae659d1

Applied the review comments

1c308a8

Rebase changes + fixing test

438d010

viktorsomogyi mentioned this pull request Apr 30, 2020

KAFKA-6342 : Move workaround for JSON parsing of non-escaped strings #4321

Closed

3 tasks

ijuma reviewed Apr 30, 2020

View reviewed changes

Comment thread core/src/main/scala/kafka/security/authorizer/AclEntry.scala Outdated

Address review comments

fec67e0

hachikuji reviewed May 1, 2020

View reviewed changes

viktorsomogyi added 4 commits May 5, 2020 13:33

Use jdk.CollectionConverters

755634f

Get rid of parseBytesWithAclFallback

b9c70ff

Optimize imports

bdfa380

Revert AclEntry import changes

802e255

hachikuji approved these changes May 6, 2020

View reviewed changes

ijuma approved these changes May 6, 2020

View reviewed changes

hachikuji merged commit d70dacb into apache:trunk May 6, 2020

viktorsomogyi deleted the KAFKA-6342 branch May 7, 2020 13:22

		@@ -35,16 +35,7 @@ object Json {
		*/
		def parseFull(input: String): Option[JsonValue] =

Uh oh!

Conversation

viktorsomogyi commented Apr 30, 2020

Committer Checklist (excluded from commit message)

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ijuma commented Apr 30, 2020

Uh oh!

viktorsomogyi commented Apr 30, 2020

Uh oh!

ijuma commented May 1, 2020

Uh oh!

ijuma commented May 1, 2020

Uh oh!

hachikuji commented May 1, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktorsomogyi May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji commented May 6, 2020

Uh oh!

hachikuji commented May 6, 2020

Uh oh!

hachikuji commented May 6, 2020

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

viktorsomogyi commented May 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viktorsomogyi May 5, 2020 •

edited

Loading