Implement more robust connection handling for asynchronous LDAP calls

Keystone's paging implementation contains a memory leak. The issue is
noticeable if you integrate keystone with an LDAP server that supports
paging and set `keystone.conf [ldap] page_size` to a low integer
(e.g., 5).

Keystone's LDAP backend uses `python-ldap` to interact with LDAP
servers. For paged requests, it uses `search_ext()`, which is an
asynchronous API [0]. The server responds with a message ID, which the
client uses to retrieve all data for the request. In keystone's case,
the `search_ext()` method is invoked with a page control that tells
the server to deliver responses in increments according to the page
size configured with `keystone.conf [ldap] page_size`. So long as the
client has the original connection used to fetch the message ID, it
can request the rest of the information associated to the request.

Keystone's paging implementation loops continuously for paged
requests. It takes the message ID it gets from `search_ext()` and
calls `result3()`, asking the server for the data associated with that
specific message. Keystone continues to do this until the server sends
an indicator that it has no more data relevant to the query (via a
cookie). The `search_ext()` and `result3()` methods must use the same
LDAP connection.

Given the above information, keystone uses context managers to provide
connections. This is relevant when deploying connection pools, where
certain connections are re-used from a pool. Keystone relies on Python
context managers to handle connections, which is pretty typical
use-case for context managers. Connection managers allow us to do the
following (assuming pseudocode):

  with self.get_connection as conn:
      response = conn.search_s()
      return format(response)

The above snippet assumes the `get_connection` method provides a
connection object and a callable that implements `search_s`. Upon
exiting the `with` statement, the connection is disconnected, or put
back into the pool, or whatever the implementation of the context
manager decides to do. Most connections in the LDAP backend are
handled in this fashion.

Unfortunately, the LDAP driver is somewhat oblivious to paging, it's
control implementation, or the fact that it uses an asynchronous API.
Instead, the driver leaves it up to the handler objects it uses for
connections to determine if the request should be controlled via
paging. This is an anti-pattern since the backend establishes the
connection for the request but doesn't ensure that connection is
safely handled for asynchronous APIs.

This forces the `search_ext()` and `result3()` implementations in the
PooledLDAPHandler to know how to handle connections and context
managers, since it needs to ensure the same connection is used for
paged requests. The current code tried to clean up the context
manager responsible for connections after the results are collected
from the server using the message ID. I believe it does this because
it needs to get a new connection for each message in the paged
results, even though it already operates from within a connection
established via a context manager and the PooledLDAPHandler almost
always returns the same connection object from the pool. The code
tries to use a weak reference to create a callback that tears down the
context manager when nothing else references it. At a high-level, the
idea is similar to the following pseudocode:

  with self.get_connection as conn:
      while True:
	ldap_data = []
	context_manager = self.get_connection()
	connection = context_manager.__enter__()
	message_id = connection.search_ext()
	results = connection.result3(message_id)
	ldap_data.append(results)
	context_manager.__exit__()

I wasn't able to see the callback get invoked or work as described in
comments, resulting in memory bloat, especially with low page sizes
which results in more requests. A weak reference invokes the callback
when the weak reference is called, but there are no other references
to the original object [1]. In our case, I don't think we invoke that
path because we don't actually do anything with the weak reference. We
assume it's going to run the callback when the object is garbage
collected.

This commit attempts to address this issue by using the concept of a
finalizer [2], which was designed for similar cases. It also attempts
to hide the cleanup implementation in the AsynchronousMessage object,
so that callers don't have to worry about making sure they invoke the
finalizer.

