Skip to content

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

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

Merged
mumrah merged 9 commits into from
Sep 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b8efe71
KAFKA-14214: Introduce read-write lock to StandardAuthorizer for cons…
akhileshchg Sep 12, 2022
cc1bf8b
Benchmark changes
akhileshchg Sep 12, 2022
3193034
Addressed the performance issues with StandardAuthorizer#acls
akhileshchg Sep 12, 2022
70061c1
remove unused imports
akhileshchg Sep 13, 2022
6e0bdd0
Addressed Jason's comments
akhileshchg Sep 13, 2022
b65180d
clean imports
akhileshchg Sep 13, 2022
426e1a5
revert benchmark changes
akhileshchg Sep 20, 2022
3171fac
reduce the crictical section for write lock on loading snapshot
akhileshchg Sep 20, 2022
442df12
Address David's comments
akhileshchg Sep 20, 2022
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
64 changes: 53 additions & 11 deletions jmh-benchmarks/src/main/java/org/apache/kafka/jmh/acl/AclAuthorizerBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import kafka.security.authorizer.AclAuthorizer;
import kafka.security.authorizer.AclAuthorizer.VersionedAcls;
import kafka.security.authorizer.AclEntry;
import org.apache.kafka.common.Uuid;
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 All @@ -34,7 +36,10 @@
import org.apache.kafka.common.resource.ResourceType;
import org.apache.kafka.common.security.auth.KafkaPrincipal;
import org.apache.kafka.common.security.auth.SecurityProtocol;
import org.apache.kafka.metadata.authorizer.StandardAcl;
import org.apache.kafka.metadata.authorizer.StandardAuthorizer;
import org.apache.kafka.server.authorizer.Action;
import org.apache.kafka.server.authorizer.Authorizer;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
Expand All @@ -50,6 +55,7 @@
import org.openjdk.jmh.annotations.Warmup;
import scala.collection.JavaConverters;

import java.io.IOException;
import java.net.InetAddress;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -61,6 +67,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;

