KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls #16493

FrankYang0529 · 2024-06-30T15:04:26Z

Use LeastLoadedBrokerOrActiveKController for describeAcls, createAcls, and deleteAcls APIs.
Add --bootstrap-controller to AclCommand.
Add related integration tests to AclCommandTest.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

tools/src/test/java/org/apache/kafka/tools/ClusterToolTest.java

chia7712

@FrankYang0529 thanks for this patch. Could you please make testAclOperations run with controller also?

kafka/core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala

Line 1121 in 42f267a

def testAclOperations(quorum: String): Unit = {

core/src/main/scala/kafka/admin/AclCommand.scala

FrankYang0529 · 2024-07-05T16:58:38Z

Hi @chia7712, I addressed all comments. Thanks for your review.

chia7712 · 2024-08-10T18:18:02Z

@FrankYang0529 Could you please move all tests to BootstrapControllersIntegrationTest after #16822 gets merged?

chia7712 · 2024-08-18T19:00:09Z

@FrankYang0529 any updates?

FrankYang0529 · 2024-08-19T10:23:30Z

@FrankYang0529 Could you please move all tests to BootstrapControllersIntegrationTest after #16822 gets merged?

Hi @chia7712, updated it. Could you take a look? Thank you.

chia7712

@FrankYang0529 thanks for this nice patch. one one small comment is left

chia7712 · 2024-08-19T21:13:30Z

core/src/test/java/kafka/test/server/BootstrapControllersIntegrationTest.java

+                    .map(server -> server.authorizer().get()).collect(Collectors.toList()));
+            allAuthorizers.addAll(clusterInstance.controllers().values().stream()
+                    .map(server -> server.authorizer().get()).collect(Collectors.toList()));
+            allAuthorizers.forEach(authorizer -> kafka.utils.TestUtils.waitAndVerifyAcls(


Could you please add those code to ClusterInstance as a helper method?

Yes, add a method authorizers to ClusterInstance. Thanks.

chia7712

@FrankYang0529 thanks for updated PR

chia7712 · 2024-08-20T19:59:57Z

core/src/test/java/kafka/test/ClusterInstance.java

@@ -204,4 +207,13 @@ default void waitForTopic(String topic, int partitions) throws InterruptedExcept
                60000L, "Timeout waiting for controller metadata propagating to brokers");
        }
    }
+
+    default List<Authorizer> authorizers() {


Could you please check the existence of Option ?

chia7712 · 2024-08-20T20:00:16Z

core/src/test/java/kafka/test/ClusterInstance.java

@@ -204,4 +207,13 @@ default void waitForTopic(String topic, int partitions) throws InterruptedExcept
                60000L, "Timeout waiting for controller metadata propagating to brokers");
        }
    }
+


for another, please add waitAcls. for example:

default void waitAcls(AclBindingFilter filter, Collection<AccessControlEntry> entries) throws InterruptedException { for (Authorizer authorizer : authorizers()) { TestUtils.waitForCondition(() -> { Assertions.assertIterableEquals(authorizer.acls(filter), entries); return true; }, "except: "); } }

chia7712

@FrankYang0529 thanks for this patch

chia7712 · 2024-08-22T05:23:38Z

core/src/main/scala/kafka/admin/AclCommand.scala


-      if (options.has(authorizerPropertiesOpt) && options.has(bootstrapServerOpt))
+      if (options.has(authorizerPropertiesOpt) && (hasServerOrController))


nit: if (options.has(authorizerPropertiesOpt) && hasServerOrController)

chia7712 · 2024-08-22T05:25:30Z

core/src/test/java/kafka/test/ClusterInstance.java

+                authorizer.acls(filter).forEach(aclBinding -> accessControlEntrySet.add(aclBinding.entry()));
+                actualEntries.set(accessControlEntrySet);
+                return accessControlEntrySet.containsAll(entries) && entries.containsAll(accessControlEntrySet);
+            }, () -> "except acls: " + entries + ", actual acls: " + actualEntries.get());


