LDAP-cache to minimize nbr of queries when unnesting groups.
A new cache named "ldap_groups_byinclude" is introduced to help lessen the number of queries needed to resolve the nested LDAP-groups. Depending on the LDAP tree structure, the unnesting of the LDAP-groups will in many cases generate most of the queries against the LDAP-server. Even though the users have lots of security groups in common, no effort was previously made to reuse the looked up LDAP-group hierarchies. This change introduce a cache which maps the group DNs inheritence. The expiration time is set to 1h, which allows any LDAP changes to be reflected in gerrit within a reasonable time. Though for most companies the hierarchical group structure is mostly static. Change-Id: I913f0e8fffb9116d24ebeb12d004dfa2495a1080
This commit is contained in:
committed by
Shawn O. Pearce
parent
d0b433d607
commit
0919a498f7
@@ -530,6 +530,10 @@ configured on this server. This cache should be configured with a
|
||||
low maxAge setting, to ensure LDAP modifications are picked up in
|
||||
a timely fashion.
|
||||
|
||||
cache `"ldap_groups_byinclude"`::
|
||||
+
|
||||
Caches the hierarchical structure of LDAP groups.
|
||||
|
||||
cache `"ldap_usernames"`::
|
||||
+
|
||||
Caches a mapping of LDAP username to Gerrit account identity. The
|
||||
|
||||
@@ -14,6 +14,8 @@
|
||||
|
||||
package com.google.gerrit.server.auth.ldap;
|
||||
|
||||
import com.google.common.cache.Cache;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.common.data.ParameterizedString;
|
||||
import com.google.gerrit.reviewdb.client.AccountGroup;
|
||||
import com.google.gerrit.server.account.AccountException;
|
||||
@@ -22,6 +24,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.util.ssl.BlindSSLSocketFactory;
|
||||
import com.google.inject.Inject;
|
||||
import com.google.inject.Singleton;
|
||||
import com.google.inject.name.Named;
|
||||
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
|
||||
@@ -48,6 +51,7 @@ import javax.net.ssl.SSLSocketFactory;
|
||||
@Singleton class Helper {
|
||||
static final String LDAP_UUID = "ldap:";
|
||||
|
||||
private final Cache<String, ImmutableSet<String>> groupsByInclude;
|
||||
private final Config config;
|
||||
private final String server;
|
||||
private final String username;
|
||||
@@ -58,7 +62,9 @@ import javax.net.ssl.SSLSocketFactory;
|
||||
private final String readTimeOutMillis;
|
||||
|
||||
@Inject
|
||||
Helper(@GerritServerConfig final Config config) {
|
||||
Helper(@GerritServerConfig final Config config,
|
||||
@Named(LdapModule.GROUPS_BYINCLUDE_CACHE)
|
||||
Cache<String, ImmutableSet<String>> groupsByInclude) {
|
||||
this.config = config;
|
||||
this.server = LdapRealm.required(config, "server");
|
||||
this.username = LdapRealm.optional(config, "username");
|
||||
@@ -73,6 +79,7 @@ import javax.net.ssl.SSLSocketFactory;
|
||||
} else {
|
||||
readTimeOutMillis = null;
|
||||
}
|
||||
this.groupsByInclude = groupsByInclude;
|
||||
}
|
||||
|
||||
private Properties createContextProperties() {
|
||||
@@ -207,24 +214,31 @@ import javax.net.ssl.SSLSocketFactory;
|
||||
private void recursivelyExpandGroups(final Set<String> groupDNs,
|
||||
final LdapSchema schema, final DirContext ctx, final String groupDN) {
|
||||
if (groupDNs.add(groupDN) && schema.accountMemberField != null) {
|
||||
// Recursively identify the groups it is a member of.
|
||||
//
|
||||
try {
|
||||
final Name compositeGroupName = new CompositeName().add(groupDN);
|
||||
final Attribute in =
|
||||
ctx.getAttributes(compositeGroupName).get(schema.accountMemberField);
|
||||
if (in != null) {
|
||||
final NamingEnumeration<?> groups = in.getAll();
|
||||
try {
|
||||
while (groups.hasMore()) {
|
||||
final String nextDN = (String) groups.next();
|
||||
recursivelyExpandGroups(groupDNs, schema, ctx, nextDN);
|
||||
ImmutableSet<String> cachedGroupDNs = groupsByInclude.getIfPresent(groupDN);
|
||||
if (cachedGroupDNs == null) {
|
||||
// Recursively identify the groups it is a member of.
|
||||
ImmutableSet.Builder<String> dns = ImmutableSet.builder();
|
||||
try {
|
||||
final Name compositeGroupName = new CompositeName().add(groupDN);
|
||||
final Attribute in =
|
||||
ctx.getAttributes(compositeGroupName).get(schema.accountMemberField);
|
||||
if (in != null) {
|
||||
final NamingEnumeration<?> groups = in.getAll();
|
||||
try {
|
||||
while (groups.hasMore()) {
|
||||
dns.add((String) groups.next());
|
||||
}
|
||||
} catch (PartialResultException e) {
|
||||
}
|
||||
} catch (PartialResultException e) {
|
||||
}
|
||||
} catch (NamingException e) {
|
||||
LdapRealm.log.warn("Could not find group " + groupDN, e);
|
||||
}
|
||||
} catch (NamingException e) {
|
||||
LdapRealm.log.warn("Could not find group " + groupDN, e);
|
||||
cachedGroupDNs = dns.build();
|
||||
groupsByInclude.put(groupDN, cachedGroupDNs);
|
||||
}
|
||||
for (String dn : cachedGroupDNs) {
|
||||
recursivelyExpandGroups(groupDNs, schema, ctx, dn);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@ package com.google.gerrit.server.auth.ldap;
|
||||
import static java.util.concurrent.TimeUnit.HOURS;
|
||||
|
||||
import com.google.common.base.Optional;
|
||||
import com.google.common.collect.ImmutableSet;
|
||||
import com.google.gerrit.extensions.registration.DynamicSet;
|
||||
import com.google.gerrit.reviewdb.client.Account;
|
||||
import com.google.gerrit.reviewdb.client.AccountGroup;
|
||||
@@ -32,6 +33,7 @@ public class LdapModule extends CacheModule {
|
||||
static final String USERNAME_CACHE = "ldap_usernames";
|
||||
static final String GROUP_CACHE = "ldap_groups";
|
||||
static final String GROUP_EXIST_CACHE = "ldap_group_existence";
|
||||
static final String GROUPS_BYINCLUDE_CACHE = "ldap_groups_byinclude";
|
||||
|
||||
|
||||
@Override
|
||||
@@ -53,6 +55,11 @@ public class LdapModule extends CacheModule {
|
||||
.expireAfterWrite(1, HOURS)
|
||||
.loader(LdapRealm.ExistenceLoader.class);
|
||||
|
||||
cache(GROUPS_BYINCLUDE_CACHE,
|
||||
String.class,
|
||||
new TypeLiteral<ImmutableSet<String>>() {})
|
||||
.expireAfterWrite(1, HOURS);
|
||||
|
||||
bind(Realm.class).to(LdapRealm.class).in(Scopes.SINGLETON);
|
||||
bind(Helper.class);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user