This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit af7bc420be4697c2cc32875f6ccd2faffd4432a8 Author: Benoit Tellier <[email protected]> AuthorDate: Thu Jun 10 11:39:30 2021 +0700 JAMES-3594 Drop manual retries and rely on UnboundID connection retries --- .../test/java/org/apache/james/DockerLdapRule.java | 4 - .../user/ldap/LdapRepositoryConfiguration.java | 95 +--------------------- .../apache/james/user/ldap/ReadOnlyLDAPUser.java | 19 +---- .../james/user/ldap/ReadOnlyLDAPUsersDAO.java | 69 ++++------------ .../user/ldap/ReadOnlyUsersLDAPRepository.java | 50 ------------ .../user/ldap/ReadOnlyUsersLDAPRepositoryTest.java | 4 - upgrade-instructions.md | 2 + 7 files changed, 23 insertions(+), 220 deletions(-) diff --git a/server/container/guice/cassandra-ldap-guice/src/test/java/org/apache/james/DockerLdapRule.java b/server/container/guice/cassandra-ldap-guice/src/test/java/org/apache/james/DockerLdapRule.java index 96f937f..4ce36af 100644 --- a/server/container/guice/cassandra-ldap-guice/src/test/java/org/apache/james/DockerLdapRule.java +++ b/server/container/guice/cassandra-ldap-guice/src/test/java/org/apache/james/DockerLdapRule.java @@ -49,10 +49,6 @@ public class DockerLdapRule implements GuiceModuleTestRule { .userBase("ou=People,dc=james,dc=org") .userIdAttribute("uid") .userObjectClass("inetOrgPerson") - .maxRetries(4) - .retryStartInterval(0) - .retryMaxInterval(8) - .scale(1000) .build(); } catch (ConfigurationException e) { throw new RuntimeException(e); diff --git a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapRepositoryConfiguration.java b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapRepositoryConfiguration.java index 16e40f5..fd1fce8 100644 --- a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapRepositoryConfiguration.java +++ b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapRepositoryConfiguration.java @@ -19,7 +19,6 @@ package org.apache.james.user.ldap; -import java.time.Duration; import java.util.Objects; import java.util.Optional; @@ -30,10 +29,6 @@ import org.apache.james.core.Username; import com.google.common.base.Preconditions; -import reactor.core.scheduler.Schedulers; -import reactor.util.retry.Retry; -import reactor.util.retry.RetryBackoffSpec; - public class LdapRepositoryConfiguration { public static final String SUPPORTS_VIRTUAL_HOSTING = "supportsVirtualHosting"; @@ -52,10 +47,6 @@ public class LdapRepositoryConfiguration { private Optional<String> userBase; private Optional<String> userIdAttribute; private Optional<String> userObjectClass; - private Optional<Integer> maxRetries; - private Optional<Long> retryStartInterval; - private Optional<Long> retryMaxInterval; - private Optional<Integer> scale; private Optional<Integer> poolSize; public Builder() { @@ -65,10 +56,6 @@ public class LdapRepositoryConfiguration { userBase = Optional.empty(); userIdAttribute = Optional.empty(); userObjectClass = Optional.empty(); - maxRetries = Optional.empty(); - retryStartInterval = Optional.empty(); - retryMaxInterval = Optional.empty(); - scale = Optional.empty(); poolSize = Optional.empty(); } @@ -102,26 +89,6 @@ public class LdapRepositoryConfiguration { return this; } - public Builder maxRetries(int maxRetries) { - this.maxRetries = Optional.of(maxRetries); - return this; - } - - public Builder retryStartInterval(long retryStartInterval) { - this.retryStartInterval = Optional.of(retryStartInterval); - return this; - } - - public Builder retryMaxInterval(long retryMaxInterval) { - this.retryMaxInterval = Optional.of(retryMaxInterval); - return this; - } - - public Builder scale(int scale) { - this.scale = Optional.of(scale); - return this; - } - public Builder poolSize(int poolSize) { this.poolSize = Optional.of(poolSize); return this; @@ -134,10 +101,6 @@ public class LdapRepositoryConfiguration { Preconditions.checkState(userBase.isPresent(), "'userBase' is mandatory"); Preconditions.checkState(userIdAttribute.isPresent(), "'userIdAttribute' is mandatory"); Preconditions.checkState(userObjectClass.isPresent(), "'userObjectClass' is mandatory"); - Preconditions.checkState(maxRetries.isPresent(), "'maxRetries' is mandatory"); - Preconditions.checkState(retryStartInterval.isPresent(), "'retryStartInterval' is mandatory"); - Preconditions.checkState(retryMaxInterval.isPresent(), "'retryMaxInterval' is mandatory"); - Preconditions.checkState(scale.isPresent(), "'scale' is mandatory"); return new LdapRepositoryConfiguration( ldapHost.get(), @@ -148,11 +111,7 @@ public class LdapRepositoryConfiguration { userObjectClass.get(), NO_CONNECTION_TIMEOUT, NO_READ_TIME_OUT, - maxRetries.get(), !ENABLE_VIRTUAL_HOSTING, - retryStartInterval.get(), - retryMaxInterval.get(), - scale.get(), poolSize.orElse(DEFAULT_POOL_SIZE), NO_RESTRICTION, NO_FILTER, @@ -174,15 +133,7 @@ public class LdapRepositoryConfiguration { // Default is to use connection pooling int connectionTimeout = configuration.getInt("[@connectionTimeout]", NO_CONNECTION_TIMEOUT); int readTimeout = configuration.getInt("[@readTimeout]", NO_READ_TIME_OUT); - // Default maximum retries is 1, which allows an alternate connection to - // be found in a multi-homed environment - int maxRetries = configuration.getInt("[@maxRetries]", 1); boolean supportsVirtualHosting = configuration.getBoolean(SUPPORTS_VIRTUAL_HOSTING, !ENABLE_VIRTUAL_HOSTING); - // Default retry start interval is 0 second - long retryStartInterval = configuration.getLong("[@retryStartInterval]", 0); - // Default maximum retry interval is 60 seconds - long retryMaxInterval = configuration.getLong("[@retryMaxInterval]", 60); - int scale = configuration.getInt("[@retryIntervalScale]", 1000); // seconds HierarchicalConfiguration<ImmutableNode> restrictionConfig = null; // Check if we have a restriction we can use @@ -208,11 +159,7 @@ public class LdapRepositoryConfiguration { userObjectClass, connectionTimeout, readTimeout, - maxRetries, supportsVirtualHosting, - retryStartInterval, - retryMaxInterval, - scale, poolSize, restriction, filter, @@ -272,13 +219,7 @@ public class LdapRepositoryConfiguration { // The LDAP read timeout in milliseconds. private final int readTimeout; - // Maximum number of times to retry a connection attempts. Default is no - // retries. - private final int maxRetries; private final boolean supportsVirtualHosting; - private final long retryStartInterval; - private final long retryMaxInterval; - private final int scale; private final int poolSize; /** @@ -302,8 +243,7 @@ public class LdapRepositoryConfiguration { private LdapRepositoryConfiguration(String ldapHost, String principal, String credentials, String userBase, String userIdAttribute, String userObjectClass, int connectionTimeout, int readTimeout, - int maxRetries, boolean supportsVirtualHosting, long retryStartInterval, long retryMaxInterval, - int scale, int poolSize, ReadOnlyLDAPGroupRestriction restriction, String filter, + boolean supportsVirtualHosting, int poolSize, ReadOnlyLDAPGroupRestriction restriction, String filter, Optional<String> administratorId) throws ConfigurationException { this.ldapHost = ldapHost; this.principal = principal; @@ -313,11 +253,7 @@ public class LdapRepositoryConfiguration { this.userObjectClass = userObjectClass; this.connectionTimeout = connectionTimeout; this.readTimeout = readTimeout; - this.maxRetries = maxRetries; this.supportsVirtualHosting = supportsVirtualHosting; - this.retryStartInterval = retryStartInterval; - this.retryMaxInterval = retryMaxInterval; - this.scale = scale; this.poolSize = poolSize; this.restriction = restriction; this.filter = filter; @@ -374,26 +310,10 @@ public class LdapRepositoryConfiguration { return readTimeout; } - public int getMaxRetries() { - return maxRetries; - } - public boolean supportsVirtualHosting() { return supportsVirtualHosting; } - public long getRetryStartInterval() { - return retryStartInterval; - } - - public long getRetryMaxInterval() { - return retryMaxInterval; - } - - public int getScale() { - return scale; - } - public ReadOnlyLDAPGroupRestriction getRestriction() { return restriction; } @@ -406,12 +326,6 @@ public class LdapRepositoryConfiguration { return administratorId; } - public RetryBackoffSpec retrySpec() { - return Retry.backoff(getMaxRetries(), Duration.ofMillis(getRetryStartInterval() * getScale())) - .maxBackoff(Duration.ofMillis(getRetryMaxInterval() * getScale())) - .scheduler(Schedulers.elastic()); - } - @Override public final boolean equals(Object o) { if (o instanceof LdapRepositoryConfiguration) { @@ -419,11 +333,7 @@ public class LdapRepositoryConfiguration { return Objects.equals(this.connectionTimeout, that.connectionTimeout) && Objects.equals(this.readTimeout, that.readTimeout) - && Objects.equals(this.maxRetries, that.maxRetries) && Objects.equals(this.supportsVirtualHosting, that.supportsVirtualHosting) - && Objects.equals(this.retryStartInterval, that.retryStartInterval) - && Objects.equals(this.retryMaxInterval, that.retryMaxInterval) - && Objects.equals(this.scale, that.scale) && Objects.equals(this.ldapHost, that.ldapHost) && Objects.equals(this.principal, that.principal) && Objects.equals(this.credentials, that.credentials) @@ -441,7 +351,6 @@ public class LdapRepositoryConfiguration { @Override public final int hashCode() { return Objects.hash(ldapHost, principal, credentials, userBase, userIdAttribute, userObjectClass, - connectionTimeout, readTimeout, maxRetries, supportsVirtualHosting, retryStartInterval, retryMaxInterval, scale, - restriction, filter, administratorId, poolSize); + connectionTimeout, readTimeout, supportsVirtualHosting, restriction, filter, administratorId, poolSize); } } diff --git a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java index 1bc7147..7356212 100644 --- a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java +++ b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java @@ -29,11 +29,8 @@ import org.slf4j.LoggerFactory; import com.unboundid.ldap.sdk.BindResult; import com.unboundid.ldap.sdk.DN; import com.unboundid.ldap.sdk.LDAPConnectionPool; -import com.unboundid.ldap.sdk.LDAPException; import com.unboundid.ldap.sdk.ResultCode; -import reactor.core.publisher.Mono; - /** * Encapsulates the details of a user as taken from an LDAP compliant directory. * Instances of this class are only applicable to the @@ -71,8 +68,6 @@ public class ReadOnlyLDAPUser implements User, Serializable { */ private final LDAPConnectionPool connectionPool; - private final LdapRepositoryConfiguration configuration; - /** * Constructs an instance for the given user-details, and which will * authenticate against the given host. @@ -87,13 +82,11 @@ public class ReadOnlyLDAPUser implements User, Serializable { * This is also the host against which the user will be * authenticated, when {@link #verifyPassword(String)} is * invoked. - * @param configuration */ - public ReadOnlyLDAPUser(Username userName, DN userDN, LDAPConnectionPool connectionPool, LdapRepositoryConfiguration configuration) { + public ReadOnlyLDAPUser(Username userName, DN userDN, LDAPConnectionPool connectionPool) { this.userName = userName; this.userDN = userDN; this.connectionPool = connectionPool; - this.configuration = configuration; } /** @@ -135,17 +128,11 @@ public class ReadOnlyLDAPUser implements User, Serializable { @Override public boolean verifyPassword(String password) { try { - return Mono.fromCallable(() -> doVerifyPassword(password)) - .retryWhen(configuration.retrySpec()) - .block(); + BindResult bindResult = connectionPool.bindAndRevertAuthentication(userDN.toString(), password); + return bindResult.getResultCode() == ResultCode.SUCCESS; } catch (Exception e) { LOGGER.error("Unexpected error upon authentication", e); return false; } } - - private boolean doVerifyPassword(String password) throws LDAPException { - BindResult bindResult = connectionPool.bindAndRevertAuthentication(userDN.toString(), password); - return bindResult.getResultCode() == ResultCode.SUCCESS; - } } diff --git a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java index 6901ab4..4588448 100644 --- a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java +++ b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java @@ -59,8 +59,6 @@ import com.unboundid.ldap.sdk.SearchResult; import com.unboundid.ldap.sdk.SearchResultEntry; import com.unboundid.ldap.sdk.SearchScope; -import reactor.core.publisher.Mono; - public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { private static final Logger LOGGER = LoggerFactory.getLogger(ReadOnlyLDAPUsersDAO.class); @@ -106,8 +104,7 @@ public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { + '\n' + "User baseDN: " + ldapConfiguration.getUserBase() + '\n' + "userIdAttribute: " + ldapConfiguration.getUserIdAttribute() + '\n' + "Group restriction: " + ldapConfiguration.getRestriction() + '\n' + "connectionTimeout: " - + ldapConfiguration.getConnectionTimeout() + '\n' + "readTimeout: " + ldapConfiguration.getReadTimeout() - + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n'); + + ldapConfiguration.getConnectionTimeout() + '\n' + "readTimeout: " + ldapConfiguration.getReadTimeout()); } LDAPConnectionOptions connectionOptions = new LDAPConnectionOptions(); @@ -118,6 +115,7 @@ public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { SocketFactory socketFactory = null; LDAPConnection ldapConnection = new LDAPConnection(socketFactory, connectionOptions, uri.getHost(), uri.getPort(), ldapConfiguration.getPrincipal(), ldapConfiguration.getCredentials()); ldapConnectionPool = new LDAPConnectionPool(ldapConnection, 4); + ldapConnectionPool.setRetryFailedOperationsDueToInvalidConnections(true); userExtraFilter = Optional.ofNullable(ldapConfiguration.getFilter()) .map(Throwing.function(Filter::create).sneakyThrow()); @@ -241,7 +239,7 @@ public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { if (!ldapConfiguration.getRestriction().isActivated() || userInGroupsMembershipList(result.getParsedDN(), ldapConfiguration.getRestriction().getGroupMembershipLists(ldapConnectionPool))) { - return new ReadOnlyLDAPUser(name, result.getParsedDN(), ldapConnectionPool, ldapConfiguration); + return new ReadOnlyLDAPUser(name, result.getParsedDN(), ldapConnectionPool); } return null; } @@ -251,37 +249,19 @@ public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { Optional<String> userName = Optional.ofNullable(userAttributes.getAttributeValue(ldapConfiguration.getUserIdAttribute())); return userName .map(Username::of) - .map(username -> new ReadOnlyLDAPUser(username, userDN, ldapConnectionPool, ldapConfiguration)); + .map(username -> new ReadOnlyLDAPUser(username, userDN, ldapConnectionPool)); } @Override public boolean contains(Username name) throws UsersRepositoryException { - try { - return Mono.fromCallable(() -> doContains(name)) - .retryWhen(ldapConfiguration.retrySpec()) - .block(); - } catch (Exception e) { - if (e.getCause() instanceof UsersRepositoryException) { - throw (UsersRepositoryException) e.getCause(); - } - throw new UsersRepositoryException("Unable to check user existence from ldap", e); - } - } - - private boolean doContains(Username name) throws LDAPException { - return doGetUserByName(name).isPresent(); + return getUserByName(name).isPresent(); } @Override public int countUsers() throws UsersRepositoryException { try { - return Mono.fromCallable(() -> Math.toIntExact(doCountUsers())) - .retryWhen(ldapConfiguration.retrySpec()) - .block(); - } catch (Exception e) { - if (e.getCause() instanceof UsersRepositoryException) { - throw (UsersRepositoryException) e.getCause(); - } + return Math.toIntExact(doCountUsers()); + } catch (LDAPException e) { throw new UsersRepositoryException("Unable to retrieve user count from ldap", e); } } @@ -300,45 +280,28 @@ public class ReadOnlyLDAPUsersDAO implements UsersDAO, Configurable { @Override public Optional<User> getUserByName(Username name) throws UsersRepositoryException { try { - return Mono.fromCallable(() -> doGetUserByName(name)) - .retryWhen(ldapConfiguration.retrySpec()) - .block(); + return Optional.ofNullable(searchAndBuildUser(name)); } catch (Exception e) { - if (e.getCause() instanceof UsersRepositoryException) { - throw (UsersRepositoryException) e.getCause(); - } throw new UsersRepositoryException("Unable check user existence from ldap", e); } } - private Optional<User> doGetUserByName(Username name) throws LDAPException { - return Optional.ofNullable(searchAndBuildUser(name)); - } - @Override public Iterator<Username> list() throws UsersRepositoryException { try { - return Mono.fromCallable(this::doList) - .retryWhen(ldapConfiguration.retrySpec()) - .block(); - } catch (Exception e) { - if (e.getCause() instanceof UsersRepositoryException) { - throw (UsersRepositoryException) e.getCause(); + if (!ldapConfiguration.getRestriction().isActivated()) { + return getAllUsernamesFromLDAP().iterator(); } + + return buildUserCollection(getValidUserDNs()) + .stream() + .map(ReadOnlyLDAPUser::getUserName) + .iterator(); + } catch (LDAPException e) { throw new UsersRepositoryException("Unable to list users from ldap", e); } } - private Iterator<Username> doList() throws LDAPException { - if (!ldapConfiguration.getRestriction().isActivated()) { - return getAllUsernamesFromLDAP().iterator(); - } - - return buildUserCollection(getValidUserDNs()) - .stream() - .map(ReadOnlyLDAPUser::getUserName) - .iterator(); - } private Collection<DN> getValidUserDNs() throws LDAPException { Set<DN> userDNs = getAllUsersDNFromLDAP(); diff --git a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java index 5a3b44f..32c9232 100644 --- a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java +++ b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java @@ -65,10 +65,6 @@ import org.apache.james.user.lib.UsersRepositoryImpl; * userBase="ou=People,o=myorg.com,ou=system" * userIdAttribute="uid" * userObjectClass="inetOrgPerson" - * maxRetries="20" - * retryStartInterval="0" - * retryMaxInterval="30" - * retryIntervalScale="1000" * administratorId="ldapAdmin" * </users-store> * </pre> @@ -99,52 +95,6 @@ import org.apache.james.user.lib.UsersRepositoryImpl; * <b>poolSize:</b> (optional, default = 4) The maximum number of connection * in the pool.</li> * <li> - * <li> - * <b>maxRetries:</b> (optional, default = 0) The maximum number of times to - * retry a failed operation. -1 means retry forever.</li> - * <li> - * <b>retryStartInterval:</b> (optional, default = 0) The interval in - * milliseconds to wait before the first retry. If > 0, subsequent retries are - * made at double the proceeding one up to the <b>retryMaxInterval</b> described - * below. If = 0, the next retry is 1 and subsequent retries proceed as above.</li> - * <li> - * <b>retryMaxInterval:</b> (optional, default = 60) The maximum interval in - * milliseconds to wait between retries</li> - * <li> - * <b>retryIntervalScale:</b> (optional, default = 1000) The amount by which to - * multiply each retry interval. The default value of 1000 (milliseconds) is 1 - * second, so the default <b>retryMaxInterval</b> of 60 is 60 seconds, or 1 - * minute. - * </ul> - * </p> - * <p> - * <em>Example Schedules</em> - * <ul> - * <li> - * Retry after 1000 milliseconds, doubling the interval for each retry up to - * 30000 milliseconds, subsequent retry intervals are 30000 milliseconds until - * 10 retries have been attempted, after which the <code>Exception</code> - * causing the fault is thrown: - * <ul> - * <li>maxRetries = 10 - * <li>retryStartInterval = 1000 - * <li>retryMaxInterval = 30000 - * <li>retryIntervalScale = 1 - * </ul> - * <li> - * Retry immediately, then retry after 1 * 1000 milliseconds, doubling the - * interval for each retry up to 30 * 1000 milliseconds, subsequent retry - * intervals are 30 * 1000 milliseconds until 20 retries have been attempted, - * after which the <code>Exception</code> causing the fault is thrown: - * <ul> - * <li>maxRetries = 20 - * <li>retryStartInterval = 0 - * <li>retryMaxInterval = 30 - * <li>retryIntervalScale = 1000 - * </ul> - * <li> - * Retry after 5000 milliseconds, subsequent retry intervals are 5000 - * milliseconds. * </ul> * </p> * diff --git a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java index 7254907..9638dbf 100644 --- a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java +++ b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java @@ -392,10 +392,6 @@ class ReadOnlyUsersLDAPRepositoryTest { configuration.addProperty("[@credentials]", ADMIN_PASSWORD); configuration.addProperty("[@userBase]", "ou=people,dc=james,dc=org"); configuration.addProperty("[@userObjectClass]", "inetOrgPerson"); - configuration.addProperty("[@maxRetries]", "1"); - configuration.addProperty("[@retryStartInterval]", "0"); - configuration.addProperty("[@retryMaxInterval]", "2"); - configuration.addProperty("[@retryIntervalScale]", "100"); configuration.addProperty("[@connectionTimeout]", "100"); configuration.addProperty("[@readTimeout]", "100"); return configuration; diff --git a/upgrade-instructions.md b/upgrade-instructions.md index bb7049d..1969894 100644 --- a/upgrade-instructions.md +++ b/upgrade-instructions.md @@ -33,6 +33,8 @@ As part of this migration the following change took place: - `useConnectionPool` : Removed. UnboundId implementation relies on a pool by default. - `poolSize` : Added. Allow controlling the count of connection in the pool. + - Retries had been removed. Invalid connections errors are retries. THe following parameters were removed: + `maxRetries`, `retryStartInterval`, `retryMaxInterval`, `retryIntervalScale`. Those values will be ignored. The "group restriction" feature should furthermore be considered experimental, its usage is discouraged. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