`expected acls:

FrankYang0529 · 2024-08-22T07:54:11Z

Hi @chia7712, thanks for the review. I address all comments.

chia7712 · 2024-08-27T09:13:33Z

@FrankYang0529 could you please fix conflicts?

FrankYang0529 · 2024-08-27T09:28:20Z

@FrankYang0529 could you please fix conflicts?

Fixed it. Thank you.

chia7712

@FrankYang0529 thanks for this patch.

core/src/test/java/kafka/admin/AclCommandTest.java

FrankYang0529 · 2024-09-04T00:39:12Z

Hi @chia7712, I addressed all comments. Could you take a look again? Thank you.

chia7712

@FrankYang0529 overall LGTM. two small comments are left.

chia7712 · 2024-09-04T06:37:40Z

core/src/test/java/kafka/admin/AclCommandTest.java

+                    .filter(e -> e.getLevel().equals(Level.WARN.toString()))
+                    .filter(e -> e.getThrowableClassName().filter(name -> name.equals(InstanceAlreadyExistsException.class.getName())).isPresent())
+                    .count(), "There should be no warnings about multiple registration of mbeans");
+        } catch (InterruptedException | IOException e) {


we don't need to catch the exception. Instead, we can add the exception to the method signature.

Hi @chia7712, thanks for your review. I add exceptions to the method signature.

chia7712 · 2024-09-04T06:37:50Z

core/src/test/java/kafka/admin/AclCommandTest.java

+                    .filter(e -> e.getLevel().equals(Level.WARN.toString()))
+                    .filter(e -> e.getThrowableClassName().filter(name -> name.equals(InstanceAlreadyExistsException.class.getName())).isPresent())
+                    .count(), "There should be no warnings about multiple registration of mbeans");
+        } catch (InterruptedException | IOException e) {


Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 · 2024-09-10T14:34:44Z

Hi @chia7712, could you help me review this when you have time? Thank you.

chia7712 reviewed Jun 30, 2024

View reviewed changes

tools/src/test/java/org/apache/kafka/tools/ClusterToolTest.java Outdated Show resolved Hide resolved

FrankYang0529 force-pushed the KAFKA-17036 branch 3 times, most recently from 5ea3dc6 to d2741e6 Compare July 4, 2024 12:49

chia7712 reviewed Jul 4, 2024

View reviewed changes

core/src/main/scala/kafka/admin/AclCommand.scala Outdated Show resolved Hide resolved

core/src/main/scala/kafka/admin/AclCommand.scala Outdated Show resolved Hide resolved

core/src/main/scala/kafka/admin/AclCommand.scala Outdated Show resolved Hide resolved

FrankYang0529 force-pushed the KAFKA-17036 branch from d2741e6 to 50a61b7 Compare July 5, 2024 16:58

FrankYang0529 requested a review from chia7712 July 9, 2024 00:54

FrankYang0529 force-pushed the KAFKA-17036 branch from 50a61b7 to d166017 Compare July 11, 2024 14:33

FrankYang0529 force-pushed the KAFKA-17036 branch from d166017 to 1fc3eeb Compare July 21, 2024 14:19

FrankYang0529 force-pushed the KAFKA-17036 branch from 1fc3eeb to 107261b Compare July 29, 2024 08:00

FrankYang0529 force-pushed the KAFKA-17036 branch from 107261b to 2bfcf53 Compare August 19, 2024 09:55

chia7712 reviewed Aug 19, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-17036 branch from 2bfcf53 to cbac335 Compare August 20, 2024 01:19

FrankYang0529 requested a review from chia7712 August 20, 2024 09:23

chia7712 reviewed Aug 20, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-17036 branch from cbac335 to a76f07d Compare August 21, 2024 04:58

chia7712 reviewed Aug 22, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-17036 branch from a76f07d to cafa09d Compare August 22, 2024 07:52

FrankYang0529 force-pushed the KAFKA-17036 branch from cafa09d to e7217a5 Compare August 27, 2024 09:28

chia7712 reviewed Sep 1, 2024

View reviewed changes

core/src/test/java/kafka/admin/AclCommandTest.java Outdated Show resolved Hide resolved

core/src/test/java/kafka/admin/AclCommandTest.java Outdated Show resolved Hide resolved

FrankYang0529 force-pushed the KAFKA-17036 branch from e7217a5 to 120f824 Compare September 3, 2024 05:06

FrankYang0529 requested a review from chia7712 September 4, 2024 00:39

chia7712 reviewed Sep 4, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-17036 branch from 120f824 to 195ce91 Compare September 4, 2024 15:10

KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls

e393678

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the KAFKA-17036 branch from 195ce91 to e393678 Compare September 6, 2024 10:50

FrankYang0529 requested a review from chia7712 September 10, 2024 14:34

chia7712 approved these changes Sep 13, 2024

View reviewed changes

chia7712 merged commit 4692aeb into apache:trunk Sep 13, 2024
5 of 6 checks passed

FrankYang0529 deleted the KAFKA-17036 branch September 14, 2024 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls #16493

KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls #16493

FrankYang0529 commented Jun 30, 2024

chia7712 left a comment

FrankYang0529 commented Jul 5, 2024

chia7712 commented Aug 10, 2024

chia7712 commented Aug 18, 2024

FrankYang0529 commented Aug 19, 2024

chia7712 left a comment

chia7712 Aug 19, 2024

FrankYang0529 Aug 20, 2024

chia7712 left a comment

chia7712 Aug 20, 2024

chia7712 Aug 20, 2024

chia7712 left a comment

chia7712 Aug 22, 2024

chia7712 Aug 22, 2024

FrankYang0529 commented Aug 22, 2024

chia7712 commented Aug 27, 2024

FrankYang0529 commented Aug 27, 2024

chia7712 left a comment

FrankYang0529 commented Sep 4, 2024

chia7712 left a comment

chia7712 Sep 4, 2024

FrankYang0529 Sep 4, 2024

chia7712 Sep 4, 2024

FrankYang0529 commented Sep 10, 2024


		if (options.has(authorizerPropertiesOpt) && options.has(bootstrapServerOpt))
		if (options.has(authorizerPropertiesOpt) && (hasServerOrController))

KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls #16493

KAFKA-17036: KIP-919 supports for createAcls, deleteAcls, describeAcls #16493

Conversation

FrankYang0529 commented Jun 30, 2024

Committer Checklist (excluded from commit message)

chia7712 left a comment

Choose a reason for hiding this comment

FrankYang0529 commented Jul 5, 2024

chia7712 commented Aug 10, 2024

chia7712 commented Aug 18, 2024

FrankYang0529 commented Aug 19, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankYang0529 commented Aug 22, 2024

chia7712 commented Aug 27, 2024

FrankYang0529 commented Aug 27, 2024

chia7712 left a comment

Choose a reason for hiding this comment

FrankYang0529 commented Sep 4, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankYang0529 commented Sep 10, 2024