[
https://issues.apache.org/jira/browse/KNOX-3386?focusedWorklogId=1033124&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1033124
]
ASF GitHub Bot logged work on KNOX-3386:
----------------------------------------
Author: ASF GitHub Bot
Created on: 30/Jul/26 23:31
Start Date: 30/Jul/26 23:31
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1331:
URL: https://github.com/apache/knox/pull/1331#discussion_r3686909146
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -511,12 +521,10 @@ public List<Entry> searchUsers(String filter,
SchemaManager schemaManager) throw
try {
connection = getConnection();
String ldapFilter = "(" + remoteUserIdentifierAttribute + "=" +
filter.trim() + ")";
- try (EntryCursor cursor = connection.search(remoteUserSearchBase,
ldapFilter, SearchScope.SUBTREE, "*")) {
- while (cursor.next()) {
- Entry sourceEntry = cursor.get();
- addGroupMemberships(sourceEntry, connection, entryCache,
resolvedParentsCache);
-
results.add(remoteSchemaConverter.convertRemoteEntryToProxyEntry(sourceEntry,
schemaManager));
- }
+ List<Entry> searchResults = performPagedSearch(connection,
remoteUserSearchBase, ldapFilter, SearchScope.SUBTREE, "*");
Review Comment:
Previously `searchUsers` had no `try/catch` around its cursor, so an
`LdapException` propagated to the caller. Now it routes through
`performPagedSearch`, which catches and logs, returning partial/empty results.
So a backend failure during user search now looks like "no users found" rather
than an error. `search()` already behaved this way, so this may be intentional.
But it's a behavior change for `searchUsers`. Please confirm silent degradation
is desired there (auth/lookup failures being indistinguishable from empty
results can be a real operational issue).
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java:
##########
@@ -809,4 +874,28 @@ public Set<String> get(Object key) {
// For the second user, many groups should have been found in the
cache.
assertEquals("Expected " + expectedCacheHits + " cache hits for shared
groups, but got " + cacheHits.get(), expectedCacheHits, cacheHits.get());
}
+
+ private static class CapturingSearchRequestHandler extends
LdapRequestHandler<SearchRequest> {
+ private final LdapRequestHandler<SearchRequest> delegate;
+ private final List<SearchRequest> requests =
Collections.synchronizedList(new ArrayList<>());
+
+ CapturingSearchRequestHandler(LdapRequestHandler<SearchRequest>
delegate) {
+ this.delegate = delegate;
+ }
+
+ public void reset() {
+ requests.clear();
+ }
+
+ public List<SearchRequest> getRequests() {
+ return List.copyOf(requests);
+ }
+
+ @Override
+ public void handle(LdapSession session, SearchRequest message) throws
Exception {
+ requests.add(message);
+ System.out.println(message.toString());
Review Comment:
nit: this should be removed.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -839,15 +841,67 @@ private List<Entry> getUserGroupsInternal(LdapConnection
connection, Dn... dns)
String filter = buildMultipleGroupMemberFilter(dns);
- try (EntryCursor cursor = connection.search(remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn")) {
- while (cursor.next()) {
- groups.add(cursor.get());
- }
- }
+ groups.addAll(performPagedSearch(connection, remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn"));
return groups;
}
+ protected List<Entry> performPagedSearch(LdapConnection connection, String
baseDn, String filter, SearchScope scope, String... attributes ) throws
LdapException, CursorException, IOException {
+ List<Entry> results = new ArrayList<>();
+
+ // 1. Setup basic search parameters
+ SearchRequest searchRequest = new SearchRequestImpl();
+ searchRequest.setBase(new Dn(baseDn));
+ searchRequest.setFilter(filter);
+ searchRequest.setScope(scope);
+ searchRequest.addAttributes(attributes);
+
+ // 2. Initialize the PagedResults control
+ PagedResults pagedControl = new PagedResultsImpl();
+ pagedControl.setSize(pageSize);
+ searchRequest.addControl(pagedControl);
+
+ byte[] cookie = null;
+
+ // 3. Loop until no more pages remain
+ int pageNumber = 1;
+ do {
+ // Update cookie for the subsequent pages
+ if (cookie != null) {
+ pagedControl.setCookie(cookie);
+ }
+
+ try (SearchCursor cursor = connection.search(searchRequest)) {
+ LOG.ldapPagedSearch(baseDn, filter, pageSize, pageNumber);
+ while (cursor.next()) {
+ Response response = cursor.get();
+
+ // Process matching entries
+ if (response instanceof SearchResultEntry) {
+ Entry entry = ((SearchResultEntry)
response).getEntry();
+ results.add(entry);
+ }
+ }
+ if (cursor.isDone()) {
+ SearchResultDone done = cursor.getSearchResultDone();
+ PagedResults responseControl = (PagedResults)
done.getControl(PagedResults.OID);
+
+ if (responseControl != null) {
+ cookie = responseControl.getCookie();
+ } else {
+ cookie = null;
+ }
+ }
+ pageNumber++;
+ } catch (LdapException e) {
+ LOG.ldapSearchFailed(baseDn, filter, e);
Review Comment:
Here we log the failure but do not clear cookie or break the loop. As a
result, it might end up in an infinite loop: `page 1` succeeds and returns a
non-empty cookie. So the loop continues; `page 2`'s `connection.search(...)`
(or cursor iteration) throws `LdapException` (server restart, timeout,
connection drop mid-paging). The exception is caught, `cookie` still holds page
1's value, `pageNumber++` is skipped, and the loop condition is still `true`.
It retries the identical failing page forever.
The old code (`search(...)`) caught `LdapException` around a single cursor
and simply stopped with partial results. To preserve that, the catch should
terminate the loop, e.g.:
```
} catch (LdapException e) {
LOG.ldapSearchFailed(baseDn, filter, e);
break; // or: cookie = null;
}
```
What do you think?
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/LdapMessages.java:
##########
@@ -133,9 +141,9 @@ public interface LdapMessages {
text = "Backend user not found: {0}")
void ldapUserNull(String username);
- @Message(level = MessageLevel.ERROR,
+ @Message(level = MessageLevel.DEBUG,
text = "Failed to copy attribute: {0}")
- void ldapAttributeCopyError(@StackTrace(level = MessageLevel.DEBUG)
Exception e);
+ void ldapAttributeCopyError(@StackTrace(level = MessageLevel.TRACE)
Exception e);
Review Comment:
Why are these changes needed here? Shouldn't this remain on `ERROR` level
instead of `DEBUG`? We might hide real conversion problems.
Issue Time Tracking
-------------------
Worklog Id: (was: 1033124)
Time Spent: 20m (was: 10m)
> Knox LDAP Proxy supports paging to backends
> -------------------------------------------
>
> Key: KNOX-3386
> URL: https://issues.apache.org/jira/browse/KNOX-3386
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
> Reporter: David Han
> Assignee: David Han
> Priority: Major
> Fix For: 3.0.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The LDAP proxy should page results from the remote backends when retrieving
> large result sets.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
