From f6408a21ccc2fa9b6ce6778d7fe2c2f9526ffb37 Mon Sep 17 00:00:00 2001 From: ChangYu Huang Date: Thu, 4 Sep 2025 17:42:09 -0400 Subject: [PATCH 1/4] Add a check before adding acls --- .../org/apache/kafka/tools/AclCommand.java | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/AclCommand.java b/tools/src/main/java/org/apache/kafka/tools/AclCommand.java index fd62e8f3b56d8..473e18249c5e1 100644 --- a/tools/src/main/java/org/apache/kafka/tools/AclCommand.java +++ b/tools/src/main/java/org/apache/kafka/tools/AclCommand.java @@ -106,9 +106,27 @@ private static void addAcls(Admin admin, AclCommandOptions opts) throws Executio for (Map.Entry> entry : resourceToAcl.entrySet()) { ResourcePattern resource = entry.getKey(); Set acls = entry.getValue(); - System.out.println("Adding ACLs for resource `" + resource + "`: " + NL + " " + acls.stream().map(a -> "\t" + a).collect(Collectors.joining(NL)) + NL); - Collection aclBindings = acls.stream().map(acl -> new AclBinding(resource, acl)).collect(Collectors.toList()); - admin.createAcls(aclBindings).all().get(); + + AclBindingFilter filter = new AclBindingFilter(resource.toFilter(), AccessControlEntryFilter.ANY); + Collection existingBindings = admin.describeAcls(filter).values().get(); + Set existingBindingsSet = new HashSet<>(existingBindings); + + List aclBindings = new ArrayList<>(); + List aclsToAdd = new ArrayList<>(); + for (AccessControlEntry acl : acls) { + AclBinding binding = new AclBinding(resource, acl); + if (existingBindingsSet.contains(binding)) { + System.out.println("Acl " + binding + " already exists."); + } else { + aclBindings.add(binding); + aclsToAdd.add(acl); + } + } + + if (!aclBindings.isEmpty()) { + System.out.println("Adding ACLs for resource `" + resource + "`: " + NL + " " + aclsToAdd.stream().map(a -> "\t" + a).collect(Collectors.joining(NL)) + NL); + admin.createAcls(aclBindings).all().get(); + } } } From 961ac76397daa49f5667594b4c616d34342dd398 Mon Sep 17 00:00:00 2001 From: ChangYu Huang Date: Thu, 4 Sep 2025 18:48:01 -0400 Subject: [PATCH 2/4] Add a test for adding duplicate acl --- .../apache/kafka/tools/AclCommandTest.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java b/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java index 7abc63f3503fd..271e0a93a1e7f 100644 --- a/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java @@ -17,6 +17,7 @@ package org.apache.kafka.tools; import org.apache.kafka.common.acl.AccessControlEntry; +import org.apache.kafka.common.acl.AclBinding; import org.apache.kafka.common.acl.AclBindingFilter; import org.apache.kafka.common.acl.AclOperation; import org.apache.kafka.common.acl.AclPermissionType; @@ -82,6 +83,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertFalse; @ClusterTestDefaults( types = {Type.KRAFT}, @@ -275,6 +277,24 @@ public void testPatternTypesWithAdminAPIAndBootstrapController(ClusterInstance c testPatternTypes(adminArgsWithBootstrapController(cluster.bootstrapControllers(), Optional.empty())); } + @ClusterTest + public void testDuplicateAdd(ClusterInstance cluster) { + final String topicName = "test-topic"; + final String principal = "User:Alice"; + ResourcePattern resource = new ResourcePattern(ResourceType.TOPIC, topicName, PatternType.LITERAL); + AccessControlEntry ace = new AccessControlEntry(principal, WILDCARD_HOST, READ, ALLOW); + AclBinding binding = new AclBinding(resource, ace); + List cmdArgs = adminArgs(cluster.bootstrapServers(), Optional.empty()); + List initialAddArgs = new ArrayList<>(cmdArgs); + initialAddArgs.addAll(List.of(ADD, TOPIC, topicName, "--allow-principal", principal, OPERATION, "Read")); + + callMain(initialAddArgs); + String out = callMain(initialAddArgs).getKey(); + + assertTrue(out.contains("Acl " + binding + " already exists.")); + assertFalse(out.contains("Adding ACLs for resource")); + } + @Test public void testUseBootstrapServerOptWithBootstrapControllerOpt() { assertInitializeInvalidOptionsExitCodeAndMsg( From 2070afe92b51321b7b4b6d25bdd2ffe337e63f31 Mon Sep 17 00:00:00 2001 From: ChangYu Huang Date: Thu, 4 Sep 2025 18:51:58 -0400 Subject: [PATCH 3/4] Fix format violation --- tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java b/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java index 271e0a93a1e7f..71572d6c33ad3 100644 --- a/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java @@ -80,10 +80,10 @@ import static org.apache.kafka.server.config.ServerConfigs.AUTHORIZER_CLASS_NAME_CONFIG; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.assertFalse; @ClusterTestDefaults( types = {Type.KRAFT}, From ce0caead56070e2c02170bd32e233f1664a1503b Mon Sep 17 00:00:00 2001 From: ChangYu Huang Date: Sat, 6 Sep 2025 13:06:00 -0400 Subject: [PATCH 4/4] Refactor --- tools/src/main/java/org/apache/kafka/tools/AclCommand.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/AclCommand.java b/tools/src/main/java/org/apache/kafka/tools/AclCommand.java index 473e18249c5e1..5b0cae7c9197a 100644 --- a/tools/src/main/java/org/apache/kafka/tools/AclCommand.java +++ b/tools/src/main/java/org/apache/kafka/tools/AclCommand.java @@ -108,23 +108,20 @@ private static void addAcls(Admin admin, AclCommandOptions opts) throws Executio Set acls = entry.getValue(); AclBindingFilter filter = new AclBindingFilter(resource.toFilter(), AccessControlEntryFilter.ANY); - Collection existingBindings = admin.describeAcls(filter).values().get(); - Set existingBindingsSet = new HashSet<>(existingBindings); + Set existingBindingsSet = Set.copyOf(admin.describeAcls(filter).values().get()); List aclBindings = new ArrayList<>(); - List aclsToAdd = new ArrayList<>(); for (AccessControlEntry acl : acls) { AclBinding binding = new AclBinding(resource, acl); if (existingBindingsSet.contains(binding)) { System.out.println("Acl " + binding + " already exists."); } else { aclBindings.add(binding); - aclsToAdd.add(acl); } } if (!aclBindings.isEmpty()) { - System.out.println("Adding ACLs for resource `" + resource + "`: " + NL + " " + aclsToAdd.stream().map(a -> "\t" + a).collect(Collectors.joining(NL)) + NL); + System.out.println("Adding ACLs for resource `" + resource + "`: " + NL + " " + aclBindings.stream().map(AclBinding::entry).map(a -> "\t" + a).collect(Collectors.joining(NL)) + NL); admin.createAcls(aclBindings).all().get(); } }