github-actions[bot] commented on code in PR #61673:
URL: https://github.com/apache/doris/pull/61673#discussion_r2991837066
##########
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java:
##########
@@ -87,7 +87,36 @@ public class LdapConfig extends ConfigBase {
public static long ldap_cache_timeout_day = 30;
/**
- * LDAP pool configuration:
+ * LDAP read timeout in milliseconds.
+ * Controls the maximum time to wait for an LDAP response after a request
is sent.
+ * Uses JNDI property "com.sun.jndi.ldap.read.timeout".
+ * Set to 0 for no timeout (not recommended). Default 5000ms.
+ */
+ @ConfigBase.ConfField(mutable = true)
+ public static int ldap_read_timeout_ms = 5000;
+
+ /**
+ * LDAP connect timeout in milliseconds.
+ * Controls the maximum time to wait for establishing a TCP connection to
the LDAP server.
+ * Uses JNDI property "com.sun.jndi.ldap.connect.timeout".
+ * Set to 0 for no timeout (not recommended). Default 5000ms.
+ */
+ @ConfigBase.ConfField(mutable = true)
+ public static int ldap_connect_timeout_ms = 5000;
+
+ /**
+ * Whether to use connection pooling for LDAP search operations.
+ * When true (default), uses Spring PoolingContextSource with ldap_pool_*
settings.
+ * When false, each LDAP search creates a fresh connection, avoiding dead
connection
+ * detection cost (testOnBorrow can burn read_timeout discovering stale
connections
+ * killed by firewalls/NAT idle timeout). Recommended to set false if
experiencing
+ * intermittent ~5s LDAP search latency spikes.
+ */
+ @ConfigBase.ConfField(mutable = true)
+ public static boolean ldap_search_use_pool = true;
+
Review Comment:
**[Configuration]** `ldap_search_use_pool` is marked `mutable = true`, but
changing it at runtime via `ADMIN SET FRONTEND CONFIG` will have **no effect**
until the LDAP admin password is changed (which triggers `ClientInfo`
re-creation in `LdapClient.init()`). The same staleness issue applies to
`ldap_read_timeout_ms` and `ldap_connect_timeout_ms`.
Options:
1. Remove `mutable = true` if runtime changes are not intended to take
effect immediately, to avoid user confusion.
2. Add detection of these config changes in `ClientInfo.checkUpdate()` so
`init()` re-creates the `ClientInfo` when any LDAP config changes, not just the
password.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapManager.java:
##########
@@ -80,14 +80,24 @@ public LdapUserInfo getUserInfo(String fullName) {
if (!checkParam(fullName)) {
return null;
}
+ long start = System.currentTimeMillis();
LdapUserInfo ldapUserInfo = getUserInfoFromCache(fullName);
if (ldapUserInfo != null && !ldapUserInfo.checkTimeout()) {
+ long elapsed = System.currentTimeMillis() - start;
+ LOG.info("[LDAP-AUTH] LdapManager.getUserInfo: user={},
cacheHit=true, elapsed={}ms",
+ fullName, elapsed);
Review Comment:
**[Performance/Observability]** This `LOG.info` is called on every login for
LDAP users, including the cache-hit fast path. For production environments with
many concurrent LDAP logins, this will produce a high volume of INFO logs.
Consider making the cache-hit path `LOG.debug` and only emitting `LOG.info`
or `LOG.warn` when elapsed time exceeds a threshold (e.g., `> 1000ms`). The
same applies to the other `[LDAP-AUTH]` LOG.info calls throughout the call
chain -- a single login currently produces ~8 INFO log lines, which is
excessive.
This pattern applies broadly to the `LOG.info` calls added in `LdapClient`,
`LdapManager`, `LdapAuthenticator`, and `AuthenticatorManager`.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/AuthenticatorManager.java:
##########
@@ -152,8 +152,12 @@ public boolean authenticate(ConnectContext context,
MysqlSerializer serializer,
MysqlAuthPacket authPacket,
MysqlHandshakePacket handshakePacket) throws
IOException {
+
String remoteIp = context.getMysqlChannel().getRemoteIp();
Authenticator primaryAuthenticator = chooseAuthenticator(userName,
remoteIp);
+ LOG.info("[LDAP-AUTH] AuthenticatorManager: user={}, authenticator={}",
+ userName, primaryAuthenticator.getClass().getSimpleName());
Review Comment:
**[Observability/Correctness]** This `[LDAP-AUTH]` log fires for **every**
authentication attempt regardless of authentication type (default, LDAP,
plugin, etc.). The tag is misleading for non-LDAP users. Consider:
1. Only logging with the `[LDAP-AUTH]` prefix when the chosen authenticator
is actually `LdapAuthenticator`, or
2. Removing the `[LDAP-AUTH]` prefix here since this is a generic
authentication manager.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapClient.java:
##########
@@ -60,7 +62,15 @@ private static class ClientInfo {
public ClientInfo(String ldapPassword) {
this.ldapPassword = ldapPassword;
setLdapTemplateNoPool(ldapPassword);
- setLdapTemplatePool(ldapPassword);
+ if (LdapConfig.ldap_search_use_pool) {
+ setLdapTemplatePool(ldapPassword);
Review Comment:
**[Configuration staleness]** `ldap_search_use_pool` is read here during
`ClientInfo` construction, but `init()` only re-creates `ClientInfo` when the
LDAP password changes (via `checkUpdate()`). A runtime config change to
`ldap_search_use_pool` will be silently ignored. Consider either:
1. Including all mutable config values in the `checkUpdate()` check, or
2. Not marking these configs as `mutable`.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapAuthenticatorTest.java:
##########
@@ -139,8 +161,64 @@ public void testCanDeal() {
Assert.assertFalse(ldapAuthenticator.canDeal("ss"));
}
+ @Test
+ public void testCanDealLogsInfoWithoutThreshold() {
+ setLdapUserExist(true);
+ try (LdapTestLogAppender appender =
LdapTestLogAppender.attach(LdapAuthenticator.class)) {
+ Assert.assertTrue(ldapAuthenticator.canDeal("ss"));
+ Assert.assertTrue(appender.contains(Level.INFO,
+ "[LDAP-AUTH] LdapAuthenticator.canDeal: user=ss,
result=true, elapsed="));
+ Assert.assertFalse(appender.contains(Level.WARN,
+ "[LDAP-AUTH] LdapAuthenticator.canDeal slow: user=ss"));
+ }
+ }
+
@Test
public void testGetPasswordResolver() {
Assert.assertTrue(ldapAuthenticator.getPasswordResolver() instanceof
ClearPasswordResolver);
}
+
+ static class LdapTestLogAppender extends AbstractAppender implements
AutoCloseable {
+ private final Logger logger;
Review Comment:
**[Code Duplication]** `LdapTestLogAppender` is a nearly identical copy of
the standalone `TestLogAppender` class added in
`fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/TestLogAppender.java`.
The tests in `LdapClientTest` and `LdapManagerTest` even reference this inner
class via `LdapAuthenticatorTest.LdapTestLogAppender`, which is awkward.
Recommend: delete this inner class and use the shared `TestLogAppender`
instead. If `TestLogAppender` was added for this purpose, `LdapTestLogAppender`
should not also exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
