KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads. by akhileshchg · Pull Request #12628 · apache/kafka

akhileshchg · 2022-09-12T22:59:42Z

KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads.

The issue with StandardAuthorizer#authorize is, that it looks up
aclsByResources (which is of type ConcurrentSkipListMap)twice for every
authorize call and uses Iterator with weak consistency guarantees on top of
aclsByResources. This can cause the authorize function call to process the
concurrent writes out of order.

Implemented ReadWrite lock at StandardAuthorizer level to make sure the reads
are strongly consistent with write order.

…istent ACL reads. The issue with StandardAuthorizer#authorize is, that it looks up aclsByResources (which is of type ConcurrentSkipListMap)twice for every authorize call and uses Iterator with weak consistency guarantees on top of aclsByResources. This can cause the authorize function call to process the concurrent writes out of order. Implemented ReadWrite lock at StandardAuthorizer level to make sure the reads are strongly consistent with write order.

mumrah

Thanks for the patch @akhileshchg! Left a question inline.

As mentioned offline, it would be good to see the performance difference between the concurrent data structures and this PR. I expect this change will be slower in the case where we take a copy of the bindings in the iterator.

mumrah · 2022-09-12T23:13:15Z


        AclIterable(AclBindingFilter filter) {
-            this.filter = filter;
+            this.aclBindingList = aclsByResource


Just to clarify my understanding.

Previously, we were wrapping the aclsByResource iterator. This was intended to be thread-safe, but as you mentioned offline, there was no guard against the underlying map getting modified during iteration (since the ConcurrentSkipListMap might show updated elements to the iterator).

Instead, we are now making a copy of the matching AclBinding and returning that's lists iterator the caller. So, basically trading off memory for consistency.

Yes, you're right. I think there is no other way we can guarantee the consistency here other than giving an Iterable that stays constant to the client accessing Authorizer#acls.

akhileshchg · 2022-09-12T23:54:33Z

AclAuthorizerBenchmark.testAclsIterator                     50               ACL                20           200000  avgt        730.070          ms/op
AclAuthorizerBenchmark.testAuthorizeByResourceType          50               ACL                20           200000  avgt          0.010          ms/op
AclAuthorizerBenchmark.testAuthorizer                       50               ACL                20           200000  avgt          4.505          ms/op
AclAuthorizerBenchmark.testUpdateCache                      50               ACL                20           200000  avgt       1936.356          ms/op

Benchmark                                           (aclCount)  (authorizerType)  (denyPercentage)  (resourceCount)  Mode  Cnt     Score   Error  Units
AclAuthorizerBenchmark.testAclsIterator                     50             KRAFT                20           200000  avgt       2084.634          ms/op
AclAuthorizerBenchmark.testAuthorizeByResourceType          50             KRAFT                20           200000  avgt       6180.318          ms/op
AclAuthorizerBenchmark.testAuthorizer                       50             KRAFT                20           200000  avgt          2.768          ms/op
AclAuthorizerBenchmark.testUpdateCache                      50             KRAFT                20           200000  avgt         ≈ 10⁻⁶          ms/op

NOTE: authorizeByResourceType is not implemented in StandardAuthorizer, so it uses the default implementation in Authorizer, hence it is not in the same ballpark as AclAuthorizer. Similarly updateCache is not implemented for StandardAuthorizer (we use AclMutator, so we cannot compare the numbers).

With the new implementation, StandardAuthorizer seems to be doing worse on the AclsIterator benchmark than AclAuthorizer and doing better in testAuthorizer which calls Authorizer#authorize.

I updated the iterator method to only loop once through acls and the performance is in the same ballpark as AclAuthorizer.

Benchmark                                           (aclCount)  (authorizerType)  (denyPercentage)  (resourceCount)  Mode  Cnt     Score   Error  Units
AclAuthorizerBenchmark.testAclsIterator          50             KRAFT                20           200000  avgt       833.482          ms/op

hachikuji

I posted some comments on the old PR: #12627 (review). Could you take a look since it looks like they are still relevant. Also, I think we should come to some agreement regarding this comment:

I do wonder about the tradeoffs between the approach used here and what we do in AclAuthorizer. The advantage of the latter approach is that reads are never blocked. Since the authorizer needs to be accessed on every request, that is a tough advantage to overlook. On the other hand, to make it work, it looks like we would need to add batching methods for adding new ACLs since doing a copy for every ACL is probably a non-starter.

akhileshchg · 2022-09-19T23:27:37Z

Reopened the PR. Moved the ReadWrite lock from StandardAuthorizerData scope to StandardAuthorizer. I'm okay with most methods after moving the lock, but loadSnapshot can be pretty heavy with this implementation. Let me know your thoughts @mumrah @cmccabe @hachikuji

cmccabe · 2022-09-20T00:00:48Z

Thanks, @akhileshchg . Can you remove the AclAuthorizerBenchmark change from here? I can post a separate PR for this (which we don't need in 3.3...)

akhileshchg · 2022-09-20T02:32:54Z

I reverted the benchmark changes.

showuon · 2022-09-20T03:20:30Z

Reopened the PR. Moved the ReadWrite lock from StandardAuthorizerData scope to StandardAuthorizer. I'm okay with most methods after moving the lock, but loadSnapshot can be pretty heavy with this implementation. Let me know your thoughts @mumrah @cmccabe @hachikuji

The loadSnapshot is indeed a concern. I'm wondering if we can remove lock for loadSnapshot method? After all, the authorizer won't be up and ready to use before loadSnapshot completed, right? Or maybe in that case, we don't even worry about adding/removing the lock in loadSnapshot since there won't be other threads waiting for the lock

akhileshchg · 2022-09-20T05:06:26Z

Reopened the PR. Moved the ReadWrite lock from StandardAuthorizerData scope to StandardAuthorizer. I'm okay with most methods after moving the lock, but loadSnapshot can be pretty heavy with this implementation. Let me know your thoughts @mumrah @cmccabe @hachikuji

Reduced the critical section for loadSnapshot. The change is ready for review. @mumrah @cmccabe @hachikuji

mumrah

@showuon I think we need the lock in loadSnapshot since it will get called any time the broker/controller makes a snapshot. With @akhileshchg latest changes, there should be a lot less contention in this method (basically just when the new StandardAuthorizerData is create and the volatile is written).

@akhileshchg thanks for picking this back up :) I left a few minor comments inline

mumrah

Thanks for the updates, LGTM!

…istent ACL reads. (#12628) Fixes an issue with StandardAuthorizer#authorize that allowed inconsistent results. The underlying concurrent data structure (ConcurrentSkipListMap) had weak consistency guarantees. This meant that a concurrent update to the authorizer data could result in the authorize function processing ACL updates out of order. This patch replaces the concurrent data structures with regular non-thread safe equivalents and uses a read/write lock for thread safety and strong consistency. Reviewers: David Arthur <mumrah@gmail.com>, Jason Gustafson <jason@confluent.io>, Colin P. McCabe <cmccabe@apache.org>, Luke Chen <showuon@gmail.com>

…eptember 2022) `Jenkinsfile` was the only conflict and we ignore the changes since they are not relevant to the Confluent build. * apache-github/3.3: (61 commits) KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads. (apache#12628) KAFKA-14243: Temporarily disable unsafe downgrade (apache#12664) KAFKA-14240; Validate kraft snapshot state on startup (apache#12653) KAFKA-14233: disable testReloadUpdatedFilesWithoutConfigChange first to fix the build (apache#12658) KAFKA-14238; KRaft metadata log should not delete segment past the latest snapshot (apache#12655) KAFKA-14156: Built-in partitioner may create suboptimal batches (apache#12570) MINOR: Adds KRaft versions of most streams system tests (apache#12458) MINOR; Add missing li end tag (apache#12640) MINOR: Mention that kraft is production ready in upgrade notes (apache#12635) MINOR: Add upgrade note regarding the Strictly Uniform Sticky Partitioner (KIP-794) (apache#12630) KAFKA-14222; KRaft's memory pool should always allocate a buffer (apache#12625) KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits (apache#12626) KAFKA-14196; Do not continue fetching partitions awaiting auto-commit prior to revocation (apache#12603) KAFKA-14215; Ensure forwarded requests are applied to broker request quota (apache#12624) MINOR; Remove end html tag from upgrade (apache#12605) Remove the html end tag from upgrade.html KAFKA-14205; Document how to replace the disk for the KRaft Controller (apache#12597) KAFKA-14203 Disable snapshot generation on broker after metadata errors (apache#12596) KAFKA-14216: Remove ZK reference from org.apache.kafka.server.quota.ClientQuotaCallback javadoc (apache#12617) KAFKA-14217: app-reset-tool.html should not show --zookeeper flag that no longer exists (apache#12618) ...

…istent ACL reads. (apache#12628) Fixes an issue with StandardAuthorizer#authorize that allowed inconsistent results. The underlying concurrent data structure (ConcurrentSkipListMap) had weak consistency guarantees. This meant that a concurrent update to the authorizer data could result in the authorize function processing ACL updates out of order. This patch replaces the concurrent data structures with regular non-thread safe equivalents and uses a read/write lock for thread safety and strong consistency. Reviewers: David Arthur <mumrah@gmail.com>, Jason Gustafson <jason@confluent.io>, Colin P. McCabe <cmccabe@apache.org>, Luke Chen <showuon@gmail.com>

akhileshchg added 2 commits September 12, 2022 15:57

Benchmark changes

cc1bf8b

mumrah reviewed Sep 12, 2022

View reviewed changes

akhileshchg added 2 commits September 12, 2022 16:55

Addressed the performance issues with StandardAuthorizer#acls

3193034

remove unused imports

70061c1

hachikuji reviewed Sep 13, 2022

View reviewed changes

Addressed Jason's comments

6e0bdd0

mumrah mentioned this pull request Sep 13, 2022

KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads. #12627

Closed

jsancio added core Kafka Broker 3.3 labels Sep 13, 2022

clean imports

b65180d

akhileshchg closed this Sep 16, 2022

akhileshchg reopened this Sep 19, 2022

revert benchmark changes

426e1a5

reduce the crictical section for write lock on loading snapshot

3171fac

mumrah reviewed Sep 20, 2022

View reviewed changes

Address David's comments

442df12

mumrah approved these changes Sep 20, 2022

View reviewed changes

mumrah merged commit 6c6b8e2 into apache:trunk Sep 20, 2022

Conversation

akhileshchg commented Sep 12, 2022

Uh oh!

mumrah left a comment • 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!

mumrah Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

akhileshchg Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

akhileshchg commented Sep 12, 2022

Uh oh!

hachikuji left a comment • 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!

akhileshchg commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmccabe commented Sep 20, 2022

Uh oh!

akhileshchg commented Sep 20, 2022

Uh oh!

showuon commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akhileshchg commented Sep 20, 2022

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mumrah left a comment •

edited

Loading

hachikuji left a comment •

edited

Loading

akhileshchg commented Sep 19, 2022 •

edited

Loading

showuon commented Sep 20, 2022 •

edited

Loading