Skip to content

KAFKA-15983 Kafka-acls should return authorization already done if repeating work is issued #20482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
+38 −3
Open

KAFKA-15983 Kafka-acls should return authorization already done if repeating work is issued #20482

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f6408a2
Add a check before adding acls
jack2012aa Sep 4, 2025
961ac76
Add a test for adding duplicate acl
jack2012aa Sep 4, 2025
2070afe
Fix format violation
jack2012aa Sep 4, 2025
9838434
Merge branch 'apache:trunk' into KAFKA-15983
jack2012aa Sep 4, 2025
390803b
Merge branch 'apache:trunk' into KAFKA-15983
jack2012aa Sep 5, 2025
ce0caea
Refactor
jack2012aa Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions tools/src/main/java/org/apache/kafka/tools/AclCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,27 @@ private static void addAcls(Admin admin, AclCommandOptions opts) throws Executio
for (Map.Entry<ResourcePattern, Set<AccessControlEntry>> entry : resourceToAcl.entrySet()) {
ResourcePattern resource = entry.getKey();
Set<AccessControlEntry> acls = entry.getValue();
System.out.println("Adding ACLs for resource `" + resource + "`: " + NL + " " + acls.stream().map(a -> "\t" + a).collect(Collectors.joining(NL)) + NL);
Collection<AclBinding> 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<AclBinding> existingBindings = admin.describeAcls(filter).values().get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var existingBindingsSet = Set.copyOf(admin.describeAcls(filter).values().get());

Set<AclBinding> existingBindingsSet = new HashSet<>(existingBindings);

List<AclBinding> aclBindings = new ArrayList<>();
List<AccessControlEntry> 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aclsToAdd.stream() could be replaced by aclBindings.stream().map(AclBinding::entry), right?

admin.createAcls(aclBindings).all().get();
}
}
}

Expand Down
20 changes: 20 additions & 0 deletions tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -79,6 +80,7 @@
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;
Expand Down Expand Up @@ -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<String> cmdArgs = adminArgs(cluster.bootstrapServers(), Optional.empty());
List<String> 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(
Expand Down