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 8 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
107 changes: 87 additions & 20 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,7 @@
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import static org.apache.kafka.server.authorizer.AuthorizationResult.ALLOWED;
import static org.apache.kafka.server.authorizer.AuthorizationResult.DENIED;
Expand All @@ -58,28 +59,46 @@ 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();

@Override
public synchronized void setAclMutator(AclMutator aclMutator) {
this.data = data.copyWithNewAclMutator(aclMutator);
public void setAclMutator(AclMutator aclMutator) {
lock.writeLock().lock();
try {
this.data = data.copyWithNewAclMutator(aclMutator);
} finally {
lock.writeLock().unlock();
}
}

@Override
public AclMutator aclMutatorOrException() {
AclMutator aclMutator = data.aclMutator;
AclMutator aclMutator;
lock.readLock().lock();
try {
aclMutator = data.aclMutator;
} finally {
lock.readLock().unlock();
}
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() {
lock.writeLock().lock();
try {
data = data.copyWithNewLoadingComplete(true);
} finally {
lock.writeLock().unlock();
}
data.log.info("Completed initial ACL load process.");
initialLoadFuture.complete(null);
}
Expand All @@ -97,17 +116,36 @@ public void completeInitialLoad(Exception e) {

@Override
public void addAcl(Uuid id, StandardAcl acl) {
data.addAcl(id, acl);
lock.writeLock().lock();
try {
data.addAcl(id, acl);
} finally {
lock.writeLock().unlock();
}
}

@Override
public void removeAcl(Uuid id) {
data.removeAcl(id);
lock.writeLock().lock();
try {
data.removeAcl(id);
} finally {
lock.writeLock().unlock();
}
}

@Override
public synchronized void loadSnapshot(Map<Uuid, StandardAcl> acls) {
data = data.copyWithNewAcls(acls.entrySet());
public void loadSnapshot(Map<Uuid, StandardAcl> acls) {
StandardAuthorizerData newData = StandardAuthorizerData.createEmpty();
for (Map.Entry<Uuid, StandardAcl> entry : acls.entrySet()) {
newData.addAcl(entry.getKey(), entry.getValue());
}
lock.writeLock().lock();
try {
data = data.copyWithNewAcls(newData.getAclsByResource(), newData.getAclsById());
} finally {
lock.writeLock().unlock();
}
}

@Override
Expand All @@ -129,23 +167,37 @@ 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);
lock.readLock().lock();
try {
for (Action action : actions) {
AuthorizationResult result = data.authorize(requestContext, action);
Comment thread
akhileshchg marked this conversation as resolved.
Outdated
results.add(result);
}
} finally {
lock.readLock().unlock();
}
return results;
}

@Override
public Iterable<AclBinding> acls(AclBindingFilter filter) {
return data.acls(filter);
lock.readLock().lock();
try {
return data.acls(filter);
Comment thread
akhileshchg marked this conversation as resolved.
} finally {
lock.readLock().unlock();
}
}

@Override
public int aclCount() {
return data.aclCount();
lock.readLock().lock();
try {
return data.aclCount();
} finally {
lock.readLock().unlock();
}
}

@Override
Expand All @@ -156,7 +208,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 +217,32 @@ public synchronized void configure(Map<String, ?> configs) {
} catch (Exception e) {
nodeId = -1;
}
this.data = data.copyWithNewConfig(nodeId, superUsers, defaultResult);
lock.writeLock().lock();
try {
data = data.copyWithNewConfig(nodeId, superUsers, defaultResult);
} finally {
lock.writeLock().unlock();
}
this.data.log.info("set super.users={}, default result={}", String.join(",", superUsers), defaultResult);
}

// VisibleForTesting
Set<String> superUsers() {
return new HashSet<>(data.superUsers());
lock.readLock().lock();
try {
return new HashSet<>(data.superUsers());
} finally {
lock.readLock().unlock();
}
}

AuthorizationResult defaultResult() {
return data.defaultResult();
lock.readLock().lock();
try {
return data.defaultResult();
} finally {
lock.readLock().unlock();
}
}

static Set<String> getConfiguredSuperUsers(Map<String, ?> configs) {
Expand Down
99 changes: 41 additions & 58 deletions metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizerData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,17 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
import java.util.NavigableSet;
import java.util.NoSuchElementException;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.TreeSet;

import static org.apache.kafka.common.acl.AclOperation.ALL;
import static org.apache.kafka.common.acl.AclOperation.ALTER;
Expand All @@ -64,7 +65,7 @@
/**
* A class which encapsulates the configuration and the ACL data owned by StandardAuthorizer.
*
* The methods in this class support lockless concurrent access.
* The class is not thread-safe.
*/
public class StandardAuthorizerData {
/**
Expand Down Expand Up @@ -111,12 +112,12 @@ public class StandardAuthorizerData {
/**
* Contains all of the current ACLs sorted by (resource type, resource name).
*/
private final ConcurrentSkipListSet<StandardAcl> aclsByResource;
private final TreeSet<StandardAcl> aclsByResource;

/**
* Contains all of the current ACLs indexed by UUID.
*/
private final ConcurrentHashMap<Uuid, StandardAcl> aclsById;
private final HashMap<Uuid, StandardAcl> aclsById;

private static Logger createLogger(int nodeId) {
return new LogContext("[StandardAuthorizer " + nodeId + "] ").logger(StandardAuthorizerData.class);
Expand All @@ -132,16 +133,16 @@ static StandardAuthorizerData createEmpty() {
false,
Collections.emptySet(),
DENIED,
new ConcurrentSkipListSet<>(), new ConcurrentHashMap<>());
new TreeSet<>(), new HashMap<>());
}

private StandardAuthorizerData(Logger log,
AclMutator aclMutator,
boolean loadingComplete,
Set<String> superUsers,
AuthorizationResult defaultResult,
ConcurrentSkipListSet<StandardAcl> aclsByResource,
ConcurrentHashMap<Uuid, StandardAcl> aclsById) {
TreeSet<StandardAcl> aclsByResource,
HashMap<Uuid, StandardAcl> aclsById) {
this.log = log;
this.auditLog = auditLogger();
this.aclMutator = aclMutator;
Expand Down Expand Up @@ -193,15 +194,29 @@ StandardAuthorizerData copyWithNewAcls(Collection<Entry<Uuid, StandardAcl>> aclE
loadingComplete,
superUsers,
defaultRule.result,
new ConcurrentSkipListSet<>(),
new ConcurrentHashMap<>());
new TreeSet<>(),
new HashMap<>());
for (Entry<Uuid, StandardAcl> entry : aclEntries) {
newData.addAcl(entry.getKey(), entry.getValue());
}
log.info("Applied {} acl(s) from image.", aclEntries.size());
return newData;
}

StandardAuthorizerData copyWithNewAcls(TreeSet<StandardAcl> aclsByResource, HashMap<Uuid,
Comment thread
akhileshchg marked this conversation as resolved.
StandardAcl> aclsById) {
StandardAuthorizerData newData = new StandardAuthorizerData(
log,
aclMutator,
loadingComplete,
superUsers,
defaultRule.result,
aclsByResource,
aclsById);
log.info("Initialized with {} acl(s).", aclsById.size());
return newData;
}

void addAcl(Uuid id, StandardAcl acl) {
try {
StandardAcl prevAcl = aclsById.putIfAbsent(id, acl);
Expand Down Expand Up @@ -530,54 +545,14 @@ static AuthorizationResult findResult(Action action,
}

Iterable<AclBinding> acls(AclBindingFilter filter) {
Comment thread
akhileshchg marked this conversation as resolved.
return new AclIterable(filter);
}

class AclIterable implements Iterable<AclBinding> {
private final AclBindingFilter filter;

AclIterable(AclBindingFilter filter) {
this.filter = filter;
}

@Override
public Iterator<AclBinding> iterator() {
return new AclIterator(filter);
}
}

class AclIterator implements Iterator<AclBinding> {
private final AclBindingFilter filter;
private final Iterator<StandardAcl> iterator;
private AclBinding next;

AclIterator(AclBindingFilter filter) {
this.filter = filter;
this.iterator = aclsByResource.iterator();
this.next = null;
}

@Override
public boolean hasNext() {
while (next == null) {
if (!iterator.hasNext()) return false;
AclBinding binding = iterator.next().toBinding();
if (filter.matches(binding)) {
next = binding;
}
List<AclBinding> aclBindingList = new ArrayList<>();
aclsByResource.forEach(acl -> {
AclBinding aclBinding = acl.toBinding();
if (filter.matches(aclBinding)) {
aclBindingList.add(aclBinding);
}
return true;
}

@Override
public AclBinding next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
AclBinding result = next;
next = null;
return result;
}
});
return aclBindingList;
}

private interface MatchingRule {
Expand Down Expand Up @@ -654,4 +629,12 @@ MatchingAclRule build() {
}
}
}

TreeSet<StandardAcl> getAclsByResource() {
return aclsByResource;
}

HashMap<Uuid, StandardAcl> getAclsById() {
return aclsById;
}
}