@State(Scope.Benchmark)
@Fork(value = 1)
Expand All @@ -69,6 +76,22 @@
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
public class AclAuthorizerBenchmark {

public enum AuthorizerType {
ACL(AclAuthorizer::new),
KRAFT(StandardAuthorizer::new);

private Supplier<Authorizer> supplier;

AuthorizerType(Supplier<Authorizer> supplier) {
this.supplier = supplier;
}

Authorizer newAuthorizer() {
return supplier.get();
}
}

@Param({"10000", "50000", "200000"})
private int resourceCount;
//no. of. rules per resource
Expand All @@ -78,10 +101,13 @@ public class AclAuthorizerBenchmark {
@Param({"0", "20", "50", "90", "99", "99.9", "99.99", "100"})
private double denyPercentage;

@Param({"ACL", "KRAFT"})
private AuthorizerType authorizerType;

private final int hostPreCount = 1000;
private final String resourceNamePrefix = "foo-bar35_resource-";
private final AclAuthorizer aclAuthorizer = new AclAuthorizer();
private final KafkaPrincipal principal = new KafkaPrincipal(KafkaPrincipal.USER_TYPE, "test-user");
private Authorizer authorizer;
private List<Action> actions = new ArrayList<>();
private RequestContext authorizeContext;
private RequestContext authorizeByResourceTypeContext;
Expand All @@ -94,6 +120,7 @@ public class AclAuthorizerBenchmark {

@Setup(Level.Trial)
public void setup() throws Exception {
authorizer = authorizerType.newAuthorizer();
prepareAclCache();
prepareAclToUpdate();
// By adding `-95` to the resource name prefix, we cause the `TreeMap.from/to` call to return
Expand Down Expand Up @@ -177,9 +204,23 @@ private void prepareAclCache() {
}
}

setupAcls(aclEntries);
}

private void setupAcls(Map<ResourcePattern, Set<AclEntry>> aclEntries) {
for (Map.Entry<ResourcePattern, Set<AclEntry>> entryMap : aclEntries.entrySet()) {
aclAuthorizer.updateCache(entryMap.getKey(),
new VersionedAcls(JavaConverters.asScalaSetConverter(entryMap.getValue()).asScala().toSet(), 1));
switch (authorizerType) {
case ACL:
((AclAuthorizer) authorizer).updateCache(entryMap.getKey(),
new VersionedAcls(JavaConverters.asScalaSetConverter(entryMap.getValue()).asScala().toSet(), 1));
break;
case KRAFT:
for (AclEntry aclEntry : entryMap.getValue()) {
StandardAcl acl = StandardAcl.fromAclBinding(new AclBinding(entryMap.getKey(), aclEntry.ace()));
((StandardAuthorizer) authorizer).addAcl(Uuid.randomUuid(), acl);
}
break;
}
}
}

Expand Down Expand Up @@ -207,30 +248,31 @@ private Boolean shouldDeny() {
}

@TearDown(Level.Trial)
public void tearDown() {
aclAuthorizer.close();
public void tearDown() throws IOException {
authorizer.close();
}

@Benchmark
public void testAclsIterator() {
aclAuthorizer.acls(AclBindingFilter.ANY);
authorizer.acls(AclBindingFilter.ANY);
}

@Benchmark
public void testAuthorizer() {
aclAuthorizer.authorize(authorizeContext, actions);
authorizer.authorize(authorizeContext, actions);
}

@Benchmark
public void testAuthorizeByResourceType() {
aclAuthorizer.authorizeByResourceType(authorizeByResourceTypeContext, AclOperation.READ, ResourceType.TOPIC);
authorizer.authorizeByResourceType(authorizeByResourceTypeContext, AclOperation.READ, ResourceType.TOPIC);
}

@Benchmark
public void testUpdateCache() {
AclAuthorizer aclAuthorizer = new AclAuthorizer();
for (Map.Entry<ResourcePattern, VersionedAcls> e : aclToUpdate.entrySet()) {
aclAuthorizer.updateCache(e.getKey(), e.getValue());
if (authorizerType == AuthorizerType.ACL) {
for (Map.Entry<ResourcePattern, VersionedAcls> e : aclToUpdate.entrySet()) {
((AclAuthorizer) authorizer).updateCache(e.getKey(), e.getValue());
}
}
}
}
74 changes: 50 additions & 24 deletions metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Supplier;

import static org.apache.kafka.server.authorizer.AuthorizationResult.ALLOWED;
import static org.apache.kafka.server.authorizer.AuthorizationResult.DENIED;
Expand All @@ -58,28 +60,48 @@ public class StandardAuthorizer implements ClusterMetadataAuthorizer {
*/
private final CompletableFuture<Void> initialLoadFuture = new CompletableFuture<>();

private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

/**
* The current data. Can be read without a lock. Must be written while holding the object lock.
* The current data. We use a read-write lock to synchronize reads and writes to the data.
Comment thread
akhileshchg marked this conversation as resolved.
Outdated
*/
private volatile StandardAuthorizerData data = StandardAuthorizerData.createEmpty();

private void inWriteLock(Runnable function) {
lock.writeLock().lock();
try {
function.run();
} finally {
lock.writeLock().unlock();
}
}

private <T> T inReadLock(Supplier<T> function) {
lock.readLock().lock();
try {
return function.get();
} finally {
lock.readLock().unlock();
}
}

@Override
public synchronized void setAclMutator(AclMutator aclMutator) {
this.data = data.copyWithNewAclMutator(aclMutator);
public void setAclMutator(AclMutator aclMutator) {
inWriteLock(() -> this.data = data.copyWithNewAclMutator(aclMutator));
}

@Override
public AclMutator aclMutatorOrException() {
AclMutator aclMutator = data.aclMutator;
AclMutator aclMutator = inReadLock(() -> data.aclMutator);
if (aclMutator == null) {
throw new NotControllerException("The current node is not the active controller.");
}
return aclMutator;
}

@Override
public synchronized void completeInitialLoad() {
data = data.copyWithNewLoadingComplete(true);
public void completeInitialLoad() {
inWriteLock(() -> data = data.copyWithNewLoadingComplete(true));
data.log.info("Completed initial ACL load process.");
initialLoadFuture.complete(null);
}
Expand All @@ -97,17 +119,17 @@ public void completeInitialLoad(Exception e) {

@Override
public void addAcl(Uuid id, StandardAcl acl) {
data.addAcl(id, acl);
inWriteLock(() -> data.addAcl(id, acl));
}

@Override
public void removeAcl(Uuid id) {
data.removeAcl(id);
inWriteLock(() -> data.removeAcl(id));
}

@Override
public synchronized void loadSnapshot(Map<Uuid, StandardAcl> acls) {
data = data.copyWithNewAcls(acls.entrySet());
public void loadSnapshot(Map<Uuid, StandardAcl> acls) {
inWriteLock(() -> data = data.copyWithNewAcls(acls.entrySet()));
}

@Override
Expand All @@ -129,23 +151,24 @@ public synchronized void loadSnapshot(Map<Uuid, StandardAcl> acls) {
public List<AuthorizationResult> authorize(
AuthorizableRequestContext requestContext,
List<Action> actions) {
StandardAuthorizerData curData = data;
List<AuthorizationResult> results = new ArrayList<>(actions.size());
for (Action action: actions) {
AuthorizationResult result = curData.authorize(requestContext, action);
results.add(result);
}
return results;
return inReadLock(() -> {
List<AuthorizationResult> results = new ArrayList<>(actions.size());
for (Action action : actions) {
AuthorizationResult result = data.authorize(requestContext, action);
Comment thread
akhileshchg marked this conversation as resolved.
Outdated
results.add(result);
}
return results;
});
}

@Override
public Iterable<AclBinding> acls(AclBindingFilter filter) {
return data.acls(filter);
return inReadLock(() -> data.acls(filter));
}

@Override
public int aclCount() {
return data.aclCount();
return inReadLock(() -> data.aclCount());
}

@Override
Expand All @@ -156,7 +179,7 @@ public void close() throws IOException {
}

@Override
public synchronized void configure(Map<String, ?> configs) {
public void configure(Map<String, ?> configs) {
Set<String> superUsers = getConfiguredSuperUsers(configs);
AuthorizationResult defaultResult = getDefaultResult(configs);
int nodeId;
Expand All @@ -165,17 +188,20 @@ public synchronized void configure(Map<String, ?> configs) {
} catch (Exception e) {
nodeId = -1;
}
this.data = data.copyWithNewConfig(nodeId, superUsers, defaultResult);
this.data.log.info("set super.users={}, default result={}", String.join(",", superUsers), defaultResult);
final int finalNodeId = nodeId;
inWriteLock(() -> {
this.data = data.copyWithNewConfig(finalNodeId, superUsers, defaultResult);
this.data.log.info("set super.users={}, default result={}", String.join(",", superUsers), defaultResult);
});
}

// VisibleForTesting
Set<String> superUsers() {
return new HashSet<>(data.superUsers());
return inReadLock(() -> new HashSet<>(data.superUsers()));
}

AuthorizationResult defaultResult() {
return data.defaultResult();
return inReadLock(() -> data.defaultResult());
}

static Set<String> getConfiguredSuperUsers(Map<String, ?> configs) {
Expand Down
Loading