An alternative approach would be to push more of the paging logic and
implementation up into the LDAP driver. This would make it easier to
put the entire asynchronous API flow for paging into a `with`
statement and relying on the normal behavior of context managers to
clean up accordingly. This approach would remove the manual cleanup
invocation, regardless of using weak references or finalizer objects.
However, this approach would likely require a non-trivial amount of
design work to refactor the entire LDAP backend. The LDAP backend has
other issues that would complicate the re-design process:

  - Handlers and connection are generalized to mean the same thing
  - Method names don't follow a convention
  - Domain-specific language from python-ldap bleeds into keystone's
    implementation (e.g., get_all, _ldap_get_all, add_member) at
    different points in the backend (e.g., UserApi (BaseLdap), GroupApi
    (BaseLdap), KeystoneLDAPHandler, PooledLDAPHandler,
    PythonLDAPHandler)
  - Backend contains dead code from when keystone supported writeable
    LDAP backends
  - Responsibility for connections and connection handling is spread
    across objects (BaseLdap, LDAPHandler)
  - Handlers will invoke methods differently based on configuration at
    runtime, which is a sign that the relationship between the driver,
    handlers, and connection objects isn't truely polymorphic

While keeping the logic for properly handling context managers and
connections in the Handlers might not be ideal, it is a relatively
minimal fix in comparison to a re-design or backend refactor. These
issues can be considered during a refactor of the LDAP backend if or
when the community decides to re-design the LDAP backend.

[0] https://www.python-ldap.org/en/python-ldap-3.3.0/reference/ldap.html#ldap.LDAPObject.search_ext
[1] https://docs.python.org/3/library/weakref.html#weakref.ref
[2] https://docs.python.org/3/library/weakref.html#finalizer-objects

Closes-Bug: 1896125
Change-Id: Ia45a45ff852d0d4e3a713dae07a46d4ff8d370f3

2 files changed, 75 insertions, 38 deletions

diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py
index cef0d9bdf..251a9f183 100644
--- a/keystone/identity/backends/ldap/common.py
+++ b/keystone/identity/backends/ldap/common.py
@@ -14,7 +14,6 @@
 
 import abc
 import codecs
-import functools
 import os.path
 import re
 import sys
@@ -635,10 +634,36 @@ def _common_ldap_initialization(url, use_tls=False, tls_cacertfile=None,
                       tls_req_cert)
 
 
-class MsgId(list):
-    """Wrapper class to hold connection and msgid."""
+class AsynchronousMessage(object):
+    """A container for handling asynchronous LDAP responses.
 
-    pass
+    Some LDAP APIs, like `search_ext`, are asynchronous and return a message ID
+    when the server successfully initiates the operation. Clients can use this
+    message ID and the original connection to make the request to fetch the
+    results using `result3`.
+
+    This object holds the message ID, the original connection, and a callable
+    weak reference Finalizer that cleans up context managers specific to the
+    connection associated to the message ID.
+
+    :param message_id: The message identifier (str).
+    :param connection: The connection associated with the message identifier
+                       (ldappool.StateConnector).
+
+    The `clean` attribute is a callable that cleans up the context manager used
+    to create or return the connection object (weakref.finalize).
+
+    """
+
+    def __init__(self, message_id, connection, context_manager):
+        self.id = message_id
+        self.connection = connection
+        self.clean = weakref.finalize(
+            self, self._cleanup_connection_context_manager, context_manager
+        )
+
+    def _cleanup_connection_context_manager(self, context_manager):
+        context_manager.__exit__(None, None, None)
 
 
 def use_conn_pool(func):
@@ -792,15 +817,17 @@ class PooledLDAPHandler(LDAPHandler):
                    filterstr='(objectClass=*)', attrlist=None, attrsonly=0,
                    serverctrls=None, clientctrls=None,
                    timeout=-1, sizelimit=0):
-        """Return a ``MsgId`` instance, it asynchronous API.
+        """Return an AsynchronousMessage instance, it asynchronous API.
 
-        The ``MsgId`` instance can be safely used in a call to ``result3()``.
+        The AsynchronousMessage instance can be safely used in a call to
+        `result3()`.
 
-        To work with ``result3()`` API in predictable manner, the same LDAP
-        connection is needed which originally provided the ``msgid``. So, this
-        method wraps the existing connection and ``msgid`` in a new ``MsgId``
-        instance. The connection associated with ``search_ext`` is released
-        once last hard reference to the ``MsgId`` instance is freed.
+        To work with `result3()` API in predictable manner, the same LDAP
+        connection is needed which originally provided the `msgid`. So, this
+        method wraps the existing connection and `msgid` in a new
+        `AsynchronousMessage` instance. The connection associated with
+        `search_ext()` is released after `result3()` fetches the data
+        associated with `msgid`.
 
         """
         conn_ctxt = self._get_pool_connection()
@@ -813,30 +840,33 @@ class PooledLDAPHandler(LDAPHandler):
         except Exception:
             conn_ctxt.__exit__(*sys.exc_info())
             raise
-        res = MsgId((conn, msgid))
-        weakref.ref(res, functools.partial(conn_ctxt.__exit__,
-                                           None, None, None))
-        return res
+        return AsynchronousMessage(msgid, conn, conn_ctxt)
 
-    def result3(self, msgid, all=1, timeout=None,
+    def result3(self, message, all=1, timeout=None,
                 resp_ctrl_classes=None):
-        """Wait for and return the result.
+        """Wait for and return the result to an asynchronous message.
 
         This method returns the result of an operation previously initiated by
-        one of the LDAP asynchronous operation routines (eg search_ext()). It
-        returned an invocation identifier (a message id) upon successful
-        initiation of their operation.
+        one of the LDAP asynchronous operation routines (e.g., `search_ext()`).
+        The `search_ext()` method in python-ldap returns an invocation
+        identifier, or a message ID, upon successful initiation of the
+        operation by the LDAP server.
 
-        Input msgid is expected to be instance of class MsgId which has LDAP
-        session/connection used to execute search_ext and message idenfier.
+        The `message` is expected to be instance of class
+        `AsynchronousMessage`, which contains the message ID and the connection
+        used to make the original request.
 
-        The connection associated with search_ext is released once last hard
-        reference to MsgId object is freed. This will happen when function
-        which requested msgId and used it in result3 exits.
+        The connection and context manager associated with `search_ext()` are
+        cleaned up when message.clean() is called.
 
         """
-        conn, msg_id = msgid
-        return conn.result3(msg_id, all, timeout)
+        results = message.connection.result3(message.id, all, timeout)
+
+        # Now that we have the results from the LDAP server for the message, we
+        # don't need the the context manager used to create the connection.
+        message.clean()
+
+        return results
 
     @use_conn_pool
     def modify_s(self, conn, dn, modlist):
@@ -997,15 +1027,15 @@ class KeystoneLDAPHandler(LDAPHandler):
                 cookie='')
             page_ctrl_oid = ldap.controls.SimplePagedResultsControl.controlType
 
-        msgid = self.conn.search_ext(base,
-                                     scope,
-                                     filterstr,
-                                     attrlist,
-                                     serverctrls=[lc])
+        message = self.conn.search_ext(base,
+                                       scope,
+                                       filterstr,
+                                       attrlist,
+                                       serverctrls=[lc])
         # Endless loop request pages on ldap server until it has no data
         while True:
             # Request to the ldap server a page with 'page_size' entries
-            rtype, rdata, rmsgid, serverctrls = self.conn.result3(msgid)
+            rtype, rdata, rmsgid, serverctrls = self.conn.result3(message)
             # Receive the data
             res.extend(rdata)
             pctrls = [c for c in serverctrls
@@ -1021,11 +1051,11 @@ class KeystoneLDAPHandler(LDAPHandler):
                 if cookie:
                     # There is more data still on the server
                     # so we request another page
-                    msgid = self.conn.search_ext(base,
-                                                 scope,
-                                                 filterstr,
-                                                 attrlist,
-                                                 serverctrls=[lc])
+                    message = self.conn.search_ext(base,
+                                                   scope,
+                                                   filterstr,
+                                                   attrlist,
+                                                   serverctrls=[lc])
                 else:
                     # Exit condition no more data on server
                     break
diff --git a/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml b/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml
new file mode 100644
index 000000000..7a758b9ab
--- /dev/null
+++ b/releasenotes/notes/bug-1896125-b17a4d12730fe493.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1896125 <https://bugs.launchpad.net/keystone/+bug/1896125>`_]
+    Introduced more robust connection handling for asynchronous LDAP requests
+    to address memory leaks fetching data from LDAP backends with low page
+    sizes.